<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Jan 28, 2020 at 11:31 AM Renato Golin via cfe-dev <<a href="mailto:cfe-dev@lists.llvm.org">cfe-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Tue, 28 Jan 2020 at 16:09, David Greene <<a href="mailto:dag@cray.com" target="_blank">dag@cray.com</a>> wrote:<br>
> The question is if everything is approved and the author does a final<br>
> cleanup as alluded to above, does that final cleanup also need to go<br>
> through review?<br>
<br>
We don't enforce that now, so I see no reason to start doing it.<br>
<br>
Phab reviews, once approved, can have last-minute modifications and<br>
direct commits.<br>
<br>
Often reviewers will reply "LGTM with this nit addressed", which means<br>
the author is supposed to fix the code, ammend/squash, and push.<br>
<br>
A rebase squash of the fixups is slightly more complex if they need to<br>
apply to different patches in the series, but the author is *expected*<br>
to run a final test on the rebased version *before* pushing.<br></blockquote><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div></div></div>