[llvm-dev] Enable Contributions Through Pull-request For LLVM

[Alex Brachet-Mialot via llvm-dev]

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