Re: [PATCH 7/5] merge-ll: report an error when reading external merge results fails

[Jeff King <peff@peff.net>]

On Tue, Sep 29, 2026 at 02:19:26PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > +test_expect_success SANITY 'rerere preserves conflicts when driver output is unreadable' '
> > +	test_create_repo unreadable-output &&
> > +	(
> 
> > +		cd unreadable-output &&
> > +		git config rerere.enabled true &&
> > +		git config rerere.autoupdate true &&
> > +		write_script merge-driver <<-\EOF &&
> > +		git merge-file "$@"
> > +		status=$?
> > +		if test -f fail-read
> > +		then
> > +			chmod 0 "$1" || exit 1
> > +		fi
> 
> Can we lose SANITY by "rm $1" instead of "chmod 0"?

Hmm, I guess we can. I was thinking for some reason that we need to fail
later in read_mmfile(). But I think I was just confusing that with the
earlier leak fix. With a stat failure, read_mmfile() will leave the
mmfile_t untouched, but we initialize it in ll_ext_merge() to NULL/0. So
the outcome should be the same from the caller's perspective.

And that's actually a more realistic example, I think. Instead of
simulating a racy read failure, we are considering a driver that
sometimes accidentally deletes the file while returning 0. Buggy, but a
plausible bug. ;)

Here's a resend of that final patch (not just a squash, because the
commit message mentioned the chmod).

-- >8 --
Subject: merge-ll: report an error when reading external merge results fails

If we can't read an external merge driver's output, ll_ext_merge()
leaves the result buffer as NULL but returns a status based only on the
driver's exit code. So a driver which exits successfully can cause us to
return LL_MERGE_OK without a result.

Most callers of ll_merge() check for a NULL buffer in addition to an
error return, so they're fine. But rerere's merge() checks only the
return value, and may write out the (incorrect) empty result as the
recorded resolution.

Let's return LL_MERGE_ERROR when read_mmfile() fails, regardless of the
driver's exit status, to make it clear that the returned value is not
valid.

Our test is a little funny; the bad case happens when reading back the
file happens to fail. That can happen due to system errors, but of
course we want it to be deterministic. We can make that happen by
removing the result file. But if we configure a driver that always does
that, we'd never record a rerere result in the first place! So we
instead create a driver that "breaks" the read only when we instruct it
to do so.

Signed-off-by: Jeff King <peff@peff.net>
---
 merge-ll.c        |  4 ++--
 t/t4200-rerere.sh | 51 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/merge-ll.c b/merge-ll.c
index 4d82836bc5..3b5327e7df 100644
--- a/merge-ll.c
+++ b/merge-ll.c
@@ -248,8 +248,8 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
 		/* died due to a signal: WTERMSIG(status) + 128 */
 		ret = LL_MERGE_ERROR;
 
-	/* We can ignore errors; result is left NULL/0 in that case. */
-	read_mmfile(result, temp[1]);
+	if (read_mmfile(result, temp[1]) < 0)
+		ret = LL_MERGE_ERROR;
 
 	for (i = 0; i < 3; i++)
 		unlink_or_warn(temp[i]);
diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh
index 7bb601e117..5be3f056f5 100755
--- a/t/t4200-rerere.sh
+++ b/t/t4200-rerere.sh
@@ -734,4 +734,55 @@ test_expect_success 'rerere does not crash with unmatched conflict marker' '
 	test_must_fail git rebase --continue
 '
 
+test_expect_success 'rerere preserves conflicts when driver output is unreadable' '
+	test_create_repo unreadable-output &&
+	(
+		cd unreadable-output &&
+		git config rerere.enabled true &&
+		git config rerere.autoupdate true &&
+		write_script merge-driver <<-\EOF &&
+		git merge-file "$@"
+		status=$?
+		if test -f fail-read
+		then
+			rm "$1" || exit 1
+		fi
+		exit "$status"
+		EOF
+		git config merge.unreadable.driver "./merge-driver %A %O %B" &&
+		echo "file merge=unreadable" >.gitattributes &&
+		test_commit base file base &&
+		git checkout -b one &&
+		test_commit --no-tag one file one &&
+		git checkout -b two base &&
+		test_commit --no-tag two file two &&
+
+		# Teach rerere a resolution while the driver works normally.
+		test_must_fail git merge one &&
+		echo resolved >file &&
+		git rerere &&
+		git merge --abort &&
+
+		# Recreate the conflict without replaying the resolution yet.
+		test_must_fail git -c rerere.enabled=false merge one &&
+
+		# We will expect the same conflicted content after rerere fails
+		# below.
+		cp file expect &&
+		git ls-files -u >expect-index &&
+		test_file_not_empty expect-index &&
+
+		# Now we try rerere again, but the merge driver will cause the
+		# read to fail.
+		>fail-read &&
+		git rerere 2>err &&
+		test_grep "Could not stat" err &&
+
+		# And we expect the conflicted state.
+		test_cmp expect file &&
+		git ls-files -u >actual-index &&
+		test_cmp expect-index actual-index
+	)
+'
+
 test_done
-- 
2.56.0.325.g545d7e68bc