From: Phillip Wood Date: Tue, 27 Jul 2021 19:43:35 GMT Subject: Re: [PATCH v2] [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP Message-ID: In-Reply-To: On 24/07/2021 15:01, ZheNing Hu via GitGitGadget wrote: > From: ZheNing Hu > > If we set the value of the environment variable GIT_CHERRY_PICK_HELP > when using `git cherry-pick`, CHERRY_PICK_HEAD will be deleted, then > we will get an error when we try to use `git cherry-pick --continue` > or other cherr-pick command. > > So unsetenv(GIT_CHERRY_PICK_HELP) in cmd_cherry_pick(), to avoid > deleting CHERRY_PICK_HEAD when we are truly cherry-picking, which can > fix this breakage. > > Signed-off-by: ZheNing Hu > Mentored-by: Christian Couder > Mentored-by Hariom Verma : > --- > [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP > > This patch fixes the bug when git cherry-pick is used with environment > variable GIT_CHERRY_PICK_HELP. > > Change from last version: > > 1. Only unsetenv(GIT_CHERRY_PICK_HELP) without touching anything in > sequencer.c. Now git cherry-pick will ignore GIT_CHERRY_PICK_HELP, > > $ GIT_CHERRY_PICK_HELP="213" git cherry-pick dev~3..dev > > will only output default advice: > > hint: after resolving the conflicts, mark the corrected paths hint: with > 'git add ' or 'git rm ' hint: and commit the result with 'git commit' > > This may still not be good enough, hope that cherry-pick will not advice > anything related to "git commit". Maybe we should make --no-commit as > cherry-pick default behavior? > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1001%2Fadlternative%2Fcherry-pick-help-fix-v2 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1001/adlternative/cherry-pick-help-fix-v2 > Pull-Request: https://github.com/gitgitgadget/git/pull/1001 > > Range-diff vs v1: > > 1: c2a6a625ac8 ! 1: fbb9c166502 [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP > @@ Commit message > we will get an error when we try to use `git cherry-pick --continue` > or other cherr-pick command. > > - So add option action check in print_advice(), we will not remove > - CHERRY_PICK_HEAD if we are indeed cherry-picking instead of rebasing. > + So unsetenv(GIT_CHERRY_PICK_HELP) in cmd_cherry_pick(), to avoid > + deleting CHERRY_PICK_HEAD when we are truly cherry-picking, which can > + fix this breakage. > > Signed-off-by: ZheNing Hu > + Mentored-by: Christian Couder > + Mentored-by Hariom Verma : > > - ## sequencer.c ## > -@@ sequencer.c: static void print_advice(struct repository *r, int show_hint, > - * (typically rebase --interactive) wants to take care > - * of the commit itself so remove CHERRY_PICK_HEAD > - */ > -- refs_delete_ref(get_main_ref_store(r), "", "CHERRY_PICK_HEAD", > -- NULL, 0); > -+ if (opts->action != REPLAY_PICK) > -+ refs_delete_ref(get_main_ref_store(r), "", "CHERRY_PICK_HEAD", > -+ NULL, 0); > - return; > - } > + ## builtin/revert.c ## > +@@ builtin/revert.c: int cmd_cherry_pick(int argc, const char **argv, const char *prefix) > > + opts.action = REPLAY_PICK; > + sequencer_init_config(&opts); > ++ unsetenv("GIT_CHERRY_PICK_HELP"); > + res = run_sequencer(argc, argv, &opts); > + if (res < 0) > + die(_("cherry-pick failed")); > > ## t/t3507-cherry-pick-conflict.sh ## > +@@ t/t3507-cherry-pick-conflict.sh: test_expect_success 'advice from failed cherry-pick --no-commit' " > + test_cmp expected actual > + " > + > ++test_expect_success 'advice from failed cherry-pick with GIT_CHERRY_PICK_HELP' " > ++ pristine_detach initial && > ++ ( > ++ picked=\$(git rev-parse --short picked) && > ++ cat <<-EOF >expected && > ++ error: could not apply \$picked... picked > ++ hint: after resolving the conflicts, mark the corrected paths > ++ hint: with 'git add ' or 'git rm ' > ++ hint: and commit the result with 'git commit' > ++ EOF > ++ GIT_CHERRY_PICK_HELP='and then do something else' && > ++ export GIT_CHERRY_PICK_HELP && > ++ test_must_fail git cherry-pick picked 2>actual && > ++ test_cmp expected actual > ++ ) > ++" > ++ > + test_expect_success 'failed cherry-pick sets CHERRY_PICK_HEAD' ' > + pristine_detach initial && > + test_must_fail git cherry-pick picked && > @@ t/t3507-cherry-pick-conflict.sh: test_expect_success \ > test_must_fail git rev-parse --verify CHERRY_PICK_HEAD > ' > > -test_expect_success 'GIT_CHERRY_PICK_HELP suppresses CHERRY_PICK_HEAD' ' > - pristine_detach initial && > -+test_expect_success 'GIT_CHERRY_PICK_HELP respects CHERRY_PICK_HEAD' ' > -+ git init repo && > - ( > -+ cd repo && > -+ git branch -M main && > -+ echo 1 >file && > -+ git add file && > -+ git commit -m 1 && > -+ echo 2 >file && > -+ git add file && > -+ git commit -m 2 && > -+ git checkout HEAD~ && > -+ echo 3 >file && > -+ git add file && > -+ git commit -m 3 && > - GIT_CHERRY_PICK_HELP="and then do something else" && > - export GIT_CHERRY_PICK_HELP && > +- ( > +- GIT_CHERRY_PICK_HELP="and then do something else" && > +- export GIT_CHERRY_PICK_HELP && > - test_must_fail git cherry-pick picked > - ) && > - test_must_fail git rev-parse --verify CHERRY_PICK_HEAD > -+ test_must_fail git cherry-pick main && > -+ git rev-parse --verify CHERRY_PICK_HEAD >actual && > -+ git rev-parse --verify main >expect && > -+ test_cmp expect actual && > -+ git cherry-pick --abort > -+ ) > - ' > - > +-' > +- > test_expect_success 'git reset clears CHERRY_PICK_HEAD' ' > + pristine_detach initial && > + > > > builtin/revert.c | 1 + > t/t3507-cherry-pick-conflict.sh | 27 +++++++++++++++++---------- > 2 files changed, 18 insertions(+), 10 deletions(-) > > diff --git a/builtin/revert.c b/builtin/revert.c > index 237f2f18d4c..ec0abe7db73 100644 > --- a/builtin/revert.c > +++ b/builtin/revert.c > @@ -245,6 +245,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix) > > opts.action = REPLAY_PICK; > sequencer_init_config(&opts); > + unsetenv("GIT_CHERRY_PICK_HELP"); This will break git-rebase--preserve-merges.sh which uses GIT_CHERRY_PICK_HELP to set the help and ensure CHERRY_PICK_HEAD is removed when picking commits. I'm a bit confused as to what the problem is - how is 'git cherry-pick' being run with GIT_CHERRY_PICK_HELP set in the environment outside of a rebase (your explanation in [1] does not mention how GIT_CHERRY_PICK_HELP is set)? As far as I can see 'git rebase -i' does not set it so the only case I can think of is cherry-picking from an exec command while running 'git rebase -p' Best Wishes Phillip [1] https://lore.kernel.org/git/CAOLTT8Ty47fyY7T3d68CYPKh9k+HAHsnCLJ=F0KaLm+0gp3+EQ@mail.gmail.com/ > res = run_sequencer(argc, argv, &opts); > if (res < 0) > die(_("cherry-pick failed")); > diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh > index 014001b8f32..6f8035399d9 100755 > --- a/t/t3507-cherry-pick-conflict.sh > +++ b/t/t3507-cherry-pick-conflict.sh > @@ -76,6 +76,23 @@ test_expect_success 'advice from failed cherry-pick --no-commit' " > test_cmp expected actual > " > > +test_expect_success 'advice from failed cherry-pick with GIT_CHERRY_PICK_HELP' " > + pristine_detach initial && > + ( > + picked=\$(git rev-parse --short picked) && > + cat <<-EOF >expected && > + error: could not apply \$picked... picked > + hint: after resolving the conflicts, mark the corrected paths > + hint: with 'git add ' or 'git rm ' > + hint: and commit the result with 'git commit' > + EOF > + GIT_CHERRY_PICK_HELP='and then do something else' && > + export GIT_CHERRY_PICK_HELP && > + test_must_fail git cherry-pick picked 2>actual && > + test_cmp expected actual > + ) > +" > + > test_expect_success 'failed cherry-pick sets CHERRY_PICK_HEAD' ' > pristine_detach initial && > test_must_fail git cherry-pick picked && > @@ -109,16 +126,6 @@ test_expect_success \ > test_must_fail git rev-parse --verify CHERRY_PICK_HEAD > ' > > -test_expect_success 'GIT_CHERRY_PICK_HELP suppresses CHERRY_PICK_HEAD' ' > - pristine_detach initial && > - ( > - GIT_CHERRY_PICK_HELP="and then do something else" && > - export GIT_CHERRY_PICK_HELP && > - test_must_fail git cherry-pick picked > - ) && > - test_must_fail git rev-parse --verify CHERRY_PICK_HEAD > -' > - > test_expect_success 'git reset clears CHERRY_PICK_HEAD' ' > pristine_detach initial && > > > base-commit: daab8a564f8bbac55f70f8bf86c070e001a9b006 >