rebase -i: improve error message when picking merge

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 and printing some
advice on how to fix it when the todo list is parsed rather than waiting
for the "pick" command to fail. The advice recommends "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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index 0e35ae5240fa523039524b52aaeccd41e62820ac..fa612417568d32918f109638e0a4048750c8f812 100644 (file)
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -96,6 +96,8 @@ advice.*::
                `pushNonFFCurrent`, `pushNonFFMatching`, `pushAlreadyExists`,
                `pushFetchFirst`, `pushNeedsForce`, and `pushRefNeedsUpdate`
                simultaneously.
+       rebaseTodoError::
+               Shown when there is an error after editing the rebase todo list.
        refSyntax::
                Shown when the user provides an illegal ref name, to
                tell the user about the ref syntax documentation.

diff --git a/advice.c b/advice.c
index d19648b7f88b75261f0666da94607a9a052dc079..6546231e9187c20f35e9f82e4372c0e71ab5db76 100644 (file)
--- a/advice.c
+++ b/advice.c
@@ -69,6 +69,7 @@ static struct {
        [ADVICE_PUSH_UNQUALIFIED_REF_NAME]              = { "pushUnqualifiedRefName" },
        [ADVICE_PUSH_UPDATE_REJECTED]                   = { "pushUpdateRejected" },
        [ADVICE_PUSH_UPDATE_REJECTED_ALIAS]             = { "pushNonFastForward" }, /* backwards compatibility */
+       [ADVICE_REBASE_TODO_ERROR]                      = { "rebaseTodoError" },
        [ADVICE_REF_SYNTAX]                             = { "refSyntax" },
        [ADVICE_RESET_NO_REFRESH_WARNING]               = { "resetNoRefresh" },
        [ADVICE_RESOLVE_CONFLICT]                       = { "resolveConflict" },

diff --git a/advice.h b/advice.h
index c8d29f97f39bef84062d541009a27bd35a47f9dd..5105d90129d68b238546d1baaafdc886e78949d1 100644 (file)
--- a/advice.h
+++ b/advice.h
@@ -37,6 +37,7 @@ enum advice_type {
        ADVICE_PUSH_UNQUALIFIED_REF_NAME,
        ADVICE_PUSH_UPDATE_REJECTED,
        ADVICE_PUSH_UPDATE_REJECTED_ALIAS,
+       ADVICE_REBASE_TODO_ERROR,
        ADVICE_REF_SYNTAX,
        ADVICE_RESET_NO_REFRESH_WARNING,
        ADVICE_RESOLVE_CONFLICT,

diff --git a/sequencer.c b/sequencer.c
index a3154ba334718ac6d55e617f6495eed6efeccdd7..0959f335f7bd98c9ff7919fb3c83a2ed4e092f5a 100644 (file)
--- a/sequencer.c
+++ b/sequencer.c
@@ -2573,7 +2573,61 @@ 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 check_merge_commit_insn(enum todo_command command)
+{
+       switch(command) {
+       case TODO_PICK:
+               error(_("'%s' does not accept merge commits"),
+                     todo_command_info[command].str);
+               advise_if_enabled(ADVICE_REBASE_TODO_ERROR, _(
+                       /*
+                        * TRANSLATORS: 'pick' and 'merge -C' should not be
+                        * translated.
+                        */
+                       "'pick' does not take a merge commit. If you wanted to\n"
+                       "replay the merge, use 'merge -C' on the commit."));
+               return -1;
+
+       case TODO_REWORD:
+               error(_("'%s' does not accept merge commits"),
+                     todo_command_info[command].str);
+               advise_if_enabled(ADVICE_REBASE_TODO_ERROR, _(
+                       /*
+                        * TRANSLATORS: 'reword' and 'merge -c' should not be
+                        * translated.
+                        */
+                       "'reword' does not take a merge commit. If you wanted to\n"
+                       "replay the merge and reword the commit message, use\n"
+                       "'merge -c' on the commit"));
+               return -1;
+
+       case TODO_EDIT:
+               error(_("'%s' does not accept merge commits"),
+                     todo_command_info[command].str);
+               advise_if_enabled(ADVICE_REBASE_TODO_ERROR, _(
+                       /*
+                        * TRANSLATORS: 'edit', 'merge -C' and 'break' should
+                        * not be translated.
+                        */
+                       "'edit' does not take a merge commit. If you wanted to\n"
+                       "replay the merge, use 'merge -C' on the commit, and then\n"
+                       "'break' to give the control back to you so that you can\n"
+                       "do 'git commit --amend && git rebase --continue'."));
+               return -1;
+
+       case TODO_FIXUP:
+       case TODO_SQUASH:
+               return error(_("cannot squash merge commit into another commit"));
+
+       case TODO_MERGE:
+               return 0;
+
+       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 +2733,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->commit->parents && item->commit->parents->next)
+               return check_merge_commit_insn(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 d1bead61fad03d8a847d75062ede8ad852f7d8e6..f92baad1381e6619a311e304865529f3c6dc46e7 100755 (executable)
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -2215,6 +2215,51 @@ 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
+       hint: ${SQ}pick${SQ} does not take a merge commit. If you wanted to
+       hint: replay the merge, use ${SQ}merge -C${SQ} on the commit.
+       hint: Disable this message with "git config advice.rebaseTodoError false"
+       error: invalid line 1: pick $oid
+       error: ${SQ}reword${SQ} does not accept merge commits
+       hint: ${SQ}reword${SQ} does not take a merge commit. If you wanted to
+       hint: replay the merge and reword the commit message, use
+       hint: ${SQ}merge -c${SQ} on the commit
+       hint: Disable this message with "git config advice.rebaseTodoError false"
+       error: invalid line 2: reword $oid
+       error: ${SQ}edit${SQ} does not accept merge commits
+       hint: ${SQ}edit${SQ} does not take a merge commit. If you wanted to
+       hint: replay the merge, use ${SQ}merge -C${SQ} on the commit, and then
+       hint: ${SQ}break${SQ} to give the control back to you so that you can
+       hint: do ${SQ}git commit --amend && git rebase --continue${SQ}.
+       hint: Disable this message with "git config advice.rebaseTodoError false"
+       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