Re: [PATCH] open_sha1_file: report "most interesting" errno
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 15, 2014, 17:02 UTC
- Message-ID
- <xmqqbnuzgelt.fsf@gitster.dls.corp.google.com>
- In-Reply-To
- <20140515085405.GA27033@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 16 quoted lines
> 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 corrupt
Hmmmmmmmm. 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 denied
may 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.
Show 34 quoted lines
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> 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;
> }