[PATCH v2] [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP

["ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com>]

From: ZheNing Hu <adlternative@gmail.com>

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 <adlternative@gmail.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by Hariom Verma <hariom18599@gmail.com>:
---
    [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 <adlternative@gmail.com>
     +    Mentored-by: Christian Couder <christian.couder@gmail.com>
     +    Mentored-by Hariom Verma <hariom18599@gmail.com>:
      
     - ## 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 <paths>' or 'git rm <paths>'
     ++		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");
 	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 <paths>' or 'git rm <paths>'
+		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
-- 
gitgitgadget