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