Re: [PATCH v5 3/5] pack-bitmap.c: using error() instead of silently returning -1

[Junio C Hamano <gitster@pobox.com>]

Teng Long <dyroneteng@gmail.com> writes:

> It's more accurate here with your suggestion. At the same time I
> found there actually exists many similar place like "ignore ENOENT
> silently" in repo. And I think it's not worth to impove them right now
> in this patch, if you want to do that it could in another pathset and
> please tell me.

It is sufficien to just mark it as #leftoverbits to find and fix
places where the code means to ignore only a missing optional file
but ignores all other errors it gets from open/fopen instead.

>> > @@ -394,7 +394,7 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
>> >
>> >  	if (!is_pack_valid(packfile)) {
>> >  		close(fd);
>> > -		return -1;
>> > +		return error(_("packfile is invalid"));
>>
>> Same "sometimes redundant" comment applies here, but not due to this
>> part of the code but due to the helpers called from is_pack_valid().
>
> Let me try to know about it, the "helpers" means the place where invoke
> is_pack_valid() like here. In fact, in is_pack_valid() they already
> return the errors in various scenarios, so it would be no need to
> return another error.

Yup.  is_pack_valid() calls open_packed_git() and the helper
functions that are called from that call chain are full of calls to
error() that tell specifically what exactly went wrong.  But ...

>> Namely, packfile.c::open_packed_git_1() is mostly chatty, but is
>> silent upon "unable to open" and "unable to fstat" (which I think is
>> safe to make chatty as well---we do not want to special case ENOENT
>> in open_packed_git(), so "cannot open because it is not there" is an
>> error).

... the error coverage is not complete.  There are some (rare) code
paths that silently "return -1", not "return error(_("..."))".  They
should be updated to say something; otherwise we will silently fail
in these "rare" codepaths.

> "cannot open because it is not there" is an error, but I think it will
> also could be a BUG, actually I'm not very sure for clarify the
> difference bwtween the use of the two, but I will look into it to dig
> something out.

The code may have many reasons to believe that a file should exist
there and try to open it, but it may find the file missing, but I
would suspect that it is never a BUG.  You may have run stat() on
the path earlier and you know it existed, but by the time you try to
open it, some other process may have removed it.  You may have found
the .idx file and expect that the corresponding .pack file to exist,
but the .pack file may be missing.  You may have just written a file
and you expect to be able to open it, but some other process may
have removed it already, or you may have run out of file descriptors
and cannot open it.  These are all runtime errors that deserve to be
reported via error() or die(), but never BUG().

Thanks.