Re: [PATCH] open_sha1_file: report "most interesting" errno
From: Junio C Hamano <hidden>
Date: 2016-06-15 23:01:10
Jeff King [off-list ref] writes:
When we try to open a loose object file, we first attempt to
open in the local object database, and then try any
alternates. This means that the errno value when we return
will be from the last place we looked (and due to the way
the code is structured, simply ENOENT if we do not have have
any alternates).
This can cause confusing error messages, as read_sha1_file
checks for ENOENT when reporting a missing object. If errno
is something else, we report that. If it is ENOENT, but
has_loose_object reports that we have it, then we claim the
object is corrupted. For example:
$ chmod 0 .git/objects/??/*
$ git rev-list --all
fatal: loose object b2d6fab18b92d49eac46dc3c5a0bcafabda20131 (stored in .git/objects/b2/d6fab18b92d49eac46dc3c5a0bcafabda20131) is corruptHmmmmmmmm. So we keep track of a more interesting errno we get from some other place than what we get for this local loose object, and we no longer give this message pointing at the local loose object---is that the idea? What I am wondering is that this report we give in the new code
$ git rev-list --all
fatal: failed to read object b2d6fab18b92d49eac46dc3c5a0bcafabda20131: Permission deniedmay want to say which of the various possible places we saw this most interesting errno, which I think was the original motivation came from e8b15e61 (sha1_file: Show the the type and path to corrupt objects, 2010-06-10) that added "(stored in ...)". But that may involve a larger surgery, and I definitely do not want to add unnecessary logic in the common-case codepath to keep track of pieces of information that are only used in the error codepath, so it smells like that this is the best fix to the issue the commit message describes. Thanks.
quoted hunk
Signed-off-by: Jeff King <redacted> --- sha1_file.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)diff --git a/sha1_file.c b/sha1_file.c index 3e9f55f..34d527f 100644 --- a/sha1_file.c +++ b/sha1_file.c@@ -1437,19 +1437,23 @@ static int open_sha1_file(const unsigned char *sha1) { int fd; struct alternate_object_database *alt; + int most_interesting_errno; fd = git_open_noatime(sha1_file_name(sha1)); if (fd >= 0) return fd; + most_interesting_errno = errno; prepare_alt_odb(); - errno = ENOENT; for (alt = alt_odb_list; alt; alt = alt->next) { fill_sha1_path(alt->name, sha1); fd = git_open_noatime(alt->base); if (fd >= 0) return fd; + if (most_interesting_errno == ENOENT) + most_interesting_errno = errno; } + errno = most_interesting_errno; return -1; }