From: Jeff King <peff@peff.net>
Date: Mon, 14 Sep 2026 16:56:54 GMT
Subject: Re: [PATCH v2 2/3] merge-ll: catch close() errors when writing external tempfiles
Message-ID: <20260914165654.GB32247@peff.net>
In-Reply-To: <CABPp-BG6wYkr4wjr-iqak9fYo4+49WvjROdZ_MK5=g27WcUmMA@mail.gmail.com>

On Fri, Sep 11, 2026 at 11:06:43AM -0700, Elijah Newren wrote:

> > -       if (write_in_full(fd, src->ptr, src->size) < 0)
> > +       if (write_in_full(fd, src->ptr, src->size) < 0 ||
> > +           close(fd) < 0)
> >                 die_errno("unable to write temp-file");
> > -       close(fd);
> >  }
> 
> I got tripped up at first on this patch; if write_in_full() < 0, then
> we won't explicitly close(), but since die will result in an implicit
> close, that's not a problem.
> 
> Instead, the only thing that changes is we also die if close() fails.

Yeah, this is a subtle mistake that we've had to fix before. Doing:

  if (write_in_full(fd, ...) || close(fd))
	return error(...);

is a hard-to-spot leak. It's not present here because we're calling
die() instead of returning, but maybe it is worth writing it out to set
a good example, like:

  if (write_in_full(...))
	die_errno("unable to write");
  if (close(...))
	die_errno("unable to close");

Since I'm re-rolling anyway.

-Peff