rebase -i: fix SIGSEGV when 'merge <branch>' fails

If a merge command in the todo list specifies just a branch to merge
with no -C/-c argument then item->commit is NULL. This means that if
there are merge conflicts error_with_patch() is passed a NULL commit
which causes a segmentation fault when make_patch() tries to look it up.

This commit implements a minimal fix which fixes the crash and allows
the user to successfully commit a conflict resolution with 'git rebase
--continue'. It does not write .git/rebase-merge/patch,
.git/rebase-merge/stopped-sha or update REBASE_HEAD. To sensibly get the
hashes of the merge parents would require refactoring do_merge() to
extract the code that parses the merge parents into a separate function
which error_with_patch() could then use to write the parents into the
stopped-sha file. To create meaningful output make_patch() and 'git
rebase --show-current-patch' would also need to be modified to diff the
merge parent and merge base in this case.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/sequencer.c b/sequencer.c
index 4034c0461b5022dad01b25d824cdc4f47ee09d13..df49199175e8b8d47c19fcd4f211ebe0f26b2de1 100644 (file)
--- a/sequencer.c
+++ b/sequencer.c
@@ -2590,8 +2590,12 @@ static int error_with_patch(struct commit *commit,
        const char *subject, int subject_len,
        struct replay_opts *opts, int exit_code, int to_amend)
 {
        const char *subject, int subject_len,
        struct replay_opts *opts, int exit_code, int to_amend)
 {

-       if (make_patch(commit, opts))
-               return -1;
+       if (commit) {
+               if (make_patch(commit, opts))
+                       return -1;
+       } else if (copy_file(rebase_path_message(), git_path_merge_msg(), 0666))
+               return error(_("unable to copy '%s' to '%s'"),
+                            git_path_merge_msg(), rebase_path_message());

 
        if (to_amend) {
                if (intend_to_amend())
 
        if (to_amend) {
                if (intend_to_amend())


@@ -2604,9 +2608,19 @@ static int error_with_patch(struct commit *commit,
                        "Once you are satisfied with your changes, run\n"
                        "\n"
                        "  git rebase --continue\n", gpg_sign_opt_quoted(opts));
                        "Once you are satisfied with your changes, run\n"
                        "\n"
                        "  git rebase --continue\n", gpg_sign_opt_quoted(opts));

-       } else if (exit_code)
-               fprintf(stderr, "Could not apply %s... %.*s\n",
-                       short_commit_name(commit), subject_len, subject);
+       } else if (exit_code) {
+               if (commit)
+                       fprintf(stderr, "Could not apply %s... %.*s\n",
+                               short_commit_name(commit),
+                               subject_len, subject);
+               else
+                       /*
+                        * We don't have the hash of the parent so
+                        * just print the line from the todo file.
+                        */
+                       fprintf(stderr, "Could not merge %.*s\n",
+                               subject_len, subject);
+       }

 
        return exit_code;
 }
 
        return exit_code;
 }

diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 31fe4268d53084f840dad6e6558cea762d176696..2e767d4f1ec97362b47dec41497d531744f0f219 100755 (executable)
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -129,7 +129,7 @@ test_expect_success '`reset` refuses to overwrite untracked files' '
        git rebase --abort
 '
 
        git rebase --abort
 '
 

-test_expect_success 'failed `merge` writes patch (may be rescheduled, too)' '
+test_expect_success 'failed `merge -C` writes patch (may be rescheduled, too)' '

        test_when_finished "test_might_fail git rebase --abort" &&
        git checkout -b conflicting-merge A &&
 
        test_when_finished "test_might_fail git rebase --abort" &&
        git checkout -b conflicting-merge A &&
 


@@ -151,6 +151,19 @@ test_expect_success 'failed `merge` writes patch (may be rescheduled, too)' '
        test_path_is_file .git/rebase-merge/patch
 '
 
        test_path_is_file .git/rebase-merge/patch
 '
 

+SQ="'"
+test_expect_success 'failed `merge <branch>` does not crash' '
+       test_when_finished "test_might_fail git rebase --abort" &&
+       git checkout conflicting-G &&
+
+       echo "merge G" >script-from-scratch &&
+       test_config sequence.editor \""$PWD"/replace-editor.sh\" &&
+       test_tick &&
+       test_must_fail git rebase -ir HEAD &&
+       ! grep "^merge G$" .git/rebase-merge/git-rebase-todo &&
+       grep "^Merge branch ${SQ}G${SQ}$" .git/rebase-merge/message
+'
+

 test_expect_success 'with a branch tip that was cherry-picked already' '
        git checkout -b already-upstream master &&
        base="$(git rev-parse --verify HEAD)" &&
 test_expect_success 'with a branch tip that was cherry-picked already' '
        git checkout -b already-upstream master &&
        base="$(git rev-parse --verify HEAD)" &&