GitHub pull request != code review

This article tries to summarize some of the problems that I have come across when trying to use GitHub pull requests for doing code reviews.

In summary: It was not all pleasant, and by the way I apologize for this rather harsh rant.

The (single) good thing first

The one thing that GitHub gets right is the tight integration with your Git work flow. Since pull requests are based on Git branches you can do things like:

• Push changes to the code review as regular commits, using any Git client (no special review tool plugin required for your Git client, for instance).
• Inspect individual Git commits.
• Easily pull the review branch to your local computer and try it out (build it, run tests, etc).
• Run server side builds and tests in the same way as you do with other builds from the repository (no special review tool integration required for your build bot, for instance).

And now the bad things…

The GitHub pull request functionality is more of a chat board than a review tool, and lacks many of the features of a useful code review tool.

Here are a few of the top things that bother me (I would like to be wrong, so please correct me if I am):

• It has NO support for recording review progress.
  • Want to mark a commit as reviewed? You can’t.
  • Want to mark a file as reviewed? You can’t.
  • Want to mark the entire pull request as reviewed? You can’t.
• It has NO support for tracking issues.
  • Want to raise an issue that must to be addressed before the review can be accepted? You can’t.
  • Want to mark a specific issue as fixed? You can’t.
  • Want to comment on a specific issue? Sometimes you can. Sometimes it’s hard. Sometimes you can’t.
  • Comments are hardly organized at all. They just appear in a long, flat, chronological list, making it very hard to see any relations between comments.
  • Different comments behave in different ways. E.g. a line comment on a commit has a very different UI compared a line comment on the squashed history.
• It has EXTREMELY POOR support for assigning different parts of the review to different people.
  • Want to make sure that your build system changes are reviewed by someone who knows it well? Well, you can sort of tell a person (if you know her/his name) in a comment to “Please look at the build system changes.” – but how do you know for sure that all the relevant changes were reviewed? You can’t.
  • If several people are reviewing the changes in a pull request, how do you know that all parts of the pull request have been reviewed, and no part was left unreviewed because everyone thought that “surely the other reviewer(s) will look at this”? You can’t.
• It has EXTREMELY POOR support for history rewrite.
  • Want to tidy up your commit history after the review (e.g. squash fixups into their parent commits, or fix some errors in the commit comments)? You actually can do that, using interactive rebase and then force push to the pull request branch, but…
    • Old commits are dropped from the pull request.
    • Comments on old commits (e.g. line comments) are dropped from the pull request.
    • And the killer: You can’t see if the rewritten history has additional changes compared to before the history rewrite. This is actually huge! Without any help from the review tool, you may need to do the code review all over again (Can you be sure that all the issues were fixed? The comments from before the weekend all disappeared. Did you write them down on paper or did you memorize them all? Did someone push a few more commits to the pull request since you looked at it the last time? Did the developer decide to sneak in a couple of fixes or even new features as part of the history rewrite?)
  • Want to rebase your topic branch on top of the latest master? You can, but… (that’s right, the same issues as above)
• As a reviewer you do not always get an email when there are changes to a pull request (even if you’re assigned to it). This is a really bad thing in a review tool, since you’ll either be wasting hours and days on waiting for a response from a person who didn’t know that there was something new to review, or you’ll have to spam the pull request with targeted @-style comments (just to be sure), adding to the already unmanageable pull request comment history.

To be fair, some of the functionality can be implemented as custom textual messages (e.g. write “All approved” to mark the review as approved, or tag your comments with custom lables, e.g. “Issue1: Please fix the…”, “Issue1: Fixed” etc), but that is just a bad workaround that is bound to fail, and it simply does not scale at all.

The alternative(s)

Actually, as far as I know there is currently no review tool that qualifies as a good Git code review tool, but that is a topic for another article.

The only Git review tool that comes near is Critic (for instance see this summary by Alexei Khlebnikov). However, it lacks documentation and you basically have to have Unix admin skills and a lot of time if you want to set it up and integrate it to your other systems (Git server, build systems, issue tracker, …). But if you are up for it, be sure to give it a try! (You can check out some Critic code reviews here).

There are of course others, but the ones I have come across so far (Gerrit, SmartBear Collaborator, Review Board, …) have no support for Git history (i.e. you can only review the squashed history), and/or poor Git integration (e.g. you have to upload diffs to a web server, rather than pushing commits to a Git repo), which pretty much make them useless in my book.