After a long time spent in management, I found myself switching back to an IC role in February. Since then, I’ve spent more time than I’d care to admit re-learning how to do all the things I used to do. Or…maybe not re-learning outright, but rebuilding habits and patterns that used to be automatic, but that I now have to think about.
One of those patterns involved code reviews. The place I work has a bias towards large, epic-sized PRs. If it’s been a minute since I’ve been an IC, it’s been an hour since I was actively using GitHub for reviews, and an aeon since I was reviewing truly massive PRs. While I was pleasantly surprised to find that GitHub has improved a lot in that area, it’s got a long way to go. You can mark a given file as reviewed, for example, but not a given chunk. You can’t really make notes to yourself in any way other than leaving draft comments, which means you need to be careful what and how you write notes lest you accidentally publish a comment saying, I dunno, “oy, we’ve got to fix Suebob’s dependency so this isn’t necessary”. And while you can zoom out to see a whole file instead of just the changes, you can’t easily navigate around the code base to understand how the PR fits into the larger whole.
Thankfully, I keep a developer’s journal where I write down the various problems I’ve hit over the years and how I’ve
approached them, so I quickly banged out hsh q review
and found my notes on doing large PRs. While not all of
them holds up (and some of them are specific to dealing with the vagaries of Phabricator and Gerrit), there’s enough
there that’s useful that I figured it’d be worth sharing.
Pull the changes locally
First things first: the best tool to understand code is the editor and tooling you use every day. If it’s a really trivial change, I’ll review it in GitHub directly, but otherwise, it’s almost always worth pulling down the changes locally and opening them up in a relevant IDE or editor. Once I’ve done that, I’ll see any warnings and suggestions that my environment offers, I’ll be able to use tooling like “find references”/“find overrides”/etc. to make sure that I understand the full impact of the change, I can run (or even debug, step through, or alter!) the code and tests, and so on. This sets me up in a much better place to understand what’s actually happening with the PR, and to verify it works as expected.
GitHub makes it really easy to grab changes locally with their CLI tool (it’s just gh co <PR number>
), but once you’ve
done so, you’ll note that git diff
shows nothing: there are no changes to look at because you’re up-to-date on the
feature branch. If you don’t do anything else, you’ll either need to keep the GitHub review open in another tab, or
you’ll need to keep the output of git diff main..
handy in another terminal window so you know what you should
actually be looking at.
Thankfully, there’s a much better way.
git reset --mixed
for fun and reviews
There’s a really easy trick to get PRs’ changes locally in such a way that Git itself, as well as your editor and
tooling, will show the changes. If you’ve ever messed up a merge, or wanted to throw out your work, you’ve probably used
git reset --hard main
, which discards all of your local changes and sets you back up on the trunk. But a great tool in
this case is to use is git reset --mixed
(a.k.a. the vanilla git reset
, without the --hard
) in the form of
git reset --mixed main
, which will leave the changes in your working copy, but reset both the index and HEAD
(i.e., what commit you’re working off of). Once you’ve done that, you’ll see that you now have all of the PR’s changes
showing in git diff
, ready to be staged. In many editors (including Visual Studio Code and Helix, the two I
use most often), you can even jump from change to change across the project, making it really easy to see what was
altered and how those changes fit into the project as a whole.
I use this pattern enough that I have a fish function, revpr
, for…well, “review PR”:
function revpr --description "check out a given PR for review"
gh co $argv[1]
git reset --mixed main
end
This is already a huge improvement: we can see what’s changed in our preferred tooling, as well as navigate around, debug, and experiment. But we haven’t solved leaving comments or tracking what we’ve viewed. Let’s solve those one at a time.
Stage hunks you’ve reviewed
One of the reasons I use git reset --mixed
instead of something like git reset --soft
is because it resets the
index. People like the index in the first place is because it lets you stage parts of a change, rather than just
unceremoniously committing all the changes at once. git add -p
and similar tools even let you add just parts of a
file to a commit. We can use that for our own purposes: in this case, if you add hunks as you review them, you can
easily see what you’ve already looked at via git diff
(stuff yet to be reviewed) versus git diff --staged
(stuff you
have reviewed). In many editors, including again the two I use, staged changes are shown differently than unstaged
changes, so I don’t even actually need to jump to the CLI to track where I am in the review.
Keep comments in the code
That just leaves where to put comments. My suggestion here is simple: put the comments in the code itself. I use two
specific forms for my comments to make them easy to find: // REVIEW(bmps)
are comments I want to put in GitHub, and
// NOTE(bmps)
are comments just for myself.[1]
Now the one part that’s annoying: once I’ve concluded the review, I find all of those review comments (you can use any
global find you want; I have a fish
function that just does rg -F 'REVIEW(bmps)'
) and…erm, well, manually copy
them into GitHub.
I’ll be honest: this is one part of the process that genuinely annoys me. I’ve looked at tools like prr for
inspiration on how to automate things, but I haven’t had any “eureka!” moment on how to do this without human
intervention. I will say that the ability to write review notes to myself however I want, knowing that I’ll have a
chance to manually review and edit everything before sharing with the author, does have some pretty strong
benefits—but so would just having a sort of review-your-review-by-hunk flow similar to git add -p
. That all said,
this annoyance is completely worth it to me due to all of the other benefits. It’s just not where I want it.
Suggestions on improving this part are always welcome in my inbox.
Use a separate Git worktree for reviews
Finally, one tip I that’s new to me as of this year (and really the past few months). All of the above has one major
drawback (at least, if you’re not using something like Jujutsu), which is that you have to either stash your
in-progress changes or throw them into a temporary commit before you review work. Yes, you can make an entirely
separate clone of every Git repo for reviews, but 1) that can eat a lot of disk space (one of the repos I work in most
often is a whopping 8.6 GiB), and 2) you have to constantly remember both to grab what you’re reviewing, and to
update main
, or else the git reset --mixed
step will result in you seeing a bunch of false changes.[2]
The fix for both is to use a git worktree specifically for reviews. If it’s a repo I’m not in much, I
usually won’t bother with one, and will just do the review right in the tree. But otherwise, a simple
git worktree add -d ../foobar-reviews
gives me a clean place to work whose branches will always be fully in sync, but
that won’t mess with my own in-progress code.
Future improvements
I’ve already called out manually copying comments over to GitHub as annoying and something I want to improve, but there are three other bits I’ve not been thrilled with.
First, I haven’t really found a good way to keep my comments locally as the review matures. With the above flow, from Git’s perspective, you’re basically squashing the entire PR, which puts you into a prime position to destroy the upstream PR and/or push up your inline comments. I’ve never gotten as far as actually doing that, but I’ve gotten uncomfortably close. I’m currently toying with a variant of the above where I make a branch to hold the review comments right when I start, and then squash/commit on that, but that ends up feeling like a lot of ceremony for relatively few benefits.
Regardless of which of the above approaches I pick, the following git rebase
when there are upstream changes
inevitably results in merge conflicts as my notes are addressed. This is actually not a huge deal, since the conflicts
will by definition be just with my review comments and thus be trivial to resolve, but it can sometimes feel like
more friction than I’d like. I feel as if a custom merge tool would probably handle this well, but I’m still chewing on
how exactly that merge tool would work and what it would actually do.
Finally, I can’t easily see what comments others have made.[3] In practice, this isn’t that annoying: I like to go through a review first without looking at what others have said and only then read their comments anyway, so it’s not a huge deal to do my review pass locally using the above workflow and then read GitHub afterwards, but I definitely feel like that sequence could be improved.[4]
That all said, for me, this flow ends up working very well in practice, and is a meaningful improvement on the vanilla GitHub PR flow for large changes. If you find yourself in a similar boat, I think this approach is worth a try.
I didn’t used to bother with the
// NOTE(bmps)
form, instead just doing plain comments for those, but I realized after awhile that my notes often evolve into review comments over time, so I started marking them separately so they’d be easier to find. ↩︎Strictly speaking,
git reset --mixed origin/main
works as long as you’ve donegit fetch
, which you have to do to grab the PR anyway, but getting that muscle memory right is tricky. ↩︎This is a problem that prr also suffers—which is really unfortunate, since having a mailing-list-style review is perfect for this exact case. I ultimately concluded that
prr
’s approach just isn’t one that appeals to me, but if I’d decided to stick with it, trying to add that functionality would’ve been a fun contribution. ↩︎It’s worth noting that Visual Studio Code can show GitHub review comments directly in the editor via the GitHub extension. If I used Visual Studio Code as my sole and primary editor, I’d be pretty tempted to look at that route more closely, but since my preferred editor is Helix, and I also spend a ton of time in PhpStorm, I prefer a workflow that works identically in all three places. ↩︎