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

From: Jeff King <peff@peff.net>
Date: Sep 29, 2026, 20:11 UTC
Message-ID: <20260929201134.GA1713437@coredump.intra.peff.net>
In-Reply-To: <xmqqzewzg8w0.fsf@gitster.g>

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

Show 36 quoted lines

> > +	/* 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.

Show 11 quoted lines

> 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.

Show 5 quoted lines

> 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".

Show 8 quoted lines

> +	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