Re: Design issue in git merge driver interface

Joshua Hudson <jhudson@cedaron.com> writes:

Even though the end-user facing documentation says:

    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.

the ll-merge.c:ll_ext_merge() function that calls an external merge driver does this:

        static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
                ...
                status = run_command(&child);
                ...
                ret = (status > 0) ? LL_MERGE_CONFLICT : status;
                return ret;
        }

so a true "failure" from run_command() to run the external merge driver will be noticed as a failure by the upper layer of the callchain. merge-ort.c:merge_3way() relays the return value of ll-merge.c:ll_merge() and merge-ort.c:handle_content_merge() reacts to a negative return as an _("Failed to execute internal merge") error, for example. merge-recursive uses the same logic.

Unfortunately, I see no provision for the merge driver to actively signal such a condition. The return value of run_command() is a return value from run-command.c:wait_or_whine() and exit status of the process is cleansed with WEXITSTATUS() so we cannot make it negative X-<.

In the worst case, we may retroactively have to reserve one exit status so that the external merge driver can actively say "I give up" to cause LL_MERGE_ERROR to be returned from the codepath, but I wonder if it is safe to abuse "exit due to signal" (which shows up as a return value greater than 128) as such a "merge driver went away without leaving a useful result"? Elijah, what do you think?

Stepping back a bit and even disregarding such a merge driver that OOMs, if a long-running merge driver is killed, by definition we cannot trust what the driver left on the filesystem, so handling "exit due to signal" case differently does sound like a sensible thing to do, at least to me, offhand.

And once we have such an enhancement to the ll-ext-merge interface, a merge driver that voluntarily "gives up" can send a signal to kill itself (or call abort(3)).

With a tentative commit log message (which would need to be updated to mention what the triggering topic was that led to this enhancement) but without associated documentation update and test, here is to summarize and illustrate the above idea.

----- >8 ---------- >8 ---------- >8 ----- ll-merge: external merge driver died with a signal causes an error

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>
---
 ll-merge.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)