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

[Phil Hord <hordp@cisco.com>]

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