The tests here check the ref after a conflicting pick, after a clean pick and after a clean pick under --no-commit, but not after a conflicting one under --no-commit. That is the combination a user runs into by accident: the pick stops with conflicts, and the ref "git commit" would take the authorship from is not there.

Pin it next to its siblings. Letting the ref be written under --no-commit when the pick conflicts leaves the rest of the cherry-pick tests green, so nothing else guards that path.

Assisted-by: LLM
Signed-off-by: Aleksei Sviridkin <f@lex.la>
---
 t/t3507-cherry-pick-conflict.sh | 6 ++++++
 1 file changed, 6 insertions(+)

The list of what happens when a change is hard to apply states without qualification that CHERRY_PICK_HEAD is set. Under --no-commit it is not: d7e5c0cbfb (Introduce CHERRY_PICK_HEAD, 2011-02-19) skips the ref on purpose there, expecting the user to pick further commits and edit the result before committing.

The option's own description says nothing about the ref or about authorship. "git commit" reads the author of a cherry-pick from CHERRY_PICK_HEAD, so a commit made after "cherry-pick --no-commit" records your own identity as the author. Picking a single commit this way still leaves its log message in MERGE_MSG, so the result reads like a faithful pick apart from the author.

Assisted-by: LLM
Signed-off-by: Aleksei Sviridkin <f@lex.la>
---

Notes:
    The trap is sharpest in the use this option's own description
    recommends.  Picking several commits in a row leaves one commit that
    carries the last picked commit's message over the combined effect of
    all of them, under the committer's authorship, with nothing on screen
    to say so.  The added text scopes the "git commit -c" remedy to the
    single-commit case, since after several picks there is no one original
    author to restore.
    
    One more thing worth knowing when following that advice: if the picks
    were made with -x, the "(cherry picked from commit ...)" line lives in
    MERGE_MSG, and "git commit -c <commit>" replaces the message with the
    original commit's and drops the annotation.
    
    I left git-revert.adoc alone on purpose.  "revert --no-commit" does
    write REVERT_HEAD (t3507), so the two commands are asymmetric here, but
    a revert's authorship belongs to the reverter either way, so there is
    no equivalent consequence to document there.  Documentation/revisions.adoc
    describes CHERRY_PICK_HEAD without the --no-commit qualification as
    well; I can send that as a follow-up if it is wanted.

 Documentation/git-cherry-pick.adoc | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Aleksei Sviridkin <f@lex.la> 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.

I also fail to parse what the second paragraph intends to say. What does "it" refer to in "Pin it next to its siblings"? A new test? Any test inserted into a sequence will naturally sit adjacent to its neighbors, so calling them "its siblings" offers little clue to help the reader understand the change.

Can you help me understand the above two paragraphs a bit better?

Thanks.

P.S.

I shamelessly asked an AI agent I had nearby to guess what your log message might have meant and got the following. I am not sure if that matches what you wanted to say, or if it is totally off the mark, but at least I can follow what it is trying to say, even though I do not think if that matches reality (for example, when "--no-commit" is in effect, we probably do not want CHERRY_PICK_HEAD, even though the version of the text given by Gemini below claims it is needed).

    When a cherry-pick is run with the --no-commit option and halts
    due to conflicts, Git must still write the CHERRY_PICK_HEAD ref.
    This ref is necessary because a subsequent "git commit" relies
    on it to preserve the authorship metadata of the original
    commit.

    Add a new test alongside the existing cherry-pick tests to
    verify this behavior.  The test suite currently checks for
    CHERRY_PICK_HEAD after a conflicting pick, after a clean pick,
    and after a clean pick with --no-commit.  However, it lacks
    coverage for a conflicting pick with --no-commit.  Indeed, if
    Git is modified to stop writing the ref in this specific
    scenario, all existing tests still pass.  This new test closes
    the coverage gap.

Junio C Hamano <gitster@pobox.com> writes:

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 message was unclear, sorry. "it" was that missing case and "siblings" the three existing checks. v2 with a reworded message goes out once 24 hours have passed since v1.

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

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

On 03/09/2026 22:32, Junio C Hamano wrote:

No, with --no-commit it must not write CHERRY_PICK_HEAD. I agree the commit message is confusing and could be much shorter.

Thanks

Phillip
  >      This ref is necessary because a subsequent "git commit" relies

Hi Alexsei

On 03/09/2026 13:55, Aleksei Sviridkin wrote:

What does pinning a test mean?

We already have a test that checks the advice that's printed when there are conflicts, so could just add

	test_must_fail git show-ref --verify CHERRY_PICK_HEAD

there. Because that test checks the command's output, we know that the cherry-pick has failed due to conflicts, and not some other reason. Using test_must_fail() here without checking the error message means we don't verify the reason that the cherry-pick failed.

Thanks

Phillip

The list of what happens when a change is hard to apply states without qualification that CHERRY_PICK_HEAD is set. Under --no-commit it is not: d7e5c0cbfb (Introduce CHERRY_PICK_HEAD, 2011-02-19) skips the ref on purpose there, expecting the user to pick further commits and edit the result before committing.

The option's own description says nothing about the ref or about authorship. "git commit" reads the author of a cherry-pick from CHERRY_PICK_HEAD, so without it a plain commit records you, not the original author, as the author. Picking a single commit this way still leaves its log message in MERGE_MSG, so the result reads like a faithful pick apart from the author.

Assisted-by: LLM
Signed-off-by: Aleksei Sviridkin <f@lex.la>
---
Changes since v1:
  - dropped the t3507 test patch: the shared !opts->no_commit guard
    is already exercised by the existing clean-pick test, so it added
    no real coverage
  - keep only the doc clarification, and make the --no-commit entry
    self-contained

 Documentation/git-cherry-pick.adoc | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Patrick Steinhardt <ps@pks.im> writes:

