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

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

[Aleksei Sviridkin]

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(+)

diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index 44596cb1e8..2ce2e88184 100755
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -100,6 +100,12 @@ test_expect_success 'cherry-pick --no-commit does not set CHERRY_PICK_HEAD' '
 	test_must_fail git rev-parse --verify CHERRY_PICK_HEAD
 '
 
+test_expect_success 'failed cherry-pick --no-commit does not set CHERRY_PICK_HEAD' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick --no-commit picked &&
+	test_must_fail git rev-parse --verify CHERRY_PICK_HEAD
+'
+
 test_expect_success 'cherry-pick w/dirty tree does not set CHERRY_PICK_HEAD' '
 	pristine_detach initial &&
 	echo foo >foo &&

base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc
-- 
2.55.0

[PATCH 2/2] doc: cherry-pick: note --no-commit skips CHERRY_PICK_HEAD

[Aleksei Sviridkin]

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(-)

diff --git a/Documentation/git-cherry-pick.adoc b/Documentation/git-cherry-pick.adoc
index 42b41923d5..d352e7e956 100644
--- a/Documentation/git-cherry-pick.adoc
+++ b/Documentation/git-cherry-pick.adoc
@@ -25,7 +25,8 @@ happens:
 1. The current branch and `HEAD` pointer stay at the last commit
    successfully made.
 2. The `CHERRY_PICK_HEAD` ref is set to point at the commit that
-   introduced the change that is difficult to apply.
+   introduced the change that is difficult to apply, unless the
+   `--no-commit` option was given.
 3. Paths in which the change applied cleanly are updated both
    in the index file and in your working tree.
 4. For conflicting paths, the index file records up to three
@@ -101,6 +102,11 @@ OPTIONS
 +
 This is useful when cherry-picking more than one commits'
 effect to your index in a row.
++
+Because `CHERRY_PICK_HEAD` is not recorded, the commit you make
+afterwards records you as its author.  When a single commit is picked
+this way, `git commit -c <commit>` keeps the original authorship and
+log message.
 
 -s::
 --signoff::
-- 
2.55.0

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

[Junio C Hamano]

Aleksei Sviridkin <f@lex.la> writes:

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

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.





>
> Assisted-by: LLM
> Signed-off-by: Aleksei Sviridkin <f@lex.la>
> ---
>  t/t3507-cherry-pick-conflict.sh | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
> index 44596cb1e8..2ce2e88184 100755
> --- a/t/t3507-cherry-pick-conflict.sh
> +++ b/t/t3507-cherry-pick-conflict.sh
> @@ -100,6 +100,12 @@ test_expect_success 'cherry-pick --no-commit does not set CHERRY_PICK_HEAD' '
>  	test_must_fail git rev-parse --verify CHERRY_PICK_HEAD
>  '
>  
> +test_expect_success 'failed cherry-pick --no-commit does not set CHERRY_PICK_HEAD' '
> +	pristine_detach initial &&
> +	test_must_fail git cherry-pick --no-commit picked &&
> +	test_must_fail git rev-parse --verify CHERRY_PICK_HEAD
> +'
> +
>  test_expect_success 'cherry-pick w/dirty tree does not set CHERRY_PICK_HEAD' '
>  	pristine_detach initial &&
>  	echo foo >foo &&
>
> base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc

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

[Aleksei Sviridkin]

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

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

[Patrick Steinhardt]

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.

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

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

[Aleksei Sviridkin]

Patrick Steinhardt <ps@pks.im> writes:
> 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".

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.

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

[Phillip Wood]

On 04/09/2026 10:41, Patrick Steinhardt wrote:
> 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.

> 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

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

[Aleksei Sviridkin]

Phillip Wood <phillip.wood123@gmail.com> writes:
> 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 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.

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

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

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

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

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

[Aleksei Sviridkin]

Patrick Steinhardt <ps@pks.im> writes:
> But we need to have a better argument than "we didn't have it yet".

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.

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

[Phillip Wood]

