Skip to main content

Reviewable Commits and Pull-Request Shape

What This Concept Is​

Reviewable code is shipped as much as written. The effort put into commit history, PR size, and PR description is effort saved in review, in debugging, and in future archaeology. A well-shaped PR has three properties:

  • Right size. Small enough to review in one sitting (often 200-400 changed lines; rarely more).
  • One intent per commit. A commit does one thing that you can name in one line. A "refactor + new feature + unrelated formatting" commit destroys the history.
  • Clear description. The PR body tells a reviewer why, links the ADR or design doc, lists risk, and tells them how to verify.

Why It Matters Here​

A 1200-line PR with one commit named "stuff" is not a technical artifact; it is a favor you are asking the reviewer to do. It will receive a superficial review. It will later be impossible to git blame meaningfully. Its risk at merge is invisible.

A PR of the same feature split into four focused commits and described in six sentences gets a thorough review in less total time. The cost is up-front and on the author. That cost is the professional practice.

Concrete Example​

Bad commit history for a "promotions" feature:

a1b2c3d stuff
e4f5g6h more stuff
i7j8k9l fix
m0n1o2p PR feedback
q3r4s5t rename file
u6v7w8x final

Good commit history for the same feature:

0f3e9ab refactor: extract PricingStrategy interface
b82d441 feat(promotions): add Promotion and PromotionUse models
5a7c118 feat(promotions): add /promotions/apply endpoint (no caller yet)
e2d63fa feat(checkout): call promotions service behind FEATURE_PROMOS flag
19ab7cf test(promotions): concurrent-redemption regression test

Each commit subject uses a Conventional Commits-style type and a clear one-line summary. Each commit is a standalone, passing change.

Good PR description:

## Summary
Implements the `Promotions v1` design doc (`library/raw/design/promotions-v1.md`)
and ADR-0012 (code redemption concurrency).

## Changes
- New `promotions` service with `POST /promotions/apply`
- Concurrency via `SELECT ... FOR UPDATE`
- Called from `checkout` behind the `FEATURE_PROMOS` flag (default off)

## Risk and rollback
- Additive migration `20260311_promotions.sql` (no drops)
- Feature flag off at merge; rollback = flip flag or revert merge commit
- No schema changes to checkout tables

## How to verify
- `make test promotions/`
- Locally: `FEATURE_PROMOS=1 make run && curl -XPOST ...`

## Not in scope
- Loyalty discounts (LOYALTY-12)
- Multi-code combinations (follow-up PR)

A reviewer can start this review with a plan in twenty seconds.

Common Confusion / Misconception​

"Small PRs are more work." They are less work for the team. Small PRs get reviewed faster and more thoroughly; large PRs sit in queue, invite superficial approvals, and produce regressions that are expensive to isolate.

"History is fine; only the diff matters." That is only true until the first production bug. git bisect becomes useful only on clean history.

"Refactor + feature in the same commit." One of the most common failures. If the commit mixes them, reviewers cannot separate "I am changing the shape" from "I am changing behavior." Two commits.

"The PR description is in the tickets." It should not be. A reviewer should not have to open another system to start reviewing. Duplicate the key context in the PR body.

"Formatting / rename in the feature PR." Do a separate PR (or separate commit) for automated formatting and renames. Mixing them with logic changes makes the diff unreadable.

"Rewriting history is bad." Rewriting shared history is dangerous. Rewriting your own branch before merge (rebase, squash, reword) is good hygiene.

How To Use It​

Author checklist before pressing "ready for review":

  • Diff is under ~400 lines, or the PR description justifies why not.
  • Each commit builds and passes tests on its own.
  • Each commit subject is a short imperative sentence; no "WIP", "stuff", "more".
  • PR description covers Summary, Risk/Rollback, How to Verify, Not in Scope.
  • Linked: ADR(s), design doc, tracking ticket, dependent PRs.
  • Automated checks are green (see next concept).

When the PR is genuinely large (migration, framework upgrade), that is an exception -- say so in the description, and split the logically-separable parts into preceding PRs if you can.

Check Yourself​

  1. Why is "one intent per commit" a better rule than "fewer commits is neater"?
  2. What belongs in a PR description that does not belong in the commit messages?
  3. Give a case where a large PR is acceptable and what mitigates the size.

Mini Drill or Application​

Take your most recent real branch. Do three things:

  1. Rewrite its commit history into 3-7 commits, each one-intent.
  2. Write the PR description using the template above.
  3. Estimate how much time this would save a reviewer, and what it cost you.

Read This Only If Stuck​