Better Code Reviews with git History Rewriting

# Rewriting history with git
git rebase --interactive

The following post is a walkthrough on how I take a larger pull request, and break it down into well-ordered commits. At Mozilla, there is a pretty strong code review culture. Each line of code added to a project gets reviewed by a peer before it is merged in. There can be several cycles of code review where changes are requested. This happens asynchronously, as it’s a globally distributed organization. It can be tough coming into this environment from more of an agency dev environment, where anything goes. However, in order to ship quality code, the code review is an excellent process to find bugs, enforce consistency, and to spread the knowledge of new code across the team.

Large PRs are hard to review. When this happens, it’s frustrating for the reviewer. It’s hard to follow the logic of lots of changes. Despite this, it sometimes makes sense to send in a large PR, especially with large architectural changes. In order to get back a quality review, I like to split my larger code chunks into smaller ones.

Here’s how I do it.

The code

For this example, I’m going to illustrate how I take code I wrote a couple of weeks ago, and rewrite its history. Right now, it’s a single commit. But often, I have lots of messy commits. In this case, if I can’t make sense of my commit history, I will squash it all into one.

The code I’m working with is adding keyboard shortcuts for a feature called “call tree transforms”. It adds labels to the context menu showing the shortcuts, and generally cleans up the shortcuts to be more readable. The shortcuts work across several different panels in the profiler.

The current commit history:

