[PATCH 0/2] [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP

[PATCH 0/2] [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP

[ZheNing Hu via GitGitGadget]

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/pull.1007.git.1627561665.gitgitgadget@gmail.com/

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

[PATCH 1/2] [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP

[ZheNing Hu via GitGitGadget]

From: ZheNing Hu <adlternative@gmail.com>

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: Christian Couder <christian.couder@gmail.com>
Mentored-by Hariom Verma <hariom18599@gmail.com>:
Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Hepled-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
 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(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 12f093121d9..5983f37d531 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -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;
diff --git a/builtin/revert.c b/builtin/revert.c
index 237f2f18d4c..15a4b6fe4ee 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -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);
diff --git a/git-rebase--preserve-merges.sh b/git-rebase--preserve-merges.sh
index b9c71d2a71b..eaa8f9de2c5 100644
--- a/git-rebase--preserve-merges.sh
+++ b/git-rebase--preserve-merges.sh
@@ -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
diff --git a/sequencer.c b/sequencer.c
index 0bec01cf38e..83cf6a5da3c 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -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;
 	}
diff --git a/sequencer.h b/sequencer.h
index d57d8ea23d7..76fb4af56fd 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -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;
 
diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index 014001b8f32..f17621d1915 100755
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -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

Re: [PATCH 1/2] [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP

[Phillip Wood]

On 31/07/2021 08:01, ZheNing Hu via GitGitGadget wrote:
> From: ZheNing Hu <adlternative@gmail.com>
> 
> 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: Christian Couder <christian.couder@gmail.com>
> Mentored-by Hariom Verma <hariom18599@gmail.com>:
> Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> Hepled-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: ZheNing Hu <adlternative@gmail.com>
> ---
>   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(-)
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 12f093121d9..5983f37d531 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -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;

I think we could just have
	replay.delete_cherry_pick_head = 1;
and not add a new member to rebase_options as we always want this set
>   
>   	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;
> diff --git a/builtin/revert.c b/builtin/revert.c
> index 237f2f18d4c..15a4b6fe4ee 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -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);
> diff --git a/git-rebase--preserve-merges.sh b/git-rebase--preserve-merges.sh
> index b9c71d2a71b..eaa8f9de2c5 100644
> --- a/git-rebase--preserve-merges.sh
> +++ b/git-rebase--preserve-merges.sh
> @@ -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
> diff --git a/sequencer.c b/sequencer.c
> index 0bec01cf38e..83cf6a5da3c 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -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);

This is a change in behavior as the advice will now be prefixed with 
"hint: ". I think that is probably an improvement so long as it does not 
make the lines too long when the advice is printed.

> +	} 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;
>   	}
> diff --git a/sequencer.h b/sequencer.h
> index d57d8ea23d7..76fb4af56fd 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -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;
>   
> diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
> index 014001b8f32..f17621d1915 100755
> --- a/t/t3507-cherry-pick-conflict.sh
> +++ b/t/t3507-cherry-pick-conflict.sh
> @@ -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 &&

I think it would be useful to add a test for the new cherry-pick option 
that is added in this patch. Overall this patch is looking pretty good 
it just needs a few small changes, thanks for working on it.

Best Wishes

Phillip

Re: [PATCH 1/2] [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP

[ZheNing Hu]

Phillip Wood <phillip.wood123@gmail.com> 于2021年8月1日周日 下午6:10写道：
>
> > diff --git a/builtin/rebase.c b/builtin/rebase.c
> > index 12f093121d9..5983f37d531 100644
> > --- a/builtin/rebase.c
> > +++ b/builtin/rebase.c
> > @@ -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;
>
> I think we could just have
>         replay.delete_cherry_pick_head = 1;
> and not add a new member to rebase_options as we always want this set

Maybe you are right.

> > -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);
>
> This is a change in behavior as the advice will now be prefixed with
> "hint: ". I think that is probably an improvement so long as it does not
> make the lines too long when the advice is printed.
>

Yeah, maybe I should mention this in commit messages.

>
> I think it would be useful to add a test for the new cherry-pick option
> that is added in this patch. Overall this patch is looking pretty good
> it just needs a few small changes, thanks for working on it.
>

