[PATCH] SubmittingPatches: avoid add-failing-test-then-fix anti-pattern

[PATCH] SubmittingPatches: avoid add-failing-test-then-fix anti-pattern

[Jon Simons]

Document the preference to avoid the 'add a failing test and fix it in a
subsequent commit' anti-pattern.  Although such a sequence is fine for
local development, reviewers instead prefer a single commit that
introduces the test together with its code change.

The goal of this addition is to capture the preference in writing, with
hopes of optimizing future review flows by reducing back-and-forths to
the list.

Signed-off-by: Jon Simons <jon@jonsimons.org>
---
 Documentation/SubmittingPatches | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index c60855f706..27b53d2b31 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -271,6 +271,16 @@ fixed by accident to avoid regression.  Also, try merging your work to
 that are still in flight may have unexpected interactions with what
 you are trying to do in your topic.
 
+When fixing a bug, you may find it useful to first write a test using
+`test_expect_failure` to confirm that the test exposes the bug.
+However, unless there is a strong reason not to, submit the test as
+`test_expect_success` in the same commit as your code change that
+fixes the bug.  A separate commit that only changes `test_expect_failure`
+to `test_expect_success` can leave the behavior being tested out of the
+diff context, making the fix harder to review.  Reviewers who want to
+verify that your test reproduces the bug can temporarily undo the code
+changes while keeping the test.
+
 Pushing to a fork of https://github.com/git/git will use their CI
 integration to test your changes on Linux, Mac and Windows. See the
 <<GHCI,GitHub CI>> section for details.
-- 
2.55.0

Re: [PATCH] SubmittingPatches: avoid add-failing-test-then-fix anti-pattern

[Kristoffer Haugsbakk]

On Sun, Oct 11, 2026, at 17:32, Jon Simons wrote:
> Document the preference to avoid the 'add a failing test and fix it in a
> subsequent commit' anti-pattern.  Although such a sequence is fine for
> local development, reviewers instead prefer a single commit that
> introduces the test together with its code change.

I have skimmed this document and indeed there does not seem to be an
existing mention of this point. And this is a useful point to mention.

I think the commit message should link to a few examples of this being
stated. Like one of the ones you seemed to have found in your previous
message.

>
> The goal of this addition is to capture the preference in writing, with
> hopes of optimizing future review flows by reducing back-and-forths to
> the list.

Sure, that’s always the case for additions to this file. ;)

>
> Signed-off-by: Jon Simons <jon@jonsimons.org>
> ---
>  Documentation/SubmittingPatches | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/Documentation/SubmittingPatches
> b/Documentation/SubmittingPatches
> index c60855f706..27b53d2b31 100644
> --- a/Documentation/SubmittingPatches
> +++ b/Documentation/SubmittingPatches
> @@ -271,6 +271,16 @@ fixed by accident to avoid regression.  Also, try
> merging your work to
>  that are still in flight may have unexpected interactions with what
>  you are trying to do in your topic.
>
> +When fixing a bug, you may find it useful to first write a test using
> +`test_expect_failure` to confirm that the test exposes the bug.
> +However, unless there is a strong reason not to, submit the test as
> +`test_expect_success` in the same commit as your code change that
> +fixes the bug.  A separate commit that only changes `test_expect_failure`
> +to `test_expect_success` can leave the behavior being tested out of the
> +diff context, making the fix harder to review.  Reviewers who want to
> +verify that your test reproduces the bug can temporarily undo the code
> +changes while keeping the test.

This is well-written.

Tis comes under `[[tests]]`, so it is appropriately placed. It also
comes after `[[separate-commits]]`, and it sort of functions as a
counterweight to making *too many* separate commits.

> +
>  Pushing to a fork of https://github.com/git/git will use their CI
>  integration to test your changes on Linux, Mac and Windows. See the
>  <<GHCI,GitHub CI>> section for details.
> --
> 2.55.0

Re: [PATCH] SubmittingPatches: avoid add-failing-test-then-fix anti-pattern

[Junio C Hamano]

Jon Simons <jon@jonsimons.org> writes:

> +When fixing a bug, you may find it useful to first write a test using
> +`test_expect_failure` to confirm that the test exposes the bug.
> +However, unless there is a strong reason not to, submit the test as
> +`test_expect_success` in the same commit as your code change that
> +fixes the bug.  A separate commit that only changes `test_expect_failure`
> +to `test_expect_success` can leave the behavior being tested out of the
> +diff context, making the fix harder to review.  Reviewers who want to
> +verify that your test reproduces the bug can temporarily undo the code
> +changes while keeping the test.

This indeed matches what contributors have been told to do.

But do we even need to say the first two and a half lines?  One
might find it convenient not to write any tests, or to be satisfied
with manual testing, or to send test_expect_failure() separately
and early.  None of these is what we recommend.

As you described, adding a test_expect_failure() test with a large
body alone in one patch, and fixing the code in another patch would
show a change from test_expect_failure() to test_expect_success()
without showing much of the body of the test in the second patch,
making the review unnecessarily cumbersome.  If the test is trivial
and small, it may fit within the diff context and avoid such a
problem, but then there is no need to add such a small test as a
failing one in a separate patch in the first place, unless the goal
is to inflate the commit count.

So, unless we can think of concrete cases that qualify as a
"strong reason", I do not think we want to write everything before
and up to "a strong reason not to" in your text.

How about a short and sweet sentence like this instead?

    A bugfix patch should include tests in test_expect_success()
    blocks that demonstrate the breakage when the code change that
    implements the fix is omitted.

