llvm dev - Jan 2020 - [cfe-dev] Phabricator -> GitHub PRs?

On Tue, 28 Jan 2020 at 12:28, Nicolai Hähnle <nhaehnle at gmail.com>
wrote:
> I don't quite follow. Yes, gratuitous useless changes tend to create
> merge and rebase problems and should generally be avoided. Why does it
> make a difference whether they're in multiple commits though?
I think you misunderstood.

Our policy [1] is that independent NFC fixups on existing code should
be separate from new changes.

The point here is that fixups *to the patch* should be squashed into
their original commits *before* merging.

Once the patches are in master, and problems have been found, then
adding a fixup to master is totally fine.

We don't want to rewrite history on master, that'd be a nightmare. If
something is botched, we revert, then reapply.

We also don't want to have fixups to things that haven't landed on
master yet, so cleaning up the patches and series before merging is
highly encouraged.

But during the review, rewriting history of the series itself makes it
hard for any review tool to have meaningful representations.

So, it's better to have separate fixups during review, but we really
should squash them into their related commits in the series before
pushing.

--renato

[1] http://llvm.org/docs/DeveloperPolicy.html

On Tue, Jan 28, 2020 at 8:23 AM Renato Golin via cfe-dev <
cfe-dev at lists.llvm.org> wrote:
> But during the review, rewriting history of the series itself makes it
> hard for any review tool to have meaningful representations.
>
This isn't really true for *any* review tool -- some are written to handle
this well. E.g. Gerrit is designed exactly for this flow.

