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

From: Phil Hord <hordp@cisco.com>
Date: Feb 22, 2012, 00:18 UTC
Message-ID: <4F443448.7010705@cisco.com>
In-Reply-To: <20120221222049.GA31934@burratino>

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