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>
This commit is contained in:
Junio C Hamano 2023-06-22 17:33:01 -07:00
parent 6640c2d06d
commit 2b7b788fb3
3 changed files with 35 additions and 2 deletions

View File

@ -1132,7 +1132,10 @@ size (see below).
The merge driver is expected to leave the result of the merge in 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 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 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 The `merge.*.recursive` variable specifies what other merge
driver to use when the merge driver is called for an internal driver to use when the merge driver is called for an internal

View File

@ -243,7 +243,14 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
unlink_or_warn(temp[i]); unlink_or_warn(temp[i]);
strbuf_release(&cmd); strbuf_release(&cmd);
strbuf_release(&path_sq); 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; return ret;
} }

View File

@ -56,6 +56,12 @@ test_expect_success setup '
) >"$ours+" ) >"$ours+"
cat "$ours+" >"$ours" cat "$ours+" >"$ours"
rm -f "$ours+" rm -f "$ours+"
if test -f ./please-abort
then
echo >>./please-abort killing myself
kill -9 $$
fi
exit "$exit" exit "$exit"
EOF EOF
chmod +x ./custom-merge chmod +x ./custom-merge
@ -162,6 +168,23 @@ test_expect_success 'custom merge backend' '
rm -f $o $a $b 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' ' test_expect_success 'up-to-date merge without common ancestor' '
git init repo1 && git init repo1 &&
git init repo2 && git init repo2 &&