In order to track the commits, it uses a "logical identity" for a
commit
(similar to Phabricator's "Differential Revision:" line), separate
from the
commit hash. A local commit hook is used to automatically generate a random
number (well, it's a hash of something, but may-as-well be a random
number), and appends it to the commit message (e.g. "Change-Id:
I0123456789abcdef0123456789abcdef01234567"). This ID subsequently serves as
the review permalink and identity. Because it's in the message, it's
preserved across rebases and amends.

Anyways, given that ID, it's easy to track the evolution of each commit in
your branch separately, and treat them as separately-reviewable changes,
which can each be individually ammended in response to review feedback.

Gerrit also makes it easy to upload an entire branch worth of commits --
just "git push" your branch to the magic-review-upload
"branch" on the
server. When you do that, every commit on your local branch either creates
a new review or updates the existing review automatically. And all the
reviews are linked together, so you can see the entire chain of reviews for
the branch. (example, see "Relation Chain" on the right:
https://go-review.googlesource.com/c/go/+/216221/2).

I think this is much nicer than Phabricator's method, because the Change-Id
is added to the message *before* it's uploaded, rather than having to go
back and amend each message after first upload to insert the
server-assigned id. And the entity which is uploaded is an actual git
commit, which means it can be more easily worked with (e.g. fetched by
someone else), similar to what you can do with github pull requests.

...But I'm not sure bringing up yet another tool really helps anything
here...sorry. =p
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200128/c6e04649/attachment.html>

On Tue, 28 Jan 2020 at 15:41, James Y Knight <jyknight at google.com>
wrote:
> In order to track the commits, it uses a "logical identity" for a
commit (similar to Phabricator's "Differential Revision:" line),
separate from the commit hash. A local commit hook is used to automatically
generate a random number (well, it's a hash of something, but may-as-well be
a random number), and appends it to the commit message (e.g. "Change-Id:
I0123456789abcdef0123456789abcdef01234567"). This ID subsequently serves as
the review permalink and identity. Because it's in the message, it's
preserved across rebases and amends.
I find Phab's handling of the differential revision as confusing as
Gerrit's commit IDs. On simple cases, it does help. When the updates
are extensive and conflicting, they both fall apart.

Of course, this is up to personal preferences. I don't want to start
any long thread about that topic, I don't think it's worth it. :)
> Gerrit also makes it easy to upload an entire branch worth of commits --
just "git push" your branch to the magic-review-upload
"branch" on the server. When you do that, every commit on your local
branch either creates a new review or updates the existing review automatically.
And all the reviews are linked together, so you can see the entire chain of
reviews for the branch. (example, see "Relation Chain" on the right:
https://go-review.googlesource.com/c/go/+/216221/2).
That is true. In that sense, Gerrit is better than GH. But GH still
allows you to update your branch and push and the review does get
updated, and to me, that's better (personally) than Phab.
> ...But I'm not sure bringing up yet another tool really helps anything
here...sorry. =p
I don't think we should consider Gerrit, but it's good to know what
other tools do, so that we can have a better perspective on the ones
we're actually considering.

The GH PR way may not be the most full featured but it's simple, "good
enough" (again, personal), and very cheap to maintain.

It'll also make trivial for people to fork LLVM on GH and send PRs.

cheers,
--renato

On Tue, Jan 28, 2020 at 2:23 PM Renato Golin <rengolin at gmail.com>
wrote:
> On Tue, 28 Jan 2020 at 12:28, Nicolai Hähnle <nhaehnle at gmail.com>
wrote:
> > I don't quite follow. Yes, gratuitous useless changes tend to
create
> > merge and rebase problems and should generally be avoided. Why does it
> > make a difference whether they're in multiple commits though?
>
> I think you misunderstood.
>
> Our policy [1] is that independent NFC fixups on existing code should
> be separate from new changes.
>
> The point here is that fixups *to the patch* should be squashed into
> their original commits *before* merging.
Okay, this intention wasn't clear to me from what was written. It
looks like we agree, then.


> Once the patches are in master, and problems have been found, then
> adding a fixup to master is totally fine.
>
> We don't want to rewrite history on master, that'd be a nightmare.
If
> something is botched, we revert, then reapply.
>
> We also don't want to have fixups to things that haven't landed on
> master yet, so cleaning up the patches and series before merging is
> highly encouraged.
>
> But during the review, rewriting history of the series itself makes it
> hard for any review tool to have meaningful representations.
This isn't quite true, as others already pointed out, and *not*
rewriting history can make reviews harder as well! In fact, I've *just
now* had that happen to me on another GitHub project, where this
sequence of events happened:

1. PR was opened with a series of commits, one of which (call it
commit B for base) is non-trivial and under review separately as a
different PR.
2. Other reviewer makes comments, asks for some refactoring changes.
3. Author makes those changes, adds them as a fixup commit.
4. I can now no longer usefully review the PR, because I only have two
options, both of which are similarly useless:
4a. I look at all changes in the PR, in which case I get a messy
mixture of commit B (which ought to be reviewed separately) and the
rest.
4b. I look at individual commits in the PR, but then I only see a
stale version of the author's work.

The fixup approach *might* work if there is only a single reviewer,
but even then I suspect things can quickly become messy. And in any
case, the default assumption in LLVM should be that anybody can join a
review at any time.

The one tool that actually gets this right is Gerrit, which
understands commit series *and* allows you to diff between versions of
a commit. It's unfortunate that Gerrit is so ugly that most people
won't even look at it (and it does have other weaknesses as well,
admittedly).

Cheers,
Nicolai


> So, it's better to have separate fixups during review, but we really
> should squash them into their related commits in the series before
> pushing.
>
> --renato
>
> [1] http://llvm.org/docs/DeveloperPolicy.html


-- 
Lerne, wie die Welt wirklich ist,
aber vergiss niemals, wie sie sein sollte.

Renato Golin <rengolin at gmail.com> writes:
> But during the review, rewriting history of the series itself makes it
> hard for any review tool to have meaningful representations.
>
> So, it's better to have separate fixups during review, but we really
> should squash them into their related commits in the series before
> pushing.
+1, regardless of which review system we use.

                  -David

The LLVM incubator projects have been using github PRs for reviews and so
far haven't really seen any significant issues.  The biggest confusion so
far has not been with reviews but with the difference between "rebase and
merge" and "squash and mere".  We have used basically 3 different
processes:

Method 1: start a review with one commit on a new branch (typically in a
personal repository 'fork'). Respond to review comments by adding
additional commits on the same branch. Then you want to "Squash and
Merge".   Real life example: https://github.com/llvm/circt/pull/62  Note
that Github keeps seems to keep a reference to commits where a review
occurred, but does not necessarily preserve other commits in the chain.
Also note that you can go back and look at either the final disposition of
the code on the main page, or view the diff "as reviewed" to
understand
comments in their original context.

Method 2: start a review as a sequence of commits. Respond to review
comments by updating your sequence of commits rebasing locally and git push
-f to update your branch with a new set of commits. The sequence of commits
is preserved using 'rebase and merge'.  Real life example:

Method 3: start a review with one commit, respond to review comments with
git commit --amend on the commit. git push -f to update your branch. In
this case, only one commit is being merged, so 'rebase and merge' and
'squash and merge'.  Real life example:
https://github.com/llvm/circt/pull/63

Some projects attempt to simplify the options and only allow "squash and
merge" as an option.  This eliminates Method 2, but also prevents mistakes
where people intend to use Method 1, but accidently select "Rebase and
merge".  This results in undesired commit history littered with a bunch of
meaningless commits like "Fix typo.", "Address comments."
etc  Method 2 is
generally harder to review and discouraged, but seems sometimes necessary
when you have dependent commits that are logically separate but need to be
reviewed together to make sense.

There seems to be a split between those who prefer to curate commits
locally and present them in the PR (i.e. Method 3) as they are to be
committed (i.e. squash/amend/etc from my workstation and push the result),
and those who seem to feel that it is better to avoid "git rebase" and
"git
commit --amend" and let github handle rewriting the history with the
commit.

Steve



On Wed, Jan 29, 2020 at 12:56 AM Nicolai Hähnle via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> On Tue, Jan 28, 2020 at 2:23 PM Renato Golin <rengolin at gmail.com>
wrote:
> > On Tue, 28 Jan 2020 at 12:28, Nicolai Hähnle <nhaehnle at
gmail.com> wrote:
> > > I don't quite follow. Yes, gratuitous useless changes tend to
create
> > > merge and rebase problems and should generally be avoided. Why
does it
> > > make a difference whether they're in multiple commits though?
> >
> > I think you misunderstood.
> >
> > Our policy [1] is that independent NFC fixups on existing code should
> > be separate from new changes.
> >
> > The point here is that fixups *to the patch* should be squashed into
> > their original commits *before* merging.
>
> Okay, this intention wasn't clear to me from what was written. It
> looks like we agree, then.
>
>
>
> > Once the patches are in master, and problems have been found, then
> > adding a fixup to master is totally fine.
> >
> > We don't want to rewrite history on master, that'd be a
nightmare. If
> > something is botched, we revert, then reapply.
> >
> > We also don't want to have fixups to things that haven't
landed on
> > master yet, so cleaning up the patches and series before merging is
> > highly encouraged.
> >
> > But during the review, rewriting history of the series itself makes it
> > hard for any review tool to have meaningful representations.
>
> This isn't quite true, as others already pointed out, and *not*
> rewriting history can make reviews harder as well! In fact, I've *just
> now* had that happen to me on another GitHub project, where this
> sequence of events happened:
>
> 1. PR was opened with a series of commits, one of which (call it
> commit B for base) is non-trivial and under review separately as a
> different PR.
> 2. Other reviewer makes comments, asks for some refactoring changes.
> 3. Author makes those changes, adds them as a fixup commit.
> 4. I can now no longer usefully review the PR, because I only have two
> options, both of which are similarly useless:
> 4a. I look at all changes in the PR, in which case I get a messy
> mixture of commit B (which ought to be reviewed separately) and the
> rest.
> 4b. I look at individual commits in the PR, but then I only see a
> stale version of the author's work.
>
> The fixup approach *might* work if there is only a single reviewer,
> but even then I suspect things can quickly become messy. And in any
> case, the default assumption in LLVM should be that anybody can join a
> review at any time.
>
> The one tool that actually gets this right is Gerrit, which
> understands commit series *and* allows you to diff between versions of
> a commit. It's unfortunate that Gerrit is so ugly that most people
> won't even look at it (and it does have other weaknesses as well,
> admittedly).
>
> Cheers,
> Nicolai
>
>
>
> > So, it's better to have separate fixups during review, but we
really
> > should squash them into their related commits in the series before
> > pushing.
> >
> > --renato
> >
> > [1] http://llvm.org/docs/DeveloperPolicy.html
>
>
>
> --
> Lerne, wie die Welt wirklich ist,
> aber vergiss niemals, wie sie sein sollte.
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200911/cbcff361/attachment.html>