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

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

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>

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>