Re: [PATCH v2 2/2] rebase -i: improve error message when picking merge

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Apr 9, 2024, 15:04 UTC
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:

Show 9 quoted lines

> "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.

Show 18 quoted lines

>>   	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.

Show 17 quoted lines

> 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