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

From: Patrick Steinhardt <ps@pks.im>
Date: Sep 4, 2026, 09:41 UTC
Message-ID: <apqSXT4lT7v0ILjp@pks.im>
In-Reply-To: <20260903214553.53942-1-f@lex.la>

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

Show 17 quoted lines

> 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.

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.

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.

Thanks!

Patrick