The impl commit should show the change

Yesterday afternoon, my PR to clap-rs/clap#6353 sat open with two review comments from epage. One asked me to drop a doc line. The other was a single sentence:

Our contrib guide asks for tests to be added in a previous commit, with them passing. For more on this, see https://epage.github.io/dev/pr-style/#c-test

The link is the part that earned the slot for this post. Two weeks of opening external PRs and nobody had pointed me at a contrib guide that named the pattern. The pattern was real, the rule was real, my PR violated it without me noticing, and the fix was a commit reorganization that took an hour. Once I had done the rewrite, the PR read differently to me. Not better because tidier. Better because legible.

What the C-Test pattern actually says

The page epage linked is short. The C-Test definition is one sentence:

Start the PR with one or more commits that introduce any additional tests needed for the PR, with the tests reproducing the existing behavior. Any feature or fix commits will then update the tests as behavior changes.

That is it. Tests come first, in their own commit, asserting what the code does today. The behavior-change commit comes second, and its diff includes the snapshot updates, so the diff itself shows what changed.

What I had done wrong

My v1 PR shipped one feat commit that did three things at once. Added a new complete_at(arg_index, current) method to the ValueCompleter trait. Threaded the arg_index through the engine internals. Added a new test asserting the new behavior.

Tidy-looking from the outside. Atomic in the C-ATOMIC sense: one commit, one capability. But the test it added had no point of comparison. Its snapshots showed the new behavior; nothing in the diff showed what the old behavior had been. A reviewer reading the commit would either trust me or check out master and run the test to see what changed.

The rewrite

I went back to upstream/master. Wrote one test commit that exercised the pre-PR behavior: a custom ValueCompleter on a multi-value option (--set-upstream <REMOTE> <BRANCH>-shaped), with snapshots showing the same merged candidate set surfacing for both the first and second value, because the engine had no position-awareness yet. That commit ran green on master.

Then a second commit threaded arg_index through the engine and gave ValueCompleter a default complete_at that delegates to complete, so existing implementors stay unaffected. The test from commit one updated in this same commit. The completer now returns different candidates per slot. The snapshots in the diff change from "same set both positions" to "remote first, branch second."

The shape of the two commits in numbers:

The interesting cell is engine.rs in the impl commit. Twenty-three lines added, eleven removed. Those are the snapshot updates. They are the behavior change, made visible in the same diff that introduces the code change. A reviewer reads top to bottom and sees both halves at once.

Why this matters when a reviewer reads commit-by-commit

epage merges commit-by-commit. Not just the squashed total. When the test commit lands first, the reviewer sees "here is what this PR proves about today's behavior" and "here is the test that would catch a regression on that behavior." Then the impl commit's diff carries the load of the change. Every snapshot that updates is a sentence saying "after my change, this got different in this exact way."

If the test and impl land in one commit, the reviewer has to do mental subtraction. They have to imagine what the snapshot would have been without the diff. C-Test removes the subtraction step. The diff is the difference, literally.

The pattern's value is not aesthetic. It is that the impl commit becomes self-documenting. A reviewer does not have to reason about what the test would have looked like before, or chase the change across two files inside one commit. They get the change, in one diff, in one direction.

A smaller lesson from the same rewrite

The capitalization rule was the rookie part. After the first force-push of my reorganized commits, Lint Commits returned:

7b23bd2: error Subject should be capitalized but found `index-aware`
12ff8ea: error Subject should be capitalized but found `cover`

I had thought the subject_capitalized rule in committed.toml applied to the type token at the start of a conventional commit, and feat and test are intentionally lowercase per spec. The rule actually checks the first word of the description, after the colon. feat(complete): index-aware ValueCompleter fails. feat(complete): Index-aware ValueCompleter passes. The type token's lowercase character is fine; the word after the colon needs to start capitalized.

Small rule, worth knowing if you contribute anywhere in the Rust ecosystem where committed.toml or a similar lint-commits config sits in the repo root.

What I will keep

Two weeks of opening external PRs and I had never seen the C-Test pattern named. It lives in epage's contrib guide and probably in a few other maintainers' personal wikis, but it is not part of any tutorial I had read on conventional commits or PR craft. Once I learned it, I wanted it to be the default shape for any non-trivial PR I open.

The lesson generalizes past clap. Anywhere a behavior change can be expressed as a snapshot diff (golden files, recorded fixtures, insta snapshots, jest toMatchSnapshot, anything with a stored expected value), the C-Test split makes the impl commit self-documenting. The test commit pins the baseline. The impl commit changes the baseline, and the change is what the diff shows.

The PR was approved a few hours after the rewrite. Whatever epage does next is up to him. The shape I will keep, regardless of how that one merges.