From: Jeff King <peff@peff.net>
Date: Tue, 14 Jul 2026 05:44:39 GMT
Subject: Re: [PATCH 1/2] http: use unique tempfiles for packfile URI downloads
Message-ID: <20260714054439.GB2516582@coredump.intra.peff.net>
In-Reply-To: <alW1tAnMtOznxrhK@com-79390>

On Mon, Jul 13, 2026 at 09:06:12PM -0700, Taylor Blau wrote:

> >  void release_http_pack_request(struct http_pack_request *preq)
> >  {
> > -	if (preq->packfile) {
> > +	if (preq->tempfile) {
> > +		delete_tempfile(&preq->tempfile);
> > +		preq->packfile = NULL;
> 
> We should be able to drop the assignment to NULL on the second line,
> since `delete_tempfile()` takes a double pointer to the 'struct
> packfile' and NULL's it out for us.
> 
> (The other callers appear to avoid explicitly setting `preq->tempfile`
> to NULL.)

It takes a double-pointer to the "struct tempfile"; the NULL assignment
is to the "packfile" member, which is the FILE handle.

I thought at first this was buggy; we still call fdopen() on the
tempfile and assign the result to preq->packfile, even in the new
non-resumable case. Don't we need to fclose() it? But the answer is no:
fdopen_tempfile() retains ownership of the result, storing it in
tempfile.fp. So it will be correctly closed during delete_tempfile(),
and in fact we must _not_ fclose it again.

But assigning NULL can happen with either style. So doing it
unconditionally like:

  if (preq->tempfile)
	delete_tempfile(&preq->tempfile);
  else if (preq->packfile)
	fclose(preq->packfile);
  preq->packfile = NULL;

makes more sense, as it is done in finish_http_pack_request(). It might
even make sense to add a comment explaining why we don't need to
fclose() in the first part of the conditional.

All that said, I do not think setting it to NULL matters at all here,
since the function ends with free(preq). So just dropping the NULL would
perhaps be more clear.

-Peff