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

[James Y Knight via cfe-dev]

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/cfe-dev/attachments/20200128/db3e755b/attachment.html>