Re: [PATCH] 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.

>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
>  * This time with an updated title, a minimal documentation, and an
>    additional test.
>
>  Documentation/gitattributes.txt |  5 ++++-
>  ll-merge.c                      |  9 ++++++++-
>  t/t6406-merge-attr.sh           | 23 +++++++++++++++++++++++
>  3 files changed, 35 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
> index 02a3ec83e4..6deb89a296 100644
> --- 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 07ec16e8e5..ba45aa2f79 100644
> --- 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 5e4e4dd6d9..b50aedbc00 100755
> --- 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
> +'
> +