[PATCH] cherry-pick: No advice to commit if --no-commit

[PATCH] cherry-pick: No advice to commit if --no-commit

[Phil Hord]

When cherry-pick fails it offers a helpful hint about how to
proceed.  The hint tells the user how to do the cleanup
needed by the conflicted cherry-pick and finish the job after
conflict resolution.  In case of cherry-pick --no-commit, the
hint goes too far. It tells the user to finish up with
'git commit'.  That is not what this git-cherry-pick was
trying to do in the first place.

Restrict the hint in case of --no-commit to avoid giving this
extra advice.

Also, add a test verifying the reduced hint for the --no-commit
version of cherry-pick.

Signed-off-by: Phil Hord <hordp@cisco.com>
---
 sequencer.c                     |   12 +++++++-----
 t/t3507-cherry-pick-conflict.sh |   14 ++++++++++++++
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 5fcbcb8..b7965e8 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -123,7 +123,7 @@ static void write_cherry_pick_head(struct commit *commit, const char *pseudoref)
 	strbuf_release(&buf);
 }
 
-static void print_advice(int show_hint)
+static void print_advice(int show_hint, struct replay_opts *opts)
 {
 	char *msg = getenv("GIT_CHERRY_PICK_HELP");
 
@@ -138,10 +138,12 @@ static void print_advice(int show_hint)
 		return;
 	}
 
-	if (show_hint)
+	if (show_hint) {
 		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'"));
+			 "with 'git add <paths>' or 'git rm <paths>'"));
+		if (!opts->no_commit)
+			advise(_( "and commit the result with 'git commit'"));
+    }
 }
 
 static void write_message(struct strbuf *msgbuf, const char *filename)
@@ -423,7 +425,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 		      : _("could not apply %s... %s"),
 		      find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),
 		      msg.subject);
-		print_advice(res == 1);
+		print_advice(res == 1, opts);
 		rerere(opts->allow_rerere_auto);
 	} else {
 		if (!opts->no_commit)
diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index ee1659c..0c81b3c 100755
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -59,6 +59,20 @@ test_expect_success 'advice from failed cherry-pick' "
 	test_i18ncmp expected actual
 "
 
+test_expect_success 'advice from failed cherry-pick --no-commit' "
+	pristine_detach initial &&
+
+	picked=\$(git rev-parse --short picked) &&
+	cat <<-EOF >expected &&
+	error: could not apply \$picked... picked
+	hint: after resolving the conflicts, mark the corrected paths
+	hint: with 'git add <paths>' or 'git rm <paths>'
+	EOF
+	test_must_fail git cherry-pick --no-commit picked 2>actual &&
+
+	test_i18ncmp expected actual
+"
+
 test_expect_success 'failed cherry-pick sets CHERRY_PICK_HEAD' '
 	pristine_detach initial &&
 	test_must_fail git cherry-pick picked &&
-- 
1.7.9.267.gda172.dirty

Re: [PATCH] cherry-pick: No advice to commit if --no-commit

[Jonathan Nieder]

Hi Phil,

Phil Hord wrote:

> Signed-off-by: Phil Hord <hordp@cisco.com>

Thanks.

> +++ b/sequencer.c
[...]
> @@ -138,10 +138,12 @@ static void print_advice(int show_hint)
>  		return;
>  	}
>  
> -	if (show_hint)
> +	if (show_hint) {
>  		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'"));
> +			 "with 'git add <paths>' or 'git rm <paths>'"));
> +		if (!opts->no_commit)
> +			advise(_( "and commit the result with 'git commit'"));

"cherry-pick --no-commit" was not about to commit, but the user might
have been.  I think the hint is intended to convey that authorship
will be correctly preserved if the user continues with "git commit"
and no special -c option is necessary.

Could you say a little more about the motivation for this patch?  For
example, did the existing message confuse someone, or was it grating
in the context of some particular workflow?

