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

[Stephen Neuendorffer via llvm-dev]

Just to clarify: All the LLVM incubator repositories have "enforce linear
history" enabled.  Neither "Squash and Merge" or "Rebase and Merge" results
in a Merge commit in the git history.

Steve

On Fri, Sep 11, 2020 at 3:32 PM Hubert Tong <
hubert.reinterpretcast at gmail.com> wrote:

> 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/5930a0c6/attachment.html>