Make sence.

> Best Wishes
>
> Phillip

Thanks.
--
ZheNing Hu

[PATCH 2/2] [GSOC] cherry-pick: use better advice message

[ZheNing Hu via GitGitGadget]

From: ZheNing Hu <adlternative@gmail.com>

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: Christian Couder <christian.couder@gmail.com>
Mentored-by Hariom Verma <hariom18599@gmail.com>:
Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Hepled-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
 sequencer.c                     |  9 ++++++++-
 t/t3507-cherry-pick-conflict.sh | 15 ++++++++++-----
 2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 83cf6a5da3c..f6e9d1fddd8 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -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
diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index f17621d1915..2cc3977f5a6 100755
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -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

Re: [PATCH 2/2] [GSOC] cherry-pick: use better advice message

[Phillip Wood]

On 31/07/2021 08:01, ZheNing Hu via GitGitGadget wrote:
> From: ZheNing Hu <adlternative@gmail.com>
> 
> 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".

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

> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by Hariom Verma <hariom18599@gmail.com>:
> Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> Hepled-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: ZheNing Hu <adlternative@gmail.com>
> ---
>   sequencer.c                     |  9 ++++++++-
>   t/t3507-cherry-pick-conflict.sh | 15 ++++++++++-----
>   2 files changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 83cf6a5da3c..f6e9d1fddd8 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -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
> diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
> index f17621d1915..2cc3977f5a6 100755
> --- a/t/t3507-cherry-pick-conflict.sh
> +++ b/t/t3507-cherry-pick-conflict.sh
> @@ -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 &&
>   
> 

Re: [PATCH 2/2] [GSOC] cherry-pick: use better advice message

[ZheNing Hu]

Phillip Wood <phillip.wood123@gmail.com> 于2021年8月1日周日 下午6:14写道：
>

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

Agree.

> Best Wishes
>
> Phillip
>
Thanks.
--
ZheNing Hu

[PATCH v2 0/2] [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP

[ZheNing Hu via GitGitGadget]

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/pull.1010.git.1627714877.gitgitgadget@gmail.com/

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: Christian Couder <christian.couder@gmail.com>
          Mentored-by Hariom Verma <hariom18599@gmail.com>:
          Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
     @@ Commit message
          Signed-off-by: ZheNing Hu <adlternative@gmail.com>
      
       ## 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

[PATCH v2 1/2] [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP

[ZheNing Hu via GitGitGadget]

From: ZheNing Hu <adlternative@gmail.com>

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: Christian Couder <christian.couder@gmail.com>
Mentored-by Hariom Verma <hariom18599@gmail.com>:
Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Hepled-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
 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(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 12f093121d9..08ba437c6a0 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -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;
 }
diff --git a/builtin/revert.c b/builtin/revert.c
index 237f2f18d4c..15a4b6fe4ee 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -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);
diff --git a/git-rebase--preserve-merges.sh b/git-rebase--preserve-merges.sh
index b9c71d2a71b..eaa8f9de2c5 100644
--- a/git-rebase--preserve-merges.sh
+++ b/git-rebase--preserve-merges.sh
@@ -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
diff --git a/sequencer.c b/sequencer.c
index 0bec01cf38e..83cf6a5da3c 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -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;
 	}
diff --git a/sequencer.h b/sequencer.h
index d57d8ea23d7..76fb4af56fd 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -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;
 
diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index 014001b8f32..af5678d981a 100755
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -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

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

[Junio C Hamano]

"ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: ZheNing Hu <adlternative@gmail.com>
>
> 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

set -> sets

> value in `git-rebase--preserve-merges.sh`, `git rebase --merge` set

set -> sets

> GIT_CHERRY_PICK_HELP value in run_specific_rebase().

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

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

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

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

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.

> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by Hariom Verma <hariom18599@gmail.com>:
> Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> Hepled-by: Junio C Hamano <gitster@pobox.com>

Heple?

> diff --git a/sequencer.c b/sequencer.c
> index 0bec01cf38e..83cf6a5da3c 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -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) {
> +		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>'"));

OK.  That makes sense.

> @@ -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);
> +		}

OK, this separation makes sense, too.

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

