A Request for Comment on Code Review Process

[tstellar]

Proposal

The LLVM Foundation Board of Directors is seeking comment on the current state of Code Review within the LLVM Project and its sub-projects. Phabricator is no longer actively maintained and we would like to move away from a self-hosted solution, so our goal is to determine if GitHub Pull Requests are a good alternative to our current code review tool: Phabricator.

Specifically we are looking for feedback on:

• What features or properties make Github Pull Requests better than Phabricator?
• What features or properties make Phabricator better than GitHub Pull Requests?
• What new workflows or process improvements will be possible with GitHub Pull Requests?
• Which workflows aren’t possible with GitHub Pull Requests?
• Any other information that you think will help the Board of Directors make the best decision.

Where to Direct Feedback

Please provide feedback on this Infrastructure Working Group ticket. This will make it easier to collect and consolidate the responses. At the end of the comment period the Infrastructure Working Group will collect the feedback for further analysis and summarization.

Timeline

The timeline for this RFC will be as follows:

• RFC posted for public review and comment
• 30 days after the date of posting, public comment closes.
• IWG will have 14 days from closure of public comments to review and summarize public comments into a pros and cons list to be present to LLVM Foundation Board
• Foundation Board will have 30 days to make a final decision about using GitHub Pull Requests and then communicate a migration plan to the community.

[sheredom]

What features or properties make Github Pull Requests better than Phabricator?

TL;DR - new users can contribute to the project using tooling that they are already familiar with.

Clang got a really cool feature because some non-LLVMer (not part of the core community) did some modifications and then our community was fortunate enough that someone within the community took the time to pull the GitHub PR for it over into Phabricator - I'm talking about -time-trace.

From talking to people outwith the core LLVM community - the number one reason that people don't attempt to push legit fixes/features back to LLVM is that the code review process is a) non-normative, and b) scary as a result. I think moving to GitHub PRs, or at least offering them as an option, would greatly help onboarding people to the project.

[davidchisnall]

[ For context: I vastly prefer GitHub's PR flow to Phabricator ]

Which workflows aren’t possible with GitHub Pull Requests?

GitHub's PRs make it very difficult to provide a patch set that can be reviewed independently. The only way that this can be done at all is:

1. Create a PR for the first patch in the series.
2. Create a PR for the second patch in the series to merge not to main, but to the branch of the first.
3. Repeat step 2 for each patch in the sequence up to N.
4. Merge PR for patch N into N-1.
5. Repeat step 4 until everything is merged into the first PR.
6. Merge the first PR.

This is very error-prone. The root problem with GitHub PRs is that the target branch cannot be changed after the PR is created. If it could, then we could create PRs to merge into the next branch in the sequence but then merge them into main in order and reset the next PR to try to merge to main (or merge it into the intermediate branch if reviewers are happy with the roll-up).

[rengolin]

GH Bad:

• GitHub confuses discussion comments with review comments in the code view.
  • You can only answer multiple comments in the code view.
  • Old discussion view keep some comments that are not reachable from code view.
• GitHub doesn't allow to see diffs between arbitrary revisions, only commits directly.
  • Commenting on a commit doesn't guarantee users can reach from discussion or code views
• Patch sets, as @davidchisnall explained above.
  • However, a patch set that is in a single branch is dead easy.

GH Good:

• It's free, it's under development
• GitHub is accessible to millions of developers that already use PRs
• GitHub has lots of options to protect branches, required reviews, run actions, etc.
• GitHub allows direct merge into the repo, while Phab requires offline merge / arcanist.

Phab Bad:

• It's dying, it's custom, it's self hosted
• It's a hassle to register and it's an interface that isn't intuitive to a lot of people.
• I have to upload a diff file... Or use "arcanist", yet another unintuitive tool.
• I have to rebase/merge by hand... or use "arcanist"...

Phab Good:

• Possible to make dependent reviews (series), but it's not really easy
  • I have to create each review separate, add parent/child links, etc.
  • When I change my branch(es), it doesn't update, I have to upload again
  • Even if the patch set is in a single branch, it's still not automatically updated

[kparzysz-quic]

@davidchisnall @rengolin Assume that you have 2 dependent patches and you push 2 branches:

• PR1: branch1 -> main
• PR2: branch2 -> branch1

When you merge PR1, and delete branch1, PR2 will automatically retarget itself from branch1 to main.

[kparzysz-quic]

Also, a PR can have the base changed manually: https://docs.github.com/en/github/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/changing-the-base-branch-of-a-pull-request

[joker-eph]

@kparzysz-quic yes that works but is has two drawbacks I believe:

1) It forces you to push all the branches to the main repo and open pull-request from within the repo, because otherwise the pull-request from branch2 to branch1 is not visible in the main repo.

2) It means that updating these requires to rebase / keep a clean history locally. While I like this and this is how I work with Phabricator, that allows goes against other advice of not rebasing or amend/force-push a pull-request to preserve the review history best.

[llvm-beanz]

In 2019 at the Women in Compilers and Tools Workshop before the LLVM Developers meeting and again this year at the Community.o summit in March, we held workshops discussing barriers for new contributors. In both workshops utilizing GitHub pull requests was identified as a way of lowering the barrier for contributions.

Good, bad, or otherwise, git has become the dominant revision control system for software, especially open source, and hosting services like GitHub, GitLab, Bitbucket and Azure DevOps have made the pull-request model the standard workflow for the industry.

Whatever warts GitHub's pull requests may have, they are the most widely used model for contributing code and providing code reviews. LLVM going against the grain is an unfortunate detriment to our community.

[rengolin]

While I'm unhappy with github PRs, @llvm-beanz point is more important than my personal preference.

While I'm also unhappy with Phabricator, the reverse is also true.

To me, the best course of action is to find a set of guidelines for people that are used to Phabricator to do the same on pull requests, and keep that document up-to-date.

I want to make sure we don't alienate existing contributors for the sake of new ones, but that we also don't make it hard to have new ones at all.

My personal view is that existing contributors know more about llvm and our community and would be more comfortable adapting to a new paradigm, for the time and love invested in the project.

But that's my personal view and in no way takes into consideration other people's difficulties, which could be many, and serious.