24200dfa  (HEAD -> transform-shortcuts) Add call tree transform shortcuts
a8c2aaa9  (main) Add labels everywhere for the marker schema, and upgrade the DOMEvent marker (PR #2813)

The branch transform-shortcuts at commit 24200dfa is my work, and a8c2aaa9 is the main branch that is merged in upstream (which coincidentally is also my work from earlier.) The first thing I want to do is start an interactive rebase before my current commit.

Interactive rebasing

Normally when I rewrite my history, I use an interactive rebase. This will let me rewrite and re-order my commits The command (which I will type out in full) is as follows.

# Begin an interactive rebase, which will present a screen where the history of
# the commits can be modified.

git rebase --interactive main

However, I want to work before my current commit, so I’ll add the ^ modifier to main.

# main^ refers to the commit before main.

git rebase --interactive main^

My last commit was a merge commit. I’ll truncate the list a bit, but it looks like this in the interactive rebase page.

# ...
pick 2a574146 Address review for DOMEvent markers and marker labels
pick 3539c1c2 Add a FileIO marker to the marker table snapshot test
pick e7ea6e08 Add a bit more test coverage for empty or null marker schema
pick 24200dfa Add call tree transform shortcuts

I want to edit the commit before my current 24200dfa.

# ...
pick 2a574146 Address review for DOMEvent markers and marker labels
pick 3539c1c2 Add a FileIO marker to the marker table snapshot test

# Edit the following commit, by changing "pick" to edit"
edit e7ea6e08 Add a bit more test coverage for empty or null marker schema
pick 24200dfa Add call tree transform shortcuts

I save and exit vim with :wc.

Stopped at e7ea6e08...  Add a bit more test coverage for empty or null marker schema
You can amend the commit now, with

  git commit --amend

Once you are satisfied with your changes, run

  git rebase --continue

Now, I want to restore my work, and begin chopping it up. To do this, I use the handy git restore command. Historically, this was also the git checkout.

git restore . --source transform-shortcuts

My command will take the fully finished branch, and apply it to my working directory.

~/dev/profiler on profiler/(no branch, rebasing transform-shortcuts)
➤ git status
interactive rebase in progress; onto 0295f41c
...

Changes not staged for commit:
  (use "git add ..." to update what will be committed)
  (use "git restore ..." to discard changes in working directory)
    modified:   res/css/style.css
    modified:   res/img/svg/merge-icon.svg
    modified:   src/actions/profile-view.js
    modified:   src/components/calltree/CallTree.js
    modified:   src/components/flame-graph/FlameGraph.js
    modified:   src/components/network-chart/index.js
    modified:   src/components/shared/CallNodeContextMenu.css
    modified:   src/components/shared/CallNodeContextMenu.js
    modified:   src/components/shared/TreeView.js
    modified:   src/components/shared/VirtualList.js
    modified:   src/profile-logic/transforms.js
    modified:   src/test/components/CallNodeContextMenu.test.js
    modified:   src/test/components/__snapshots__/CallNodeContextMenu.test.js.snap

Coming up with the commit plan

Now begins the fun work. I take the list of files here, and copy them over to my editor to begin creating a commit plan of action. This is a relatively small changeset compared to some I’ve done this way, but it really helps to be explicit here.

res/css/style.css
res/img/svg/merge-icon.svg
src/actions/profile-view.js
src/components/calltree/CallTree.js
src/components/flame-graph/FlameGraph.js
src/components/network-chart/index.js
src/components/shared/CallNodeContextMenu.css
src/components/shared/CallNodeContextMenu.js
src/components/shared/TreeView.js
src/components/shared/VirtualList.js
src/profile-logic/transforms.js
src/test/components/CallNodeContextMenu.test.js
src/test/components/__snapshots__/CallNodeContextMenu.test.js.snap

I run git diff and read through my code changes. I annotate each file listing with what happened in that file.

res/css/style.css
  Fix some minor styling issues with the context menu
res/img/svg/merge-icon.svg
  Fix some minor styling issues with the context menu
src/actions/profile-view.js
  Add the redux action – handleCallNodeTransformShortcut
src/components/calltree/CallTree.js
  Teach the call tree how to use transform shortcuts
src/components/flame-graph/FlameGraph.js
  Teach the flame graph how to use transform shortcuts
src/components/network-chart/index.js
  Teach the call tree how to use transform shortcuts
src/components/shared/CallNodeContextMenu.css
  Fix some minor styling issues with the context menu
  Add a TransformMenuItem component that can use transform shortcuts
src/components/shared/CallNodeContextMenu.js
  Add a TransformMenuItem component that can use transform shortcuts
src/components/shared/TreeView.js
  Teach the call tree how to use transform shortcuts
src/components/shared/VirtualList.js
  Teach the call tree how to use transform shortcuts
src/profile-logic/transforms.js
  Unneeded whitespace change
src/test/components/CallNodeContextMenu.test.js
  Add a TransformMenuItem component that can use transform shortcuts
src/test/components/__snapshots__/CallNodeContextMenu.test.js.snap
  Add a TransformMenuItem component that can use transform shortcuts

Now I re-order the commit messages in the order I want them

Fix some minor styling issues with the context menu
Add the redux action – handleCallNodeTransformShortcut
Add a TransformMenuItem component that can use transform shortcuts
Teach the call tree how to use transform shortcuts
Teach the flame graph how to use transform shortcuts

Normally, I start working through my list, and delete items as I do them, but for this tutorial I will write up a complete plan on how to commit each one.

git add res/css/style.css
git add res/img/svg/merge-icon.svg
# Run through this interactively, as only part of it applies.
git add --patch src/components/shared/CallNodeContextMenu.css
git commit --message 'Fix some minor styling issues with the context menu'

git add src/actions/profile-view.js
git commit --message 'Add the redux action – handleCallNodeTransformShortcut'

git add src/components/shared/CallNodeContextMenu.css
git add src/components/shared/CallNodeContextMenu.js
git add src/test/components/CallNodeContextMenu.test.js
git add src/test/components/__snapshots__/CallNodeContextMenu.test.js.snap
git commit --message 'Add a TransformMenuItem component that can use transform shortcuts'

git add src/components/calltree/CallTree.js
git add src/components/network-chart/index.js
git add src/components/shared/TreeView.js
git add src/components/shared/VirtualList.js
git commit --message 'Teach the call tree how to use transform shortcuts'

git add src/components/flame-graph/FlameGraph.js
git commit --message 'Teach the flame graph how to use transform shortcuts'

#Unneeded whitespace change
git restore src/profile-logic/transforms.js

git add --patch

The most interesting line to call out here is:

git add --patch src/components/shared/CallNodeContextMenu.css

My CSS file changes included some that I wanted to attribute to two different commits. Here I use the --patch flag to signal to git that I want to decide which parts to add. --patch is available for a variety of commands, including the git restore command used earlier. It is the command that really makes me feel like I’m in control of moving chunks of code around.

--- a/CallNodeContextMenu.css
+++ b/CallNodeContextMenu.css
@@ -7,7 +7,8 @@
   margin-inline-end: 8px;
   pointer-events: auto;
 }
