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

[Hubert Tong via llvm-dev]

On Fri, Sep 11, 2020 at 6:12 PM Renato Golin <rengolin at gmail.com> wrote:

> 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,
>
Current phab has better diff-to-diff comparison than anything I've seen on
GitHub. It even deals with merge-from-master/rebase-on-master fairly well.
"Reviewing like current phab" matches "Method 1" more.


> restrict to rebase/squash
>
As in no final "merge". I'm fine with that.
I'll note that "rebase" is a departure from current phab in that a single
review creates multiple commits. Not that I am opposed to such commits, but
I am concerned with continual force pushes to PR branches. A "final history
clean-up" after reviews are done is okay in my book.


> , let people decide, continue encouraging decent commit messages and
> squashing final review changes.
>
>From the above, I take it to mean that this "squashing" you mean last here
is about rewriting the history (if not using GitHub's "squash and merge")
before using GitHub's "rebase and merge"?


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