Re: [PATCH v2 1/2] Change how HTTP response body is returned

From: Jeff King <peff@peff.net>
Date: Jan 4, 2019, 10:11 UTC
Message-ID: <20190104101149.GA26185@sigill.intra.peff.net>
In-Reply-To: <xmqqtviph835.fsf@gitster-ct.c.googlers.com>

On Thu, Jan 03, 2019 at 11:09:02AM -0800, Junio C Hamano wrote:

Show 19 quoted lines

> > +	if (dest->file) {
> > +		/*
> > +		 * At this point, the file contains the response body of the
> > +		 * previous request. We need to truncate the file.
> > +		 */
> > +		FILE *new_file = freopen(dest->filename, "w", dest->file);
> 
> Now freopen() lets us restart the file anew with a new "FILE *".
> 
> > +		if (new_file == NULL) {
> > +			error("Unable to open local file %s", dest->filename);
> 
> error_errno(), perhaps?
> 
> At this point, I presume that dest->file is closed by the failed
> freopen(), but dest->file is still non-NULL and causes further calls
> to http_request() with this dest would be a disaster?  As long as
> the caller of this function reacts to HTTP_ERROR and kill the dest,
> it would be fine.

I also wondered what timing guarantees freopen() gives us (i.e., is it possible for it to open and truncate the file, and then close the old handle, flushing some in-memory buffer). C99 says:

  The freopen function first attempts to close any file that is
  associated with the specified stream. Failure to close the file is
  ignored. The error and end-of-file indicators for the stream are
  cleared.

So I think the order is OK for my concern, but not for yours. I.e., on an error, dest->file is now undefined.

It might be nice to set "dest->file == NULL" in that case. There's no guarantee that the caller did not hold onto its own copy of the handle, but since this struct is only exposed internally within http.c, that's probably OK.

The most robust thing would perhaps be:

  fflush(dest->file);
  ftruncate(fileno(dest->file), 0);

which leaves the handle intact.

(I agree with the rest of your review, especially that it would be easier to read if this were split into separate refactor and change-behavior steps).

-Peff