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

[Nicolai Hähnle via cfe-dev]

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.