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, and makes git chery-pick advice message better.

v3:
https://lore.kernel.org/git/[email protected]/

v3-->v4:

1. Add hidden option --delete-cherry-pick-head to git cherry-pick wihch used to delete CHERRY_PICK_HEAD when conflict occurs.
2. add delete_cherry_pick_head flag to struct replay_opts and struct rebase_options.
3. Split print_advice() into print advice and delete CHERRY_PICK_HEAD two part.
4. Use better git cherry-pick advice message.

ZheNing Hu (2):
[GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP [GSOC] cherry-pick: use better advice message

 builtin/rebase.c                |  3 +++
 builtin/revert.c                |  2 ++
 git-rebase--preserve-merges.sh  |  2 +-
 sequencer.c                     | 37 +++++++++++++++++-------------
 sequencer.h                     |  1 +
 t/t3507-cherry-pick-conflict.sh | 40 ++++++++++++++++++++-------------
 6 files changed, 53 insertions(+), 32 deletions(-)

base-commit: daab8a564f8bbac55f70f8bf86c070e001a9b006
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1010%2Fadlternative%2Fcherry-pick-help-fix-3-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1010/adlternative/cherry-pick-help-fix-3-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1010

gitgitgadget

From: ZheNing Hu <[email protected]>

GIT_CHERRY_PICK_HELP is an environment variable, as the implementation detail of some porcelain in git to help realize the rebasing steps. E.g. git rebase -p set GIT_CHERRY_PICK_HELP value in git-rebase--preserve-merges.sh, git rebase --merge set GIT_CHERRY_PICK_HELP value in run_specific_rebase(). But If we set the value of 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.

Introduce new "hidden" option --delete-cherry-pick-head for git cherry-pick which indicates that CHERRY_PICK_HEAD will be deleted when conflict occurs, which provided for some porcelain commands of git like git-rebase--preserve-merges.sh. After git rebase -p completely abolished, this option should be removed. At the same time, add the flag delete_cherry_pick_head to struct rebase_options and struct replay_opts, We can decide whether to delete CHERRY_PICK_HEAD by setting, passing, and checking this flag bit.

Then we split print_advice() into two part: Firstly, print_advice() will only be responsible for outputting content; Secondly, check if we set the delete_cherry_pick_head flag; if set, delete CHERRY_PICK_HEAD. In this way, the steps of printing advice and deleting CHERRY_PICK_HEAD are decoupled. Finally, let git-rebase--preserve-merges.sh use the --delete-cherry-pick-head option when it executes git cherry-pick, and set the delete_cherry_pick_head flag in run_specific_rebase() when we are using git rebase --merge, which can fix this breakage.

Mentored-by Hariom Verma <[email protected]>:

---
 builtin/rebase.c                |  3 +++
 builtin/revert.c                |  2 ++
 git-rebase--preserve-merges.sh  |  2 +-
 sequencer.c                     | 28 +++++++++++++---------------
 sequencer.h                     |  1 +
 t/t3507-cherry-pick-conflict.sh | 25 +++++++++++++++----------
 6 files changed, 35 insertions(+), 26 deletions(-)

index 12f093121d9..5983f37d531 100644
@@ -84,6 +84,7 @@ struct rebase_options {
 		REBASE_FORCE = 1<<3,
 		REBASE_INTERACTIVE_EXPLICIT = 1<<4,
 	} flags;
+	int delete_cherry_pick_head;
 	struct strvec git_am_opts;
 	const char *action;
 	int signoff;

@@ -152,6 +153,7 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
 		oidcpy(&replay.squash_onto, opts->squash_onto);
 		replay.have_squash_onto = 1;
 	}
+	replay.delete_cherry_pick_head = opts->delete_cherry_pick_head;
 
 	return replay;
 }

