From: Phillip Wood <phillip.wood123@gmail.com>
Date: Tue, 09 Apr 2024 15:04:33 GMT
Subject: Re: [PATCH v2 2/2] rebase -i: improve error message when picking merge
Message-ID: <1b74f6f9-f4b9-4909-82b3-26f19b7a1347@gmail.com>
In-Reply-To: <xmqqle5n7ajt.fsf@gitster.g>

Hi Junio

On 08/04/2024 23:29, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
>> +	case TODO_EDIT:
>> +		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.

>>   	item->commit = lookup_commit_reference(r, &commit_oid);
>> -	return item->commit ? 0 : -1;
>> +	if (!item->commit)
>> +		return -1;
>> +	if (is_rebase_i(opts) && item->command != TODO_MERGE &&
>> +	    item->commit->parents && item->commit->parents->next)
>> +		return error_merge_commit(item->command);
> 
> This is good for now, but we may see command other than TODO_MERGE
> learn how to handle a merge commit, and when that happens, I wonder
> what we want to do here.  One thought is to do this:
> 
> 	if (is_rebase_i(opts) && is_merge_commit(item->commit))
>          	return error_merge_commit(item);
> 
> and teach error_merge_commit() to silently return 0 on TODO_MERGE.
> Other commands, when they learn how to deal with a merge commit,
> will then update their entries in error_merge_commit().
f
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.

> Would we want to customize the message from error_merge_commit() to
> make it closer to cut-and-paste ready?  For that, something like
> 
> 	int error_merge_commit(struct todo_item *item)
> 	{
> 		switch (item->command) {
> 		case TODO_PICK:
> 			return error(_("'%s'" is bad, plase use "
> 				       '%s %s'"),
> 				todo_command_info[item->command].str,
> 				"merge -C",
> 				oid_to_hex(item->commit->oid));
> 		...
> 		}
> 	}
> 
> might go in a more friendly way.

If we want something the user can cut and paste then we would need to 
suggest a second parent for the merge. We cannot just use the existing 
parent as it may be rebased which means tracking all the commits we see 
while parsing the list. If the second parent is rebased we need to make 
sure the user labels it so we can use that label here. For that reason 
(and the fact that I don't think can we really tell what the user was 
hoping to achieve) I think the best we can do is point the user in the 
direction of the "merge" command even though that leaves them to figure 
out what to do with that command.

Best Wishes

Phillip