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

On 01/24, Renato Golin wrote:
> On Fri, 24 Jan 2020 at 17:11, Doerfert, Johannes <jdoerfert at
anl.gov> wrote:
> > As I understand it, the GH model is to amend a new commit to your PR
> > which addresses the review comments. The "problem" is that
"we" are used
> > to the force push model in which each commit is always as "clean
and self
> > contained" as possible (this is not only because of Phab I'd
argue).
> 
> With Git, *adding* commits to a pull request that address the comments
> is good practice.>
> This does not change the the original commits, and in the web UI,
> doesn't disturb the comments.
> 
> New comments can be added, and fixups can be approved with a comment:
> "commit X is good, wait for the series to be approved to push".
> 
> Better still, it's very easy for anyone (with git) to checkout,
> cherry-pick, rebase and try things locally during code review.
Arguably, having clean self contained commits is for this use case even
better. While you can cherry-pick N commits after N rounds of review it
is easier and more convenient to have a single one.
> In the end, when the series is approved, there are a number of options
> before pushing:
> * You can merge as is (works if the author had good commit messages
> for all fixups)
> * You can squash-and-merge, which will add all commit messages to the
> one patch (merger can edit)
> * You can request the author to squash the fixups on the original commits
I can see that a suitable model can be found with the last two, or a
combination of the three.

> I prefer the last one. It's more work, but it lands "as the author
> intended" because it gives them more time to rework local history.
Agreed.

> Single commits are harder to find cause of breakages and the commit
> message may lose info if the merger is not the author.
I don't understand:
  - Single commits per feature should make it easier to find cause of
    breakage. What do I misunderstand?
> Multiple useless commits (fix typo, whitespace, reordering
> switch-case) can create rebase problems for everyone else.
I think most people are in agreement that those should be avoided if
possible, e.g., as part of a review, but are fine if they come alone as
NFC commits.

> > The first experience I had with GH, someone modified my pull request
> > through the web interface. (There is a way to disallow this but I
didn't
> > know.) Then I force pushed my updates to my private branch and the
> > person was angry that I undid all the work. We did manage to recover
it
> > but it is not easily accessible as far as I know.
> 
> Editing on the web is a horrendous practice and I was very cross when
> someone did that on my patch before merging, because they got it wrong
> and I had to do another fix.
> 
> Patch review is intended as quality control. They're not inalienable
> right to do whatever you want it someone else's work. I find the
> practice deeply disrespectful.
> 
> So, if there's a way to disable that, I'm strongly in favour of
doing so.
I agree with you completely. I was surprised and unhappy that this was
possible myself. If we move to GH this should be disabled if possible
and "shunned upon" if we cannot disable it. (To be fair, it might be
the
case that only owners can do it so there is little risk it happens to
us.)

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200124/6c33c4a9/attachment.sig>

On Fri, 24 Jan 2020 at 18:22, Doerfert, Johannes <jdoerfert at anl.gov>
wrote:
> > Single commits are harder to find cause of breakages and the commit
> > message may lose info if the merger is not the author.
>
> I don't understand:
>   - Single commits per feature should make it easier to find cause of
>     breakage. What do I misunderstand?
I meant single commit as in "whole patch series merged into one before
push".

This is usually a bad idea, unless the series is super-destructive and
the reviewers suggest a squash to minimise the damage.

One commit per feature / step is the best way, IMO, so I think we're
in agreement.

--renato

On Fri, Jan 24, 2020 at 7:22 PM Doerfert, Johannes <jdoerfert at anl.gov>
wrote:
> On 01/24, Renato Golin wrote:
> > On Fri, 24 Jan 2020 at 17:11, Doerfert, Johannes <jdoerfert at
anl.gov> wrote:
> > > As I understand it, the GH model is to amend a new commit to your
PR
> > > which addresses the review comments. The "problem" is
that "we" are used
> > > to the force push model in which each commit is always as
"clean and self
> > > contained" as possible (this is not only because of Phab
I'd argue).
> >
> > With Git, *adding* commits to a pull request that address the comments
> > is good practice.>
>
> > This does not change the the original commits, and in the web UI,
> > doesn't disturb the comments.
> >
> > New comments can be added, and fixups can be approved with a comment:
> > "commit X is good, wait for the series to be approved to
push".
> >
> > Better still, it's very easy for anyone (with git) to checkout,
> > cherry-pick, rebase and try things locally during code review.
>
> Arguably, having clean self contained commits is for this use case even
> better. While you can cherry-pick N commits after N rounds of review it
> is easier and more convenient to have a single one.
Agreed.

> > In the end, when the series is approved, there are a number of options
> > before pushing:
> > * You can merge as is (works if the author had good commit messages
> > for all fixups)
> > * You can squash-and-merge, which will add all commit messages to the
> > one patch (merger can edit)
> > * You can request the author to squash the fixups on the original
commits
>
> I can see that a suitable model can be found with the last two, or a
> combination of the three.
I find it much more important to have a clean history and would
therefore frown upon the first option; keeping the fixup commits
around is almost never a good idea.

One of the most important goals of history management should be to
have a useful `git bisect`. This requires that every individual commit
leaves the codebase in a "clean" state. (Ideally we'd have bots
that
check each individual commit, but that's a resourcing problem.)

> > I prefer the last one. It's more work, but it lands "as the
author
> > intended" because it gives them more time to rework local
history.
>
> Agreed.
Agreed.

> > Single commits are harder to find cause of breakages and the commit
> > message may lose info if the merger is not the author.
>
> I don't understand:
>   - Single commits per feature should make it easier to find cause of
>     breakage. What do I misunderstand?
>
> > Multiple useless commits (fix typo, whitespace, reordering
> > switch-case) can create rebase problems for everyone else.
>
> I think most people are in agreement that those should be avoided if
> possible, e.g., as part of a review, but are fine if they come alone as
> NFC commits.
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 agree that it's good practice to have such reorderings etc. in
separate, NFC commits. That's actually one of the reasons why I think
a good flow for patch-series-centric review is so important, because
it facilitates this good practice.

Cheers,
Nicolai

> > > The first experience I had with GH, someone modified my pull
request
> > > through the web interface. (There is a way to disallow this but I
didn't
> > > know.) Then I force pushed my updates to my private branch and
the
> > > person was angry that I undid all the work. We did manage to
recover it
> > > but it is not easily accessible as far as I know.
> >
> > Editing on the web is a horrendous practice and I was very cross when
> > someone did that on my patch before merging, because they got it wrong
> > and I had to do another fix.
> >
> > Patch review is intended as quality control. They're not
inalienable
> > right to do whatever you want it someone else's work. I find the
> > practice deeply disrespectful.
> >
> > So, if there's a way to disable that, I'm strongly in favour
of doing so.
>
> I agree with you completely. I was surprised and unhappy that this was
> possible myself. If we move to GH this should be disabled if possible
> and "shunned upon" if we cannot disable it. (To be fair, it might
be the
> case that only owners can do it so there is little risk it happens to
> us.)


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

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