Page MenuHomePhabricator

Commit hashes of landed patches need not match the latest ones shown in Differential
Closed, DeclinedPublic

Description

The second (i.e. last) Differential Revision of D20, is listed as containing only commit c38e72900ac5.

When arc land-ing this commit after master has incorporated other commits, Phabricator correctly closed the D20.

But although D20 has been marked as closed, the above commit c38e72900ac5 never made it into master. Instead, master has: 855d3a19c625, which is a rebased merge commit of c38e72900ac5 and (back then) master.

This is confusing, as qchris2 reviewed and arc land-ed a different commit than what is shown in master. And when looking at master alone, it is (due to arc land's behaviour) not even visible that qchris2 reviewed a different commit. :-(

And although qchris2 never saw master's commit, it's commit message nevertheless says “Reviewed By: qchris2” :-(

So when looking on the git-only side, it is impossible to see which commit qchris2 reviewed.

And when only looking at the Phabricator side, it's really hard to identify as well. The only indication (I could find) that a different commit has been merged to master is that D20's header says

Commits: rQCHRISTESTFOREIGN855d3a19c625: Add foo10.txt

instead of

Commits: rQCHRISTESTFOREIGNc38e72900ac5: Add foo10.txt

(which would be expected when looking at the Commit hash of first commit of
the second Differential Revision). I doubt I'd check that each time or
notice the mismatch.

The fact that a different commit got merged is too hidden from both
the commits generated by arc land, and also Phabricator's web UI.


Here's a git log of the different commits:

. *   bdf9a71 (foo10-arc-land-suggested) Automatic merge by 'arc land'
. |\  
. * | c38e729 (foo10-reviewed) Add foo10.txt
. | | * 855d3a1 (HEAD, master, foo10-merged) Add foo10.txt
. | |/  
. | * f1414cd intermission
. |/  
. * 145c288 Add foo9.txt
. [...]

Refs:

  • foo10-reviewed is the second Differential of D20. This is the commit qchris2 reviewed.
  • foo10-arc-land-suggested is the commit that arc land suggested to checkout if I want the killed feature branch back.
  • foo10-merged is the commit that got merged to master

You can clone them from https://github.com/somechris/fabtest2 if you want to take a look.

See also T136: Pulling patches from Phabricator does not give consistent commit hashes

Details

Reference
fl268

Event Timeline

flimport raised the priority of this task from to Low.Sep 12 2014, 1:33 AM
flimport set Reference to fl268.

milimetric wrote on 2014-05-01 13:48:12 (UTC)

[paraphrasing] commit sha1 is different after arc land

arc land can be configured to, by default, not squash commits.

[paraphrasing] reviewer information is lost or confusing if we're not looking at phabricator, but instead are just looking at the local checkout

I agree that this is an issue. But Gerrit does not have *any* reviewer information in the commits when you're looking at the local checkout. So while this could qualify as a feature request, it shouldn't be something we miss when migrating from Gerrit.

qchris wrote on 2014-05-01 17:55:44 (UTC)

[paraphrasing] commit sha1 is different after arc land

arc land can be configured to, by default, not squash commits.

Right. The one who arc lands has the option to choose between
squashing and merging. But in order for both options to be valid,
Phabricator should make clear which commits got reviewed and which
commits got merged—regardless of whether the one who landed
the change used “squash” or “merge” was used.

This bug exhibits that for “squash”, Phabricator does not show this
information appropriately.

But Gerrit does not have *any* reviewer information in the commits when you're looking at the local checkout.

Gerrit bashing where Gerrit bashing is due :-) but our Gerrit has
and exposes this information.

But as this information is not crucial to the commit (and for some it
would just be noise), Gerrit does the only sensible thing and stores
review information in git notes.

If origin is a remote of a repo from WMF's gerrit, just run

git fetch origin refs/notes/review:refs/notes/review

And you have all review information in your local checkout. To see it,
just tell the viewer of your choice to display review notes. For
example via:

git log --show-notes=review

As git notes are a git built in, you can add them as default to all
relevant places, and for example can have it fetched automatically
along with other refs. And repo tools (such as gitk) typically
handle them nicely.

So while this could qualify as a feature request, it shouldn't be something we miss when migrating from Gerrit.

I would miss it.

But regardless of whether or not Gerrit provides this
information ... Having Phabricator add wrong reviewer information
is worse than having no reviewer information.

milimetric wrote on 2014-05-01 18:02:02 (UTC)

This bug exhibits that for “squash”, Phabricator does not show this information appropriately.

Fair enough, but I think "squash" is a new workflow that has no parallel in Gerrit. We would probably just use "merge" at first (since it's more similar) and talk about "squash" while we maybe upstream some of these fixes.

git log --show-notes=review

As soon as I clicked "Submit" on my comment, I knew that you would know of some crazy way that gerrit *does* indeed store this information in git. And sure enough, you managed to teach me about yet another feature of git that I was not aware of. Thank you, and that's really cool :)

I think, yeah, it seems that Phabricator's git integration is just more loose. But upstreaming some tighter integration seems like a win to me. It's certainly easier than building project management and all the other Phabricator features into Gerrit.

mattflaschen wrote on 2014-05-01 21:07:29 (UTC)

The title is true (the hash is different). However, there is no actual squashing going on in this example. It's just a straight rebase (Gerrit also has a rebase on submit option; see https://gerrit-review.googlesource.com/Documentation/project-setup.html).

I can't speak about other Phabricator squash scenarios, but in this case, the two commits have exactly the same diff, but different parents:

git diff foo10-merged^ foo10-merged|shasum
f7956d2423f33fd4283087abb0c0a9572912a3e0 -

git diff foo10-reviewed^ foo10-reviewed|shasum
f7956d2423f33fd4283087abb0c0a9572912a3e0 -

I do agree we should look at requiring a "Merge if Necessary" (rather than squash or rebase) workflow, since I think that's what's most familiar to Wikimedia Gerrit users.

In T162#2189, @flimport wrote:

mattflaschen wrote on 2014-05-01 21:07:29 (UTC)
I do agree we should look at requiring a "Merge if Necessary" (rather than squash or rebase) workflow, since I think that's what's most familiar to Wikimedia Gerrit users.

@mattflaschen, is this something that we should take into account for T597: Identify Arcanist showstoppers for wikimedians? Or a process that we should include already in T560: Proof of concept of code review in Phabricator?

In T162#10227, @Qgil wrote:
In T162#2189, @flimport wrote:

mattflaschen wrote on 2014-05-01 21:07:29 (UTC)
I do agree we should look at requiring a "Merge if Necessary" (rather than squash or rebase) workflow, since I think that's what's most familiar to Wikimedia Gerrit users.

@mattflaschen, is this something that we should take into account for T597: Identify Arcanist showstoppers for wikimedians? Or a process that we should include already in T560: Proof of concept of code review in Phabricator?

I think for now, we should set history.immutable to true (see https://secure.phabricator.com/book/phabricator/article/arcanist_new_project/). Although Wikimedia Gerrit users are used to amending their commits (in response to code review), they are not used to them being squashed together by the code review tool.

Using history.immutable causes (among other things) the equivalent of --merge when landing:

--merge
    Supports: git
    Perform a --no-ff merge, not a --squash merge. If the project
    is marked as having an immutable history, this is the default
    behavior.

If we do that, we should also set a convention for now not to use --squash (it might be confusing for some people to use --squash despite the project being history.immutable).

See T163: `arc diff` changes current ref in local checkout which proposes the same solution.

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 9 2015, 3:18 PM
greg moved this task from To Triage to Backlog on the Differential board.Nov 4 2015, 7:27 PM
hashar added a subscriber: hashar.Nov 5 2015, 10:20 AM

For what it is worth, it seems arc default behavior is similar to the Gerrit cherry-pick merge strategy. If set on a repo, Gerrit always:

  • cherry pick the patch on the top of the branch
  • rewrite the summary message to add review informations
  • updates notes
  • craft a new patchset on the change
  • merge

The commit that lands in Gerrit does not reflect the local one, but it might rebase just fine since the only difference is in the commit message.

I don't think it is going to be much of an issue with arc.

greg moved this task from To Triage to Tooling on the Gerrit-Migration board.Dec 2 2015, 12:30 AM
Qgil removed a subscriber: Qgil.Dec 2 2015, 11:50 AM
mmodell closed this task as Declined.Oct 31 2018, 7:37 PM