cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP

From: ZheNing Hu <[email protected]>

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.

Quoted 2 lines · ZheNing Hu via GitGitGadget

From: ZheNing Hu <[email protected]>

Quoted 4 lines

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.

So, I am not enthused to see this change.

Quoted 10 lines · Junio C Hamano <[email protected]> 于2021年7月23日周五 上午5:25写道：

Quoted 6 lines · ZheNing Hu via GitGitGadget

From: ZheNing Hu <[email protected]>

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.

Quoted 4 lines

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 <[email protected]>

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 <[email protected]>

Mentored-by Christian Couder <[email protected]>

Mentored-by Hariom Verma <[email protected]>:
---
[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 <[email protected]>
      +    Mentored-by: Christian Couder <[email protected]>
      +    Mentored-by Hariom Verma <[email protected]>:
       
      - ## 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(-)

builtin/revert.c +0 -0

index 237f2f18d4c..ec0abe7db73 100644

builtin/revert.c +0 -0

  	res = run_sequencer(argc, argv, &opts);
  	if (res < 0)
  		die(_("cherry-pick failed"));

t/t3507-cherry-pick-conflict.sh +17 -10

index 014001b8f32..6f8035399d9 100755
@@ -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

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

Quoted 3 lines · Phillip Wood

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.

Quoted 4 lines

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

Quoted 13 lines · Junio C Hamano

Quoted 3 lines · Phillip Wood

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.

Quoted 8 lines

Quoted 4 lines

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?

Quoted 13 lines · Junio C Hamano

Quoted 3 lines · Phillip Wood

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.

Quoted 8 lines

Quoted 4 lines

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?

I think this function "check_need_delete_cherry_pick_head()" should be before print_advice(), like this:

(quoted diff) +2 -0

+               const char *help_msgs = NULL;
+

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?

Quoted 7 lines

builtin/revert.c +0 -0

index 237f2f18d4c..ec0abe7db73 100644
@@ -245,6 +245,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 

builtin/revert.c int cmd_cherry_pick(int argc, const char **argv, const char *prefix) · line 248

Quoted 3 lines

builtin/revert.c +1 -0

      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/

Quoted 7 lines · Phillip Wood <[email protected]> 于2021年7月28日周三 上午3:43写道：

Quoted 7 lines

builtin/revert.c +0 -0

index 237f2f18d4c..ec0abe7db73 100644
@@ -245,6 +245,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 

Quoted 7 lines

builtin/revert.c int cmd_cherry_pick(int argc, const char **argv, const char *prefix) · line 248

Quoted 3 lines

builtin/revert.c +1 -0

       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.

Quoted 6 lines

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

Quoted 7 lines

Best Wishes

Phillip

[1]
https://lore.kernel.org/git/CAOLTT8Ty47fyY7T3d68CYPKh9k+HAHsnCLJ=F0KaLm+0gp3+EQ@mail.gmail.com/

Thanks.
--
ZheNing Hu

Hi ZheNing

Quoted 3 lines

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

Quoted 7 lines · Phillip Wood <[email protected]> 于2021年7月28日周三 下午5:46写道：

Hi ZheNing

Quoted 3 lines

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

Quoted 7 lines

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:

(quoted diff) +6 -0

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

(quoted diff) +3 -0

+                        "with 'git add <paths>' or 'git rm <paths>'"));
+       else
+               advise(_("after resolving the conflicts, mark the

corrected paths\n"

(quoted diff) +2 -0

+                        "with 'git add <paths>' or 'git rm <paths>'\n"
+                        "and commit the result with 'git commit'"));
  }

Quoted 5 lines

Best Wishes

Phillip

Thanks.
--
ZheNing Hu