{"thread":{"id":"36664","subject":"[PATCH] open_sha1_file: report \"most interesting\" errno","startedAt":"2014-05-15T08:54:06Z","lastAt":"2014-05-15T19:11:28Z","messageCount":3,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"241658","messageId":"20140515085405.GA27033@sigill.intra.peff.net","threadId":"36664","inReplyTo":null,"subject":"[PATCH] open_sha1_file: report \"most interesting\" errno","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-15T08:54:06Z","receivedAt":"2014-05-15T08:54:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we try to open a loose object file, we first attempt to\nopen in the local object database, and then try any\nalternates. This means that the errno value when we return\nwill be from the last place we looked (and due to the way\nthe code is structured, simply ENOENT if we do not have have\nany alternates).\n\nThis can cause confusing error messages, as read_sha1_file\nchecks for ENOENT when reporting a missing object. If errno\nis something else, we report that. If it is ENOENT, but\nhas_loose_object reports that we have it, then we claim the\nobject is corrupted. For example:\n\n    $ chmod 0 .git/objects/??/*\n    $ git rev-list --all\n    fatal: loose object b2d6fab18b92d49eac46dc3c5a0bcafabda20131 (stored in .git/objects/b2/d6fab18b92d49eac46dc3c5a0bcafabda20131) is corrupt\n\nThis patch instead keeps track of the \"most interesting\"\nerrno we receive during our search. We consider ENOENT to be\nthe least interesting of all, and otherwise report the first\nerror found (so problems in the object database take\nprecedence over ones in alternates). Here it is with this\npatch:\n\n    $ git rev-list --all\n    fatal: failed to read object b2d6fab18b92d49eac46dc3c5a0bcafabda20131: Permission denied\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n sha1_file.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 3e9f55f..34d527f 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1437,19 +1437,23 @@ static int open_sha1_file(const unsigned char *sha1)\n {\n \tint fd;\n \tstruct alternate_object_database *alt;\n+\tint most_interesting_errno;\n \n \tfd = git_open_noatime(sha1_file_name(sha1));\n \tif (fd >= 0)\n \t\treturn fd;\n+\tmost_interesting_errno = errno;\n \n \tprepare_alt_odb();\n-\terrno = ENOENT;\n \tfor (alt = alt_odb_list; alt; alt = alt->next) {\n \t\tfill_sha1_path(alt->name, sha1);\n \t\tfd = git_open_noatime(alt->base);\n \t\tif (fd >= 0)\n \t\t\treturn fd;\n+\t\tif (most_interesting_errno == ENOENT)\n+\t\t\tmost_interesting_errno = errno;\n \t}\n+\terrno = most_interesting_errno;\n \treturn -1;\n }\n \n-- \n2.0.0.rc1.436.g03cb729\n"},{"id":"241700","messageId":"xmqqbnuzgelt.fsf@gitster.dls.corp.google.com","threadId":"36664","inReplyTo":"20140515085405.GA27033@sigill.intra.peff.net","subject":"Re: [PATCH] open_sha1_file: report \"most interesting\" errno","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-15T17:02:06Z","receivedAt":"2014-05-15T17:02:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> When we try to open a loose object file, we first attempt to\n> open in the local object database, and then try any\n> alternates. This means that the errno value when we return\n> will be from the last place we looked (and due to the way\n> the code is structured, simply ENOENT if we do not have have\n> any alternates).\n>\n> This can cause confusing error messages, as read_sha1_file\n> checks for ENOENT when reporting a missing object. If errno\n> is something else, we report that. If it is ENOENT, but\n> has_loose_object reports that we have it, then we claim the\n> object is corrupted. For example:\n>\n>     $ chmod 0 .git/objects/??/*\n>     $ git rev-list --all\n>     fatal: loose object b2d6fab18b92d49eac46dc3c5a0bcafabda20131 (stored in .git/objects/b2/d6fab18b92d49eac46dc3c5a0bcafabda20131) is corrupt\n\nHmmmmmmmm.  So we keep track of a more interesting errno we get from\nsome other place than what we get for this local loose object, and\nwe no longer give this message pointing at the local loose\nobject---is that the idea?\n\nWhat I am wondering is that this report we give in the new code\n\n>     $ git rev-list --all\n>     fatal: failed to read object b2d6fab18b92d49eac46dc3c5a0bcafabda20131: Permission denied\n\nmay want to say which of the various possible places we saw this\nmost interesting errno, which I think was the original motivation\ncame from e8b15e61 (sha1_file: Show the the type and path to corrupt\nobjects, 2010-06-10) that added \"(stored in ...)\".\n\nBut that may involve a larger surgery, and I definitely do not want\nto add unnecessary logic in the common-case codepath to keep track\nof pieces of information that are only used in the error codepath,\nso it smells like that this is the best fix to the issue the commit\nmessage describes.\n\nThanks.\n\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  sha1_file.c | 6 +++++-\n>  1 file changed, 5 insertions(+), 1 deletion(-)\n>\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 3e9f55f..34d527f 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -1437,19 +1437,23 @@ static int open_sha1_file(const unsigned char *sha1)\n>  {\n>  \tint fd;\n>  \tstruct alternate_object_database *alt;\n> +\tint most_interesting_errno;\n>  \n>  \tfd = git_open_noatime(sha1_file_name(sha1));\n>  \tif (fd >= 0)\n>  \t\treturn fd;\n> +\tmost_interesting_errno = errno;\n>  \n>  \tprepare_alt_odb();\n> -\terrno = ENOENT;\n>  \tfor (alt = alt_odb_list; alt; alt = alt->next) {\n>  \t\tfill_sha1_path(alt->name, sha1);\n>  \t\tfd = git_open_noatime(alt->base);\n>  \t\tif (fd >= 0)\n>  \t\t\treturn fd;\n> +\t\tif (most_interesting_errno == ENOENT)\n> +\t\t\tmost_interesting_errno = errno;\n>  \t}\n> +\terrno = most_interesting_errno;\n>  \treturn -1;\n>  }\n"},{"id":"241767","messageId":"20140515191127.GA29746@sigill.intra.peff.net","threadId":"36664","inReplyTo":"xmqqbnuzgelt.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] open_sha1_file: report \"most interesting\" errno","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-15T19:11:28Z","receivedAt":"2014-05-15T19:11:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 15, 2014 at 10:02:06AM -0700, Junio C Hamano wrote:\n\n> >     $ chmod 0 .git/objects/??/*\n> >     $ git rev-list --all\n> >     fatal: loose object b2d6fab18b92d49eac46dc3c5a0bcafabda20131 (stored in .git/objects/b2/d6fab18b92d49eac46dc3c5a0bcafabda20131) is corrupt\n> \n> Hmmmmmmmm.  So we keep track of a more interesting errno we get from\n> some other place than what we get for this local loose object, and\n> we no longer give this message pointing at the local loose\n> object---is that the idea?\n\nYes, though my main goal was to stop saying \"corrupt\" when that is not\nthe problem at all. Not pointing to the wrong object was a secondary\nconsideration. :)\n\nI would also be happy to just show the error for the local object, even\nif it is exists somewhere else.  The main thing I am changing here is\nthat we currently _never_ show the errno from the main odb. We either\nshow the errno from the last alternate we looked at, or we show ENOENT\n(because we explicitly set ENOENT right before looking at the\nalternates).\n\nI think it's a separate problem that the \"stored in...\" is sometimes\nwrong. That comes when we get ENOENT, and we check has_loose_object().\nIOW, we guess \"we couldn't find it, but we claim to have it, so it must\nhave been corrupt\". But that does not say _where_ we found it, and our\ncall to sha1_file_name is a guess that may be wrong.\n\nI'm actually not sure if we can even trigger that code path now. It\ndepended on returning ENOENT from read_object, which we used to\nfrequently do erroneously. Now we will only do it when the object truly\ndoes not exist, which means has_loose_object should generally not return\ntrue.\n\nI'm also a bit surprised that errno actually survives here. That clearly\nwas the intent, so I don't think my patch is making anything worse. But\nit's possible that we would prepare_packed_git or open/mmap packfiles\nbetween the call to open_sha1_file and when read_sha1_file_extended\nlooks at errno.\n\n> What I am wondering is that this report we give in the new code\n> \n> >     $ git rev-list --all\n> >     fatal: failed to read object b2d6fab18b92d49eac46dc3c5a0bcafabda20131: Permission denied\n> \n> may want to say which of the various possible places we saw this\n> most interesting errno, which I think was the original motivation\n> came from e8b15e61 (sha1_file: Show the the type and path to corrupt\n> objects, 2010-06-10) that added \"(stored in ...)\".\n> \n> But that may involve a larger surgery, and I definitely do not want\n> to add unnecessary logic in the common-case codepath to keep track\n> of pieces of information that are only used in the error codepath,\n> so it smells like that this is the best fix to the issue the commit\n> message describes.\n\nYes, I think doing this right would involve a lot more surgery, and I\ndon't know if it is worth the effort. But in addition to the problems\nabove, I note that we simply open the first object we can find, and do\nnot loop if mmap or checksums fail. So unlike packed objects, which are\nresilient to corruption, we would fail immediately.\n\nSo I think the right thing to do would be:\n\n  1. Don't loop across alternates in open_sha1_file. Loop in read_object\n     (which means looping in _other_ calls to map_sha1_file, like in\n     sha1_object_info).\n\n  2. Fail quickly, since the common case is that we will find the object\n     elsewhere. But when we do have an error, take time to go back and\n     actually find the location of the object and the real error (i.e.,\n     have a diagnose_object or something).\n\nNeither is a particularly high priority to me, though, so I don't plan\non working on them anytime soon. The only reason I went this far was\nthat I saw the \"loose object is corrupt\" / EPERM confusion in the real\nworld.\n\n-Peff\n"}]}