Check out the changes locally

For everything but the most trivial of patches, check change out locally. Not only is this a technical prerequisite for some of the other tips in this article, but I’ve found it is easier to remain focused on the review when it takes place outside of my email inbox/GitHub/Gitlab/etc.

Use more context when viewing changes

The default context for a diff is rather narrow. It will show lines added and removed next to only a few other lines of the code. This can reduce the efficacy of the review unless you would consider yourself already intimately familiar with the surrounding codebase without having it in front of you.

If you’re on the command line, the --unified=<n> or -U<n> flag can be passed to git-diff(1) and git-show(1) to show more context for the diff. Some graphical git tools show the entire context by default. I do almost everything git-related on the command line, but lately I have been using the gitk GUI client when reviewing patches because it shows the entire context for each patch and makes it easy for me to jump around from patch to patch.

This lets me see the entire contents of the files that the diff modifies. Expanding the context makes it particularly easy to see:

• if the change is stylistically consistent with the rest of the code;
• if there are any existing abstractions (types, functions, constants, etc) that could have been leveraged to better accomplish this change, but weren’t;
• if some other part of the code should have been updated to take advantage of this change, but wasn’t;
• if there’s a better home for this change

The list above is a great way to better familiarize yourself with the code while performing code reviews.

Exercise it yourself

Hitting the code paths that the patches modify is especially critical if the change didn’t come without any automated unit, integration, or end-to-end tests. If this is the case, should it have come with automated tests? Probably. This is very dependent on the thing you’re working on.

There are a number of benefits to figuring out how to exercise these code paths:

• It allows you to learn or verify what you know about the “flow” of the code. What modules/types/subsystems are crossed from start to finish to reach this code path? How do they communicate?
• It may help you spot potential improvements to the patch or confirm its correctness.
• If it is a customer/user visible change, try to empathize with them. What is the experience like for a non-technical user compared to a technical user?

Determine the change’s discoverability (if applicable)

Should this change include documentation? If so, does it already? If it’s a new subcommand to a command line tool, is it added to the tool’s usage or --help output? Is this an addition to some API that is meant to be consumed by other services or developers? Where will they learn about its existence?