@@ -948,6 +950,7 @@ static int run_specific_rebase(struct rebase_options *opts, enum action action)
 	if (opts->type == REBASE_MERGE) {
 		/* Run sequencer-based rebase */
 		setenv("GIT_CHERRY_PICK_HELP", resolvemsg, 1);
+		opts->delete_cherry_pick_head = 1;
 		if (!(opts->flags & REBASE_INTERACTIVE_EXPLICIT)) {
 			setenv("GIT_SEQUENCE_EDITOR", ":", 1);
 			opts->autosquash = 0;

index 237f2f18d4c..15a4b6fe4ee 100644
@@ -127,6 +127,8 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
 			OPT_BOOL(0, "allow-empty", &opts->allow_empty, N_("preserve initially empty commits")),
 			OPT_BOOL(0, "allow-empty-message", &opts->allow_empty_message, N_("allow commits with empty messages")),
 			OPT_BOOL(0, "keep-redundant-commits", &opts->keep_redundant_commits, N_("keep redundant, empty commits")),
+			OPT_BOOL_F(0, "delete-cherry-pick-head", &opts->delete_cherry_pick_head,
+				   N_("delete CHERRY_PICK_HEAD when conflict occurs"), PARSE_OPT_HIDDEN),
 			OPT_END(),
 		};
 		options = parse_options_concat(options, cp_extra);

index b9c71d2a71b..eaa8f9de2c5 100644
@@ -444,7 +444,7 @@ pick_one_preserving_merges () {
 			output eval git cherry-pick $allow_rerere_autoupdate \
 				$allow_empty_message \
 				${gpg_sign_opt:+$(git rev-parse --sq-quote "$gpg_sign_opt")} \
-				"$strategy_args" "$@" ||
+				"$strategy_args" --delete-cherry-pick-head "$@" ||
 				die_with_patch $sha1 "$(eval_gettext "Could not pick \$sha1")"
 			;;
 		esac

index 0bec01cf38e..83cf6a5da3c 100644
@@ -397,24 +397,13 @@ static void free_message(struct commit *commit, struct commit_message *msg)
 	unuse_commit_buffer(commit, msg->message);
 }
 
-static void print_advice(struct repository *r, int show_hint,
-			 struct replay_opts *opts)
+static void print_advice(struct replay_opts *opts, int show_hint)
 {
 	char *msg = getenv("GIT_CHERRY_PICK_HELP");
 
 	if (msg) {
-		fprintf(stderr, "%s\n", msg);
-		/*
-		 * A conflict has occurred but the porcelain
-		 * (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);
-		return;
-	}
-
-	if (show_hint) {
+		advise("%s\n", msg);
+	} else if (show_hint) {
 		if (opts->no_commit)
 			advise(_("after resolving the conflicts, mark the corrected paths\n"
 				 "with 'git add <paths>' or 'git rm <paths>'"));

@@ -2265,7 +2254,16 @@ static int do_pick_commit(struct repository *r,
 		      ? _("could not revert %s... %s")
 		      : _("could not apply %s... %s"),
 		      short_commit_name(commit), msg.subject);
-		print_advice(r, res == 1, opts);
+		print_advice(opts, res == 1);
+		if (opts->delete_cherry_pick_head) {
+			/*
+			 * A conflict has occurred but the porcelain
+			 * (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);
+		}
 		repo_rerere(r, opts->allow_rerere_auto);
 		goto leave;
 	}

index d57d8ea23d7..76fb4af56fd 100644
@@ -49,6 +49,7 @@ struct replay_opts {
 	int reschedule_failed_exec;
 	int committer_date_is_author_date;
 	int ignore_date;
+	int delete_cherry_pick_head;
 
 	int mainline;
 

index 014001b8f32..f17621d1915 100755
@@ -76,6 +76,21 @@ 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: and then do something else
+		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 +124,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 &&
 

gitgitgadget

Maybe you are right.

Yeah, maybe I should mention this in commit messages.

Make sence.

Thanks.
--
ZheNing Hu

From: ZheNing Hu <[email protected]>

In the past, git cherry-pick would print such advice when there was a conflict:

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'

But in fact, when we want to cherry-pick multiple commits, we should not use "git commit" after resolving conflicts, which will make Git generate some errors. We should recommend users to use git cherry-pick --continue, git cherry-pick --abort, just like git rebase does.

This is the improved advice:

hint: Resolve all conflicts manually, mark them as resolved with
hint: "git add/rm <conflicted_files>", then run "git cherry-pick \
--continue".
hint: You can instead skip this commit: run "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".

Mentored-by Hariom Verma <[email protected]>:

---
 sequencer.c                     |  9 ++++++++-
 t/t3507-cherry-pick-conflict.sh | 15 ++++++++++-----
 2 files changed, 18 insertions(+), 6 deletions(-)

index 83cf6a5da3c..f6e9d1fddd8 100644
@@ -404,7 +404,14 @@ static void print_advice(struct replay_opts *opts, int show_hint)
 	if (msg) {
 		advise("%s\n", msg);
 	} else if (show_hint) {
-		if (opts->no_commit)
+		if (opts->action == REPLAY_PICK) {
+			advise(_("Resolve all conflicts manually, mark them as resolved with\n"
+				 "\"git add/rm <conflicted_files>\", then run \"git cherry-pick --continue\".\n"
+				 "You can instead skip this commit: run \"git cherry-pick --skip\".\n"
+				 "To abort and get back to the state before \"git cherry-pick\",\n"
+				 "run \"git cherry-pick --abort\"."));
+
+		} else if (opts->no_commit)
 			advise(_("after resolving the conflicts, mark the corrected paths\n"
 				 "with 'git add <paths>' or 'git rm <paths>'"));
 		else

index f17621d1915..2cc3977f5a6 100755
@@ -53,9 +53,11 @@ test_expect_success 'advice from failed cherry-pick' "
 	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'
+	hint: Resolve all conflicts manually, mark them as resolved with
+	hint: \"git add/rm <conflicted_files>\", then run \"git cherry-pick --continue\".
+	hint: You can instead skip this commit: run \"git cherry-pick --skip\".
+	hint: To abort and get back to the state before \"git cherry-pick\",
+	hint: run \"git cherry-pick --abort\".
 	EOF
 	test_must_fail git cherry-pick picked 2>actual &&
 

@@ -68,8 +70,11 @@ test_expect_success 'advice from failed cherry-pick --no-commit' "
 	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: Resolve all conflicts manually, mark them as resolved with
+	hint: \"git add/rm <conflicted_files>\", then run \"git cherry-pick --continue\".
+	hint: You can instead skip this commit: run \"git cherry-pick --skip\".
+	hint: To abort and get back to the state before \"git cherry-pick\",
+	hint: run \"git cherry-pick --abort\".
 	EOF
 	test_must_fail git cherry-pick --no-commit picked 2>actual &&
 

gitgitgadget

This new wording matches what we have for rebase which is good, I am slightly worried that the lines end up being quite long though they are just under 80 characters. It might be worth splitting the line that mentions running "git cherry-pick --continue" so it is a bit shorter.

Best Wishes

Phillip

Agree.

Thanks.
--
ZheNing Hu

This patch fixes the bug when git cherry-pick is used with environment variable GIT_CHERRY_PICK_HELP, and makes git chery-pick advice message better.

v4:
https://lore.kernel.org/git/[email protected]/

v4-->v5:

1. Delete struct rebase_options member delete_cherry_pick_head, and set the delete_cherry_pick_head in struct replay_opts replay to 1 in get_replay_opts().
2. Avoid too long line length of advice of cherry-pick, split "git cherry-pick --continue" in advice to new line.

ZheNing Hu (2):
[GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP [GSOC] cherry-pick: use better advice message

 builtin/rebase.c                |  1 +
 builtin/revert.c                |  2 ++
 git-rebase--preserve-merges.sh  |  2 +-
 sequencer.c                     | 38 +++++++++++++++-----------
 sequencer.h                     |  1 +
 t/t3507-cherry-pick-conflict.sh | 48 ++++++++++++++++++++++-----------
 6 files changed, 60 insertions(+), 32 deletions(-)

base-commit: daab8a564f8bbac55f70f8bf86c070e001a9b006
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1010%2Fadlternative%2Fcherry-pick-help-fix-3-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1010/adlternative/cherry-pick-help-fix-3-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1010

Range-diff vs v1:

1: 0d0a55bd9c4 ! 1: 5d2fd55c580 [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP
@@ Commit message
conflict occurs, which provided for some porcelain commands of git like git-rebase--preserve-merges.sh. After git rebase -p completely abolished, this option should be removed. At the same time, add the flag
- delete_cherry_pick_head to struct rebase_options and
- struct replay_opts, We can decide whether to delete CHERRY_PICK_HEAD
- by setting, passing, and checking this flag bit.
+ delete_cherry_pick_head to struct replay_opts, We can decide whether
+ to delete CHERRY_PICK_HEAD by setting and checking this flag bit.

Then we split print_advice() into two part: Firstly, print_advice() will only be responsible for outputting content; Secondly, check if
@@ Commit message
In this way, the steps of printing advice and deleting CHERRY_PICK_HEAD are decoupled. Finally, let git-rebase--preserve-merges.sh use the --delete-cherry-pick-head option when it executes git cherry-pick, and
- set the delete_cherry_pick_head flag in run_specific_rebase() when we
+ set the delete_cherry_pick_head flag in get_replay_opts() when we
are using git rebase --merge, which can fix this breakage.

+ It is worth mentioning that now we use advice() to print the content
+ of GIT_CHERRY_PICK_HELP in print_advice(), each line of output will
+ start with "hint: ".
+

Mentored-by Hariom Verma <[email protected]>:

@@ Commit message

## builtin/rebase.c ##
-@@ builtin/rebase.c: struct rebase_options {
- REBASE_FORCE = 1<<3,
- REBASE_INTERACTIVE_EXPLICIT = 1<<4,
- } flags;
-+ int delete_cherry_pick_head;
- struct strvec git_am_opts;
- const char *action;
- int signoff;
@@ builtin/rebase.c: static struct replay_opts get_replay_opts(const struct rebase_options *opts)
oidcpy(&replay.squash_onto, opts->squash_onto); replay.have_squash_onto = 1;
}
-+ replay.delete_cherry_pick_head = opts->delete_cherry_pick_head;
++ replay.delete_cherry_pick_head = 1;

return replay;
}
-@@ builtin/rebase.c: static int run_specific_rebase(struct rebase_options *opts, enum action action)
- if (opts->type == REBASE_MERGE) {
- /* Run sequencer-based rebase */
- setenv("GIT_CHERRY_PICK_HELP", resolvemsg, 1);
-+ opts->delete_cherry_pick_head = 1;
- if (!(opts->flags & REBASE_INTERACTIVE_EXPLICIT)) {
- setenv("GIT_SEQUENCE_EDITOR", ":", 1);
- opts->autosquash = 0;

## builtin/revert.c ##
@@ builtin/revert.c: static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
@@ t/t3507-cherry-pick-conflict.sh: test_expect_success 'advice from failed cherry-
test_expect_success 'failed cherry-pick sets CHERRY_PICK_HEAD' '
pristine_detach initial &&
test_must_fail git cherry-pick picked &&
+ test_cmp_rev picked CHERRY_PICK_HEAD
+ '
+
++test_expect_success 'failed cherry-pick with --delete-cherry-pick-head does not set CHERRY_PICK_HEAD' '
++ pristine_detach initial &&
++ test_must_fail git cherry-pick --delete-cherry-pick-head picked &&
++ test_must_fail git rev-parse --verify CHERRY_PICK_HEAD
++'
++
+ test_expect_success 'successful cherry-pick does not set CHERRY_PICK_HEAD' '
+ pristine_detach initial &&
+ git cherry-pick base &&
@@ t/t3507-cherry-pick-conflict.sh: test_expect_success \
test_must_fail git rev-parse --verify CHERRY_PICK_HEAD
'
2: 7e1ed49728d ! 2: 5279bca7a79 [GSOC] cherry-pick: use better advice message
@@ Commit message
This is the improved advice:

hint: Resolve all conflicts manually, mark them as resolved with
- hint: "git add/rm <conflicted_files>", then run "git cherry-pick \
- --continue".
+ hint: "git add/rm <conflicted_files>", then run
+ hint: "git cherry-pick --continue".
hint: You can instead skip this commit: run "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".
@@ sequencer.c: static void print_advice(struct replay_opts *opts, int show_hint)
- if (opts->no_commit)
+ if (opts->action == REPLAY_PICK) {
+ advise(_("Resolve all conflicts manually, mark them as resolved with\n"
-+ "\"git add/rm <conflicted_files>\", then run \"git cherry-pick --continue\".\n"
++ "\"git add/rm <conflicted_files>\", then run\n"
++ "\"git cherry-pick --continue\".\n"
+ "You can instead skip this commit: run \"git cherry-pick --skip\".\n"
+ "To abort and get back to the state before \"git cherry-pick\",\n"
+ "run \"git cherry-pick --abort\"."));
@@ t/t3507-cherry-pick-conflict.sh: test_expect_success 'advice from failed cherry-
- hint: with 'git add <paths>' or 'git rm <paths>'
- hint: and commit the result with 'git commit'
+ hint: Resolve all conflicts manually, mark them as resolved with
-+ hint: \"git add/rm <conflicted_files>\", then run \"git cherry-pick --continue\".
++ hint: \"git add/rm <conflicted_files>\", then run
++ hint: \"git cherry-pick --continue\".
+ hint: You can instead skip this commit: run \"git cherry-pick --skip\".
+ hint: To abort and get back to the state before \"git cherry-pick\",
+ hint: run \"git cherry-pick --abort\".
@@ t/t3507-cherry-pick-conflict.sh: test_expect_success 'advice from failed cherry-
- hint: after resolving the conflicts, mark the corrected paths
- hint: with 'git add <paths>' or 'git rm <paths>'
+ hint: Resolve all conflicts manually, mark them as resolved with
-+ hint: \"git add/rm <conflicted_files>\", then run \"git cherry-pick --continue\".
++ hint: \"git add/rm <conflicted_files>\", then run
++ hint: \"git cherry-pick --continue\".
+ hint: You can instead skip this commit: run \"git cherry-pick --skip\".
+ hint: To abort and get back to the state before \"git cherry-pick\",
+ hint: run \"git cherry-pick --abort\".

gitgitgadget

From: ZheNing Hu <[email protected]>

GIT_CHERRY_PICK_HELP is an environment variable, as the implementation detail of some porcelain in git to help realize the rebasing steps. E.g. git rebase -p set GIT_CHERRY_PICK_HELP value in git-rebase--preserve-merges.sh, git rebase --merge set GIT_CHERRY_PICK_HELP value in run_specific_rebase(). But If we set the value of 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.

Introduce new "hidden" option --delete-cherry-pick-head for git cherry-pick which indicates that CHERRY_PICK_HEAD will be deleted when conflict occurs, which provided for some porcelain commands of git like git-rebase--preserve-merges.sh. After git rebase -p completely abolished, this option should be removed. At the same time, add the flag delete_cherry_pick_head to struct replay_opts, We can decide whether to delete CHERRY_PICK_HEAD by setting and checking this flag bit.

Then we split print_advice() into two part: Firstly, print_advice() will only be responsible for outputting content; Secondly, check if we set the delete_cherry_pick_head flag; if set, delete CHERRY_PICK_HEAD. In this way, the steps of printing advice and deleting CHERRY_PICK_HEAD are decoupled. Finally, let git-rebase--preserve-merges.sh use the --delete-cherry-pick-head option when it executes git cherry-pick, and set the delete_cherry_pick_head flag in get_replay_opts() when we are using git rebase --merge, which can fix this breakage.

It is worth mentioning that now we use advice() to print the content of GIT_CHERRY_PICK_HELP in print_advice(), each line of output will start with "hint: ".

Mentored-by Hariom Verma <[email protected]>:

---
 builtin/rebase.c                |  1 +
 builtin/revert.c                |  2 ++
 git-rebase--preserve-merges.sh  |  2 +-
 sequencer.c                     | 28 +++++++++++++---------------
 sequencer.h                     |  1 +
 t/t3507-cherry-pick-conflict.sh | 31 +++++++++++++++++++++----------
 6 files changed, 39 insertions(+), 26 deletions(-)

index 12f093121d9..08ba437c6a0 100644
@@ -152,6 +152,7 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
 		oidcpy(&replay.squash_onto, opts->squash_onto);
 		replay.have_squash_onto = 1;
 	}
+	replay.delete_cherry_pick_head = 1;
 
 	return replay;
 }

index 237f2f18d4c..15a4b6fe4ee 100644
@@ -127,6 +127,8 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
 			OPT_BOOL(0, "allow-empty", &opts->allow_empty, N_("preserve initially empty commits")),
 			OPT_BOOL(0, "allow-empty-message", &opts->allow_empty_message, N_("allow commits with empty messages")),
 			OPT_BOOL(0, "keep-redundant-commits", &opts->keep_redundant_commits, N_("keep redundant, empty commits")),
+			OPT_BOOL_F(0, "delete-cherry-pick-head", &opts->delete_cherry_pick_head,
+				   N_("delete CHERRY_PICK_HEAD when conflict occurs"), PARSE_OPT_HIDDEN),
 			OPT_END(),
 		};
 		options = parse_options_concat(options, cp_extra);

index b9c71d2a71b..eaa8f9de2c5 100644
@@ -444,7 +444,7 @@ pick_one_preserving_merges () {
 			output eval git cherry-pick $allow_rerere_autoupdate \
 				$allow_empty_message \
 				${gpg_sign_opt:+$(git rev-parse --sq-quote "$gpg_sign_opt")} \
-				"$strategy_args" "$@" ||
+				"$strategy_args" --delete-cherry-pick-head "$@" ||
 				die_with_patch $sha1 "$(eval_gettext "Could not pick \$sha1")"
 			;;
 		esac

index 0bec01cf38e..83cf6a5da3c 100644
@@ -397,24 +397,13 @@ static void free_message(struct commit *commit, struct commit_message *msg)
 	unuse_commit_buffer(commit, msg->message);
 }
 
-static void print_advice(struct repository *r, int show_hint,
-			 struct replay_opts *opts)
+static void print_advice(struct replay_opts *opts, int show_hint)
 {
 	char *msg = getenv("GIT_CHERRY_PICK_HELP");
 
 	if (msg) {
-		fprintf(stderr, "%s\n", msg);
-		/*
-		 * A conflict has occurred but the porcelain
-		 * (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);
-		return;
-	}
-
-	if (show_hint) {
+		advise("%s\n", msg);
+	} else if (show_hint) {
 		if (opts->no_commit)
 			advise(_("after resolving the conflicts, mark the corrected paths\n"
 				 "with 'git add <paths>' or 'git rm <paths>'"));

@@ -2265,7 +2254,16 @@ static int do_pick_commit(struct repository *r,
 		      ? _("could not revert %s... %s")
 		      : _("could not apply %s... %s"),
 		      short_commit_name(commit), msg.subject);
-		print_advice(r, res == 1, opts);
+		print_advice(opts, res == 1);
+		if (opts->delete_cherry_pick_head) {
+			/*
+			 * A conflict has occurred but the porcelain
+			 * (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);
+		}
 		repo_rerere(r, opts->allow_rerere_auto);
 		goto leave;
 	}

index d57d8ea23d7..76fb4af56fd 100644
@@ -49,6 +49,7 @@ struct replay_opts {
 	int reschedule_failed_exec;
 	int committer_date_is_author_date;
 	int ignore_date;
+	int delete_cherry_pick_head;
 
 	int mainline;
 

index 014001b8f32..af5678d981a 100755
@@ -76,12 +76,33 @@ 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: and then do something else
+		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 &&
 	test_cmp_rev picked CHERRY_PICK_HEAD
 '
 
+test_expect_success 'failed cherry-pick with --delete-cherry-pick-head does not set CHERRY_PICK_HEAD' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick --delete-cherry-pick-head picked &&
+	test_must_fail git rev-parse --verify CHERRY_PICK_HEAD
+'
+
 test_expect_success 'successful cherry-pick does not set CHERRY_PICK_HEAD' '
 	pristine_detach initial &&
 	git cherry-pick base &&

@@ -109,16 +130,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 &&
 

gitgitgadget

set -> sets

set -> sets

"help realize the rebasing steps" did not tell us much on "how" the environment variable helps or what it is used for. A sentence at this point, e.g.

The variable carries a custom help message to be shown when one step of replaying an existing commit fails in conflict.

may help. And there is one leap in the logic flow here.

However, the code also removes CHERRY_PICK_HEAD pseudoref when this environment variable exists, assuming that the presence of it means the sequencer machinery and not end-user is doing the cherry-picking.

And then we can drop "But" before "If" here.

indicates that CHERRY_PICK_HEAD will be ... ->

tells Git remove CHERRY_PICK_HEAD to separate the decision from message customization to clean up this mess.

Heple?

Thanks, such a supplement is very good.

You mean that cherry_pick with GIT_CHERRY_PICK_HELP suppresses CHERRY_PICK_HEAD is not even a bug?

It is reasonable for git rebase -p and git rebase -m to delete CHERRY_PICK_HEAD when a conflict occurs, but it is not necessarily for git cherry-pick to delete it too. IOW, I suspect that instead of letting users
not touch the trap here, it is better to remove the trap completely.

Thanks.
--
ZheNing Hu

Looking at the history I think it is fair to conclude that GIT_CHERRY_PICK_HELP was introduced as a way to help people writing scripts built on top of 'git cherry-pick' like 'git rebase' that want to have a custom message and do not want to leave CHERRY_PICK_HEAD around if there are conflicts. I don't think it was intended as a way for users to change the help when cherry-picking and has never been documented as such. I think we'd be better to focus on improving the default help that cherry-pick prints as the second patch in this series does.

Best Wishes

Phillip

I think that it is what the existing test is telling us. Of course, with a good reason, an earlier design can be updated as long as we make sure we won't hurt existing users who may rely on the current design, but ...

... I think that is a reasonable stance to take [*1*]. If the default help message can be improved, that is a good thing to do regardless.

Thanks.

[Footnote]

*1* Tying the "here is a custom HELP text" environment variable to "having a customization means whoever is driving the cherry-pick machinery is ALSO responsible for sequencing and we will remove CHERRY_PICK_HEAD" is a rather unfortunate design, but as long as that is documented, it is a workable design.

Well, this is indeed a bit strange, but maybe your and Phillip's intuitions are right,
then I will delete the content of the first patch and keep the second.

Thanks.
--
ZheNing Hu

From: ZheNing Hu <[email protected]>

In the past, git cherry-pick would print such advice when there was a conflict:

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'

But in fact, when we want to cherry-pick multiple commits, we should not use "git commit" after resolving conflicts, which will make Git generate some errors. We should recommend users to use git cherry-pick --continue, git cherry-pick --abort, just like git rebase does.

This is the improved advice:

hint: Resolve all conflicts manually, mark them as resolved with
hint: "git add/rm <conflicted_files>", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit: run "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".

Mentored-by Hariom Verma <[email protected]>:

---
 sequencer.c                     | 10 +++++++++-
 t/t3507-cherry-pick-conflict.sh | 17 ++++++++++++-----
 2 files changed, 21 insertions(+), 6 deletions(-)

index 83cf6a5da3c..bf7dbea81dd 100644
@@ -404,7 +404,15 @@ static void print_advice(struct replay_opts *opts, int show_hint)
 	if (msg) {
 		advise("%s\n", msg);
 	} else if (show_hint) {
-		if (opts->no_commit)
+		if (opts->action == REPLAY_PICK) {
+			advise(_("Resolve all conflicts manually, mark them as resolved with\n"
+				 "\"git add/rm <conflicted_files>\", then run\n"
+				 "\"git cherry-pick --continue\".\n"
+				 "You can instead skip this commit: run \"git cherry-pick --skip\".\n"
+				 "To abort and get back to the state before \"git cherry-pick\",\n"
+				 "run \"git cherry-pick --abort\"."));
+
+		} else if (opts->no_commit)
 			advise(_("after resolving the conflicts, mark the corrected paths\n"
 				 "with 'git add <paths>' or 'git rm <paths>'"));
 		else

index af5678d981a..e953b54e54d 100755
@@ -53,9 +53,12 @@ test_expect_success 'advice from failed cherry-pick' "
 	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'
+	hint: Resolve all conflicts manually, mark them as resolved with
+	hint: \"git add/rm <conflicted_files>\", then run
+	hint: \"git cherry-pick --continue\".
+	hint: You can instead skip this commit: run \"git cherry-pick --skip\".
+	hint: To abort and get back to the state before \"git cherry-pick\",
+	hint: run \"git cherry-pick --abort\".
 	EOF
 	test_must_fail git cherry-pick picked 2>actual &&
 

@@ -68,8 +71,12 @@ test_expect_success 'advice from failed cherry-pick --no-commit' "
 	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: Resolve all conflicts manually, mark them as resolved with
+	hint: \"git add/rm <conflicted_files>\", then run
+	hint: \"git cherry-pick --continue\".
+	hint: You can instead skip this commit: run \"git cherry-pick --skip\".
+	hint: To abort and get back to the state before \"git cherry-pick\",
+	hint: run \"git cherry-pick --abort\".
 	EOF
 	test_must_fail git cherry-pick --no-commit picked 2>actual &&
 

gitgitgadget

From: ZheNing Hu <[email protected]>

In the past, git cherry-pick would print such advice when there was a conflict:

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'

But in fact, when we want to cherry-pick multiple commits, we should not use "git commit" after resolving conflicts, which will make Git generate some errors. We should recommend users to use git cherry-pick --continue, git cherry-pick --abort, just like git rebase does.

This is the improved advice:

hint: Resolve all conflicts manually, mark them as resolved with
hint: "git add/rm <conflicted_files>", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit: run "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".

It is worth mentioning that now we use advice() to print the content of GIT_CHERRY_PICK_HELP in print_advice(), each line of output will start with "hint: ".

Mentored-by Hariom Verma <[email protected]>:

---
[GSOC] cherry-pick: use better advice message

Because git cherry-pick's past advice message is not good enough, it often misleads new users of Git, so this patch makes git chery-pick advice message better.

v5:
https://lore.kernel.org/git/[email protected]/

v5-->v6:

1. Deleted the first patch, which could have made git cherry-pick not delete CHERRY_PICK_HEAD when using GIT_CHERRY_PICK_HELP, but both Junio and Phillip believe that this design does not need to be changed.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1010%2Fadlternative%2Fcherry-pick-help-fix-3-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1010/adlternative/cherry-pick-help-fix-3-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1010

Range-diff vs v2:

 1:  5d2fd55c580 < -:  ----------- [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP
 2:  5279bca7a79 ! 1:  701645dde17 [GSOC] cherry-pick: use better advice message

@@ Commit message
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".

+ It is worth mentioning that now we use advice() to print the content
+ of GIT_CHERRY_PICK_HELP in print_advice(), each line of output will
+ start with "hint: ".
+

Mentored-by Hariom Verma <[email protected]>:

@@ Commit message

## sequencer.c ##
-@@ sequencer.c: static void print_advice(struct replay_opts *opts, int show_hint) +@@ sequencer.c: static void print_advice(struct repository *r, int show_hint,
+ char *msg = getenv("GIT_CHERRY_PICK_HELP");
+
if (msg) {
- advise("%s\n", msg);
- } else if (show_hint) {
+- fprintf(stderr, "%s\n", msg);
++ advise("%s\n", msg);
+ /*
+ * A conflict has occurred but the porcelain
+ * (typically rebase --interactive) wants to take care
+@@ sequencer.c: static void print_advice(struct repository *r, int show_hint,
+ }
+
+ if (show_hint) {
- if (opts->no_commit)
+ if (opts->action == REPLAY_PICK) {
+ advise(_("Resolve all conflicts manually, mark them as resolved with\n"
@@ sequencer.c: static void print_advice(struct replay_opts *opts, int show_hint)
+ "You can instead skip this commit: run \"git cherry-pick --skip\".\n"
+ "To abort and get back to the state before \"git cherry-pick\",\n"
+ "run \"git cherry-pick --abort\"."));
-+
+ } else if (opts->no_commit)
advise(_("after resolving the conflicts, mark the corrected paths\n"
"with 'git add <paths>' or 'git rm <paths>'"));

 sequencer.c                     | 11 +++++++++--
 t/t3507-cherry-pick-conflict.sh | 17 ++++++++++++-----
 2 files changed, 21 insertions(+), 7 deletions(-)

index 0bec01cf38e..7fa91b99870 100644
@@ -403,7 +403,7 @@ static void print_advice(struct repository *r, int show_hint,
 	char *msg = getenv("GIT_CHERRY_PICK_HELP");
 
 	if (msg) {
-		fprintf(stderr, "%s\n", msg);
+		advise("%s\n", msg);
 		/*
 		 * A conflict has occurred but the porcelain
 		 * (typically rebase --interactive) wants to take care

@@ -415,7 +415,14 @@ static void print_advice(struct repository *r, int show_hint,
 	}
 
 	if (show_hint) {
-		if (opts->no_commit)
+		if (opts->action == REPLAY_PICK) {
+			advise(_("Resolve all conflicts manually, mark them as resolved with\n"
+				 "\"git add/rm <conflicted_files>\", then run\n"
+				 "\"git cherry-pick --continue\".\n"
+				 "You can instead skip this commit: run \"git cherry-pick --skip\".\n"
+				 "To abort and get back to the state before \"git cherry-pick\",\n"
+				 "run \"git cherry-pick --abort\"."));
+		} else if (opts->no_commit)
 			advise(_("after resolving the conflicts, mark the corrected paths\n"
 				 "with 'git add <paths>' or 'git rm <paths>'"));
 		else

index 014001b8f32..d3ed9d7ce0d 100755
@@ -53,9 +53,12 @@ test_expect_success 'advice from failed cherry-pick' "
 	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'
+	hint: Resolve all conflicts manually, mark them as resolved with
+	hint: \"git add/rm <conflicted_files>\", then run
+	hint: \"git cherry-pick --continue\".
+	hint: You can instead skip this commit: run \"git cherry-pick --skip\".
+	hint: To abort and get back to the state before \"git cherry-pick\",
+	hint: run \"git cherry-pick --abort\".
 	EOF
 	test_must_fail git cherry-pick picked 2>actual &&
 

@@ -68,8 +71,12 @@ test_expect_success 'advice from failed cherry-pick --no-commit' "
 	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: Resolve all conflicts manually, mark them as resolved with
+	hint: \"git add/rm <conflicted_files>\", then run
+	hint: \"git cherry-pick --continue\".
+	hint: You can instead skip this commit: run \"git cherry-pick --skip\".
+	hint: To abort and get back to the state before \"git cherry-pick\",
+	hint: run \"git cherry-pick --abort\".
 	EOF
 	test_must_fail git cherry-pick --no-commit picked 2>actual &&
 

base-commit: daab8a564f8bbac55f70f8bf86c070e001a9b006

gitgitgadget

Hi,
sorry for the late reply, I am busy processing the patches on the ref-filter side.

Make sense.

Thanks, I haven't paid attention to this detail before.

Thanks,
--
ZheNing Hu

Hmph, actually the double-quote that opens the body of test_expect_success should be stared at with a very cautious eyes, as they often do not mean what the author of the patch thought they do. I can see that it already fooled your eyes into thinking that there is no substitution, but $picked needs to be substituted into its value. The backslash before it is *not* about guarding it from substitution inside here-doc; it is to pass literal "$" into the string, which is the last parameter to test_expect_success, that gets eval'ed.

The original of this one, for example, would probably have been better if written like so:

test_expect_success 'advice from failed cherry-pick' '

	pristine_detach initial &&
	SQ='\'' &&

	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 ${SQ}git add <paths>${SQ} or ${SQ}git rm <paths>${SQ}
	hint: and commit the result with ${SQ}git commit${SQ}
	EOF
	test_must_fail git cherry-pick picked 2>actual &&

	test_cmp expected actual

'

And because there is no single quote in the updated text, it would become:

test_expect_success 'advice from failed cherry-pick' '

	pristine_detach initial &&

	picked=$(git rev-parse --short picked) &&
	cat <<-EOF >expected &&
	error: could not apply $picked... picked
	hint: Resolve all conflicts manually, mark them as resolved with
	hint: "git add/rm <conflicted_files>", then run
	EOF
	test_must_fail git cherry-pick picked 2>actual &&

	test_cmp expected actual

'

which makes it far easier to see that $picked needs to be substituted, and the "git add/rm" are to be enclosed in dq pair.

Yes, the escaping here comes from double-quote of test_expect_success instead of here-doc.

Um? This section is not working for me.

./test-lib.sh: eval: line 917: unexpected EOF while looking for matching `'' ./test-lib.sh: eval: line 932: syntax error: unexpected end of file

This section is exactly what I want. Using single quotes does make it look a lot easier.

--
ZheNing Hu

Good point, you're right I had completely missed that, thanks for pointing it out

Phillip

From: ZheNing Hu <[email protected]>

In the past, git cherry-pick would print such advice when there was a conflict:

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'

But in fact, when we want to cherry-pick multiple commits, we should not use "git commit" after resolving conflicts, which will make Git generate some errors. We should recommend users to use git cherry-pick --continue, git cherry-pick --abort, just like git rebase does.

This is the improved advice:

hint: Resolve all conflicts manually, mark them as resolved with
hint: "git add/rm <conflicted_files>", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit: run "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".

Similarly, this optimization can be applied to git revert:

hint: Resolve all conflicts manually, mark them as resolved with
hint: "git add/rm <conflicted_files>", then run
hint: "git revert --continue".
hint: You can instead skip this commit: run "git revert --skip".
hint: To abort and get back to the state before "git revert",
hint: run "git revert --abort".

It is worth mentioning that now we use advice() to print the content of GIT_CHERRY_PICK_HELP in print_advice(), each line of output will start with "hint: ".

Mentored-by Hariom Verma <[email protected]>:

---
[GSOC] cherry-pick: use better advice message

Because git cherry-pick's past advice message is not good enough, it often misleads new users of Git, so this patch makes git chery-pick advice message better.

v6:
https://lore.kernel.org/git/[email protected]/

v6-->v7:

1. Modify the advice order, respect git cherry-pick --no-commit advice.
2. Let git revert also use better message.
3. Use double quotes instead of single quotes for the test body.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1010%2Fadlternative%2Fcherry-pick-help-fix-3-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1010/adlternative/cherry-pick-help-fix-3-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1010

Range-diff vs v3:

1: 701645dde17 ! 1: dc51c0b8c2b [GSOC] cherry-pick: use better advice message
@@ Commit message
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".

+ Similarly, this optimization can be applied to git revert:
+
+ hint: Resolve all conflicts manually, mark them as resolved with
+ hint: "git add/rm <conflicted_files>", then run
+ hint: "git revert --continue".
+ hint: You can instead skip this commit: run "git revert --skip".
+ hint: To abort and get back to the state before "git revert",
+ hint: run "git revert --abort".
+
It is worth mentioning that now we use advice() to print the content
of GIT_CHERRY_PICK_HELP in print_advice(), each line of output will
start with "hint: ".
@@ sequencer.c: static void print_advice(struct repository *r, int show_hint,
* A conflict has occurred but the porcelain
* (typically rebase --interactive) wants to take care
@@ sequencer.c: static void print_advice(struct repository *r, int show_hint,
- }
-
- if (show_hint) {
-- if (opts->no_commit)
-+ if (opts->action == REPLAY_PICK) {
+ if (opts->no_commit)
+ advise(_("after resolving the conflicts, mark the corrected paths\n"
+ "with 'git add <paths>' or 'git rm <paths>'"));
++ else if (opts->action == REPLAY_PICK)
+ advise(_("Resolve all conflicts manually, mark them as resolved with\n"
+ "\"git add/rm <conflicted_files>\", then run\n"
+ "\"git cherry-pick --continue\".\n"
+ "You can instead skip this commit: run \"git cherry-pick --skip\".\n"
+ "To abort and get back to the state before \"git cherry-pick\",\n"
+ "run \"git cherry-pick --abort\"."));
-+ } else if (opts->no_commit)
- advise(_("after resolving the conflicts, mark the corrected paths\n"
- "with 'git add <paths>' or 'git rm <paths>'"));
++ else if (opts->action == REPLAY_REVERT)
++ advise(_("Resolve all conflicts manually, mark them as resolved with\n"
++ "\"git add/rm <conflicted_files>\", then run\n"
++ "\"git revert --continue\".\n"
++ "You can instead skip this commit: run \"git revert --skip\".\n"
++ "To abort and get back to the state before \"git revert\",\n"
++ "run \"git revert --abort\"."));
else
+ advise(_("after resolving the conflicts, mark the corrected paths\n"
+ "with 'git add <paths>' or 'git rm <paths>'\n"
+
+ ## t/t3501-revert-cherry-pick.sh ##
+@@ t/t3501-revert-cherry-pick.sh: test_expect_success 'cherry-pick works with dirty renamed file' '
+ grep -q "^modified$" renamed
+ '
+
++test_expect_success 'advice from failed revert' '
++ echo dream >dream &&
++ git add dream &&
++ git commit -m "add dream" &&
++ dream_oid=$(git rev-parse --short HEAD) &&
++ cat <<-EOF >expected &&
++ error: could not revert $dream_oid... add dream
++ hint: Resolve all conflicts manually, mark them as resolved with
++ hint: "git add/rm <conflicted_files>", then run
++ hint: "git revert --continue".
++ hint: You can instead skip this commit: run "git revert --skip".
++ hint: To abort and get back to the state before "git revert",
++ hint: run "git revert --abort".
++ EOF
++ echo dream >>dream &&
++ git add dream &&
++ git commit -m "double-add dream" &&
++ test_must_fail git revert HEAD^ 2>actual &&
++ test_cmp expected actual
++'
+ test_done

## t/t3507-cherry-pick-conflict.sh ##
-@@ t/t3507-cherry-pick-conflict.sh: test_expect_success 'advice from failed cherry-pick' "
- picked=\$(git rev-parse --short picked) &&
+@@ t/t3507-cherry-pick-conflict.sh: test_expect_success 'failed cherry-pick does not advance HEAD' '
+ test "$head" = "$newhead"
+ '
+
+-test_expect_success 'advice from failed cherry-pick' "
++test_expect_success 'advice from failed cherry-pick' '
+ pristine_detach initial &&
+
+- picked=\$(git rev-parse --short picked) &&
++ picked=$(git rev-parse --short picked) &&
cat <<-EOF >expected &&
- error: could not apply \$picked... picked
+- 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'
++ error: could not apply $picked... picked
+ hint: Resolve all conflicts manually, mark them as resolved with
-+ hint: \"git add/rm <conflicted_files>\", then run
-+ hint: \"git cherry-pick --continue\".
-+ hint: You can instead skip this commit: run \"git cherry-pick --skip\".
-+ hint: To abort and get back to the state before \"git cherry-pick\",
-+ hint: run \"git cherry-pick --abort\".
++ hint: "git add/rm <conflicted_files>", then run
++ hint: "git cherry-pick --continue".
++ hint: You can instead skip this commit: run "git cherry-pick --skip".
++ hint: To abort and get back to the state before "git cherry-pick",
++ hint: run "git cherry-pick --abort".
EOF
test_must_fail git cherry-pick picked 2>actual &&

-@@ t/t3507-cherry-pick-conflict.sh: test_expect_success 'advice from failed cherry-pick --no-commit' "
- 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: Resolve all conflicts manually, mark them as resolved with
-+ hint: \"git add/rm <conflicted_files>\", then run
-+ hint: \"git cherry-pick --continue\".
-+ hint: You can instead skip this commit: run \"git cherry-pick --skip\".
-+ hint: To abort and get back to the state before \"git cherry-pick\",
-+ hint: run \"git cherry-pick --abort\".
- EOF
- test_must_fail git cherry-pick --no-commit picked 2>actual &&
+ test_cmp expected actual
+-"
++'

+ test_expect_success 'advice from failed cherry-pick --no-commit' "
+ pristine_detach initial &&

 sequencer.c                     | 16 +++++++++++++++-
 t/t3501-revert-cherry-pick.sh   | 20 ++++++++++++++++++++
 t/t3507-cherry-pick-conflict.sh | 17 ++++++++++-------
 3 files changed, 45 insertions(+), 8 deletions(-)

index 0bec01cf38e..2dd73d24a87 100644
@@ -403,7 +403,7 @@ static void print_advice(struct repository *r, int show_hint,
 	char *msg = getenv("GIT_CHERRY_PICK_HELP");
 
 	if (msg) {
-		fprintf(stderr, "%s\n", msg);
+		advise("%s\n", msg);
 		/*
 		 * A conflict has occurred but the porcelain
 		 * (typically rebase --interactive) wants to take care

@@ -418,6 +418,20 @@ static void print_advice(struct repository *r, int show_hint,
 		if (opts->no_commit)
 			advise(_("after resolving the conflicts, mark the corrected paths\n"
 				 "with 'git add <paths>' or 'git rm <paths>'"));
+		else if (opts->action == REPLAY_PICK)
+			advise(_("Resolve all conflicts manually, mark them as resolved with\n"
+				 "\"git add/rm <conflicted_files>\", then run\n"
+				 "\"git cherry-pick --continue\".\n"
+				 "You can instead skip this commit: run \"git cherry-pick --skip\".\n"
+				 "To abort and get back to the state before \"git cherry-pick\",\n"
+				 "run \"git cherry-pick --abort\"."));
+		else if (opts->action == REPLAY_REVERT)
+			advise(_("Resolve all conflicts manually, mark them as resolved with\n"
+				 "\"git add/rm <conflicted_files>\", then run\n"
+				 "\"git revert --continue\".\n"
+				 "You can instead skip this commit: run \"git revert --skip\".\n"
+				 "To abort and get back to the state before \"git revert\",\n"
+				 "run \"git revert --abort\"."));
 		else
 			advise(_("after resolving the conflicts, mark the corrected paths\n"
 				 "with 'git add <paths>' or 'git rm <paths>'\n"

index 9d100cd1884..6766aed7282 100755
@@ -158,4 +158,24 @@ test_expect_success 'cherry-pick works with dirty renamed file' '
 	grep -q "^modified$" renamed
 '
 
+test_expect_success 'advice from failed revert' '
+	echo dream >dream &&
+	git add dream &&
+	git commit -m "add dream" &&
+	dream_oid=$(git rev-parse --short HEAD) &&
+	cat <<-EOF >expected &&
+	error: could not revert $dream_oid... add dream
+	hint: Resolve all conflicts manually, mark them as resolved with
+	hint: "git add/rm <conflicted_files>", then run
+	hint: "git revert --continue".
+	hint: You can instead skip this commit: run "git revert --skip".
+	hint: To abort and get back to the state before "git revert",
+	hint: run "git revert --abort".
+	EOF
+	echo dream >>dream &&
+	git add dream &&
+	git commit -m "double-add dream" &&
+	test_must_fail git revert HEAD^ 2>actual &&
+	test_cmp expected actual
+'
 test_done

index 014001b8f32..cb2ebea9ad3 100755
@@ -47,20 +47,23 @@ test_expect_success 'failed cherry-pick does not advance HEAD' '
 	test "$head" = "$newhead"
 '
 
-test_expect_success 'advice from failed cherry-pick' "
+test_expect_success 'advice from failed cherry-pick' '
 	pristine_detach initial &&
 
-	picked=\$(git rev-parse --short picked) &&
+	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'
+	error: could not apply $picked... picked
+	hint: Resolve all conflicts manually, mark them as resolved with
+	hint: "git add/rm <conflicted_files>", then run
+	hint: "git cherry-pick --continue".
+	hint: You can instead skip this commit: run "git cherry-pick --skip".
+	hint: To abort and get back to the state before "git cherry-pick",
+	hint: run "git cherry-pick --abort".
 	EOF
 	test_must_fail git cherry-pick picked 2>actual &&
 
 	test_cmp expected actual
-"
+'
 
 test_expect_success 'advice from failed cherry-pick --no-commit' "
 	pristine_detach initial &&

base-commit: daab8a564f8bbac55f70f8bf86c070e001a9b006

gitgitgadget

In our log messages, we desciribe the current state in the present tense.

It may be worth mentioning that this is because the program originally was about picking only one commit and the hint was inherited from those days. Or it may not. I dunno.

I am not sure "should not" is the right phrase. Also it does not help readers to have a vague "generate some errors" than not saying anything---it leaves readers puzzled with "what errors???"

Whether picking a single commit or a series of commits, after resolving conflicts in the current step, wouldn't

    $ git commit ;# to conclude the resolution
    $ git cherry-pick --continue

do the right thing?

It may be an improved advice, but just omit it and say something like:

Suggest use of "git cherry-pick --contiue", so that it would also apply to cases where multiple commits are being picked.

The actual message does not have to be reproduced here, as it is in the source, and it can be seen in the test ;-)

That would make the proposed log message conform to our norm, i.e. brief description of what happens with the current system, followed by description of the perceived problem, followed by an order to the codebase to become different in a specific way that solves the problem.

Taken together, perhaps

"git cherry-pick", upon seeing a conflict, says:

hint: ...

as if running "git commit" to conclude the resolution of this single step were the end of the story. This stems from the fact that the command originally was to pick a single commit and not a range of commits, and the message was written back then and has not been adjusted.

When picking a range of commits and the command stops with a conflict in the middle of the range, however, after resolving the conflict and (optionally) recording the result with "git commit", the user has to run "git cherry-pick --continue" to have the rest of the range dealt with, "--skip" to drop the current commit, or "--abort" to discard the series.

Suggest use of "git cherry-pick --continue/--skip/--abort" so that the message also covers the case where a range of commits are being picked.

It seems that unlike other people I keep hepling you, whatever that verb means ;-).

Thanks.

Yes, may look at the time, the design is correct.

yes, this error is specifically that if we want pick a series of commits, after resolving conflicts and use git commit, .git/sequencer still exists, but git cherry-pick has ended (the next step cherry-pick is not performed). This may be a very strange critical state.

I think you mean that git commit is used to end a single cherry-pick. But in the face of a series of commits, errors will occur.

Yes, maybe the commit message should be shorter and more concise.

Ok, I understand now, git commit just stopped git cherry-pick by delete CHERRY_PICK_HEAD in sequencer_post_commit_cleanup(). we can still resume the process with git cherry-pick --continue.

Ehhh, I seem to have forgotten the correction here.

Thanks.
--
ZheNing Hu

From: ZheNing Hu <[email protected]>

"git cherry-pick", upon seeing a conflict, says:

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'

as if running "git commit" to conclude the resolution of this single step were the end of the story. This stems from the fact that the command originally was to pick a single commit and not a range of commits, and the message was written back then and has not been adjusted.

When picking a range of commits and the command stops with a conflict in the middle of the range, however, after resolving the conflict and (optionally) recording the result with "git commit", the user has to run "git cherry-pick --continue" to have the rest of the range dealt with, "--skip" to drop the current commit, or "--abort" to discard the series.

Suggest use of "git cherry-pick --continue/--skip/--abort" so that the message also covers the case where a range of commits are being picked.

Similarly, this optimization can be applied to git revert, suggest use of "git revert --continue/--skip/--abort" so that the message also covers the case where a range of commits are being reverted.

It is worth mentioning that now we use advice() to print the content of GIT_CHERRY_PICK_HELP in print_advice(), each line of output will start with "hint: ".

Mentored-by Hariom Verma <[email protected]>:

---
[GSOC] cherry-pick: use better advice message

The cherry-pick and revert advice message are only suitable for picking one commit or reverting one commit, but not for multiple commits. So correct the advice message to have the rest of the range dealt with.

v7:
https://lore.kernel.org/git/[email protected]/

v7-->v8:

1. Modify the commit message to make it more in line with the specification.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1010%2Fadlternative%2Fcherry-pick-help-fix-3-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1010/adlternative/cherry-pick-help-fix-3-v5
Pull-Request: https://github.com/gitgitgadget/git/pull/1010

Range-diff vs v4:

1: dc51c0b8c2b ! 1: 6cf78ffd088 [GSOC] cherry-pick: use better advice message
@@ Metadata
## Commit message ##
[GSOC] cherry-pick: use better advice message

- In the past, git cherry-pick would print such advice when
- there was a conflict:
+ "git cherry-pick", upon seeing a conflict, says:

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'

- But in fact, when we want to cherry-pick multiple commits,
- we should not use "git commit" after resolving conflicts, which
- will make Git generate some errors. We should recommend users to
- use git cherry-pick --continue, git cherry-pick --abort, just
- like git rebase does.
+ as if running "git commit" to conclude the resolution of
+ this single step were the end of the story. This stems from
+ the fact that the command originally was to pick a single
+ commit and not a range of commits, and the message was
+ written back then and has not been adjusted.

- This is the improved advice:
+ When picking a range of commits and the command stops with a
+ conflict in the middle of the range, however, after
+ resolving the conflict and (optionally) recording the result
+ with "git commit", the user has to run "git cherry-pick
+ --continue" to have the rest of the range dealt with,
+ "--skip" to drop the current commit, or "--abort" to discard
+ the series.

- hint: Resolve all conflicts manually, mark them as resolved with
- hint: "git add/rm <conflicted_files>", then run
- hint: "git cherry-pick --continue".
- hint: You can instead skip this commit: run "git cherry-pick --skip".
- hint: To abort and get back to the state before "git cherry-pick",
- hint: run "git cherry-pick --abort".
+ Suggest use of "git cherry-pick --continue/--skip/--abort"
+ so that the message also covers the case where a range of
+ commits are being picked.

- Similarly, this optimization can be applied to git revert:
+ Similarly, this optimization can be applied to git revert,
+ suggest use of "git revert --continue/--skip/--abort" so
+ that the message also covers the case where a range of
+ commits are being reverted.

- hint: Resolve all conflicts manually, mark them as resolved with
- hint: "git add/rm <conflicted_files>", then run
- hint: "git revert --continue".
- hint: You can instead skip this commit: run "git revert --skip".
- hint: To abort and get back to the state before "git revert",
- hint: run "git revert --abort".
-
- It is worth mentioning that now we use advice() to print the content
- of GIT_CHERRY_PICK_HELP in print_advice(), each line of output will
- start with "hint: ".
+ It is worth mentioning that now we use advice() to print
+ the content of GIT_CHERRY_PICK_HELP in print_advice(), each
+ line of output will start with "hint: ".

Mentored-by Hariom Verma <[email protected]>:

- Hepled-by: Junio C Hamano <[email protected]>
+ Helped-by: Junio C Hamano <[email protected]>

## sequencer.c ##

 sequencer.c                     | 16 +++++++++++++++-
 t/t3501-revert-cherry-pick.sh   | 20 ++++++++++++++++++++
 t/t3507-cherry-pick-conflict.sh | 17 ++++++++++-------
 3 files changed, 45 insertions(+), 8 deletions(-)

index 0bec01cf38e..2dd73d24a87 100644
@@ -403,7 +403,7 @@ static void print_advice(struct repository *r, int show_hint,
 	char *msg = getenv("GIT_CHERRY_PICK_HELP");
 
 	if (msg) {
-		fprintf(stderr, "%s\n", msg);
+		advise("%s\n", msg);
 		/*
 		 * A conflict has occurred but the porcelain
 		 * (typically rebase --interactive) wants to take care

@@ -418,6 +418,20 @@ static void print_advice(struct repository *r, int show_hint,
 		if (opts->no_commit)
 			advise(_("after resolving the conflicts, mark the corrected paths\n"
 				 "with 'git add <paths>' or 'git rm <paths>'"));
+		else if (opts->action == REPLAY_PICK)
+			advise(_("Resolve all conflicts manually, mark them as resolved with\n"
+				 "\"git add/rm <conflicted_files>\", then run\n"
+				 "\"git cherry-pick --continue\".\n"
+				 "You can instead skip this commit: run \"git cherry-pick --skip\".\n"
+				 "To abort and get back to the state before \"git cherry-pick\",\n"
+				 "run \"git cherry-pick --abort\"."));
+		else if (opts->action == REPLAY_REVERT)
+			advise(_("Resolve all conflicts manually, mark them as resolved with\n"
+				 "\"git add/rm <conflicted_files>\", then run\n"
+				 "\"git revert --continue\".\n"
+				 "You can instead skip this commit: run \"git revert --skip\".\n"
+				 "To abort and get back to the state before \"git revert\",\n"
+				 "run \"git revert --abort\"."));
 		else
 			advise(_("after resolving the conflicts, mark the corrected paths\n"
 				 "with 'git add <paths>' or 'git rm <paths>'\n"

index 9d100cd1884..6766aed7282 100755
@@ -158,4 +158,24 @@ test_expect_success 'cherry-pick works with dirty renamed file' '
 	grep -q "^modified$" renamed
 '
 
+test_expect_success 'advice from failed revert' '
+	echo dream >dream &&
+	git add dream &&
+	git commit -m "add dream" &&
+	dream_oid=$(git rev-parse --short HEAD) &&
+	cat <<-EOF >expected &&
+	error: could not revert $dream_oid... add dream
+	hint: Resolve all conflicts manually, mark them as resolved with
+	hint: "git add/rm <conflicted_files>", then run
+	hint: "git revert --continue".
+	hint: You can instead skip this commit: run "git revert --skip".
+	hint: To abort and get back to the state before "git revert",
+	hint: run "git revert --abort".
+	EOF
+	echo dream >>dream &&
+	git add dream &&
+	git commit -m "double-add dream" &&
+	test_must_fail git revert HEAD^ 2>actual &&
+	test_cmp expected actual
+'
 test_done

index 014001b8f32..cb2ebea9ad3 100755
@@ -47,20 +47,23 @@ test_expect_success 'failed cherry-pick does not advance HEAD' '
 	test "$head" = "$newhead"
 '
 
-test_expect_success 'advice from failed cherry-pick' "
+test_expect_success 'advice from failed cherry-pick' '
 	pristine_detach initial &&
 
-	picked=\$(git rev-parse --short picked) &&
+	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'
+	error: could not apply $picked... picked
+	hint: Resolve all conflicts manually, mark them as resolved with
+	hint: "git add/rm <conflicted_files>", then run
+	hint: "git cherry-pick --continue".
+	hint: You can instead skip this commit: run "git cherry-pick --skip".
+	hint: To abort and get back to the state before "git cherry-pick",
+	hint: run "git cherry-pick --abort".
 	EOF
 	test_must_fail git cherry-pick picked 2>actual &&
 
 	test_cmp expected actual
-"
+'
 
 test_expect_success 'advice from failed cherry-pick --no-commit' "
 	pristine_detach initial &&

base-commit: daab8a564f8bbac55f70f8bf86c070e001a9b006

gitgitgadget

Thanks for reviewing too.

Maybe you are right, If no one else will touch it, it may be reasonable to set it as BUG().

Thanks, I may also need a --no-tag option, because "add dream" is not a valid tag name.

Same too, test_commit --append --no-tag "double-add dream" dream dream

Thanks.
--
ZheNing Hu

GIT_CHERRY_PICK_HELP should be set for other commands except cherry-pick and revert.

--
ZheNing

From: ZheNing Hu <[email protected]>

"git cherry-pick", upon seeing a conflict, says:

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'

as if running "git commit" to conclude the resolution of this single step were the end of the story. This stems from the fact that the command originally was to pick a single commit and not a range of commits, and the message was written back then and has not been adjusted.

When picking a range of commits and the command stops with a conflict in the middle of the range, however, after resolving the conflict and (optionally) recording the result with "git commit", the user has to run "git cherry-pick --continue" to have the rest of the range dealt with, "--skip" to drop the current commit, or "--abort" to discard the series.

Suggest use of "git cherry-pick --continue/--skip/--abort" so that the message also covers the case where a range of commits are being picked.

Similarly, this optimization can be applied to git revert, suggest use of "git revert --continue/--skip/--abort" so that the message also covers the case where a range of commits are being reverted.

It is worth mentioning that now we use advice() to print the content of GIT_CHERRY_PICK_HELP in print_advice(), each line of output will start with "hint: ".

Mentored-by Hariom Verma <[email protected]>

---
[GSOC] cherry-pick: use better advice message

The cherry-pick and revert advice message are only suitable for picking one commit or reverting one commit, but not for multiple commits. So correct the advice message to have the rest of the range dealt with.

v8:
https://lore.kernel.org/git/[email protected]/

v8-->v9:

1. Use the test_commit function in the test.
2. Treat these commands which are not using cherry-pick or revert and without setting GIT_CHERRY_PICK_HELP as a BUG.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1010%2Fadlternative%2Fcherry-pick-help-fix-3-v6
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1010/adlternative/cherry-pick-help-fix-3-v6
Pull-Request: https://github.com/gitgitgadget/git/pull/1010

Range-diff vs v5:

1: 6cf78ffd088 ! 1: 84676c475ee [GSOC] cherry-pick: use better advice message
@@ Commit message
line of output will start with "hint: ".

- Mentored-by Hariom Verma <[email protected]>:
+ Mentored-by Hariom Verma <[email protected]>

@@ sequencer.c: static void print_advice(struct repository *r, int show_hint,
+ "To abort and get back to the state before \"git revert\",\n"
+ "run \"git revert --abort\"."));
else
- advise(_("after resolving the conflicts, mark the corrected paths\n"
- "with 'git add <paths>' or 'git rm <paths>'\n"
+- 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'"));
++ BUG("unexpected pick action in print_advice()");
+ }
+ }
+

## t/t3501-revert-cherry-pick.sh ##
@@ t/t3501-revert-cherry-pick.sh: test_expect_success 'cherry-pick works with dirty renamed file' '
@@ t/t3501-revert-cherry-pick.sh: test_expect_success 'cherry-pick works with dirty
'

+test_expect_success 'advice from failed revert' '
-+ echo dream >dream &&
-+ git add dream &&
-+ git commit -m "add dream" &&
++ test_commit --no-tag "add dream" dream dream &&
+ dream_oid=$(git rev-parse --short HEAD) &&
+ cat <<-EOF >expected &&
+ error: could not revert $dream_oid... add dream
@@ t/t3501-revert-cherry-pick.sh: test_expect_success 'cherry-pick works with dirty
+ hint: To abort and get back to the state before "git revert",
+ hint: run "git revert --abort".
+ EOF
-+ echo dream >>dream &&
-+ git add dream &&
-+ git commit -m "double-add dream" &&
++ test_commit --append --no-tag "double-add dream" dream dream &&
+ test_must_fail git revert HEAD^ 2>actual &&
+ test_cmp expected actual
+'

 sequencer.c                     | 20 ++++++++++++++++----
 t/t3501-revert-cherry-pick.sh   | 16 ++++++++++++++++
 t/t3507-cherry-pick-conflict.sh | 17 ++++++++++-------
 3 files changed, 42 insertions(+), 11 deletions(-)

index 0bec01cf38e..f114b4ba80e 100644
@@ -403,7 +403,7 @@ static void print_advice(struct repository *r, int show_hint,
 	char *msg = getenv("GIT_CHERRY_PICK_HELP");
 
 	if (msg) {
-		fprintf(stderr, "%s\n", msg);
+		advise("%s\n", msg);
 		/*
 		 * A conflict has occurred but the porcelain
 		 * (typically rebase --interactive) wants to take care

@@ -418,10 +418,22 @@ static void print_advice(struct repository *r, int show_hint,
 		if (opts->no_commit)
 			advise(_("after resolving the conflicts, mark the corrected paths\n"
 				 "with 'git add <paths>' or 'git rm <paths>'"));
+		else if (opts->action == REPLAY_PICK)
+			advise(_("Resolve all conflicts manually, mark them as resolved with\n"
+				 "\"git add/rm <conflicted_files>\", then run\n"
+				 "\"git cherry-pick --continue\".\n"
+				 "You can instead skip this commit: run \"git cherry-pick --skip\".\n"
+				 "To abort and get back to the state before \"git cherry-pick\",\n"
+				 "run \"git cherry-pick --abort\"."));
+		else if (opts->action == REPLAY_REVERT)
+			advise(_("Resolve all conflicts manually, mark them as resolved with\n"
+				 "\"git add/rm <conflicted_files>\", then run\n"
+				 "\"git revert --continue\".\n"
+				 "You can instead skip this commit: run \"git revert --skip\".\n"
+				 "To abort and get back to the state before \"git revert\",\n"
+				 "run \"git revert --abort\"."));
 		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'"));
+			BUG("unexpected pick action in print_advice()");
 	}
 }
 

index 9d100cd1884..e70a33f3e60 100755
@@ -158,4 +158,20 @@ test_expect_success 'cherry-pick works with dirty renamed file' '
 	grep -q "^modified$" renamed
 '
 
+test_expect_success 'advice from failed revert' '
+	test_commit --no-tag "add dream" dream dream &&
+	dream_oid=$(git rev-parse --short HEAD) &&
+	cat <<-EOF >expected &&
+	error: could not revert $dream_oid... add dream
+	hint: Resolve all conflicts manually, mark them as resolved with
+	hint: "git add/rm <conflicted_files>", then run
+	hint: "git revert --continue".
+	hint: You can instead skip this commit: run "git revert --skip".
+	hint: To abort and get back to the state before "git revert",
+	hint: run "git revert --abort".
+	EOF
+	test_commit --append --no-tag "double-add dream" dream dream &&
+	test_must_fail git revert HEAD^ 2>actual &&
+	test_cmp expected actual
+'
 test_done

index 014001b8f32..cb2ebea9ad3 100755
@@ -47,20 +47,23 @@ test_expect_success 'failed cherry-pick does not advance HEAD' '
 	test "$head" = "$newhead"
 '
 
-test_expect_success 'advice from failed cherry-pick' "
+test_expect_success 'advice from failed cherry-pick' '
 	pristine_detach initial &&
 
-	picked=\$(git rev-parse --short picked) &&
+	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'
+	error: could not apply $picked... picked
+	hint: Resolve all conflicts manually, mark them as resolved with
+	hint: "git add/rm <conflicted_files>", then run
+	hint: "git cherry-pick --continue".
+	hint: You can instead skip this commit: run "git cherry-pick --skip".
+	hint: To abort and get back to the state before "git cherry-pick",
+	hint: run "git cherry-pick --abort".
 	EOF
 	test_must_fail git cherry-pick picked 2>actual &&
 
 	test_cmp expected actual
-"
+'
 
 test_expect_success 'advice from failed cherry-pick --no-commit' "
 	pristine_detach initial &&

base-commit: daab8a564f8bbac55f70f8bf86c070e001a9b006

gitgitgadget

I think you meant to move ':' not remove it.

Yeah. Note the $reslovemsg in git-rebase--preserve-merges.sh, I copied it from here. Perhaps it is also worth modifying?

Indeed so.

Thanks.
--
ZheNing Hu

From: ZheNing Hu <[email protected]>

"git cherry-pick", upon seeing a conflict, says:

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'

as if running "git commit" to conclude the resolution of this single step were the end of the story. This stems from the fact that the command originally was to pick a single commit and not a range of commits, and the message was written back then and has not been adjusted.

When picking a range of commits and the command stops with a conflict in the middle of the range, however, after resolving the conflict and (optionally) recording the result with "git commit", the user has to run "git cherry-pick --continue" to have the rest of the range dealt with, "--skip" to drop the current commit, or "--abort" to discard the series.

Suggest use of "git cherry-pick --continue/--skip/--abort" so that the message also covers the case where a range of commits are being picked.

Similarly, this optimization can be applied to git revert, suggest use of "git revert --continue/--skip/--abort" so that the message also covers the case where a range of commits are being reverted.

It is worth mentioning that now we use advice() to print the content of GIT_CHERRY_PICK_HELP in print_advice(), each line of output will start with "hint: ".

---
[GSOC] cherry-pick: use better advice message

The cherry-pick and revert advice message are only suitable for picking one commit or reverting one commit, but not for multiple commits. So correct the advice message to have the rest of the range dealt with.

v9:
https://lore.kernel.org/git/[email protected]/

v9-->v10:

1. Correct the wording of the advice message.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1010%2Fadlternative%2Fcherry-pick-help-fix-3-v7
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1010/adlternative/cherry-pick-help-fix-3-v7
Pull-Request: https://github.com/gitgitgadget/git/pull/1010

Range-diff vs v6:

1: 84676c475ee ! 1: 07440ea5ffe [GSOC] cherry-pick: use better advice message
@@ Commit message
line of output will start with "hint: ".

- Mentored-by Hariom Verma <[email protected]>
+ Mentored-by: Hariom Verma <[email protected]>

@@ sequencer.c: static void print_advice(struct repository *r, int show_hint,
advise(_("after resolving the conflicts, mark the corrected paths\n"
"with 'git add <paths>' or 'git rm <paths>'"));
+ else if (opts->action == REPLAY_PICK)
-+ advise(_("Resolve all conflicts manually, mark them as resolved with\n"
-+ "\"git add/rm <conflicted_files>\", then run\n"
++ advise(_("After resolving the conflicts, mark them with\n"
++ "\"git add/rm <pathspec>\", then run\n"
+ "\"git cherry-pick --continue\".\n"
-+ "You can instead skip this commit: run \"git cherry-pick --skip\".\n"
++ "You can instead skip this commit with \"git cherry-pick --skip\".\n"
+ "To abort and get back to the state before \"git cherry-pick\",\n"
+ "run \"git cherry-pick --abort\"."));
+ else if (opts->action == REPLAY_REVERT)
-+ advise(_("Resolve all conflicts manually, mark them as resolved with\n"
-+ "\"git add/rm <conflicted_files>\", then run\n"
++ advise(_("After resolving the conflicts, mark them with\n"
++ "\"git add/rm <pathspec>\", then run\n"
+ "\"git revert --continue\".\n"
-+ "You can instead skip this commit: run \"git revert --skip\".\n"
++ "You can instead skip this commit with \"git revert --skip\".\n"
+ "To abort and get back to the state before \"git revert\",\n"
+ "run \"git revert --abort\"."));
else
@@ t/t3501-revert-cherry-pick.sh: test_expect_success 'cherry-pick works with dirty
+ dream_oid=$(git rev-parse --short HEAD) &&
+ cat <<-EOF >expected &&
+ error: could not revert $dream_oid... add dream
-+ hint: Resolve all conflicts manually, mark them as resolved with
-+ hint: "git add/rm <conflicted_files>", then run
++ hint: After resolving the conflicts, mark them with
++ hint: "git add/rm <pathspec>", then run
+ hint: "git revert --continue".
-+ hint: You can instead skip this commit: run "git revert --skip".
++ hint: You can instead skip this commit with "git revert --skip".
+ hint: To abort and get back to the state before "git revert",
+ hint: run "git revert --abort".
+ EOF
@@ t/t3507-cherry-pick-conflict.sh: test_expect_success 'failed cherry-pick does no
- hint: with 'git add <paths>' or 'git rm <paths>'
- hint: and commit the result with 'git commit'
+ error: could not apply $picked... picked
-+ hint: Resolve all conflicts manually, mark them as resolved with
-+ hint: "git add/rm <conflicted_files>", then run
++ hint: After resolving the conflicts, mark them with
++ hint: "git add/rm <pathspec>", then run
+ hint: "git cherry-pick --continue".
-+ hint: You can instead skip this commit: run "git cherry-pick --skip".
++ hint: You can instead skip this commit with "git cherry-pick --skip".
+ hint: To abort and get back to the state before "git cherry-pick",
+ hint: run "git cherry-pick --abort".
EOF

 sequencer.c                     | 20 ++++++++++++++++----
 t/t3501-revert-cherry-pick.sh   | 16 ++++++++++++++++
 t/t3507-cherry-pick-conflict.sh | 17 ++++++++++-------
 3 files changed, 42 insertions(+), 11 deletions(-)

index 0bec01cf38e..03c7b3dcadc 100644
@@ -403,7 +403,7 @@ static void print_advice(struct repository *r, int show_hint,
 	char *msg = getenv("GIT_CHERRY_PICK_HELP");
 
 	if (msg) {
-		fprintf(stderr, "%s\n", msg);
+		advise("%s\n", msg);
 		/*
 		 * A conflict has occurred but the porcelain
 		 * (typically rebase --interactive) wants to take care

@@ -418,10 +418,22 @@ static void print_advice(struct repository *r, int show_hint,
 		if (opts->no_commit)
 			advise(_("after resolving the conflicts, mark the corrected paths\n"
 				 "with 'git add <paths>' or 'git rm <paths>'"));
+		else if (opts->action == REPLAY_PICK)
+			advise(_("After resolving the conflicts, mark them with\n"
+				 "\"git add/rm <pathspec>\", then run\n"
+				 "\"git cherry-pick --continue\".\n"
+				 "You can instead skip this commit with \"git cherry-pick --skip\".\n"
+				 "To abort and get back to the state before \"git cherry-pick\",\n"
+				 "run \"git cherry-pick --abort\"."));
+		else if (opts->action == REPLAY_REVERT)
+			advise(_("After resolving the conflicts, mark them with\n"
+				 "\"git add/rm <pathspec>\", then run\n"
+				 "\"git revert --continue\".\n"
+				 "You can instead skip this commit with \"git revert --skip\".\n"
+				 "To abort and get back to the state before \"git revert\",\n"
+				 "run \"git revert --abort\"."));
 		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'"));
+			BUG("unexpected pick action in print_advice()");
 	}
 }
 

index 9d100cd1884..4b5b6076733 100755
@@ -158,4 +158,20 @@ test_expect_success 'cherry-pick works with dirty renamed file' '
 	grep -q "^modified$" renamed
 '
 
+test_expect_success 'advice from failed revert' '
+	test_commit --no-tag "add dream" dream dream &&
+	dream_oid=$(git rev-parse --short HEAD) &&
+	cat <<-EOF >expected &&
+	error: could not revert $dream_oid... add dream
+	hint: After resolving the conflicts, mark them with
+	hint: "git add/rm <pathspec>", then run
+	hint: "git revert --continue".
+	hint: You can instead skip this commit with "git revert --skip".
+	hint: To abort and get back to the state before "git revert",
+	hint: run "git revert --abort".
+	EOF
+	test_commit --append --no-tag "double-add dream" dream dream &&
+	test_must_fail git revert HEAD^ 2>actual &&
+	test_cmp expected actual
+'
 test_done

index 014001b8f32..979e843c65a 100755
@@ -47,20 +47,23 @@ test_expect_success 'failed cherry-pick does not advance HEAD' '
 	test "$head" = "$newhead"
 '
 
-test_expect_success 'advice from failed cherry-pick' "
+test_expect_success 'advice from failed cherry-pick' '
 	pristine_detach initial &&
 
-	picked=\$(git rev-parse --short picked) &&
+	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'
+	error: could not apply $picked... picked
+	hint: After resolving the conflicts, mark them with
+	hint: "git add/rm <pathspec>", then run
+	hint: "git cherry-pick --continue".
+	hint: You can instead skip this commit with "git cherry-pick --skip".
+	hint: To abort and get back to the state before "git cherry-pick",
+	hint: run "git cherry-pick --abort".
 	EOF
 	test_must_fail git cherry-pick picked 2>actual &&
 
 	test_cmp expected actual
-"
+'
 
 test_expect_success 'advice from failed cherry-pick --no-commit' "
 	pristine_detach initial &&

base-commit: daab8a564f8bbac55f70f8bf86c070e001a9b006

gitgitgadget