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 (Discussions · community · GitHub), 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^\.\.\.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.
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.
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: What is Phabricator · facebook/hhvm Wiki · GitHub
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., GitHub - framawiki/Github-notif-bot: Github to Phabricator gateway) 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., ⚓ T12739 Nuance: As an External Bridge, to GitHub and other similar systems), although the status seems unclear to me.
-Hal
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
Strong -1 personally.
Likewise, for many of the same reasons detailed below.
~Aaron
