Reorganize Patches For Review

General

Overview

In the Firefox code base, and in many other Mozilla code bases, commits are reviewed individually. There is no "review squashed series" workflow. Many individual small commits are easier to understand than one large commit. Submitting small commits for review helps catch bugs during review. And in the case that a regression does get introduced, having a fully working product after every commit makes it easy to bisect which change caused the bug or performance regression.

Goals

• Smoothe out the landing process by avoiding known sources of review and landing friction.
• Reduce cognitive load during review, so that it's easy for the reviewer to spot bugs, and to understand the impact of a change.
• Every "prefix" of the patch series should leave the world in a meaningful valid state.
• Every patch should be an incremental improvement that makes sense to a reviewer in isolation.
• Every patch should look "natural" and not depend on later work to justify its existence.

Sources of review friction

• Unclear value proposition, if the patch only looks natural once combined with other work in the series
• Mixing of unrelated concerns, e.g. functionality changes and cleanup, or changes made by a tool (e.g. global search and replace + fmt) and other changes
• Messy intermediate states that gets cleaned up in a later commit
• Large number of reviewers on a single patch, if the patch touches files across multiple directories with distinct review responsibilities

Strategies to minimize friction

• "Land-early nuggets": Often, a larger change will include small tweaks which are orthogonal to the goal of the larger change, but which are non-controversial and which make sense to adopt even if the larger change is rejected. Find those nuggets of value and pull them out into patches that go before the larger work - they can be reviewed quickly and will reduce the amount of rebasing that has to happen for the larger change if that one is stuck in review for a while.
• Refactor first, then change behavior: In the process of writing a patch, the main focus is usually on a certain behavior change, and then sometimes some cleanup is done afterwards. But during review, it's usually better to do the "cleanup" / refactor first, in such a way that the actual behavior change that follows will look very natural.
• Predict reviewer response: Do a review of each patch yourself, and predict what a reviewer would say about it. Then shift things around until you think that the reviewer will have nothing to complain about - or at least until they could only disagree with the effect of the patch / the proposed change, and not with the mechanics of the implementation.
• Put behavior changes front-and-center: In patches which change behavior, minimize distractions from unrelated changes.

Multiple bugs

Sometimes it can make sense to split the series across multiple Bugzilla bugs. Here, the term "bug" is used loosely in the sense of "patch subseries container" / "unit of landing". You can use "Bug TBF-consolidate-rdm-styles - [...]" placeholders in the commit message (TBF = to-be-filed). When done, give the user a list of bugs that need to be filed prior to patch submission, and let them know which placeholders they will need to substitute.

Another approach is to put all patches on the same bug first, and move them out as needed for individual landings. More concretely: As the reviews come in, the developer can move a "fully-reviewed prefix" of the patch series into a different bug and land them there, while the remaining patches stay in the original bug where they wait for the rest of the reviews to come in. The goal is to have one landing per bug. With Phabricator, patches can be moved across bugs without losing the review status.

Trade-offs

Some of the goals above pull in opposite directions. This section acknowledges that there are some judgment calls involved.

For example, there are often multiple options when making a change to a widely-used API:

• Option 1: In the same commit, change both the API as well as all consumers of that API across all directories. Good: atomic change, passes build, no messy intermediate state. Bad: large patch, large number of reviewers
• Option 2: One commit for the API change, multiple commits (grouped by reviewer) for updating consumers. Good: Small patch, low number of reviewers per patch. Bad: Intermediate state fails to build
• Option 3: One commit makes the API change but keeps a compatibility stub so that existing callers still work, and then multiple patches convert various directories, with a final patch removing the compat stub. Good: Small patch, low number of reviewers per patch, passes build. Bad: somewhat messy intermediate state with compat stub, risk that compat stub stays around indefinitely.

Option 2 should be avoided. Prefer option 1 here in most cases, unless there are so many consumers that updating them all in a single commit is impractical. Option 3 becomes more attractive if the intermediate state still appears somewhat natural.

Going overboard on patch splitting can backfire: Too many individual patches can be a drag to review. Depending on the situation it can be a good idea to group similar changes into the same commit. If a refactoring looks non-sensical without the associated behavior change, it can make sense to do both in the same commit.

FAQ

What should be moved to the front of the series?

• Front-load non-controversial improvements.
• Front-load risk: If the entire larger change is doomed if one of its pieces doesn't work out, and if that deal-breaker piece can be validated independently, it can make sense to get just that piece reviewed and landed first. Then it can go through Nightly testing while the specifics of the rest are still being discussed.
• Front-load changes which don't change behavior but which, by being separate, improve the clarity of upcoming behavior change patches.

What should I do if, in the process of reorganizing patches, I suddenly notice an entirely new and better implementation approach which was non-obvious before? Should I implement the better approach instead?

For this skill, the goal is to have a patch series whose overall diff exactly matches the original overall diff. So resist the temptation to switch approaches; if the temptation is unbearable, ask the user for permission first.

For cosmetic differences, you can have a "residue" patch at the end of the series which makes the diffs match, but which the user is free to abandon.

Mechanics

This skill has two phases: 1. Envision, and 2. Execute.

Phase 1: Envision

Preconditions

Before you start, ensure a clean starting state with no uncommited changes and no conflicts in the original commit series. If the repo is a jj (Jujutsu) repo, run jj st and jj log -r 'main..@'. Also note the current op-id (jj op log -n1) so you can jj op restore if anything goes sideways.

Then follow these steps:

1. If the original commit series is small, do a quick review of the original commit series. If it's clear that the patches are already clean, well-ordered, and ready for review, you're done.
2. Remember how to get the overall diff of the original commit series. E.g. jj diff --git --from oty --to yuxp --at-op aab4
3. Make a list of the original commits. For each commit:

• List which files are touched by the commit
• List which "logical units" the patch consists of. E.g. individual cleanups, orthogonal behavior changes, plumbing, refactors.

4. The hard part: Brainstorm various orderings of the logical units, regardless of what original commit the unit of change was originally part of. Here you create a fresh "origin story" for the final state, and this new origin story should satisfy all the goals above. This process can sometimes some time. One challenge is that you need to keep many different states of the code base in your head at the same time. For example, comments in earlier patches can't refer to concepts that only get introduced in later patches, because that would create a non-sensical intermediate state.
5. Settle on an ideal organization, think of commit messages.

Example:

• Commits A, B, C with logical units A: [M, N, O], B: [P], C: [Q, R, S].
• Settled on ideal organization: R, M, Q, P, [N, O], S.