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

On Thu, Nov 7, 2019 at 3:09 AM Roman Lebedev via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
>
> Strong -1 personally.
Likewise, for many of the same reasons detailed below.

~Aaron
> * 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

Having been using Github internally for code reviews of private patches on
LLVM, and Phabricator for upstream ones, I've found the latter to be far
easier to use. Prior to working with LLVM, I had basically no experience
with either, so I'd say I'm coming from a fairly neutral starting point.
Here are some of my observations (I want to highlight 9 as a particular
issue, due to other recent discussions I've seen on the mailing list):

1) I don't know of a natural way to chain related patches together on
Github (aside from explicitly mentioning them), but as separate reviews.
This is useful for bigger features/refactorings where each individual step
provides some benefit, but seeing the bigger picture is easy. Phabricator
has the child/parent objects option.
2) Phabricator allows placing comments anywhere in the diff context, which
is useful when the commit affects lines that haven't actually been changed,
and those lines need addressing/referencing in some way. Github doesn't
allow comments outside the immediate context around changed files
3) As a +1 to Github on the other hand, commits always come with full
context, so you don't have to remind people to include it.
4) Phabricator's ability to see what has changed since the previous time
you commented seems to be much more reliable than what Github provides.
5) With a Github PR, if you want to include a minor change to the patch
prior to committing, you have to commit it to the PR, if you wish to use
the UI to do the merging. Of course, you could just not push the patch via
Github.
6) I myself have on numerous occasions messed up my commit message when
committing via the Github UI, because it's not obvious unless you are a
seasoned user that the title of the commit appears as the first line of the
commit message. This is important when doing a squash and merge.
7) The PR approach does at least allow committing via the UI, which is
perhaps a little less fiddly in some cases.
8) I rarely bother creating a branch for Phabricator reviews, because I
don't need it (I'm often not working on multiple things at once), so the
extra hassle of creating a branch/checking it out etc that Github requires
is annoying. On the other hand, Github PRs are generally quite easy to
create once you have pushed a branch.
9) On the note of branches for PRs, don't they require users to push their
local branches to the remote repo to create? That means we'll end up
thousands of branches in git. Not sure that this will do performance any
good, and I seem to remember there was general agreement that we didn't
want people to push their branches generally. Yes, in theory branches
should be deleted after they're merged, but I've seen that locally not
happen regularly, and that's even assuming that all PRs get merged in (they
won't).
10) Expanding context on Github is a pain: there is no option to just
expand the whole context in a block, which means that if you need to see
something much earlier in a large file, you have to click a LOT. Also, the
browser view often doesn't then go where you expect it to from my
experience. Phabricator has a "expand all N lines" option.
11) Small one this one, but missing new lines at end of file are much more
obvious on Phabricator than Github.
12) Not sure if this is a real issue, but Github reviewers are limited in
number (I think it's 15?). To my knowledge, there is no such limit with
Phabricator (but then how often do you end up with 15 people marked as
reviewers?).

I'm sure I could come up with other points for/against Github PRs. On
balance I definitely prefer Phabricator.

James

On Thu, 7 Nov 2019 at 09:53, Aaron Ballman via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> On Thu, Nov 7, 2019 at 3:09 AM Roman Lebedev via llvm-dev
> <llvm-dev at lists.llvm.org> wrote:
> >
> > Strong -1 personally.
>
> Likewise, for many of the same reasons detailed below.
>
> ~Aaron
>
> > * 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/74546924/attachment.html>

I know from experience that many people have not filed issues due to
bugzilla, and not submitted PRs due to phabricator.

An example of where a completely new contributor (my colleague Aras who
contributed the build-cost stats to Clang for 9.0.0) intuitively used
GitHub is here https://github.com/aras-p/llvm-project-20170507/pull/2 -
which I think really awesomely shows how quick a newcomer to the ecosystem
could do something really useful and productive when they are channeled
through tools they are already used to (GitHub issues/PRs).

So from us it's a big +1.

On Thu, Nov 7, 2019 at 10:31 AM James Henderson via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> Having been using Github internally for code reviews of private patches on
> LLVM, and Phabricator for upstream ones, I've found the latter to be
far
> easier to use. Prior to working with LLVM, I had basically no experience
> with either, so I'd say I'm coming from a fairly neutral starting
point.
> Here are some of my observations (I want to highlight 9 as a particular
> issue, due to other recent discussions I've seen on the mailing list):
>
> 1) I don't know of a natural way to chain related patches together on
> Github (aside from explicitly mentioning them), but as separate reviews.
> This is useful for bigger features/refactorings where each individual step
> provides some benefit, but seeing the bigger picture is easy. Phabricator
> has the child/parent objects option.
> 2) Phabricator allows placing comments anywhere in the diff context, which
> is useful when the commit affects lines that haven't actually been
changed,
> and those lines need addressing/referencing in some way. Github doesn't
> allow comments outside the immediate context around changed files
> 3) As a +1 to Github on the other hand, commits always come with full
> context, so you don't have to remind people to include it.
> 4) Phabricator's ability to see what has changed since the previous
time
> you commented seems to be much more reliable than what Github provides.
> 5) With a Github PR, if you want to include a minor change to the patch
> prior to committing, you have to commit it to the PR, if you wish to use
> the UI to do the merging. Of course, you could just not push the patch via
> Github.
> 6) I myself have on numerous occasions messed up my commit message when
> committing via the Github UI, because it's not obvious unless you are a
> seasoned user that the title of the commit appears as the first line of the
> commit message. This is important when doing a squash and merge.
> 7) The PR approach does at least allow committing via the UI, which is
> perhaps a little less fiddly in some cases.
> 8) I rarely bother creating a branch for Phabricator reviews, because I
> don't need it (I'm often not working on multiple things at once),
so the
> extra hassle of creating a branch/checking it out etc that Github requires
> is annoying. On the other hand, Github PRs are generally quite easy to
> create once you have pushed a branch.
> 9) On the note of branches for PRs, don't they require users to push
their
> local branches to the remote repo to create? That means we'll end up
> thousands of branches in git. Not sure that this will do performance any
> good, and I seem to remember there was general agreement that we didn't
> want people to push their branches generally. Yes, in theory branches
> should be deleted after they're merged, but I've seen that locally
not
> happen regularly, and that's even assuming that all PRs get merged in
(they
> won't).
> 10) Expanding context on Github is a pain: there is no option to just
> expand the whole context in a block, which means that if you need to see
> something much earlier in a large file, you have to click a LOT. Also, the
> browser view often doesn't then go where you expect it to from my
> experience. Phabricator has a "expand all N lines" option.
> 11) Small one this one, but missing new lines at end of file are much more
> obvious on Phabricator than Github.
> 12) Not sure if this is a real issue, but Github reviewers are limited in
> number (I think it's 15?). To my knowledge, there is no such limit with
> Phabricator (but then how often do you end up with 15 people marked as
> reviewers?).
>
> I'm sure I could come up with other points for/against Github PRs. On
> balance I definitely prefer Phabricator.
>
> James
>
> On Thu, 7 Nov 2019 at 09:53, Aaron Ballman via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
>> On Thu, Nov 7, 2019 at 3:09 AM Roman Lebedev via llvm-dev
>> <llvm-dev at lists.llvm.org> wrote:
>> >
>> > Strong -1 personally.
>>
>> Likewise, for many of the same reasons detailed below.
>>
>> ~Aaron
>>
>> > * 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
>>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>

-- 
Neil Henning
Senior Software Engineer Compiler
unity.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20191107/a2d1ba67/attachment.html>

I think that it's really important that we try to strike some balance here.
Based on my experience, this thread, and offline conversations, two things seem
clear to me:

 1. Overall, Phabricator is a superior tool for managing code reviews and some
related processes (although GitHub's tools certainly have some benefits, and
both are getting better over time).

 2. Not accepting GitHub PRs forms a barrier to entry for casual contributors,
and perhaps, causes workflow-integration inefficiencies for some others.

I'm also deeply concerned about having another place to search for
historical data, track conversations (including the ability to cross-link), and
so on.

I think that we should first explore the solution that Facebook uses or used,
see: https://github.com/facebook/hhvm/wiki/What-is-Phabricator

They had an import system that, when a PR was submitted, would import it into
Phabriactor and post a reply providing an explanation and a link to the imported
differential. This might be the right balance between ease of use for casual
contributors and the needs of the more-regular contributors providing the code
review (and, likely, those actually committing changes).

Other projects have worked on various kinds of integration schemes (e.g.,
https://github.com/framawiki/Github-notif-bot) that we should investigate. It
seems that the Phabricator developers are also working on some more-general API
which better supports this kind of use case (e.g.,
https://secure.phabricator.com/T12739), although the status seems unclear to me.

 -Hal

On 11/7/19 4:30 AM, James Henderson via llvm-dev wrote:
Having been using Github internally for code reviews of private patches on LLVM,
and Phabricator for upstream ones, I've found the latter to be far easier to
use. Prior to working with LLVM, I had basically no experience with either, so
I'd say I'm coming from a fairly neutral starting point. Here are some
of my observations (I want to highlight 9 as a particular issue, due to other
recent discussions I've seen on the mailing list):

1) I don't know of a natural way to chain related patches together on Github
(aside from explicitly mentioning them), but as separate reviews. This is useful
for bigger features/refactorings where each individual step provides some
benefit, but seeing the bigger picture is easy. Phabricator has the child/parent
objects option.
2) Phabricator allows placing comments anywhere in the diff context, which is
useful when the commit affects lines that haven't actually been changed, and
those lines need addressing/referencing in some way. Github doesn't allow
comments outside the immediate context around changed files
3) As a +1 to Github on the other hand, commits always come with full context,
so you don't have to remind people to include it.
4) Phabricator's ability to see what has changed since the previous time you
commented seems to be much more reliable than what Github provides.
5) With a Github PR, if you want to include a minor change to the patch prior to
committing, you have to commit it to the PR, if you wish to use the UI to do the
merging. Of course, you could just not push the patch via Github.
6) I myself have on numerous occasions messed up my commit message when
committing via the Github UI, because it's not obvious unless you are a
seasoned user that the title of the commit appears as the first line of the
commit message. This is important when doing a squash and merge.
7) The PR approach does at least allow committing via the UI, which is perhaps a
little less fiddly in some cases.
8) I rarely bother creating a branch for Phabricator reviews, because I
don't need it (I'm often not working on multiple things at once), so the
extra hassle of creating a branch/checking it out etc that Github requires is
annoying. On the other hand, Github PRs are generally quite easy to create once
you have pushed a branch.
9) On the note of branches for PRs, don't they require users to push their
local branches to the remote repo to create? That means we'll end up
thousands of branches in git. Not sure that this will do performance any good,
and I seem to remember there was general agreement that we didn't want
people to push their branches generally. Yes, in theory branches should be
deleted after they're merged, but I've seen that locally not happen
regularly, and that's even assuming that all PRs get merged in (they
won't).
10) Expanding context on Github is a pain: there is no option to just expand the
whole context in a block, which means that if you need to see something much
earlier in a large file, you have to click a LOT. Also, the browser view often
doesn't then go where you expect it to from my experience. Phabricator has a
"expand all N lines" option.
11) Small one this one, but missing new lines at end of file are much more
obvious on Phabricator than Github.
12) Not sure if this is a real issue, but Github reviewers are limited in number
(I think it's 15?). To my knowledge, there is no such limit with Phabricator
(but then how often do you end up with 15 people marked as reviewers?).

I'm sure I could come up with other points for/against Github PRs. On
balance I definitely prefer Phabricator.

James

On Thu, 7 Nov 2019 at 09:53, Aaron Ballman via llvm-dev <llvm-dev at
lists.llvm.org<mailto:llvm-dev at lists.llvm.org>> wrote:
On Thu, Nov 7, 2019 at 3:09 AM Roman Lebedev via llvm-dev
<llvm-dev at lists.llvm.org<mailto:llvm-dev at lists.llvm.org>>
wrote:
>
> Strong -1 personally.
Likewise, for many of the same reasons detailed below.

~Aaron
> * 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<mailto: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<mailto: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<mailto: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<mailto: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<mailto: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<mailto:llvm-dev at lists.llvm.org>
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev


--
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20191107/61b8e308/attachment-0001.html>

On Thu, Nov 7, 2019 at 2:30 AM James Henderson via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> Having been using Github internally for code reviews of private patches on
> LLVM, and Phabricator for upstream ones, I've found the latter to be
far
> easier to use. Prior to working with LLVM, I had basically no experience
> with either, so I'd say I'm coming from a fairly neutral starting
point.
> Here are some of my observations (I want to highlight 9 as a particular
> issue, due to other recent discussions I've seen on the mailing list):
>
> 1) I don't know of a natural way to chain related patches together on
> Github (aside from explicitly mentioning them), but as separate reviews.
> This is useful for bigger features/refactorings where each individual step
> provides some benefit, but seeing the bigger picture is easy. Phabricator
> has the child/parent objects option.
> 2) Phabricator allows placing comments anywhere in the diff context, which
> is useful when the commit affects lines that haven't actually been
changed,
> and those lines need addressing/referencing in some way. Github doesn't
> allow comments outside the immediate context around changed files
> 3) As a +1 to Github on the other hand, commits always come with full
> context, so you don't have to remind people to include it.
> 4) Phabricator's ability to see what has changed since the previous
time
> you commented seems to be much more reliable than what Github provides.
> 5) With a Github PR, if you want to include a minor change to the patch
> prior to committing, you have to commit it to the PR, if you wish to use
> the UI to do the merging. Of course, you could just not push the patch via
> Github.
> 6) I myself have on numerous occasions messed up my commit message when
> committing via the Github UI, because it's not obvious unless you are a
> seasoned user that the title of the commit appears as the first line of the
> commit message. This is important when doing a squash and merge.
> 7) The PR approach does at least allow committing via the UI, which is
> perhaps a little less fiddly in some cases.
> 8) I rarely bother creating a branch for Phabricator reviews, because I
> don't need it (I'm often not working on multiple things at once),
so the
> extra hassle of creating a branch/checking it out etc that Github requires
> is annoying. On the other hand, Github PRs are generally quite easy to
> create once you have pushed a branch.
> 9) On the note of branches for PRs, don't they require users to push
their
> local branches to the remote repo to create? That means we'll end up
> thousands of branches in git. Not sure that this will do performance any
> good, and I seem to remember there was general agreement that we didn't
> want people to push their branches generally. Yes, in theory branches
> should be deleted after they're merged, but I've seen that locally
not
> happen regularly, and that's even assuming that all PRs get merged in
(they
> won't).
> 10) Expanding context on Github is a pain: there is no option to just
> expand the whole context in a block, which means that if you need to see
> something much earlier in a large file, you have to click a LOT. Also, the
> browser view often doesn't then go where you expect it to from my
> experience. Phabricator has a "expand all N lines" option.
> 11) Small one this one, but missing new lines at end of file are much more
> obvious on Phabricator than Github.
> 12) Not sure if this is a real issue, but Github reviewers are limited in
> number (I think it's 15?). To my knowledge, there is no such limit with
> Phabricator (but then how often do you end up with 15 people marked as
> reviewers?).
>
> I'm sure I could come up with other points for/against Github PRs. On
> balance I definitely prefer Phabricator.
>
Thanks James, that is an excellent list!

Something that came up in a discussion with Swift folks about their choice
of not using GitHub issues originally is that over time GitHub issues added
the missing features and they regret now not being on GitHub issues. The
same may apply to pull-request though: I think there are some PMs inside
GitHub who try to add features to the roadmap to make GitHub better for
projects like LLVM. For instance until a few weeks before our move to
GitHub there wasn't a feature to preserve linear history and they added it
right before our move. So I suspect history can repeat as well here, I
don't know what are true blockers to try it, and how much we can expect
from GitHub here, but we should definitely engage with them and get them to
improve the "minus" you noticed against Phab (and other review
systems).

Best,

-- 
Mehdi



>
> James
>
> On Thu, 7 Nov 2019 at 09:53, Aaron Ballman via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
>> On Thu, Nov 7, 2019 at 3:09 AM Roman Lebedev via llvm-dev
>> <llvm-dev at lists.llvm.org> wrote:
>> >
>> > Strong -1 personally.
>>
>> Likewise, for many of the same reasons detailed below.
>>
>> ~Aaron
>>
>> > * 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
>>
> _______________________________________________
> 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/27b7e5ef/attachment.html>

Hi all,

from me also a big -1 due to the many reasons others have already mentioned
(especially Roman).

I want to give an additional consideration: One thing I always liked at the
Phabricator workflow is that it is based on diffs only. Pull requests are no git
but a Github feature. That means that by adopting Github's pull request
workflow you essentially adopt a proprietary feature that will only work on this
specific proprietary platform. Phabricator is essentially just a convenient tool
to review plain diffs (first class git citizens) and the fact that it works
relatively independent of the underlying repository is something I really like.

My -1 holds even more since I personally find the Phabricator workflow to be
very good in particular as it even got easier since arcanist works perfectly
fine with git and the complete arc workflow can be used since the migration. In
contrast to e.g. Bugzilla where we searched for an alternative since the current
situation was bad, this is not the case for Phabricator.

Regarding the hurdle for new contributors: Is the Github workflow really easier
or is it just the case that more people have Github accounts? As I've
already said I think the current workflow is absolutely fine and really
intuitive. We should however make our contribution guide more welcoming and
complete (I know, there are people working on this which is awesome!).

    David
> On 7. Nov 2019, at 10:53, Aaron Ballman via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> On Thu, Nov 7, 2019 at 3:09 AM Roman Lebedev via llvm-dev
> <llvm-dev at lists.llvm.org <mailto:llvm-dev at
lists.llvm.org>> wrote:
>> 
>> Strong -1 personally.
> 
> Likewise, for many of the same reasons detailed below.
> 
> ~Aaron
> 
>> * 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 <mailto:llvm-dev at lists.llvm.org>
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
<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/12ed8fa5/attachment.html>

James Henderson via llvm-dev <llvm-dev at lists.llvm.org> writes:
> 9) On the note of branches for PRs, don't they require users to push
their
> local branches to the remote repo to create? That means we'll end up
> thousands of branches in git. Not sure that this will do performance any
> good, and I seem to remember there was general agreement that we didn't
> want people to push their branches generally. Yes, in theory branches
> should be deleted after they're merged, but I've seen that locally
not
> happen regularly, and that's even assuming that all PRs get merged in
(they
> won't).
This is why Mehdi suggested using forks, I believe.  The branches only
live on the fork of the project.  The main canonical copy remains
branch-free.

                   -David

David Tellenbach via llvm-dev <llvm-dev at lists.llvm.org> writes:
> My -1 holds even more since I personally find the Phabricator workflow
> to be very good in particular as it even got easier since arcanist
> works perfectly fine with git and the complete arc workflow can be
> used since the migration. In contrast to e.g. Bugzilla where we
> searched for an alternative since the current situation was bad, this
> is not the case for Phabricator.
After days of trying, I gave up trying to get arcanist to work.  So it's
all manual diff + cut and paste into a browser for me.  That is
significantly worse than simply pushing to a PR.
> Regarding the hurdle for new contributors: Is the Github workflow
> really easier or is it just the case that more people have Github
> accounts?
They're related, right?  GitHub is easier because lots of people are
already familiar with it.

                    -David

> On 7 Nov 2019, at 6:30 pm, James Henderson via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> Having been using Github internally for code reviews of private patches on
LLVM, and Phabricator for upstream ones, I've found the latter to be far
easier to use. Prior to working with LLVM, I had basically no experience with
either, so I'd say I'm coming from a fairly neutral starting point. Here
are some of my observations (I want to highlight 9 as a particular issue, due to
other recent discussions I've seen on the mailing list):
> 9) On the note of branches for PRs, don't they require users to push
their local branches to the remote repo to create? That means we'll end up
thousands of branches in git. Not sure that this will do performance any good,
and I seem to remember there was general agreement that we didn't want
people to push their branches generally. Yes, in theory branches should be
deleted after they're merged, but I've seen that locally not happen
regularly, and that's even assuming that all PRs get merged in (they
won't).
You would only ever end up with thousands of branches if everyone push to a
branch on the LLVM’s “fork” of the monorepo. That still wouldn’t be a problem
performance wise because branch are very cheap by design, it might clog up the
branch selection UI a bit but not much more.

But from the perspective of outside contributors, which seems to be one of the
main reasons for this change, that is not generally the way things are done,
even on projects where you have commit access. What usually happens is you push
your local branch to your GitHub fork and issue the PR from there, in which case
branches will accumulate there. (GitHub will also invisibly mirror the branch on
the monorepo, but branches are cheap and it won’t show up)

Nic