Re: Confusing git messages when disk is full.

From: Jeff King <peff@peff.net>
Date: Feb 15, 2017, 21:51 UTC
Message-ID: <20170215215151.a5chtxyjhbe3og4p@sigill.intra.peff.net>
In-Reply-To: <xmqqk28r2kk4.fsf@gitster.mtv.corp.google.com>

On Wed, Feb 15, 2017 at 01:47:23PM -0800, Junio C Hamano wrote:

Show 21 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > On Sun, Feb 12, 2017 at 05:37:30PM +0100, Jáchym Barvínek wrote:
> > If FETCH_HEAD failed to write because of a full disk (or any other
> > reason), then the right thing is for "git fetch" to write an error to
> > stderr, and git-pull should not continue the operation at all.
> >
> > If we're not doing that, then that is certainly a bug.
> 
> One suspect would be store_updated_refs().  We do catch failure from
> fopen("a") of FETCH_HEAD (it is truncated earlier in the code when
> the --append option is not given), but all the writes go through
> stdio without error checking.
> 
> I wonder if this lazy patch is sufficient.  I want to avoid having
> to sprinkle
> 
> 	if (fputs("\\n", fp))
> 		error(...);
> 
> all over the code.

Heh, I was just tracking down the exact same spot.

I think that yes, the lazy check-error-flag-at-the-end approach is fine for stdio.

I tried to reproduce the original problem on a full loopback filesystem, but got:

  fatal: update_ref failed for ref 'ORIG_HEAD': could not write to '.git/ORIG_HEAD'

I suspect you'd need the _exact_ right amount of free space to get all of the predecessor steps done, and then run out of space right when trying to flush the FETCH_HEAD contents.

Show 14 quoted lines

> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index b5ad09d046..72347f0054 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -868,7 +868,10 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
>   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);

-Peff