Page MenuHomePhabricator

Ambiguous display of commits in maniphest
Closed, ResolvedPublic

Description

Due to the way we are using branches maniphest will sometime display ambiguous commit lines - that is, two identical commits are shown are on different branches. The ambiguity would be resolved if the branch name were to show either in the view directly or when the user hovers a given commit.

There are two instances where commit hashes are ambiguous:

(1) when visiting a revision

(2) when attaching a revision to a task

In both cases it would help if the branch would be indicated nearby. I attached two mocks what attach the branch to the right of the links. A less intrusive option would be if hovering would show the branch.

Event Timeline

cos created this task.Apr 11 2017, 5:14 PM

This is somewhat difficult because querying which branches a commit appears on is slow (in some cases, extremely slow; see T9898), and it is difficult to cache because a small write (pushing one new branch) can change the branches that millions of commits appear on. For example, if I do this:

$ git checkout master
$ git checkout -b master2
$ git push origin master2

...and master has 1M ancestors, I've now invalidated any branch caches for 1M commits. This workflow is very common for some installs (see some discussion of "git push = save changes" in T5000 and elsewhere).

This isn't necessarily intractable, but maybe we can find a simpler solution by understanding your workflow in more detail -- why do you end up with the same commits on multiple different branches?

In Phabricator, we end up in this situation rarely, and mostly because we sometimes cherry-pick bug fixes from master to stable. To disambiguate the commits, we currently just prefix these cherry-picks with (stable) in the title. For example:

The former is the original; the latter is the cherry-pick to stable. The (stable) prefix in the subject line makes this clear in Phabricator, but also in other tools like git log --oneline, email, etc. This approach seems to work well for us, although these commits are rare enough that they're all handled manually today.

Could that be a workable approach, or are there reasons you'd prefer not to pursue it?

("We don't want to do that just to make Phabricator work" is a completely reasonable response, but at least in this project this practice seems useful and we'd probably keep doing it even if branch information was more accessible.)

We routinely have multiple commits on multiple branches because we mirror (via Harbormaster) our master branch to an experimental/master branch. This is safe because the two branches are never touching the same paths.

The simplified version of the script that does the cherry-picking is following:

commits=$(git cherry origin/experimental/master HEAD | grep '^\+ ' | cut -d ' ' -f2 | xargs) 
if [[ -n "$commits" ]]; then
  (git cherry-pick --abort; true) &&
  git reset --hard origin/experimental/master &&
  git cherry-pick $commits &&
  git push origin HEAD:experimental/master
fi

I don't see how to automatically attach a prefix to the title using git cherry-pick. In our case the commits coming from the master are obvious because they require code review (hence include things like Summary: and Reviewers:) while the commits on the experimental/master don't.

epriestley added a comment.EditedApr 12 2017, 10:13 PM

I think you could do something like this (made-up pseudocode because my bash is garbage):

git reset --hard ...
foreach ($commits as $commit) {
  git cherry-pick $commit && git log -n 1 --format="(experimental) %B" HEAD | git commit --amend -F -
}
git push origin ...

You could also possibly set Diffusion to track only master (and not experimental/master) like this:

But that's only useful if you never care about experimental/master.

Er, that wasn't even close -- this is simpler and actually works:

... && git log -n 1 --format="(experimental) %B" HEAD | git commit --amend -F -

Thanks for the suggestion. I'm trying that now.

I can confirm that the solution works properly. Once again, thanks! :-)

epriestley closed this task as Resolved.Apr 12 2017, 11:24 PM
epriestley claimed this task.

Great! Handling of branches is definitely something we'd like to improve in the future, but most of the improvements we have planned require significant amounts of work. If you eventually run into limitations with this approach, hopefully we'll have some improvements in place by then.