Hmph, this is a bit troubling.  So has this been part of the
"published" behaviour since d7e5c0cb (Introduce CHERRY_PICK_HEAD,
2011-02-19) that introduced this test, and there are people who are
relying on it?  IOW, should the resolution to the original problem
report have been "if it hurts, don't do it" (in other words, "setting
GIT_CHERRY_PICK_HELP will remove CHERRY_PICK_HEAD, so if you do not
want to get the latter removed, do not set the former")?

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

[ZheNing Hu]

Junio C Hamano <gitster@pobox.com> 于2021年8月4日周三 上午6:36写道：
>

> > GIT_CHERRY_PICK_HELP value in run_specific_rebase().
>
> "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.
>

Thanks, such a supplement is very good.

> Hmph, this is a bit troubling.  So has this been part of the
> "published" behaviour since d7e5c0cb (Introduce CHERRY_PICK_HEAD,
> 2011-02-19) that introduced this test, and there are people who are
> relying on it?  IOW, should the resolution to the original problem
> report have been "if it hurts, don't do it" (in other words, "setting
> GIT_CHERRY_PICK_HELP will remove CHERRY_PICK_HEAD, so if you do not
> want to get the latter removed, do not set the former")?
>

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

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

[Phillip Wood]

On 04/08/2021 09:35, ZheNing Hu wrote:
> Junio C Hamano <gitster@pobox.com> 于2021年8月4日周三 上午6:36写道：
>> Hmph, this is a bit troubling.  So has this been part of the
>> "published" behaviour since d7e5c0cb (Introduce CHERRY_PICK_HEAD,
>> 2011-02-19) that introduced this test, and there are people who are
>> relying on it?  IOW, should the resolution to the original problem
>> report have been "if it hurts, don't do it" (in other words, "setting
>> GIT_CHERRY_PICK_HELP will remove CHERRY_PICK_HEAD, so if you do not
>> want to get the latter removed, do not set the former")?
>>
> 
> 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.

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

> Thanks.
> --
> ZheNing Hu
> 

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

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

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

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

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

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

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

[ZheNing Hu]

Junio C Hamano <gitster@pobox.com> 于2021年8月5日周四 上午1:31写道：
>
> Phillip Wood <phillip.wood123@gmail.com> writes:
>
> >> You mean that cherry_pick with GIT_CHERRY_PICK_HELP suppresses
> >> CHERRY_PICK_HEAD is not even a bug?
>
> 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 ...
>
> > 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.
>
> ... 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.
>

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

Thanks.
--
ZheNing Hu

[PATCH v2 2/2] [GSOC] cherry-pick: use better advice message

[ZheNing Hu via GitGitGadget]

From: ZheNing Hu <adlternative@gmail.com>

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: Christian Couder <christian.couder@gmail.com>
Mentored-by Hariom Verma <hariom18599@gmail.com>:
Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Hepled-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
 sequencer.c                     | 10 +++++++++-
 t/t3507-cherry-pick-conflict.sh | 17 ++++++++++++-----
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 83cf6a5da3c..bf7dbea81dd 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -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
diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index af5678d981a..e953b54e54d 100755
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -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

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

[ZheNing Hu via GitGitGadget]

From: ZheNing Hu <adlternative@gmail.com>

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: Christian Couder <christian.couder@gmail.com>
Mentored-by Hariom Verma <hariom18599@gmail.com>:
Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Hepled-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
    [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/pull.1010.git.1627714877.gitgitgadget@gmail.com/
    
    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: Christian Couder <christian.couder@gmail.com>
          Mentored-by Hariom Verma <hariom18599@gmail.com>:
          Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
     @@ Commit message
          Signed-off-by: ZheNing Hu <adlternative@gmail.com>
      
       ## 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(-)

diff --git a/sequencer.c b/sequencer.c
index 0bec01cf38e..7fa91b99870 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -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
diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index 014001b8f32..d3ed9d7ce0d 100755
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -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

Re: [PATCH v3] [GSOC] cherry-pick: use better advice message

[Phillip Wood]

On 05/08/2021 06:48, ZheNing Hu via GitGitGadget wrote:
> From: ZheNing Hu <adlternative@gmail.com>
> [...]
>   sequencer.c                     | 11 +++++++++--
>   t/t3507-cherry-pick-conflict.sh | 17 ++++++++++++-----
>   2 files changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 0bec01cf38e..7fa91b99870 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -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) {

This changes means we give the wrong advice for 'git cherry-pick 
--no-commit'. I think you want to keep the existing clause as the first 
one and insert this before the else. The advice itself looks good. It 
would be nice to improve the advice for 'git revert' in the same way.

> +			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
> diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
> index 014001b8f32..d3ed9d7ce0d 100755
> --- a/t/t3507-cherry-pick-conflict.sh
> +++ b/t/t3507-cherry-pick-conflict.sh
> @@ -53,9 +53,12 @@ test_expect_success 'advice from failed cherry-pick' "
>   	picked=\$(git rev-parse --short picked) &&
>   	cat <<-EOF >expected &&

If you quote the here doc end marker then there is no substitution in 
the here doc so writing

	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

means you can replace \" with " here

Best Wishes

Phillip

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

Re: [PATCH v3] [GSOC] cherry-pick: use better advice message

[ZheNing Hu]

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

Phillip Wood <phillip.wood123@gmail.com> 于2021年8月11日周三 下午6:00写道：
>
> On 05/08/2021 06:48, ZheNing Hu via GitGitGadget wrote:
> > From: ZheNing Hu <adlternative@gmail.com>
> > [...]
> >   sequencer.c                     | 11 +++++++++--
> >   t/t3507-cherry-pick-conflict.sh | 17 ++++++++++++-----
> >   2 files changed, 21 insertions(+), 7 deletions(-)
> >
> > diff --git a/sequencer.c b/sequencer.c
> > index 0bec01cf38e..7fa91b99870 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -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) {
>
> This changes means we give the wrong advice for 'git cherry-pick
> --no-commit'. I think you want to keep the existing clause as the first
> one and insert this before the else. The advice itself looks good. It
> would be nice to improve the advice for 'git revert' in the same way.
>

Make sense.

> > +                     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
> > diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
> > index 014001b8f32..d3ed9d7ce0d 100755
> > --- a/t/t3507-cherry-pick-conflict.sh
> > +++ b/t/t3507-cherry-pick-conflict.sh
> > @@ -53,9 +53,12 @@ test_expect_success 'advice from failed cherry-pick' "
> >       picked=\$(git rev-parse --short picked) &&
> >       cat <<-EOF >expected &&
>
> If you quote the here doc end marker then there is no substitution in
> the here doc so writing
>
>         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
>
> means you can replace \" with " here
>

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

> Best Wishes
>
> Phillip
>

Thanks,
--
ZheNing Hu

Re: [PATCH v3] [GSOC] cherry-pick: use better advice message

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

>> +++ b/t/t3507-cherry-pick-conflict.sh
>> @@ -53,9 +53,12 @@ test_expect_success 'advice from failed cherry-pick' "
>>   	picked=\$(git rev-parse --short picked) &&
>>   	cat <<-EOF >expected &&
>
> If you quote the here doc end marker then there is no substitution in
> the here doc so writing
>
> 	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
>
> means you can replace \" with " here

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.

Re: [PATCH v3] [GSOC] cherry-pick: use better advice message

[ZheNing Hu]

Junio C Hamano <gitster@pobox.com> 于2021年8月14日周六 上午4:14写道：
>
> Phillip Wood <phillip.wood123@gmail.com> writes:
>
> >> +++ b/t/t3507-cherry-pick-conflict.sh
> >> @@ -53,9 +53,12 @@ test_expect_success 'advice from failed cherry-pick' "
> >>      picked=\$(git rev-parse --short picked) &&
> >>      cat <<-EOF >expected &&
> >
> > If you quote the here doc end marker then there is no substitution in
> > the here doc so writing
> >
> >       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
> >
> > means you can replace \" with " here
>
> 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.
>

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

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

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

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

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

--
ZheNing Hu

Re: [PATCH v3] [GSOC] cherry-pick: use better advice message

[Phillip Wood]

On 13/08/2021 21:14, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>>> +++ b/t/t3507-cherry-pick-conflict.sh
>>> @@ -53,9 +53,12 @@ test_expect_success 'advice from failed cherry-pick' "
>>>    	picked=\$(git rev-parse --short picked) &&
>>>    	cat <<-EOF >expected &&
>>
>> If you quote the here doc end marker then there is no substitution in
>> the here doc so writing
>>
>> 	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
>>
>> means you can replace \" with " here
> 
> 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.

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

Phillip

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

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

[ZheNing Hu via GitGitGadget]

From: ZheNing Hu <adlternative@gmail.com>

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: Christian Couder <christian.couder@gmail.com>
Mentored-by Hariom Verma <hariom18599@gmail.com>:
Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Hepled-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
    [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/pull.1010.v3.git.1628142482640.gitgitgadget@gmail.com/
    
    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(-)

diff --git a/sequencer.c b/sequencer.c
index 0bec01cf38e..2dd73d24a87 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -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"
diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index 9d100cd1884..6766aed7282 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh
@@ -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
diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index 014001b8f32..cb2ebea9ad3 100755
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -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

Re: [PATCH v4] [GSOC] cherry-pick: use better advice message

[Junio C Hamano]

"ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: ZheNing Hu <adlternative@gmail.com>
>
> 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'

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.

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

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?

> This is the improved advice:

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.


> Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> Hepled-by: Junio C Hamano <gitster@pobox.com>

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

Thanks.

> diff --git a/sequencer.c b/sequencer.c
> index 0bec01cf38e..2dd73d24a87 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -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"
> diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
> index 9d100cd1884..6766aed7282 100755
> --- a/t/t3501-revert-cherry-pick.sh
> +++ b/t/t3501-revert-cherry-pick.sh
> @@ -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
> diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
> index 014001b8f32..cb2ebea9ad3 100755
> --- a/t/t3507-cherry-pick-conflict.sh
> +++ b/t/t3507-cherry-pick-conflict.sh
> @@ -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

Re: [PATCH v4] [GSOC] cherry-pick: use better advice message

[ZheNing Hu]

Junio C Hamano <gitster@pobox.com> 于2021年8月15日周日 上午4:33写道：
>
> "ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: ZheNing Hu <adlternative@gmail.com>
> >
> > 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'
>
> 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.
>

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

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

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.

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

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.

> > This is the improved advice:
>
> 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 ;-)
>

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

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

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.

>         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.
>
>
> > Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> > Hepled-by: Junio C Hamano <gitster@pobox.com>
>
> It seems that unlike other people I keep hepling you, whatever that
> verb means ;-).
>

Ehhh, I seem to have forgotten the correction here.

> Thanks.
>

Thanks.
--
ZheNing Hu

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

[ZheNing Hu via GitGitGadget]

From: ZheNing Hu <adlternative@gmail.com>

"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: Christian Couder <christian.couder@gmail.com>
Mentored-by Hariom Verma <hariom18599@gmail.com>:
Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
    [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/pull.1010.v4.git.1628936863733.gitgitgadget@gmail.com/
    
    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: Christian Couder <christian.couder@gmail.com>
          Mentored-by Hariom Verma <hariom18599@gmail.com>:
          Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
     -    Hepled-by: Junio C Hamano <gitster@pobox.com>
     +    Helped-by: Junio C Hamano <gitster@pobox.com>
          Signed-off-by: ZheNing Hu <adlternative@gmail.com>
      
       ## 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(-)

diff --git a/sequencer.c b/sequencer.c
index 0bec01cf38e..2dd73d24a87 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -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"
diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index 9d100cd1884..6766aed7282 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh
@@ -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
diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index 014001b8f32..cb2ebea9ad3 100755
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -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

Re: [PATCH v5] [GSOC] cherry-pick: use better advice message

[Phillip Wood]

On 16/08/2021 01:55, ZheNing Hu via GitGitGadget wrote:
> From: ZheNing Hu <adlternative@gmail.com>
> [...]
>   sequencer.c                     | 16 +++++++++++++++-
>   t/t3501-revert-cherry-pick.sh   | 20 ++++++++++++++++++++
>   t/t3507-cherry-pick-conflict.sh | 17 ++++++++++-------
>   3 files changed, 45 insertions(+), 8 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 0bec01cf38e..2dd73d24a87 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -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\"."));

Thanks for making the revert advice better as well

>   		else
>   			advise(_("after resolving the conflicts, mark the corrected paths\n"
>   				 "with 'git add <paths>' or 'git rm <paths>'\n"

I think this last else arm should probably be a bug now as anything 
other than cherry-pick or revert should be setting GIT_CHERRY_PICK_HELP

	else
		BUG("unexpected pick action in print_advice()");

> diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
> index 9d100cd1884..6766aed7282 100755
> --- a/t/t3501-revert-cherry-pick.sh
> +++ b/t/t3501-revert-cherry-pick.sh
> @@ -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" &&

A minor comment: you can condense these three lines by using test_commit 
(see test-lib-functions.sh for the documentation)

	test_commit "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
> +	echo dream >>dream &&
> +	git add dream &&
> +	git commit -m "double-add dream" &&

test_commit can also append to a file

	test_commit --append "double-add dream" dream dream

This is looking very close to being ready now I think

Thanks

Phillip

> +	test_must_fail git revert HEAD^ 2>actual &&
> +	test_cmp expected actual
> +'
>   test_done
> diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
> index 014001b8f32..cb2ebea9ad3 100755
> --- a/t/t3507-cherry-pick-conflict.sh
> +++ b/t/t3507-cherry-pick-conflict.sh
> @@ -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
> 

Re: [PATCH v5] [GSOC] cherry-pick: use better advice message

[ZheNing Hu]

Phillip Wood <phillip.wood123@gmail.com> 于2021年8月18日周三 下午5:51写道：
>
> Thanks for making the revert advice better as well
>

Thanks for reviewing too.

> >               else
> >                       advise(_("after resolving the conflicts, mark the corrected paths\n"
> >                                "with 'git add <paths>' or 'git rm <paths>'\n"
>
> I think this last else arm should probably be a bug now as anything
> other than cherry-pick or revert should be setting GIT_CHERRY_PICK_HELP
>
>         else
>                 BUG("unexpected pick action in print_advice()");
>

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

> > diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
> > index 9d100cd1884..6766aed7282 100755
> > --- a/t/t3501-revert-cherry-pick.sh
> > +++ b/t/t3501-revert-cherry-pick.sh
> > @@ -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" &&
>
> A minor comment: you can condense these three lines by using test_commit
> (see test-lib-functions.sh for the documentation)
>
>         test_commit "add dream" dream dream
>

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

> > +     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_commit can also append to a file
>
>         test_commit --append "double-add dream" dream dream
>

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

> This is looking very close to being ready now I think
>
> Thanks
>
> Phillip
>

Thanks.
--
ZheNing Hu

Re: [PATCH v5] [GSOC] cherry-pick: use better advice message

[ZheNing Hu]

ZheNing Hu <adlternative@gmail.com> 于2021年8月19日周四 上午9:55写道：
>
> Phillip Wood <phillip.wood123@gmail.com> 于2021年8月18日周三 下午5:51写道：
> >
> > Thanks for making the revert advice better as well
> >
>
> Thanks for reviewing too.
>
> > >               else
> > >                       advise(_("after resolving the conflicts, mark the corrected paths\n"
> > >                                "with 'git add <paths>' or 'git rm <paths>'\n"
> >
> > I think this last else arm should probably be a bug now as anything
> > other than cherry-pick or revert should be setting GIT_CHERRY_PICK_HELP
> >

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

> >         else
> >                 BUG("unexpected pick action in print_advice()");
> >
>
> Maybe you are right, If no one else will touch it, it may be
> reasonable to set it as BUG().
>

--
ZheNing

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

[ZheNing Hu via GitGitGadget]

From: ZheNing Hu <adlternative@gmail.com>

"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: Christian Couder <christian.couder@gmail.com>
Mentored-by Hariom Verma <hariom18599@gmail.com>
Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
    [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/pull.1010.v5.git.1629075306706.gitgitgadget@gmail.com/
    
    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: Christian Couder <christian.couder@gmail.com>
     -    Mentored-by Hariom Verma <hariom18599@gmail.com>:
     +    Mentored-by Hariom Verma <hariom18599@gmail.com>
          Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
          Helped-by: Junio C Hamano <gitster@pobox.com>
          Signed-off-by: ZheNing Hu <adlternative@gmail.com>
     @@ 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(-)

diff --git a/sequencer.c b/sequencer.c
index 0bec01cf38e..f114b4ba80e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -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()");
 	}
 }
 
diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index 9d100cd1884..e70a33f3e60 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh
@@ -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
diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index 014001b8f32..cb2ebea9ad3 100755
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -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

Re: [PATCH v6] [GSOC] cherry-pick: use better advice message

[Junio C Hamano]

"ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:

>           Mentored-by: Christian Couder <christian.couder@gmail.com>
>      -    Mentored-by Hariom Verma <hariom18599@gmail.com>:
>      +    Mentored-by Hariom Verma <hariom18599@gmail.com>

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

> +		else if (opts->action == REPLAY_PICK)
> +			advise(_("Resolve all conflicts manually, mark them as resolved with\n"

Do we need to say "manually"?  Dropping the word would make the
message more concise.  Not repeating the word "resolve" by saying
something like "after resolving the conflicts, mark them with ..."
would probably be a much better phrasing, too.

> +				 "\"git add/rm <conflicted_files>\", then run\n"

<paths> (or <pathspec>) not <conflicted_files>; you can resolve many
files in a directory and give the pathspec to match the directory to
mark all the files in there as resolved.

> +				 "\"git cherry-pick --continue\".\n"
> +				 "You can instead skip this commit: run \"git cherry-pick --skip\".\n"

Inconsistent use of prose in the above and ": run" here.

        You can instead skip this commit with "git cherry-pick --skip"

> +		else if (opts->action == REPLAY_REVERT)

Likewise.

Thanks.

Re: [PATCH v6] [GSOC] cherry-pick: use better advice message

[ZheNing Hu]

Junio C Hamano <gitster@pobox.com> 于2021年8月20日周五 上午1:10写道：
>
> > +             else if (opts->action == REPLAY_PICK)
> > +                     advise(_("Resolve all conflicts manually, mark them as resolved with\n"
>
> Do we need to say "manually"?  Dropping the word would make the
> message more concise.  Not repeating the word "resolve" by saying
> something like "after resolving the conflicts, mark them with ..."
> would probably be a much better phrasing, too.
>

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

> > +                              "\"git add/rm <conflicted_files>\", then run\n"
>
> <paths> (or <pathspec>) not <conflicted_files>; you can resolve many
> files in a directory and give the pathspec to match the directory to
> mark all the files in there as resolved.
>

Indeed so.

> > +                              "\"git cherry-pick --continue\".\n"
> > +                              "You can instead skip this commit: run \"git cherry-pick --skip\".\n"
>
> Inconsistent use of prose in the above and ": run" here.
>
>         You can instead skip this commit with "git cherry-pick --skip"
>
> > +             else if (opts->action == REPLAY_REVERT)
>
> Likewise.
>
> Thanks.

Thanks.
--
ZheNing Hu

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

[ZheNing Hu via GitGitGadget]

From: ZheNing Hu <adlternative@gmail.com>

"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: Christian Couder <christian.couder@gmail.com>
Mentored-by: Hariom Verma <hariom18599@gmail.com>
Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
    [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/pull.1010.v6.git.1629352277151.gitgitgadget@gmail.com/
    
    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: Christian Couder <christian.couder@gmail.com>
     -    Mentored-by Hariom Verma <hariom18599@gmail.com>
     +    Mentored-by: Hariom Verma <hariom18599@gmail.com>
          Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
          Helped-by: Junio C Hamano <gitster@pobox.com>
          Signed-off-by: ZheNing Hu <adlternative@gmail.com>
     @@ 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(-)

diff --git a/sequencer.c b/sequencer.c
index 0bec01cf38e..03c7b3dcadc 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -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()");
 	}
 }
 
diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index 9d100cd1884..4b5b6076733 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh
@@ -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
diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index 014001b8f32..979e843c65a 100755
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -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