GitHub’s code review tools are pretty good
We do all of our code review in GitHub’s pull requests. These have a bunch of nice features: they have nice syntax highlighting; they store comments against the PR, but also show those made on individual commits; any comments against lines which have changed subsequently are hidden by default; the comments support nice formatting; and more.
In some cases, they come up short
Imagine this pull request:
- Author adds commits A, B, and C.
- Reviewer adds some line comments, and some comments to the pull request as a whole.
- Author adds commits D and E to address those comments.
- Author asks Reviewer to look at the latest changes.
- Reviewer has a problem. How do they comment on just those commits?
- Repeat steps 2-6.
This can happen with or without a workflow based around rebasing, although heavy use of
--autosquash will tend to lead to more commits starting with fixup! or
squash!, as it vastly reduces the overhead of rewriting history later.
Currently available workarounds
In this case, there are several options, none of which are ideal:
- View the diff for the whole PR again (from the Files tab), and try to remember which parts were there before and which are new. If the latest changes are either a very large or a very small part of the overall diff, or the PR as a whole is small, this can work well. However, when both the initially-reviewed changes and the new changes are large, this can waste a lot of time and effort.
- Review the commits individually. As I said at the top of the page, any commit comments will show up in the PR along with those made on the PR directly. They don’t display as nicely, though, and if there are a lot of commits, this can get very time-consuming very quickly. Also, if commits D and E are new commits, and commit E changes a line that also changes in commit D, then it’s easy to make a comment on an already-outdated change.
- Use the GitHub UI or the CLI to compare the commits outside of the PR view, make notes of which lines need comments, and add them in the standard PR view after finding the lines there as well.
How to fix it
I would like GitHub to allow reviewers of a PR to select which changes they want to see, and use some intelligent defaults to give them relevant changes automatically.
That’s it.
I wrote Gitique, which is an extension for Chrome and Firefox, to demonstrate this. It does the stupidest possible thing that could work: given that every added line in the overall PR view must have the same content in the most recent change set, it styles lines to be context lines if they haven’t changed since the last review comment, and hides files which haven’t changed at all.
Here’s a quick demo:
Reviewable is a hosted tool which does this in a much more advanced way, with a very slick demo. It is much more fully-featured than Gitique: it handles rebases properly, allows for commit sets to be selected which don’t include the latest commit, and can expand and collapse sections individually.
For teams who want to adopt this wholesale, this looks like an excellent option. Gitique is basically a feature request, but Reviewable is an actual working product. (Gitcolony seems similar to Reviewable, but I haven’t tried it.) And if GitHub implemented this natively, I would be very happy.