{"thread":{"id":"56148","subject":"[PATCH] [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP","startedAt":"2021-07-22T14:06:13Z","lastAt":"2021-07-28T17:26:12Z","messageCount":17,"participants":["ZheNing Hu via GitGitGadget","Junio C Hamano","ZheNing Hu","Felipe Contreras","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"430933","messageId":"pull.1001.git.1626962763373.gitgitgadget@gmail.com","threadId":"56148","inReplyTo":null,"subject":"[PATCH] [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-22T14:06:03Z","receivedAt":"2021-07-22T14:06:13Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"From: ZheNing Hu \n\nIf we set the value of the environment variable GIT_CHERRY_PICK_HELP\nwhen using `git cherry-pick`, CHERRY_PICK_HEAD will be deleted, then\nwe will get an error when we try to use `git cherry-pick --continue`\nor other cherr-pick command.\n\nSo add option action check in print_advice(), we will not remove\nCHERRY_PICK_HEAD if we are indeed cherry-picking instead of rebasing.\n\nSigned-off-by: ZheNing Hu \n---\n [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP\n \n E.g. We are now in the branch main, we want to cherry-pick commit\n ac346df, then we want to use GIT_CHERRY_PICK_HELP to use customized\n advice:\n \n GIT_CHERRY_PICK_HELP=\"you should use git cherry-pick --continue after\n resloving the conflict!\" git cherry-pick ac346df\n \n then we can see the correct advice message, but after that, if we want\n to use \"git cherry-pick --continue\" or other cherry-pick commands, an\n error will appear.\n \n So this patch fixes this bug when git cherry-pick is used with\n environment variable GIT_CHERRY_PICK_HELP.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1001%2Fadlternative%2Fcherry-pick-help-fix-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1001/adlternative/cherry-pick-help-fix-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1001\n\n sequencer.c | 5 +++--\n t/t3507-cherry-pick-conflict.sh | 25 ++++++++++++++++++++-----\n 2 files changed, 23 insertions(+), 7 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 0bec01cf38e..c01b0b9e9c9 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -409,8 +409,9 @@ static void print_advice(struct repository *r, int show_hint,\n \t\t * (typically rebase --interactive) wants to take care\n \t\t * of the commit itself so remove CHERRY_PICK_HEAD\n \t\t */\n-\t\trefs_delete_ref(get_main_ref_store(r), \"\", \"CHERRY_PICK_HEAD\",\n-\t\t\t\tNULL, 0);\n+\t\tif (opts->action != REPLAY_PICK)\n+\t\t\trefs_delete_ref(get_main_ref_store(r), \"\", \"CHERRY_PICK_HEAD\",\n+\t\t\t\t\tNULL, 0);\n \t\treturn;\n \t}\n \ndiff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\nindex 014001b8f32..70becaf27aa 100755\n--- a/t/t3507-cherry-pick-conflict.sh\n+++ b/t/t3507-cherry-pick-conflict.sh\n@@ -109,14 +109,29 @@ test_expect_success \\\n \ttest_must_fail git rev-parse --verify CHERRY_PICK_HEAD\n '\n \n-test_expect_success 'GIT_CHERRY_PICK_HELP suppresses CHERRY_PICK_HEAD' '\n-\tpristine_detach initial &&\n+test_expect_success 'GIT_CHERRY_PICK_HELP respects CHERRY_PICK_HEAD' '\n+\tgit init repo &&\n \t(\n+\t\tcd repo &&\n+\t\tgit branch -M main &&\n+\t\techo 1 >file &&\n+\t\tgit add file &&\n+\t\tgit commit -m 1 &&\n+\t\techo 2 >file &&\n+\t\tgit add file &&\n+\t\tgit commit -m 2 &&\n+\t\tgit checkout HEAD~ &&\n+\t\techo 3 >file &&\n+\t\tgit add file &&\n+\t\tgit commit -m 3 &&\n \t\tGIT_CHERRY_PICK_HELP=\"and then do something else\" &&\n \t\texport GIT_CHERRY_PICK_HELP &&\n-\t\ttest_must_fail git cherry-pick picked\n-\t) &&\n-\ttest_must_fail git rev-parse --verify CHERRY_PICK_HEAD\n+\t\ttest_must_fail git cherry-pick main &&\n+\t\tgit rev-parse --verify CHERRY_PICK_HEAD >actual &&\n+\t\tgit rev-parse --verify main >expect &&\n+\t\ttest_cmp expect actual &&\n+\t\tgit cherry-pick --abort\n+\t)\n '\n \n test_expect_success 'git reset clears CHERRY_PICK_HEAD' '\n\nbase-commit: daab8a564f8bbac55f70f8bf86c070e001a9b006\n-- \ngitgitgadget\n"},{"id":"430953","messageId":"xmqqo8au82i0.fsf@gitster.g","threadId":"56148","inReplyTo":"pull.1001.git.1626962763373.gitgitgadget@gmail.com","subject":"Re: [PATCH] [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-22T21:25:43Z","receivedAt":"2021-07-22T21:25:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"ZheNing Hu via GitGitGadget\" writes:\n\n> From: ZheNing Hu \n>\n> If we set the value of the environment variable GIT_CHERRY_PICK_HELP\n> when using `git cherry-pick`, CHERRY_PICK_HEAD will be deleted, then\n> we will get an error when we try to use `git cherry-pick --continue`\n> or other cherr-pick command.\n\nI think that the GIT_CHERRY_PICK_HELP is an implemention detail for\nvarious forms of rebase to use cherry-pick as a backend and not for\nuse by end users. \n\nI strongly suspect that the right solution for the breakage is to\nunsetenv(GIT_CHERRY_PICK_HELP) in cmd_cherry_pick() without touching\nsequencer.c at all.\n\nIt _is_ ugly that a helper that is responsible for emitting an\nadvise message also makes a decision whether the pseudo-ref gets\ndeleted or not, but a fix to that problem should be done byy making\nthe logic for the decision less complex, not more.\n\nSo, I am not enthused to see this change.\n"},{"id":"431012","messageId":"CAOLTT8Ty47fyY7T3d68CYPKh9k+HAHsnCLJ=F0KaLm+0gp3+EQ@mail.gmail.com","threadId":"56148","inReplyTo":"xmqqo8au82i0.fsf@gitster.g","subject":"Re: [PATCH] [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-07-23T09:37:42Z","receivedAt":"2021-07-23T09:37:19Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Junio C Hamano 于2021年7月23日周五 上午5:25写道：\n>\n> \"ZheNing Hu via GitGitGadget\" writes:\n>\n> > From: ZheNing Hu \n> >\n> > If we set the value of the environment variable GIT_CHERRY_PICK_HELP\n> > when using `git cherry-pick`, CHERRY_PICK_HEAD will be deleted, then\n> > we will get an error when we try to use `git cherry-pick --continue`\n> > or other cherr-pick command.\n>\n> I think that the GIT_CHERRY_PICK_HELP is an implemention detail for\n> various forms of rebase to use cherry-pick as a backend and not for\n> use by end users.\n>\n\nBut someone complain to me that the cherry-pick advice is not good enough.\nThink about a git newbie is cherry-picking a patch series containing\nseveral commits,\nE.g.\n\ngit cherry-pick dev~3..dev\n\nAnd then he (she) will see these advice info:\nhint: after resolving the conflicts, mark the corrected paths\nhint: with 'git add ' or 'git rm '\nhint: and commit the result with 'git commit'\n\nAfter he resolving git conflict, execute 'git commit' according to the\nprompt, A terrible thing happened: CHERRY_PICK_HEAD is deleted\nby git and no errors are output. But in fact .git/sequencer still exists,\nWait until he uses the cherry-pick command next time, the error appears:\n\nerror: cherry-pick is already in progress\nhint: try \"git cherry-pick (--continue | --abort | --quit)\"\nfatal: cherry-pick failed\n\nSo we should not encourage users to use git commit when git cherry-pick.\nIt would be great if it could provide advice similar to rebase, like this:\n\nOnce you are satisfied with your changes, run\n\n git cherry-pick --continue\n\n> I strongly suspect that the right solution for the breakage is to\n> unsetenv(GIT_CHERRY_PICK_HELP) in cmd_cherry_pick() without touching\n> sequencer.c at all.\n>\n> It _is_ ugly that a helper that is responsible for emitting an\n> advise message also makes a decision whether the pseudo-ref gets\n> deleted or not, but a fix to that problem should be done byy making\n> the logic for the decision less complex, not more.\n>\n\nMay be you are right :)\n\n> So, I am not enthused to see this change.\n\nThanks.\n--\nZheNing Hu\n"},{"id":"431057","messageId":"60faf601bcd7c_defb20881@natae.notmuch","threadId":"56148","inReplyTo":"CAOLTT8Ty47fyY7T3d68CYPKh9k+HAHsnCLJ=F0KaLm+0gp3+EQ@mail.gmail.com","subject":"Re: [PATCH] [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-07-23T17:01:53Z","receivedAt":"2021-07-23T17:01:57Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"ZheNing Hu wrote:\n> Junio C Hamano 于2021年7月23日周五 上午5:25写道：\n> >\n> > \"ZheNing Hu via GitGitGadget\" writes:\n> >\n> > > From: ZheNing Hu \n> > >\n> > > If we set the value of the environment variable GIT_CHERRY_PICK_HELP\n> > > when using `git cherry-pick`, CHERRY_PICK_HEAD will be deleted, then\n> > > we will get an error when we try to use `git cherry-pick --continue`\n> > > or other cherr-pick command.\n> >\n> > I think that the GIT_CHERRY_PICK_HELP is an implemention detail for\n> > various forms of rebase to use cherry-pick as a backend and not for\n> > use by end users.\n> >\n> \n> But someone complain to me that the cherry-pick advice is not good enough.\n\nI agree it's not good enough.\n\nIn my opinion `git cherry-pick` should be a more prominent command, and\na lot functionality is missing.\n\n> Think about a git newbie is cherry-picking a patch series containing\n> several commits,\n> E.g.\n> \n> git cherry-pick dev~3..dev\n> \n> And then he (she) will see these advice info:\n> hint: after resolving the conflicts, mark the corrected paths\n> hint: with 'git add ' or 'git rm '\n> hint: and commit the result with 'git commit'\n> \n> After he resolving git conflict, execute 'git commit' according to the\n> prompt, A terrible thing happened: CHERRY_PICK_HEAD is deleted\n> by git and no errors are output. But in fact .git/sequencer still exists,\n> Wait until he uses the cherry-pick command next time, the error appears:\n> \n> error: cherry-pick is already in progress\n> hint: try \"git cherry-pick (--continue | --abort | --quit)\"\n> fatal: cherry-pick failed\n> \n> So we should not encourage users to use git commit when git cherry-pick.\n> It would be great if it could provide advice similar to rebase, like this:\n> \n> Once you are satisfied with your changes, run\n> \n> git cherry-pick --continue\n\nAgreed.\n\n-- \nFelipe Contreras"},{"id":"431112","messageId":"pull.1001.v2.git.1627135281887.gitgitgadget@gmail.com","threadId":"56148","inReplyTo":"pull.1001.git.1626962763373.gitgitgadget@gmail.com","subject":"[PATCH v2] [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-24T14:01:21Z","receivedAt":"2021-07-24T14:01:28Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"From: ZheNing Hu \n\nIf we set the value of the environment variable GIT_CHERRY_PICK_HELP\nwhen using `git cherry-pick`, CHERRY_PICK_HEAD will be deleted, then\nwe will get an error when we try to use `git cherry-pick --continue`\nor other cherr-pick command.\n\nSo unsetenv(GIT_CHERRY_PICK_HELP) in cmd_cherry_pick(), to avoid\ndeleting CHERRY_PICK_HEAD when we are truly cherry-picking, which can\nfix this breakage.\n\nSigned-off-by: ZheNing Hu \nMentored-by: Christian Couder \nMentored-by Hariom Verma :\n---\n [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP\n \n This patch fixes the bug when git cherry-pick is used with environment\n variable GIT_CHERRY_PICK_HELP.\n \n Change from last version:\n \n 1. Only unsetenv(GIT_CHERRY_PICK_HELP) without touching anything in\n sequencer.c. Now git cherry-pick will ignore GIT_CHERRY_PICK_HELP,\n \n $ GIT_CHERRY_PICK_HELP=\"213\" git cherry-pick dev~3..dev\n \n will only output default advice:\n \n hint: after resolving the conflicts, mark the corrected paths hint: with\n 'git add ' or 'git rm ' hint: and commit the result with 'git commit'\n \n This may still not be good enough, hope that cherry-pick will not advice\n anything related to \"git commit\". Maybe we should make --no-commit as\n cherry-pick default behavior?\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1001%2Fadlternative%2Fcherry-pick-help-fix-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1001/adlternative/cherry-pick-help-fix-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1001\n\nRange-diff vs v1:\n\n 1: c2a6a625ac8 ! 1: fbb9c166502 [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP\n @@ Commit message\n we will get an error when we try to use `git cherry-pick --continue`\n or other cherr-pick command.\n \n - So add option action check in print_advice(), we will not remove\n - CHERRY_PICK_HEAD if we are indeed cherry-picking instead of rebasing.\n + So unsetenv(GIT_CHERRY_PICK_HELP) in cmd_cherry_pick(), to avoid\n + deleting CHERRY_PICK_HEAD when we are truly cherry-picking, which can\n + fix this breakage.\n \n Signed-off-by: ZheNing Hu \n + Mentored-by: Christian Couder \n + Mentored-by Hariom Verma :\n \n - ## sequencer.c ##\n -@@ sequencer.c: static void print_advice(struct repository *r, int show_hint,\n - \t\t * (typically rebase --interactive) wants to take care\n - \t\t * of the commit itself so remove CHERRY_PICK_HEAD\n - \t\t */\n --\t\trefs_delete_ref(get_main_ref_store(r), \"\", \"CHERRY_PICK_HEAD\",\n --\t\t\t\tNULL, 0);\n -+\t\tif (opts->action != REPLAY_PICK)\n -+\t\t\trefs_delete_ref(get_main_ref_store(r), \"\", \"CHERRY_PICK_HEAD\",\n -+\t\t\t\t\tNULL, 0);\n - \t\treturn;\n - \t}\n + ## builtin/revert.c ##\n +@@ builtin/revert.c: int cmd_cherry_pick(int argc, const char **argv, const char *prefix)\n \n + \topts.action = REPLAY_PICK;\n + \tsequencer_init_config(&opts);\n ++\tunsetenv(\"GIT_CHERRY_PICK_HELP\");\n + \tres = run_sequencer(argc, argv, &opts);\n + \tif (res < 0)\n + \t\tdie(_(\"cherry-pick failed\"));\n \n ## t/t3507-cherry-pick-conflict.sh ##\n +@@ t/t3507-cherry-pick-conflict.sh: test_expect_success 'advice from failed cherry-pick --no-commit' \"\n + \ttest_cmp expected actual\n + \"\n + \n ++test_expect_success 'advice from failed cherry-pick with GIT_CHERRY_PICK_HELP' \"\n ++\tpristine_detach initial &&\n ++\t(\n ++\t\tpicked=\\$(git rev-parse --short picked) &&\n ++\t\tcat <<-EOF >expected &&\n ++\t\terror: could not apply \\$picked... picked\n ++\t\thint: after resolving the conflicts, mark the corrected paths\n ++\t\thint: with 'git add ' or 'git rm '\n ++\t\thint: and commit the result with 'git commit'\n ++\t\tEOF\n ++\t\tGIT_CHERRY_PICK_HELP='and then do something else' &&\n ++\t\texport GIT_CHERRY_PICK_HELP &&\n ++\t\ttest_must_fail git cherry-pick picked 2>actual &&\n ++\t\ttest_cmp expected actual\n ++\t)\n ++\"\n ++\n + test_expect_success 'failed cherry-pick sets CHERRY_PICK_HEAD' '\n + \tpristine_detach initial &&\n + \ttest_must_fail git cherry-pick picked &&\n @@ t/t3507-cherry-pick-conflict.sh: test_expect_success \\\n \ttest_must_fail git rev-parse --verify CHERRY_PICK_HEAD\n '\n \n -test_expect_success 'GIT_CHERRY_PICK_HELP suppresses CHERRY_PICK_HEAD' '\n -\tpristine_detach initial &&\n -+test_expect_success 'GIT_CHERRY_PICK_HELP respects CHERRY_PICK_HEAD' '\n -+\tgit init repo &&\n - \t(\n -+\t\tcd repo &&\n -+\t\tgit branch -M main &&\n -+\t\techo 1 >file &&\n -+\t\tgit add file &&\n -+\t\tgit commit -m 1 &&\n -+\t\techo 2 >file &&\n -+\t\tgit add file &&\n -+\t\tgit commit -m 2 &&\n -+\t\tgit checkout HEAD~ &&\n -+\t\techo 3 >file &&\n -+\t\tgit add file &&\n -+\t\tgit commit -m 3 &&\n - \t\tGIT_CHERRY_PICK_HELP=\"and then do something else\" &&\n - \t\texport GIT_CHERRY_PICK_HELP &&\n +-\t(\n +-\t\tGIT_CHERRY_PICK_HELP=\"and then do something else\" &&\n +-\t\texport GIT_CHERRY_PICK_HELP &&\n -\t\ttest_must_fail git cherry-pick picked\n -\t) &&\n -\ttest_must_fail git rev-parse --verify CHERRY_PICK_HEAD\n -+\t\ttest_must_fail git cherry-pick main &&\n -+\t\tgit rev-parse --verify CHERRY_PICK_HEAD >actual &&\n -+\t\tgit rev-parse --verify main >expect &&\n -+\t\ttest_cmp expect actual &&\n -+\t\tgit cherry-pick --abort\n -+\t)\n - '\n - \n +-'\n +-\n test_expect_success 'git reset clears CHERRY_PICK_HEAD' '\n + \tpristine_detach initial &&\n + \n\n\n builtin/revert.c | 1 +\n t/t3507-cherry-pick-conflict.sh | 27 +++++++++++++++++----------\n 2 files changed, 18 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 237f2f18d4c..ec0abe7db73 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -245,6 +245,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)\n \n \topts.action = REPLAY_PICK;\n \tsequencer_init_config(&opts);\n+\tunsetenv(\"GIT_CHERRY_PICK_HELP\");\n \tres = run_sequencer(argc, argv, &opts);\n \tif (res < 0)\n \t\tdie(_(\"cherry-pick failed\"));\ndiff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\nindex 014001b8f32..6f8035399d9 100755\n--- a/t/t3507-cherry-pick-conflict.sh\n+++ b/t/t3507-cherry-pick-conflict.sh\n@@ -76,6 +76,23 @@ test_expect_success 'advice from failed cherry-pick --no-commit' \"\n \ttest_cmp expected actual\n \"\n \n+test_expect_success 'advice from failed cherry-pick with GIT_CHERRY_PICK_HELP' \"\n+\tpristine_detach initial &&\n+\t(\n+\t\tpicked=\\$(git rev-parse --short picked) &&\n+\t\tcat <<-EOF >expected &&\n+\t\terror: could not apply \\$picked... picked\n+\t\thint: after resolving the conflicts, mark the corrected paths\n+\t\thint: with 'git add ' or 'git rm '\n+\t\thint: and commit the result with 'git commit'\n+\t\tEOF\n+\t\tGIT_CHERRY_PICK_HELP='and then do something else' &&\n+\t\texport GIT_CHERRY_PICK_HELP &&\n+\t\ttest_must_fail git cherry-pick picked 2>actual &&\n+\t\ttest_cmp expected actual\n+\t)\n+\"\n+\n test_expect_success 'failed cherry-pick sets CHERRY_PICK_HEAD' '\n \tpristine_detach initial &&\n \ttest_must_fail git cherry-pick picked &&\n@@ -109,16 +126,6 @@ test_expect_success \\\n \ttest_must_fail git rev-parse --verify CHERRY_PICK_HEAD\n '\n \n-test_expect_success 'GIT_CHERRY_PICK_HELP suppresses CHERRY_PICK_HEAD' '\n-\tpristine_detach initial &&\n-\t(\n-\t\tGIT_CHERRY_PICK_HELP=\"and then do something else\" &&\n-\t\texport GIT_CHERRY_PICK_HELP &&\n-\t\ttest_must_fail git cherry-pick picked\n-\t) &&\n-\ttest_must_fail git rev-parse --verify CHERRY_PICK_HEAD\n-'\n-\n test_expect_success 'git reset clears CHERRY_PICK_HEAD' '\n \tpristine_detach initial &&\n \n\nbase-commit: daab8a564f8bbac55f70f8bf86c070e001a9b006\n-- \ngitgitgadget\n"},{"id":"431335","messageId":"a8b260be-dae5-e717-d4cb-3ee123f93620@gmail.com","threadId":"56148","inReplyTo":"pull.1001.v2.git.1627135281887.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-07-27T19:43:35Z","receivedAt":"2021-07-27T19:43:42Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 24/07/2021 15:01, ZheNing Hu via GitGitGadget wrote:\n> From: ZheNing Hu \n> \n> If we set the value of the environment variable GIT_CHERRY_PICK_HELP\n> when using `git cherry-pick`, CHERRY_PICK_HEAD will be deleted, then\n> we will get an error when we try to use `git cherry-pick --continue`\n> or other cherr-pick command.\n> \n> So unsetenv(GIT_CHERRY_PICK_HELP) in cmd_cherry_pick(), to avoid\n> deleting CHERRY_PICK_HEAD when we are truly cherry-picking, which can\n> fix this breakage.\n> \n> Signed-off-by: ZheNing Hu \n> Mentored-by: Christian Couder \n> Mentored-by Hariom Verma :\n> ---\n> [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP\n> \n> This patch fixes the bug when git cherry-pick is used with environment\n> variable GIT_CHERRY_PICK_HELP.\n> \n> Change from last version:\n> \n> 1. Only unsetenv(GIT_CHERRY_PICK_HELP) without touching anything in\n> sequencer.c. Now git cherry-pick will ignore GIT_CHERRY_PICK_HELP,\n> \n> $ GIT_CHERRY_PICK_HELP=\"213\" git cherry-pick dev~3..dev\n> \n> will only output default advice:\n> \n> hint: after resolving the conflicts, mark the corrected paths hint: with\n> 'git add ' or 'git rm ' hint: and commit the result with 'git commit'\n> \n> This may still not be good enough, hope that cherry-pick will not advice\n> anything related to \"git commit\". Maybe we should make --no-commit as\n> cherry-pick default behavior?\n> \n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1001%2Fadlternative%2Fcherry-pick-help-fix-v2\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1001/adlternative/cherry-pick-help-fix-v2\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1001\n> \n> Range-diff vs v1:\n> \n> 1: c2a6a625ac8 ! 1: fbb9c166502 [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP\n> @@ Commit message\n> we will get an error when we try to use `git cherry-pick --continue`\n> or other cherr-pick command.\n> \n> - So add option action check in print_advice(), we will not remove\n> - CHERRY_PICK_HEAD if we are indeed cherry-picking instead of rebasing.\n> + So unsetenv(GIT_CHERRY_PICK_HELP) in cmd_cherry_pick(), to avoid\n> + deleting CHERRY_PICK_HEAD when we are truly cherry-picking, which can\n> + fix this breakage.\n> \n> Signed-off-by: ZheNing Hu \n> + Mentored-by: Christian Couder \n> + Mentored-by Hariom Verma :\n> \n> - ## sequencer.c ##\n> -@@ sequencer.c: static void print_advice(struct repository *r, int show_hint,\n> - \t\t * (typically rebase --interactive) wants to take care\n> - \t\t * of the commit itself so remove CHERRY_PICK_HEAD\n> - \t\t */\n> --\t\trefs_delete_ref(get_main_ref_store(r), \"\", \"CHERRY_PICK_HEAD\",\n> --\t\t\t\tNULL, 0);\n> -+\t\tif (opts->action != REPLAY_PICK)\n> -+\t\t\trefs_delete_ref(get_main_ref_store(r), \"\", \"CHERRY_PICK_HEAD\",\n> -+\t\t\t\t\tNULL, 0);\n> - \t\treturn;\n> - \t}\n> + ## builtin/revert.c ##\n> +@@ builtin/revert.c: int cmd_cherry_pick(int argc, const char **argv, const char *prefix)\n> \n> + \topts.action = REPLAY_PICK;\n> + \tsequencer_init_config(&opts);\n> ++\tunsetenv(\"GIT_CHERRY_PICK_HELP\");\n> + \tres = run_sequencer(argc, argv, &opts);\n> + \tif (res < 0)\n> + \t\tdie(_(\"cherry-pick failed\"));\n> \n> ## t/t3507-cherry-pick-conflict.sh ##\n> +@@ t/t3507-cherry-pick-conflict.sh: test_expect_success 'advice from failed cherry-pick --no-commit' \"\n> + \ttest_cmp expected actual\n> + \"\n> +\n> ++test_expect_success 'advice from failed cherry-pick with GIT_CHERRY_PICK_HELP' \"\n> ++\tpristine_detach initial &&\n> ++\t(\n> ++\t\tpicked=\\$(git rev-parse --short picked) &&\n> ++\t\tcat <<-EOF >expected &&\n> ++\t\terror: could not apply \\$picked... picked\n> ++\t\thint: after resolving the conflicts, mark the corrected paths\n> ++\t\thint: with 'git add ' or 'git rm '\n> ++\t\thint: and commit the result with 'git commit'\n> ++\t\tEOF\n> ++\t\tGIT_CHERRY_PICK_HELP='and then do something else' &&\n> ++\t\texport GIT_CHERRY_PICK_HELP &&\n> ++\t\ttest_must_fail git cherry-pick picked 2>actual &&\n> ++\t\ttest_cmp expected actual\n> ++\t)\n> ++\"\n> ++\n> + test_expect_success 'failed cherry-pick sets CHERRY_PICK_HEAD' '\n> + \tpristine_detach initial &&\n> + \ttest_must_fail git cherry-pick picked &&\n> @@ t/t3507-cherry-pick-conflict.sh: test_expect_success \\\n> \ttest_must_fail git rev-parse --verify CHERRY_PICK_HEAD\n> '\n> \n> -test_expect_success 'GIT_CHERRY_PICK_HELP suppresses CHERRY_PICK_HEAD' '\n> -\tpristine_detach initial &&\n> -+test_expect_success 'GIT_CHERRY_PICK_HELP respects CHERRY_PICK_HEAD' '\n> -+\tgit init repo &&\n> - \t(\n> -+\t\tcd repo &&\n> -+\t\tgit branch -M main &&\n> -+\t\techo 1 >file &&\n> -+\t\tgit add file &&\n> -+\t\tgit commit -m 1 &&\n> -+\t\techo 2 >file &&\n> -+\t\tgit add file &&\n> -+\t\tgit commit -m 2 &&\n> -+\t\tgit checkout HEAD~ &&\n> -+\t\techo 3 >file &&\n> -+\t\tgit add file &&\n> -+\t\tgit commit -m 3 &&\n> - \t\tGIT_CHERRY_PICK_HELP=\"and then do something else\" &&\n> - \t\texport GIT_CHERRY_PICK_HELP &&\n> +-\t(\n> +-\t\tGIT_CHERRY_PICK_HELP=\"and then do something else\" &&\n> +-\t\texport GIT_CHERRY_PICK_HELP &&\n> -\t\ttest_must_fail git cherry-pick picked\n> -\t) &&\n> -\ttest_must_fail git rev-parse --verify CHERRY_PICK_HEAD\n> -+\t\ttest_must_fail git cherry-pick main &&\n> -+\t\tgit rev-parse --verify CHERRY_PICK_HEAD >actual &&\n> -+\t\tgit rev-parse --verify main >expect &&\n> -+\t\ttest_cmp expect actual &&\n> -+\t\tgit cherry-pick --abort\n> -+\t)\n> - '\n> -\n> +-'\n> +-\n> test_expect_success 'git reset clears CHERRY_PICK_HEAD' '\n> + \tpristine_detach initial &&\n> +\n> \n> \n> builtin/revert.c | 1 +\n> t/t3507-cherry-pick-conflict.sh | 27 +++++++++++++++++----------\n> 2 files changed, 18 insertions(+), 10 deletions(-)\n> \n> diff --git a/builtin/revert.c b/builtin/revert.c\n> index 237f2f18d4c..ec0abe7db73 100644\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -245,6 +245,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)\n> \n> \topts.action = REPLAY_PICK;\n> \tsequencer_init_config(&opts);\n> +\tunsetenv(\"GIT_CHERRY_PICK_HELP\");\n\nThis will break git-rebase--preserve-merges.sh which uses \nGIT_CHERRY_PICK_HELP to set the help and ensure CHERRY_PICK_HEAD is \nremoved when picking commits. I'm a bit confused as to what the problem \nis - how is 'git cherry-pick' being run with GIT_CHERRY_PICK_HELP set in \nthe environment outside of a rebase (your explanation in [1] does not \nmention how GIT_CHERRY_PICK_HELP is set)? As far as I can see 'git \nrebase -i' does not set it so the only case I can think of is \ncherry-picking from an exec command while running 'git rebase -p'\n\nBest Wishes\n\nPhillip\n\n[1] \nhttps://lore.kernel.org/git/CAOLTT8Ty47fyY7T3d68CYPKh9k+HAHsnCLJ=F0KaLm+0gp3+EQ@mail.gmail.com/\n\n> \tres = run_sequencer(argc, argv, &opts);\n> \tif (res < 0)\n> \t\tdie(_(\"cherry-pick failed\"));\n> diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\n> index 014001b8f32..6f8035399d9 100755\n> --- a/t/t3507-cherry-pick-conflict.sh\n> +++ b/t/t3507-cherry-pick-conflict.sh\n> @@ -76,6 +76,23 @@ test_expect_success 'advice from failed cherry-pick --no-commit' \"\n> \ttest_cmp expected actual\n> \"\n> \n> +test_expect_success 'advice from failed cherry-pick with GIT_CHERRY_PICK_HELP' \"\n> +\tpristine_detach initial &&\n> +\t(\n> +\t\tpicked=\\$(git rev-parse --short picked) &&\n> +\t\tcat <<-EOF >expected &&\n> +\t\terror: could not apply \\$picked... picked\n> +\t\thint: after resolving the conflicts, mark the corrected paths\n> +\t\thint: with 'git add ' or 'git rm '\n> +\t\thint: and commit the result with 'git commit'\n> +\t\tEOF\n> +\t\tGIT_CHERRY_PICK_HELP='and then do something else' &&\n> +\t\texport GIT_CHERRY_PICK_HELP &&\n> +\t\ttest_must_fail git cherry-pick picked 2>actual &&\n> +\t\ttest_cmp expected actual\n> +\t)\n> +\"\n> +\n> test_expect_success 'failed cherry-pick sets CHERRY_PICK_HEAD' '\n> \tpristine_detach initial &&\n> \ttest_must_fail git cherry-pick picked &&\n> @@ -109,16 +126,6 @@ test_expect_success \\\n> \ttest_must_fail git rev-parse --verify CHERRY_PICK_HEAD\n> '\n> \n> -test_expect_success 'GIT_CHERRY_PICK_HELP suppresses CHERRY_PICK_HEAD' '\n> -\tpristine_detach initial &&\n> -\t(\n> -\t\tGIT_CHERRY_PICK_HELP=\"and then do something else\" &&\n> -\t\texport GIT_CHERRY_PICK_HELP &&\n> -\t\ttest_must_fail git cherry-pick picked\n> -\t) &&\n> -\ttest_must_fail git rev-parse --verify CHERRY_PICK_HEAD\n> -'\n> -\n> test_expect_success 'git reset clears CHERRY_PICK_HEAD' '\n> \tpristine_detach initial &&\n> \n> \n> base-commit: daab8a564f8bbac55f70f8bf86c070e001a9b006\n> \n"},{"id":"431338","messageId":"xmqq1r7jebbf.fsf@gitster.g","threadId":"56148","inReplyTo":"a8b260be-dae5-e717-d4cb-3ee123f93620@gmail.com","subject":"Re: [PATCH v2] [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-27T20:44:36Z","receivedAt":"2021-07-27T20:44:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> This will break git-rebase--preserve-merges.sh which uses\n> GIT_CHERRY_PICK_HELP to set the help and ensure CHERRY_PICK_HEAD is \n> removed when picking commits.\n\nAhh, I didn't realize we still had scripted rebase backends that\ncalled cherry-pick as an executable. I was hoping that all rebase\nbackends by now would be calling into the cherry-pick machinery\ndirectly, bypassing cmd_cherry_pick(), and that was why I suggested\nto catch stray one the end-users set manually in the environment\nand clear it there.\n\n> I'm a bit confused as to what the\n> problem is - how is 'git cherry-pick' being run with\n> GIT_CHERRY_PICK_HELP set in the environment outside of a rebase (your\n> explanation in [1] does not mention how GIT_CHERRY_PICK_HELP is set)?\n\nI didn't press for the information too hard, but I guessed that it\nwas perhaps because somebody like stackoverflow suggested to set a\nmessage in their environment to get a \"better message.\"\n"},{"id":"431339","messageId":"xmqqtukfcvzv.fsf@gitster.g","threadId":"56148","inReplyTo":"xmqq1r7jebbf.fsf@gitster.g","subject":"Re: [PATCH v2] [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-27T21:00:52Z","receivedAt":"2021-07-27T21:02:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Phillip Wood writes:\n>\n>> This will break git-rebase--preserve-merges.sh which uses\n>> GIT_CHERRY_PICK_HELP to set the help and ensure CHERRY_PICK_HEAD is \n>> removed when picking commits.\n>\n> Ahh, I didn't realize we still had scripted rebase backends that\n> called cherry-pick as an executable. I was hoping that all rebase\n> backends by now would be calling into the cherry-pick machinery\n> directly, bypassing cmd_cherry_pick(), and that was why I suggested\n> to catch stray one the end-users set manually in the environment\n> and clear it there.\n>\n>> I'm a bit confused as to what the\n>> problem is - how is 'git cherry-pick' being run with\n>> GIT_CHERRY_PICK_HELP set in the environment outside of a rebase (your\n>> explanation in [1] does not mention how GIT_CHERRY_PICK_HELP is set)?\n>\n> I didn't press for the information too hard, but I guessed that it\n> was perhaps because somebody like stackoverflow suggested to set a\n> message in their environment to get a \"better message.\"\n\nA good way forward may be to relieve sequencer.c::print_advice() of\nthe responsibility of optinally removing CHERRY_PICK_HEAD; make it a\nseparate function that bases its decision on a more direct cue, not\non the presense of a custom message in GIT_CHERRY_PICK_HELP, make\ndo_pick_commit(), which is the sole caller of print_advice(), call\nit after calling print_advice().\n\nI do not offhand know what that \"direct cue\" should be, but we may\nalready have an appropriate field in the replay_opts structure;\n\"replay.action is neither REVERT nor PICK\" could be a good enough\napproximation, I dunno.\n\nOtherwise we can allocate a new bit in the structure, have relevant\ncallers set it, and teach cherry-pick an unadvertised command line\noption that sets the bit, and use that option only from\ngit-rebase--preserve-merges when it makes a call to cherry-pick.\nWhen \"rebase -p\" is either retired or rewritten in C, we can retire\nthe option from cherry-pick.\n\nWorkable?\n"},{"id":"431387","messageId":"CAOLTT8T9Fu4-r5-2PhNmopW751TBN1LRrwBU70HuKZ48JD1X5Q@mail.gmail.com","threadId":"56148","inReplyTo":"a8b260be-dae5-e717-d4cb-3ee123f93620@gmail.com","subject":"Re: [PATCH v2] [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-07-28T07:39:22Z","receivedAt":"2021-07-28T07:38:52Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Phillip Wood 于2021年7月28日周三 上午3:43写道：\n>\n> > diff --git a/builtin/revert.c b/builtin/revert.c\n> > index 237f2f18d4c..ec0abe7db73 100644\n> > --- a/builtin/revert.c\n> > +++ b/builtin/revert.c\n> > @@ -245,6 +245,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)\n> >\n> > opts.action = REPLAY_PICK;\n> > sequencer_init_config(&opts);\n> > + unsetenv(\"GIT_CHERRY_PICK_HELP\");\n>\n> This will break git-rebase--preserve-merges.sh which uses\n> GIT_CHERRY_PICK_HELP to set the help and ensure CHERRY_PICK_HEAD is\n> removed when picking commits. I'm a bit confused as to what the problem\n\nYeah, I thought it would call some rebase-related code before, I\ndidn’t expect it to\ncall cherry-pick. On the other hand, I passed all tests, so I ignore\nit, there should be\na test for it.\n\n> is - how is 'git cherry-pick' being run with GIT_CHERRY_PICK_HELP set in\n> the environment outside of a rebase (your explanation in [1] does not\n> mention how GIT_CHERRY_PICK_HELP is set)? As far as I can see 'git\n> rebase -i' does not set it so the only case I can think of is\n> cherry-picking from an exec command while running 'git rebase -p'\n>\n\nAh, because I want to find a way to suppress its advice messages about\n\"git commit\",\nand I don’t think anyone else is using this \"feature\".\n\n> Best Wishes\n>\n> Phillip\n>\n> [1]\n> https://lore.kernel.org/git/CAOLTT8Ty47fyY7T3d68CYPKh9k+HAHsnCLJ=F0KaLm+0gp3+EQ@mail.gmail.com/\n>\n\nThanks.\n--\nZheNing Hu\n"},{"id":"431390","messageId":"8893cce8-61a7-3469-0c2e-8f6fe9f1b295@gmail.com","threadId":"56148","inReplyTo":"CAOLTT8T9Fu4-r5-2PhNmopW751TBN1LRrwBU70HuKZ48JD1X5Q@mail.gmail.com","subject":"Re: [PATCH v2] [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-07-28T09:46:39Z","receivedAt":"2021-07-28T09:46:45Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi ZheNing\n\nOn 28/07/2021 08:39, ZheNing Hu wrote:\n> Phillip Wood 于2021年7月28日周三 上午3:43写道：\n>>\n>>> diff --git a/builtin/revert.c b/builtin/revert.c\n>>> index 237f2f18d4c..ec0abe7db73 100644\n>>> --- a/builtin/revert.c\n>>> +++ b/builtin/revert.c\n>>> @@ -245,6 +245,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)\n>>>\n>>> opts.action = REPLAY_PICK;\n>>> sequencer_init_config(&opts);\n>>> + unsetenv(\"GIT_CHERRY_PICK_HELP\");\n>>\n>> This will break git-rebase--preserve-merges.sh which uses\n>> GIT_CHERRY_PICK_HELP to set the help and ensure CHERRY_PICK_HEAD is\n>> removed when picking commits. I'm a bit confused as to what the problem\n> \n> Yeah, I thought it would call some rebase-related code before, I\n> didn’t expect it to\n> call cherry-pick. On the other hand, I passed all tests, so I ignore\n> it, there should be\n> a test for it.\n> \n>> is - how is 'git cherry-pick' being run with GIT_CHERRY_PICK_HELP set in\n>> the environment outside of a rebase (your explanation in [1] does not\n>> mention how GIT_CHERRY_PICK_HELP is set)? As far as I can see 'git\n>> rebase -i' does not set it so the only case I can think of is\n>> cherry-picking from an exec command while running 'git rebase -p'\n>>\n> \n> Ah, because I want to find a way to suppress its advice messages about\n> \"git commit\",\n> and I don’t think anyone else is using this \"feature\".\n\nI'd welcome a patch to improve the advice. I suspect the current advice \npredates the introduction of the '--continue' flag for cherry-pick. I \nthink that would be a better route forward as it would benefit all \nusers. Setting GIT_CHERRY_PICK_HELP is undocumented and has always \nremoved CHERRY_PICK_HEAD since CHERRY_PICK_HEAD was introduced in commit\n7e5c0cbf (Introduce CHERRY_PICK_HEAD, 2011-02-19).\n\nBest Wishes\n\nPhillip\n\n\n>> Best Wishes\n>>\n>> Phillip\n>>\n>> [1]\n>> https://lore.kernel.org/git/CAOLTT8Ty47fyY7T3d68CYPKh9k+HAHsnCLJ=F0KaLm+0gp3+EQ@mail.gmail.com/\n>>\n> \n> Thanks.\n> --\n> ZheNing Hu\n> \n"},{"id":"431391","messageId":"e5c6d620-6c02-8824-741a-4136081edd02@gmail.com","threadId":"56148","inReplyTo":"xmqqtukfcvzv.fsf@gitster.g","subject":"Re: [PATCH v2] [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-07-28T09:56:09Z","receivedAt":"2021-07-28T09:56:17Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 27/07/2021 22:00, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n>> Phillip Wood writes:\n>>\n>>> This will break git-rebase--preserve-merges.sh which uses\n>>> GIT_CHERRY_PICK_HELP to set the help and ensure CHERRY_PICK_HEAD is\n>>> removed when picking commits.\n>>\n>> Ahh, I didn't realize we still had scripted rebase backends that\n>> called cherry-pick as an executable. I was hoping that all rebase\n>> backends by now would be calling into the cherry-pick machinery\n>> directly, bypassing cmd_cherry_pick(), and that was why I suggested\n>> to catch stray one the end-users set manually in the environment\n>> and clear it there.\n>>\n>>> I'm a bit confused as to what the\n>>> problem is - how is 'git cherry-pick' being run with\n>>> GIT_CHERRY_PICK_HELP set in the environment outside of a rebase (your\n>>> explanation in [1] does not mention how GIT_CHERRY_PICK_HELP is set)?\n>>\n>> I didn't press for the information too hard, but I guessed that it\n>> was perhaps because somebody like stackoverflow suggested to set a\n>> message in their environment to get a \"better message.\"\n> \n> A good way forward may be to relieve sequencer.c::print_advice() of\n> the responsibility of optinally removing CHERRY_PICK_HEAD; make it a\n> separate function that bases its decision on a more direct cue, not\n> on the presense of a custom message in GIT_CHERRY_PICK_HELP, make\n> do_pick_commit(), which is the sole caller of print_advice(), call\n> it after calling print_advice().\n> \n> I do not offhand know what that \"direct cue\" should be, but we may\n> already have an appropriate field in the replay_opts structure;\n> \"replay.action is neither REVERT nor PICK\" could be a good enough\n> approximation, I dunno.\n> \n> Otherwise we can allocate a new bit in the structure, have relevant\n> callers set it, and teach cherry-pick an unadvertised command line\n> option that sets the bit, and use that option only from\n> git-rebase--preserve-merges when it makes a call to cherry-pick.\n> When \"rebase -p\" is either retired or rewritten in C, we can retire\n> the option from cherry-pick.\n>\n> Workable?\n\nMost of the time the builtin rebase should not be writing \nCHERRY_PICK_HEAD in the first place (it needs it when a commit becomes \nempty but not otherwise). For 'rebase -p' adding a command line option \nto cherry-pick as you suggest is probably the cleanest solution - in the \nshort term 'rebase -i' could set it until we refactor the code that \ncreates CHERRY_PICK_HEAD. One thing to note is that I think \nGIT_CHERRY_PICK_HELP was introduced to allow assist scripted rebase like \nporcelains rather than as a way to allow users to customize the help and \nsetting it has always removed CHERRY_PICK_HEAD. There are however no \nusers of GIT_CHERRY_PICK_HELP on codesearch.debian.org\n\nBest Wishes\n\nPhillip\n\n"},{"id":"431392","messageId":"CAOLTT8RRbwss5sJAM-a=LQrZjr0fyOP4C_x8oRGdLFFnpc+M3g@mail.gmail.com","threadId":"56148","inReplyTo":"xmqqtukfcvzv.fsf@gitster.g","subject":"Re: [PATCH v2] [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-07-28T10:56:02Z","receivedAt":"2021-07-28T10:55:58Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Junio C Hamano 于2021年7月28日周三 上午5:00写道：\n>\n> Junio C Hamano writes:\n>\n> > Phillip Wood writes:\n> >\n> >> This will break git-rebase--preserve-merges.sh which uses\n> >> GIT_CHERRY_PICK_HELP to set the help and ensure CHERRY_PICK_HEAD is\n> >> removed when picking commits.\n> >\n> > Ahh, I didn't realize we still had scripted rebase backends that\n> > called cherry-pick as an executable. I was hoping that all rebase\n> > backends by now would be calling into the cherry-pick machinery\n> > directly, bypassing cmd_cherry_pick(), and that was why I suggested\n> > to catch stray one the end-users set manually in the environment\n> > and clear it there.\n> >\n> >> I'm a bit confused as to what the\n> >> problem is - how is 'git cherry-pick' being run with\n> >> GIT_CHERRY_PICK_HELP set in the environment outside of a rebase (your\n> >> explanation in [1] does not mention how GIT_CHERRY_PICK_HELP is set)?\n> >\n> > I didn't press for the information too hard, but I guessed that it\n> > was perhaps because somebody like stackoverflow suggested to set a\n> > message in their environment to get a \"better message.\"\n>\n> A good way forward may be to relieve sequencer.c::print_advice() of\n> the responsibility of optinally removing CHERRY_PICK_HEAD; make it a\n> separate function that bases its decision on a more direct cue, not\n> on the presense of a custom message in GIT_CHERRY_PICK_HELP, make\n> do_pick_commit(), which is the sole caller of print_advice(), call\n> it after calling print_advice().\n>\n\nI think this function \"check_need_delete_cherry_pick_head()\" should be before\nprint_advice(), like this:\n\n+ const char *help_msgs = NULL;\n+\n error(command == TODO_REVERT\n ? _(\"could not revert %s... %s\")\n : _(\"could not apply %s... %s\"),\n short_commit_name(commit), msg.subject);\n- print_advice(r, res == 1, opts);\n+ if (((opts->action == REPLAY_PICK &&\n+ !opts->rebase_preserve_merges_mode) ||\n+ (help_msgs = check_need_delete_cherry_pick_head(r))) &&\n+ res == 1)\n+ print_advice(opts, help_msgs);\n\n> I do not offhand know what that \"direct cue\" should be, but we may\n> already have an appropriate field in the replay_opts structure;\n> \"replay.action is neither REVERT nor PICK\" could be a good enough\n> approximation, I dunno.\n>\n> Otherwise we can allocate a new bit in the structure, have relevant\n> callers set it, and teach cherry-pick an unadvertised command line\n> option that sets the bit, and use that option only from\n> git-rebase--preserve-merges when it makes a call to cherry-pick.\n> When \"rebase -p\" is either retired or rewritten in C, we can retire\n> the option from cherry-pick.\n>\n\nI think this one can be easily achieved.\n\n> Workable?\n\nThanks.\n--\nZheNing Hu\n"},{"id":"431393","messageId":"CAOLTT8T-NYEyxabCbSuLkXVKf15LMqf75a0gSR0aGCmeC8dSZw@mail.gmail.com","threadId":"56148","inReplyTo":"8893cce8-61a7-3469-0c2e-8f6fe9f1b295@gmail.com","subject":"Re: [PATCH v2] [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-07-28T11:01:16Z","receivedAt":"2021-07-28T11:00:47Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Phillip Wood 于2021年7月28日周三 下午5:46写道：\n>\n> Hi ZheNing\n>\n> > Ah, because I want to find a way to suppress its advice messages about\n> > \"git commit\",\n> > and I don’t think anyone else is using this \"feature\".\n>\n> I'd welcome a patch to improve the advice. I suspect the current advice\n> predates the introduction of the '--continue' flag for cherry-pick. I\n> think that would be a better route forward as it would benefit all\n> users. Setting GIT_CHERRY_PICK_HELP is undocumented and has always\n> removed CHERRY_PICK_HEAD since CHERRY_PICK_HEAD was introduced in commit\n> 7e5c0cbf (Introduce CHERRY_PICK_HEAD, 2011-02-19).\n>\n\nAfter I modify the interface of print_advice() in the way suggested by\nJunio, I can provide a\nhelp_msg parameter for print_advice(), and maybe we can use it to\nprovide better advice later.\nSomething like this:\n\n+static void print_advice(struct replay_opts *opts, const char *help_msgs)\n+{\n+ if (help_msgs)\n+ fprintf(stderr, \"%s\\n\", help_msgs);\n+ else if (opts->no_commit)\n+ advise(_(\"after resolving the conflicts, mark the\ncorrected paths\\n\"\n+ \"with 'git add ' or 'git rm '\"));\n+ else\n+ advise(_(\"after resolving the conflicts, mark the\ncorrected paths\\n\"\n+ \"with 'git add ' or 'git rm '\\n\"\n+ \"and commit the result with 'git commit'\"));\n }\n\n> Best Wishes\n>\n> Phillip\n>\n\nThanks.\n--\nZheNing Hu\n"},{"id":"431394","messageId":"CAOLTT8SR86q+q37traSpk7iWEzAaGTtZzAWrck+0LnqgSefKRA@mail.gmail.com","threadId":"56148","inReplyTo":"xmqqtukfcvzv.fsf@gitster.g","subject":"Re: [PATCH v2] [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-07-28T11:34:25Z","receivedAt":"2021-07-28T11:33:55Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Junio C Hamano 于2021年7月28日周三 上午5:00写道：\n\n> Otherwise we can allocate a new bit in the structure, have relevant\n> callers set it, and teach cherry-pick an unadvertised command line\n> option that sets the bit, and use that option only from\n> git-rebase--preserve-merges when it makes a call to cherry-pick.\n> When \"rebase -p\" is either retired or rewritten in C, we can retire\n> the option from cherry-pick.\n>\n\nJust use a PARSE_OPT_HIDDEN cannot prevent the user from using\nthe option... Is there a better way?\n\n--\nZheNing Hu\n"},{"id":"431404","messageId":"3627fd65-54bc-2dd0-85fc-93ced57fc7ac@gmail.com","threadId":"56148","inReplyTo":"CAOLTT8T-NYEyxabCbSuLkXVKf15LMqf75a0gSR0aGCmeC8dSZw@mail.gmail.com","subject":"Re: [PATCH v2] [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-07-28T16:52:25Z","receivedAt":"2021-07-28T16:52:36Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 28/07/2021 12:01, ZheNing Hu wrote:\n> Phillip Wood 于2021年7月28日周三 下午5:46写道：\n>>\n>> Hi ZheNing\n>>\n>>> Ah, because I want to find a way to suppress its advice messages about\n>>> \"git commit\",\n>>> and I don’t think anyone else is using this \"feature\".\n>>\n>> I'd welcome a patch to improve the advice. I suspect the current advice\n>> predates the introduction of the '--continue' flag for cherry-pick. I\n>> think that would be a better route forward as it would benefit all\n>> users. Setting GIT_CHERRY_PICK_HELP is undocumented and has always\n>> removed CHERRY_PICK_HEAD since CHERRY_PICK_HEAD was introduced in commit\n>> 7e5c0cbf (Introduce CHERRY_PICK_HEAD, 2011-02-19).\n>>\n> \n> After I modify the interface of print_advice() in the way suggested by\n> Junio, I can provide a\n> help_msg parameter for print_advice(), and maybe we can use it to\n> provide better advice later.\n\nI think that we could change the advice now to suggest using 'git \ncherry-pick --continue' instead of running 'git commit'. I think that in \nthe long term we want to move away from being able to customize the \nadvice when 'git rebase -p' is retired.\n\nBest Wishes\n\nPhillip\n\n> Something like this:\n> \n> +static void print_advice(struct replay_opts *opts, const char *help_msgs)\n> +{\n> + if (help_msgs)\n> + fprintf(stderr, \"%s\\n\", help_msgs);\n> + else if (opts->no_commit)\n> + advise(_(\"after resolving the conflicts, mark the\n> corrected paths\\n\"\n> + \"with 'git add ' or 'git rm '\"));\n> + else\n> + advise(_(\"after resolving the conflicts, mark the\n> corrected paths\\n\"\n> + \"with 'git add ' or 'git rm '\\n\"\n> + \"and commit the result with 'git commit'\"));\n> }\n> \n>> Best Wishes\n>>\n>> Phillip\n>>\n> \n> Thanks.\n> --\n> ZheNing Hu\n> \n"},{"id":"431408","messageId":"xmqqr1fi9wsj.fsf@gitster.g","threadId":"56148","inReplyTo":"CAOLTT8RRbwss5sJAM-a=LQrZjr0fyOP4C_x8oRGdLFFnpc+M3g@mail.gmail.com","subject":"Re: [PATCH v2] [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-28T17:24:12Z","receivedAt":"2021-07-28T17:24:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"ZheNing Hu writes:\n\n> I think this function \"check_need_delete_cherry_pick_head()\" should be before\n> print_advice(), like this:\n>\n> + const char *help_msgs = NULL;\n> +\n> error(command == TODO_REVERT\n> ? _(\"could not revert %s... %s\")\n> : _(\"could not apply %s... %s\"),\n> short_commit_name(commit), msg.subject);\n> - print_advice(r, res == 1, opts);\n> + if (((opts->action == REPLAY_PICK &&\n> + !opts->rebase_preserve_merges_mode) ||\n> + (help_msgs = check_need_delete_cherry_pick_head(r))) &&\n> + res == 1)\n> + print_advice(opts, help_msgs);\n\nSorry, but I am not sure what problem this separation is trying to\nsolve.\n\nThe root cause of the issue we have, I think, is because the\ndecision to delete or keep the cherry-pick-head pseudoref is tied to\nwhat message we give to users in the current code, and the suggested\nsplit of concern is to limit print_advice() to only print the advice\nmessage, and a new helper to decide and remove the pseudoref,\nwithout relying on what is in the GIT_CHERRY_PICK_HELP environment.\n\nIt is unclear where you are making the decision to keep or remove\nthe pseudoref in the above arrangement that lets the new\ncheck_need_delete_cherry_pick_head() decide if the advice is printed\nor not.\n"},{"id":"431409","messageId":"xmqqmtq69wpd.fsf@gitster.g","threadId":"56148","inReplyTo":"CAOLTT8SR86q+q37traSpk7iWEzAaGTtZzAWrck+0LnqgSefKRA@mail.gmail.com","subject":"Re: [PATCH v2] [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-28T17:26:06Z","receivedAt":"2021-07-28T17:26:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"ZheNing Hu writes:\n\n> Just use a PARSE_OPT_HIDDEN cannot prevent the user from using\n> the option... Is there a better way?\n\nIf a command can be invoked from a script, you can invoke it\ninteractively the same way. Not advertising on short help, not\ncompleting in the completion and documenting it for internal use is\nthe standard arrangement for such \"implementation detail only\"\noptions.\n"}]}