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

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

On Tue, Jan 28, 2020 at 11:31 AM Renato Golin via cfe-dev <
cfe-dev at lists.llvm.org> wrote:
> 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.
>
I would really not want to review a Pull Request that both has multiple
"real" commits, as well as unsquashed "fixup" commits in it.
That makes it
quite difficult to see the final state of things for any of the commits.
Since Github lets you see the diff for the entire PR, if the entire branch
is intended to be squashed before commit, it seems potentially-OK (if ugly)
for it to temporarily have multiple commits during review, rather than
force-pushing a new commit.

But, if you're making a pull request which is intended to result in
multiple logically-distinct commits on master, that no longer works. in
that case, ISTM that the only realistically-reviewable option is to
rebase/squash before uploading every time.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200128/db3e755b/attachment-0001.html>

On Tue, 28 Jan 2020 at 16:41, James Y Knight <jyknight at google.com>
wrote:
> I would really not want to review a Pull Request that both has multiple
"real" commits, as well as unsquashed "fixup" commits in it.
That makes it quite difficult to see the final state of things for any of the
commits. Since Github lets you see the diff for the entire PR, if the entire
branch is intended to be squashed before commit, it seems potentially-OK (if
ugly) for it to temporarily have multiple commits during review, rather than
force-pushing a new commit.
Indeed, this works better when the number of commits is small and the
nature of the fixups is trivial.

If the change is significant, then having the old commit laying around
is counter-productive and rebase-squashing is the only sane way.

--renato