..

Using Gerrit for GitHub Reviews

Where there’s a will, there’s a way.

The problem(s)

Unfortunately, GitHub is the de-facto platform for open-source software collaboration. Yes, of course, there are SourceHut and Codeberg and whatever else, but I have to use what everybody else uses. As it has been for a while, that somewhere else is GitHub. One of the worst (in my opinion) things that has resulted from the proliferation of GitHub is poor management of changes.

I should preface this section by saying it is particularly opinionated. That said, I find there is a disconnect between my preferred git workflow and the GitHub Pull Request model. In the former, I would like to stick to the platonic ideal of the git commit, as much as possible. In my opinion, that is a minimal set of logically-related changes. This does mean that you have to think about things ahead of time. A refactor of some part of the code is fairly straightforward. Introducing a new feature may start by writing its interface as one change, and wiring it in as another. Should tests come alongside or after their associated feature? I think there are arguments for either approach on that one.

Pull Requests aren’t the right model

By contrast, a Pull Request is an all-in-one bundle. Ideally, it would be a collection of logical commits as defined above. For example:

  1. refactor foo_func();
  2. introduce BarFactory;
  3. add tests for BarFactory;
  4. create a new Bar inside foo_func().

On a change-by-change level, this is sensible and justifiable. The issue is that by submitting the feature as Pull Request, the default is to evaluate the state of the repository before and after the feature. Even in the small example, the refactor and introduction of a new object are conflated. This is more mental demand on the reviewer. You could argue that a refactor shouldn’t be included in the changeset for a feature, but that’s just not how things work. Often the review cycle is long enough that the overhead of requiring two reviews just won’t fly, and the author is probably hesitant to split their work into two. The second point is a cultural thing, but it’s certainly common enough.

Let’s return to the reviewer who has been tasked with this Pull Request. They click on the Files changed tab, and see a large diff across several files. While the changes are related in that they’re part of the single feature, they’re logically disconnected. Even worse is that they’re presented alphabetically across the repository. The reviewer doesn’t have the context of the author yet. Starting from the top, they might even be looking at meta configuration or workflow changes. The order in which they see the implementation of the new feature and its consumer is determined by their location in the repository.

The savvy reviewer is aware that they can view the individual commits comprising the Pull Request. This is a good thing, and it allows them to build up context in the order constructed by the author. They can build their review commit-by-commit, evaluating a change in its context: in the example, the refactor is presented before the new feature has been interspersed with it.

Unfortunately, I think our ideal story ends here. At this point, we find quite a few papercuts in the process. There’s a couple of options for the reviewer before they hand back to the author. They can remember to submit a batch of comments for each commit, which is quite a manual process. Alternatively, they can review the entire Pull Request in one go (hopefully it’s not a monster!). In both cases, the author can at least easily work through their proposal in a logical method: they can browse their own changes commit-by-commit and read the review. The first method is noisy, in that there will be several notifications. It’s not a terrible solution: the reviewer and author can be working in parallel. The second method doesn’t provide much context in the web interface: the comments that show up in the review won’t be attributed to their respective commits, unless you’re already viewing the commit!

There are a couple more downsides stemming from the web interface. The “suggestion” feature, where the reviewer can propose an alternative implementation, is unavailable except on the “all commits” view. There’s a blessing in disguise element to this, because the suggestion feature can often lead to some awful commit structure. In commit view, you can’t mark a file as “viewed”, so you can’t as easily put down and resume your review. And finally, the web interface and diff views are particularly slow!

People are bad at git

Unfortunate, but true. git is often a fairly thin abstraction on top of a directed acyclic graph of tree snapshots. In my opinion just knowing (and understanding) this goes a long way to having a good mental model of how to interact with a git repository. For some, however, the abstraction is the model. Commits are the only way to change things, history is immutable, and merges are free. In the context of Pull Requests, all three of these shortcomings significantly degrade the experience.

When people see git as a “bag of commits”, commits are (almost) the only way they interact with it. Even if they started with a the “ideal” commit structure for their feature (which it never does), it will quickly diverge. They create their Pull Request which fires off some Continuous Integration (CI) tests. The code linter fails on their proposed changes. They run their code formatter of choice, run git commit -a type “lint” into their editor (maybe they’ll use the -m flag, but unlikely) and push. As far as sins, this is hardly cardinal. Because it adds a new commit to the tip of the branch, it doesn’t really disturb the reviewer. It does disturb the equilibrium of logical commits, and it’s a sign of things to come.

