Hi,
Johannes Sixt wrote:
> In sha1-file.c:read_object_file_extended() we have the following pattern:
>
> errno = 0;
> data = read_object(r, repl, type, size);
> if (data)
> return data;
>
> if (errno && errno != ENOENT)
> die_errno(_("failed to read object %s"), oid_to_hex(oid));
>
> That is, it is expected that read_object() does not change the value of
> errno in the non-error case. I find it intriguing that we expect a quite
> large call graph that is behind read_object() to behave this way.
Yes, this seems dubious.
In fact this is only inspecting errno in the returned-NULL case. If I
look only at the code above and not at the implementation of
read_object, then I would say that the bug is the 'errno &&' part: when
errno is meaningful for a function for a given return value, the usual
convention is
(1) it *always* sets errno for errors, not conditionally
(2) it never sets errno to 0
There are some exceptions (like strtoul) but they are few and
unfortunate, not something to be imitated.
Do you have more details about the case where read_object is expected
to produce errno == 0? I'm wondering whether we forgot to set 'errno
= ENOENT' explicitly somewhere.
Thanks and hope that helps,
Jonathan
