From: Phillip Wood <phillip.wood123@gmail.com>
Date: Fri, 12 Apr 2024 13:24:39 GMT
Subject: Re: [PATCH v2 2/2] rebase -i: improve error message when picking merge
Message-ID: <88bc0787-e7ae-49e5-99e8-97f6c55ea8c6@gmail.com>
In-Reply-To: <xmqqpluy1f9e.fsf@gitster.g>

Hi Junio

On 09/04/2024 20:56, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>>>> +		return error(_("'%s' does not accept merge commits, "
>>>> +			       "please use '%s' followed by '%s'"),
>>>> +			     todo_command_info[command].str,
>>>> +			     "merge -C", "break");
>>> OK.  And when hitting the "break", they know that they are supposed
>>> to say "git commit --amend" and then "git rebase --continue"?
>>
>> Yes. I guess we could add a hint to that effect if you think its worth it.
> 
> As I said elsewhere, I do not deal with non-linear stuff using
> sequencer, so I _may_ be missing something obvious to the target
> audience of this message.  But if I were dipping my toes to try
> mucking with sequencer and edit the todo myself by inserting a random
> merge commit there and got this message, I would have probably
> appreciated if the message were a bit more explicit _why_ I would
> want to use the 'break' there.  Otherwise, I probably would be lost
> sitting in front of the shell command prompt.  If it were
> 
> 	'pick' does not take a merge commit.  If you wanted to
> 	replay the merge, use 'merge -C' on the commit, and then
> 	'break' to give the control back to you so that you can
> 	do 'git commit --amend && git rebase --continue'.
> 
> that would have given me 70% of what I needed (the other 30% is why
> I would want to "--amend", instead of just taking the result of
> 'merge -C' as-is, though).
> 
> We can formulate the message in such a way that a succinct first
> part is sufficient for people who know the command, while the latter
> more verbose and prescriptive part can be hidden behind the advice
> mechanism.

I'll re-roll with a short error message plus some advice. I'll have a 
think about adding advice for the other commands as well explaining how 
the "merge" command should be used as well.

>> It feels funny to call error_merge_commit() for a command that we know
>> supports merges but I can see that would make it easier to extend in
>> the future.
> 
> Yes, I think that it is just a sign that the function is misnamed.
> "Gee we have a merge commit, please tell me what you want to do with
> it" is what the caller is asking, and the error messages are side
> effects the caller does not have to care too much about.

Sure

Thanks

Phillip