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

[Junio C Hamano <gitster@pobox.com>]

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

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

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

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

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

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