Re: [PATCH v2 2/2] rebase -i: improve error message when picking merge
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Apr 12, 2024, 13:24 UTC
- 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:
Show 33 quoted lines
> 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.
Show 8 quoted lines
>> 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