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

[Patrick Steinhardt <ps@pks.im>]

On Tue, Sep 29, 2026 at 02:54:42AM -0400, Jeff King wrote:
> diff --git a/merge-ll.c b/merge-ll.c
> index dfed6411a8..7fab7c5438 100644
> --- a/merge-ll.c
> +++ b/merge-ll.c
> @@ -241,20 +240,10 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
>  	child.use_shell = 1;
>  	strvec_push(&child.args, cmd.buf);
>  	status = run_command(&child);
> -	fd = open(temp[1], O_RDONLY);
> -	if (fd < 0)
> -		goto bad;
> -	if (fstat(fd, &st))
> -		goto close_bad;
> -	result->size = st.st_size;
> -	result->ptr = xmallocz(result->size);

So we do lose the NUL-termination that `xmallocz()` gave us, as
`read_mmfile()` doesn't do that. You reinstate that in the last patch
though, which makes me lean more into the direction of having that last
optional patch. If so though, we may want to reorder it to come first.

> -	if (read_in_full(fd, result->ptr, result->size) != result->size) {
> -		FREE_AND_NULL(result->ptr);
> -		result->size = 0;
> -	}
> - close_bad:
> -	close(fd);
> - bad:
> +
> +	/* We can ignore errors; result is left NULL/0 in that case. */
> +	read_mmfile(result, temp[1]);

One change in behaviour that wasn't called out is that this will now
make us write an error message in case we failed reading the file. That
could be a good change, but that's hard to say.

Patrick