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

From: Patrick Steinhardt <ps@pks.im>
Date: Sep 30, 2026, 15:33 UTC
Message-ID: <ar0rrVE0ZxcU7uG-@pks.im>
In-Reply-To: <20260929065442.GD1697497@coredump.intra.peff.net>

On Tue, Sep 29, 2026 at 02:54:42AM -0400, Jeff King wrote:

Show 15 quoted lines

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

Show 10 quoted lines

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