Re: Confusing git messages when disk is full.

From: Jeff King <peff@peff.net>
Date: Feb 15, 2017, 22:32 UTC
Message-ID: <20170215223246.mkaz22yrovnscnne@sigill.intra.peff.net>
In-Reply-To: <xmqq7f4r2io5.fsf@gitster.mtv.corp.google.com>

On Wed, Feb 15, 2017 at 02:28:10PM -0800, Junio C Hamano wrote:

Show 23 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> >>   abort:
> >>  	strbuf_release(&note);
> >>  	free(url);
> >> -	fclose(fp);
> >> +	if (ferror(fp))
> >> +		rc = -1;
> >> +	if (fclose(fp))
> >> +		rc = -1;
> >>  	return rc;
> >
> > Yeah, I think this works. Normally you'd want to flush before checking
> > ferror(), but since you detect errors from fclose, too, it should be
> > fine.
> >
> > We probably should write something stderr, though. Maybe:
> >
> >   if (ferror(fp) || fclose(fp))
> > 	rc = error_errno("unable to write to %s", filename);
> 
> Yes, and somehow make sure we do fclose(fp) even when we have an
> error already ;-)

Good catch. I think we use a nasty bitwise-OR elsewhere to do that. Ah, here it is, in tempfile.c:

                /*
                 * Note: no short-circuiting here; we want to fclose()
                 * in any case!
                 */
                err = ferror(fp) | fclose(fp);

That works, but the fact that we need a comment is a good sign that it's kind of gross. It's too bad stdio does not specify the return of fclose to report an error in the close _or_ any previous error. I guess we could wrap it with our own function.

-Peff