Code reviews: the path less traveled

What if Pull Requests weren't linear?

Saša Jurić has a wonderful talk titled “Tell me a Story”, described as a “three-act monodrama that explores the quiet art of narrative structure in collaborative software work”. It's not available online yet, but if you have a chance to see it in-person, you should!

I won't spoil the theme here, but the talk in part touches on topic of code reviews using git and GitHub (or similar system, like GitLab): in a nutshell, how do you present and keep a coherent story of your changes for the reviewers.

For example: nice commit history

While I share the ideals, the reality starts to get messy when review feedback commits get added to the PR:

it's downhill from here

Saša does address that somewhat, but I still feel it's messy, cumbersome and tedious to deal with:

  1. You can merge the reviewed branch as-is, leaving the mess in your git history
  2. You can amend (fixup) individiual commits as per the feedback, which will produce different commits and reviewers will need to re-review everything on GitHub
  3. You can amend the commits after the PR is approved, cleaning up the branch before merging. If the feedback and rework is extensive, this can be hard to do and error-prone.
  4. You can squash everything while merging, which won't leave any trace of the discussion (Saša argues strongly against this)

I admit I often do 4 just because it's the easiest, and because I treat a branch as the atomic unit of work (once finished), but it does lead to large commits and often unrelated things creep in.

I resigned to live in this messy reality until I stumbled on Code Review Can Be Better post by Alex Kladov, which introduced me to interdiff reviews.

Interdiff reviews are a way to have your cake and eat it too:

  1. amend the relevant commits
  2. push everything as a separate series of commits
  3. the reviewer reviews the difference between the original series of commits and the new one
  4. repeat as needed
  5. merge the final reviewed series of commits, discard the intermediate ones

Intuitively this sounds like a good approach: each original commit is fixed where needed, the fixes are all reviewed, and the resulting git history looks nice and is easy to navigate.

This is all natively supported by git with the range-diff command.

Assuming the PR is made off of main branch, new-feature is the original PR branch and new-feature-v2 is fixed after the PR feedback: git range-diff main new-feature new-feature-v1 shows the changes between those two branches, per-commit.

A big downside of this approach is complete lack of support from Github, Gitlab, and other similar services. Given that many developers use one of these for PR reviews, it's hard to implement.

However, moving code reviews to local machine (perhaps helped with git worktree to avoid the pain of WIP branch changes) could allow curious teams to experiement and perhaps adopt the workflow.

One of the related challenges is where to hold the comments (discussion). The Code Review Can Be Better post mentions placing the review comments as code comments (the rationale is the comment should live next to the code that's being commented) and links to a very interesting talk on how Jane Street does code review.

I find the idea of PR-comments-in-code-comments intriguing: ideally, it does make sense! I don't think we're anywhere near there with the tooling though (Jane Street built their own).

I do want to explore the branch less traveled, though!