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

On Fri, Sep 11, 2020 at 6:53 PM David Blaikie <dblaikie at gmail.com>
wrote:
> Is there any observable difference between "Squash and Merge" or
"Rebase
> and Merge" when "enforce linear history" is enabled, then?
>
"Squash and Merge" will only generate one commit.

>
> 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.
>>
> I am not aware of/still have not experienced a case where "Rebase and
Merge" retains merge commits.

>
>> 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/llvm-dev/attachments/20200911/a705ceda/attachment-0001.html>

On Fri, 11 Sep 2020 at 23:56, Hubert Tong via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> 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.
>>>
>> I am not aware of/still have not experienced a case where "Rebase
and
> Merge" retains merge commits.
>
It does not. We can disable the merge commit on Github settings. Plus, the
master branch history protection will stop people from merge-commit or
force-push by hand anyway.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200912/16f73b3e/attachment.html>

On 2020-09-12, Renato Golin via llvm-dev wrote:
>On Fri, 11 Sep 2020 at 23:56, Hubert Tong via llvm-dev <
>llvm-dev at lists.llvm.org> wrote:
>
>> 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.
>>>>
>>> I am not aware of/still have not experienced a case where
"Rebase and
>> Merge" retains merge commits.
>>
>
>It does not. We can disable the merge commit on Github settings. Plus, the
>master branch history protection will stop people from merge-commit or
>force-push by hand anyway.
One property of "Squash and merge" is that it will add intermediate
commits as bullet points (`* `). In many cases the merger does not spend
more time cleaning up the description so a commit may look like:

```
     RFC: treat small negative λ as 0 for sqrt(::Hermitian) (#35057)

     * treat small negative λ as 0 for sqrt(::Hermitian) and log(::Hermitian)

     * typo

     * added tests, docs; removed rtol argument for log

     * don't ask for rtol so close to eps(Float64)
```

The typo and `added tests` messages should probably not be there. The circt
commit demonstrates the exact same problem:
https://github.com/llvm/circt/commit/9ab7d667f9ad5de06c7986a71ad5e07e19cd55e7