From: Jeff King <peff@peff.net>
Date: Wed, 30 Sep 2026 22:50:11 GMT
Subject: Re: [PATCH 4/5] merge-ll: use read_mmfile() to read external merge results
Message-ID: <20260930225011.GC765052@coredump.intra.peff.net>
In-Reply-To: <ar0rrVE0ZxcU7uG-@pks.im>

On Wed, Sep 30, 2026 at 05:33:01PM +0200, Patrick Steinhardt wrote:

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

Yeah, I'll do that re-order.

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

True, I hadn't even thought about that. It seems like a strict
improvement to me, but I'll mention it in the commit message.

-Peff