Dropped the test. I went looking for that better argument and did not find one: t3507 already has 'cherry-pick --no-commit does not set CHERRY_PICK_HEAD' for the clean pick, and the clean and the conflicting path go through the same !opts->no_commit guard in do_pick_commit(), so the regression I described is covered already.

v2 is the documentation change alone.

Phillip Wood <phillip.wood123@gmail.com> writes:

Dropped the test, so the wording goes with it. "pin" was jargon, sorry.

Your placement was the right one: the advice test is what tells us the pick stopped on a conflict, which the bare test_must_fail did not. But the clean-pick test at t3507:98 and the conflicting case share the !opts->no_commit guard, so the assertion had no coverage left to add.

v2 is the doc change alone.

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

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.

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.

Agreed

Thanks

Phillip

On 04/09/2026 13:45, Aleksei Sviridkin wrote:

I don't follow this at all - where is the existing check that CHERRY_PICK_HEAD does not exist when "git cherry-pick --no-commit" stops for conflicts? I was suggesting that we add a check for that to the test "advice from failed cherry-pick --no-commit", I'd forgotten when I wrote my earlier email that we have a helper function test_ref_missing() to do just that.

Thanks

Phillip

Patrick Steinhardt <ps@pks.im> writes:

Very true. There needs a very good justification to add overhead to protect what has been working fine for a long time ;-).

Phillip Wood <phillip.wood123@gmail.com> writes:

You know that I know that ;-).

My point of asking an AI was to show that it was so unclear to confuse AI into summarizing it down to a complete opposite statement.

Aleksei Sviridkin <f@lex.la> writes:

While the added text does not say anything false, I am not sure if the last sentence hits the mark.

Maybe we should hint that this is a deliberate design decision behind the '--no-commit' option, perhaps in the description of that option?

The reason 'cherry-pick --no-commit <commit>' does not record <commit> in CHERRY_PICK_HEAD is that the command is meant to work as a better version [*] of 'git show <commit> | git apply'. The point of the operation is that you can continue to futz with the resulting modified working tree to build your own work, and in that context, you do not want the original authorship information.

So "When a single commit is ...", while not false, misses the point. After continuing to futz with the resulting modified working tree to build your own work, which may include picking (with the same '--no-commit' option) many more commits or writing your own code, you may still want to borrow a large part of the commit message from a commit, and 'git commit -c <borrowed-commit>' would be the natural thing to use.

But that advice belongs in the 'git commit' documentation, not the 'git cherry-pick' documentation.

Other than that, looking good.

Thanks.

[Footnote]
 * "better" because unlike patch application, it can use 3-way merge
   machinery to take the full file contents to wiggle the changes
   from a different context into the code that is currently checked
   out.

v2 was doc-only. I had dropped the test claiming the existing clean-pick test already covered it, which was wrong: nothing checks the ref after a conflicting --no-commit pick. 1/2 puts it back as a single test_ref_missing call in the existing conflicting-pick test.

2/2 drops the "git commit -c" advice, which belongs in git-commit documentation, and says instead that the missing ref is the point of the option rather than a wrinkle.

The Assisted-by trailer is gone from both.

Aleksei Sviridkin (2):
  t3507: check no CHERRY_PICK_HEAD after conflicting --no-commit
  doc: cherry-pick: note --no-commit skips CHERRY_PICK_HEAD

 Documentation/git-cherry-pick.adoc | 8 +++++++-
 t/t3507-cherry-pick-conflict.sh    | 3 ++-
 2 files changed, 9 insertions(+), 2 deletions(-)

base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc

Whether CHERRY_PICK_HEAD is written depends on the command, on whether the merge started, and on --no-commit, all in one condition in do_pick_commit(). The suite checks the clean --no-commit pick; nothing checks the conflicting one.

The test that already runs a conflicting --no-commit pick compares the advice the command prints, which is what tells us it stopped on a conflict. Assert the ref is missing there too.

Signed-off-by: Aleksei Sviridkin <f@lex.la>
---
 t/t3507-cherry-pick-conflict.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

The list of what happens when a change is hard to apply states without qualification that CHERRY_PICK_HEAD is set. Under --no-commit it is not: d7e5c0cbfb (Introduce CHERRY_PICK_HEAD, 2011-02-19) skips the ref on purpose there, presuming the user intends to further edit the result and possibly pick more commits on top.

The option's own description says nothing about the ref or about authorship. "git commit" reads the author of a cherry-pick from CHERRY_PICK_HEAD, so without it a plain commit records you as the author. Say so where the option is described, and say that this is the point of the option rather than a wrinkle: what is being built is the user's own work, not a reproduction of the original commit.

Signed-off-by: Aleksei Sviridkin <f@lex.la>
---
 Documentation/git-cherry-pick.adoc | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Phillip Wood <phillip.wood123@gmail.com> writes:

There is none. That was my error: the guard in do_pick_commit() is shared, but the suite only exercises the clean pick. v3 adds test_ref_missing CHERRY_PICK_HEAD to the conflicting advice test, the helper you pointed at.

Phillip Wood <phillip.wood123@gmail.com> writes:

That is the justification I should have given. Whether the ref is written depends on the command, on whether the merge started, and on --no-commit, all in one condition, and the conflicting --no-commit case is the one nothing checks. v3 states it that way.

Patrick Steinhardt <ps@pks.im> writes:

Correcting my earlier mail: I said I was dropping the test because the existing clean-pick test covered it, and that is wrong. The clean pick is checked, the conflicting one is not.

The justification is the condition itself: whether CHERRY_PICK_HEAD is written depends on the command, on whether the merge started, and on --no-commit, all in one place. v3 puts it back as a single test_ref_missing call in an existing test, so no new test and no new setup.