A smaller detail: splitting the message into two like this gives
translators less control over how to phrase the message and where to
wrap lines.  Luckily that is easy to fix with

	if (opts->no_commit)
		advise(...);
	else
		advise(...);

which means more flexibility in phrasing the message with pertinent
advice for each case. ;-)

Ciao,
Jonathan

Re: [PATCH] cherry-pick: No advice to commit if --no-commit

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

>> +	if (show_hint) {
>>  		advise(_("after resolving the conflicts, mark the corrected paths\n"
>> +			 "with 'git add <paths>' or 'git rm <paths>'"));
>> +		if (!opts->no_commit)
>> +			advise(_( "and commit the result with 'git commit'"));
>
> "cherry-pick --no-commit" was not about to commit, but the user might
> have been.  I think the hint is intended to convey that authorship
> will be correctly preserved if the user continues with "git commit"
> and no special -c option is necessary.

Actually, an often used but perhaps neglected use case of "cherry-pick --no-commit"
is to use it as a substitute for

	git show $that_commit | git apply [--index]

to lift the change from $that_commit and incorporate it into your
unrelated (from the point of view of the author of $that_commit) work.
The user would be doing "edit $this_file" and "edit $that_file" before or
after the "show | apply" aka "cherry-pick --no-commit", and it is merely
a type-saver, not having to re-type the change $that_commit introduced
relative to its parent.

So in that context, I can understand the suggestion to commit is at best
annoying ("I am still working on the current change. Why do you talk about
committing?") and at worst alarming and misleading ("Oh, am I to lose some
change if I do not commit right now?").

> A smaller detail: splitting the message into two like this gives
> translators less control over how to phrase the message and where to
> wrap lines.  Luckily that is easy to fix with
>
> 	if (opts->no_commit)
> 		advise(...);
> 	else
> 		advise(...);

Yes, this pattern must be followed in the reroll.

Thanks for a review and comment.

Re: [PATCH] cherry-pick: No advice to commit if --no-commit

[Phil Hord]

Jonathan Nieder <jrnieder@gmail.com> wrote:

>> -	if (show_hint)
>> +	if (show_hint) {
>>  		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'"));
>> +			 "with 'git add <paths>' or 'git rm <paths>'"));
>> +		if (!opts->no_commit)
>> +			advise(_( "and commit the result with 'git commit'"));
> 
> "cherry-pick --no-commit" was not about to commit, but the user might
> have been.  I think the hint is intended to convey that authorship
> will be correctly preserved if the user continues with "git commit"
> and no special -c option is necessary.

If that were the case, the hint would also appear when there is no conflict.

> Could you say a little more about the motivation for this patch?  For
> example, did the existing message confuse someone, or was it grating
> in the context of some particular workflow?

I found it mildly confusing myself.  I cherry-picked a commit with
--no-commit with no intention of committing it.  I was testing how the
changes would build, but I do not need them on my branch yet.  After I
resolved the conflict and tested them, I wanted to make sure there was
no lingering effects, leaving git thinking a CP was still in progress.

  $ git cherry-pick --abort
  error: no cherry-pick or revert in progress

Ok, so the sequencer was smart enough to leave me on my own.  But just
in case, I wondered what the hint was. And I found it was not telling me
how to clean up at all, but instead telling me how to commit. It seemed
incongruous, and I assumed it was only someone's forgetting to consider
the --no-commit case.

It smelled like a bug.  I started to ask about it, but it seemed easier
to just correct it and lower the list noise.

> A smaller detail: splitting the message into two like this gives
> translators less control over how to phrase the message and where to
> wrap lines.  Luckily that is easy to fix with
> 
> 	if (opts->no_commit)
> 		advise(...);
> 	else
> 		advise(...);
>
> which means more flexibility in phrasing the message with pertinent
> advice for each case. ;-)

I did this at first and didn't like it.  I started to ask, but -- you
know, list noise.

I'll fix it in v2.

Thanks,
Phil