sequencer: be more careful with external merge

If an external merge strategy cannot merge (for example because it
would overwrite an untracked file) it exits with a non-zero exit
code other than 1. This should be treated differently to a merge
with conflicts which is signalled by an exit code of 1 because as
the merge failed we need to reschedule the last pick. The caller
expects us to return -1 in this case. Also reschedule without trying
to merge if the commit message cannot be written as that prevents us
from successfully picking the commit.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/sequencer.c b/sequencer.c
index 32a09b6e87da5b3ad93c8d808a22d8d6fc34575e..e6626c4db4e61c3f2b4a6fd6a23734c0038abc5c 100644 (file)
--- a/sequencer.c
+++ b/sequencer.c
@@ -2453,14 +2453,25 @@ static int do_pick_commit(struct repository *r,
                struct commit_list *common = NULL;
                struct commit_list *remotes = NULL;
 
-               res = write_message(ctx->message.buf, ctx->message.len,
-                                   git_path_merge_msg(r), 0);
+               if (write_message(ctx->message.buf, ctx->message.len,
+                                 git_path_merge_msg(r), 0)) {
+                       res = -1;
+                       goto leave;
+               }
 
                commit_list_insert(base, &common);
                commit_list_insert(next, &remotes);
-               res |= try_merge_command(r, opts->strategy,
-                                        opts->xopts.nr, opts->xopts.v,
+               res = try_merge_command(r, opts->strategy,
+                                       opts->xopts.nr, opts->xopts.v,
                                        common, oid_to_hex(&head), remotes);
+               /*
+                * If the there were conflicts, try_merge_command() returns 1,
+                * any other no-zero return code means that either the merge
+                * command could not be run, or it failed to merge.
+                */
+               if (res && res != 1)
+                       res = -1;
+
                commit_list_free(common);
                commit_list_free(remotes);
        }

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 58b3bb0c271aaebab4381795c9d9dcf8ba3a7593..297b84e60d59ccbb486bc8f3668dc30d8cfe1c71 100755 (executable)
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1251,6 +1251,17 @@ test_expect_success 'interrupted rebase -i with --strategy and -X' '
        test $(cat file1) = Z
 '
 
+test_expect_success 'failing pick with --strategy is rescheduled' '
+       test_when_finished "rm -rf bin; test_might_fail git rebase --abort" &&
+       mkdir bin &&
+       echo exit 2 | write_script bin/git-merge-fail &&
+       git log -1 --format="pick %H # %s" HEAD >expect &&
+       test_must_fail env PATH="$PWD/bin:$PATH" \
+               git rebase --no-ff --strategy fail HEAD^ &&
+       test_cmp expect .git/rebase-merge/git-rebase-todo &&
+       test_cmp expect .git/rebase-merge/done
+'
+
 test_expect_success 'rebase -i error on commits with \ in message' '
        current_head=$(git rev-parse HEAD) &&
        test_when_finished "git rebase --abort; git reset --hard $current_head; rm -f error" &&