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

[Junio C Hamano <gitster@pobox.com>]

"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