Code Review Process Update

Here’s my 2 cents on reviewable.io and git spr:

We’ve been using reviewable.io for a few months on the project GitHub - llsoftsec/llsoftsecbook: Low-Level Software Security for Compiler Developers. We have not done a huge number of reviews with the tool yet, but all of us using it prefer it very much over using the native github review interface.
I haven’t explicitly checked if it fixes all of the shortcomings that have been reported on the github review interface, but we haven’t found any issues with it. It seems it is very good at making sure no review comment gets lost and it’s easy to keep track of which feedback still needs to be addressed.
All review comments made in reviewable.io also appear well on the native github PR interface.
It is recommended though to not mix writing comments in the native github UI and the reviewable.io UI on a given pull request.

I’m not sure if it’s very useful, but here is a link that shows PRs that have used reviewable and had at least a bit of forth-and-back review: Pull requests · llsoftsec/llsoftsecbook · GitHub. To see a review in the reviewable interface for a specific pull request, click on the button in the text saying “This change is Reviewable”.

Overall, without having done a detailed investigation, it seems reviewable.io solves most issues that have been reported with github PR reviews, apart from support for stacked pull requests.

I did try out the git spr project once to create a stacked pull request. I had made a small mistake in preparing the stacked pull request in my downstream repo. When I used the tool to push the stack of PRs, it ended up creating a PR for every commit in the history of the project! (Luckily I could interrupt the tool quickly when I realized something wasn’t right).
That experience makes me believe that a tool like git spr isn’t suitable for a project the scale of LLVM: it’s too easy to make a mistake using it, resulting in huge pollution in branches or pull requests in the upstream repo.

The best I can think of for doing the equivalent of “stacked” pull requests using github is to simply maintain the full stack of commits in a github fork of the LLVM repo, to let reviewers see the full stack. Then, to get commits into the main LLVM repo, start by only creating PRs on the main repo for the commits in the stack that are independent of other commits. As these PRs get merged, other commits from the fork get “unblocked” to create a PR for. It is something that some authors already do today when they have a huge stack of commits that they would like to integrate into mainline LLVM.

1 Like