diff mbox series

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

Message ID pull.1001.v2.git.1627135281887.gitgitgadget@gmail.com (mailing list archive)
State New
Headers show
Series [v2,GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP | expand

Commit Message

ZheNing Hu July 24, 2021, 2:01 p.m. UTC
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(-)


base-commit: daab8a564f8bbac55f70f8bf86c070e001a9b006

Comments

Phillip Wood July 27, 2021, 7:43 p.m. UTC | #1
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
>
Junio C Hamano July 27, 2021, 8:44 p.m. UTC | #2
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 July 27, 2021, 9 p.m. UTC | #3
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?
ZheNing Hu July 28, 2021, 7:39 a.m. UTC | #4
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
Phillip Wood July 28, 2021, 9:46 a.m. UTC | #5
Hi ZheNing

On 28/07/2021 08:39, ZheNing Hu wrote:
> 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".

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

Best Wishes

Phillip


>> Best Wishes
>>
>> Phillip
>>
>> [1]
>> https://lore.kernel.org/git/CAOLTT8Ty47fyY7T3d68CYPKh9k+HAHsnCLJ=F0KaLm+0gp3+EQ@mail.gmail.com/
>>
> 
> Thanks.
> --
> ZheNing Hu
>
Phillip Wood July 28, 2021, 9:56 a.m. UTC | #6
On 27/07/2021 22:00, Junio C Hamano wrote:
> 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?

Most of the time the builtin rebase should not be writing 
CHERRY_PICK_HEAD in the first place (it needs it when a commit becomes 
empty but not otherwise). For 'rebase -p' adding a command line option 
to cherry-pick as you suggest is probably the cleanest solution - in the 
short term 'rebase -i' could set it until we refactor the code that 
creates CHERRY_PICK_HEAD. One thing to note is that I think 
GIT_CHERRY_PICK_HELP was introduced to allow assist scripted rebase like 
porcelains rather than as a way to allow users to customize the help and 
setting it has always removed CHERRY_PICK_HEAD. There are however no 
users of GIT_CHERRY_PICK_HELP on codesearch.debian.org

Best Wishes

Phillip
ZheNing Hu July 28, 2021, 10:56 a.m. UTC | #7
Junio C Hamano <gitster@pobox.com> 于2021年7月28日周三 上午5:00写道:
>
> 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 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);

> 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 one can be easily achieved.

> Workable?

Thanks.
--
ZheNing Hu
ZheNing Hu July 28, 2021, 11:01 a.m. UTC | #8
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
ZheNing Hu July 28, 2021, 11:34 a.m. UTC | #9
Junio C Hamano <gitster@pobox.com> 于2021年7月28日周三 上午5:00写道:

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

Just use a PARSE_OPT_HIDDEN cannot prevent the user from using
the option... Is there a better way?

--
ZheNing Hu
Phillip Wood July 28, 2021, 4:52 p.m. UTC | #10
On 28/07/2021 12:01, ZheNing Hu wrote:
> 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.

I think that we could change the advice now to suggest using 'git 
cherry-pick --continue' instead of running 'git commit'. I think that in 
the long term we want to move away from being able to customize the 
advice when 'git rebase -p' is retired.

Best Wishes

Phillip

> 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
>
Junio C Hamano July 28, 2021, 5:24 p.m. UTC | #11
ZheNing Hu <adlternative@gmail.com> writes:

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

Sorry, but I am not sure what problem this separation is trying to
solve.

The root cause of the issue we have, I think, is because the
decision to delete or keep the cherry-pick-head pseudoref is tied to
what message we give to users in the current code, and the suggested
split of concern is to limit print_advice() to only print the advice
message, and a new helper to decide and remove the pseudoref,
without relying on what is in the GIT_CHERRY_PICK_HELP environment.

It is unclear where you are making the decision to keep or remove
the pseudoref in the above arrangement that lets the new
check_need_delete_cherry_pick_head() decide if the advice is printed
or not.
Junio C Hamano July 28, 2021, 5:26 p.m. UTC | #12
ZheNing Hu <adlternative@gmail.com> writes:

> Just use a PARSE_OPT_HIDDEN cannot prevent the user from using
> the option... Is there a better way?

If a command can be invoked from a script, you can invoke it
interactively the same way.  Not advertising on short help, not
completing in the completion and documenting it for internal use is
the standard arrangement for such "implementation detail only"
options.
diff mbox series

Patch

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