Better Code Reviews with Mercurial History Rewriting
# Rewriting history with hg
hg histedit --interactive
This is a companion piece to the Better Code Reviews with git History Rewriting, but this time with mercurial. The intro from that post works for this one as well with a few minor changes:
The following post is a walkthrough on how I take a larger [changeset], 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 [patch queue], 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 that ended up touching a few different components, and then separate it out into a well-order list of commits. My patch is dealing with an API migration of profiler markers from “Markers 1.0” to “Markers 2.0”. In addition, I’m back-filling in some tests that were missing.
Mozilla has an hg wip command that will show a diagram of where you are with various revisions. I will use this when I get confused, but often will prefer a custom hg log I created that I call hg stack. It lists everything out nicely as the current stack of commits since central. I will use that for demonstrating the operations.
The current commit history:
3160069417e0 Greg (*fileio-markers-2-0) WIP - Markers 2.0 FileIO markers
e2b3376280d3 Gerald (phab-D93738) Bug 1671536 - Remove BaseProfilerMarkerPayload.h and dependents - r?gregtatum
c2fa6d5d1092 Gerald Bug 1671536 - Stop using baseprofiler::TextMarkerPayload - r?gregtatum
a7939b97db9f Gerald Bug 1670954 - MarkerThreadId::MainThread() - r?gregtatum
a1f7120fc7d5 Gerald Bug 1670954 - profiler_main_thread_id() and profiler_is_main_thread() - r?gregtatum
a6aaf0c8e183 Csoregi (central) Backed out 4 changesets (bug 1633712) for leaks. CLOSED TREE
I have a single WIP patch with all of my work in it that I’ve been amending onto. This represents about a day and half of engineering work. This is bookmarked at (fileio-markers-2-0). It is based off of a peer’s work that has not been landed yet, which is bookmarked at (phab-D93738). I got this patch on to my local machine using moz-phab patch D93738 -r central, and then ran hg graft to get my work ontop of his. My hg stack (my custom hg log) command shows the main upstream commit, which is (central). This is the latest from Mozilla.
histedit
Mercurial has some great rewriting capability with the histedit command. I will do that now.
# Run histedit which will consider the revisions that have not been merged in yet.
hg histedit
?: help, k/up: move up, j/down: move down, space: select, v: view patch
d: drop, e: edit, f: fold, m: mess, p: pick, r: roll
pgup/K: move patch up, pgdn/J: move patch down, c: commit, q: abort
#0 pick 556188:a1f7120fc7d5 556188 Bug 1670954 - profiler_main_thread_id() and profiler_is_main_thread() - r?gregtatum
#1 pick 556189:a7939b97db9f 556189 Bug 1670954 - MarkerThreadId::MainThread() - r?gregtatum
#2 pick 556190:c2fa6d5d1092 556190 Bug 1671536 - Stop using baseprofiler::TextMarkerPayload - r?gregtatum
#3 pick 556191:e2b3376280d3 556191 Bug 1671536 - Remove BaseProfilerMarkerPayload.h and dependents - r?gregtatum
#4 pick 556195:3160069417e0 556195 WIP - Markers 2.0 FileIO markers
I am presented with the following interactive screen, which includes my co-workers work.
Here I will choose to edit my revision, by highlighting it and tapping e.
#0 pick 556188:a1f7120fc7d5 556188 Bug 1670954 - profiler_main_thread_id() and profiler_is_main_thread() - r?gregtatum
#1 pick 556189:a7939b97db9f 556189 Bug 1670954 - MarkerThreadId::MainThread() - r?gregtatum
#2 pick 556190:c2fa6d5d1092 556190 Bug 1671536 - Stop using baseprofiler::TextMarkerPayload - r?gregtatum
#3 pick 556191:e2b3376280d3 556191 Bug 1671536 - Remove BaseProfilerMarkerPayload.h and dependents - r?gregtatum
#4 edit 556195:3160069417e0 556195 WIP - Markers 2.0 FileIO markers
The revisions grow down, which is the opposite as my log command. Hitting c will “commit” my histedit session, and I can begin. I am presented with the following information.
~/dev/gecko on fileio-markers-2-0
➤ hg histedit
performing changes
9 files updated, 0 files merged, 1 files removed, 0 files unresolved
Editing (3160069417e0), you may commit or record as needed now.
(hg histedit --continue to resume)
Next an hg status will show me what I am dealing with.
➤ hg status
M mozglue/baseprofiler/public/BaseProfilerMarkersPrerequisites.h
M tools/profiler/core/ProfilerMarkerPayload.cpp
M tools/profiler/gecko/ProfilerIOInterposeObserver.cpp
M tools/profiler/public/ProfilerMarkerPayload.h
M tools/profiler/tests/gtest/GeckoProfiler.cpp
M tools/profiler/tests/shared-head.js
M tools/profiler/tests/xpcshell/head.js
M tools/profiler/tests/xpcshell/test_feature_mainthreadio.js
M tools/profiler/tests/xpcshell/xpcshell.ini
A tools/profiler/tests/xpcshell/test_feature_fileioall.js
# The repository is in an unfinished *histedit* state.
# To continue: hg histedit --continue
# To abort: hg histedit --abort
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.
Now I read through everything in an hg diff and annotate my list.
mozglue/baseprofiler/public/BaseProfilerMarkersPrerequisites.h
Migrated FileIO markers to the Markers 2.0 API
tools/profiler/core/ProfilerMarkerPayload.cpp
Remove FileIOMarkerPayload
tools/profiler/gecko/ProfilerIOInterposeObserver.cpp
Migrated FileIO markers to the Markers 2.0 API
tools/profiler/public/ProfilerMarkerPayload.h
Remove FileIOMarkerPayload
tools/profiler/tests/gtest/GeckoProfiler.cpp
Remove FileIOMarkerPayload
tools/profiler/tests/shared-head.js
tools/profiler/tests/xpcshell/head.js
Add more comprehensive tests for FileIO markers
tools/profiler/tests/xpcshell/test_feature_mainthreadio.js
Add more comprehensive tests for FileIO markers
tools/profiler/tests/xpcshell/xpcshell.ini
Add more comprehensive tests for FileIO markers
tools/profiler/tests/xpcshell/test_feature_fileioall.js
Add more comprehensive tests for FileIO markers
I have a pretty good commit plan here, and can begin interactive committing it.
hg commit --interactive
hg has some brilliant tools around picking chunks of code. This histedit ended up being pretty straightforward, and I can select the files I want to include in a commit, and I didn’t need to select things per-line in a file.
[x]** mozglue/baseprofiler/public/BaseProfilerMarkersPrerequisites.h
[ ]** tools/profiler/core/ProfilerMarkerPayload.cpp
[x]** tools/profiler/gecko/ProfilerIOInterposeObserver.cpp
[ ]** tools/profiler/public/ProfilerMarkerPayload.h
[ ]** tools/profiler/tests/gtest/GeckoProfiler.cpp
[ ]** tools/profiler/tests/shared-head.js
[ ]** tools/profiler/tests/xpcshell/head.js
[ ]** tools/profiler/tests/xpcshell/test_feature_fileioall.js
[ ]** tools/profiler/tests/xpcshell/test_feature_mainthreadio.js
[ ]** tools/profiler/tests/xpcshell/xpcshell.ini
Here I’ve chosen two files, and then hit c to “confirm” and create the commit.
I get presented with the standard commit dialog in my editor, which I can fill out with the appropriate Mozilla-formatted commit message. I actually hit the wrong button the first time, and ran :q! in vim to discard the interactive commit. However, the second time I got it correct, and was able to commit my code with a brief description.
1 Bug 1671701 - Migrated FileIO markers to the Markers 2.0 API; r?gerald
2
3 This commit uses the new Markers 2.0 API for FileIO Markers. I had to
4 create another option for the MarkerStack class in order to conditionally
5 capture a backtrace inside of the Macro. Otherwise the macro invocation
6 failed.
7
8 HG: Enter commit message. Lines beginning with 'HG:' are removed.
9 HG: Leave message empty to abort commit.
10 HG: --
11 HG: user: Greg Tatum
12 HG: branch 'default'
13 HG: changed mozglue/baseprofiler/public/BaseProfilerMarkersPrerequisites.h
14 HG: changed tools/profiler/gecko/ProfilerIOInterposeObserver.cpp
Now I continue on in this process for the other commits, and my hg diff gets smaller and smaller.
My second commit I actually missed a file, but it was pretty easy to add it to the previous commit. I ran the following, and I can select new things to add to the previous commit.
hg commit --amend --interactive
If I missed something a few commits back, I would probably create a “fixup” commit, and go run hg histedit again at the end of the process.
My last commit doesn’t need to be interactive, as it’s all that’s left. It’s good enough to run:
➤ hg status
M tools/profiler/tests/shared-head.js
M tools/profiler/tests/xpcshell/head.js
M tools/profiler/tests/xpcshell/test_feature_mainthreadio.js
M tools/profiler/tests/xpcshell/xpcshell.ini
A tools/profiler/tests/xpcshell/test_feature_fileioall.js
# The repository is in an unfinished *histedit* state.
# To continue: hg histedit --continue
# To abort: hg histedit --abort
~/dev/platform-gecko2 at 14:51:04
➤ hg commit
One more hg status reveals:
➤ hg status
# The repository is in an unfinished *histedit* state.
# To continue: hg histedit --continue
# To abort: hg histedit --abort
# Continue on with the histedit, which completes it.
➤ hg histedit --continue
# My custom "hg log" command.
➤ hg stack
f0c45dbc6bb8 Greg (*fileio-markers-2-0) Bug 1671701 - Add more comprehensive tests for FileIO markers; r?gerald
dabf610c4822 Greg Bug 1671701 - Remove FileIOMarkerPayload; r?gerald
8c0cb763ca53 Greg Bug 1671701 - Migrated FileIO markers to the Markers 2.0 API; r?gerald
e2b3376280d3 Gerald (phab-D93738) Bug 1671536 - Remove BaseProfilerMarkerPayload.h and dependents - r?gregtatum
c2fa6d5d1092 Gerald Bug 1671536 - Stop using baseprofiler::TextMarkerPayload - r?gregtatum
a7939b97db9f Gerald Bug 1670954 - MarkerThreadId::MainThread() - r?gregtatum
a1f7120fc7d5 Gerald Bug 1670954 - profiler_main_thread_id() and profiler_is_main_thread() - r?gregtatum
a6aaf0c8e183 Csoregi (central) Backed out 4 changesets (bug 1633712) for leaks. CLOSED TREE
My rewriting is complete. Time to submit it for review, and trigger some testing runs to validate my new tests.
Why it’s worth the effort
In summary, I will repeat what I said in my last article on the subject.
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.
