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

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

>
> "ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > 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.
>
> I think that the GIT_CHERRY_PICK_HELP is an implemention detail for
> various forms of rebase to use cherry-pick as a backend and not for
> use by end users.
>

> I strongly suspect that the right solution for the breakage is to
> unsetenv(GIT_CHERRY_PICK_HELP) in cmd_cherry_pick() without touching
> sequencer.c at all.
>
> It _is_ ugly that a helper that is responsible for emitting an
> advise message also makes a decision whether the pseudo-ref gets
> deleted or not, but a fix to that problem should be done byy making
> the logic for the decision less complex, not more.
>

> Junio C Hamano <gitster@pobox.com> 于2021年7月23日周五 上午5:25写道：
> >
> > "ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >
> > > 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.
> >
> > I think that the GIT_CHERRY_PICK_HELP is an implemention detail for
> > various forms of rebase to use cherry-pick as a backend and not for
> > use by end users.
> >
> 
> But someone complain to me that the cherry-pick advice is not good enough.

> Think about a git newbie is cherry-picking a patch series containing
> several commits,
> E.g.
> 
> git cherry-pick dev~3..dev
> 
> And then he (she) will see these advice info:
> 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'
> 
> After he resolving git conflict, execute 'git commit' according to the
> prompt, A terrible thing happened: CHERRY_PICK_HEAD is deleted
> by git and no errors are output. But in fact .git/sequencer still exists,
> Wait until he uses the cherry-pick command next time, the error appears:
> 
> error: cherry-pick is already in progress
> hint: try "git cherry-pick (--continue | --abort | --quit)"
> fatal: cherry-pick failed
> 
> So we should not encourage users to use git commit when git cherry-pick.
> It would be great if it could provide advice similar to rebase, like this:
> 
> Once you are satisfied with your changes, run
> 
>   git cherry-pick --continue

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

> Phillip Wood <phillip.wood123@gmail.com> writes:
>
>> 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.
>
> Ahh, I didn't realize we still had scripted rebase backends that
> called cherry-pick as an executable.  I was hoping that all rebase
> backends by now would be calling into the cherry-pick machinery
> directly, bypassing cmd_cherry_pick(), and that was why I suggested
> to catch stray one the end-users set manually in the environment
> and clear it there.
>
>> 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)?
>
> I didn't press for the information too hard, but I guessed that it
> was perhaps because somebody like stackoverflow suggested to set a
> message in their environment to get a "better message."

> Junio C Hamano <gitster@pobox.com> writes:
> 
>> Phillip Wood <phillip.wood123@gmail.com> writes:
>>
>>> 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.
>>
>> Ahh, I didn't realize we still had scripted rebase backends that
>> called cherry-pick as an executable.  I was hoping that all rebase
>> backends by now would be calling into the cherry-pick machinery
>> directly, bypassing cmd_cherry_pick(), and that was why I suggested
>> to catch stray one the end-users set manually in the environment
>> and clear it there.
>>
>>> 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)?
>>
>> I didn't press for the information too hard, but I guessed that it
>> was perhaps because somebody like stackoverflow suggested to set a
>> message in their environment to get a "better message."
> 
> A good way forward may be to relieve sequencer.c::print_advice() of
> the responsibility of optinally removing CHERRY_PICK_HEAD; make it a
> separate function that bases its decision on a more direct cue, not
> on the presense of a custom message in GIT_CHERRY_PICK_HELP, make
> do_pick_commit(), which is the sole caller of print_advice(), call
> it after calling print_advice().
> 
> I do not offhand know what that "direct cue" should be, but we may
> already have an appropriate field in the replay_opts structure;
> "replay.action is neither REVERT nor PICK" could be a good enough
> approximation, I dunno.
> 
> Otherwise we can allocate a new bit in the structure, have relevant
> callers set it, and teach cherry-pick an unadvertised command line
> option that sets the bit, and use that option only from
> git-rebase--preserve-merges when it makes a call to cherry-pick.
> When "rebase -p" is either retired or rewritten in C, we can retire
> the option from cherry-pick.
>
> Workable?

>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Phillip Wood <phillip.wood123@gmail.com> writes:
> >
> >> 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.
> >
> > Ahh, I didn't realize we still had scripted rebase backends that
> > called cherry-pick as an executable.  I was hoping that all rebase
> > backends by now would be calling into the cherry-pick machinery
> > directly, bypassing cmd_cherry_pick(), and that was why I suggested
> > to catch stray one the end-users set manually in the environment
> > and clear it there.
> >
> >> 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)?
> >
> > I didn't press for the information too hard, but I guessed that it
> > was perhaps because somebody like stackoverflow suggested to set a
> > message in their environment to get a "better message."
>
> A good way forward may be to relieve sequencer.c::print_advice() of
> the responsibility of optinally removing CHERRY_PICK_HEAD; make it a
> separate function that bases its decision on a more direct cue, not
> on the presense of a custom message in GIT_CHERRY_PICK_HELP, make
> do_pick_commit(), which is the sole caller of print_advice(), call
> it after calling print_advice().
>