On 03/09/2026 22:32, Junio C Hamano wrote:
> Aleksei Sviridkin <f@lex.la> writes:
> 
> 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.

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
>      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.
> 
> 
> 
> 
> 
>>
>> Assisted-by: LLM
>> Signed-off-by: Aleksei Sviridkin <f@lex.la>
>> ---
>>   t/t3507-cherry-pick-conflict.sh | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
>> index 44596cb1e8..2ce2e88184 100755
>> --- a/t/t3507-cherry-pick-conflict.sh
>> +++ b/t/t3507-cherry-pick-conflict.sh
>> @@ -100,6 +100,12 @@ test_expect_success 'cherry-pick --no-commit does not set CHERRY_PICK_HEAD' '
>>   	test_must_fail git rev-parse --verify CHERRY_PICK_HEAD
>>   '
>>   
>> +test_expect_success 'failed cherry-pick --no-commit does not set CHERRY_PICK_HEAD' '
>> +	pristine_detach initial &&
>> +	test_must_fail git cherry-pick --no-commit picked &&
>> +	test_must_fail git rev-parse --verify CHERRY_PICK_HEAD
>> +'
>> +
>>   test_expect_success 'cherry-pick w/dirty tree does not set CHERRY_PICK_HEAD' '
>>   	pristine_detach initial &&
>>   	echo foo >foo &&
>>
>> base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc
> 

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

[Junio C Hamano]

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

> On 03/09/2026 22:32, Junio C Hamano wrote:
>> Aleksei Sviridkin <f@lex.la> writes:
>> 
>> 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.
>
> No, with --no-commit it must not write CHERRY_PICK_HEAD.

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.

> I agree the commit message is confusing and could be much
> shorter.

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

[Phillip Wood]

Hi Alexsei

On 03/09/2026 13:55, Aleksei Sviridkin wrote:
> 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.

What does pinning a test mean?

> 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(+)
> 
> diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
> index 44596cb1e8..2ce2e88184 100755
> --- a/t/t3507-cherry-pick-conflict.sh
> +++ b/t/t3507-cherry-pick-conflict.sh
> @@ -100,6 +100,12 @@ test_expect_success 'cherry-pick --no-commit does not set CHERRY_PICK_HEAD' '
>   	test_must_fail git rev-parse --verify CHERRY_PICK_HEAD
>   '
>   
> +test_expect_success 'failed cherry-pick --no-commit does not set CHERRY_PICK_HEAD' '
> +	pristine_detach initial &&
> +	test_must_fail git cherry-pick --no-commit picked &&

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
> +	test_must_fail git rev-parse --verify CHERRY_PICK_HEAD
> +'
> +
>   test_expect_success 'cherry-pick w/dirty tree does not set CHERRY_PICK_HEAD' '
>   	pristine_detach initial &&
>   	echo foo >foo &&
> 
> base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc

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

[Aleksei Sviridkin]

Phillip Wood <phillip.wood123@gmail.com> writes:
> What does pinning a test mean?
> [...]
> Using test_must_fail() here without checking the error message means
> we don't verify the reason that the cherry-pick failed.

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.

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

[Phillip Wood]

On 04/09/2026 13:45, Aleksei Sviridkin wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
>> What does pinning a test mean?
>> [...]
>> Using test_must_fail() here without checking the error message means
>> we don't verify the reason that the cherry-pick failed.
> 
> 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.

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

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

[Aleksei Sviridkin]

Phillip Wood <phillip.wood123@gmail.com> writes:
> 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?

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.

[PATCH v2] doc: cherry-pick: note --no-commit skips CHERRY_PICK_HEAD

[Aleksei Sviridkin]

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(-)

diff --git a/Documentation/git-cherry-pick.adoc b/Documentation/git-cherry-pick.adoc
index 42b41923d5..24a28d4e65 100644
--- a/Documentation/git-cherry-pick.adoc
+++ b/Documentation/git-cherry-pick.adoc
@@ -25,7 +25,8 @@ happens:
 1. The current branch and `HEAD` pointer stay at the last commit
    successfully made.
 2. The `CHERRY_PICK_HEAD` ref is set to point at the commit that
