{"thread":{"id":"60832","subject":"[PATCH] t/t3515-cherry-pick-rebase.sh: new testcase demonstrating broken behavior","startedAt":"2024-02-02T09:19:04Z","lastAt":"2024-02-15T17:36:27Z","messageCount":16,"participants":["Vegard Nossum","Phillip Wood","Kristoffer Haugsbakk","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"487797","messageId":"20240202091850.160203-1-vegard.nossum@oracle.com","threadId":"60832","inReplyTo":null,"subject":"[PATCH] t/t3515-cherry-pick-rebase.sh: new testcase demonstrating broken behavior","fromName":"Vegard Nossum","fromEmail":"vegard.nossum@oracle.com","sentAt":"2024-02-02T09:18:50Z","receivedAt":"2024-02-02T09:19:04Z","isPatch":true,"sender":{"key":"vegard.nossum@oracle.com","avatar":"https://avatars.githubusercontent.com/u/24173?v=4"},"body":"Running \"git cherry-pick\" as an x-command in the rebase plan loses the\noriginal authorship information.\n\nWrite a known-broken test case for this:\n\n $ (cd t && ./t3515-cherry-pick-rebase.sh)\n ok 1 - setup\n ok 2 - cherry-pick preserves authorship information\n not ok 3 - cherry-pick inside rebase preserves authorship information # TODO known breakage\n # still have 1 known breakage(s)\n # passed all remaining 2 test(s)\n 1..3\n\nRunning with --verbose we see the diff between expected and actual:\n\n --- expected 2024-02-02 08:54:48.954753285 +0000\n +++ actual 2024-02-02 08:54:48.966753294 +0000\n @@ -1 +1 @@\n -Original Author\n +A U Thor\n\nAs far as I can tell, this is due to the check in print_advice()\nwhich deletes CHERRY_PICK_HEAD when GIT_CHERRY_PICK_HELP is set,\nbut I'm not sure what a good fix would be.\n\nCc: Harshit Mogalapalli \nSigned-off-by: Vegard Nossum \n---\n t/t3515-cherry-pick-rebase.sh | 37 +++++++++++++++++++++++++++++++++++\n 1 file changed, 37 insertions(+)\n create mode 100755 t/t3515-cherry-pick-rebase.sh\n\ndiff --git a/t/t3515-cherry-pick-rebase.sh b/t/t3515-cherry-pick-rebase.sh\nnew file mode 100755\nindex 0000000000..ffe6f5fe2a\n--- /dev/null\n+++ b/t/t3515-cherry-pick-rebase.sh\n@@ -0,0 +1,37 @@\n+#!/bin/sh\n+\n+test_description='test cherry-pick during a rebase'\n+\n+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n+\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\ttest_commit --author \"Original Author \" foo file contents1 &&\n+\tgit checkout -b feature &&\n+\ttest_commit --author \"Another Author \" bar file contents2\n+'\n+\n+test_expect_success 'cherry-pick preserves authorship information' '\n+\tgit checkout -B tmp feature &&\n+\ttest_must_fail git cherry-pick foo &&\n+\tgit add file &&\n+\tgit commit --no-edit &&\n+\tgit log -1 --format='%an' foo >expected &&\n+\tgit log -1 --format='%an' >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_failure 'cherry-pick inside rebase preserves authorship information' '\n+\tgit checkout -B tmp feature &&\n+\techo \"x git cherry-pick -x foo\" >rebase-plan &&\n+\ttest_must_fail env GIT_SEQUENCE_EDITOR=\"cp rebase-plan\" git rebase -i feature &&\n+\tgit add file &&\n+\tgit commit --no-edit &&\n+\tgit log -1 --format='%an' foo >expected &&\n+\tgit log -1 --format='%an' >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_done\n-- \n2.34.1\n\n"},{"id":"487890","messageId":"0adb1068-ef10-44ed-ad1d-e0927a09245d@gmail.com","threadId":"60832","inReplyTo":"20240202091850.160203-1-vegard.nossum@oracle.com","subject":"Re: [PATCH] t/t3515-cherry-pick-rebase.sh: new testcase demonstrating broken behavior","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-02-04T11:14:55Z","receivedAt":"2024-02-04T11:14:59Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Vegard\n\nOn 02/02/2024 09:18, Vegard Nossum wrote:\n> Running \"git cherry-pick\" as an x-command in the rebase plan loses the\n> original authorship information.\n> \n> Write a known-broken test case for this:\n> \n> $ (cd t && ./t3515-cherry-pick-rebase.sh)\n> ok 1 - setup\n> ok 2 - cherry-pick preserves authorship information\n> not ok 3 - cherry-pick inside rebase preserves authorship information # TODO known breakage\n> # still have 1 known breakage(s)\n> # passed all remaining 2 test(s)\n> 1..3\n> \n> Running with --verbose we see the diff between expected and actual:\n> \n> --- expected 2024-02-02 08:54:48.954753285 +0000\n> +++ actual 2024-02-02 08:54:48.966753294 +0000\n> @@ -1 +1 @@\n> -Original Author\n> +A U Thor\n> \n> As far as I can tell, this is due to the check in print_advice()\n> which deletes CHERRY_PICK_HEAD when GIT_CHERRY_PICK_HELP is set,\n> but I'm not sure what a good fix would be.\n\nThanks for reporting this and for the test case. I agree with your \ndiagnosis. I think the simplest fix would be to unset \nGIT_CHERRY_PICK_HELP in the child environment in sequencer.c:do_exec(). \nLong term we should stop setting GIT_CHERRY_PICK_HELP when rebasing and \nhard code the rebase conflicts message in sequencer.c as the environment \nvariable is a vestige of the scripted rebase implementation.\n\nTo work around the bug I think you can change the exec lines in the todo \nlist to\n\n exec unset GIT_CHERRY_PICK_HELP; git cherry-pick ...\n\nBest Wishes\n\nPhillip\n\n> Cc: Harshit Mogalapalli \n> Signed-off-by: Vegard Nossum \n> ---\n> t/t3515-cherry-pick-rebase.sh | 37 +++++++++++++++++++++++++++++++++++\n> 1 file changed, 37 insertions(+)\n> create mode 100755 t/t3515-cherry-pick-rebase.sh\n> \n> diff --git a/t/t3515-cherry-pick-rebase.sh b/t/t3515-cherry-pick-rebase.sh\n> new file mode 100755\n> index 0000000000..ffe6f5fe2a\n> --- /dev/null\n> +++ b/t/t3515-cherry-pick-rebase.sh\n> @@ -0,0 +1,37 @@\n> +#!/bin/sh\n> +\n> +test_description='test cherry-pick during a rebase'\n> +\n> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success setup '\n> +\ttest_commit --author \"Original Author \" foo file contents1 &&\n> +\tgit checkout -b feature &&\n> +\ttest_commit --author \"Another Author \" bar file contents2\n> +'\n> +\n> +test_expect_success 'cherry-pick preserves authorship information' '\n> +\tgit checkout -B tmp feature &&\n> +\ttest_must_fail git cherry-pick foo &&\n> +\tgit add file &&\n> +\tgit commit --no-edit &&\n> +\tgit log -1 --format='%an' foo >expected &&\n> +\tgit log -1 --format='%an' >actual &&\n> +\ttest_cmp expected actual\n> +'\n> +\n> +test_expect_failure 'cherry-pick inside rebase preserves authorship information' '\n> +\tgit checkout -B tmp feature &&\n> +\techo \"x git cherry-pick -x foo\" >rebase-plan &&\n> +\ttest_must_fail env GIT_SEQUENCE_EDITOR=\"cp rebase-plan\" git rebase -i feature &&\n> +\tgit add file &&\n> +\tgit commit --no-edit &&\n> +\tgit log -1 --format='%an' foo >expected &&\n> +\tgit log -1 --format='%an' >actual &&\n> +\ttest_cmp expected actual\n> +'\n> +\n> +test_done\n\n"},{"id":"487928","messageId":"20240205141335.762947-1-vegard.nossum@oracle.com","threadId":"60832","inReplyTo":"0adb1068-ef10-44ed-ad1d-e0927a09245d@gmail.com","subject":"[PATCH 2/2] sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands","fromName":"Vegard Nossum","fromEmail":"vegard.nossum@oracle.com","sentAt":"2024-02-05T14:13:35Z","receivedAt":"2024-02-05T14:13:47Z","isPatch":true,"sender":{"key":"vegard.nossum@oracle.com","avatar":"https://avatars.githubusercontent.com/u/24173?v=4"},"body":"Running \"git cherry-pick\" as an x-command in the rebase plan loses the\noriginal authorship information.\n\nTo fix this, unset GIT_CHERRY_PICK_HELP for 'exec' commands.\n\nLink: https://lore.kernel.org/git/0adb1068-ef10-44ed-ad1d-e0927a09245d@gmail.com/\nSuggested-by: Phillip Wood \nSigned-off-by: Vegard Nossum \n---\n sequencer.c | 1 +\n t/t3515-cherry-pick-rebase.sh | 2 +-\n 2 files changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 91de546b32..f49a871ac0 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3641,6 +3641,7 @@ static int do_exec(struct repository *r, const char *command_line)\n \tfprintf(stderr, _(\"Executing: %s\\n\"), command_line);\n \tcmd.use_shell = 1;\n \tstrvec_push(&cmd.args, command_line);\n+\tstrvec_push(&cmd.env, \"GIT_CHERRY_PICK_HELP\");\n \tstatus = run_command(&cmd);\n \n \t/* force re-reading of the cache */\ndiff --git a/t/t3515-cherry-pick-rebase.sh b/t/t3515-cherry-pick-rebase.sh\nindex ffe6f5fe2a..5cb2b96f66 100755\n--- a/t/t3515-cherry-pick-rebase.sh\n+++ b/t/t3515-cherry-pick-rebase.sh\n@@ -23,7 +23,7 @@ test_expect_success 'cherry-pick preserves authorship information' '\n \ttest_cmp expected actual\n '\n \n-test_expect_failure 'cherry-pick inside rebase preserves authorship information' '\n+test_expect_success 'cherry-pick inside rebase preserves authorship information' '\n \tgit checkout -B tmp feature &&\n \techo \"x git cherry-pick -x foo\" >rebase-plan &&\n \ttest_must_fail env GIT_SEQUENCE_EDITOR=\"cp rebase-plan\" git rebase -i feature &&\n-- \n2.34.1\n\n"},{"id":"487930","messageId":"ebe188e5-7289-4f7b-b845-d59a47cd06fe@app.fastmail.com","threadId":"60832","inReplyTo":"20240205141335.762947-1-vegard.nossum@oracle.com","subject":"Re: [PATCH 2/2] sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-02-05T14:38:20Z","receivedAt":"2024-02-05T14:38:45Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Mon, Feb 5, 2024, at 15:13, Vegard Nossum wrote:\n> Link: https://lore.kernel.org/git/0adb1068-ef10-44ed-ad1d-e0927a09245d@gmail.com/\n> Suggested-by: Phillip Wood \n> Signed-off-by: Vegard Nossum \n\n`Link` is not really used a lot. Junio’s `refs/notes/amlog` will point\nback to the patch (which is often close to the “suggested by” and so\non).\n\n-- \nKristoffer Haugsbakk\n"},{"id":"487968","messageId":"xmqqy1bymru0.fsf@gitster.g","threadId":"60832","inReplyTo":"ebe188e5-7289-4f7b-b845-d59a47cd06fe@app.fastmail.com","subject":"Re: [PATCH 2/2] sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-05T23:09:11Z","receivedAt":"2024-02-05T23:09:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kristoffer Haugsbakk\" writes:\n\n> On Mon, Feb 5, 2024, at 15:13, Vegard Nossum wrote:\n>> Link: https://lore.kernel.org/git/0adb1068-ef10-44ed-ad1d-e0927a09245d@gmail.com/\n>> Suggested-by: Phillip Wood \n>> Signed-off-by: Vegard Nossum \n>\n> `Link` is not really used a lot. Junio’s `refs/notes/amlog` will point\n> back to the patch (which is often close to the “suggested by” and so\n> on).\n\nGood. Also, is there [PATCH 1/2] that comes before this patch?\n"},{"id":"487969","messageId":"b3ec5d0b-ac17-4d1e-a17d-d5adfbfc6ccf@oracle.com","threadId":"60832","inReplyTo":"xmqqy1bymru0.fsf@gitster.g","subject":"Re: [PATCH 2/2] sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands","fromName":"Vegard Nossum","fromEmail":"vegard.nossum@oracle.com","sentAt":"2024-02-05T23:14:26Z","receivedAt":"2024-02-05T23:14:48Z","isPatch":true,"sender":{"key":"vegard.nossum@oracle.com","avatar":"https://avatars.githubusercontent.com/u/24173?v=4"},"body":"On 06/02/2024 00:09, Junio C Hamano wrote:\n> \"Kristoffer Haugsbakk\" writes:\n> \n>> On Mon, Feb 5, 2024, at 15:13, Vegard Nossum wrote:\n>>> Link: https://lore.kernel.org/git/0adb1068-ef10-44ed-ad1d-e0927a09245d@gmail.com/\n>>> Suggested-by: Phillip Wood \n>>> Signed-off-by: Vegard Nossum \n>>\n>> `Link` is not really used a lot. Junio’s `refs/notes/amlog` will point\n>> back to the patch (which is often close to the “suggested by” and so\n>> on).\n> \n> Good. Also, is there [PATCH 1/2] that comes before this patch?\n\nYes, kind of -- that's the testcase at the root of the thread:\n\nhttps://lore.kernel.org/git/20240202091850.160203-1-vegard.nossum@oracle.com/\n\n(\"t/t3515-cherry-pick-rebase.sh: new testcase demonstrating broken \nbehavior\")\n\n\nVegard\n"},{"id":"487990","messageId":"xmqqcytal01i.fsf@gitster.g","threadId":"60832","inReplyTo":"b3ec5d0b-ac17-4d1e-a17d-d5adfbfc6ccf@oracle.com","subject":"Re: [PATCH 2/2] sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-06T03:54:49Z","receivedAt":"2024-02-06T03:54:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Vegard Nossum writes:\n\n> On 06/02/2024 00:09, Junio C Hamano wrote:\n>> \"Kristoffer Haugsbakk\" writes:\n>> \n>>> On Mon, Feb 5, 2024, at 15:13, Vegard Nossum wrote:\n>>>> Link: https://lore.kernel.org/git/0adb1068-ef10-44ed-ad1d-e0927a09245d@gmail.com/\n>>>> Suggested-by: Phillip Wood \n>>>> Signed-off-by: Vegard Nossum \n>>>\n>>> `Link` is not really used a lot. Junio’s `refs/notes/amlog` will point\n>>> back to the patch (which is often close to the “suggested by” and so\n>>> on).\n>> Good. Also, is there [PATCH 1/2] that comes before this patch?\n>\n> Yes, kind of -- that's the testcase at the root of the thread:\n>\n> https://lore.kernel.org/git/20240202091850.160203-1-vegard.nossum@oracle.com/\n>\n> (\"t/t3515-cherry-pick-rebase.sh: new testcase demonstrating broken\n> behavior\")\n\nIf the first one was NOT marked as [1/2], it is customary to call\nsuch an \"we thought just one patch was sufficient, but here is\nanother\" step [2/1] instead, and that was why I was confused.\n\nPerhaps it is a good idea to squash them together as a single bugfix\npatch?\n\nThanks.\n"},{"id":"488136","messageId":"4e6d503a-8564-4536-82a7-29c489f5fec3@gmail.com","threadId":"60832","inReplyTo":"xmqqcytal01i.fsf@gitster.g","subject":"Re: [PATCH 2/2] sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-02-07T14:03:16Z","receivedAt":"2024-02-07T14:03:20Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 06/02/2024 03:54, Junio C Hamano wrote:\n> Vegard Nossum writes:\n> \n>> On 06/02/2024 00:09, Junio C Hamano wrote:\n> Perhaps it is a good idea to squash them together as a single bugfix\n> patch?\n\nI think so, I'm not sure we want to add a new test file just for this\neither. Having the test in a separate file was handy for debugging but\nI think something like the diff below would suffice though I wouldn't\nobject to checking the author of the cherry-picked commit\n\nBest Wishes\n\nPhillip\n\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex c5f30554c6..84a92d6da0 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -153,6 +153,18 @@ test_expect_success 'rebase -i with the exec command checks tree cleanness' '\n \tgit rebase --continue\n '\n \n+test_expect_success 'cherry-pick works with rebase --exec' '\n+\ttest_when_finished \"git cherry-pick --abort; \\\n+\t\t\t git rebase --abort; \\\n+\t\t\t git checkout primary\" &&\n+\techo \"exec git cherry-pick G\" >todo &&\n+\t(\n+\t\tset_replace_editor todo &&\n+\t\ttest_must_fail git rebase -i D D\n+\t) &&\n+\ttest_cmp_rev G CHERRY_PICK_HEAD\n+'\n+\n test_expect_success 'rebase -x with empty command fails' '\n \ttest_when_finished \"git rebase --abort ||:\" &&\n \ttest_must_fail env git rebase -x \"\" @ 2>actual &&\n"},{"id":"488146","messageId":"xmqq8r3wcjq2.fsf@gitster.g","threadId":"60832","inReplyTo":"4e6d503a-8564-4536-82a7-29c489f5fec3@gmail.com","subject":"Re: [PATCH 2/2] sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-07T16:39:01Z","receivedAt":"2024-02-07T16:39:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> On 06/02/2024 03:54, Junio C Hamano wrote:\n>> Vegard Nossum writes:\n>> \n>>> On 06/02/2024 00:09, Junio C Hamano wrote:\n>> Perhaps it is a good idea to squash them together as a single bugfix\n>> patch?\n>\n> I think so, I'm not sure we want to add a new test file just for this\n> either. Having the test in a separate file was handy for debugging but\n> I think something like the diff below would suffice though I wouldn't\n> object to checking the author of the cherry-picked commit\n\nVery true (I didn't even notice that the original \"bug report\ndisguised as a test addition\" was inventing a totally new file).\n\nThanks.\n\n>\n> Best Wishes\n>\n> Phillip\n>\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index c5f30554c6..84a92d6da0 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -153,6 +153,18 @@ test_expect_success 'rebase -i with the exec command checks tree cleanness' '\n> \tgit rebase --continue\n> '\n> +test_expect_success 'cherry-pick works with rebase --exec' '\n> +\ttest_when_finished \"git cherry-pick --abort; \\\n> +\t\t\t git rebase --abort; \\\n> +\t\t\t git checkout primary\" &&\n> +\techo \"exec git cherry-pick G\" >todo &&\n> +\t(\n> +\t\tset_replace_editor todo &&\n> +\t\ttest_must_fail git rebase -i D D\n> +\t) &&\n> +\ttest_cmp_rev G CHERRY_PICK_HEAD\n> +'\n> +\n> test_expect_success 'rebase -x with empty command fails' '\n> \ttest_when_finished \"git rebase --abort ||:\" &&\n> \ttest_must_fail env git rebase -x \"\" @ 2>actual &&\n"},{"id":"488220","messageId":"ae8d96b7-93b0-4460-b7ed-ffebaddd6f97@oracle.com","threadId":"60832","inReplyTo":"xmqq8r3wcjq2.fsf@gitster.g","subject":"Re: [PATCH 2/2] sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands","fromName":"Vegard Nossum","fromEmail":"vegard.nossum@oracle.com","sentAt":"2024-02-08T08:48:52Z","receivedAt":"2024-02-08T08:49:05Z","isPatch":true,"sender":{"key":"vegard.nossum@oracle.com","avatar":"https://avatars.githubusercontent.com/u/24173?v=4"},"body":"\nOn 07/02/2024 17:39, Junio C Hamano wrote:\n> Phillip Wood writes:\n> \n>> On 06/02/2024 03:54, Junio C Hamano wrote:\n>>> Vegard Nossum writes:\n>>>\n>>>> On 06/02/2024 00:09, Junio C Hamano wrote:\n>>> Perhaps it is a good idea to squash them together as a single bugfix\n>>> patch?\n>>\n>> I think so, I'm not sure we want to add a new test file just for this\n>> either. Having the test in a separate file was handy for debugging but\n>> I think something like the diff below would suffice though I wouldn't\n>> object to checking the author of the cherry-picked commit\n> \n> Very true (I didn't even notice that the original \"bug report\n> disguised as a test addition\" was inventing a totally new file).\n\nI'm sorry, but I'm confused about what I'm supposed to do now.\n\nThere is now another test case and it sounds like you would prefer that\none over mine, but I didn't write it and there is no SOB, so I cannot\nsubmit that with the fix if I were to \"squash them together\".\n\nI am not a regular contributor so I don't have a good grasp on things\nlike why you don't want a new test file for this, or why you (as the\nmaintainer) can't just squash the patches yourself if that's what you\nprefer.\n\nThanks,\n\n\nVegard\n"},{"id":"488229","messageId":"eaf511ff-f9e0-47ac-ae2e-3de0efa928dd@gmail.com","threadId":"60832","inReplyTo":"ae8d96b7-93b0-4460-b7ed-ffebaddd6f97@oracle.com","subject":"Re: [PATCH 2/2] sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-02-08T14:26:11Z","receivedAt":"2024-02-08T14:26:16Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Vegard\n\nOn 08/02/2024 08:48, Vegard Nossum wrote:\n> I'm sorry, but I'm confused about what I'm supposed to do now.\n> \n> There is now another test case and it sounds like you would prefer that\n> one over mine, but I didn't write it and there is no SOB, so I cannot\n> submit that with the fix if I were to \"squash them together\".\n\nHere's my SOB for the diff in \nhttps://lore.kernel.org/git/4e6d503a-8564-4536-82a7-29c489f5fec3@gmail.com/\n\nSigned-off-by: Phillip Wood \n\nI think that typically for small suggestions like that we just add a \nHelped-by: trailer but feel free to add my SOB if you want.\n\n> I am not a regular contributor so I don't have a good grasp on things\n> like why you don't want a new test file for this,\n\nI think keeping related tests together helps contributors see which test \nfiles to run when they're changing code (running the whole suite each \ntime is too slow). There is also a (small) setup overhead for each new \nfile. For tests like this it is a bit ambiguous whether it belongs with \nthe other \"rebase --exec\" tests or the other \"cherry-pick\" tests. I \nopted to put it with the other \"rebase --exec\" tests as I think it is \nreally fixing a bug with rebase rather than cherry-pick.\n\nBest Wishes\n\nPhillip\n"},{"id":"488240","messageId":"xmqqv86yoot3.fsf@gitster.g","threadId":"60832","inReplyTo":"eaf511ff-f9e0-47ac-ae2e-3de0efa928dd@gmail.com","subject":"Re: [PATCH 2/2] sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-08T17:20:40Z","receivedAt":"2024-02-08T17:20:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> I think that typically for small suggestions like that we just add a\n> Helped-by: trailer but feel free to add my SOB if you want.\n\nThanks, both. Here is what I assembled from the pieces.\n\n----- >8 --------- >8 --------- >8 --------- >8 -----\nFrom: Vegard Nossum \nDate: Fri, 2 Feb 2024 10:18:50 +0100\nSubject: [PATCH] sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands\n\nRunning \"git cherry-pick\" as an x-command in the rebase plan loses\nthe original authorship information.\n\nTo fix this, unset GIT_CHERRY_PICK_HELP for 'exec' commands.\n\nHelped-by: Phillip Wood \nSigned-off-by: Vegard Nossum \nSigned-off-by: Junio C Hamano \n---\n sequencer.c | 1 +\n t/t3404-rebase-interactive.sh | 12 ++++++++++++\n 2 files changed, 13 insertions(+)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex d584cac8ed..ed30ceaf8b 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3647,6 +3647,7 @@ static int do_exec(struct repository *r, const char *command_line)\n \tfprintf(stderr, _(\"Executing: %s\\n\"), command_line);\n \tcmd.use_shell = 1;\n \tstrvec_push(&cmd.args, command_line);\n+\tstrvec_push(&cmd.env, \"GIT_CHERRY_PICK_HELP\");\n \tstatus = run_command(&cmd);\n \n \t/* force re-reading of the cache */\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex c5f30554c6..84a92d6da0 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -153,6 +153,18 @@ test_expect_success 'rebase -i with the exec command checks tree cleanness' '\n \tgit rebase --continue\n '\n \n+test_expect_success 'cherry-pick works with rebase --exec' '\n+\ttest_when_finished \"git cherry-pick --abort; \\\n+\t\t\t git rebase --abort; \\\n+\t\t\t git checkout primary\" &&\n+\techo \"exec git cherry-pick G\" >todo &&\n+\t(\n+\t\tset_replace_editor todo &&\n+\t\ttest_must_fail git rebase -i D D\n+\t) &&\n+\ttest_cmp_rev G CHERRY_PICK_HEAD\n+'\n+\n test_expect_success 'rebase -x with empty command fails' '\n \ttest_when_finished \"git rebase --abort ||:\" &&\n \ttest_must_fail env git rebase -x \"\" @ 2>actual &&\n-- \n2.43.0-561-g235986be82\n\n"},{"id":"488386","messageId":"4073b764-ab6a-4b4b-a8a3-2e898620b2f5@gmail.com","threadId":"60832","inReplyTo":"xmqqv86yoot3.fsf@gitster.g","subject":"Re: [PATCH 2/2] sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-02-11T11:11:52Z","receivedAt":"2024-02-11T11:11:56Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nOn 08/02/2024 17:20, Junio C Hamano wrote:\n> Phillip Wood writes:\n> \n>> I think that typically for small suggestions like that we just add a\n>> Helped-by: trailer but feel free to add my SOB if you want.\n> \n> Thanks, both. Here is what I assembled from the pieces.\n> \n> ----- >8 --------- >8 --------- >8 --------- >8 -----\n> From: Vegard Nossum \n> Date: Fri, 2 Feb 2024 10:18:50 +0100\n> Subject: [PATCH] sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands\n> \n> Running \"git cherry-pick\" as an x-command in the rebase plan loses\n> the original authorship information.\n\nIt might be worth explaining why this happens\n\nThis is because rebase sets the GIT_CHERRY_PICK_HELP environment \nvariable to customize the advice given to users when there are conflicts \nwhich causes the sequencer to remove CHERRY_PICK_HEAD.\n\n> To fix this, unset GIT_CHERRY_PICK_HELP for 'exec' commands.\n\nThe patch itself looks fine\n\nBest Wishes\n\nPhillip\n\n> Helped-by: Phillip Wood \n> Signed-off-by: Vegard Nossum \n> Signed-off-by: Junio C Hamano \n> ---\n> sequencer.c | 1 +\n> t/t3404-rebase-interactive.sh | 12 ++++++++++++\n> 2 files changed, 13 insertions(+)\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index d584cac8ed..ed30ceaf8b 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -3647,6 +3647,7 @@ static int do_exec(struct repository *r, const char *command_line)\n> \tfprintf(stderr, _(\"Executing: %s\\n\"), command_line);\n> \tcmd.use_shell = 1;\n> \tstrvec_push(&cmd.args, command_line);\n> +\tstrvec_push(&cmd.env, \"GIT_CHERRY_PICK_HELP\");\n> \tstatus = run_command(&cmd);\n> \n> \t/* force re-reading of the cache */\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index c5f30554c6..84a92d6da0 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -153,6 +153,18 @@ test_expect_success 'rebase -i with the exec command checks tree cleanness' '\n> \tgit rebase --continue\n> '\n> \n> +test_expect_success 'cherry-pick works with rebase --exec' '\n> +\ttest_when_finished \"git cherry-pick --abort; \\\n> +\t\t\t git rebase --abort; \\\n> +\t\t\t git checkout primary\" &&\n> +\techo \"exec git cherry-pick G\" >todo &&\n> +\t(\n> +\t\tset_replace_editor todo &&\n> +\t\ttest_must_fail git rebase -i D D\n> +\t) &&\n> +\ttest_cmp_rev G CHERRY_PICK_HEAD\n> +'\n> +\n> test_expect_success 'rebase -x with empty command fails' '\n> \ttest_when_finished \"git rebase --abort ||:\" &&\n> \ttest_must_fail env git rebase -x \"\" @ 2>actual &&\n"},{"id":"488400","messageId":"xmqq8r3r9biz.fsf@gitster.g","threadId":"60832","inReplyTo":"4073b764-ab6a-4b4b-a8a3-2e898620b2f5@gmail.com","subject":"Re: [PATCH 2/2] sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-11T17:05:40Z","receivedAt":"2024-02-11T17:05:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> Hi Junio\n>\n> On 08/02/2024 17:20, Junio C Hamano wrote:\n>> Phillip Wood writes:\n>> \n>>> I think that typically for small suggestions like that we just add a\n>>> Helped-by: trailer but feel free to add my SOB if you want.\n>> Thanks, both. Here is what I assembled from the pieces.\n>> ----- >8 --------- >8 --------- >8 --------- >8 -----\n>> From: Vegard Nossum \n>> Date: Fri, 2 Feb 2024 10:18:50 +0100\n>> Subject: [PATCH] sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands\n>> Running \"git cherry-pick\" as an x-command in the rebase plan loses\n>> the original authorship information.\n>\n> It might be worth explaining why this happens\n>\n> This is because rebase sets the GIT_CHERRY_PICK_HELP environment\n> variable to customize the advice given to users when there are\n> conflicts which causes the sequencer to remove CHERRY_PICK_HEAD.\n\nTrue. I'd prefer to see the original submitter assemble the pieces\nand come up with the final version, rather than me doing so.\n\nThanks.\n"},{"id":"488739","messageId":"76c62133-30e4-4145-99ea-aeef644d617a@oracle.com","threadId":"60832","inReplyTo":"xmqq8r3r9biz.fsf@gitster.g","subject":"Re: [PATCH 2/2] sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands","fromName":"Vegard Nossum","fromEmail":"vegard.nossum@oracle.com","sentAt":"2024-02-15T14:24:53Z","receivedAt":"2024-02-15T14:25:06Z","isPatch":true,"sender":{"key":"vegard.nossum@oracle.com","avatar":"https://avatars.githubusercontent.com/u/24173?v=4"},"body":"\nOn 11/02/2024 18:05, Junio C Hamano wrote:\n> Phillip Wood writes:\n>> On 08/02/2024 17:20, Junio C Hamano wrote:\n>>> Phillip Wood writes:\n>> It might be worth explaining why this happens\n>>\n>> This is because rebase sets the GIT_CHERRY_PICK_HELP environment\n>> variable to customize the advice given to users when there are\n>> conflicts which causes the sequencer to remove CHERRY_PICK_HEAD.\n> \n> True. I'd prefer to see the original submitter assemble the pieces\n> and come up with the final version, rather than me doing so.\n\nThanks for explaining and sorry for the delay. I saw the patch was\nmerged to main now, but I will keep this in mind for next time.\n\n\nVegard\n"},{"id":"488745","messageId":"xmqq8r3lsk87.fsf@gitster.g","threadId":"60832","inReplyTo":"76c62133-30e4-4145-99ea-aeef644d617a@oracle.com","subject":"Re: [PATCH 2/2] sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-15T17:36:24Z","receivedAt":"2024-02-15T17:36:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Vegard Nossum writes:\n\n> On 11/02/2024 18:05, Junio C Hamano wrote:\n>> Phillip Wood writes:\n>>> On 08/02/2024 17:20, Junio C Hamano wrote:\n>>>> Phillip Wood writes:\n>>> It might be worth explaining why this happens\n>>>\n>>> This is because rebase sets the GIT_CHERRY_PICK_HELP environment\n>>> variable to customize the advice given to users when there are\n>>> conflicts which causes the sequencer to remove CHERRY_PICK_HEAD.\n>> True. I'd prefer to see the original submitter assemble the pieces\n>> and come up with the final version, rather than me doing so.\n>\n> Thanks for explaining and sorry for the delay. I saw the patch was\n> merged to main now, but I will keep this in mind for next time.\n\nThanks for finding and fixing. Hope we'll see more of your\ncontributions in the future.\n\n"}]}