Ref T13001, URLs that return multiple commits should show a list of those commits. Not sure if the actual list looks very pretty this way, but was wondering if this approach was vaguely correct.
Details
Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T13001: Ambiguous commits should prompt you to select from options
- Commits
- rP46d496b8cc06: Fix error for URL's that could mean several commits
- Navigate to install/rPbd3c23
- User should see a list view providing links to install/rPbd3c2355e8e2b220ae5e3cbfe4a057c8088c6a38 and install/rPbd3c239d5aada68a31db5742bbb8ec099074a561
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
I think this breaks unambiguous commits (like /rP<full 40-character hash>), since $commit is now an array:
The UI also uses elements a little bit unusually -- in particular, no other UI puts non-text elements inside a PHUIInfoView. Instead, try this:
- Render the commit list more similarly to the User Profile → Commits or Diffusion → Some Repository → History pages, so it's a standalone element (not inside a box) with spacing between the dates.
- Put the PHUIInfoView on top with an explicit error message like "The identifier abc123 is ambiguous and matches more than one commit.", similar to this warning in Differential:
| src/applications/diffusion/controller/DiffusionCommitController.php | ||
|---|---|---|
| 76 | Maybe do something like $crumbs->addTextCrumb(pht('Ambiguous Commit')) in this case. | |
Comment Actions
- The commit identifier in the warning message isn't bold... should it be?
- The repository/branch in the crumbs isn't a link... should it be? I looked at the crumbs code a bit and I think I'd have to add a new parameter to pass to crumbs so that we can have a link without a branch appended to it.
Comment Actions
You can bold it with phutil_tag('strong', array(), $commit_identifier) if you like. I think bolding it does make it read slightly more nicely.
Ideally the repository crumb would be linked but I don't think it's easy or worth the trouble. I think you're right that we'd have to add a dedicated mode.
| src/applications/diffusion/controller/DiffusionCommitController.php | ||
|---|---|---|
| 109 | Maaaaybe slightly more clear/common as head(...) ("get the first list item") instead of array_pop(...) ("remove and return the last list item") since head() doesn't modify $commits, but either will behave correctly here. | |