threads / patch / 29685

patchcherry-pick: No advice to commit if --no-commit

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

## tl;dr

4 messages between Feb 21, 2012 and Feb 22, 2012. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

Phil Hord· Feb 21, 2012, 21:05 UTC · lore

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(-)
Show changes to 2 files +21 −5

sequencer.c, t/t3507-cherry-pick-conflict.sh

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
Jonathan Nieder· Feb 21, 2012, 22:23 UTC · re: Phil Hord · lore

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

Hi Phil,
Phil Hord wrote:
> Signed-off-by: Phil Hord <hordp@cisco.com>
Thanks.
> +++ b/sequencer.c
[...]
Show 12 quoted lines
> @@ -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

Junio C Hamano· Feb 21, 2012, 22:31 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 10 quoted lines
>> +	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?").

Show 8 quoted lines
> 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.
Phil Hord· Feb 22, 2012, 00:18 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 13 quoted lines
>> -	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.

Show 11 quoted lines
> 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

← back to recent threads