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

On 01/22, David Greene via cfe-dev wrote:
> Renato Golin via cfe-dev <cfe-dev at lists.llvm.org> writes:
> 
> >> My understanding from other people's comments (I've not
tried it myself) is that updating a patch series in github is problematic - that
it sends new/separate review email rather than being able to associate each
patch in the series with ongoing review feedback and updates?
> >
> > Both Phab and GitHub are problematic in different ways. In Phab, an
> > earlier change that trickles to the rest of the series needs to be
> > updated on all patches. In GitHub, some information is lost,
> > especially if it's a force push, but it only sends one email for
the
> > series.
> 
> Isn't force push on a private branch (series) going to be rather common
> as authors respond to reviews?  Or is there a better model?
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).

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.
-------------- 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/f6ad3338/attachment.sig>

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.

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 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.

Single commits are harder to find cause of breakages and the commit
message may lose info if the merger is not the author.

Multiple useless commits (fix typo, whitespace, reordering
switch-case) can create rebase problems for everyone else.
> 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.

--renato

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>

"Doerfert, Johannes" <jdoerfert at anl.gov> writes:
> As I understand it, the GH model is to amend a new commit to your PR
> which addresses the review comments.
I'm fine with that during the review process as long as the commit
history is cleaned up before final merge.
> 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).
The question is if everything is approved and the author does a final
cleanup as alluded to above, does that final cleanup also need to go
through review?

                    -David

On Tue, 28 Jan 2020 at 16:09, David Greene <dag at cray.com>
wrote:
> The question is if everything is approved and the author does a final
> cleanup as alluded to above, does that final cleanup also need to go
> through review?
We don't enforce that now, so I see no reason to start doing it.

Phab reviews, once approved, can have last-minute modifications and
direct commits.

Often reviewers will reply "LGTM with this nit addressed", which means
the author is supposed to fix the code, ammend/squash, and push.

A rebase squash of the fixups is slightly more complex if they need to
apply to different patches in the series, but the author is *expected*
to run a final test on the rebased version *before* pushing.

If not, buildbots will break and patches reverted.

An author that keeps doing that will raise their own bar on future
reviews, where reviewers would start asking them to rebase and apply
before approving and merging.

cheers,
--renato