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

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

In general I mildly favor moving to GitHub PRs, mainly because it is
a de-facto standard in the industry and many people are already
familiar with it.  LLVM's idiosyncratic and essentially undocumented
tool is not, whatever minor advantages it might have in certain
technical respects.

David Tellenbach wrote:
> One thing I always liked at the Phabricator workflow is that it is
> based on diffs only.
And other people on the thread find this an annoyance.

Being unable to chain related reviews is about the only case where
GitHub would appear to be at a significant disadvantage, and offhand
it would not seem like an insurmountable technical obstacle if we can
persuade the GitHub folks that it's actually pretty useful.


Alex Brachet-Mialot wrote:
> Just because more people might be familiar with GitHub, doesn't mean
> it is superior.
"The perfect is the enemy of the good."  Automobile controls are 
largely standardized; you might design a superior set of controls, 
but the simple fact of being different from all the others would be 
a barrier to adoption.  Even if GitHub's workflow is flawed, it is 
what new contributors will be used to, and new contributors are the
lifeblood of the project. And as a major project hosted on GitHub, 
it is not impossible that we could influence GitHub to smooth it out 
to better suit our use-cases.

Or, check out Hal Finkel's suggestion, which would allow casual
contributors to at least start out with their accustomed workflow
and then be funneled into LLVM's idiosyncratic review tool, which 
is fundamentally user-hostile but our community has gotten used to.
> I think many people agree that Phabricator is really good,
And many people agree that Phabricator is a major pain.  It has
zero useful documentation (other than what the LLVM project has
written for its own use) and IME makes every effort to make useful
features difficult to find and use.  Ingenious people still manage
to find and use them, but Phab absolutely does not make it easy.
> Getting reviewers [on GitHub] is a much bigger barrier
This would depend on how new reviews are advertised.  With Phab, we
understand how they are advertised (i.e., mailing list).  With GitHub
this is apparently not automatic, but still automatable, IIUC.
--paulr

-1 -- switch to PR's
+1 -- Hal's compromise proposal (as long as I can continue to use Phab)

I agree that the documentation could be better, but I don't see that as a
justification for switching from a superior tool to an inferior one.  Let's
work on the documentation first, then if there's still a compelling reason
to switch, do it then,

I share most of the concerns already mentioned, but my biggest problem on a
daily basis is the use of force pushes to PR's, which loose information (no
way to tell what changed from the previous version -- I deal with this
everyday, and have to re-review the entire patch because I can't see what
was changed/fixed) . Since the branch is in someone's personal fork, I
don't see how it could be outlawed, and that's a problem and huge time
sync.

However, I do agree that the patches should be squashed into a single
commit when merged to master.

Btw, I've read several posts claiming github is willing to work with us to
improve the process.  Perhaps we could get them to adopt Phab.

thanks...
don

On Tue, Nov 12, 2019 at 9:52 AM Robinson, Paul via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> In general I mildly favor moving to GitHub PRs, mainly because it is
> a de-facto standard in the industry and many people are already
> familiar with it.  LLVM's idiosyncratic and essentially undocumented
> tool is not, whatever minor advantages it might have in certain
> technical respects.
>
> David Tellenbach wrote:
>
> > One thing I always liked at the Phabricator workflow is that it is
> > based on diffs only.
>
> And other people on the thread find this an annoyance.
>
> Being unable to chain related reviews is about the only case where
> GitHub would appear to be at a significant disadvantage, and offhand
> it would not seem like an insurmountable technical obstacle if we can
> persuade the GitHub folks that it's actually pretty useful.
>
>
> Alex Brachet-Mialot wrote:
>
> > Just because more people might be familiar with GitHub, doesn't
mean
> > it is superior.
>
> "The perfect is the enemy of the good."  Automobile controls are
> largely standardized; you might design a superior set of controls,
> but the simple fact of being different from all the others would be
> a barrier to adoption.  Even if GitHub's workflow is flawed, it is
> what new contributors will be used to, and new contributors are the
> lifeblood of the project. And as a major project hosted on GitHub,
> it is not impossible that we could influence GitHub to smooth it out
> to better suit our use-cases.
>
> Or, check out Hal Finkel's suggestion, which would allow casual
> contributors to at least start out with their accustomed workflow
> and then be funneled into LLVM's idiosyncratic review tool, which
> is fundamentally user-hostile but our community has gotten used to.
>
> > I think many people agree that Phabricator is really good,
>
> And many people agree that Phabricator is a major pain.  It has
> zero useful documentation (other than what the LLVM project has
> written for its own use) and IME makes every effort to make useful
> features difficult to find and use.  Ingenious people still manage
> to find and use them, but Phab absolutely does not make it easy.
>
> > Getting reviewers [on GitHub] is a much bigger barrier
>
> This would depend on how new reviews are advertised.  With Phab, we
> understand how they are advertised (i.e., mailing list).  With GitHub
> this is apparently not automatic, but still automatable, IIUC.
> --paulr
>
> _______________________________________________
> 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/20191112/0246abe5/attachment.html>