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

[David Blaikie via cfe-dev]

Is there any observable difference between "Squash and Merge" or "Rebase
and Merge" when "enforce linear history" is enabled, then?

On Fri, Sep 11, 2020 at 3:45 PM Stephen Neuendorffer via llvm-dev <
llvm-dev at lists.llvm.org> wrote:

> 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
>>>
>> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20200911/49ece510/attachment.html>