[kparzysz-quic]

@joker-eph Yeah, we could end up with hundreds of branches in the main repo. I guess we could agree that branches that are merged should be deleted, and, if left undeleted, can be deleted by anyone (unless they are explicitly made exceptions). The force pushes are probably inevitable given that PRs work on commits, not just diffs...

[rengolin]

I would suggest that all series have to be managed in the developers repos, and only one of their branches to be requested against the official repo's main branch.

So we can all have different branches and a meta branch that Cherry picks from those, probably even have a script for that, and then when the metà branch is pushed to origin, the PR is updated as expected.

[AaronBallman]

To me, what tool I use for code review is one of the most important technical decisions that impacts me as a community member. Last time I gathered data, I participate in approx 80-100 distinct reviews a month in the clang and clang-tools-extra projects, so the choice of code review tools impacts me basically all day, every day.

Specifically we are looking for feedback on:

• What features or properties make Github Pull Requests better than Phabricator?

Once you get used to the GitHub workflow, creating patches from PRs off a branch is quite easy. I'd note that some people use arcanist to simplify creating a Phab patch the same way (I'm not one of those folks, I use Phab directly through the web interface). I also like how easy it is to link to issues with the GitHub UI compared to Phab, but this is a lesser benefit to me (copy-pasting a link to bugzilla is not a ton of effort). The more full-featured markdown abilities in the editor are a nice touch, but not a critical thing to me.

• What features or properties make Phabricator better than GitHub Pull Requests?

The code review interface has far less surprises and annoyances. I'm sure this comes somewhat from the amount of time I've used Phab (many years) vs the amount of time I've used GitHub (~2 years), but I've never found GitHub's code review workflow to be particularly ergonomic. It hides the most important files in a review because there are "too many changes" in it, too many comments on a review slows down the web interface, etc. It's basically the same usability concerns that have been brought up by myself and others every time this topic comes up.

Having used both for a few years now, I still find I am productive in Phab in ways that I'm not productive in GitHub, but it's not easy to boil it down to a succinct "if you fix this, my issue goes away" issue so much as an overall workflow within the tool issue. Anecdotally, it seems to take me about 25% longer when reviewing in GitHub compared to Phabricator, so I'm worried whether I'll be capable of reviewing at the same pace I currently do if we switched.

• What new workflows or process improvements will be possible with GitHub Pull Requests?

None that matter to my day-to-day work, at least that I'm aware of.

• Which workflows aren’t possible with GitHub Pull Requests?

