Re: Confusing git messages when disk is full.

[Jeff King]

On Thu, Feb 16, 2017 at 11:10:18AM +0100, Andreas Schwab wrote:

> >>int xfclose(FILE *fp)
> >>{
> >>return ferror(fp) | fclose(fp);
> >>}
> >
> > Yes, that's exactly what I had in mind (might be worth calling out the
> > bitwise-OR, though, just to make it clear it's not a typo).
> 
> Since the order of evaluation is unspecified, it would be better to
> force sequencing ferror before fclose.

Good point. Arguably the call in tempfile.c is buggy.

-Peff

Re: Confusing git messages when disk is full.

[Andreas Schwab]

On Feb 15 2017, Jeff King  wrote:

> On Wed, Feb 15, 2017 at 02:50:19PM -0800, Junio C Hamano wrote:
>
>> > 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.
>> 
>> Sure.  I am happy to add something like this:
>> 
>>  /*
>>   * closes a FILE *, returns 0 if closing and all the
>>   * previous stdio operations on fp were successful,
>>   * otherwise non-zero.
>>   */
>>  int xfclose(FILE *fp)
>>  {
>>  return ferror(fp) | fclose(fp);
>>  }
>
> Yes, that's exactly what I had in mind (might be worth calling out the
> bitwise-OR, though, just to make it clear it's not a typo).

Since the order of evaluation is unspecified, it would be better to
force sequencing ferror before fclose.

Andreas.

-- 
Andreas Schwab, sch...@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Confusing git messages when disk is full.

[Jeff King]

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

> > 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.
> 
> Sure.  I am happy to add something like this:
> 
>   /*
>* closes a FILE *, returns 0 if closing and all the
>* previous stdio operations on fp were successful,
>* otherwise non-zero.
>*/
>   int xfclose(FILE *fp)
>   {
>   return ferror(fp) | fclose(fp);
>   }

Yes, that's exactly what I had in mind (might be worth calling out the
bitwise-OR, though, just to make it clear it's not a typo).

> I do not think we should try to do anything fancier to allow the
> caller to tell ferror() and fclose() apart, as such a caller would
> then need to do

Absolutely. If they care, they can call the two separately.

You are right that errno is untrustworthy in the ferror() case, though.
Maybe that is reason not to add xfclose, and just force this caller to
do something like:

  if (ferror(fp))
rc = error("unable to write to %s", filename);
  if (fclose(fp))
rc = error_errno("unable to write to %s", filename);

Of course, if the earlier error persists through fclose, we'd print two
errors. This would all be much easier if the filehandles kept not just
an error bit, but the original errno. 

Maybe a not-terrible compromise is to fake the errno in the ferror case,
like:

  int xfclose(FILE *fp)
  {
int error_flag = ferror(fp);

/*
 * If we get an error from fclose, the current errno value is
 * trustworthy. But if it succeeds and we had a previous error,
 * we need to report failure, but the value of errno could
 * be unrelated. Make up a generic errno value.
 */
if (fclose(fp))
return EOF;
} else if (error_flag) {
errno = EINVAL; /* or EIO? */
return EOF;
} else {
return 0;
}
  }

Or maybe that would just confuse people when they later get "invalid
argument" in the error message. I wish there was an errno value for "I
don't remember what the error was".

I dunno. This whole thing is ending up a lot more complicated than I had
hoped. I just didn't want to have to say "unable to write to %s" twice. ;)

-Peff

Re: Confusing git messages when disk is full.

[Junio C Hamano]

Jeff King  writes:

> 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.

Sure.  I am happy to add something like this:

/*
 * closes a FILE *, returns 0 if closing and all the
 * previous stdio operations on fp were successful,
 * otherwise non-zero.
 */
int xfclose(FILE *fp)
{
return ferror(fp) | fclose(fp);
}

I do not think we should try to do anything fancier to allow the
caller to tell ferror() and fclose() apart, as such a caller would
then need to do

switch (xfclose(fp)) {
case 0: /* happy */ break;
case XFCLOSE_CLOSE: do "close failed" thing; break;
case XFCLOSE_ERROR: do "other things failed" thing; break;
}

and at that point, "other things failed" code would not have much to
work with to do more detailed diagnosis anyway (the errno is likely
not trustable), and it is not too much to write

if (ferror(fp))
do "saw some failure before" thing;
if (fclose(fp))
do "close failed" thing;

instead.

Re: Confusing git messages when disk is full.

[Jeff King]

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

> Jeff King  writes:
> 
> >>   abort:
> >>strbuf_release();
> >>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

Re: Confusing git messages when disk is full.

[Junio C Hamano]

Jeff King  writes:

>>   abort:
>>  strbuf_release();
>>  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 ;-)

Re: Confusing git messages when disk is full.

[Jeff King]

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

> Jeff King  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.

> 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();
>   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

Re: Confusing git messages when disk is full.

[Junio C Hamano]

Jeff King  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.

 builtin/fetch.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

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();
free(url);
-   fclose(fp);
+   if (ferror(fp))
+   rc = -1;
+   if (fclose(fp))
+   rc = -1;
return rc;
 }
 

Re: Confusing git messages when disk is full.

[Jeff King]

On Sun, Feb 12, 2017 at 05:37:30PM +0100, Jáchym Barvínek wrote:

> Hello, I would like to report what I consider a bug in git, I hope I'm
> doing it the right way.
> I was trying to run `git pull` in  my repository and got the following
> error: "git pull
> Your configuration specifies to merge with the ref 'refs/heads/master'
> from the remote, but no such ref was fetched."

It sounds like writing FETCH_HEAD failed, and git-pull became
confused that the ref wasn't fetched.

> Which was very confusing to me, I found some answers to what might be the 
> cause
> but none was the right one. The actual cause was that the filesystem
> had no more free space.
> When I cleaned the space, `git pull` then gave the expected answer
> ("Already up-to-date.").
> I think the message is confusing and git should be able to report to
> the user that the cause
> is full disk.

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.

-Peff