ll-merge: killing the external merge driver aborts the merge

When an external merge driver dies with a signal, we should not
expect that the result left on the filesystem is in any useful
state.  However, because the current code uses the return value from
run_command() and declares any positive value as a sign that the
driver successfully left conflicts in the result, and because the
return value from run_command() for a subprocess that died upon a
signal is positive, we end up treating whatever garbage left on the
filesystem as the result the merge driver wanted to leave us.

run_command() returns larger than 128 (WTERMSIG(status) + 128, to be
exact) when it notices that the subprocess died with a signal, so
detect such a case and return LL_MERGE_ERROR from ll_ext_merge().

Signed-off-by: Junio C Hamano <gitster@pobox.com>
Reviewed-by: Elijah Newren <newren@gmail.com>

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 02a3ec83e4d8583ad9c69f9d76ae5dc4d58a2944..6deb89a2967708dfc2c560ac72c29907a5b6334a 100644 (file)
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -1132,7 +1132,10 @@ size (see below).
 The merge driver is expected to leave the result of the merge in
 the file named with `%A` by overwriting it, and exit with zero
 status if it managed to merge them cleanly, or non-zero if there
-were conflicts.
+were conflicts.  When the driver crashes (e.g. killed by SEGV),
+it is expected to exit with non-zero status that are higher than
+128, and in such a case, the merge results in a failure (which is
+different from producing a conflict).
 
 The `merge.*.recursive` variable specifies what other merge
 driver to use when the merge driver is called for an internal

diff --git a/ll-merge.c b/ll-merge.c
index 07ec16e8e5bb59f091a1c41df123abce3cefe337..ba45aa2f7908a3313639354960198de695105878 100644 (file)
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -243,7 +243,14 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
                unlink_or_warn(temp[i]);
        strbuf_release(&cmd);
        strbuf_release(&path_sq);
-       ret = (status > 0) ? LL_MERGE_CONFLICT : status;
+
+       if (!status)
+               ret = LL_MERGE_OK;
+       else if (status <= 128)
+               ret = LL_MERGE_CONFLICT;
+       else
+               /* died due to a signal: WTERMSIG(status) + 128 */
+               ret = LL_MERGE_ERROR;
        return ret;
 }
 

diff --git a/t/t6406-merge-attr.sh b/t/t6406-merge-attr.sh
index 5e4e4dd6d9e6b967f8c1d94fe7ce1a79c787a177..b50aedbc007a402bdd92f02ff1f42f805718fd46 100755 (executable)
--- a/t/t6406-merge-attr.sh
+++ b/t/t6406-merge-attr.sh
@@ -56,6 +56,12 @@ test_expect_success setup '
        ) >"$ours+"
        cat "$ours+" >"$ours"
        rm -f "$ours+"
+
+       if test -f ./please-abort
+       then
+               echo >>./please-abort killing myself
+               kill -9 $$
+       fi
        exit "$exit"
        EOF
        chmod +x ./custom-merge
@@ -162,6 +168,23 @@ test_expect_success 'custom merge backend' '
        rm -f $o $a $b
 '
 
+test_expect_success 'custom merge driver that is killed with a signal' '
+       test_when_finished "rm -f output please-abort" &&
+
+       git reset --hard anchor &&
+       git config --replace-all \
+       merge.custom.driver "./custom-merge %O %A %B 0 %P" &&
+       git config --replace-all \
+       merge.custom.name "custom merge driver for testing" &&
+
+       >./please-abort &&
+       echo "* merge=custom" >.gitattributes &&
+       test_must_fail git merge main &&
+       git ls-files -u >output &&
+       git diff --name-only HEAD >>output &&
+       test_must_be_empty output
+'
+
 test_expect_success 'up-to-date merge without common ancestor' '
        git init repo1 &&
        git init repo2 &&