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

[Phillip Wood <phillip.wood123@gmail.com>]

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
>