-   introduced the change that is difficult to apply.
+   introduced the change that is difficult to apply, unless the
+   `--no-commit` option was given.
 3. Paths in which the change applied cleanly are updated both
    in the index file and in your working tree.
 4. For conflicting paths, the index file records up to three
@@ -101,6 +102,11 @@ OPTIONS
 +
 This is useful when cherry-picking more than one commits'
 effect to your index in a row.
++
+This option does not record `CHERRY_PICK_HEAD`, so a plain `git commit`
+afterwards records you, not the original author, as the author.  When a
+single commit is picked this way, `git commit -c <commit>` keeps the
+original authorship and log message.
 
 -s::
 --signoff::

base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc
-- 
2.55.0

Re: [PATCH v2] doc: cherry-pick: note --no-commit skips CHERRY_PICK_HEAD

[Junio C Hamano]

Aleksei Sviridkin <f@lex.la> writes:

> diff --git a/Documentation/git-cherry-pick.adoc b/Documentation/git-cherry-pick.adoc
> index 42b41923d5..24a28d4e65 100644
> --- a/Documentation/git-cherry-pick.adoc
> +++ b/Documentation/git-cherry-pick.adoc
> @@ -25,7 +25,8 @@ happens:
>  1. The current branch and `HEAD` pointer stay at the last commit
>     successfully made.
>  2. The `CHERRY_PICK_HEAD` ref is set to point at the commit that
> -   introduced the change that is difficult to apply.
> +   introduced the change that is difficult to apply, unless the
> +   `--no-commit` option was given.
>  3. Paths in which the change applied cleanly are updated both
>     in the index file and in your working tree.
>  4. For conflicting paths, the index file records up to three
> @@ -101,6 +102,11 @@ OPTIONS
>  +
>  This is useful when cherry-picking more than one commits'
>  effect to your index in a row.
> ++
> +This option does not record `CHERRY_PICK_HEAD`, so a plain `git commit`
> +afterwards records you, not the original author, as the author.  When a
> +single commit is picked this way, `git commit -c <commit>` keeps the
> +original authorship and log message.

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.

[PATCH v3 0/2] cherry-pick: document that --no-commit skips CHERRY_PICK_HEAD

[Aleksei Sviridkin]

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

[PATCH v3 1/2] t3507: check no CHERRY_PICK_HEAD after conflicting --no-commit

[Aleksei Sviridkin]

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(-)

diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index 44596cb1e8..aa004d929b 100755
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -79,7 +79,8 @@ test_expect_success 'advice from failed cherry-pick --no-commit' "
 	EOF
 	test_must_fail git cherry-pick --no-commit picked 2>actual &&
 
-	test_cmp expected actual
+	test_cmp expected actual &&
+	test_ref_missing CHERRY_PICK_HEAD
 "
 
 test_expect_success 'failed cherry-pick sets CHERRY_PICK_HEAD' '
-- 
2.55.0

[PATCH v3 2/2] doc: cherry-pick: note --no-commit skips CHERRY_PICK_HEAD

[Aleksei Sviridkin]

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(-)

diff --git a/Documentation/git-cherry-pick.adoc b/Documentation/git-cherry-pick.adoc
index 42b41923d5..f4cd8b9db7 100644
--- a/Documentation/git-cherry-pick.adoc
+++ b/Documentation/git-cherry-pick.adoc
@@ -25,7 +25,8 @@ happens:
 1. The current branch and `HEAD` pointer stay at the last commit
    successfully made.
 2. The `CHERRY_PICK_HEAD` ref is set to point at the commit that
-   introduced the change that is difficult to apply.
+   introduced the change that is difficult to apply, unless the
+   `--no-commit` option was given.
 3. Paths in which the change applied cleanly are updated both
    in the index file and in your working tree.
 4. For conflicting paths, the index file records up to three
@@ -101,6 +102,11 @@ OPTIONS
 +
 This is useful when cherry-picking more than one commits'
 effect to your index in a row.
++
+This option does not record `CHERRY_PICK_HEAD`, so a plain `git commit`
+afterwards records you as the author.  That is by design: what you are
+building is your own work, which you keep changing before committing,
+rather than a reproduction of the original commit.
 
 -s::
 --signoff::
-- 
2.55.0