Diffs closed by the wrong commit with merge commits


#1

It appears that when using arc land --merge on a git repository, the commit which closes a Differential isn’t the merge commit but is the second commit in a branch being merge.

Reproduction steps:

  1. Set up a hosted git repository in Diffusion with history.immutable set to true
  2. Clone it
  3. Create a branch and commit something to it (the brancn now points to SHA X)
  4. Create a differential from that branch (arc diff)
  5. Make additional changes on that branch (in response to code review, say) and commit them (the branch now points to SHA Y)
  6. Update the differential with those changes (arc diff again)
  7. Land with a merge (arc land or arc land --merge if you want to be explicit). Master now points to sha Z which is a merge commit between SHA Y and the old master
  8. Observe that the Differential is closed not by the merge commit but by either commit X or commit Y. If you press the “Explain Why” button in differential, you get the following somewhat odd message:

42

Ideally, Phabricator should use the actual merge commit (which has a commit message that includes the Differential Revision field) to close the diff, not the first commit it sees whose SHA matches a SHA attached to the diff.

I started looking through the code to figure out where this matching is occurring but figured I’d post here while I look.


Diffusion graph view review link points to wrong diff?
#2

BTW, the main reason this is annoying to us is that it means Differential shows the wrong diff for Revisions closed via a merge commit.

You can find the right diff by using the History viewer to select the right commits, but that’s an extra (and annoying) step.


#3

Are there any plans to properly support merge workflows in Phabricator?

We ran into an elaboration of this problem today where if you have two branches which are based on each other (ie., master points to sha A, branch1 points to A->B, and branch2 points to A->B->C), then you land branch1, Phabricator will attach commit B to branch2 with some non-zero probability. This caused a huge amount of breakage today when some engineers who were working on a project landed a large diff which contained ~40 commits most of which were shared between ~10 differentials.


#4

This sounds like a legitimate bug, but I don’t have the time right now to properly reproduce and file a ticket. Hopefully this weekend.


#5

I resolved this issue in my case by disabling autoclose (Put autoclose on “none” to repository config branch tab. I did a sql query to modify this on all repos at once).

Then i patched arcanist to automatically run arc --close-revision ${revision_id} after execution of arc land command (modified arcanist land workflow files)


#6

unfortunately, I don’t think there’s any chance I could get my entire userbase to use a modified arcanist.


#7

You can still disable autoclose and make users close reviews by hand on review page. This way no erroneus information is presented in the graph view.


#8

This is causing a large amount of friction with my users. Is there any way I can help get this ticketed in secure.phabricator.com, @avivey?


#9

I don’t think I’ll have the time in the next week or two to upstream it, sorry.


#10

See https://secure.phabricator.com/T4453 and https://secure.phabricator.com/T11051.