Re: [PATCH] SubmittingPatches: avoid add-failing-test-then-fix anti-pattern

[Jon Simons]

On 10/11/26 7:44 PM, Junio C Hamano wrote:
> Jon Simons <jon@jonsimons.org> writes:
> 
>> +When fixing a bug, you may find it useful to first write a test using
>> +`test_expect_failure` to confirm that the test exposes the bug.
>> +However, unless there is a strong reason not to, submit the test as
>> +`test_expect_success` in the same commit as your code change that
>> +fixes the bug.  A separate commit that only changes `test_expect_failure`
>> +to `test_expect_success` can leave the behavior being tested out of the
>> +diff context, making the fix harder to review.  Reviewers who want to
>> +verify that your test reproduces the bug can temporarily undo the code
>> +changes while keeping the test.
> 
> This indeed matches what contributors have been told to do.
> 
> But do we even need to say the first two and a half lines?  One
> might find it convenient not to write any tests, or to be satisfied
> with manual testing, or to send test_expect_failure() separately
> and early.  None of these is what we recommend.
> 
> As you described, adding a test_expect_failure() test with a large
> body alone in one patch, and fixing the code in another patch would
> show a change from test_expect_failure() to test_expect_success()
> without showing much of the body of the test in the second patch,
> making the review unnecessarily cumbersome.  If the test is trivial
> and small, it may fit within the diff context and avoid such a
> problem, but then there is no need to add such a small test as a
> failing one in a separate patch in the first place, unless the goal
> is to inflate the commit count.
> 
> So, unless we can think of concrete cases that qualify as a
> "strong reason", I do not think we want to write everything before
> and up to "a strong reason not to" in your text.

Ack, I think this makes sense.  I do not have a list of concrete cases
that would fit a "strong reason" and be applicable in the general sense.

> How about a short and sweet sentence like this instead?
> 
>     A bugfix patch should include tests in test_expect_success()
>     blocks that demonstrate the breakage when the code change that
>     implements the fix is omitted.

Much crisper, I will apply this to a v2.


Thanks,
-Jon

[PATCH v2] SubmittingPatches: avoid add-failing-test-then-fix anti-pattern

[Jon Simons]

Document the preference to avoid the 'add a failing test and fix it in
a subsequent commit' anti-pattern.  Although such a sequence is fine
for local development, reviewers instead prefer a single commit that
introduces the test together with its code change [1].

The goal of this addition is to capture the preference in writing, with
hopes of optimizing future review flows by reducing back-and-forths to
the list.

[1] A couple examples of feedback, with rationale for avoiding
    the pattern, from August 2025 here:

    - https://lore.kernel.org/git/xmqqqzxpzo8b.fsf@gitster.g/
    - https://lore.kernel.org/git/xmqqfrdk3aqy.fsf@gitster.g/

Signed-off-by: Jon Simons <jon@jonsimons.org>
---
 Documentation/SubmittingPatches | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index c60855f706..3238a33932 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -271,6 +271,15 @@ fixed by accident to avoid regression.  Also, try merging your work to
 that are still in flight may have unexpected interactions with what
 you are trying to do in your topic.
 
+A bugfix patch should include tests in test_expect_success() blocks that
+demonstrate the breakage when the code change that implements the fix is
+omitted.  Submit tests as `test_expect_success` in the same commit as
+your code change that fixes the bug.  A separate commit that only changes
+`test_expect_failure` to `test_expect_success` can leave the behavior
+being tested out of the diff context, making the fix harder to review.
+Reviewers who want to verify that your test reproduces the bug can
+temporarily undo the code changes while keeping the test.
+
 Pushing to a fork of https://github.com/git/git will use their CI
 integration to test your changes on Linux, Mac and Windows. See the
 <<GHCI,GitHub CI>> section for details.
-- 
2.55.0

Re: [PATCH v2] SubmittingPatches: avoid add-failing-test-then-fix anti-pattern

[Junio C Hamano]

Jon Simons <jon@jonsimons.org> writes:

> +A bugfix patch should include tests in test_expect_success() blocks that
> +demonstrate the breakage when the code change that implements the fix is
> +omitted.  Submit tests as `test_expect_success` in the same commit as
> +your code change that fixes the bug.  A separate commit that only changes
> +`test_expect_failure` to `test_expect_success` can leave the behavior
> +being tested out of the diff context, making the fix harder to review.
> +Reviewers who want to verify that your test reproduces the bug can
> +temporarily undo the code changes while keeping the test.
> +
>  Pushing to a fork of https://github.com/git/git will use their CI
>  integration to test your changes on Linux, Mac and Windows. See the
>  <<GHCI,GitHub CI>> section for details.

Doesn't the second sentence "Submit tests ..." merely repeat what
the first sentence already says without adding much value?  After
re-reading them three times, I think "the same commit" is the only
new point.  I would condense the first two sentences down to

    A bugfix patch should add, in the same commit, code change and
    test_expect_success() blocks that demonstrate the breakage when
    the fix is omitted.

or something, if I were writing this from scratch.

By the way, I consider that the SubmittingPatches document is
primarily about giving readers a set of rules they should follow,
and not about teaching the reasoning behind a rule to such a deep
level, unless the rule requires judgment on when or when not to
apply it.  We agreed we do not need "unless there is a strong reason
not to", which means there is no judgment needed, so I was not
expecting to see the latter half of the paragraph.

Having said all that, i will queue this version as-is because we are
quickly reaching the point of diminishing returns.  Thanks.