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

[Phillip Wood <phillip.wood123@gmail.com>]

On 24/07/2021 15:01, ZheNing Hu via GitGitGadget wrote:
> 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");

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 <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
>