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.

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.