Page MenuHomePhabricator

Fix error for URL's that could mean several commits
ClosedPublic

Authored by lpriestley on Dec 3 2017, 8:26 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Mar 22, 11:37 AM
Unknown Object (File)
Feb 24 2024, 4:53 PM
Unknown Object (File)
Feb 13 2024, 4:50 PM
Unknown Object (File)
Feb 3 2024, 5:14 PM
Unknown Object (File)
Feb 3 2024, 5:13 PM
Unknown Object (File)
Feb 3 2024, 5:13 PM
Unknown Object (File)
Feb 3 2024, 5:13 PM
Unknown Object (File)
Feb 3 2024, 5:13 PM
Subscribers
Tokens
"Dat Boi" token, awarded by epriestley.

Details

Summary

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.

Test Plan
  • Navigate to install/rPbd3c23
  • User should see a list view providing links to install/rPbd3c2355e8e2b220ae5e3cbfe4a057c8088c6a38 and install/rPbd3c239d5aada68a31db5742bbb8ec099074a561

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

I think this breaks unambiguous commits (like /rP<full 40-character hash>), since $commit is now an array:

Screen Shot 2017-12-03 at 12.29.18 PM.png (1×1 px, 165 KB)

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 ProfileCommits or DiffusionSome RepositoryHistory 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:

Screen Shot 2017-12-03 at 12.34.12 PM.png (129×1 px, 18 KB)

src/applications/diffusion/controller/DiffusionCommitController.php
76

Maybe do something like $crumbs->addTextCrumb(pht('Ambiguous Commit')) in this case.

This revision now requires changes to proceed.Dec 3 2017, 8:36 PM

Conforming to best practices for commit lists

Screen Shot 2017-12-04 at 11.13.23 AM.png (561×1 px, 72 KB)

  • 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.

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.

This revision is now accepted and ready to land.Dec 5 2017, 2:36 PM
lpriestley marked an inline comment as done.

Make ambiguous commit identifier bold in the warning message

This revision was automatically updated to reflect the committed changes.