Re: [PATCH 4/5] merge-ll: use read_mmfile() to read external merge results

[Jeff King <peff@peff.net>]

On Tue, Sep 29, 2026 at 12:22:39PM -0700, Junio C Hamano wrote:

> > +	/* We can ignore errors; result is left NULL/0 in that case. */
> > +	read_mmfile(result, temp[1]);
> > +
> >  	for (i = 0; i < 3; i++)
> >  		unlink_or_warn(temp[i]);
> >  	strbuf_release(&cmd);
> 
> Lets see if I understand why we can safely ignore errors.
> 
> If the external driver claims that it successfully merged (i.e.,
> status = run_command(&child) returns 0), and yet read_mmfile() fails
> (e.g., perhaps the driver unlinks "%A"), read_mmfile() will leave
> result->ptr and result->size as initialized, and we return
> LL_MERGE_OK from this function.  The result is eventually relayed to
> the caller of ll_merge(), like merge-ort.c:merge_3way(), or
> apply.c:three_way_merge().  Both have something like
> 
> 	status = ll_merge(&result, path,
> 			  &base_file, "base",
> 			  &our_file, "ours",
> 			  &their_file, "theirs",
> 			  state->repo->index,
> 			  &merge_opts);
> 	if (status == LL_MERGE_BINARY_CONFLICT)
> 		warning("Cannot merge binary files: %s (%s vs. %s)",
> 			path, "ours", "theirs");
> 	free(base_file.ptr);
> 	free(our_file.ptr);
> 	free(their_file.ptr);
> 	if (status < 0 || !result.ptr) {
> 		free(result.ptr);
> 		return -1;
> 	}
> 
> to treat that result.ptr==NULL is just as bad as any error from
> ll_merge() (i.e., status < 0).

Yeah, exactly. This confused me quite a bit at first, and I thought I'd
found another bug. It feels like we should return LL_MERGE_ERROR for
this case (it is not the external merge driver's error, but rather ours,
but from the caller's perspective does it matter?).

But then I saw that the callers did check for NULL already (which is
what the existing code reliably returned on error). So there's no bug,
but I agree it's subtle. For the purposes of this refactor I tried to
draw the line at retaining the same visible behavior from
ll_ext_merge(), just to keep scope creep to a minimum.

But I'm definitely not opposed to refactoring further on top, and I
think you may have actually found a bug below.

> merge-blobs.c:merge_blobs() does not check the !result.ptr
> condition, and its sole caller builtin/merge-tree.c:result() passes
> the NULL to show_diff(), which uses a <NULL, 0> mmfile_t as one side
> of xdi_diff(), which the callee is prepared to handle, so this is OK.
> 
> rerere.c:try_merge() does not check the !result.ptr condition, and
> its caller rerere.c:merge() ends up calling
> 
> 	fwrite(NULL, (size_t)0, 1, f)
> 
> which may happen to work on most systems, but is not exactly kosher.

Even if it works and sends an empty output, I think it is the wrong
behavior. It's possible the driver actually returned a real output, but
we failed to read it in. And now we're propagating a bogus empty value
instead.

It's hard to test, though, because the easiest way to trigger a read
failure is for the driver to actually _not_ return an output (i.e., to
delete the %A file). And in that case it happens to coincide with the
correct behavior. ;)

I guess a more interesting one is one where the driver changes the mode
on %A so that it cannot be read.

We can trigger that case like this:

-- >8 --
git init

echo base >file
git add file
git commit -am base

git checkout -b one
echo one >file
git commit -am one

git checkout -b two HEAD^
echo two >file
git commit -am two

git config merge.foo.driver 'echo result >%A; chmod 0 %A'
echo 'file merge=foo' >.gitattributes

git merge one
-- 8< --

But I'm not sure how to convince rerere to work on it. The merge command
produces output like:

  error: Could not open /home/peff/tmp/repo/.merge_file_ma1Kcq: Permission denied
  error: failed to execute internal merge for file
  Merge with strategy ort failed.

which is reasonable (probably mentioning the external driver would be
better still, but at least we notice the problem).

I guess to confuse rerere we probably have to do a regular merge, record
the result, and then configure our broken driver, and then try to merge
to run rerere on the result.

So if we amend the end of that script to:

-- >8 --
# merge that records resolution (we abort here, but it
# could just be that we create the same merge elsewhere)
git -c rerere.enabled=true merge one
echo result >file
git rerere
git reset --hard

# now we merge in a way that creates the conflict again
git -c rerere.enabled=false merge one

# but then in the middle we start using the broken driver
git config merge.foo.driver 'echo result >%A; chmod 0 %A'
echo 'file merge=foo' >.gitattributes

# and now rerere gets confused; we claim to use the recorded
# resolution, but it's incorrectly empty
git rerere
-- 8< --

That sequence is quite fishy (changing the attributes mid-merge!?) but
in theory it could trigger racily due to a system error, fread()
failing, and so on.

> Perhaps something like this on top might make it safer?  Not even
> compile tested and I haven't thought through the ramifications to
> rerere.c:merge() code path, that used to take such a bogus merge
> result as successful merge and relied on the fwrite(NULL) becoming
> a no-op to produce an empty file.

This does fix the case above (modulo some s/./->/ in your patch). We end
up with the unresolved contents in "file".

> +	if (!result_buf.ptr && result == LL_MERGE_OK) {
> +		/*
> +		 * Forbid the driver from giving bogus result and claim
> +		 * that the merge succeeded.
> +		 */
> +		result = LL_MERGE_ERROR;
> +		result_buf.size = 0;
> +	}

I had imagined just fixing this in ll_ext_merge(), like:

diff --git a/merge-ll.c b/merge-ll.c
index 7fab7c5438..0e56e303fa 100644
--- a/merge-ll.c
+++ b/merge-ll.c
@@ -241,8 +241,13 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
 	strvec_push(&child.args, cmd.buf);
 	status = run_command(&child);
 
-	/* We can ignore errors; result is left NULL/0 in that case. */
-	read_mmfile(result, temp[1]);
+	/*
+	 * fake a driver error when we can't read the result; a slightly more
+	 * elegant solution is to hoist the status-to-ret conversion from
+	 * below, and then we can directly assign ret = LL_MERGE_ERROR.
+	 */
+	if (read_mmfile(result, temp[1]) < 0)
+		status = 129;
 
 	for (i = 0; i < 3; i++)
 		unlink_or_warn(temp[i]);

which reduces the weirdness coming out of that function. But it wouldn't
help with other drivers (which may or may not have similar problems? I'd
guess not, since they are all operating internally).

-Peff