Herald-like subscriptions where I can be added as a required reviewer for changes with a file-level granularity. (At least, I've not found the functionality if it exists.)

• Any other information that you think will help the Board of Directors make the best decision.

My personal preference is to continue to support Phabricator (whether through Phorge, our own fork, whatever). However, I understand the desire to not host our own solutions or have to maintain them, and there's a lot of allure for using GitHub because of its popularity.

But I'd like to reiterate my concern about the danger of switching tools (regardless of what we switch to). The Clang frontend already struggles with review velocity, as can be seen by the number of time people need to "ping" a review thread. This isn't because reviewers are struggling with the tools, it's because reviewers who know the internals of the product well enough to make a decision about a patch are hard to get time from. Increasing the number of PRs the community receives while decreasing the amount of qualified reviews is a very real possibility and is not the kind of experience we want people new to the community to have. I don't know how to address that, but I do know that for myself, I definitely see a decreased amount of productivity when working in GitHub compared to Phab and I know that my review queue already sometimes fills up faster than I can perform the reviews today.

[rengolin]

@joker-eph Yeah, we could end up with hundreds of branches in the main repo. I guess we could agree that branches that are merged should be deleted, and, if left undeleted, can be deleted by anyone (unless they are explicitly made exceptions). The force pushes are probably inevitable given that PRs work on commits, not just diffs...

Having hundreds of branches in the main repo because of PRs and patch sets is unmaintainable. Anyone deleting any branches is dangerous, as GH doesn't really check much before deleting. I don't think that's a viable solution.

[Meinersbur]

IMHO we should prioritize switching from Bugzilla from GitHub over Phabricator. Reasons:

1. bugs.llvm.org is still on version 5.0.2 (released in 2016)
2. Latest release of Bugzilla was in 2019 and its last development commit was in February, while Phabricator went out of maintenance just this June
3. As by the announcement, there will still be some maintenance, just not new features
4. A 3rd party might fork Phabricator and continue its development
5. Switching to GitHub Issues seems to be less controversial
6. bugs.llvm.org still has self-registration disabled, a discouraging first obstacle for new community members; IMHO more than new developers having to become familiar with Phabricator.

Given point 3 and 4, there seems no immediate reason to switch away from Phabricator, especially when compared to the state of our bugtracker.

[kparzysz-quic]

Having hundreds of branches in the main repo because of PRs and patch sets is unmaintainable. Anyone deleting any branches is dangerous, as GH doesn't really check much before deleting. I don't think that's a viable solution.

1. Github allows you to set branch protection rules, so that important branches cannot be deleted.
2. You cannot delete a branch if there is an open PR from it.

[rengolin]

5. Switching to GitHub Issues seems to be less controversial

I wish that was true. While most focus of PR vs Phab is around patch sets, GH issues have a lot more shortcomings than bugzilla (by design, I assume), so there are a lot more things to worry about than PRs.

However, I'd also refrain from commenting about bug tracker on a code review thread. We can only handle so much controversy at a time. :)

[nickdesaulniers]

I think having a bot enforce that presubmit tests run, with documentation on how downstream consumers of LLVM might help set up bots to help participate in the CI presubmit, might be nice. The bot could work as a "merge queue" in that patches must pass CI before being merged, and that the bot alone is responsible for merges, of patches passing CI in FIFO. Also, having some dedicated reviewers could help replace "herald" from phab, AND help new contributors get reviewers auto assigned. (I suspect many newbies don't know who to ask for code review of their initial patches; having a bot analyze git log for the most recent contributors (akin to what the Linux kernel has a script for; scripts/get_maintainer.pl) might be better than this round robin stuff.)

[pogo59]

I've used I don't know how many code-review systems over 40 years. They all fail in one way or another. Moving to GitHub PRs will if anything simplify my life, as I'm already used to GH Enterprise for my downstream repo.

Migrating to GitHub PRs (and Issues) is inevitable. It's how the open-source world works nowadays; let's go ahead and bite the bullet. But let's also not shoot ourselves in the foot.

• Work on bot infrastructure FIRST. It's eternally painful to have every single push send me a half-dozen or more bot-fail messages.
• Move bugzilla to Issues SECOND. Bug numbers are embedded in too many places, they need to be preserved.
• Move off of Phabricator LAST. Phab hates me with a deep abiding passion, but I've learned how to appease it and get the minimum necessary done. We'll want to keep it available in archive mode forever, but at least links to it from commit logs are URLs and not just numbers, so it's okay to have a new numbering scheme afterward.

[rengolin]

@pogo59 that sounds like a good plan. I also support moving to github, both issues and pull requests, but the work to get there isn't trivial and I would hate to rush this and make things worse in the end.

[llvm-beanz]

I think that in moving to pull requests the bots sending mass emails problems should get easier. Most PR testing systems support bots commenting in the PR instead of emailing so the audience is just people subscribed to the PR.

Also with the expansion of pre-merge testing the number of failures post-merge should drop dramatically.

[teqdruid]

I strongly support moving to GH PRs. We've been using them in the CIRCT project from the beginning. Yes, there are downsides mostly stemming from GH's desire to simplify things; but, I personally haven't come across anything which I needed to do which just isn't possible. Our project is far smaller and far less complex than LLVM, so mileage may vary.

• GH now has a CLI tool. I've not used it much, but it's got a ton of functionality. Presumably, it's just a thin wrapper on top of the very full featured GH REST API. Could be useful for anyone concerned about reviewing productivity. https://cli.github.com/
• A killer feature of said CLI (IMO) is the ability to 'git checkout' the PR which I'm reviewing -- even if it's off a fork. This enables running tests locally and using my editor to crawl around code with which I am unfamiliar but the PR uses. Yeah, there's probably a way to get arc to do the same, but I'm scared of arc.
• There are alternative interfaces to GH PRs. There are also some alternative PR review systems which use GH PRs as a backend (e.g. https://www.pullrequest.com/) so as to maintain some compatibility.
• GH workflows can be triggered on PRs (and pretty much anything https://docs.github.com/en/actions/learn-github-actions/events-that-trigger-workflows). This could be useful as an escape hatch.
• There exist a ton of actions that can be run in a workflow some of which to do useful stuff on each PR. (e.g. identify the correct reviewers based on the files modified). There are a few gotchas involved though.
• A bunch of IDEs/editors have integrations with GH PRs.

Some downsides:

• The GH rules for email notifications are either non-existent or overly obtuse. I looked into setting them up for myself a while back, but it was easier just to use email filters.
• Not-unrelated: the GH hosted agents (on which we may want to run actions) run on a very small VM size. While larger ones are supported on private repos, AFAIK they are not yet supported for public ones: https://github.community/t/github-hosted-runners-vm-size/136482. Setting up our own would be fraught with security and setup issues. The CIRCT project uses GH workflows on GH hosted agents to do all of our validation builds. Doing an MLIR build is downright painful -- it routinely takes 1.5 to 2+ hours for a 'unified' build. Fortunately, we cache the MLIR build so our PR checks don't take that long to run.

Overall, I think the network effect is extremely beneficial and probably the best reason to switch. If there's a feature we need, we could likely add it via either a GH workflow or a bot. I've also noticed a significant acceleration in GH feature velocity over the last few years, so things are just going to get more featureful. To that end, GH does publish their roadmap: https://github.com/github/roadmap/projects/1.

[richardxia]

• The GH rules for email notifications are either non-existent or overly obtuse. I looked into setting them up for myself a while back, but it was easier just to use email filters.

For what it's worth, GitHub actually has documentation on the special foo@noreply.github.com CC addresses that you can use for setting up your own email filters: https://docs.github.com/en/account-and-profile/managing-subscriptions-and-notifications-on-github/setting-up-notifications/configuring-notifications#filtering-email-notifications. This doesn't beat having a more sophisticated way to configure email notifications, but at least it does give you something a bit better to filter on than just the email subject.

[jh7370]

I'm strongly against moving to PRs at this time, for reasons I'll detail below. I don't think any of my concerns are unsolveable necessarily, but if we move without addressing them, I'll really not be happy, and my ability to review contributions will decrease. This is particularly important on both counts, because I'm currently not even assigned to an LLVM-related team in my company, so I can't justify spending more than a relatively small amount of time reviewing things upstream, and I'm also one of about 2 people who currently reviews changes in most of the LLVM binutils, so my review bandwidth is critical to development in this area.

Also, for context, my company uses Github Enterprise internally, and has done for a number of years. I feel like I have about equal experience in both GHE and Phabricator, and the latter is my preferred review system by an absolutely huge margin.

What features or properties make Github Pull Requests better than Phabricator?

• Integration with other infrastructure, such as Github issues, and the main repo definitely are useful.
• UI options for directly merging in changes are useful (although I find I only use them about 20% of the time in my internal company work, preferring the command-line instead).
• New contributors will often be more familiar with PRs with Phab reviews (but not always).
• No "missing context" due to people forgetting to add -U999999 to their diff generation.

What features or properties make Phabricator better than GitHub Pull Requests?

• For new contributors who have no familiarity with Phabricator or GH PRs, I think Phabricator's entry barrier is actually lower as things stand as there are fewer things that need to be got right. Certainly, it didn't take me very long to get up to speed with it.
• Patches don't have to correspond to branches: I often do my work in a feature branch, with a Phabricator review per commit. This makes updating individual commits easier for me. It also means that where I have a small piece of work, I can just have a commit directly on top of main, without needing to update things, which is simpler than going through the branch creation process.
• Github PRs don't really provide any true commit stacks, for patch series, where you want to keep the reviews separate: instead, you have to have a branch off a branch off a branch off ... which has the potential to get confusing very quickly. There's also no at-a-glance view option of what the patch series looks like in such a model, whereas in Phabricator the "Stack" button shows all linked-together patches.
• The comments in Phabricator don't disappear, unless the file they were in is completely removed from the patch (even then they are still available in the history). In Github, they often "hide" in various odd places, due to being "out-of-date" when the relevant area is moved. In Phabricator, there is only one view of the whole patch, with inline comments always being visible unless explicitly hidden by the reviewer. Yes, they sometimes move and lose some context, but it's usually possible to figure out where they're from. This point is perhaps my biggest complaint against Github. It is much easier in Github to miss comments, and much harder to keep conversations in an area going.
• Comments in Phabricator can be placed anywhere in the available context. In Github, they can only be placed within a few lines of modified code. Sometimes, this makes commenting where a comment is necessary impossible.
• It can be easy to not be looking at the full patch as planned to land in Github, due to it sometimes deciding to show you only the latest commit. Whilst the same is possible in Phabricator, I find it much less likely to happen.
• Both provide the ability to view diffs in the history of the review, but in Github, this only is possible if people haven't force-pushed changes, yet it seems to be a common workflow to do so. In Phabricator, the History tab provides the sequence of diffs that have existed for the patch, and it is sometimes useful to go back and see the differences between two specific stages, neither of which are the latest. I feel like this isn't as straightforward in Github.
• Github allows a person to mark an item as "Resolved", but if memory serves me rightly, this prevents further commenting on the comment chain, which is inferior to the "Mark Done" option in Phabricator.
• Having to switch tabs between reviewing files and discussion comments is a too-many clicks issue, and also makes it harder to easily flick between things. I often end up with two separate browser tabs when reviewing GHE PRs. This is exarcebated by the "disappearing comments" issue.

What new workflows or process improvements will be possible with GitHub Pull Requests?

• As noted above, the merging options can make integrating patches simpler.
• Sometimes, dividing a single review into multiple commits, is useful, as each commit can have its own comments etc, and this can only be done in Github.

Which workflows aren’t possible with GitHub Pull Requests?

• Patch series are more intuitive in Phabricator.
• Auto-subscribing/auto-reviewing seems to be more straightforward. In partciular, if memory serves me rightly, there is a hard limit on the number of reviewers in our GHE instance, which may be problematic in some cases.

[SamTebbs33]

• Which workflows aren’t possible with GitHub Pull Requests?

Herald-like subscriptions where I can be added as a required reviewer for changes with a file-level granularity. (At least, I've not found the functionality if it exists.)

PRs will get reviewers added automatically for specific files/folders if you have a CODEOWNERS file in the repository.

[jh7370]

• Which workflows aren’t possible with GitHub Pull Requests?

Herald-like subscriptions where I can be added as a required reviewer for changes with a file-level granularity. (At least, I've not found the functionality if it exists.)

PRs will get reviewers added automatically for specific files/folders if you have a CODEOWNERS file in the repository.

Within LLVM, there's a big difference between code owners and reviewers. I'm automatically subscribed as a reviewer to several areas of code, but I don't consider myself a code owner of these. Also, IIRC within our GHE instance at my company, when trying to add reviewers to a list that had been prefilled with many code owners, several of the existing reviewers were removed (or new ones not added) due to there being a relatively low limit.

[peterwaller-arm]

A negative against arcanist: A newcomer may have to learn both the idiosyncrasies of git (which I think most would agree is not straightforward) compounded against those of Phabricator and arcanist. One specific harm of this is that it makes it harder to pull down a patch from Phabricator without mangling something or ending up in a strange git repository state. Simple enough for an experienced person to fix, but a barrier to entry for a newcomer. Phabricator's tooling, documentation and ethos is playful in admitting that it is 'arcane' but this is not practical and utilitarian when it comes to growing a community. Hosting on GitHub is better in this regard since it exposes pull requests as part of the usual 'git push/pull' mechanism that everyone learns if they use Git anyway.

[tru]

As a fairly newcomer to the project I vastly prefer GitHub Pull Requests because of the ease of use and the familiarity. Phab was a huge obstacle and learning curve for me. I have not been able to install arcanist since it depends on PHP.

[SamTebbs33]

• Which workflows aren’t possible with GitHub Pull Requests?

Herald-like subscriptions where I can be added as a required reviewer for changes with a file-level granularity. (At least, I've not found the functionality if it exists.)

PRs will get reviewers added automatically for specific files/folders if you have a CODEOWNERS file in the repository.

Within LLVM, there's a big difference between code owners and reviewers. I'm automatically subscribed as a reviewer to several areas of code, but I don't consider myself a code owner of these.

Well that depends on how you interpret "code owners". In GitHub it just means someone that normally reviews changes to a particular file, folder etc. I would be added for the Arm backend, for example, as my colleagues add me as a reviewer to their Arm backend patches.

[jh7370]

• Which workflows aren’t possible with GitHub Pull Requests?

Herald-like subscriptions where I can be added as a required reviewer for changes with a file-level granularity. (At least, I've not found the functionality if it exists.)

PRs will get reviewers added automatically for specific files/folders if you have a CODEOWNERS file in the repository.

Within LLVM, there's a big difference between code owners and reviewers. I'm automatically subscribed as a reviewer to several areas of code, but I don't consider myself a code owner of these.

Well that depends on how you interpret "code owners". In GitHub it just means someone that normally reviews changes to a particular file, folder etc. I would be added for the Arm backend, for example, as my colleagues add me as a reviewer to their Arm backend patches.

Right, but my point is that the LLVM definition doesn't match GitHub's (see https://llvm.org/docs/DeveloperPolicy.html#code-owners) - code owners perhaps should be reviewers on all patches (even that's debateable), but not all people who want to always be a reviewer are code owners.

[nemanjai]

While I always hated the GitHub interface for reviewing code and still do (for reasons others have mentioned so I won't repeat), I will of course adapt to whatever tool the community chooses.

One thing that seems quite concerning to me though is that I have seen several mentions that "force push will be inevitable". Although I don't understand why that would be, I always think that using the force option is an indication that something has gone terribly wrong and needs to be fixed. So I am honestly concerned/afraid of any process that routinely requires using the force option.

Also, I find branches to be a very useful concept when there is a very limited number of them (i.e. main and release branches that we have now). However, once everyone needs to push a branch to get their code reviewed in a project as large as LLVM, the number of branches is essentially guaranteed to be so high that in my opinion, the whole concept of branches becomes rather useless. But of course, this is just my opinion and many people on many projects have found ways to work effectively in the presence of so many branches.

[rengolin]

One thing that seems quite concerning to me though is that I have seen several mentions that "force push will be inevitable".

I think this was only about non-main branches and patch sets, which was a side discussion on how to make that work with multiple branches. I seriously hope this does not become the norm.

The main branch is protected against force pushes.

Also, I find branches to be a very useful concept when there is a very limited number of them (i.e. main and release branches that we have now). However, once everyone needs to push a branch to get their code reviewed in a project as large as LLVM, the number of branches is essentially guaranteed to be so high that in my opinion, the whole concept of branches becomes rather useless.

I concur. With hundreds of active contributors, we could easily end up with thousands of branches at any given point, and we'll need branch naming conventions, and name policing and cleanups, and the maintenance doesn't scale.

I believe developers should keep their internal structure to their own repositories and only expose it via a pull request from their own local branches.

If they want more branches to create dependent development, but want to publish PRs that can be merged, then they should merge multiple branches into a single one (on their repo) and create a PR on that branch.

If the need is only to inform (ex. this is what the next stage looks like), then they can just add a link to their own branch from the first PR, and when that gets merged, they rebase and create a new PR from the second branch, etc.

[LebedevRI]

If the need is only to inform (ex. this is what the next stage looks like), then they can just add a link to their own branch from the first PR, and when that gets merged, they rebase and create a new PR from the second branch, etc.

So in other words, there won't be any more stacked reviews, and everything must be done in sequence.

Example: Over the last ~10 days i've performed X86TTIImpl::getInterleavedMemoryOpCostAVX2() rehaul, which spanned some ~70 patches, with each ~5 of those being stacked ontop of each other. If effectively only a single review can exist at the time, that would have taken months, while, as i have said, it only look just over a week. (starting at https://github.com/llvm/llvm-project/commit/d9413f46b308df5afd7fc106df2af809757bb0c9)

[rengolin]

So in other words, there won't be any more stacked reviews, and everything must be done in sequence.

I'm not sure what you mean by that. Perhaps I didn't explain it well enough.

I believe a stack of PR against each other and a branch with stacked commits will have similar costs to developers and reviewers.

In either case you'll need the branches against each other. In the first case, you expose those branches in PRs but that needs the branches to be in the main repo. In the second case, you have those branches on your repo, and expose only one branch (still in your repo) that is a flat representation of all the branches.

On both cases, you have to keep rebasing the branches against each other on changes, but on the chained PR case, those rebases are pushed directly to the PR, while in the local branch case you have one additional step, to reassemble the public PR branch. However, in the chained PR case, when approved, you have to rebase the chain and then merge, while on the local branch, you just merge.

I don't think either case is remotely good. But neither is also worse than Phab. With the current model, I still have to keep branches on my local repo and upload them as patches to Phab, then rebase them all before merging. Essentially, the bureaucracy is more or less the same (but still less than Phab).

It wold be much better if Github allowed us to create PRs on top of each other directly, and when all are approved, merge them in sequence (if the tree is consistent). But that's not a trivial feature either, and if done badly, can corrupt the main branch. For instance, it would need a way to lock the branches against future pushes, check consistency, probably run CI on the final mega-branch, and then merge, closing all PRs in one go.

Example: Over the last ~10 days i've performed X86TTIImpl::getInterleavedMemoryOpCostAVX2() rehaul, which spanned some ~70 patches, with each ~5 of those being stacked ontop of each other. If effectively only a single review can exist at the time, that would have taken months, while, as i have said, it only look just over a week. (starting at llvm/llvm-project@d9413f4)

I'm not sure what you're using to infer time taken, or what kind of thing you're comparing here. Definitely not stacked PR vs local branches, because we have neither of them at the time.

I also didn't propose to end patch sets or multiple branches, so perhaps you just misunderstood what I said?

[pogo59]

For new contributors who have no familiarity with Phabricator or GH PRs, I think Phabricator's entry barrier is actually lower as things stand as there are fewer things that need to be got right. Certainly, it didn't take me very long to get up to speed with it.

Phabricator's help is effectively non-existent, terminology is not always clear, and useful commands/buttons aren't always in obvious places. The guidance in the LLVM docs for how to use Phabricator was the product of much blood, sweat, and tears. I'm glad it's helpful to others but I do not take it as evidence that Phabricator is easy to use.

[tstellar]

If the need is only to inform (ex. this is what the next stage looks like), then they can just add a link to their own branch from the first PR, and when that gets merged, they rebase and create a new PR from the second branch, etc.

So in other words, there won't be any more stacked reviews, and everything must be done in sequence.

Example: Over the last ~10 days i've performed X86TTIImpl::getInterleavedMemoryOpCostAVX2() rehaul, which spanned some ~70 patches, with each ~5 of those being stacked ontop of each other. If effectively only a single review can exist at the time, that would have taken months, while, as i have said, it only look just over a week. (starting at llvm/llvm-project@d9413f4)

@LebedevRI Can you explain more about what it means to have 'stacked' changes in Phabricator and how this is different or similar to a pull request with multiple commits?

[ghost]

Since I don't think they've been mentioned, I'll add a few points about 7 specific features of the two systems:

• GH PRs can be done across different hosted repositories. This would address the concerns about having too many branches in the LLVM repo. We could require that all PRs be from a second repository, separate from the main LLVM repo; either from an individual's personal/company fork of the LLVM repo, or from a second communal GH repo dedicated to this purpose (or we could allow both). Users won't even need to have separate clones of the main LLVM repo and the repo they're using for a PR, since they could be added as two different remotes.
• Some have expressed concern about the use of rebase and force-push to keep a PR's commit history clean, and the inability to compare with previous versions of a PR. This is actually not true, and in fact GH has 2 much more versatile and powerful methods of allowing users to look at old versions of a PR. In either case, once we have the commit hash from before the force-push, it can be checked out by the user locally (using git fetch <remote> <commit hash>, then git checkout <commit hash>), allowing pretty much unlimited flexibility in what the user wants to do with it.
  • First, and most straightforward is that GH will put a note in the PR after a force push that looks something like this: "@Daniel-McIntosh-IBM force-pushed the branch from a1b2c3 to 3a2b1c on ", even providing a convenient link at "force-pushed" that links to a comparison of the before and after.
  • Alternatively, while it's not well documented, GH lets you use many of the git revision shorthands in URLs. For example, https://github.com/llvm/llvm-project/tree/main@{1} and https://github.com/llvm/llvm-project/compare/main~4..main@{2 seconds ago}. This can be used to look at older versions of a PR (using <pr-source-branch>@{n}) and compare versions (using <pr-source-branch>@{n}..<pr-source-branch>@{m}). Unfortunately for this approach, the GH UI doesn't provide any indication this is possible, so it's not something people are likely to discover on their own, but it's not very complicated if you're familiar with the short hands, and would be easy to include in our own documentation on the review process for LLVM. On the other hand, this is a bit more powerful because it allows comparisons between arbitrary versions of PRs, and can be combined with other revision short hands to do things like compare the 3rd last commit of the PR 2 versions ago with the 4th last commit of the PR in its current form (<source-branch>@{2}~3..<source-branch>~4) . The only limitation is that it depends on the reflog, which may eventually get cleared (especially once the branch is deleted), potentially making it unusable for people trying look back at a PR several years later. If we care about this, a workaround would be to set the reflog to never expire.
• GH lets you compare edits of comments and PR descriptions, this information is lost in Phabricator
• GH will show in a PR when it gets mentioned in a PR or issue from a different public GH repository
• GH allows viewing .rst files in a PR as a rich diff with formatting
• GH allows language specific syntax highlighting within markdown code blocks
• Phabricator has the ability to make a PR depend on multiple parent revisions. GH stacked PRs can't do this (A stacked PR is where the base branch of one PR is the source branch of another). It does make up for this somewhat with it's ability to have a PR contain multiple commits, however using a single PR with multiple commits instead of stacked PRs requires that the whole thing be reviewed as one.

As for my overall thoughts: I think the GH features I mentioned address all the concerns that could make GH unworkable, the rest seem to be matters of convinience/UI/workflow/etc. which are manageable. Overall, I have to agree with the general sentiments already here: The biggest advantage GH pull requests has comes from it being a more widely used tool - Much more beginner friendly, better integration with other tools and other GH features like issue tracking, etc.; while Phabricator's advantages seem to mostly be a result of its legacy as the review tool for LLVM - the existing workflow for review, terminology, and guidelines we have are all designed to work with phabricator

[Meinersbur]

While it's not well documented, GH lets you use many of the git revision shorthands in URLs. For example, https://github.com/llvm/llvm-project/tree/main@{1} and https://github.com/llvm/llvm-project/compare/main~4..main@{2 seconds ago}. This can be used to look at older versions of a PR (using <pr-base-branch>@{n}) and compare versions (using <pr-base-branch>@{n}..<pr-base-branch>@{m}). In fact, since the commits still exist in the repository, they can be checked out by the user locally (using git fetch <remote> <commit hash>, then git checkout <commit hash>).

I may misunderstand something, please correct me if that's the case. What you are referring to is referencing a specific commit. The commits that a GitHub Pull Request contains can be seen in the PR's "commits" tab.

Problem is, as soon as you are rebasing, all the previous commits, including the comments associated to specific lines, are lost. Since will want to rebase PRs instead of merging them, that is inevitable. You mention extending the lifetime in the reflog. That's only the case with your local repository, not the remote GitHub one, we have no control over that. In fact, if you git clone or git fetch from a remote, you will not get the content of the remote's reflog (nor any review comments from the GitHub UI). Also, in my experience, GitHub removes commits almost immediately after they have been orphaned.

[ghost]

While it's not well documented, GH lets you use many of the git revision shorthands in URLs. For example, https://github.com/llvm/llvm-project/tree/main@{1} and https://github.com/llvm/llvm-project/compare/main~4..main@{2 seconds ago}. This can be used to look at older versions of a PR (using <pr-source-branch>@{n}) and compare versions (using <pr-source-branch>@{n}..<pr-source-branch>@{m}). In fact, since the commits still exist in the repository, they can be checked out by the user locally (using git fetch <remote> <commit hash>, then git checkout <commit hash>).

I may misunderstand something, please correct me if that's the case. What you are referring to is referencing a specific commit. The commits that a GitHub Pull Request contains can be seen in the PR's "commits" tab.

Problem is, as soon as you are rebasing, all the previous commits, including the comments associated to specific lines, are lost. Since will want to rebase PRs instead of merging them, that is inevitable. You mention extending the lifetime in the reflog. That's only the case with your local repository, not the remote GitHub one, we have no control over that. In fact, if you git clone or git fetch from a remote, you will not get the content of the remote's reflog (nor any review comments from the GitHub UI). Also, in my experience, GitHub removes commits almost immediately after they have been orphaned.

No, I'm not referencing a specific commit from within a PR. That would be done using the <pr-source-branch>~<n> or <pr-source-branch>^<n> shorthands. The <ref>@{n} form uses the reflog and "specifies the n-th prior value of <ref>", and can thus be used to reference commits no longer contained within the PR.

For example, suppose I start with my_branch pointing to 123abcd and the parent of 123abcd is 012abcd. Then I update my_branch using git commit --amend, and now my_branch points at 234abcd. Then the parent of 234abcd is 012abcd, but my_branch@{1} points to 123abcd. After a rebase or amend, the commits aren't actually lost as you say, they just become hard to find, but if you have the commit hash from before the rebase/amend, you'll see that the commit still exists. git only deletes commits when they become unreachable (including by the reflog), and even then, only actually deletes them when it runs the garbage collection.

Using the <ref>@{n} syntax in a url basically gives us the ability to peek into the reflog on the GitHub server (Note that this is NOT what you get using origin/<branch>@{n} locally). Since updates to the PR are accomplished by pushing <pr-source-branch> (i.e. by updating the value of <pr-source-branch> on the GitHub server), each of the "n-th prior value"s of <pr-source-branch> corresponds to a previous version of the PR, and vise-versa.

As for extending the lifetime of the reflog, I'm not 100% sure on this, but I imagine the remote would behave as any other git repository, and obey any configs in the repository's .gitconfig and/or .git/config. So, if we set the reflog expiry config in the repository (and not globally for the user), I would think it should apply to the server after we push the updated config file. Even if we don't update it though, 30 days seems like it should be enough time for any reviewers or interested parties following a PR to take a look a what changed.

[stephenneuendorffer]

The root problem with GitHub PRs is that the target branch cannot be changed after the PR is created.

I think this is possible at some level. If you 'edit' a pull request, you can select a branch to merge against. Maybe this could support what you're looking for?

[zygoloid]

What features or properties make Github Pull Requests better than Phabricator?

Being part of GitHub, in line with user expectations for a GitHub-hosted project in general.

What features or properties make Phabricator better than GitHub Pull Requests?

Phabricator makes it much easier to keep track of all active comment threads and ensure they're all addressed prior to the change being approved. GitHub pull requests frequently hit situations where comment threads disappear despite not being resolved, and generally GitHub PRs make it challenging to find all the current unresolved comment threads and ensure they've all been resolved satisfactorily.

Phabricator permits attaching comments to unchanged lines. This is frequently useful when pointing out places where changes should have been made but weren't, and is part of the motivation for requesting full-context diffs in Phabricator PRs. GitHub is currently architecturally unable to even represent such comments.

What new workflows or process improvements will be possible with GitHub Pull Requests?

Better integration between our issues system and review system, once we migrate from Bugzilla to GitHub issues.

GitHub has many integration points with third-party tools, which bring opportunities for process and workflow improvements.

Which workflows aren’t possible with GitHub Pull Requests?

Having used GitHub PRs for a while, I do not think they provide a reasonable patch review workflow at all, and switching to them would regress the quality of code reviews compared to Phabricator. However, GitHub PRs combined with reviewable.io or similar might provide a good Phabricator replacement.

Additionally: Phabricator provides reasonable support for dependent change review. It's easy to submit for review a diff between the outcome of an earlier patch and the outcome of a later patch, and review just that change in isolation. In GitHub, doing this requires unreasonable contortions: you need to push to the target repository a separate branch containing the result of the earlier patch, or similar. This need doesn't currently come up all that often in Clang development, but it does seem to arise at least a few times a year.

[joker-eph]

GH PRs can be done across different hosted repositories. This would address the concerns about having too many branches in the LLVM repo.

@DanielMcIntosh-IBM : the point about branches in the repo was brought in particular for stacked pull-request. Working with forks doesn't enable to have "cascading pull-request" (which emulates a stack of revision in Phabricator) where you open branch1->master and branch2 -> branch1.

@LebedevRI Can you explain more about what it means to have 'stacked' changes in Phabricator and how this is different or similar to a pull request with multiple commits?

"Stacked changes" are reviewed and discussed independently on Phabricator. Commits in the same pull-request don't have "separate thread of review". You can't start approving one commit before reviewing the entire PR, while you can approve revisions independently in Phab, allowing to start landing change in sequence. You may also have dependent changes where each change aren't expected to be reviewed by the same folks. Also, updating a PR with many commit requires force-push, while some people consider that we shouldn't rebase/force-push a PR during its review (we may be fine with this, but then we have to consider the other issue that people said is addressed with avoiding force-push: you can't have your cake and eat it too).

[RKSimon]

I'd much prefer the foundation first focuses its very limited time and resource on completing existing infrastructure changes and only start yet another project once these are completed.

In particular the migration from bugzilla to github issues has been going on for far too long.

After that there is the proposed mailing list -> discourse transition.

[tstellar]

@joker-eph @LebedevRI Could you post a link to an example of a stacked review in Phabricator?

[ChristianKuehnel]

Could you post a link to an example of a stacked review in Phabricator?

@tstellar I can offer a current example I'm working on:

• D105177 implements the new logic for indexing some code
• D108119 wires it up with the main function

In Phabricator this is modeled as child/parent relationship between revisions.

[LebedevRI]

@joker-eph @LebedevRI Could you post a link to an example of a stacked review in Phabricator?

https://reviews.llvm.org/D111094 e.g.

[ghost]

GH PRs can be done across different hosted repositories. This would address the concerns about having too many branches in the LLVM repo.

@DanielMcIntosh-IBM : the point about branches in the repo was brought in particular for stacked pull-request. Working with forks doesn't enable to have "cascading pull-request" (which emulates a stack of revision in Phabricator) where you open branch1->master and branch2 -> branch1.

Requiring that all non-cascading pull requests be done from a personal/company fork would eliminate almost all the branches from the repository.

If that's not enough, and we want to keep the main LLVM repo free from all PR branches, we could have a separate community repo for (stacked PR) reviews, and automatically copy over commits from the main branch there to the main branch in the main LLVM repo*. If we don't want too many branches there, we could even still allow/encourage/require non-cascading PRs to be done from personal/company forks and into the main repo. Or, if we also want to keep all the PRs in one location, but keep the number of branches in the community review repo small, have non-cascading PRs be done from personal/company forks into the review repo.

My point is, there are so many ways of splitting things up so the branches don't all end up in one place, that I'm sure we can come up with a method that satisfies our needs. And with GH actions, the maintenance cost (including enforcing any rules about branches) should be minimal. Re-reading @rengolin's comments so far, it seems he was already suggesting some form of this.

*: We'd want all the PRs in a set of cascading PRs to be between branches in the same repo so we can take advantage of the automatic branch re-targeting that @kparzysz-quic mentioned. Thus, they need to start at the main branch in the review repo, and not the main branch in the main repo.

[ChristianKuehnel]

Having hundreds of branches in the main repo because of PRs and patch sets is unmaintainable.

I do agree with that concern and would propose a policy to solve that situation. Some ideas what this policy could look like:

• Developers shall create the PRs from their own forks. This way they can manage the lifecycle of their branches as they wish.
• Only branches for stacked PRs shall be stored in the main LLVM repo.
• Old/stale branches and PRs are deleted from the main LLVM repo:
  • We will close all PRs after 6 months of inactivity (no new commits or comments).
  • We will delete all branches 2 months after their PRs are merged or closed. So users still have time to re-open PRs if needed.
  • We will delete all branches older than 2 months if they are not related to any PR. You can open a (draft) PR to keep them alive.
• If developers want to keep their branches for a longer period, they should keep a copy in their fork.

There a several Github bots/apps available for this type of work. Even if none of them matches our policy, we can implement our own cron job to do this cleanup work using the GitHub API.

[ChristianKuehnel]

• What features or properties make Github Pull Requests better than Phabricator?

There is a ton of tools available to make pre-merge testing really simple and convenient to the point where we could have 100% of the commits pre-merge tested before merging them to the main branch. This would give us a significant improvement in stability. And that is without requiring more work from the contributors!

We certainly can't catch all test/build failures with pre-merge testing, we just have too many different variants to be checked. However we can catch the ones where we can get enough cheap compute resources. Today's pre-merge testing is already covering 60-80% of all commits, so it's possible to do that.

The Rust folks have an amazing CI setup for their project with 100% pre-merge testing and automatic merging of PRs to the main branch. So this can be done.

Why can't we do that with Phabricator?

A couple of issues:

• With Phabricator users need push access to the main branch and we can't prevent this. So users can still merge code that doesn't build.
• The data model of Phabricator (see below) makes pre-merge testing hard. About 30% of all pre-merge builds are failing just because we can't apply the patch.
• The tool integration for GitHub is much better, for Phabricator we need lots of our own glue code to make pre-merge testing work at all.

What the problem with the data model?

I would like to call out the differences in the data model these tools are using:

• Phabricator works on diffs. It also works on a plain diff that is not in any way related to a git revision.
• GitHub PR works on full commits (with history).

So you can immediately run a build on the commits from a PullRequest. This makes automatic checks (e.g.pre-merge testing) very easy.

For Phabricator we first need to apply the patch to a git commit, before we can test it. We keep seeing this fail for any number of reasons:

• There is not base commit specified (e.g. when uploading the diff via the Web UI).
• The given base commit is not in the public repo, but only in a local one.
• The patch depends on another patch and this is not/incorrectly set in Phabricator.
• And probably a lot more things we did not even identify so far.

[kwk]

@davidchisnall , in https://github.com/llvm/llvm-iwg/issues/73#issuecomment-934581493 you've written:

The root problem with GitHub PRs is that the target branch cannot be changed after the PR is created. If it could, then we could create PRs to merge into the next branch in the sequence but then merge them into main in order and reset the next PR to try to merge to main (or merge it into the intermediate branch if reviewers are happy with the roll-up).

If I understand @kparzysz-quic in https://github.com/llvm/llvm-iwg/issues/73#issuecomment-934625520 correctly, you can in fact change the target branch of a PR:

Also, a PR can have the base changed manually: https://docs.github.com/en/github/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/changing-the-base-branch-of-a-pull-request

Do you agree @davidchisnall ?

[kwk]

What new workflows or process improvements will be possible with GitHub Pull Requests?

In this comment I'm trying to summarize my ideas for a more natural feel to more interaction with the CI. I don't want the CI to implicitly do something. I want some sort of dialogue and communication between the human part of written text in a PR an any CI system. And I think that Github offers some great stuff here (see below).

Current Post merge testing with buildbot

As you probably all know: We have a pretty big buildbot infrastructure. That is a collection of machines that register with a so called buildbot master to run builds once new commits have landed in the main branch of LLVM. That is what we call post-merge testing. Nearly everyone can donate a machine by connecting to the master with some manual effort. But once the machine is registered it automatically is triggered because the master listens to changes on the main branch of the LLVM repository.

Proof of Concept to utilize the buildbot power for pre-merge testing

I have a Proof of Concept (PoC) that demonstrates in isolation how we can utilize our existing buildbot infrastructure to kick-off test builds on a worker of your needs by using a PR comment such as /build-on-worker XY. A Github Action will find this comment and federate the access to the buildbot master to kick of a build on that XY worker. The The benefit of this is that we can have pre-commit testing with any extremes you can imagine out of the box. We only need to tweak the master configuration to add an pre-merge-opt-in flag to the machines that shall be enabled.

Examples of the seamless workflow

Non-existent worker

Suppose you want to build your PR on a non existing worker called foobar. This is how it could look like.

Notice that the the kwk-bot is just the user I've chosen to speak with when buildbot reports back. In this case the message is very small with just enough bits to help you understand the error. If you expand the section with the little black triangle, you can see the full details:

Existing worker

Now, let's assume you have found yourself an existing worker and you have successfully created your build on that machine. This is how it could end up looking like:

Please, notice that the one comment we began with has been edited multiple times by the buildbot master in order report the progress and the end result. For the purpose of this example I've manually edited the description by hand. You can see all revisions by taking a look in the history of a comment through this

Summary

I know there are voices who think that pre-merge testing is too slow and cannot be done effectively but I'd say that if done right, we can get people to like it for several reasons:

1. Suppose you need to change code in LLVM for a setup which you don't have access to but there's a buildbot worker for to test it. Why not use that?
2. We don't need to pre-merge test all possible scenarios of the virtually endless configuration matrix of LLVM with all it's OSes, compilers, library versions, dependencies and compiler and linker flags. But we can test for what we think fits. Heck, we could even tell you, based upon the files you've touched in your patch, what buildbot worker you should consider to run tests on in your PR.
3. And if you patch code that is very slow to test, then guess what, you'll have to test it and wait as long as it takes. But that doesn't necessarily mean we all have to test it, right? Maybe if your employer builds GPUs and you think, all existing buildbot workers are too slow, then add your own and use that or allow others to use it as well.

I hope, you begin to see why I think that this "review-driven and cost effective focus testing" is a nice approach and shows new horizons of what's possible with the Github Action and Workflow model.

P.S. I plan to show case the PoC that I've build in a video soon when I'm back from vacation. Until then I hope to read some comments about this.

[davidchisnall]

Do you agree @davidchisnall ?

Yup, it looks as if this feature has been added since I last checked. Thanks @kwk