Fixes T12931. Adds a branch selector that's always visible if the repo has commits.
Details
Details
- Reviewers
epriestley - Maniphest Tasks
- T12931: Fallback for incorrect default branch in Diffusion could be nicer
- Commits
- rP69a7d57c3fda: Add a branch selector to Diffusion
Test a plain hg, svn, git repository. Test setting a bad default branch. Test a good default branch. Test on desktop, mobile layouts.
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| src/applications/diffusion/controller/DiffusionRepositoryController.php | ||
|---|---|---|
| 66 | Although this value is correct anyway, consider passing $default instead of $drequest->getBranch() as the parameter. The phrasing seems a little weird to me too, e.g. "The master sword is not in this dungeon." implies it's in some other dungeon. Maybe something like "This repository is configured with default branch "%s" but there is no branch with that name in this repository." | |
| 159–161 | If getPath() throws an exception, $path will be left uninitialized and we'll fatal when trying to use it later. This (stuff a bunch of code in a try/catch block and ignore any exceptions) generally doesn't seem like the right approach to me. In what conditions can these methods throw? What made you decide to ignore (rather than handle) errors here? | |
| 188 | This should be browseFuture, not branchFuture. | |
| src/applications/diffusion/controller/DiffusionRepositoryController.php | ||
|---|---|---|
| 159–161 | Before, if we have no content, we don't build anything (or go into this method). However now we build out the branch selector if there are other branches, but not a default. So if that happens, this was fataling on $drequest->getCommit() because we're not in a branch. The "wrong branch" error is still handled above. I'm not sure how else to better code this, it felt flimsy. | |
Comment Actions
Let me counter-diff you on rewriting buildNormalContent(), I think it'll be easier to just feel out the restructuring myself than try to do it inlines.
Comment Actions
I plan to bring Tags back in a similar dropdown fashion. Then eventually get the courage up to make those two widgets all JS.
Comment Actions
If you want to split this into "selector" and "better behavior for a missing default branch", I'm happy to accept the selector.
I don't want to bring try { a bunch of stuff } catch ($ex) { just ignore it } upstream since I'm worried that kind of change is particularly likely to lead to support headaches.
