Re: [PATCH 1/2] t3507: pin CHERRY_PICK_HEAD absence for a conflicting --no-commit

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Sep 4, 2026, 13:53 UTC
Message-ID: <639f29ff-59f3-403d-acbe-e6173a8fbf04@gmail.com>
In-Reply-To: <apqSXT4lT7v0ILjp@pks.im>

On 04/09/2026 10:41, Patrick Steinhardt wrote:

Show 24 quoted lines

> On Fri, Sep 04, 2026 at 12:45:53AM +0300, Aleksei Sviridkin wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>>> It is not apparent what problem, if any, the description
>>> above claims the commit addresses.  Nor is it clear why
>>> checking these combinations is relevant.
>>> [...]
>>> Can you help me understand the above two paragraphs a bit better?
>>
>> The test pins the one combination t3507 did not cover. The file already
>> checks CHERRY_PICK_HEAD after a conflicting pick, after a clean pick, and
>> after a clean pick under --no-commit, but not after a conflicting pick
>> under --no-commit. That is the case a user hits by accident: the pick
>> stops on conflicts, they resolve and run "git commit", and the original
>> author is not restored. --no-commit never wrote the ref, d7e5c0cbfb skips
>> it on purpose. Your reading is right and Gemini's is backwards: under
>> --no-commit we do not want CHERRY_PICK_HEAD, and the test asserts it is
>> absent. Without it, teaching git to write the ref there would leave the
>> whole file green.
> 
> The question is whether it really makes sense to have tests for every
> single edge case. In a perfect world we of course would, but in the real
> world there are a) gazillions of different combinations and b) every
> test brings its own overhead as it increases both wall time and
> maintenance costs.

We should certainly be careful about adding too many tests - I often ask for tests to be revised to remove duplicate coverage when reviewing patches from enthusiastic contributors. In this case I think it is worth checking as we can do it by adding a single call to test_ref_missing to an existing test and the logic around when we do and do not write CHERRY_PICK_HEAD is a bit tricky.

Show 5 quoted lines

> That doesn't specifically mean that this one test you add here is not
> useful. But we need to have a better argument than "we didn't have it
> yet". For example we might've seen regressions, the logic is extremely
> fragile or we risk bad consequences like data loss or an unrecoverable
> situation if a property does not hold.

I agree we should have a more substantial justification when adding tests. As I said above I think in this case the justification is "the logic is tricky" and it is cheap to check it.

Show 5 quoted lines

> It's a thin line to walk at times, and I usually wouldn't care about
> this too much. But over the last couple weeks we've seen more patch
> series that add random tests to our test case without good reasoning
> just for the sake of adding a test. And that's something that we need to
> contain a bit.

Agreed

Thanks

Phillip