> I do not offhand know what that "direct cue" should be, but we may
> already have an appropriate field in the replay_opts structure;
> "replay.action is neither REVERT nor PICK" could be a good enough
> approximation, I dunno.
>
> Otherwise we can allocate a new bit in the structure, have relevant
> callers set it, and teach cherry-pick an unadvertised command line
> option that sets the bit, and use that option only from
> git-rebase--preserve-merges when it makes a call to cherry-pick.
> When "rebase -p" is either retired or rewritten in C, we can retire
> the option from cherry-pick.
>

> I think this function "check_need_delete_cherry_pick_head()" should be before
> print_advice(), like this:
>
> +               const char *help_msgs = NULL;
> +
>                 error(command == TODO_REVERT
>                       ? _("could not revert %s... %s")
>                       : _("could not apply %s... %s"),
>                       short_commit_name(commit), msg.subject);
> -               print_advice(r, res == 1, opts);
> +               if (((opts->action == REPLAY_PICK &&
> +                     !opts->rebase_preserve_merges_mode) ||
> +                     (help_msgs = check_need_delete_cherry_pick_head(r))) &&
> +                     res == 1)
> +                       print_advice(opts, help_msgs);

> Otherwise we can allocate a new bit in the structure, have relevant
> callers set it, and teach cherry-pick an unadvertised command line
> option that sets the bit, and use that option only from
> git-rebase--preserve-merges when it makes a call to cherry-pick.
> When "rebase -p" is either retired or rewritten in C, we can retire
> the option from cherry-pick.
>

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

> Phillip Wood <phillip.wood123@gmail.com> 于2021年7月28日周三 上午3:43写道：
>>
>>> 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
> 
> Yeah, I thought it would call some rebase-related code before, I
> didn’t expect it to
> call cherry-pick. On the other hand, I passed all tests, so I ignore
> it, there should be
> a test for it.
> 
>> 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'
>>
> 
> Ah, because I want to find a way to suppress its advice messages about
> "git commit",
> and I don’t think anyone else is using this "feature".

>> Best Wishes
>>
>> Phillip
>>
>> [1]
>> https://lore.kernel.org/git/CAOLTT8Ty47fyY7T3d68CYPKh9k+HAHsnCLJ=F0KaLm+0gp3+EQ@mail.gmail.com/
>>
> 
> Thanks.
> --
> ZheNing Hu
> 

>
> Hi ZheNing
>
> > Ah, because I want to find a way to suppress its advice messages about
> > "git commit",
> > and I don’t think anyone else is using this "feature".
>
> I'd welcome a patch to improve the advice. I suspect the current advice
> predates the introduction of the '--continue' flag for cherry-pick. I
> think that would be a better route forward as it would benefit all
> users. Setting GIT_CHERRY_PICK_HELP is undocumented and has always
> removed CHERRY_PICK_HEAD since CHERRY_PICK_HEAD was introduced in commit
> 7e5c0cbf (Introduce CHERRY_PICK_HEAD, 2011-02-19).
>

> Phillip Wood <phillip.wood123@gmail.com> 于2021年7月28日周三 下午5:46写道：
>>
>> Hi ZheNing
>>
>>> Ah, because I want to find a way to suppress its advice messages about
>>> "git commit",
>>> and I don’t think anyone else is using this "feature".
>>
>> I'd welcome a patch to improve the advice. I suspect the current advice
>> predates the introduction of the '--continue' flag for cherry-pick. I
>> think that would be a better route forward as it would benefit all
>> users. Setting GIT_CHERRY_PICK_HELP is undocumented and has always
>> removed CHERRY_PICK_HEAD since CHERRY_PICK_HEAD was introduced in commit
>> 7e5c0cbf (Introduce CHERRY_PICK_HEAD, 2011-02-19).
>>
> 
> After I modify the interface of print_advice() in the way suggested by
> Junio, I can provide a
> help_msg parameter for print_advice(), and maybe we can use it to
> provide better advice later.

> Something like this:
> 
> +static void print_advice(struct replay_opts *opts, const char *help_msgs)
> +{
> +       if (help_msgs)
> +               fprintf(stderr, "%s\n", help_msgs);
> +       else if (opts->no_commit)
> +               advise(_("after resolving the conflicts, mark the
> corrected paths\n"
> +                        "with 'git add <paths>' or 'git rm <paths>'"));
> +       else
> +               advise(_("after resolving the conflicts, mark the
> corrected paths\n"
> +                        "with 'git add <paths>' or 'git rm <paths>'\n"
> +                        "and commit the result with 'git commit'"));
>   }
> 
>> Best Wishes
>>
>> Phillip
>>
> 
> Thanks.
> --
> ZheNing Hu
>