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

["Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com>]

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.

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

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(-)

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");
+
+	case TODO_FIXUP:
+	case TODO_SQUASH:
+		return error(_("cannot squash merge commit into another commit"));
+
+	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);
+	return 0;
 }
 
 int sequencer_get_last_command(struct repository *r UNUSED, enum replay_action *action)
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index d1bead61fad..8565fd4b5ae 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -2215,6 +2215,39 @@ test_expect_success 'bad labels and refs rejected when parsing todo list' '
 	test_path_is_missing execed
 '
 
+test_expect_success 'non-merge commands reject merge commits' '
+	test_when_finished "test_might_fail git rebase --abort" &&
+	git checkout E &&
+	git merge I &&
+	oid=$(git rev-parse HEAD) &&
+	cat >todo <<-EOF &&
+	pick $oid
+	reword $oid
+	edit $oid
+	fixup $oid
+	squash $oid
+	EOF
+	(
+		set_replace_editor todo &&
+		test_must_fail git rebase -i HEAD 2>actual
+	) &&
+	cat >expect <<-EOF &&
+	error: ${SQ}pick${SQ} does not accept merge commits, please use ${SQ}merge -C${SQ}
+	error: invalid line 1: pick $oid
+	error: ${SQ}reword${SQ} does not accept merge commits, please use ${SQ}merge -c${SQ}
+	error: invalid line 2: reword $oid
+	error: ${SQ}edit${SQ} does not accept merge commits, please use ${SQ}merge -C${SQ} followed by ${SQ}break${SQ}
+	error: invalid line 3: edit $oid
+	error: cannot squash merge commit into another commit
+	error: invalid line 4: fixup $oid
+	error: cannot squash merge commit into another commit
+	error: invalid line 5: squash $oid
+	You can fix this with ${SQ}git rebase --edit-todo${SQ} and then run ${SQ}git rebase --continue${SQ}.
+	Or you can abort the rebase with ${SQ}git rebase --abort${SQ}.
+	EOF
+	test_cmp expect actual
+'
+
 # This must be the last test in this file
 test_expect_success '$EDITOR and friends are unchanged' '
 	test_editor_unchanged
-- 
gitgitgadget