<div dir="ltr"><div dir="ltr">On Fri, 11 Sep 2020 at 19:32, Hubert Tong via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org">llvm-dev@lists.llvm.org</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>There seems to be a split between those who prefer to curate commits locally and present them in the PR (i.e. Method 3) as they are to be committed (i.e. squash/amend/etc from my workstation and push the result), and those who seem to feel that it is better to avoid "git rebase" and "git commit --amend" and let github handle rewriting the history with the commit.<br></div></div></blockquote><div>Are these primarily developers or reviewers with these preferences? Are the people using Method 3 handling inline comments diligently? Is GitHub still "great" if Method 1 is the model with the least surprises?</div></div></div></blockquote><div><br></div><div>I don't think there's a clear difference between developers and reviewers, we're all usually both at some point. I also tend to fluctuate between manually curating commits to not caring much at all. I think if we try to codify this with precision, no one will be satisfied. Worst still, a lot of people will end up doing "the wrong thing" by accident and we'll all get upset.</div><div><br></div><div>On my current project, we all have very different views and styles, but we reached a common consensus:</div><div>1. No master rewrite, just like LLVM. This is madness, period.</div><div>2. No branch merge commit into master.  Just like LLVM. We only thoroughly test the master branch.</div><div>3. The original author decides if they want to rebase or squash at the end.</div><div><br></div><div>We're already lenient with commits and reverts, there's no point in being tough just because we have a tool that allows us to do so.</div><div><br></div><div>We already require people to write decent commit messages. This would cover the review comments nicely: all comment reviews should be bundled in one last commit, with a tag like "address review comments" or whatever and a list of changes.</div><div><br></div><div>Of course, if some review changes the series consistently, it could give rise to a few more meaningful commits, with decent messages on their own. No rules broken there.</div><div><br></div><div>So, in a nutshell, review/approval like current phab, restrict to rebase/squash, let people decide, continue encouraging decent commit messages and squashing final review changes.</div><div><br></div><div>cheers,</div><div>--renato</div></div></div>