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

-1 from me as well. GitHub UI feels extremely sluggish with large
projects, forks feel like needless hurdles to jump over,
especially for small patches - having to fork the entire monorepo,
just to do a GitHub PR would be extremely cumbersome.

Just recently I submitted a very tiny PR for PcapPlusPlus on GitHub
which involved:
1). Forking the repository
2). Cloning the fork
3). Applying the patch
4). Pushing it and creating a PR.
5). Deleting the fork (Since I didn't want it cluttering up my account
and I'm not a regular contributor to PcapPlusPlus).

Neither the pull request mechanism nor GitHub issues feel like they're
suitable for large projects since it's common to see large GitHub
projects with ignored issues and worse filtering/categorizing. And PRs
as a review mechanism are far worse than Phabricator or any other
dedicated code review software, and I think a lot of GitHub projects
use internal or separate code review tools as-is, with PRs being
second class citizens and are more prone to getting derailed. It's not
uncommon for GitHub issue or PR discussions to turn into massive
threads that quickly turn into bikeshedding and off-topic conversations.

It also feels like this will inevitably force people to use external
tools to handle it, instead of just being able to use Git on its
own (something like `git cl` comes to mind, which some may like and
some may not).

And Roman raised a lot of good points regarding other, more
bureaucratic aspects of GitHub and it raising the bar for existing
contributors.

Just my two cents in.

On Thu, Nov 7, 2019 at 8:09 AM Roman Lebedev via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
>
> Strong -1 personally.
>
> * What is the endgoal? To fully kill phab and move to github pullrequests?
>   it might be best to discuss *that* first. (did i miss an RFC?)
> * Separation of attention - does everyone who cares
>   now has to also look at pull requests for reviews;
>   or should they be exempt from general review attention?
> * For me personally after using phabricator, github now seems
>   extremely crude, laggy, limited. To name a few:
>   * There is no way to see previous version of the patch.
>     I don't think there is any way to disable force-push for PR's.
>     While this is only 'slightly' limiting for the reviewer,
>     this can be more limiting for the author - how do i go back
>     to previous revision? I can't, i need to maintain a copy
>     of every branch i pushed manually.
>   * Impossible to chain reviews - a PR diff can only be made
>     on top of git master branch. Any non-trivial change consists of
>     separable PR's. Now either one will only be able to submit
>     dependent PR after the prereqs are merged, or the diff will be
>     impossible to read.
>   * Does not load large diffs by default.
>     That caught me by surprise several times
>     when i was searching for something.
>   * No diffs in mail - *super* inconvenient.
>     One has to open each pr in browser (or fetch via git etc etc)
>   * Github is an official US-based commercial organisation.
>     It *has* to follow U.S. export law. In particular i'm thinking of
>       https://techcrunch.com/2019/07/29/github-ban-sanctioned-countries/
>       https://github.com/tkashkin/GameHub/issues/289
>     Does phabricator already have such restrictions, blocks?
>     If not, wouldn't you say adding such restrictions is not being
>     more open for contributors?
>     What happens when, in not so long in the future, the entirety of, say,
>     china or russian federation is blocked as such?
>   * Same question can be asked about internet "iron" curtains
>     certain (*cough*) countries are raising. That also has already happened
>     before (and *will* happen again), and i was personally affected:
>       https://en.wikipedia.org/wiki/Censorship_of_GitHub#Russia
>     I don't recall that happening to phabricator yet.
>     I fail to see how that is more contributor-friendly.
>   * Not sure anyone cares, but while using github as main git
>     repository "mirror" is perfectly fine - git is distributed,
only canonical
>     write-repo would be affected anything bad happen. But that isn't
the case
>     for reviews, issues; as it has been discussed in the "let's
migrate bugzilla
>     to github issues", it is far more involved.
>   * What about DMCA? Not sure how this is currently handled.
>   * UI feels laggy. Not much to add here, pretty subjective.
>   * I'm sure i'm missing a few.
>
> The github does come with it's benefits, sure:
> * It is *simpler* to preserve git commit author.
>   Currently one has to ask the author for the "Author: e at
ma.il" line,
>   and do `git commit --amend --author="<>"`.
> * @mention is wide-r-reaching - whole github, not just llvm phabricator
> * No more "phabricator disk full" issues
> * ???
>
> TLDR: such migration lowers the bar for new, first time,
> unestabilished contributors, but i personally feel it *significantly*
> raises the bar for the existing contributors, reviewers.
> We don't live in perfect world. Aspirational goals are aspirational.
> They should be attempted to be reached, but they shouldn't shadow and
> overweight, take over the main goal of the LLVM project.
>
> Personally, i don't see that benefits out-/over- weight the drawbacks.
>
> Roman.
>
> On Thu, Nov 7, 2019 at 8:32 AM Mehdi AMINI via llvm-dev
> <llvm-dev at lists.llvm.org> wrote:
> >
> > Hi all,
> >
> > Now that we're on GitHub, we can discuss about pull-requests.
> > I'd like to propose to enable pull-request on GitHub, as a first
step as an experimental channel alongside the existing methods for contributing
to LLVM.
> > This would allow to find workflow issues and address them, and also
LLVM contributors in general to start getting familiar with pull-requests
without committing to switching to pull-requests immediately. The community
should evaluate after a few months what would the next steps be.
> >
> > GitHub pull-requests is the natural way to contribute to project
hosted on GitHub: this feature is so core to GitHub that there is no option to
disable it!
> >
> > The current proposal is to allow to integrate contributions to the
LLVM project directly from pull-requests. In particular the exact setup would be
the following:
> >
> >   - Contributors should use their own fork and push a branch in their
fork.
> >   - Reviews can be performed on GitHub. The canonical tools are still
the mailing-list and Phabricator: a reviewer can request the review to move to
Phabricator.
> >   - The only option available will be to “squash and merge”. This mode
of review matches the closest our current workflow (both phabricator and
mailing-list): conceptually independent contributions belongs to separate review
threads, and thus separate pull-requests.
> > This also allow the round of reviews to not force-push the original
branch and accumulate commits: this keeps the contextual history of comments and
allow to visualize the incremental diff across revision of the pull-request.
> >   - Upon “merge” of a pull-request: history is linear and a single
commit lands in master after review is completed.
> >
> > As an alternative staging proposal: we could enable pull-requests only
for a small subset of sub-projects in LLVM (i.e. not LLVM/clang to begin with
for example) in the repo. In this case, we would propose to begin with the MLIR
project (as soon as it gets integrated in the monorepo). This would be a good
candidate to be the guinea pig for this process since it does not yet have a
wide established community of contributors, and the current contributors are
already exclusively using pull-requests.
> >
> > Here is a more complete doc on the topic:
https://docs.google.com/document/d/1DSHQrfydSjoqU9zEnj3rIcds6YN59Jxc37MdiggOyaI
> >
> > Cheers,
> >
> > --
> > Mehdi
> >
> > _______________________________________________
> > LLVM Developers mailing list
> > llvm-dev at lists.llvm.org
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

On Thu, Nov 7, 2019 at 1:06 AM Kristina Brooks via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> -1 from me as well. GitHub UI feels extremely sluggish with large
> projects, forks feel like needless hurdles to jump over,
> especially for small patches - having to fork the entire monorepo,
> just to do a GitHub PR would be extremely cumbersome.
>
> Just recently I submitted a very tiny PR for PcapPlusPlus on GitHub
> which involved:
> 1). Forking the repository
> 2). Cloning the fork
> 3). Applying the patch
> 4). Pushing it and creating a PR.
> 5). Deleting the fork (Since I didn't want it cluttering up my account
> and I'm not a regular contributor to PcapPlusPlus).
>
I am not sure I understand why you went through all these steps. If you had
a patch in the first place, you likely had already a local clone of the
repo? At this point you need to create a fork indeed (either through a
click on the canonical repo or using GitHub command line utility), and then
you can just push your current HEAD to your fork (after adding it locally
as a remote). This kind of manipulation can be strange coming from SVN but
are extremely common working on git/GitHub. Assuming you have a clone of
the PcapPlusPlus project locally this is just a matter of:

$ git remote add fork git at github.com:christinaa/PcapPlusPlus.git
$ git checkout -b my_branch
$ git push fork my_branch

Iterating on the review can be done by just committing more into the branch
and pushing, the PR is automatically updated.


>
> Neither the pull request mechanism nor GitHub issues feel like they're
> suitable for large projects since it's common to see large GitHub
> projects with ignored issues and worse filtering/categorizing. And PRs
> as a review mechanism are far worse than Phabricator or any other
> dedicated code review software, and I think a lot of GitHub projects
> use internal or separate code review tools as-is, with PRs being
> second class citizens and are more prone to getting derailed. It's not
> uncommon for GitHub issue or PR discussions to turn into massive
> threads that quickly turn into bikeshedding and off-topic conversations.
>
> It also feels like this will inevitably force people to use external
> tools to handle it, instead of just being able to use Git on its
> own (something like `git cl` comes to mind, which some may like and
> some may not).
>
Actually my experience is radically the opposite: Phabricator is the one
that does not allow to "just being able to use Git on its
own", you're either stuck with manually exporting .patch files and
manually
add them to the UI or use a php-based CLI indeed (which is the point you
seem to be against).

Best,

-- 
Mehdi



>
> And Roman raised a lot of good points regarding other, more
> bureaucratic aspects of GitHub and it raising the bar for existing
> contributors.
>
> Just my two cents in.
>
> On Thu, Nov 7, 2019 at 8:09 AM Roman Lebedev via llvm-dev
> <llvm-dev at lists.llvm.org> wrote:
> >
> > Strong -1 personally.
> >
> > * What is the endgoal? To fully kill phab and move to github
> pullrequests?
> >   it might be best to discuss *that* first. (did i miss an RFC?)
> > * Separation of attention - does everyone who cares
> >   now has to also look at pull requests for reviews;
> >   or should they be exempt from general review attention?
> > * For me personally after using phabricator, github now seems
> >   extremely crude, laggy, limited. To name a few:
> >   * There is no way to see previous version of the patch.
> >     I don't think there is any way to disable force-push for
PR's.
> >     While this is only 'slightly' limiting for the reviewer,
> >     this can be more limiting for the author - how do i go back
> >     to previous revision? I can't, i need to maintain a copy
> >     of every branch i pushed manually.
> >   * Impossible to chain reviews - a PR diff can only be made
> >     on top of git master branch. Any non-trivial change consists of
> >     separable PR's. Now either one will only be able to submit
> >     dependent PR after the prereqs are merged, or the diff will be
> >     impossible to read.
> >   * Does not load large diffs by default.
> >     That caught me by surprise several times
> >     when i was searching for something.
> >   * No diffs in mail - *super* inconvenient.
> >     One has to open each pr in browser (or fetch via git etc etc)
> >   * Github is an official US-based commercial organisation.
> >     It *has* to follow U.S. export law. In particular i'm thinking
of
> >      
https://techcrunch.com/2019/07/29/github-ban-sanctioned-countries/
> >       https://github.com/tkashkin/GameHub/issues/289
> >     Does phabricator already have such restrictions, blocks?
> >     If not, wouldn't you say adding such restrictions is not being
> >     more open for contributors?
> >     What happens when, in not so long in the future, the entirety of,
> say,
> >     china or russian federation is blocked as such?
> >   * Same question can be asked about internet "iron"
curtains
> >     certain (*cough*) countries are raising. That also has already
> happened
> >     before (and *will* happen again), and i was personally affected:
> >       https://en.wikipedia.org/wiki/Censorship_of_GitHub#Russia
> >     I don't recall that happening to phabricator yet.
> >     I fail to see how that is more contributor-friendly.
> >   * Not sure anyone cares, but while using github as main git
> >     repository "mirror" is perfectly fine - git is
distributed, only
> canonical
> >     write-repo would be affected anything bad happen. But that
isn't the
> case
> >     for reviews, issues; as it has been discussed in the
"let's migrate
> bugzilla
> >     to github issues", it is far more involved.
> >   * What about DMCA? Not sure how this is currently handled.
> >   * UI feels laggy. Not much to add here, pretty subjective.
> >   * I'm sure i'm missing a few.
> >
> > The github does come with it's benefits, sure:
> > * It is *simpler* to preserve git commit author.
> >   Currently one has to ask the author for the "Author: e at
ma.il" line,
> >   and do `git commit --amend --author="<>"`.
> > * @mention is wide-r-reaching - whole github, not just llvm
phabricator
> > * No more "phabricator disk full" issues
> > * ???
> >
> > TLDR: such migration lowers the bar for new, first time,
> > unestabilished contributors, but i personally feel it *significantly*
> > raises the bar for the existing contributors, reviewers.
> > We don't live in perfect world. Aspirational goals are
aspirational.
> > They should be attempted to be reached, but they shouldn't shadow
and
> > overweight, take over the main goal of the LLVM project.
> >
> > Personally, i don't see that benefits out-/over- weight the
drawbacks.
> >
> > Roman.
> >
> > On Thu, Nov 7, 2019 at 8:32 AM Mehdi AMINI via llvm-dev
> > <llvm-dev at lists.llvm.org> wrote:
> > >
> > > Hi all,
> > >
> > > Now that we're on GitHub, we can discuss about pull-requests.
> > > I'd like to propose to enable pull-request on GitHub, as a
first step
> as an experimental channel alongside the existing methods for contributing
> to LLVM.
> > > This would allow to find workflow issues and address them, and
also
> LLVM contributors in general to start getting familiar with pull-requests
> without committing to switching to pull-requests immediately. The community
> should evaluate after a few months what would the next steps be.
> > >
> > > GitHub pull-requests is the natural way to contribute to project
> hosted on GitHub: this feature is so core to GitHub that there is no option
> to disable it!
> > >
> > > The current proposal is to allow to integrate contributions to
the
> LLVM project directly from pull-requests. In particular the exact setup
> would be the following:
> > >
> > >   - Contributors should use their own fork and push a branch in
their
> fork.
> > >   - Reviews can be performed on GitHub. The canonical tools are
still
> the mailing-list and Phabricator: a reviewer can request the review to move
> to Phabricator.
> > >   - The only option available will be to “squash and merge”. This
mode
> of review matches the closest our current workflow (both phabricator and
> mailing-list): conceptually independent contributions belongs to separate
> review threads, and thus separate pull-requests.
> > > This also allow the round of reviews to not force-push the
original
> branch and accumulate commits: this keeps the contextual history of
> comments and allow to visualize the incremental diff across revision of the
> pull-request.
> > >   - Upon “merge” of a pull-request: history is linear and a
single
> commit lands in master after review is completed.
> > >
> > > As an alternative staging proposal: we could enable pull-requests
only
> for a small subset of sub-projects in LLVM (i.e. not LLVM/clang to begin
> with for example) in the repo. In this case, we would propose to begin with
> the MLIR project (as soon as it gets integrated in the monorepo). This
> would be a good candidate to be the guinea pig for this process since it
> does not yet have a wide established community of contributors, and the
> current contributors are already exclusively using pull-requests.
> > >
> > > Here is a more complete doc on the topic:
>
https://docs.google.com/document/d/1DSHQrfydSjoqU9zEnj3rIcds6YN59Jxc37MdiggOyaI
> > >
> > > Cheers,
> > >
> > > --
> > > Mehdi
> > >
> > > _______________________________________________
> > > LLVM Developers mailing list
> > > llvm-dev at lists.llvm.org
> > > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> > _______________________________________________
> > LLVM Developers mailing list
> > llvm-dev at lists.llvm.org
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> _______________________________________________
> 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/20191107/e35c4c55/attachment-0001.html>

Mehdi AMINI via llvm-dev <llvm-dev at lists.llvm.org> writes:
>> It also feels like this will inevitably force people to use external
>> tools to handle it, instead of just being able to use Git on its
>> own (something like `git cl` comes to mind, which some may like and
>> some may not).
>>
>
> Actually my experience is radically the opposite: Phabricator is the one
> that does not allow to "just being able to use Git on its
> own", you're either stuck with manually exporting .patch files and
manually
> add them to the UI or use a php-based CLI indeed (which is the point you
> seem to be against).
+1.  Phab still seems alien to me and I've been using it for over a
year now.

                     -David