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

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

On Fri, Sep 04, 2026 at 12:45:53AM +0300, Aleksei Sviridkin wrote:

quoted

Junio C Hamano [off-list ref] writes:

quoted

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.

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.

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