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

[Renato Golin via cfe-dev]

On Fri, 11 Sep 2020 at 19:32, Hubert Tong via llvm-dev <
llvm-dev at lists.llvm.org> wrote:

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

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.

On my current project, we all have very different views and styles, but we
reached a common consensus:
1. No master rewrite, just like LLVM. This is madness, period.
2. No branch merge commit into master. Just like LLVM. We only thoroughly
test the master branch.
3. The original author decides if they want to rebase or squash at the end.

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.

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.

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.

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.

cheers,
--renato
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20200911/4797073a/attachment-0001.html>