Act based on your goals

When figuring out the strictness of git policies, be it level of review or anything else, first assess the importance of the code you’re working on. Even within a highly important repo, there may be places (eg helpful debugging scripts) that don’t warrant the same rigour as the core.

Choose git commands based on what will make the git history readable, usually through the lens of what commit messages people end up looking at. Consider the following use-cases:

Running git blame to figure out where and why something was introduced
Looking at the latest commit message on a deployment dashboard
Skimming the commit history to get a sense of what some

Figure out what you want to optimize for, and decide from there. This applies to the rest of these notes: these are recommendations based on what I find useful, not hard rules that apply to every possible repo. There is a right answer for a certain user for a certain use-case, but there’s no final rule.

Squash when the unit of work is the PR, not the individual commits

For example, say you have a PR that adds a feature, and the individual commits are things like “Start adding thing”, “fix”, “whoops”, “fix 2”. The PR title is likely to give much more useful context than any of the individual commit messages about what you were doing and why you were doing it. Note that the PR itself

Squashing is still worth it even when the commit messages are well written if the unit of work is the PR: it’s fundamentally more useful to know about the feature than all the twists and turns you took when building that feature. The main place this does not hold is when the PR grows hugely in scope (eg adding many features). In that scenario, squashing can compact too much context.

PR titles should be held to a high standard that explains their purpose.

If you don’t squash, commit messages should be held to a high standard

When merging without squashing, your messages will be exposed directly to the end reader. They must be descriptive, explaining what the unit of work completed was.

BAD: “Fix”. Only way to understand what happened is the actual diff, and that won’t explain why a change was made.

GOOD: “Fix bug in maps preventing isobars from rendering”. This provides context — and if a new bug was introduced, you / an agent knows what to check to make sure you’re not reintroducing the old bug again when fixing it.

Rebasing is (usually) better than merging

Rebasing is conceptually clean, sticking commits onto the end as if you were on main the entire time. This avoids merge commits, which I generally think is worthwhile for cleaner git history. The main reason to merge is if you want to fix merge conflicts in an explicit merge commit (can be worthwhile with reconciling massively diverged branches, as it allows easier tracing of history).

I recommend setting up git to rebase by default:

git config --global pull.rebase true

NB: stash-and-rebase ocassionally has sharp edges related to overwriting changes, so if my branch is dirty and I want to pull I’ll usually:

git add -A # stage
git commit -m 'TMP' # commit
git pull --rebase # actually do the pull
git reset --soft HEAD~1 # undo my temporary commit

Increase iteration speed by making it easier to merge PRs

Latency with merging PRs doesn’t actually make them safer; it just slows the rate at which you can make changes. As review is by far the highest-latency component of getting a PR merged, optimize for minimizing the amount of review needed.

If your PRs take multiple rounds of review, that may be a sign that not enough work was done at the planning stage.

Pushing to main isn’t inherently bad

Ask yourself: is a PR review really just a security blanket — or will your PR even be reviewed in the first place? To be justified, the reviewer should fullfil at least a few of the following criteria:

Neurons activated. Are you putting real, deep thought into it or just a cursory look?
Be catching things proportionate to the amount of time spent. If the author feels confident in their changes (and has the skill to have very few unknown unknowns), a review is likely a waste of time.
Have opinions on architecture or code style.
Have insight into the purpose of the task. For example, a designer reviewing screenshots as part of a PR can often lead to improvements earlier.
Have knowledge of something the author doesn’t.
Care deeply about the codebase. If it’s throwaway prototyping, there’s no point.

It can be okay for a reviewer to focus only on the subset of the PR where those criteria will be met.

Note: if there’s tooling around PRs — like CI that runs before you can merge — it can still be appropriate to make a PR, but I generally view that as equivalent to pushing to main.

Make it clear to the reviewer what to focus on

Understand what your reviewer’s expertise is and where they add value relative to your own code. Manage your reviewer’s context window by guiding them to the places you’re most uncertain about.

Approvals can be conditional

An approval translates to “I don’t need to re-review”. Comments must still be addressed (or explicitly rejected) before merging, but this can spare an extra round of review.

You only need one reviewer

If you do need a review, you probably only need a single reviewer. It’s quite rare for multiple reviewers to actually be warranted; if they are, it’s often in a scenario where you want them looking at different things. In that scenario, make the different focuses clear when requesting a review.

Merge PRs as soon as they’re ready

Batching PRs will just make it harder to understand why prod suddenly has a bug. If your reviewer went through everything at once, so be it, but don’t artifically add latency to batch things.

It’s okay to defer work to future PRs

Make your PR a discrete unit of work. Do not try to add more and more and more features; get the original goal of the PR working, then make a follow-up. This is especially true when the code change is time sensitive. If you’re fixing a bug, getting the fix live sooner shouldn’t be blocked on non-critical followups to that.