{"thread":{"id":"66263","subject":"[PATCH 1/2] t3507: pin CHERRY_PICK_HEAD absence for a conflicting --no-commit","startedAt":"2026-09-03T12:55:27Z","lastAt":"2026-09-05T17:13:53Z","messageCount":21,"participants":["Aleksei Sviridkin","Junio C Hamano","Patrick Steinhardt","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"551860","messageId":"20260903125524.67889-1-f@lex.la","threadId":"66263","inReplyTo":null,"subject":"[PATCH 1/2] t3507: pin CHERRY_PICK_HEAD absence for a conflicting --no-commit","fromName":"Aleksei Sviridkin","fromEmail":"f@lex.la","sentAt":"2026-09-03T12:55:23Z","receivedAt":"2026-09-03T12:55:27Z","isPatch":true,"body":"The tests here check the ref after a conflicting pick, after a clean\npick and after a clean pick under --no-commit, but not after a\nconflicting one under --no-commit. That is the combination a user\nruns into by accident: the pick stops with conflicts, and the ref\n\"git commit\" would take the authorship from is not there.\n\nPin it next to its siblings. Letting the ref be written under\n--no-commit when the pick conflicts leaves the rest of the cherry-pick\ntests green, so nothing else guards that path.\n\nAssisted-by: LLM\nSigned-off-by: Aleksei Sviridkin \n---\n t/t3507-cherry-pick-conflict.sh | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\nindex 44596cb1e8..2ce2e88184 100755\n--- a/t/t3507-cherry-pick-conflict.sh\n+++ b/t/t3507-cherry-pick-conflict.sh\n@@ -100,6 +100,12 @@ test_expect_success 'cherry-pick --no-commit does not set CHERRY_PICK_HEAD' '\n \ttest_must_fail git rev-parse --verify CHERRY_PICK_HEAD\n '\n \n+test_expect_success 'failed cherry-pick --no-commit does not set CHERRY_PICK_HEAD' '\n+\tpristine_detach initial &&\n+\ttest_must_fail git cherry-pick --no-commit picked &&\n+\ttest_must_fail git rev-parse --verify CHERRY_PICK_HEAD\n+'\n+\n test_expect_success 'cherry-pick w/dirty tree does not set CHERRY_PICK_HEAD' '\n \tpristine_detach initial &&\n \techo foo >foo &&\n\nbase-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc\n-- \n2.55.0\n\n"},{"id":"551861","messageId":"20260903125524.67889-2-f@lex.la","threadId":"66263","inReplyTo":"20260903125524.67889-1-f@lex.la","subject":"[PATCH 2/2] doc: cherry-pick: note --no-commit skips CHERRY_PICK_HEAD","fromName":"Aleksei Sviridkin","fromEmail":"f@lex.la","sentAt":"2026-09-03T12:55:24Z","receivedAt":"2026-09-03T12:55:29Z","isPatch":true,"body":"The list of what happens when a change is hard to apply states without\nqualification that CHERRY_PICK_HEAD is set. Under --no-commit it is\nnot: d7e5c0cbfb (Introduce CHERRY_PICK_HEAD, 2011-02-19) skips the ref\non purpose there, expecting the user to pick further commits and edit\nthe result before committing.\n\nThe option's own description says nothing about the ref or about\nauthorship. \"git commit\" reads the author of a cherry-pick from\nCHERRY_PICK_HEAD, so a commit made after \"cherry-pick --no-commit\"\nrecords your own identity as the author. Picking a single commit this\nway still leaves its log message in MERGE_MSG, so the result reads\nlike a faithful pick apart from the author.\n\nAssisted-by: LLM\nSigned-off-by: Aleksei Sviridkin \n---\n\nNotes:\n The trap is sharpest in the use this option's own description\n recommends. Picking several commits in a row leaves one commit that\n carries the last picked commit's message over the combined effect of\n all of them, under the committer's authorship, with nothing on screen\n to say so. The added text scopes the \"git commit -c\" remedy to the\n single-commit case, since after several picks there is no one original\n author to restore.\n \n One more thing worth knowing when following that advice: if the picks\n were made with -x, the \"(cherry picked from commit ...)\" line lives in\n MERGE_MSG, and \"git commit -c \" replaces the message with the\n original commit's and drops the annotation.\n \n I left git-revert.adoc alone on purpose. \"revert --no-commit\" does\n write REVERT_HEAD (t3507), so the two commands are asymmetric here, but\n a revert's authorship belongs to the reverter either way, so there is\n no equivalent consequence to document there. Documentation/revisions.adoc\n describes CHERRY_PICK_HEAD without the --no-commit qualification as\n well; I can send that as a follow-up if it is wanted.\n\n Documentation/git-cherry-pick.adoc | 8 +++++++-\n 1 file changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-cherry-pick.adoc b/Documentation/git-cherry-pick.adoc\nindex 42b41923d5..d352e7e956 100644\n--- a/Documentation/git-cherry-pick.adoc\n+++ b/Documentation/git-cherry-pick.adoc\n@@ -25,7 +25,8 @@ happens:\n 1. The current branch and `HEAD` pointer stay at the last commit\n successfully made.\n 2. The `CHERRY_PICK_HEAD` ref is set to point at the commit that\n- introduced the change that is difficult to apply.\n+ introduced the change that is difficult to apply, unless the\n+ `--no-commit` option was given.\n 3. Paths in which the change applied cleanly are updated both\n in the index file and in your working tree.\n 4. For conflicting paths, the index file records up to three\n@@ -101,6 +102,11 @@ OPTIONS\n +\n This is useful when cherry-picking more than one commits'\n effect to your index in a row.\n++\n+Because `CHERRY_PICK_HEAD` is not recorded, the commit you make\n+afterwards records you as its author. When a single commit is picked\n+this way, `git commit -c ` keeps the original authorship and\n+log message.\n \n -s::\n --signoff::\n-- \n2.55.0\n\n"},{"id":"551915","messageId":"xmqq7bl29g2p.fsf@gitster.g","threadId":"66263","inReplyTo":"20260903125524.67889-1-f@lex.la","subject":"Re: [PATCH 1/2] t3507: pin CHERRY_PICK_HEAD absence for a conflicting --no-commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-03T21:32:14Z","receivedAt":"2026-09-03T21:32:17Z","isPatch":true,"body":"Aleksei Sviridkin writes:\n\n> The tests here check the ref after a conflicting pick, after a clean\n> pick and after a clean pick under --no-commit, but not after a\n> conflicting one under --no-commit. That is the combination a user\n> runs into by accident: the pick stops with conflicts, and the ref\n> \"git commit\" would take the authorship from is not there.\n>\n> Pin it next to its siblings. Letting the ref be written under\n> --no-commit when the pick conflicts leaves the rest of the cherry-pick\n> tests green, so nothing else guards that path.\n\nIt is not apparent what problem, if any, the description\nabove claims the commit addresses. Nor is it clear why\nchecking these combinations is relevant.\n\nI also fail to parse what the second paragraph intends to say.\nWhat does \"it\" refer to in \"Pin it next to its siblings\"? A new\ntest? Any test inserted into a sequence will naturally sit\nadjacent to its neighbors, so calling them \"its siblings\" offers\nlittle clue to help the reader understand the change.\n\nCan you help me understand the above two paragraphs a bit better?\n\nThanks.\n\nP.S.\n\nI shamelessly asked an AI agent I had nearby to guess what your log\nmessage might have meant and got the following. I am not sure if\nthat matches what you wanted to say, or if it is totally off the\nmark, but at least I can follow what it is trying to say, even\nthough I do not think if that matches reality (for example, when\n\"--no-commit\" is in effect, we probably do not want CHERRY_PICK_HEAD,\neven though the version of the text given by Gemini below claims it\nis needed).\n\n When a cherry-pick is run with the --no-commit option and halts\n due to conflicts, Git must still write the CHERRY_PICK_HEAD ref.\n This ref is necessary because a subsequent \"git commit\" relies\n on it to preserve the authorship metadata of the original\n commit.\n\n Add a new test alongside the existing cherry-pick tests to\n verify this behavior. The test suite currently checks for\n CHERRY_PICK_HEAD after a conflicting pick, after a clean pick,\n and after a clean pick with --no-commit. However, it lacks\n coverage for a conflicting pick with --no-commit. Indeed, if\n Git is modified to stop writing the ref in this specific\n scenario, all existing tests still pass. This new test closes\n the coverage gap.\n\n\n\n\n\n>\n> Assisted-by: LLM\n> Signed-off-by: Aleksei Sviridkin \n> ---\n> t/t3507-cherry-pick-conflict.sh | 6 ++++++\n> 1 file changed, 6 insertions(+)\n>\n> diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\n> index 44596cb1e8..2ce2e88184 100755\n> --- a/t/t3507-cherry-pick-conflict.sh\n> +++ b/t/t3507-cherry-pick-conflict.sh\n> @@ -100,6 +100,12 @@ test_expect_success 'cherry-pick --no-commit does not set CHERRY_PICK_HEAD' '\n> \ttest_must_fail git rev-parse --verify CHERRY_PICK_HEAD\n> '\n> \n> +test_expect_success 'failed cherry-pick --no-commit does not set CHERRY_PICK_HEAD' '\n> +\tpristine_detach initial &&\n> +\ttest_must_fail git cherry-pick --no-commit picked &&\n> +\ttest_must_fail git rev-parse --verify CHERRY_PICK_HEAD\n> +'\n> +\n> test_expect_success 'cherry-pick w/dirty tree does not set CHERRY_PICK_HEAD' '\n> \tpristine_detach initial &&\n> \techo foo >foo &&\n>\n> base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc\n"},{"id":"551918","messageId":"20260903214553.53942-1-f@lex.la","threadId":"66263","inReplyTo":"xmqq7bl29g2p.fsf@gitster.g","subject":"Re: [PATCH 1/2] t3507: pin CHERRY_PICK_HEAD absence for a conflicting --no-commit","fromName":"Aleksei Sviridkin","fromEmail":"f@lex.la","sentAt":"2026-09-03T21:45:53Z","receivedAt":"2026-09-03T21:45:57Z","isPatch":true,"body":"Junio C Hamano writes:\n> It is not apparent what problem, if any, the description\n> above claims the commit addresses. Nor is it clear why\n> checking these combinations is relevant.\n> [...]\n> Can you help me understand the above two paragraphs a bit better?\n\nThe test pins the one combination t3507 did not cover. The file already\nchecks CHERRY_PICK_HEAD after a conflicting pick, after a clean pick, and\nafter a clean pick under --no-commit, but not after a conflicting pick\nunder --no-commit. That is the case a user hits by accident: the pick\nstops on conflicts, they resolve and run \"git commit\", and the original\nauthor is not restored. --no-commit never wrote the ref, d7e5c0cbfb skips\nit on purpose. Your reading is right and Gemini's is backwards: under\n--no-commit we do not want CHERRY_PICK_HEAD, and the test asserts it is\nabsent. Without it, teaching git to write the ref there would leave the\nwhole file green.\n\nThe message was unclear, sorry. \"it\" was that missing case and\n\"siblings\" the three existing checks. v2 with a reworded message goes\nout once 24 hours have passed since v1.\n"},{"id":"551938","messageId":"apqSXT4lT7v0ILjp@pks.im","threadId":"66263","inReplyTo":"20260903214553.53942-1-f@lex.la","subject":"Re: [PATCH 1/2] t3507: pin CHERRY_PICK_HEAD absence for a conflicting --no-commit","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-09-04T09:41:49Z","receivedAt":"2026-09-04T09:41:57Z","isPatch":true,"body":"On Fri, Sep 04, 2026 at 12:45:53AM +0300, Aleksei Sviridkin wrote:\n> Junio C Hamano writes:\n> > It is not apparent what problem, if any, the description\n> > above claims the commit addresses. Nor is it clear why\n> > checking these combinations is relevant.\n> > [...]\n> > Can you help me understand the above two paragraphs a bit better?\n> \n> The test pins the one combination t3507 did not cover. The file already\n> checks CHERRY_PICK_HEAD after a conflicting pick, after a clean pick, and\n> after a clean pick under --no-commit, but not after a conflicting pick\n> under --no-commit. That is the case a user hits by accident: the pick\n> stops on conflicts, they resolve and run \"git commit\", and the original\n> author is not restored. --no-commit never wrote the ref, d7e5c0cbfb skips\n> it on purpose. Your reading is right and Gemini's is backwards: under\n> --no-commit we do not want CHERRY_PICK_HEAD, and the test asserts it is\n> absent. Without it, teaching git to write the ref there would leave the\n> whole file green.\n\nThe question is whether it really makes sense to have tests for every\nsingle edge case. In a perfect world we of course would, but in the real\nworld there are a) gazillions of different combinations and b) every\ntest brings its own overhead as it increases both wall time and\nmaintenance costs.\n\nThat doesn't specifically mean that this one test you add here is not\nuseful. But we need to have a better argument than \"we didn't have it\nyet\". For example we might've seen regressions, the logic is extremely\nfragile or we risk bad consequences like data loss or an unrecoverable\nsituation if a property does not hold.\n\nIt's a thin line to walk at times, and I usually wouldn't care about\nthis too much. But over the last couple weeks we've seen more patch\nseries that add random tests to our test case without good reasoning\njust for the sake of adding a test. And that's something that we need to\ncontain a bit.\n\nThanks!\n\nPatrick\n"},{"id":"551939","messageId":"5a5c7552-8fc8-48be-abf6-063aa31f7711@gmail.com","threadId":"66263","inReplyTo":"xmqq7bl29g2p.fsf@gitster.g","subject":"Re: [PATCH 1/2] t3507: pin CHERRY_PICK_HEAD absence for a conflicting --no-commit","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-09-04T09:53:29Z","receivedAt":"2026-09-04T09:53:33Z","isPatch":true,"body":"On 03/09/2026 22:32, Junio C Hamano wrote:\n> Aleksei Sviridkin writes:\n> \n> I shamelessly asked an AI agent I had nearby to guess what your log\n> message might have meant and got the following. I am not sure if\n> that matches what you wanted to say, or if it is totally off the\n> mark, but at least I can follow what it is trying to say, even\n> though I do not think if that matches reality (for example, when\n> \"--no-commit\" is in effect, we probably do not want CHERRY_PICK_HEAD,\n> even though the version of the text given by Gemini below claims it\n> is needed).\n> \n> When a cherry-pick is run with the --no-commit option and halts\n> due to conflicts, Git must still write the CHERRY_PICK_HEAD ref.\n\nNo, with --no-commit it must not write CHERRY_PICK_HEAD. I agree the \ncommit message is confusing and could be much shorter.\n\nThanks\n\nPhillip\n > This ref is necessary because a subsequent \"git commit\" relies\n> on it to preserve the authorship metadata of the original\n> commit.\n> \n> Add a new test alongside the existing cherry-pick tests to\n> verify this behavior. The test suite currently checks for\n> CHERRY_PICK_HEAD after a conflicting pick, after a clean pick,\n> and after a clean pick with --no-commit. However, it lacks\n> coverage for a conflicting pick with --no-commit. Indeed, if\n> Git is modified to stop writing the ref in this specific\n> scenario, all existing tests still pass. This new test closes\n> the coverage gap.\n> \n> \n> \n> \n> \n>>\n>> Assisted-by: LLM\n>> Signed-off-by: Aleksei Sviridkin \n>> ---\n>> t/t3507-cherry-pick-conflict.sh | 6 ++++++\n>> 1 file changed, 6 insertions(+)\n>>\n>> diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\n>> index 44596cb1e8..2ce2e88184 100755\n>> --- a/t/t3507-cherry-pick-conflict.sh\n>> +++ b/t/t3507-cherry-pick-conflict.sh\n>> @@ -100,6 +100,12 @@ test_expect_success 'cherry-pick --no-commit does not set CHERRY_PICK_HEAD' '\n>> \ttest_must_fail git rev-parse --verify CHERRY_PICK_HEAD\n>> '\n>> \n>> +test_expect_success 'failed cherry-pick --no-commit does not set CHERRY_PICK_HEAD' '\n>> +\tpristine_detach initial &&\n>> +\ttest_must_fail git cherry-pick --no-commit picked &&\n>> +\ttest_must_fail git rev-parse --verify CHERRY_PICK_HEAD\n>> +'\n>> +\n>> test_expect_success 'cherry-pick w/dirty tree does not set CHERRY_PICK_HEAD' '\n>> \tpristine_detach initial &&\n>> \techo foo >foo &&\n>>\n>> base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc\n> \n\n"},{"id":"551940","messageId":"5e77651d-38a1-451e-b96b-33c91c414eb5@gmail.com","threadId":"66263","inReplyTo":"20260903125524.67889-1-f@lex.la","subject":"Re: [PATCH 1/2] t3507: pin CHERRY_PICK_HEAD absence for a conflicting --no-commit","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-09-04T09:57:38Z","receivedAt":"2026-09-04T09:57:41Z","isPatch":true,"body":"Hi Alexsei\n\nOn 03/09/2026 13:55, Aleksei Sviridkin wrote:\n> The tests here check the ref after a conflicting pick, after a clean\n> pick and after a clean pick under --no-commit, but not after a\n> conflicting one under --no-commit. That is the combination a user\n> runs into by accident: the pick stops with conflicts, and the ref\n> \"git commit\" would take the authorship from is not there.\n> \n> Pin it next to its siblings.\n\nWhat does pinning a test mean?\n\n> Letting the ref be written under\n> --no-commit when the pick conflicts leaves the rest of the cherry-pick\n> tests green, so nothing else guards that path.\n> \n> Assisted-by: LLM\n> Signed-off-by: Aleksei Sviridkin \n> ---\n> t/t3507-cherry-pick-conflict.sh | 6 ++++++\n> 1 file changed, 6 insertions(+)\n> \n> diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\n> index 44596cb1e8..2ce2e88184 100755\n> --- a/t/t3507-cherry-pick-conflict.sh\n> +++ b/t/t3507-cherry-pick-conflict.sh\n> @@ -100,6 +100,12 @@ test_expect_success 'cherry-pick --no-commit does not set CHERRY_PICK_HEAD' '\n> \ttest_must_fail git rev-parse --verify CHERRY_PICK_HEAD\n> '\n> \n> +test_expect_success 'failed cherry-pick --no-commit does not set CHERRY_PICK_HEAD' '\n> +\tpristine_detach initial &&\n> +\ttest_must_fail git cherry-pick --no-commit picked &&\n\nWe already have a test that checks the advice that's printed when there \nare conflicts, so could just add\n\n\ttest_must_fail git show-ref --verify CHERRY_PICK_HEAD\n\nthere. Because that test checks the command's output, we know that the \ncherry-pick has failed due to conflicts, and not some other reason. \nUsing test_must_fail() here without checking the error message means we \ndon't verify the reason that the cherry-pick failed.\n\nThanks\n\nPhillip\n> +\ttest_must_fail git rev-parse --verify CHERRY_PICK_HEAD\n> +'\n> +\n> test_expect_success 'cherry-pick w/dirty tree does not set CHERRY_PICK_HEAD' '\n> \tpristine_detach initial &&\n> \techo foo >foo &&\n> \n> base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc\n\n"},{"id":"551955","messageId":"20260904124435.12865-1-f@lex.la","threadId":"66263","inReplyTo":"20260903125524.67889-1-f@lex.la","subject":"[PATCH v2] doc: cherry-pick: note --no-commit skips CHERRY_PICK_HEAD","fromName":"Aleksei Sviridkin","fromEmail":"f@lex.la","sentAt":"2026-09-04T12:44:35Z","receivedAt":"2026-09-04T12:44:38Z","isPatch":true,"body":"The list of what happens when a change is hard to apply states without\nqualification that CHERRY_PICK_HEAD is set. Under --no-commit it is\nnot: d7e5c0cbfb (Introduce CHERRY_PICK_HEAD, 2011-02-19) skips the ref\non purpose there, expecting the user to pick further commits and edit\nthe result before committing.\n\nThe option's own description says nothing about the ref or about\nauthorship. \"git commit\" reads the author of a cherry-pick from\nCHERRY_PICK_HEAD, so without it a plain commit records you, not the\noriginal author, as the author. Picking a single commit this way\nstill leaves its log message in MERGE_MSG, so the result reads like a\nfaithful pick apart from the author.\n\nAssisted-by: LLM\nSigned-off-by: Aleksei Sviridkin \n---\nChanges since v1:\n - dropped the t3507 test patch: the shared !opts->no_commit guard\n is already exercised by the existing clean-pick test, so it added\n no real coverage\n - keep only the doc clarification, and make the --no-commit entry\n self-contained\n\n Documentation/git-cherry-pick.adoc | 8 +++++++-\n 1 file changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-cherry-pick.adoc b/Documentation/git-cherry-pick.adoc\nindex 42b41923d5..24a28d4e65 100644\n--- a/Documentation/git-cherry-pick.adoc\n+++ b/Documentation/git-cherry-pick.adoc\n@@ -25,7 +25,8 @@ happens:\n 1. The current branch and `HEAD` pointer stay at the last commit\n successfully made.\n 2. The `CHERRY_PICK_HEAD` ref is set to point at the commit that\n- introduced the change that is difficult to apply.\n+ introduced the change that is difficult to apply, unless the\n+ `--no-commit` option was given.\n 3. Paths in which the change applied cleanly are updated both\n in the index file and in your working tree.\n 4. For conflicting paths, the index file records up to three\n@@ -101,6 +102,11 @@ OPTIONS\n +\n This is useful when cherry-picking more than one commits'\n effect to your index in a row.\n++\n+This option does not record `CHERRY_PICK_HEAD`, so a plain `git commit`\n+afterwards records you, not the original author, as the author. When a\n+single commit is picked this way, `git commit -c ` keeps the\n+original authorship and log message.\n \n -s::\n --signoff::\n\nbase-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc\n-- \n2.55.0\n\n"},{"id":"551956","messageId":"20260904124505.12952-1-f@lex.la","threadId":"66263","inReplyTo":"apqSXT4lT7v0ILjp@pks.im","subject":"Re: [PATCH 1/2] t3507: pin CHERRY_PICK_HEAD absence for a conflicting --no-commit","fromName":"Aleksei Sviridkin","fromEmail":"f@lex.la","sentAt":"2026-09-04T12:45:05Z","receivedAt":"2026-09-04T12:45:09Z","isPatch":true,"body":"Patrick Steinhardt writes:\n> That doesn't specifically mean that this one test you add here is not\n> useful. But we need to have a better argument than \"we didn't have it\n> yet\".\n\nDropped the test. I went looking for that better argument and did not\nfind one: t3507 already has 'cherry-pick --no-commit does not set\nCHERRY_PICK_HEAD' for the clean pick, and the clean and the conflicting\npath go through the same !opts->no_commit guard in do_pick_commit(), so\nthe regression I described is covered already.\n\nv2 is the documentation change alone.\n"},{"id":"551957","messageId":"20260904124507.12977-1-f@lex.la","threadId":"66263","inReplyTo":"5e77651d-38a1-451e-b96b-33c91c414eb5@gmail.com","subject":"Re: [PATCH 1/2] t3507: pin CHERRY_PICK_HEAD absence for a conflicting --no-commit","fromName":"Aleksei Sviridkin","fromEmail":"f@lex.la","sentAt":"2026-09-04T12:45:07Z","receivedAt":"2026-09-04T12:45:11Z","isPatch":true,"body":"Phillip Wood writes:\n> What does pinning a test mean?\n> [...]\n> Using test_must_fail() here without checking the error message means\n> we don't verify the reason that the cherry-pick failed.\n\nDropped the test, so the wording goes with it. \"pin\" was jargon, sorry.\n\nYour placement was the right one: the advice test is what tells us the\npick stopped on a conflict, which the bare test_must_fail did not. But\nthe clean-pick test at t3507:98 and the conflicting case share the\n!opts->no_commit guard, so the assertion had no coverage left to add.\n\nv2 is the doc change alone.\n"},{"id":"551963","messageId":"639f29ff-59f3-403d-acbe-e6173a8fbf04@gmail.com","threadId":"66263","inReplyTo":"apqSXT4lT7v0ILjp@pks.im","subject":"Re: [PATCH 1/2] t3507: pin CHERRY_PICK_HEAD absence for a conflicting --no-commit","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-09-04T13:53:30Z","receivedAt":"2026-09-04T13:53:33Z","isPatch":true,"body":"On 04/09/2026 10:41, Patrick Steinhardt wrote:\n> On Fri, Sep 04, 2026 at 12:45:53AM +0300, Aleksei Sviridkin wrote:\n>> Junio C Hamano writes:\n>>> It is not apparent what problem, if any, the description\n>>> above claims the commit addresses. Nor is it clear why\n>>> checking these combinations is relevant.\n>>> [...]\n>>> Can you help me understand the above two paragraphs a bit better?\n>>\n>> The test pins the one combination t3507 did not cover. The file already\n>> checks CHERRY_PICK_HEAD after a conflicting pick, after a clean pick, and\n>> after a clean pick under --no-commit, but not after a conflicting pick\n>> under --no-commit. That is the case a user hits by accident: the pick\n>> stops on conflicts, they resolve and run \"git commit\", and the original\n>> author is not restored. --no-commit never wrote the ref, d7e5c0cbfb skips\n>> it on purpose. Your reading is right and Gemini's is backwards: under\n>> --no-commit we do not want CHERRY_PICK_HEAD, and the test asserts it is\n>> absent. Without it, teaching git to write the ref there would leave the\n>> whole file green.\n> \n> The question is whether it really makes sense to have tests for every\n> single edge case. In a perfect world we of course would, but in the real\n> world there are a) gazillions of different combinations and b) every\n> test brings its own overhead as it increases both wall time and\n> maintenance costs.\n\nWe should certainly be careful about adding too many tests - I often ask \nfor tests to be revised to remove duplicate coverage when reviewing \npatches from enthusiastic contributors. In this case I think it is worth \nchecking as we can do it by adding a single call to test_ref_missing to \nan existing test and the logic around when we do and do not write \nCHERRY_PICK_HEAD is a bit tricky.\n\n> That doesn't specifically mean that this one test you add here is not\n> useful. But we need to have a better argument than \"we didn't have it\n> yet\". For example we might've seen regressions, the logic is extremely\n> fragile or we risk bad consequences like data loss or an unrecoverable\n> situation if a property does not hold.\n\nI agree we should have a more substantial justification when adding \ntests. As I said above I think in this case the justification is \"the \nlogic is tricky\" and it is cheap to check it.\n\n> It's a thin line to walk at times, and I usually wouldn't care about\n> this too much. But over the last couple weeks we've seen more patch\n> series that add random tests to our test case without good reasoning\n> just for the sake of adding a test. And that's something that we need to\n> contain a bit.\n\nAgreed\n\nThanks\n\nPhillip\n\n"},{"id":"551964","messageId":"6d9595fc-6dc0-4bbc-b060-b34d782c6215@gmail.com","threadId":"66263","inReplyTo":"20260904124507.12977-1-f@lex.la","subject":"Re: [PATCH 1/2] t3507: pin CHERRY_PICK_HEAD absence for a conflicting --no-commit","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-09-04T13:57:16Z","receivedAt":"2026-09-04T13:57:19Z","isPatch":true,"body":"On 04/09/2026 13:45, Aleksei Sviridkin wrote:\n> Phillip Wood writes:\n>> What does pinning a test mean?\n>> [...]\n>> Using test_must_fail() here without checking the error message means\n>> we don't verify the reason that the cherry-pick failed.\n> \n> Dropped the test, so the wording goes with it. \"pin\" was jargon, sorry.\n> \n> Your placement was the right one: the advice test is what tells us the\n> pick stopped on a conflict, which the bare test_must_fail did not. But\n> the clean-pick test at t3507:98 and the conflicting case share the\n> !opts->no_commit guard, so the assertion had no coverage left to add.\n\nI don't follow this at all - where is the existing check that \nCHERRY_PICK_HEAD does not exist when \"git cherry-pick --no-commit\" stops \nfor conflicts? I was suggesting that we add a check for that to the test \n\"advice from failed cherry-pick --no-commit\", I'd forgotten when I wrote \nmy earlier email that we have a helper function test_ref_missing() to do \njust that.\n\nThanks\n\nPhillip\n"},{"id":"551983","messageId":"xmqqh5k556up.fsf@gitster.g","threadId":"66263","inReplyTo":"apqSXT4lT7v0ILjp@pks.im","subject":"Re: [PATCH 1/2] t3507: pin CHERRY_PICK_HEAD absence for a conflicting --no-commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-04T16:17:18Z","receivedAt":"2026-09-04T16:17:21Z","isPatch":true,"body":"Patrick Steinhardt writes:\n\n> The question is whether it really makes sense to have tests for every\n> single edge case. In a perfect world we of course would, but in the real\n> world there are a) gazillions of different combinations and b) every\n> test brings its own overhead as it increases both wall time and\n> maintenance costs.\n\nVery true. There needs a very good justification to add overhead to\nprotect what has been working fine for a long time ;-).\n\n"},{"id":"551984","messageId":"xmqqcxut56pv.fsf@gitster.g","threadId":"66263","inReplyTo":"5a5c7552-8fc8-48be-abf6-063aa31f7711@gmail.com","subject":"Re: [PATCH 1/2] t3507: pin CHERRY_PICK_HEAD absence for a conflicting --no-commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-04T16:20:12Z","receivedAt":"2026-09-04T16:20:15Z","isPatch":true,"body":"Phillip Wood writes:\n\n> On 03/09/2026 22:32, Junio C Hamano wrote:\n>> Aleksei Sviridkin writes:\n>> \n>> I shamelessly asked an AI agent I had nearby to guess what your log\n>> message might have meant and got the following. I am not sure if\n>> that matches what you wanted to say, or if it is totally off the\n>> mark, but at least I can follow what it is trying to say, even\n>> though I do not think if that matches reality (for example, when\n>> \"--no-commit\" is in effect, we probably do not want CHERRY_PICK_HEAD,\n>> even though the version of the text given by Gemini below claims it\n>> is needed).\n>> \n>> When a cherry-pick is run with the --no-commit option and halts\n>> due to conflicts, Git must still write the CHERRY_PICK_HEAD ref.\n>\n> No, with --no-commit it must not write CHERRY_PICK_HEAD.\n\nYou know that I know that ;-).\n\nMy point of asking an AI was to show that it was so unclear to\nconfuse AI into summarizing it down to a complete opposite\nstatement.\n\n> I agree the commit message is confusing and could be much\n> shorter.\n"},{"id":"552036","messageId":"xmqqik4j64qo.fsf@gitster.g","threadId":"66263","inReplyTo":"20260904124435.12865-1-f@lex.la","subject":"Re: [PATCH v2] doc: cherry-pick: note --no-commit skips CHERRY_PICK_HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-05T16:29:51Z","receivedAt":"2026-09-05T16:29:58Z","isPatch":true,"body":"Aleksei Sviridkin writes:\n\n> diff --git a/Documentation/git-cherry-pick.adoc b/Documentation/git-cherry-pick.adoc\n> index 42b41923d5..24a28d4e65 100644\n> --- a/Documentation/git-cherry-pick.adoc\n> +++ b/Documentation/git-cherry-pick.adoc\n> @@ -25,7 +25,8 @@ happens:\n> 1. The current branch and `HEAD` pointer stay at the last commit\n> successfully made.\n> 2. The `CHERRY_PICK_HEAD` ref is set to point at the commit that\n> - introduced the change that is difficult to apply.\n> + introduced the change that is difficult to apply, unless the\n> + `--no-commit` option was given.\n> 3. Paths in which the change applied cleanly are updated both\n> in the index file and in your working tree.\n> 4. For conflicting paths, the index file records up to three\n> @@ -101,6 +102,11 @@ OPTIONS\n> +\n> This is useful when cherry-picking more than one commits'\n> effect to your index in a row.\n> ++\n> +This option does not record `CHERRY_PICK_HEAD`, so a plain `git commit`\n> +afterwards records you, not the original author, as the author. When a\n> +single commit is picked this way, `git commit -c ` keeps the\n> +original authorship and log message.\n\nWhile the added text does not say anything false, I am not sure if\nthe last sentence hits the mark.\n\nMaybe we should hint that this is a deliberate design decision\nbehind the '--no-commit' option, perhaps in the description of that\noption?\n\nThe reason 'cherry-pick --no-commit ' does not record\n in CHERRY_PICK_HEAD is that the command is meant to work as\na better version [*] of 'git show | git apply'. The point\nof the operation is that you can continue to futz with the resulting\nmodified working tree to build your own work, and in that context,\nyou do not want the original authorship information.\n\nSo \"When a single commit is ...\", while not false, misses the point.\nAfter continuing to futz with the resulting modified working tree to\nbuild your own work, which may include picking (with the same\n'--no-commit' option) many more commits or writing your own code, you\nmay still want to borrow a large part of the commit message from a\ncommit, and 'git commit -c ' would be the natural\nthing to use.\n\nBut that advice belongs in the 'git commit' documentation, not the\n'git cherry-pick' documentation.\n\nOther than that, looking good.\n\nThanks.\n\n\n[Footnote]\n * \"better\" because unlike patch application, it can use 3-way merge\n machinery to take the full file contents to wiggle the changes\n from a different context into the code that is currently checked\n out.\n"},{"id":"552040","messageId":"20260905171332.34670-1-f@lex.la","threadId":"66263","inReplyTo":"20260903125524.67889-1-f@lex.la","subject":"[PATCH v3 0/2] cherry-pick: document that --no-commit skips CHERRY_PICK_HEAD","fromName":"Aleksei Sviridkin","fromEmail":"f@lex.la","sentAt":"2026-09-05T17:13:30Z","receivedAt":"2026-09-05T17:13:36Z","isPatch":true,"body":"v2 was doc-only. I had dropped the test claiming the existing\nclean-pick test already covered it, which was wrong: nothing checks\nthe ref after a conflicting --no-commit pick. 1/2 puts it back as a\nsingle test_ref_missing call in the existing conflicting-pick test.\n\n2/2 drops the \"git commit -c\" advice, which belongs in git-commit\ndocumentation, and says instead that the missing ref is the point of\nthe option rather than a wrinkle.\n\nThe Assisted-by trailer is gone from both.\n\nAleksei Sviridkin (2):\n t3507: check no CHERRY_PICK_HEAD after conflicting --no-commit\n doc: cherry-pick: note --no-commit skips CHERRY_PICK_HEAD\n\n Documentation/git-cherry-pick.adoc | 8 +++++++-\n t/t3507-cherry-pick-conflict.sh | 3 ++-\n 2 files changed, 9 insertions(+), 2 deletions(-)\n\n\nbase-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc\n-- \n2.55.0\n\n"},{"id":"552041","messageId":"20260905171332.34670-2-f@lex.la","threadId":"66263","inReplyTo":"20260905171332.34670-1-f@lex.la","subject":"[PATCH v3 1/2] t3507: check no CHERRY_PICK_HEAD after conflicting --no-commit","fromName":"Aleksei Sviridkin","fromEmail":"f@lex.la","sentAt":"2026-09-05T17:13:31Z","receivedAt":"2026-09-05T17:13:37Z","isPatch":true,"body":"Whether CHERRY_PICK_HEAD is written depends on the command, on whether\nthe merge started, and on --no-commit, all in one condition in\ndo_pick_commit(). The suite checks the clean --no-commit pick; nothing\nchecks the conflicting one.\n\nThe test that already runs a conflicting --no-commit pick compares the\nadvice the command prints, which is what tells us it stopped on a\nconflict. Assert the ref is missing there too.\n\nSigned-off-by: Aleksei Sviridkin \n---\n t/t3507-cherry-pick-conflict.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\nindex 44596cb1e8..aa004d929b 100755\n--- a/t/t3507-cherry-pick-conflict.sh\n+++ b/t/t3507-cherry-pick-conflict.sh\n@@ -79,7 +79,8 @@ test_expect_success 'advice from failed cherry-pick --no-commit' \"\n \tEOF\n \ttest_must_fail git cherry-pick --no-commit picked 2>actual &&\n \n-\ttest_cmp expected actual\n+\ttest_cmp expected actual &&\n+\ttest_ref_missing CHERRY_PICK_HEAD\n \"\n \n test_expect_success 'failed cherry-pick sets CHERRY_PICK_HEAD' '\n-- \n2.55.0\n\n"},{"id":"552042","messageId":"20260905171332.34670-3-f@lex.la","threadId":"66263","inReplyTo":"20260905171332.34670-1-f@lex.la","subject":"[PATCH v3 2/2] doc: cherry-pick: note --no-commit skips CHERRY_PICK_HEAD","fromName":"Aleksei Sviridkin","fromEmail":"f@lex.la","sentAt":"2026-09-05T17:13:32Z","receivedAt":"2026-09-05T17:13:37Z","isPatch":true,"body":"The list of what happens when a change is hard to apply states without\nqualification that CHERRY_PICK_HEAD is set. Under --no-commit it is\nnot: d7e5c0cbfb (Introduce CHERRY_PICK_HEAD, 2011-02-19) skips the ref\non purpose there, presuming the user intends to further edit the\nresult and possibly pick more commits on top.\n\nThe option's own description says nothing about the ref or about\nauthorship. \"git commit\" reads the author of a cherry-pick from\nCHERRY_PICK_HEAD, so without it a plain commit records you as the\nauthor. Say so where the option is described, and say that this is\nthe point of the option rather than a wrinkle: what is being built is\nthe user's own work, not a reproduction of the original commit.\n\nSigned-off-by: Aleksei Sviridkin \n---\n Documentation/git-cherry-pick.adoc | 8 +++++++-\n 1 file changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-cherry-pick.adoc b/Documentation/git-cherry-pick.adoc\nindex 42b41923d5..f4cd8b9db7 100644\n--- a/Documentation/git-cherry-pick.adoc\n+++ b/Documentation/git-cherry-pick.adoc\n@@ -25,7 +25,8 @@ happens:\n 1. The current branch and `HEAD` pointer stay at the last commit\n successfully made.\n 2. The `CHERRY_PICK_HEAD` ref is set to point at the commit that\n- introduced the change that is difficult to apply.\n+ introduced the change that is difficult to apply, unless the\n+ `--no-commit` option was given.\n 3. Paths in which the change applied cleanly are updated both\n in the index file and in your working tree.\n 4. For conflicting paths, the index file records up to three\n@@ -101,6 +102,11 @@ OPTIONS\n +\n This is useful when cherry-picking more than one commits'\n effect to your index in a row.\n++\n+This option does not record `CHERRY_PICK_HEAD`, so a plain `git commit`\n+afterwards records you as the author. That is by design: what you are\n+building is your own work, which you keep changing before committing,\n+rather than a reproduction of the original commit.\n \n -s::\n --signoff::\n-- \n2.55.0\n\n"},{"id":"552044","messageId":"20260905171345.34749-1-f@lex.la","threadId":"66263","inReplyTo":"6d9595fc-6dc0-4bbc-b060-b34d782c6215@gmail.com","subject":"Re: [PATCH 1/2] t3507: pin CHERRY_PICK_HEAD absence for a conflicting --no-commit","fromName":"Aleksei Sviridkin","fromEmail":"f@lex.la","sentAt":"2026-09-05T17:13:45Z","receivedAt":"2026-09-05T17:13:48Z","isPatch":true,"body":"Phillip Wood writes:\n> I don't follow this at all - where is the existing check that\n> CHERRY_PICK_HEAD does not exist when \"git cherry-pick --no-commit\"\n> stops for conflicts?\n\nThere is none. That was my error: the guard in do_pick_commit() is\nshared, but the suite only exercises the clean pick. v3 adds\ntest_ref_missing CHERRY_PICK_HEAD to the conflicting advice test, the\nhelper you pointed at.\n"},{"id":"552045","messageId":"20260905171347.34775-1-f@lex.la","threadId":"66263","inReplyTo":"639f29ff-59f3-403d-acbe-e6173a8fbf04@gmail.com","subject":"Re: [PATCH 1/2] t3507: pin CHERRY_PICK_HEAD absence for a conflicting --no-commit","fromName":"Aleksei Sviridkin","fromEmail":"f@lex.la","sentAt":"2026-09-05T17:13:47Z","receivedAt":"2026-09-05T17:13:50Z","isPatch":true,"body":"Phillip Wood writes:\n> In this case I think it is worth checking as we can do it by adding a\n> single call to test_ref_missing to an existing test and the logic\n> around when we do and do not write CHERRY_PICK_HEAD is a bit tricky.\n\nThat is the justification I should have given. Whether the ref is\nwritten depends on the command, on whether the merge started, and on\n--no-commit, all in one condition, and the conflicting --no-commit case\nis the one nothing checks. v3 states it that way.\n"},{"id":"552046","messageId":"20260905171349.34798-1-f@lex.la","threadId":"66263","inReplyTo":"apqSXT4lT7v0ILjp@pks.im","subject":"Re: [PATCH 1/2] t3507: pin CHERRY_PICK_HEAD absence for a conflicting --no-commit","fromName":"Aleksei Sviridkin","fromEmail":"f@lex.la","sentAt":"2026-09-05T17:13:49Z","receivedAt":"2026-09-05T17:13:53Z","isPatch":true,"body":"Patrick Steinhardt writes:\n> But we need to have a better argument than \"we didn't have it yet\".\n\nCorrecting my earlier mail: I said I was dropping the test because the\nexisting clean-pick test covered it, and that is wrong. The clean pick\nis checked, the conflicting one is not.\n\nThe justification is the condition itself: whether CHERRY_PICK_HEAD is\nwritten depends on the command, on whether the merge started, and on\n--no-commit, all in one place. v3 puts it back as a single\ntest_ref_missing call in an existing test, so no new test and no new\nsetup.\n"}]}