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

I don't intend to weigh in on either side, but just give a perspective on a
few questions asked.
>From an outsiders perspective, that list isn't what I'd typically
describe
as the workflow, since it makes "fork" and "branch" sound
like difficult
operations. This sounds akin to thinking that someone would reclone the svn
repo before working on a new commit—possible, but rather extreme. The
distributed nature of git means that the fork is pretty simple for github
(essentially creates a new branch in their database, but that is just
defining a new name for an existing commit). I've noticed GitHub recently
made it easier to delete the fork, but I'd argue most users shouldn't
care.
What you'd typically do instead is to do `git remote add <name>
<url>`
and/or use their `hub` CLI wrapper (https://hub.github.com) to synchronize
work between multiple remote and local locations.
> 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.

Yes, but a branch is just a name for a particular list of commits. Unlike
svn, it is not the commits themselves, so they have almost no performance
implications. I don't think LLVM would want everyone pushing to their work
in progress to the central repo, so the "thousands of branches" would
be
scattered across each individual contributor's repo, and would represent an
likely inconsequential fraction of the number of branches already on GitHub.
> Impossible to chain reviews - a PR diff can only be made on top of git
master branch
It is possible to make a PR against someone else's PR on their fork,
although I hesitate to compare this to the far more usable phabricator
capability, so perhaps "impossible" is the right term anyways.
By contrast though, GitHub has no intrinsic concept of a "master"
branch.
Even aside from the question of using GitHub PRs, you can ask GitHub to
make a diff between any two (or three) points in time in a few ways, such
as you could also do locally with git. For example, this'll show you the
last two commits:
https://github.com/llvm/llvm-project/compare/HEAD^^...master
<https://github.com/llvm/llvm-project/compare/HEAD%5E...master> (two dots
for a direct diff, three for showing only changes since the branch point).
There's also some convenient additional tooling such as adding
".patch"
will download it as a raw patch file instead showing in the rich editor.
> There is no way to see previous version of the patch.
This is a fairly new feature, but the PR itself now lists these as events
(e.g. "user force-pushed from abcdefg to gfedcba", where each of those
are
hyperlinks) in the comment feed.
> how do i go back to previous revision
Answering a slightly different question in the hopes someone finds this
information useful: locally you can do `git reflog <branchname>` to get a
history of the state of the HEAD of any tag on that particular machine for
the last 90 days.


On Thu, Nov 7, 2019 at 8:10 AM Nemanja Ivanovic via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> I find Hal's analysis of this subject to be the most pragmatic one in
the
> thread and really want to chime in with a big +1 for the balanced approach.
>
> TL; DR;
> As it was outlined by previous comments in the thread, the GitHub PR
> review tool is inferior to Phabricator (and I would argue that this is an
> objective statement). Those that are only familiar with GitHub may not
> ascribe much value to capabilities such as commenting on unchanged lines,
> expanding whatever context you're interested in, seeing the diff of two
> revisions, etc. But for a good portion of the LLVM community, these things
> are important.
>
> I think a well integrated solution would not place undue burden on the
> author or the reviewer. Allowing pull requests seems to be desired from the
> perspective of making it easy for authors. Allowing the review to happen on
> Phabricator seems to be desired from the perspective of making it easy for
> the reviewers. So a solution that can do both is the best of both worlds.
>
> One comment on the initial proposal is that it seems like what is proposed
> may take away the supposed benefit of moving to PR's in the first place
-
> making it easy for the author. I fail to see how the workflow*:
> - Fork
> - Clone
> - Create branch and modify code
> - Push
> - Create PR
> - Squash+merge after approval
> - Delete the fork
>
> Is easier from the current:
> - Clone
> - Create branch and modify code
> - git diff -U99999
> - Post through web UI (or use arcanist for these two steps)
> - Apply the approved version and git push
>
> * There is a distinct possibility that I am misunderstanding how the new
> workflow would work and it would be easier than what I outlined above, but
> that's just the way I understand it.
>
> On Thu, Nov 7, 2019 at 6:42 AM Finkel, Hal J. via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
>> 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> 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 listllvm-dev at
lists.llvm.orghttps://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>
>> --
>> Hal Finkel
>> Lead, Compiler Technology and Programming Languages
>> Leadership Computing Facility
>> Argonne National Laboratory
>>
>> _______________________________________________
>> 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/ebfdbd01/attachment.html>

On 11/7/19 10:13 AM, Jameson Nash wrote:
I don't intend to weigh in on either side, but just give a perspective on a
few questions asked.

From an outsiders perspective, that list isn't what I'd typically
describe as the workflow, since it makes "fork" and "branch"
sound like difficult operations. This sounds akin to thinking that someone would
reclone the svn repo before working on a new commit—possible, but rather
extreme. The distributed nature of git means that the fork is pretty simple for
github (essentially creates a new branch in their database, but that is just
defining a new name for an existing commit).


FWIW, I've not found it to be so straightforward in practice. If you have
multiple LLVM forks, representing different distinct projects, because GitHub
only lets you have one fork per account, the process via which you can work with
multiple derived projects is actually quite annoying.

GitHub has a page on this
(https://github.community/t5/Support-Protips/Alternatives-to-forking-into-the-same-account/ba-p/7428),
but all of the work-arounds are pretty bad.

In short, only the first fork is easy. After that, it gets harder.

 -Hal


I've noticed GitHub recently made it easier to delete the fork, but I'd
argue most users shouldn't care. What you'd typically do instead is to
do `git remote add <name> <url>` and/or use their `hub` CLI wrapper
(https://hub.github.com) to synchronize work between multiple remote and local
locations.
> 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.
Yes, but a branch is just a name for a particular list of commits. Unlike svn,
it is not the commits themselves, so they have almost no performance
implications. I don't think LLVM would want everyone pushing to their work
in progress to the central repo, so the "thousands of branches" would
be scattered across each individual contributor's repo, and would represent
an likely inconsequential fraction of the number of branches already on GitHub.
> Impossible to chain reviews - a PR diff can only be made on top of git
master branch
It is possible to make a PR against someone else's PR on their fork,
although I hesitate to compare this to the far more usable phabricator
capability, so perhaps "impossible" is the right term anyways.
By contrast though, GitHub has no intrinsic concept of a "master"
branch. Even aside from the question of using GitHub PRs, you can ask GitHub to
make a diff between any two (or three) points in time in a few ways, such as you
could also do locally with git. For example, this'll show you the last two
commits:
https://github.com/llvm/llvm-project/compare/HEAD^^...master<https://github.com/llvm/llvm-project/compare/HEAD%5E...master>
(two dots for a direct diff, three for showing only changes since the branch
point). There's also some convenient additional tooling such as adding
".patch" will download it as a raw patch file instead showing in the
rich editor.
> There is no way to see previous version of the patch.
This is a fairly new feature, but the PR itself now lists these as events (e.g.
"user force-pushed from abcdefg to gfedcba", where each of those are
hyperlinks) in the comment feed.
> how do i go back to previous revision
Answering a slightly different question in the hopes someone finds this
information useful: locally you can do `git reflog <branchname>` to get a
history of the state of the HEAD of any tag on that particular machine for the
last 90 days.


On Thu, Nov 7, 2019 at 8:10 AM Nemanja Ivanovic via llvm-dev <llvm-dev at
lists.llvm.org<mailto:llvm-dev at lists.llvm.org>> wrote:
I find Hal's analysis of this subject to be the most pragmatic one in the
thread and really want to chime in with a big +1 for the balanced approach.

TL; DR;
As it was outlined by previous comments in the thread, the GitHub PR review tool
is inferior to Phabricator (and I would argue that this is an objective
statement). Those that are only familiar with GitHub may not ascribe much value
to capabilities such as commenting on unchanged lines, expanding whatever
context you're interested in, seeing the diff of two revisions, etc. But for
a good portion of the LLVM community, these things are important.

I think a well integrated solution would not place undue burden on the author or
the reviewer. Allowing pull requests seems to be desired from the perspective of
making it easy for authors. Allowing the review to happen on Phabricator seems
to be desired from the perspective of making it easy for the reviewers. So a
solution that can do both is the best of both worlds.

One comment on the initial proposal is that it seems like what is proposed may
take away the supposed benefit of moving to PR's in the first place - making
it easy for the author. I fail to see how the workflow*:
- Fork
- Clone
- Create branch and modify code
- Push
- Create PR
- Squash+merge after approval
- Delete the fork

Is easier from the current:
- Clone
- Create branch and modify code
- git diff -U99999
- Post through web UI (or use arcanist for these two steps)
- Apply the approved version and git push

* There is a distinct possibility that I am misunderstanding how the new
workflow would work and it would be easier than what I outlined above, but
that's just the way I understand it.

On Thu, Nov 7, 2019 at 6:42 AM Finkel, Hal J. via llvm-dev <llvm-dev at
lists.llvm.org<mailto:llvm-dev at lists.llvm.org>> wrote:

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

_______________________________________________
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/548ccbdf/attachment-0001.html>

I'm not sure I understand the need for that, although that's perhaps
just
my lack of experience with certain types of workflows. And seems somewhat
more relevant to the choice to switch to git than whether to use GitHub PR
workflow. As that page says, each fork supports an unlimited number of
branches, each of which is essentially equivalent to a full fork. So you
can have multiple clones locally each working from a different branch of
the same upstream fork. That's less true if you are using the issue tracker
or PR features, but that seems like it'd be a relatively small set of teams
that wanted those extra features.

What would this look like in Phabricator today? My impression has been that
the approximate equivalent was manually copying around patch files. Or
perhaps setting up a new svn remote copy?

Thinking back on my own transition from svn to git, perhaps one of the
points of confusion is in the way to use the respective tools. In svn, it's
usually necessary to do most work directly on `trunk`. In git, it's much
better to always create feature branches. The git tool won't stop you from
committing to the master branch directly (it's the default), but I'd say
that git will keep trying to make life hard for you if you do. Since git
will keep trying to synchronize the meaning of `master` across all remotes,
it can keep creating conflicts with work-in-progress or old work. I don't
know if there's any resources warning you not to make a GitHub PR from
master on your fork (or between any other two branches with the same name);
this is just something I have learned from experience to avoid.

On Thu, Nov 7, 2019 at 12:32 PM Finkel, Hal J. <hfinkel at anl.gov> wrote:
>
> On 11/7/19 10:13 AM, Jameson Nash wrote:
>
> I don't intend to weigh in on either side, but just give a perspective
on
> a few questions asked.
>
> From an outsiders perspective, that list isn't what I'd typically
describe
> as the workflow, since it makes "fork" and "branch"
sound like difficult
> operations. This sounds akin to thinking that someone would reclone the svn
> repo before working on a new commit—possible, but rather extreme. The
> distributed nature of git means that the fork is pretty simple for github
> (essentially creates a new branch in their database, but that is just
> defining a new name for an existing commit).
>
>
> FWIW, I've not found it to be so straightforward in practice. If you
have
> multiple LLVM forks, representing different distinct projects, because
> GitHub only lets you have one fork per account, the process via which you
> can work with multiple derived projects is actually quite annoying.
>
> GitHub has a page on this (
>
https://github.community/t5/Support-Protips/Alternatives-to-forking-into-the-same-account/ba-p/7428),
> but all of the work-arounds are pretty bad.
>
> In short, only the first fork is easy. After that, it gets harder.
>
>  -Hal
>
>
> I've noticed GitHub recently made it easier to delete the fork, but
I'd
> argue most users shouldn't care. What you'd typically do instead is
to do
> `git remote add <name> <url>` and/or use their `hub` CLI
wrapper (
> https://hub.github.com) to synchronize work between multiple remote and
> local locations.
>
> > 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.
>
> Yes, but a branch is just a name for a particular list of commits. Unlike
> svn, it is not the commits themselves, so they have almost no performance
> implications. I don't think LLVM would want everyone pushing to their
work
> in progress to the central repo, so the "thousands of branches"
would be
> scattered across each individual contributor's repo, and would
represent an
> likely inconsequential fraction of the number of branches already on
GitHub.
>
> > Impossible to chain reviews - a PR diff can only be made on top of git
> master branch
> It is possible to make a PR against someone else's PR on their fork,
> although I hesitate to compare this to the far more usable phabricator
> capability, so perhaps "impossible" is the right term anyways.
> By contrast though, GitHub has no intrinsic concept of a "master"
branch.
> Even aside from the question of using GitHub PRs, you can ask GitHub to
> make a diff between any two (or three) points in time in a few ways, such
> as you could also do locally with git. For example, this'll show you
the
> last two commits:
> https://github.com/llvm/llvm-project/compare/HEAD^^...master
> <https://github.com/llvm/llvm-project/compare/HEAD%5E...master> (two
dots
> for a direct diff, three for showing only changes since the branch point).
> There's also some convenient additional tooling such as adding
".patch"
> will download it as a raw patch file instead showing in the rich editor.
>
> > There is no way to see previous version of the patch.
> This is a fairly new feature, but the PR itself now lists these as events
> (e.g. "user force-pushed from abcdefg to gfedcba", where each of
those are
> hyperlinks) in the comment feed.
>
> > how do i go back to previous revision
> Answering a slightly different question in the hopes someone finds this
> information useful: locally you can do `git reflog <branchname>` to
get a
> history of the state of the HEAD of any tag on that particular machine for
> the last 90 days.
>
>
> On Thu, Nov 7, 2019 at 8:10 AM Nemanja Ivanovic via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
>> I find Hal's analysis of this subject to be the most pragmatic one
in the
>> thread and really want to chime in with a big +1 for the balanced
approach.
>>
>> TL; DR;
>> As it was outlined by previous comments in the thread, the GitHub PR
>> review tool is inferior to Phabricator (and I would argue that this is
an
>> objective statement). Those that are only familiar with GitHub may not
>> ascribe much value to capabilities such as commenting on unchanged
lines,
>> expanding whatever context you're interested in, seeing the diff of
two
>> revisions, etc. But for a good portion of the LLVM community, these
things
>> are important.
>>
>> I think a well integrated solution would not place undue burden on the
>> author or the reviewer. Allowing pull requests seems to be desired from
the
>> perspective of making it easy for authors. Allowing the review to
happen on
>> Phabricator seems to be desired from the perspective of making it easy
for
>> the reviewers. So a solution that can do both is the best of both
worlds.
>>
>> One comment on the initial proposal is that it seems like what is
>> proposed may take away the supposed benefit of moving to PR's in
the first
>> place - making it easy for the author. I fail to see how the workflow*:
>> - Fork
>> - Clone
>> - Create branch and modify code
>> - Push
>> - Create PR
>> - Squash+merge after approval
>> - Delete the fork
>>
>> Is easier from the current:
>> - Clone
>> - Create branch and modify code
>> - git diff -U99999
>> - Post through web UI (or use arcanist for these two steps)
>> - Apply the approved version and git push
>>
>> * There is a distinct possibility that I am misunderstanding how the
new
>> workflow would work and it would be easier than what I outlined above,
but
>> that's just the way I understand it.
>>
>> On Thu, Nov 7, 2019 at 6:42 AM Finkel, Hal J. via llvm-dev <
>> llvm-dev at lists.llvm.org> wrote:
>>
>>> 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> 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 listllvm-dev at
lists.llvm.orghttps://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>>
>>> --
>>> Hal Finkel
>>> Lead, Compiler Technology and Programming Languages
>>> Leadership Computing Facility
>>> Argonne National Laboratory
>>>
>>> _______________________________________________
>>> 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
>>
> --
> 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/288c1f3e/attachment.html>

On Thu, Nov 7, 2019 at 9:32 AM Finkel, Hal J. via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
>
> On 11/7/19 10:13 AM, Jameson Nash wrote:
>
> I don't intend to weigh in on either side, but just give a perspective
on
> a few questions asked.
>
> From an outsiders perspective, that list isn't what I'd typically
describe
> as the workflow, since it makes "fork" and "branch"
sound like difficult
> operations. This sounds akin to thinking that someone would reclone the svn
> repo before working on a new commit—possible, but rather extreme. The
> distributed nature of git means that the fork is pretty simple for github
> (essentially creates a new branch in their database, but that is just
> defining a new name for an existing commit).
>
>
> FWIW, I've not found it to be so straightforward in practice. If you
have
> multiple LLVM forks, representing different distinct projects, because
> GitHub only lets you have one fork per account, the process via which you
> can work with multiple derived projects is actually quite annoying.
>
I didn't quite get why you need multiple forks for a given canonical
repository? Maybe you're trying to setup a non-LLVM sub-project being
developed in a fork of the LLVM repo?

Thanks,

--
Mehdi


> GitHub has a page on this (
>
https://github.community/t5/Support-Protips/Alternatives-to-forking-into-the-same-account/ba-p/7428),
> but all of the work-arounds are pretty bad.
>
> In short, only the first fork is easy. After that, it gets harder.
>
>  -Hal
>
>
> I've noticed GitHub recently made it easier to delete the fork, but
I'd
> argue most users shouldn't care. What you'd typically do instead is
to do
> `git remote add <name> <url>` and/or use their `hub` CLI
wrapper (
> https://hub.github.com) to synchronize work between multiple remote and
> local locations.
>
> > 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.
>
> Yes, but a branch is just a name for a particular list of commits. Unlike
> svn, it is not the commits themselves, so they have almost no performance
> implications. I don't think LLVM would want everyone pushing to their
work
> in progress to the central repo, so the "thousands of branches"
would be
> scattered across each individual contributor's repo, and would
represent an
> likely inconsequential fraction of the number of branches already on
GitHub.
>
> > Impossible to chain reviews - a PR diff can only be made on top of git
> master branch
> It is possible to make a PR against someone else's PR on their fork,
> although I hesitate to compare this to the far more usable phabricator
> capability, so perhaps "impossible" is the right term anyways.
> By contrast though, GitHub has no intrinsic concept of a "master"
branch.
> Even aside from the question of using GitHub PRs, you can ask GitHub to
> make a diff between any two (or three) points in time in a few ways, such
> as you could also do locally with git. For example, this'll show you
the
> last two commits:
> https://github.com/llvm/llvm-project/compare/HEAD^^...master
> <https://github.com/llvm/llvm-project/compare/HEAD%5E...master> (two
dots
> for a direct diff, three for showing only changes since the branch point).
> There's also some convenient additional tooling such as adding
".patch"
> will download it as a raw patch file instead showing in the rich editor.
>
> > There is no way to see previous version of the patch.
> This is a fairly new feature, but the PR itself now lists these as events
> (e.g. "user force-pushed from abcdefg to gfedcba", where each of
those are
> hyperlinks) in the comment feed.
>
> > how do i go back to previous revision
> Answering a slightly different question in the hopes someone finds this
> information useful: locally you can do `git reflog <branchname>` to
get a
> history of the state of the HEAD of any tag on that particular machine for
> the last 90 days.
>
>
> On Thu, Nov 7, 2019 at 8:10 AM Nemanja Ivanovic via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
>> I find Hal's analysis of this subject to be the most pragmatic one
in the
>> thread and really want to chime in with a big +1 for the balanced
approach.
>>
>> TL; DR;
>> As it was outlined by previous comments in the thread, the GitHub PR
>> review tool is inferior to Phabricator (and I would argue that this is
an
>> objective statement). Those that are only familiar with GitHub may not
>> ascribe much value to capabilities such as commenting on unchanged
lines,
>> expanding whatever context you're interested in, seeing the diff of
two
>> revisions, etc. But for a good portion of the LLVM community, these
things
>> are important.
>>
>> I think a well integrated solution would not place undue burden on the
>> author or the reviewer. Allowing pull requests seems to be desired from
the
>> perspective of making it easy for authors. Allowing the review to
happen on
>> Phabricator seems to be desired from the perspective of making it easy
for
>> the reviewers. So a solution that can do both is the best of both
worlds.
>>
>> One comment on the initial proposal is that it seems like what is
>> proposed may take away the supposed benefit of moving to PR's in
the first
>> place - making it easy for the author. I fail to see how the workflow*:
>> - Fork
>> - Clone
>> - Create branch and modify code
>> - Push
>> - Create PR
>> - Squash+merge after approval
>> - Delete the fork
>>
>> Is easier from the current:
>> - Clone
>> - Create branch and modify code
>> - git diff -U99999
>> - Post through web UI (or use arcanist for these two steps)
>> - Apply the approved version and git push
>>
>> * There is a distinct possibility that I am misunderstanding how the
new
>> workflow would work and it would be easier than what I outlined above,
but
>> that's just the way I understand it.
>>
>> On Thu, Nov 7, 2019 at 6:42 AM Finkel, Hal J. via llvm-dev <
>> llvm-dev at lists.llvm.org> wrote:
>>
>>> 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> 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 listllvm-dev at
lists.llvm.orghttps://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>>
>>> --
>>> Hal Finkel
>>> Lead, Compiler Technology and Programming Languages
>>> Leadership Computing Facility
>>> Argonne National Laboratory
>>>
>>> _______________________________________________
>>> 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
>>
> --
> Hal Finkel
> Lead, Compiler Technology and Programming Languages
> Leadership Computing Facility
> Argonne National Laboratory
>
> _______________________________________________
> 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/76aacfa0/attachment-0001.html>