Once the lint phase of the CI passes, the actual tests run. They fail: maybe the implementation is incorrect, maybe the test is too tight. Either way, a new “fix test” commit is introduced, with an empty commit message. The only way to understand what changed is to read the diff. This kind of commit is where we get into problem territory. On one hand, it is meritable on its own: it’s a logical change that corrects behaviour. In that case, it should have a full commit message that allows the reviewer to piece together the context of what failed and how it was fixed. On the other hand, it’s part of a pending feature, and the proposed change should be amended to include it.

People are scared of rewriting history

Amendment of existing changes falls into the category of “rewriting” history, and is a step too far for many people. There is in fact a (relatively) new git subcommand called git history that smooths over a lot of the history rewriting features so they’re not particularly difficult. I’d still argue that it’s a massive boon to have the mental model of what the commands are actually doing. At the moment, there are four operations that somewhat wrap an interactive rebase targeted at a specified commit:

  1. drop a commit from history, rebasing descendants onto its parent;
  2. fixup by applying staged changes to a commit;
  3. reword a commit’s message;
  4. split a given commit interactively (chosen hunks become part of a new commit, that is a parent of the remainder).

Of these, split and fixup would be very useful for preparing and modifying a Pull Request, respectively. Even with history rewriting, GitHub’s interface again doesn’t make things easy. It doesn’t support any notion of inter-commit diff. If you force push to a Pull Request branch, the changes are completely invisible to the previous revision. This in itself would discourage a history-rewriting workflow, leading to a pile-up of commits on top of the pull branch.

People are bad at git, part two: merges

The merge is a fundamental operation in version control, so I’m certainly not advocating for its removal. However, I think they’re significantly over- and misused in lieu of other operations. The most egregious misuse is to update a Pull Request branch from its parent by merging. Doing this clutters the branch history in a way that can make it very difficult to backtrack its lineage. If the Pull Request is resolved by a squash, this disappears entirely. Using “rebase and merge” will drop the merge commits, as long as there were no conflicts resolved by the merges. However, for my own sanity I tend to adopt a squash policy on repositories where it is likely that contributors will have unclean history on their branches.

What do we do?

At this point I’ve established, at least to myself, that the Pull Request is the wrong model for general contribution. This holds with or without good commit and history hygiene. GitHub has recently introduced the notion of a stacked Pull Request, where the base is another Pull Request itself. This introduces something of a parallel to a commit: when we don’t have commits as logical units, we can at least have Pull Requests that are. The advantage here is that the review workflow tips in the reviewer’s favour: they can browse and review the changes in a more logical order, building context naturally instead of having to tease it out. Beyond this, it just seems a bit clunky, and we already had the right level of abstraction for a logical change: a commit!

Use a different tool?

Personally, I’ve always found the idea of gerrit to match my expectations better. It works with discrete “changes”, and there’s easy support for seeing the inter-diff on a given changeset. As these things tend to go, it is polarising and I certainly don’t speak from experience of having used it in a proper team setting. However, as a way of managing incoming reviews that preserves context for me as a reviewer, I find it works much better for me.

After all the ranting, I only have a short set of steps for using gerrit as a personal review tool. The quickest/easiest start is to just run its container:

$ podman run -it -p 8080:8080 -p 29418:29418 \
  docker.io/gerritcodereview/gerrit

This is where things get a bit fiddly. Create a repository for the project you’re reviewing. Import its base, for example:

$ git remote add gerrit <gerrit-url>
$ git push -o skip-validation gerrit main:refs/heads/main

This will require a few permission changes to allow you to use skip-validation instead of treating them all as changes that gerrit should verify. Then, the Pull Request branch goes to a special namespace:

$ git push gerrit HEAD:refs/for/main

Now you can talk to yourself to your heart’s content while consuming the review. There’s a JSON API that can pull out the comments for given changes, per-commit. With a bit of fiddling with the GitHub REST and GraphQL APIs, we can write a script to import these and ensure they end up in the right place in the overall change set.

The GraphQL endpoint that allows a review comment on a change within a commit is deprecated (but has been for 3 years):

mutation($input: AddPullRequestReviewCommentInput!) {
  addPullRequestReviewComment(input: $input) {
    comment {
      id
    }
  }
}

Within the input, we have:

  • pullRequestReviewId: the node_id from a REST query to repos/<org>/<repo>/pulls/<id>/reviews;
  • commitOID: the commit to which the comment applies;
  • path: the path of the file in the commit;
  • position: a diff position (not the line number in the file) for the comment;
  • body: the comment itself.

What now?

This feels much nicer as a personal workflow. There are others: using Jujutsu we can have different branches for different revisions of a Pull Request, and add comments from our editor! This misses the diff-based nature of the GitHub and gerrit approaches, however. Of course, the biggest caveat for all of this is that it requires a reasonably well-structured Pull Request in the first place: a huge repository rewrite isn’t going to be tractable no matter what tool you use.