-.react-contextmenu-item--selected > .callNodeContextMenuIcon {
+.react-contextmenu-item:hover .callNodeContextMenuIcon,
+.react-contextmenu-item:active .callNodeContextMenuIcon {
   filter: invert(1);
 }

(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]? y

Here is an example of the decision I can make. At the end it asks me a question on how I want to proceed. Entering ? will list out the full commands available.

y - stage this hunk
n - do not stage this hunk
q - quit; do not stage this hunk or any of the remaining ones
a - stage this hunk and all later hunks in the file
d - do not stage this hunk or any of the later hunks in the file
e - manually edit the current hunk
? - print help

I chose y to “Yes, stage this hunk”. Another useful one is to split the hunks into smaller ones.

For example, my next code hunk included some that I wanted, and some that I didn’t for this commit.

@@ -31,8 +32,39 @@
 .callNodeContextMenuLabel {
   color: #555;
   font-weight: bold;
+  margin-inline: 2px;
 }

 .react-contextmenu-item--selected > .callNodeContextMenuLabel {
   color: #fff;
 }
+
+.react-contextmenu-item:hover .callNodeContextMenuLabel,
+.react-contextmenu-item:active .callNodeContextMenuLabel {
+  color: #fff;
+}
+
(2/2) Stage this hunk [y,n,q,a,d,K,g,/,s,e,?]? s

Here I chose s, for splitting the hunk up.

Split into 2 hunks.
@@ -31,8 +32,9 @@
 .callNodeContextMenuLabel {
   color: #555;
   font-weight: bold;
+  margin-inline: 2px;
 }

 .react-contextmenu-item--selected > .callNodeContextMenuLabel {
   color: #fff;
 }
(2/3) Stage this hunk [y,n,q,a,d,K,j,J,g,/,e,?]? y

Finally when I was done I chose q for quitting.

(1/1) Stage this hunk [y,n,q,a,d,e,?]? q

For more advanced situations, I like to use the e option to directly edit the diff of the hunk. This is helpful to break apart edits that are on the same line, or that won’t split apart nicely into hunks.

The committed code

After running through all of my commit plan, I have a list of well-ordered commits.

437f93c3  (HEAD) Teach the flame graph how to use transform shortcuts
88c178ee  Teach the call tree how to use transform shortcuts
4e859def  Add a TransformMenuItem component that can use transform shortcuts
4083211b  Add the redux action – handleCallNodeTransformShortcut
5ddd034b  Fix some minor styling issues with the context menu

I’m still in the interactive rebase mode, and my working directory is now clean.

git rebase --continue

Since I touched a merge commit in my history editing, I’ll run one final rebase against the main branch.

git rebase main

This will ensure the only changes are my new ones.

b8339103  (HEAD -> transform-shortcuts) Add call tree transform shortcuts
44278a4d  Teach the flame graph how to use transform shortcuts
6d189d53  Teach the call tree how to use transform shortcuts
2bb4c7a7  Add a TransformMenuItem component that can use transform shortcuts
0999c634  Add the redux action – handleCallNodeTransformShortcut
8cc0548f  Fix some minor styling issues with the context menu
a8c2aaa9  (main) Add labels everywhere for the marker schema, and upgrade the DOMEvent marker (PR #2813)

My original squashed commit is there: Add call tree transform shortcuts. It only contains the unneeded whitespace changes. I’ll get rid of it with a git reset --hard 44278a4d – although I could also do a git rebase --interactive main and drop the commit that way.

b8339103  (HEAD -> transform-shortcuts) Add call tree transform shortcuts
44278a4d  Teach the flame graph how to use transform shortcuts
6d189d53  Teach the call tree how to use transform shortcuts
2bb4c7a7  Add a TransformMenuItem component that can use transform shortcuts
0999c634  Add the redux action – handleCallNodeTransformShortcut
8cc0548f  Fix some minor styling issues with the context menu
a8c2aaa9  (main) Add labels everywhere for the marker schema, and upgrade the DOMEvent marker (PR #2813)

Now my commit history tells a nice story about what I’m adding with this new feature. I’ve taken the time to look through all of the code. Each commit is well-ordered. I can now go through and verify I have adequate test coverage for each commit, as the new changes are much more granular. I might go back and add more detailed, multi-line commit messages where it makes sense.

I add a new commit to the top of my stack with a test I was missing, and it’s ready for review.

git commit --message 'Add a test for call tree transform keyboard shortcuts'

The examples I gave of git commands were the full unabbreviated versions. I do this kind of work most days, so I have a list of aliases I prefer to use in order to do a bit less typing.

alias g="git"
alias gs='git status'
alias ga="git add"
alias gap="git add --patch"
alias gc="git commit -m"
alias gri="git rebase --interactive"
alias grim="git rebase --interactive main"
alias grc="git rebase --continue"
alias gra="git rebase --abort"

Why it’s worth the effort

In an organization with a strong focus on code quality, taking the time to break up commits like this results in a much smoother review process. In the past, I’ve sent in big messy commits, and a detailed reviewer struggled through it. They did not have a fun time with it, and then I struggled with figuring out how to apply the feedback. This happened multiple times. Once I took the time to learn how to nicely split up my commits, even if I sent in a large stack of commits to review, the conversation was much smoother. Sometimes it’s fine to struggle through a few tough rounds of code review, but an even better outcome is that the quality of the review received is much better. The reviewer can more easily read the code, and the decisions to modify things along the way. It brings more intentionality to the process.

This rewriting does take time, at times several hours, or even a full morning. This usually happens for projects that I’ve been working on for more than a week. For example, I can take a big piece of complicated architectural work, and split it out into 20 commits. The process to write that code was messy and confusing. However, after splitting it up into logical chunks of code, a clear story can emerge.

Update: other tools

As I get feedback from this post, I’ll share it below.

Another option beyond git rebase -i is git revise. It includes some quality improvements to some of the rewriting process, as well as some speed improvements due to avoiding working on the file system, and doing things directly in memory. I use this quite frequently nowadays to stage some changes with git add --patch and then run git revise fb92283a to revise a specific patch.

Many people have good success with git absorb, which is based off of hg absorb. It can help absorb changes into earlier patches. I find it works sometimes for me, but not always. When it does, it saves time on some of the manual work from above.