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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 8, 2024, 22:29 UTC
Message-ID: <xmqqle5n7ajt.fsf@gitster.g>
In-Reply-To: <fbc6746e0188ed7b69c238935ec85b69112ddd79.1712585788.git.gitgitgadget@gmail.com>

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 19 quoted lines

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> The only todo commands that accept a merge commit are "merge" and
> "reset". All the other commands like "pick" or "reword" fail when they
> try to pick a a merge commit and print the message
>
>     error: commit abc123 is a merge but no -m option was given.
>
> followed by a hint about the command being rescheduled. This message is
> designed to help the user when they cherry-pick a merge and forget to
> pass "-m". For users who are rebasing the message is confusing as there
> is no way for rebase to cherry-pick the merge.
>
> Improve the user experience by detecting the error when the todo list is
> parsed rather than waiting for the "pick" command to fail and print a
> message recommending the "merge" command instead. We recommend "merge"
> rather than "exec git cherry-pick -m ..." on the assumption that
> cherry-picking merges is relatively rare and it is more likely that the
> user chose "pick" by a mistake.

Now, the mention of "all the other commands" makes me curious what should happen when your "squash" and "fixup" named a merge commit. I think it should just error out without any recourse, but it is more than likely that I am missing some use cases where it is useful to "squash" or "fixup" a merge commit on top of an existing commit?

Show 5 quoted lines

> It would be possible to support cherry-picking merges by allowing the
> user to pass "-m" to "pick" commands but that adds complexity to do
> something that can already be achieved with
>
>     exec git cherry-pick -m1 abc123

I have no strong opinions between this and "merge" for "pick", "edit", and "reword".

Show 6 quoted lines

> Reported-by: Stefan Haller <lists@haller-berlin.de>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  sequencer.c                   | 37 +++++++++++++++++++++++++++++++++--
>  t/t3404-rebase-interactive.sh | 33 +++++++++++++++++++++++++++++++
>  2 files changed, 68 insertions(+), 2 deletions(-)

So, having thought about my version of a solution from the problem description above without looking at your answers, let's see how you solved it.

Show 27 quoted lines

> diff --git a/sequencer.c b/sequencer.c
> index a3154ba3347..4012c6f88d9 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2573,7 +2573,35 @@ static int check_label_or_ref_arg(enum todo_command command, const char *arg)
>  	return 0;
>  }
>  
> -static int parse_insn_line(struct repository *r, struct replay_opts *opts UNUSED,
> +static int error_merge_commit(enum todo_command command)
> +{
> +	switch(command) {
> +	case TODO_PICK:
> +		return error(_("'%s' does not accept merge commits, "
> +			       "please use '%s'"),
> +			     todo_command_info[command].str, "merge -C");
> +
> +	case TODO_REWORD:
> +		return error(_("'%s' does not accept merge commits, "
> +			       "please use '%s'"),
> +			     todo_command_info[command].str, "merge -c");
> +
> +	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"?

> +	case TODO_FIXUP:
> +	case TODO_SQUASH:
> +		return error(_("cannot squash merge commit into another commit"));

OK, this is as I expected.

Show 19 quoted lines

> +	default:
> +		BUG("unexpected todo_command");
> +	}
> +}
> +
> +static int parse_insn_line(struct repository *r, struct replay_opts *opts,
>  			   struct todo_item *item, const char *buf,
>  			   const char *bol, char *eol)
>  {
> @@ -2679,7 +2707,12 @@ static int parse_insn_line(struct repository *r, struct replay_opts *opts UNUSED
>  		return status;
>  
>  	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().

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.