"Phillip Wood via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
> From: Phillip Wood <phillip.wood@xxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxx>
> Signed-off-by: Phillip Wood <phillip.wood@xxxxxxxxxxxxx>
> ---
> 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.
