<div dir="ltr">I know from experience that many people have not filed issues due to bugzilla, and not submitted PRs due to phabricator.<div><br></div><div>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 <a href="https://github.com/aras-p/llvm-project-20170507/pull/2">https://github.com/aras-p/llvm-project-20170507/pull/2</a> - 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).</div><div><br></div><div>So from us it's a big +1.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Nov 7, 2019 at 10:31 AM James Henderson via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org">llvm-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>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):</div><div><br></div><div>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.</div><div>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<br></div><div>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.</div><div>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.</div><div>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.</div><div>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.<br></div><div>7) The PR approach does at least allow committing via the UI, which is perhaps a little less fiddly in some cases.</div><div>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.</div><div>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).<br></div><div>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.</div><div>11) Small one this one, but missing new lines at end of file are much more obvious on Phabricator than Github.</div><div>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?).</div><div><br></div><div>I'm sure I could come up with other points for/against Github PRs. On balance I definitely prefer Phabricator.</div><div><br></div><div>James<br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, 7 Nov 2019 at 09:53, Aaron Ballman via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Thu, Nov 7, 2019 at 3:09 AM Roman Lebedev via llvm-dev<br>
<<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:<br>
><br>
> Strong -1 personally.<br>
<br>
Likewise, for many of the same reasons detailed below.<br>
<br>
~Aaron<br>
<br>
> * What is the endgoal? To fully kill phab and move to github pullrequests?<br>
> it might be best to discuss *that* first. (did i miss an RFC?)<br>
> * Separation of attention - does everyone who cares<br>
> now has to also look at pull requests for reviews;<br>
> or should they be exempt from general review attention?<br>
> * For me personally after using phabricator, github now seems<br>
> extremely crude, laggy, limited. To name a few:<br>
> * There is no way to see previous version of the patch.<br>
> I don't think there is any way to disable force-push for PR's.<br>
> While this is only 'slightly' limiting for the reviewer,<br>
> this can be more limiting for the author - how do i go back<br>
> to previous revision? I can't, i need to maintain a copy<br>
> of every branch i pushed manually.<br>
> * Impossible to chain reviews - a PR diff can only be made<br>
> on top of git master branch. Any non-trivial change consists of<br>
> separable PR's. Now either one will only be able to submit<br>
> dependent PR after the prereqs are merged, or the diff will be<br>
> impossible to read.<br>
> * Does not load large diffs by default.<br>
> That caught me by surprise several times<br>
> when i was searching for something.<br>
> * No diffs in mail - *super* inconvenient.<br>
> One has to open each pr in browser (or fetch via git etc etc)<br>
> * Github is an official US-based commercial organisation.<br>
> It *has* to follow U.S. export law. In particular i'm thinking of<br>
> <a href="https://techcrunch.com/2019/07/29/github-ban-sanctioned-countries/" rel="noreferrer" target="_blank">https://techcrunch.com/2019/07/29/github-ban-sanctioned-countries/</a><br>
> <a href="https://github.com/tkashkin/GameHub/issues/289" rel="noreferrer" target="_blank">https://github.com/tkashkin/GameHub/issues/289</a><br>
> Does phabricator already have such restrictions, blocks?<br>
> If not, wouldn't you say adding such restrictions is not being<br>
> more open for contributors?<br>
> What happens when, in not so long in the future, the entirety of, say,<br>
> china or russian federation is blocked as such?<br>
> * Same question can be asked about internet "iron" curtains<br>
> certain (*cough*) countries are raising. That also has already happened<br>
> before (and *will* happen again), and i was personally affected:<br>
> <a href="https://en.wikipedia.org/wiki/Censorship_of_GitHub#Russia" rel="noreferrer" target="_blank">https://en.wikipedia.org/wiki/Censorship_of_GitHub#Russia</a><br>
> I don't recall that happening to phabricator yet.<br>
> I fail to see how that is more contributor-friendly.<br>
> * Not sure anyone cares, but while using github as main git<br>
> repository "mirror" is perfectly fine - git is distributed, only canonical<br>
> write-repo would be affected anything bad happen. But that isn't the case<br>
> for reviews, issues; as it has been discussed in the "let's migrate bugzilla<br>
> to github issues", it is far more involved.<br>
> * What about DMCA? Not sure how this is currently handled.<br>
> * UI feels laggy. Not much to add here, pretty subjective.<br>
> * I'm sure i'm missing a few.<br>
><br>
> The github does come with it's benefits, sure:<br>
> * It is *simpler* to preserve git commit author.<br>
> Currently one has to ask the author for the "Author: <a href="mailto:e@ma.il" target="_blank">e@ma.il</a>" line,<br>
> and do `git commit --amend --author="<>"`.<br>
> * @mention is wide-r-reaching - whole github, not just llvm phabricator<br>
> * No more "phabricator disk full" issues<br>
> * ???<br>
><br>
> TLDR: such migration lowers the bar for new, first time,<br>
> unestabilished contributors, but i personally feel it *significantly*<br>
> raises the bar for the existing contributors, reviewers.<br>
> We don't live in perfect world. Aspirational goals are aspirational.<br>
> They should be attempted to be reached, but they shouldn't shadow and<br>
> overweight, take over the main goal of the LLVM project.<br>
><br>
> Personally, i don't see that benefits out-/over- weight the drawbacks.<br>
><br>
> Roman.<br>
><br>
> On Thu, Nov 7, 2019 at 8:32 AM Mehdi AMINI via llvm-dev<br>
> <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:<br>
> ><br>
> > Hi all,<br>
> ><br>
> > Now that we're on GitHub, we can discuss about pull-requests.<br>
> > 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.<br>
> > 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.<br>
> ><br>
> > 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!<br>
> ><br>
> > 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:<br>
> ><br>
> > - Contributors should use their own fork and push a branch in their fork.<br>
> > - 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.<br>
> > - 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.<br>
> > 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.<br>
> > - Upon “merge” of a pull-request: history is linear and a single commit lands in master after review is completed.<br>
> ><br>
> > 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.<br>
> ><br>
> > Here is a more complete doc on the topic: <a href="https://docs.google.com/document/d/1DSHQrfydSjoqU9zEnj3rIcds6YN59Jxc37MdiggOyaI" rel="noreferrer" target="_blank">https://docs.google.com/document/d/1DSHQrfydSjoqU9zEnj3rIcds6YN59Jxc37MdiggOyaI</a><br>
> ><br>
> > Cheers,<br>
> ><br>
> > --<br>
> > Mehdi<br>
> ><br>
> > _______________________________________________<br>
> > LLVM Developers mailing list<br>
> > <a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
> > <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
> _______________________________________________<br>
> LLVM Developers mailing list<br>
> <a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
> <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
</blockquote></div>
_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail_signature"><div dir="ltr"><table width="100%" border="0" cellspacing="0" cellpadding="0" style="border-collapse:collapse;border-spacing:0px;color:rgb(90,90,91);font-size:13px;margin:0px 0px 20px;padding:0px"><tbody style="margin:0px;padding:0px"><tr style="margin:0px;padding:0px"><td align="left" style="border-collapse:collapse;font-size:0px;line-height:1.5em;padding:0px 0px 20px;vertical-align:top"><table align="left" style="border-collapse:collapse;border-spacing:0px;margin:0px;padding:0px" border="0" cellspacing="0" cellpadding="0"><tbody style="margin:0px;padding:0px"><tr style="margin:0px;padding:0px"><td style="border-collapse:collapse;font-size:1.12em;line-height:1.5em;padding:0px;vertical-align:top;width:64px"><img width="64" height="auto" style="border: medium none currentcolor; border-radius: 0px; display: block; font-size: 13px; height: auto; line-height: 100%; margin: 0px; max-width: 100%; outline-style: none; outline-width: medium; padding: 20px 0px 0px; width: 100%;" alt="" src="https://unity3d.com/profiles/unity3d/themes/unity/images/ui/other/unity-logo-dark-email.png"></td></tr></tbody></table></td></tr><tr style="margin:0px;padding:0px"><td align="left" style="border-collapse:collapse;font-size:0px;line-height:1.5em;padding:0px;vertical-align:top"><div style="color:rgb(0,0,0);font-family:Roboto,Arial;font-size:14px;font-weight:600;line-height:15px;margin:0px;padding:0px">Neil Henning</div></td></tr><tr style="margin:0px;padding:0px"><td align="left" style="border-collapse:collapse;font-size:0px;line-height:1.5em;padding:0px;vertical-align:top"><div style="color:rgb(0,0,0);font-family:Roboto,Arial;font-size:14px;line-height:15px;margin:0px;padding:0px 0px 10px">Senior Software Engineer Compiler</div></td></tr><tr style="margin:0px;padding:0px"><td align="left" style="border-collapse:collapse;font-size:0px;line-height:1.5em;padding:0px;vertical-align:top"><div style="color:rgb(0,0,0);font-family:Roboto,Arial;font-size:12px;line-height:15px;margin:0px;padding:0px"><a href="http://unity.com" target="_blank">unity.com</a></div></td></tr></tbody></table></div></div>