llvm dev - Nov 2019 - Enable Contributions Through Pull-request For LLVM

Daniel Sanders via llvm-dev <llvm-dev at lists.llvm.org> writes:
> Personally, I'd like us to drop the linear history requirement but I
> know there's some strong opposition there. The main reason I'd like
us
> to accept non-linear history in conjunction with pull requests is that
> it means that the commits that were tested (whether by the author or
> by CI) still exist in the repository after the merge. If we
> rebase+push like we do today or squash+merge as in this proposal, then
> the information that a particular commit passed testing (or failed a
> particular test) gets lost.
-1 from me.  We have one project where we decided to "keep the original
commits" as you say above and another where we enforce linear history.
The linear history project is *much* easier to follow.  With merge
commits you start running into issues like foxtrot commits
(https://blog.developer.atlassian.com/stop-foxtrots-now) and the whole
thing easily becomes a mess.  There may be various rules/plugins/etc.
that attempt to enforce "good" behavior but in the long run my
experience has been that simpler rules make things simpler.

                     -David

> On Nov 8, 2019, at 08:28, David Greene <greened at obbligato.org>
wrote:
> 
> Daniel Sanders via llvm-dev <llvm-dev at lists.llvm.org> writes:
> 
>> Personally, I'd like us to drop the linear history requirement but
I
>> know there's some strong opposition there. The main reason I'd
like us
>> to accept non-linear history in conjunction with pull requests is that
>> it means that the commits that were tested (whether by the author or
>> by CI) still exist in the repository after the merge. If we
>> rebase+push like we do today or squash+merge as in this proposal, then
>> the information that a particular commit passed testing (or failed a
>> particular test) gets lost.
> 
> -1 from me.  We have one project where we decided to "keep the
original
> commits" as you say above and another where we enforce linear history.
> The linear history project is *much* easier to follow.
FWIW, I can't say I've had any real difficulty following non-linear
history. I think that's mainly because even non-linear history has a linear
history in it (the first-parent one that that article is trying to protect).
Then in addition to the linear first-parent history you have more detailed
linear histories on the side if you want to see them. It's certainly
possible to get into a mess if your pull requests contain merge commits though.
> With merge
> commits you start running into issues like foxtrot commits
> (https://blog.developer.atlassian.com/stop-foxtrots-now) and the whole
> thing easily becomes a mess.  There may be various rules/plugins/etc.
> that attempt to enforce "good" behavior but in the long run my
> experience has been that simpler rules make things simpler.
> 
>                     -David
The rule you need to avoid those kinds of mess is essentially the same one as
the one we currently have for pushing directly to master: No merge commits in
the pull request.
Or to put it another way that accounts for push to master and pull requests:
Humans aren't allowed to make merge commits.

From that article:
> You could disable direct pushes to master, and hope that pull-requests
never merge with fast-forward.
That's actually how our downstream repo works except we don't rely on
hope. The server is the one merging pull requests and it's doing the merge
with --no-ff. It's the equivalent of the 'Create merge commit'
option in GitHub.

Based on reading GitHub's documentation
(https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/merging-a-pull-request#merging-a-pull-request-on-github
<https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/merging-a-pull-request#merging-a-pull-request-on-github>)
all three of the options for merging pull requests will preserve a good
first-parent linear history. GitHub doesn't ban pushing to master of course
and we shouldn't do that as it will make pull-requests mandatory.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20191108/4ddbfca5/attachment.html>

I'm not sure the idea that enabling pull requests will make it easier
for new contributors is fully thought out. Just because more people
might be familiar with GitHub, doesn't mean it is superior. I've found
the workflow on Phabricator to be really easy. I think many people
agree that Phabricator is really good, I don't think we would want to
get rid of Phabricator and while its still an option it will be
difficult for GitHub PR's to get a lot of attention. The barrier of
creating an account at reviews.llvm.org is very small, you can even
use your GitHub account so really the barrier is the same. I think
people who submit patches on GitHub are going to have a much harder
time finding reviewers than if they were to submit them on
Phabricator. Getting reviewers is a much bigger barrier than making an
account and uploading a diff, and that will only be harder than if we
only allowed submitting patches through Phabricator.

Cheers,
Alex

On Fri, Nov 8, 2019 at 2:25 PM Daniel Sanders via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
>
>
>
> On Nov 8, 2019, at 08:28, David Greene <greened at obbligato.org>
wrote:
>
> Daniel Sanders via llvm-dev <llvm-dev at lists.llvm.org> writes:
>
> Personally, I'd like us to drop the linear history requirement but I
> know there's some strong opposition there. The main reason I'd like
us
> to accept non-linear history in conjunction with pull requests is that
> it means that the commits that were tested (whether by the author or
> by CI) still exist in the repository after the merge. If we
> rebase+push like we do today or squash+merge as in this proposal, then
> the information that a particular commit passed testing (or failed a
> particular test) gets lost.
>
>
> -1 from me.  We have one project where we decided to "keep the
original
> commits" as you say above and another where we enforce linear history.
> The linear history project is *much* easier to follow.
>
>
> FWIW, I can't say I've had any real difficulty following non-linear
history. I think that's mainly because even non-linear history has a linear
history in it (the first-parent one that that article is trying to protect).
Then in addition to the linear first-parent history you have more detailed
linear histories on the side if you want to see them. It's certainly
possible to get into a mess if your pull requests contain merge commits though.
>
> With merge
> commits you start running into issues like foxtrot commits
> (https://blog.developer.atlassian.com/stop-foxtrots-now) and the whole
> thing easily becomes a mess.  There may be various rules/plugins/etc.
> that attempt to enforce "good" behavior but in the long run my
> experience has been that simpler rules make things simpler.
>
>                     -David
>
>
> The rule you need to avoid those kinds of mess is essentially the same one
as the one we currently have for pushing directly to master: No merge commits in
the pull request.
> Or to put it another way that accounts for push to master and pull
requests: Humans aren't allowed to make merge commits.
>
> From that article:
> > You could disable direct pushes to master, and hope that pull-requests
never merge with fast-forward.
>
> That's actually how our downstream repo works except we don't rely
on hope. The server is the one merging pull requests and it's doing the
merge with --no-ff. It's the equivalent of the 'Create merge commit'
option in GitHub.
>
> Based on reading GitHub's documentation
(https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/merging-a-pull-request#merging-a-pull-request-on-github)
all three of the options for merging pull requests will preserve a good
first-parent linear history. GitHub doesn't ban pushing to master of course
and we shouldn't do that as it will make pull-requests mandatory.
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev