{"thread":{"id":"58877","subject":"[PATCH 0/4] Don't lazy-fetch commits when parsing them","startedAt":"2022-11-30T20:31:24Z","lastAt":"2022-12-15T00:13:08Z","messageCount":85,"participants":["Jonathan Tan","Jeff King","Junio C Hamano","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"468278","messageId":"cover.1669839849.git.jonathantanmy@google.com","threadId":"58877","inReplyTo":null,"subject":"[PATCH 0/4] Don't lazy-fetch commits when parsing them","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-11-30T20:30:45Z","receivedAt":"2022-11-30T20:31:24Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"This is a follow-up from my previous email about the possibility of not\nfetching when we know that we're fetching a commit [1]. I had to refactor a few\nthings mostly due to replace objects, so the number of patches might be larger\nthan you would expect. I tried to keep each patch small and easy to understand,\nthough.\n\nPatches 1-3 contain some forward-compatibility and refactoring changes, and\npatch 4 contains the actual logic change.\n\n[1] https://lore.kernel.org/git/20221124004205.1777255-1-jonathantanmy@google.com/\n\nJonathan Tan (4):\n object-file: reread object with exact same args\n object-file: refactor corrupt object diagnosis\n object-file: refactor replace object lookup\n commit: don't lazy-fetch commits\n\n commit.c | 18 ++++++++++++++--\n object-file.c | 57 ++++++++++++++++++++++++++++++++++----------------\n object-store.h | 10 +++++++++\n 3 files changed, 65 insertions(+), 20 deletions(-)\n\n-- \n2.38.1.584.g0f3c55d4c2-goog\n\n"},{"id":"468279","messageId":"604160e79cef94fd8e03fe025990c999bb795395.1669839849.git.jonathantanmy@google.com","threadId":"58877","inReplyTo":"cover.1669839849.git.jonathantanmy@google.com","subject":"[PATCH 1/4] object-file: reread object with exact same args","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-11-30T20:30:46Z","receivedAt":"2022-11-30T20:31:32Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When an object in do_oid_object_info_extended() is found in a packfile,\nbut corrupt, that packfile entry is marked as bad and the read is\nretried. Currently, this is done by invoking the function again but with\nthe replace target of the object and with no flags.\n\nThis currently works, but will be clumsy when a later patch modifies\nthis function to also return the \"real\" object being read (that is, the\nreplace target). It does not make sense to pass a pointer in order to\nreceive this information when no replace lookups are requested, which is\nexactly what the reinvocation does.\n\nTherefore, change this reinvocation to pass exactly the arguments which\nwere originally passed. This also makes us forwards compatible with\nfuture flags that may change the behavior of this function. This does\nslow down the case when packfile corruption is detected, but that is\nexpected to be a very rare case.\n\nSigned-off-by: Jonathan Tan \n---\n object-file.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex 26290554bb..1cde477267 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1621,7 +1621,7 @@ static int do_oid_object_info_extended(struct repository *r,\n \trtype = packed_object_info(r, e.p, e.offset, oi);\n \tif (rtype < 0) {\n \t\tmark_bad_packed_object(e.p, real);\n-\t\treturn do_oid_object_info_extended(r, real, oi, 0);\n+\t\treturn do_oid_object_info_extended(r, oid, oi, flags);\n \t} else if (oi->whence == OI_PACKED) {\n \t\toi->u.packed.offset = e.offset;\n \t\toi->u.packed.pack = e.p;\n-- \n2.38.1.584.g0f3c55d4c2-goog\n\n"},{"id":"468280","messageId":"a8c5fcd9f860a0434f974779bae6edf6a8ceeaca.1669839849.git.jonathantanmy@google.com","threadId":"58877","inReplyTo":"cover.1669839849.git.jonathantanmy@google.com","subject":"[PATCH 2/4] object-file: refactor corrupt object diagnosis","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-11-30T20:30:47Z","receivedAt":"2022-11-30T20:31:33Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"This functionality will be used from another file in a subsequent patch,\nso refactor it into a public function.\n\nSigned-off-by: Jonathan Tan \n---\n object-file.c | 29 ++++++++++++++++++-----------\n object-store.h | 9 +++++++++\n 2 files changed, 27 insertions(+), 11 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex 1cde477267..37468bc256 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1705,9 +1705,6 @@ void *read_object_file_extended(struct repository *r,\n \t\t\t\tint lookup_replace)\n {\n \tvoid *data;\n-\tconst struct packed_git *p;\n-\tconst char *path;\n-\tstruct stat st;\n \tconst struct object_id *repl = lookup_replace ?\n \t\tlookup_replace_object(r, oid) : oid;\n \n@@ -1715,26 +1712,36 @@ void *read_object_file_extended(struct repository *r,\n \tdata = read_object(r, repl, type, size);\n \tif (data)\n \t\treturn data;\n+\tdie_if_corrupt(r, oid, repl);\n+\n+\treturn NULL;\n+}\n+\n+void die_if_corrupt(struct repository *r,\n+\t\t const struct object_id *oid,\n+\t\t const struct object_id *real_oid)\n+{\n+\tconst struct packed_git *p;\n+\tconst char *path;\n+\tstruct stat st;\n \n \tobj_read_lock();\n \tif (errno && errno != ENOENT)\n \t\tdie_errno(_(\"failed to read object %s\"), oid_to_hex(oid));\n \n \t/* die if we replaced an object with one that does not exist */\n-\tif (repl != oid)\n+\tif (real_oid != oid)\n \t\tdie(_(\"replacement %s not found for %s\"),\n-\t\t oid_to_hex(repl), oid_to_hex(oid));\n+\t\t oid_to_hex(real_oid), oid_to_hex(oid));\n \n-\tif (!stat_loose_object(r, repl, &st, &path))\n+\tif (!stat_loose_object(r, real_oid, &st, &path))\n \t\tdie(_(\"loose object %s (stored in %s) is corrupt\"),\n-\t\t oid_to_hex(repl), path);\n+\t\t oid_to_hex(real_oid), path);\n \n-\tif ((p = has_packed_and_bad(r, repl)))\n+\tif ((p = has_packed_and_bad(r, real_oid)))\n \t\tdie(_(\"packed object %s (stored in %s) is corrupt\"),\n-\t\t oid_to_hex(repl), p->pack_name);\n+\t\t oid_to_hex(real_oid), p->pack_name);\n \tobj_read_unlock();\n-\n-\treturn NULL;\n }\n \n void *read_object_with_reference(struct repository *r,\ndiff --git a/object-store.h b/object-store.h\nindex 1be57abaf1..88c879c61e 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -256,6 +256,15 @@ static inline void *repo_read_object_file(struct repository *r,\n #define read_object_file(oid, type, size) repo_read_object_file(the_repository, oid, type, size)\n #endif\n \n+/*\n+ * Dies if real_oid is corrupt, not just missing.\n+ *\n+ * real_oid should be an oid that could not be read.\n+ */\n+void die_if_corrupt(struct repository *r,\n+\t\t const struct object_id *oid,\n+\t\t const struct object_id *real_oid);\n+\n /* Read and unpack an object file into memory, write memory to an object file */\n int oid_object_info(struct repository *r, const struct object_id *, unsigned long *);\n \n-- \n2.38.1.584.g0f3c55d4c2-goog\n\n"},{"id":"468281","messageId":"940396307fea59b434d33edbf2c7f98adc62c053.1669839849.git.jonathantanmy@google.com","threadId":"58877","inReplyTo":"cover.1669839849.git.jonathantanmy@google.com","subject":"[PATCH 3/4] object-file: refactor replace object lookup","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-11-30T20:30:48Z","receivedAt":"2022-11-30T20:31:41Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Move the replace object lookup (specifically, the ability for the caller\nto know the result of the lookup) from read_object_file_extended()\nto one of the functions that it indirectly calls,\ndo_oid_object_info_extended(), because a subsequent patch will need that\nability from the latter.\n\nSigned-off-by: Jonathan Tan \n---\n object-file.c | 28 +++++++++++++++++++++-------\n object-store.h | 1 +\n 2 files changed, 22 insertions(+), 7 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex 37468bc256..fd394f1ace 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1546,6 +1546,11 @@ static int do_oid_object_info_extended(struct repository *r,\n \n \tif (flags & OBJECT_INFO_LOOKUP_REPLACE)\n \t\treal = lookup_replace_object(r, oid);\n+\tif (oi && oi->real_oidp) {\n+\t\tif (!(flags & OBJECT_INFO_LOOKUP_REPLACE))\n+\t\t\tBUG(\"specifying real_oidp does not make sense without OBJECT_INFO_LOOKUP_REPLACE\");\n+\t\t*oi->real_oidp = real;\n+\t}\n \n \tif (is_null_oid(real))\n \t\treturn -1;\n@@ -1659,17 +1664,27 @@ int oid_object_info(struct repository *r,\n \treturn type;\n }\n \n+/*\n+ * If real_oid is not NULL, check if oid has a replace object and store the\n+ * object that we end up using there.\n+ */\n static void *read_object(struct repository *r,\n \t\t\t const struct object_id *oid, enum object_type *type,\n-\t\t\t unsigned long *size)\n+\t\t\t unsigned long *size, const struct object_id **real_oid)\n {\n \tstruct object_info oi = OBJECT_INFO_INIT;\n \tvoid *content;\n+\tunsigned int flags = 0;\n \toi.typep = type;\n \toi.sizep = size;\n \toi.contentp = &content;\n \n-\tif (oid_object_info_extended(r, oid, &oi, 0) < 0)\n+\tif (real_oid) {\n+\t\tflags |= OBJECT_INFO_LOOKUP_REPLACE;\n+\t\toi.real_oidp = real_oid;\n+\t}\n+\n+\tif (oid_object_info_extended(r, oid, &oi, flags) < 0)\n \t\treturn NULL;\n \treturn content;\n }\n@@ -1705,14 +1720,13 @@ void *read_object_file_extended(struct repository *r,\n \t\t\t\tint lookup_replace)\n {\n \tvoid *data;\n-\tconst struct object_id *repl = lookup_replace ?\n-\t\tlookup_replace_object(r, oid) : oid;\n+\tconst struct object_id *real_oid;\n \n \terrno = 0;\n-\tdata = read_object(r, repl, type, size);\n+\tdata = read_object(r, oid, type, size, &real_oid);\n \tif (data)\n \t\treturn data;\n-\tdie_if_corrupt(r, oid, repl);\n+\tdie_if_corrupt(r, oid, real_oid);\n \n \treturn NULL;\n }\n@@ -2283,7 +2297,7 @@ int force_object_loose(const struct object_id *oid, time_t mtime)\n \n \tif (has_loose_object(oid))\n \t\treturn 0;\n-\tbuf = read_object(the_repository, oid, &type, &len);\n+\tbuf = read_object(the_repository, oid, &type, &len, NULL);\n \tif (!buf)\n \t\treturn error(_(\"cannot read object for %s\"), oid_to_hex(oid));\n \thdrlen = format_object_header(hdr, sizeof(hdr), type, len);\ndiff --git a/object-store.h b/object-store.h\nindex 88c879c61e..9684562eb2 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -406,6 +406,7 @@ struct object_info {\n \tstruct object_id *delta_base_oid;\n \tstruct strbuf *type_name;\n \tvoid **contentp;\n+\tconst struct object_id **real_oidp;\n \n \t/* Response */\n \tenum {\n-- \n2.38.1.584.g0f3c55d4c2-goog\n\n"},{"id":"468282","messageId":"6af8dcebd14d803fc8d2a01fbcc7f42ff380719d.1669839849.git.jonathantanmy@google.com","threadId":"58877","inReplyTo":"cover.1669839849.git.jonathantanmy@google.com","subject":"[PATCH 4/4] commit: don't lazy-fetch commits","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-11-30T20:30:49Z","receivedAt":"2022-11-30T20:31:45Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When parsing commits, fail fast when the commit is missing or\ncorrupt, instead of attempting to fetch them. This is done by inlining\nrepo_read_object_file() and setting the flag that prevents fetching.\n\nThis is motivated by a situation in which through a bug (not necessarily\nthrough Git), there was corruption in the object store of a partial\nclone. In this particular case, the problem was exposed when \"git gc\"\ntried to expire reflogs, which calls repo_parse_commit(), which triggers\nfetches of the missing commits.\n\n(There are other possible solutions to this problem including passing an\nargument from \"git gc\" to \"git reflog\" to inhibit all lazy fetches, but\nI think that this fix is at the wrong level - fixing \"git reflog\" means\nthat this particular command works fine, or so we think (it will fail if\nit somehow needs to read a legitimately missing blob, say, a .gitmodules\nfile), but fixing repo_parse_commit() will fix a whole class of bugs.)\n\nSigned-off-by: Jonathan Tan \n---\n commit.c | 18 ++++++++++++++++--\n 1 file changed, 16 insertions(+), 2 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex 572301b80a..17e71f5be4 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -508,6 +508,13 @@ int repo_parse_commit_internal(struct repository *r,\n \tenum object_type type;\n \tvoid *buffer;\n \tunsigned long size;\n+\tconst struct object_id *real_oid;\n+\tstruct object_info oi = {\n+\t\t.typep = &type,\n+\t\t.sizep = &size,\n+\t\t.contentp = &buffer,\n+\t\t.real_oidp = &real_oid,\n+\t};\n \tint ret;\n \n \tif (!item)\n@@ -516,11 +523,18 @@ int repo_parse_commit_internal(struct repository *r,\n \t\treturn 0;\n \tif (use_commit_graph && parse_commit_in_graph(r, item))\n \t\treturn 0;\n-\tbuffer = repo_read_object_file(r, &item->object.oid, &type, &size);\n-\tif (!buffer)\n+\n+\t/*\n+\t * Git does not support partial clones that exclude commits, so set\n+\t * OBJECT_INFO_SKIP_FETCH_OBJECT to fail fast when an object is missing.\n+\t */\n+\tif (oid_object_info_extended(r, &item->object.oid, &oi,\n+\t OBJECT_INFO_LOOKUP_REPLACE | OBJECT_INFO_SKIP_FETCH_OBJECT) < 0) {\n+\t\tdie_if_corrupt(r, &item->object.oid, real_oid);\n \t\treturn quiet_on_missing ? -1 :\n \t\t\terror(\"Could not read %s\",\n \t\t\t oid_to_hex(&item->object.oid));\n+\t}\n \tif (type != OBJ_COMMIT) {\n \t\tfree(buffer);\n \t\treturn error(\"Object %s not a commit\",\n-- \n2.38.1.584.g0f3c55d4c2-goog\n\n"},{"id":"468284","messageId":"Y4fBdW4IMhVXazm4@coredump.intra.peff.net","threadId":"58877","inReplyTo":"a8c5fcd9f860a0434f974779bae6edf6a8ceeaca.1669839849.git.jonathantanmy@google.com","subject":"Re: [PATCH 2/4] object-file: refactor corrupt object diagnosis","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-30T20:47:49Z","receivedAt":"2022-11-30T20:47:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 30, 2022 at 12:30:47PM -0800, Jonathan Tan wrote:\n\n> +void die_if_corrupt(struct repository *r,\n> +\t\t const struct object_id *oid,\n> +\t\t const struct object_id *real_oid)\n> +{\n> +\tconst struct packed_git *p;\n> +\tconst char *path;\n> +\tstruct stat st;\n> \n> \tobj_read_lock();\n> \tif (errno && errno != ENOENT)\n> \t\tdie_errno(_(\"failed to read object %s\"), oid_to_hex(oid));\n> \n> \t/* die if we replaced an object with one that does not exist */\n> -\tif (repl != oid)\n> +\tif (real_oid != oid)\n> \t\tdie(_(\"replacement %s not found for %s\"),\n> -\t\t oid_to_hex(repl), oid_to_hex(oid));\n> +\t\t oid_to_hex(real_oid), oid_to_hex(oid));\n\nThis kind of pointer comparison is a little subtle. Within a single\nfunction, as this code was before this patch, it's probably OK to assume\nthat we use pointer indirection, and a non-replaced object will use the\noriginal pointer. But for a public function, it seems like a gotcha\nthat:\n\n oidcpy(&real_oid, lookup_replace_object(r, &oid));\n die_if_corrupt(r, &oid, &real_oid);\n\nwould produce the wrong answer (it would think replacement happened even\nif it didn't).\n\nSo maybe:\n\n if (!oideq(real_oid, oid))\n\ninstead? It's a little slower, but the point of this is to diagnose and\ndie, so it's not exactly a hot path. :)\n\n-Peff\n"},{"id":"468285","messageId":"Y4fC6HSkpYXKPVCr@coredump.intra.peff.net","threadId":"58877","inReplyTo":"940396307fea59b434d33edbf2c7f98adc62c053.1669839849.git.jonathantanmy@google.com","subject":"Re: [PATCH 3/4] object-file: refactor replace object lookup","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-30T20:54:00Z","receivedAt":"2022-11-30T20:54:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 30, 2022 at 12:30:48PM -0800, Jonathan Tan wrote:\n\n> diff --git a/object-store.h b/object-store.h\n> index 88c879c61e..9684562eb2 100644\n> --- a/object-store.h\n> +++ b/object-store.h\n> @@ -406,6 +406,7 @@ struct object_info {\n> \tstruct object_id *delta_base_oid;\n> \tstruct strbuf *type_name;\n> \tvoid **contentp;\n> +\tconst struct object_id **real_oidp;\n\nOK. The double-pointer here is a bit funky as an interface. It may point\nback to the \"oid\" we fed the function, or it may point to long-term\nstorage owned by the replace mechanism. A more straightforward one would\nbe to store a single-pointer to caller-owned storage, and copy to that\n(like we do for delta_base_oid, for example).\n\nBut doing it this way avoids extra oid copies in the normal,\nnon-replaced case, and matches how the current callers view things. So\nwhile it's a little convoluted, I think it makes sense to do it as you\ndid.\n\n-Peff\n"},{"id":"468286","messageId":"Y4fFaoRFro2hNDdv@coredump.intra.peff.net","threadId":"58877","inReplyTo":"6af8dcebd14d803fc8d2a01fbcc7f42ff380719d.1669839849.git.jonathantanmy@google.com","subject":"Re: [PATCH 4/4] commit: don't lazy-fetch commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-30T21:04:42Z","receivedAt":"2022-11-30T21:04:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 30, 2022 at 12:30:49PM -0800, Jonathan Tan wrote:\n\n> When parsing commits, fail fast when the commit is missing or\n> corrupt, instead of attempting to fetch them. This is done by inlining\n> repo_read_object_file() and setting the flag that prevents fetching.\n> \n> This is motivated by a situation in which through a bug (not necessarily\n> through Git), there was corruption in the object store of a partial\n> clone. In this particular case, the problem was exposed when \"git gc\"\n> tried to expire reflogs, which calls repo_parse_commit(), which triggers\n> fetches of the missing commits.\n\nMakes sense.\n\n> diff --git a/commit.c b/commit.c\n> index 572301b80a..17e71f5be4 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -508,6 +508,13 @@ int repo_parse_commit_internal(struct repository *r,\n> \tenum object_type type;\n> \tvoid *buffer;\n> \tunsigned long size;\n> +\tconst struct object_id *real_oid;\n> +\tstruct object_info oi = {\n> +\t\t.typep = &type,\n> +\t\t.sizep = &size,\n> +\t\t.contentp = &buffer,\n> +\t\t.real_oidp = &real_oid,\n> +\t};\n> \tint ret;\n> \n> \tif (!item)\n> @@ -516,11 +523,18 @@ int repo_parse_commit_internal(struct repository *r,\n> \t\treturn 0;\n> \tif (use_commit_graph && parse_commit_in_graph(r, item))\n> \t\treturn 0;\n> -\tbuffer = repo_read_object_file(r, &item->object.oid, &type, &size);\n> -\tif (!buffer)\n> +\n> +\t/*\n> +\t * Git does not support partial clones that exclude commits, so set\n> +\t * OBJECT_INFO_SKIP_FETCH_OBJECT to fail fast when an object is missing.\n> +\t */\n> +\tif (oid_object_info_extended(r, &item->object.oid, &oi,\n> +\t OBJECT_INFO_LOOKUP_REPLACE | OBJECT_INFO_SKIP_FETCH_OBJECT) < 0) {\n> +\t\tdie_if_corrupt(r, &item->object.oid, real_oid);\n> \t\treturn quiet_on_missing ? -1 :\n> \t\t\terror(\"Could not read %s\",\n> \t\t\t oid_to_hex(&item->object.oid));\n> +\t}\n\nOK, so we know we want a commit object because we're in the\ncommit-parsing function, so we just ask to disable fetching.\n\nTwo devil's advocate thoughts:\n\n 1. What if we're wrong that it's a commit? If somebody references a\n blob in a commit \"parent\" header, the \"right\" outcome is for us to\n say \"oops, the type is wrong\" when we try to parse it as a commit.\n But now in a partial clone, we might avoid fetching that supposed\n commit and say \"you don't have this object\", even though we could\n get it.\n\n I think I'm OK with that. Either way, the repo is corrupt, and\n we'll have informed the user of that. The fact that this bizarre\n and specific sequence of corruptions might not go as far as it can\n to deduce the root cause is probably fine.\n\n 2. Are there other places where we'd want to do the same thing? E.g.,\n in parse_object() we might ask for an object (not knowing its type)\n only to find out that it is a commit. But we have no idea if we\n lazy-fetched it or not!\n\n I had somehow imagined your series would be hooking in at the level\n of the lazy-fetch code, and complaining about fetching commits. But\n that may be tricky to do, because we really don't know the type\n until after we fetch it, and selectively removing an object from\n the odb is quite hard. We'd probably have to tell the other side\n \"please, don't send me any commits\", which requires a protocol\n extension, and...yuck.\n\n By comparison, your approach is an easy win that may catch problems\n in practice (and is certainly better than the status quo).\n\n-Peff\n"},{"id":"468287","messageId":"Y4fF7e+PquFgq7VF@coredump.intra.peff.net","threadId":"58877","inReplyTo":"cover.1669839849.git.jonathantanmy@google.com","subject":"Re: [PATCH 0/4] Don't lazy-fetch commits when parsing them","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-30T21:06:53Z","receivedAt":"2022-11-30T21:07:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 30, 2022 at 12:30:45PM -0800, Jonathan Tan wrote:\n\n> This is a follow-up from my previous email about the possibility of not\n> fetching when we know that we're fetching a commit [1]. I had to refactor a few\n> things mostly due to replace objects, so the number of patches might be larger\n> than you would expect. I tried to keep each patch small and easy to understand,\n> though.\n> \n> Patches 1-3 contain some forward-compatibility and refactoring changes, and\n> patch 4 contains the actual logic change.\n\nThese look pretty good to me. I raised a minor nit in patch 2; if you\nagree it should be a trivial re-roll.\n\nI left some thoughts on the approach in patch 4, but I think given that\nthis is a strict improvement over the status quo, it's a good step\nforward, even if it won't catch all such cases.\n\n-Peff\n"},{"id":"468294","messageId":"xmqqa648uhz2.fsf@gitster.g","threadId":"58877","inReplyTo":"Y4fBdW4IMhVXazm4@coredump.intra.peff.net","subject":"Re: [PATCH 2/4] object-file: refactor corrupt object diagnosis","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-11-30T23:42:57Z","receivedAt":"2022-11-30T23:43:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So maybe:\n>\n> if (!oideq(real_oid, oid))\n>\n> instead? It's a little slower, but the point of this is to diagnose and\n> die, so it's not exactly a hot path. :)\n\nVery true, including the part that the original is fine because it\nis localized and fairly obvious. For a public function, we cannot\nassume any additional constraints between oid and real_oid (other\nthan they are of the same \"struct object_id *\" type) like the two\npointers prepared locally in the original had, and use of oideq()\nwould be more appropriate here.\n\nThanks.\n\n\n"},{"id":"468295","messageId":"xmqq5yewuhc0.fsf@gitster.g","threadId":"58877","inReplyTo":"6af8dcebd14d803fc8d2a01fbcc7f42ff380719d.1669839849.git.jonathantanmy@google.com","subject":"Re: [PATCH 4/4] commit: don't lazy-fetch commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-11-30T23:56:47Z","receivedAt":"2022-11-30T23:56:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> When parsing commits, fail fast when the commit is missing or\n> corrupt, instead of attempting to fetch them. This is done by\n> inlining repo_read_object_file() and setting the flag that\n> prevents fetching.\n>\n> This is motivated by a situation in which through a bug (not\n> necessarily through Git), there was corruption in the object store\n> of a partial clone. In this particular case, the problem was\n> exposed when \"git gc\" tried to expire reflogs, which calls\n> repo_parse_commit(), which triggers fetches of the missing\n> commits.\n\nThe assumption is that there will never be a filtering mode that\nsays \"give us tags and we will lazy-fetch everything reachable from\nit when we need it\", with which the \"solution\" will break down, I\nthink, and I would say it is probably good enough.\n"},{"id":"468339","messageId":"20221201190644.3604867-1-jonathantanmy@google.com","threadId":"58877","inReplyTo":"xmqqa648uhz2.fsf@gitster.g","subject":"Re: [PATCH 2/4] object-file: refactor corrupt object diagnosis","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-12-01T19:06:43Z","receivedAt":"2022-12-01T19:06:53Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Junio C Hamano writes:\n> Jeff King writes:\n> \n> > So maybe:\n> >\n> > if (!oideq(real_oid, oid))\n> >\n> > instead? It's a little slower, but the point of this is to diagnose and\n> > die, so it's not exactly a hot path. :)\n> \n> Very true, including the part that the original is fine because it\n> is localized and fairly obvious. For a public function, we cannot\n> assume any additional constraints between oid and real_oid (other\n> than they are of the same \"struct object_id *\" type) like the two\n> pointers prepared locally in the original had, and use of oideq()\n> would be more appropriate here.\n> \n> Thanks.\n\nMakes sense. I'll make the change.\n"},{"id":"468340","messageId":"20221201191150.3605771-1-jonathantanmy@google.com","threadId":"58877","inReplyTo":"Y4fFaoRFro2hNDdv@coredump.intra.peff.net","subject":"Re: [PATCH 4/4] commit: don't lazy-fetch commits","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-12-01T19:11:50Z","receivedAt":"2022-12-01T19:12:00Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Jeff King writes:\n> OK, so we know we want a commit object because we're in the\n> commit-parsing function, so we just ask to disable fetching.\n> \n> Two devil's advocate thoughts:\n> \n> 1. What if we're wrong that it's a commit? If somebody references a\n> blob in a commit \"parent\" header, the \"right\" outcome is for us to\n> say \"oops, the type is wrong\" when we try to parse it as a commit.\n> But now in a partial clone, we might avoid fetching that supposed\n> commit and say \"you don't have this object\", even though we could\n> get it.\n> \n> I think I'm OK with that. Either way, the repo is corrupt, and\n> we'll have informed the user of that. The fact that this bizarre\n> and specific sequence of corruptions might not go as far as it can\n> to deduce the root cause is probably fine.\n> \n> 2. Are there other places where we'd want to do the same thing? E.g.,\n> in parse_object() we might ask for an object (not knowing its type)\n> only to find out that it is a commit. But we have no idea if we\n> lazy-fetched it or not!\n> \n> I had somehow imagined your series would be hooking in at the level\n> of the lazy-fetch code, and complaining about fetching commits. But\n> that may be tricky to do, because we really don't know the type\n> until after we fetch it, and selectively removing an object from\n> the odb is quite hard. We'd probably have to tell the other side\n> \"please, don't send me any commits\", which requires a protocol\n> extension, and...yuck.\n> \n> By comparison, your approach is an easy win that may catch problems\n> in practice (and is certainly better than the status quo).\n> \n> -Peff\n\nThanks for taking a look. Let me know if you think that the commit message\ncould be improved to cover these cases. Right now I think that e.g. \"When\nparsing an object believed to be a commit in repo_parse_commit_internal()\"\ninstead of \"When parsing commits\" wouldn't add much value, but I might be\nmissing something.\n"},{"id":"468342","messageId":"cover.1669922792.git.jonathantanmy@google.com","threadId":"58877","inReplyTo":"cover.1669839849.git.jonathantanmy@google.com","subject":"[PATCH v2 0/4] Don't lazy-fetch commits when parsing them","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-12-01T19:27:29Z","receivedAt":"2022-12-01T19:27:41Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Thanks everyone for your reviews. Here is a reroll with the requested change\n(just one small one).\n\nJonathan Tan (4):\n object-file: reread object with exact same args\n object-file: refactor corrupt object diagnosis\n object-file: refactor replace object lookup\n commit: don't lazy-fetch commits\n\n commit.c | 18 ++++++++++++++--\n object-file.c | 57 ++++++++++++++++++++++++++++++++++----------------\n object-store.h | 10 +++++++++\n 3 files changed, 65 insertions(+), 20 deletions(-)\n\nRange-diff against v1:\n1: 604160e79c = 1: 604160e79c object-file: reread object with exact same args\n2: a8c5fcd9f8 ! 2: 1be60f1bf2 object-file: refactor corrupt object diagnosis\n @@ object-file.c: void *read_object_file_extended(struct repository *r,\n \n \t/* die if we replaced an object with one that does not exist */\n -\tif (repl != oid)\n -+\tif (real_oid != oid)\n ++\tif (!oideq(real_oid, oid))\n \t\tdie(_(\"replacement %s not found for %s\"),\n -\t\t oid_to_hex(repl), oid_to_hex(oid));\n +\t\t oid_to_hex(real_oid), oid_to_hex(oid));\n3: 940396307f = 3: 28935ba1b0 object-file: refactor replace object lookup\n4: 6af8dcebd1 = 4: a38229c42a commit: don't lazy-fetch commits\n-- \n2.39.0.rc0.267.gcb52ba06e7-goog\n\n"},{"id":"468343","messageId":"604160e79cef94fd8e03fe025990c999bb795395.1669922792.git.jonathantanmy@google.com","threadId":"58877","inReplyTo":"cover.1669922792.git.jonathantanmy@google.com","subject":"[PATCH v2 1/4] object-file: reread object with exact same args","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-12-01T19:27:30Z","receivedAt":"2022-12-01T19:27:44Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When an object in do_oid_object_info_extended() is found in a packfile,\nbut corrupt, that packfile entry is marked as bad and the read is\nretried. Currently, this is done by invoking the function again but with\nthe replace target of the object and with no flags.\n\nThis currently works, but will be clumsy when a later patch modifies\nthis function to also return the \"real\" object being read (that is, the\nreplace target). It does not make sense to pass a pointer in order to\nreceive this information when no replace lookups are requested, which is\nexactly what the reinvocation does.\n\nTherefore, change this reinvocation to pass exactly the arguments which\nwere originally passed. This also makes us forwards compatible with\nfuture flags that may change the behavior of this function. This does\nslow down the case when packfile corruption is detected, but that is\nexpected to be a very rare case.\n\nSigned-off-by: Jonathan Tan \n---\n object-file.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex 26290554bb..1cde477267 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1621,7 +1621,7 @@ static int do_oid_object_info_extended(struct repository *r,\n \trtype = packed_object_info(r, e.p, e.offset, oi);\n \tif (rtype < 0) {\n \t\tmark_bad_packed_object(e.p, real);\n-\t\treturn do_oid_object_info_extended(r, real, oi, 0);\n+\t\treturn do_oid_object_info_extended(r, oid, oi, flags);\n \t} else if (oi->whence == OI_PACKED) {\n \t\toi->u.packed.offset = e.offset;\n \t\toi->u.packed.pack = e.p;\n-- \n2.39.0.rc0.267.gcb52ba06e7-goog\n\n"},{"id":"468344","messageId":"28935ba1b0132fa4d1a9f93e1d65835a8da12455.1669922792.git.jonathantanmy@google.com","threadId":"58877","inReplyTo":"cover.1669922792.git.jonathantanmy@google.com","subject":"[PATCH v2 3/4] object-file: refactor replace object lookup","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-12-01T19:27:32Z","receivedAt":"2022-12-01T19:27:46Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Move the replace object lookup (specifically, the ability for the caller\nto know the result of the lookup) from read_object_file_extended()\nto one of the functions that it indirectly calls,\ndo_oid_object_info_extended(), because a subsequent patch will need that\nability from the latter.\n\nSigned-off-by: Jonathan Tan \n---\n object-file.c | 28 +++++++++++++++++++++-------\n object-store.h | 1 +\n 2 files changed, 22 insertions(+), 7 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex 36f81c7958..8adef99a7c 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1546,6 +1546,11 @@ static int do_oid_object_info_extended(struct repository *r,\n \n \tif (flags & OBJECT_INFO_LOOKUP_REPLACE)\n \t\treal = lookup_replace_object(r, oid);\n+\tif (oi && oi->real_oidp) {\n+\t\tif (!(flags & OBJECT_INFO_LOOKUP_REPLACE))\n+\t\t\tBUG(\"specifying real_oidp does not make sense without OBJECT_INFO_LOOKUP_REPLACE\");\n+\t\t*oi->real_oidp = real;\n+\t}\n \n \tif (is_null_oid(real))\n \t\treturn -1;\n@@ -1659,17 +1664,27 @@ int oid_object_info(struct repository *r,\n \treturn type;\n }\n \n+/*\n+ * If real_oid is not NULL, check if oid has a replace object and store the\n+ * object that we end up using there.\n+ */\n static void *read_object(struct repository *r,\n \t\t\t const struct object_id *oid, enum object_type *type,\n-\t\t\t unsigned long *size)\n+\t\t\t unsigned long *size, const struct object_id **real_oid)\n {\n \tstruct object_info oi = OBJECT_INFO_INIT;\n \tvoid *content;\n+\tunsigned int flags = 0;\n \toi.typep = type;\n \toi.sizep = size;\n \toi.contentp = &content;\n \n-\tif (oid_object_info_extended(r, oid, &oi, 0) < 0)\n+\tif (real_oid) {\n+\t\tflags |= OBJECT_INFO_LOOKUP_REPLACE;\n+\t\toi.real_oidp = real_oid;\n+\t}\n+\n+\tif (oid_object_info_extended(r, oid, &oi, flags) < 0)\n \t\treturn NULL;\n \treturn content;\n }\n@@ -1705,14 +1720,13 @@ void *read_object_file_extended(struct repository *r,\n \t\t\t\tint lookup_replace)\n {\n \tvoid *data;\n-\tconst struct object_id *repl = lookup_replace ?\n-\t\tlookup_replace_object(r, oid) : oid;\n+\tconst struct object_id *real_oid;\n \n \terrno = 0;\n-\tdata = read_object(r, repl, type, size);\n+\tdata = read_object(r, oid, type, size, &real_oid);\n \tif (data)\n \t\treturn data;\n-\tdie_if_corrupt(r, oid, repl);\n+\tdie_if_corrupt(r, oid, real_oid);\n \n \treturn NULL;\n }\n@@ -2283,7 +2297,7 @@ int force_object_loose(const struct object_id *oid, time_t mtime)\n \n \tif (has_loose_object(oid))\n \t\treturn 0;\n-\tbuf = read_object(the_repository, oid, &type, &len);\n+\tbuf = read_object(the_repository, oid, &type, &len, NULL);\n \tif (!buf)\n \t\treturn error(_(\"cannot read object for %s\"), oid_to_hex(oid));\n \thdrlen = format_object_header(hdr, sizeof(hdr), type, len);\ndiff --git a/object-store.h b/object-store.h\nindex 88c879c61e..9684562eb2 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -406,6 +406,7 @@ struct object_info {\n \tstruct object_id *delta_base_oid;\n \tstruct strbuf *type_name;\n \tvoid **contentp;\n+\tconst struct object_id **real_oidp;\n \n \t/* Response */\n \tenum {\n-- \n2.39.0.rc0.267.gcb52ba06e7-goog\n\n"},{"id":"468345","messageId":"1be60f1bf2f368f5e5c8b6550b3e4d4f3efe1496.1669922792.git.jonathantanmy@google.com","threadId":"58877","inReplyTo":"cover.1669922792.git.jonathantanmy@google.com","subject":"[PATCH v2 2/4] object-file: refactor corrupt object diagnosis","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-12-01T19:27:31Z","receivedAt":"2022-12-01T19:27:48Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"This functionality will be used from another file in a subsequent patch,\nso refactor it into a public function.\n\nSigned-off-by: Jonathan Tan \n---\n object-file.c | 29 ++++++++++++++++++-----------\n object-store.h | 9 +++++++++\n 2 files changed, 27 insertions(+), 11 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex 1cde477267..36f81c7958 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1705,9 +1705,6 @@ void *read_object_file_extended(struct repository *r,\n \t\t\t\tint lookup_replace)\n {\n \tvoid *data;\n-\tconst struct packed_git *p;\n-\tconst char *path;\n-\tstruct stat st;\n \tconst struct object_id *repl = lookup_replace ?\n \t\tlookup_replace_object(r, oid) : oid;\n \n@@ -1715,26 +1712,36 @@ void *read_object_file_extended(struct repository *r,\n \tdata = read_object(r, repl, type, size);\n \tif (data)\n \t\treturn data;\n+\tdie_if_corrupt(r, oid, repl);\n+\n+\treturn NULL;\n+}\n+\n+void die_if_corrupt(struct repository *r,\n+\t\t const struct object_id *oid,\n+\t\t const struct object_id *real_oid)\n+{\n+\tconst struct packed_git *p;\n+\tconst char *path;\n+\tstruct stat st;\n \n \tobj_read_lock();\n \tif (errno && errno != ENOENT)\n \t\tdie_errno(_(\"failed to read object %s\"), oid_to_hex(oid));\n \n \t/* die if we replaced an object with one that does not exist */\n-\tif (repl != oid)\n+\tif (!oideq(real_oid, oid))\n \t\tdie(_(\"replacement %s not found for %s\"),\n-\t\t oid_to_hex(repl), oid_to_hex(oid));\n+\t\t oid_to_hex(real_oid), oid_to_hex(oid));\n \n-\tif (!stat_loose_object(r, repl, &st, &path))\n+\tif (!stat_loose_object(r, real_oid, &st, &path))\n \t\tdie(_(\"loose object %s (stored in %s) is corrupt\"),\n-\t\t oid_to_hex(repl), path);\n+\t\t oid_to_hex(real_oid), path);\n \n-\tif ((p = has_packed_and_bad(r, repl)))\n+\tif ((p = has_packed_and_bad(r, real_oid)))\n \t\tdie(_(\"packed object %s (stored in %s) is corrupt\"),\n-\t\t oid_to_hex(repl), p->pack_name);\n+\t\t oid_to_hex(real_oid), p->pack_name);\n \tobj_read_unlock();\n-\n-\treturn NULL;\n }\n \n void *read_object_with_reference(struct repository *r,\ndiff --git a/object-store.h b/object-store.h\nindex 1be57abaf1..88c879c61e 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -256,6 +256,15 @@ static inline void *repo_read_object_file(struct repository *r,\n #define read_object_file(oid, type, size) repo_read_object_file(the_repository, oid, type, size)\n #endif\n \n+/*\n+ * Dies if real_oid is corrupt, not just missing.\n+ *\n+ * real_oid should be an oid that could not be read.\n+ */\n+void die_if_corrupt(struct repository *r,\n+\t\t const struct object_id *oid,\n+\t\t const struct object_id *real_oid);\n+\n /* Read and unpack an object file into memory, write memory to an object file */\n int oid_object_info(struct repository *r, const struct object_id *, unsigned long *);\n \n-- \n2.39.0.rc0.267.gcb52ba06e7-goog\n\n"},{"id":"468346","messageId":"a38229c42ae1dec4dcc52e6dc949f4a90846129d.1669922792.git.jonathantanmy@google.com","threadId":"58877","inReplyTo":"cover.1669922792.git.jonathantanmy@google.com","subject":"[PATCH v2 4/4] commit: don't lazy-fetch commits","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-12-01T19:27:33Z","receivedAt":"2022-12-01T19:27:58Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When parsing commits, fail fast when the commit is missing or\ncorrupt, instead of attempting to fetch them. This is done by inlining\nrepo_read_object_file() and setting the flag that prevents fetching.\n\nThis is motivated by a situation in which through a bug (not necessarily\nthrough Git), there was corruption in the object store of a partial\nclone. In this particular case, the problem was exposed when \"git gc\"\ntried to expire reflogs, which calls repo_parse_commit(), which triggers\nfetches of the missing commits.\n\n(There are other possible solutions to this problem including passing an\nargument from \"git gc\" to \"git reflog\" to inhibit all lazy fetches, but\nI think that this fix is at the wrong level - fixing \"git reflog\" means\nthat this particular command works fine, or so we think (it will fail if\nit somehow needs to read a legitimately missing blob, say, a .gitmodules\nfile), but fixing repo_parse_commit() will fix a whole class of bugs.)\n\nSigned-off-by: Jonathan Tan \n---\n commit.c | 18 ++++++++++++++++--\n 1 file changed, 16 insertions(+), 2 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex 572301b80a..17e71f5be4 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -508,6 +508,13 @@ int repo_parse_commit_internal(struct repository *r,\n \tenum object_type type;\n \tvoid *buffer;\n \tunsigned long size;\n+\tconst struct object_id *real_oid;\n+\tstruct object_info oi = {\n+\t\t.typep = &type,\n+\t\t.sizep = &size,\n+\t\t.contentp = &buffer,\n+\t\t.real_oidp = &real_oid,\n+\t};\n \tint ret;\n \n \tif (!item)\n@@ -516,11 +523,18 @@ int repo_parse_commit_internal(struct repository *r,\n \t\treturn 0;\n \tif (use_commit_graph && parse_commit_in_graph(r, item))\n \t\treturn 0;\n-\tbuffer = repo_read_object_file(r, &item->object.oid, &type, &size);\n-\tif (!buffer)\n+\n+\t/*\n+\t * Git does not support partial clones that exclude commits, so set\n+\t * OBJECT_INFO_SKIP_FETCH_OBJECT to fail fast when an object is missing.\n+\t */\n+\tif (oid_object_info_extended(r, &item->object.oid, &oi,\n+\t OBJECT_INFO_LOOKUP_REPLACE | OBJECT_INFO_SKIP_FETCH_OBJECT) < 0) {\n+\t\tdie_if_corrupt(r, &item->object.oid, real_oid);\n \t\treturn quiet_on_missing ? -1 :\n \t\t\terror(\"Could not read %s\",\n \t\t\t oid_to_hex(&item->object.oid));\n+\t}\n \tif (type != OBJ_COMMIT) {\n \t\tfree(buffer);\n \t\treturn error(\"Object %s not a commit\",\n-- \n2.39.0.rc0.267.gcb52ba06e7-goog\n\n"},{"id":"468347","messageId":"Y4kBor6o+Sclifny@coredump.intra.peff.net","threadId":"58877","inReplyTo":"20221201191150.3605771-1-jonathantanmy@google.com","subject":"Re: [PATCH 4/4] commit: don't lazy-fetch commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-12-01T19:33:54Z","receivedAt":"2022-12-01T19:35:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 01, 2022 at 11:11:50AM -0800, Jonathan Tan wrote:\n\n> Jeff King writes:\n> > OK, so we know we want a commit object because we're in the\n> > commit-parsing function, so we just ask to disable fetching.\n> > \n> > Two devil's advocate thoughts:\n> [...]\n> \n> Thanks for taking a look. Let me know if you think that the commit message\n> could be improved to cover these cases. Right now I think that e.g. \"When\n> parsing an object believed to be a commit in repo_parse_commit_internal()\"\n> instead of \"When parsing commits\" wouldn't add much value, but I might be\n> missing something.\n\nI think your commit message is OK as-is. I was mostly just laying out my\nthoughts in reviewing. Some of that could go into the commit message as\nnotes, but I think it is sufficient that they're here in the list\narchive.\n\n-Peff\n"},{"id":"468349","messageId":"Y4kGiEXdTOpn5Eyi@coredump.intra.peff.net","threadId":"58877","inReplyTo":"cover.1669922792.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 0/4] Don't lazy-fetch commits when parsing them","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-12-01T19:54:48Z","receivedAt":"2022-12-01T19:54:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 01, 2022 at 11:27:29AM -0800, Jonathan Tan wrote:\n\n> Thanks everyone for your reviews. Here is a reroll with the requested change\n> (just one small one).\n\nThanks, this looks OK to me. However Junio noted in \"What's cooking\"\nthat it seems to break CI on windows. The problem is in t5318.93:\n\n 2022-12-01T09:26:44.8887018Z ++ cat test_err\n 2022-12-01T09:26:44.8887414Z error: Could not read 0000000000000000000000000000000000000000\n 2022-12-01T09:26:44.8887825Z error: Could not read 0000000000000000000000000000000000000000\n 2022-12-01T09:26:44.8888240Z error: Could not read 0000000000000000000000000000000000000000\n 2022-12-01T09:26:44.8888639Z error: Could not read 0000000000000000000000000000000000000000\n 2022-12-01T09:26:44.8889052Z error: Could not read 0000000000000000000000000000000000000000\n 2022-12-01T09:26:44.8889512Z error: Could not read 0000000000000000000000000000000000000000\n 2022-12-01T09:26:44.8889991Z fatal: failed to read object 0000000000000000000000000000000000000000: Function not implemented\n 2022-12-01T09:26:44.8890401Z ++ return 1\n 2022-12-01T09:26:44.8890761Z error: last command exited with $?=1\n 2022-12-01T09:26:44.8891263Z not ok 93 - corrupt commit-graph write (broken parent)\n\nLooks like the check in die_if_corrupt() is seeing a different errno\nvalue than ENOENT. I wonder if we need to take more care to preserve it\nacross calls. It does look like we hit the same sequence of functions\nthat read_object_file_extended() did, but perhaps this was buggy all\nalong, and you're now exposing it through a new code path.\n\nIn particular I wonder if obj_read_unlock() might be the culprit here,\nand something like this might help:\n\ndiff --git a/object-file.c b/object-file.c\nindex 8adef99a7c..db2d35519e 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1641,9 +1641,12 @@ int oid_object_info_extended(struct repository *r, const struct object_id *oid,\n \t\t\t struct object_info *oi, unsigned flags)\n {\n \tint ret;\n+\tint save_errno;\n \tobj_read_lock();\n \tret = do_oid_object_info_extended(r, oid, oi, flags);\n+\tsave_errno = errno;\n \tobj_read_unlock();\n+\terrno = save_errno;\n \treturn ret;\n }\n \n\n-Peff\n"},{"id":"468354","messageId":"20221201212650.414069-1-jonathantanmy@google.com","threadId":"58877","inReplyTo":"Y4kGiEXdTOpn5Eyi@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/4] Don't lazy-fetch commits when parsing them","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-12-01T21:26:50Z","receivedAt":"2022-12-01T21:27:00Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Jeff King writes:\n> On Thu, Dec 01, 2022 at 11:27:29AM -0800, Jonathan Tan wrote:\n> \n> > Thanks everyone for your reviews. Here is a reroll with the requested change\n> > (just one small one).\n> \n> Thanks, this looks OK to me. However Junio noted in \"What's cooking\"\n> that it seems to break CI on windows. The problem is in t5318.93:\n> \n> 2022-12-01T09:26:44.8887018Z ++ cat test_err\n> 2022-12-01T09:26:44.8887414Z error: Could not read 0000000000000000000000000000000000000000\n> 2022-12-01T09:26:44.8887825Z error: Could not read 0000000000000000000000000000000000000000\n> 2022-12-01T09:26:44.8888240Z error: Could not read 0000000000000000000000000000000000000000\n> 2022-12-01T09:26:44.8888639Z error: Could not read 0000000000000000000000000000000000000000\n> 2022-12-01T09:26:44.8889052Z error: Could not read 0000000000000000000000000000000000000000\n> 2022-12-01T09:26:44.8889512Z error: Could not read 0000000000000000000000000000000000000000\n> 2022-12-01T09:26:44.8889991Z fatal: failed to read object 0000000000000000000000000000000000000000: Function not implemented\n> 2022-12-01T09:26:44.8890401Z ++ return 1\n> 2022-12-01T09:26:44.8890761Z error: last command exited with $?=1\n> 2022-12-01T09:26:44.8891263Z not ok 93 - corrupt commit-graph write (broken parent)\n> \n> Looks like the check in die_if_corrupt() is seeing a different errno\n> value than ENOENT. I wonder if we need to take more care to preserve it\n> across calls. It does look like we hit the same sequence of functions\n> that read_object_file_extended() did, but perhaps this was buggy all\n> along, and you're now exposing it through a new code path.\n> \n> In particular I wonder if obj_read_unlock() might be the culprit here,\n> and something like this might help:\n> \n> diff --git a/object-file.c b/object-file.c\n> index 8adef99a7c..db2d35519e 100644\n> --- a/object-file.c\n> +++ b/object-file.c\n> @@ -1641,9 +1641,12 @@ int oid_object_info_extended(struct repository *r, const struct object_id *oid,\n> \t\t\t struct object_info *oi, unsigned flags)\n> {\n> \tint ret;\n> +\tint save_errno;\n> \tobj_read_lock();\n> \tret = do_oid_object_info_extended(r, oid, oi, flags);\n> +\tsave_errno = errno;\n> \tobj_read_unlock();\n> +\terrno = save_errno;\n> \treturn ret;\n> }\n \nCopying die_if_corrupt() until \"failed to read object\":\n\n> 1734 void die_if_corrupt(struct repository *r, \n> 1735 const struct object_id *oid, \n> 1736 const struct object_id *real_oid) \n> 1737 { \n> 1738 const struct packed_git *p; \n> 1739 const char *path; \n> 1740 struct stat st; \n> 1741 \n> 1742 obj_read_lock(); \n> 1743 if (errno && errno != ENOENT) \n> 1744 die_errno(_(\"failed to read object %s\"), oid_to_hex(oid));\n\nI wonder if we could just remove this check. Even as it is, I don't think that\nthere is any guarantee that obj_read_lock() would not clobber errno. Removing\nit makes all tests pass locally, but I haven't tried it on CI.\n\n(One argument that could be made is that we shouldn't have any die_if_corrupt()\nrefactoring or other refactoring of the sort, because previously its contents\nwas part of a function and it could thus rely on the errno of what has happened\npreviously. But I think that even without my patches, we couldn't rely on it\nin the first place - looking at obj_read_lock(), it looks like it could init a\nmutex, and depending on the implementation of that, it could clobber errno.)\n"},{"id":"468369","messageId":"xmqqv8mura9l.fsf@gitster.g","threadId":"58877","inReplyTo":"Y4kGiEXdTOpn5Eyi@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/4] Don't lazy-fetch commits when parsing them","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-01T23:09:58Z","receivedAt":"2022-12-01T23:11:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Dec 01, 2022 at 11:27:29AM -0800, Jonathan Tan wrote:\n>\n>> Thanks everyone for your reviews. Here is a reroll with the requested change\n>> (just one small one).\n>\n> Thanks, this looks OK to me. However Junio noted in \"What's cooking\"\n> that it seems to break CI on windows. The problem is in t5318.93:\n> ...\n> In particular I wonder if obj_read_unlock() might be the culprit here,\n> and something like this might help:\n\nThanks for following-up.\n"},{"id":"468382","messageId":"Y4lFbemK4HHiCsyJ@coredump.intra.peff.net","threadId":"58877","inReplyTo":"20221201212650.414069-1-jonathantanmy@google.com","subject":"Re: [PATCH v2 0/4] Don't lazy-fetch commits when parsing them","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-12-02T00:23:09Z","receivedAt":"2022-12-02T00:28:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 01, 2022 at 01:26:50PM -0800, Jonathan Tan wrote:\n\n> > 1734 void die_if_corrupt(struct repository *r,\n> > 1735 const struct object_id *oid,\n> > 1736 const struct object_id *real_oid)\n> > 1737 {\n> > 1738 const struct packed_git *p;\n> > 1739 const char *path;\n> > 1740 struct stat st;\n> > 1741\n> > 1742 obj_read_lock();\n> > 1743 if (errno && errno != ENOENT)\n> > 1744 die_errno(_(\"failed to read object %s\"), oid_to_hex(oid));\n> \n> I wonder if we could just remove this check. Even as it is, I don't think that\n> there is any guarantee that obj_read_lock() would not clobber errno. Removing\n> it makes all tests pass locally, but I haven't tried it on CI.\n> \n> (One argument that could be made is that we shouldn't have any die_if_corrupt()\n> refactoring or other refactoring of the sort, because previously its contents\n> was part of a function and it could thus rely on the errno of what has happened\n> previously. But I think that even without my patches, we couldn't rely on it\n> in the first place - looking at obj_read_lock(), it looks like it could init a\n> mutex, and depending on the implementation of that, it could clobber errno.)\n\nYeah, I don't see any difference in the new caller versus what the\noriginal was doing. The errno we care about comes from inside\noid_object_info_extended(). So in any case, we'll see at least\nobj_read_unlock() followed by obj_read_lock() between the syscalls of\ninterest and this check. And I don't even really see any indication that\noid_object_info_extended() tries to set or preserve errno itself. The\nlikely sequence is:\n\n - find_pack_entry() fails to find it; errno isn't set at all\n - loose_object_info() tries to open it and probably gets ENOENT\n - we check find_pack_entry() again after reprepare_packed_git()\n - that fails so we return -1, barring submodule or partial clone\n tricks\n\nSo it really seems like we're quite likely to get an errno from opening\nor mapping packs. Which implies the original suffers from the same\nissue, but we simply never triggered it meaningfully in a test.\n\nI'm not entirely sure on just removing the check. It comes from\n3ba7a06552 (A loose object is not corrupt if it cannot be read due to\nEMFILE, 2010-10-28), so we'd lose what that commit is trying to do.\nThough I think even back then, I think it would have suffered from the\nsame problems (minus the lock/unlock; I'm still unclear which syscall is\nthe actual culprit here).\n\nIf we assume that errno from reading the object isn't reliable, I think\nyou'd have to actually re-check things. Something like:\n\n if (find_pack_entry(...) || !stat_loose_object(...))\n /* ok, it's not missing */\n\nbut of course we don't have the actual errno that _did_ cause us to\nfail, which makes the error message we'd print a lot less useful. Maybe\nthis check should be ditched and we should complain much closer to the\nsource of the problem:\n\ndiff --git a/object-file.c b/object-file.c\nindex 26290554bb..743ba8210e 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1460,8 +1460,12 @@ static int loose_object_info(struct repository *r,\n \t}\n \n \tmap = map_loose_object(r, oid, &mapsize);\n-\tif (!map)\n+\tif (!map) {\n+\t\tif (errno != ENOENT)\n+\t\t\terror_errno(\"unable to open loose object %s\",\n+\t\t\t\t oid_to_hex(oid));\n \t\treturn -1;\n+\t}\n \n \tif (!oi->sizep)\n \t\toi->sizep = &size_scratch;\n\nThat might make things more verbose for other code paths, but that kind\nof seems like a good thing. If you have an object file that we can't\nopen, we probably _should_ be complaining loudly about it.\n\nWe may need to be a little more careful about preserving errno in\nmap_loose_object_1(), though (gee, another place where the existing\ncheck could run into trouble).\n\n-Peff\n"},{"id":"468569","messageId":"20221206004935.1794596-1-jonathantanmy@google.com","threadId":"58877","inReplyTo":"Y4lFbemK4HHiCsyJ@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/4] Don't lazy-fetch commits when parsing them","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-12-06T00:49:34Z","receivedAt":"2022-12-06T00:49:43Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Jeff King writes:\n> Yeah, I don't see any difference in the new caller versus what the\n> original was doing. The errno we care about comes from inside\n> oid_object_info_extended(). So in any case, we'll see at least\n> obj_read_unlock() followed by obj_read_lock() between the syscalls of\n> interest and this check. And I don't even really see any indication that\n> oid_object_info_extended() tries to set or preserve errno itself. The\n> likely sequence is:\n> \n> - find_pack_entry() fails to find it; errno isn't set at all\n> - loose_object_info() tries to open it and probably gets ENOENT\n> - we check find_pack_entry() again after reprepare_packed_git()\n> - that fails so we return -1, barring submodule or partial clone\n> tricks\n> \n> So it really seems like we're quite likely to get an errno from opening\n> or mapping packs. Which implies the original suffers from the same\n> issue, but we simply never triggered it meaningfully in a test.\n\nThanks for checking. I'm still not sure how the current code passes CI, but my\npatches don't. \n\n> I'm not entirely sure on just removing the check. It comes from\n> 3ba7a06552 (A loose object is not corrupt if it cannot be read due to\n> EMFILE, 2010-10-28), so we'd lose what that commit is trying to do.\n> Though I think even back then, I think it would have suffered from the\n> same problems (minus the lock/unlock; I'm still unclear which syscall is\n> the actual culprit here).\n\nAh, thanks for the pointer to that commit. Without that, my patch would report\ncorruption even if the real issue was EMFILE, as the commit message of that\ncommit describes.\n\n> If we assume that errno from reading the object isn't reliable, I think\n> you'd have to actually re-check things. Something like:\n> \n> if (find_pack_entry(...) || !stat_loose_object(...))\n> /* ok, it's not missing */\n> \n> but of course we don't have the actual errno that _did_ cause us to\n> fail, which makes the error message we'd print a lot less useful. Maybe\n> this check should be ditched and we should complain much closer to the\n> source of the problem:\n> \n> diff --git a/object-file.c b/object-file.c\n> index 26290554bb..743ba8210e 100644\n> --- a/object-file.c\n> +++ b/object-file.c\n> @@ -1460,8 +1460,12 @@ static int loose_object_info(struct repository *r,\n> \t}\n> \n> \tmap = map_loose_object(r, oid, &mapsize);\n> -\tif (!map)\n> +\tif (!map) {\n> +\t\tif (errno != ENOENT)\n> +\t\t\terror_errno(\"unable to open loose object %s\",\n> +\t\t\t\t oid_to_hex(oid));\n> \t\treturn -1;\n> +\t}\n> \n> \tif (!oi->sizep)\n> \t\toi->sizep = &size_scratch;\n> \n> That might make things more verbose for other code paths, but that kind\n> of seems like a good thing. If you have an object file that we can't\n> open, we probably _should_ be complaining loudly about it.\n> \n> We may need to be a little more careful about preserving errno in\n> map_loose_object_1(), though (gee, another place where the existing\n> check could run into trouble).\n\nBesides needing to be careful in map_loose_object_1(), I'm not sure if this\nfully solves the problem. This is non-fatal, so the EMFILE commit's work would\nstill remain undone. If this were made fatal, I think this would change the\nbehavior of too much code, especially those that can tolerate loose objects\nbeing missing.\n \nWhat do you think of not putting any die_if_corrupt() calls in the commit\nparsing code at all? The error message printed would then be different (just a\ngeneric message about being unable to parse a commit, versus the specific one\nhere) but it does pass CI [1]. Also, I don't think that we should be doing errno\ndiagnostics separate from what causes the errno anyway.\n\n[1] https://github.com/jonathantanmy/git/actions/runs/3624495729\n"},{"id":"468579","messageId":"Y46i/npXcnsT1pqF@coredump.intra.peff.net","threadId":"58877","inReplyTo":"20221206004935.1794596-1-jonathantanmy@google.com","subject":"Re: [PATCH v2 0/4] Don't lazy-fetch commits when parsing them","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-12-06T02:03:42Z","receivedAt":"2022-12-06T02:03:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 05, 2022 at 04:49:34PM -0800, Jonathan Tan wrote:\n\n> > So it really seems like we're quite likely to get an errno from opening\n> > or mapping packs. Which implies the original suffers from the same\n> > issue, but we simply never triggered it meaningfully in a test.\n> \n> Thanks for checking. I'm still not sure how the current code passes CI, but my\n> patches don't. \n\nHmm. Actually, I am now, too.\n\nMy assumption was that only certain tests would notice the problem,\nbecause both outcomes are an error, and they only differ in what stderr\nsays (\"this does not exist\" versus \"we got this weird errno\"). And so I\nassumed that the old spot in read_object_file() did not happen to\ntrigger any tests which check stderr, but your new caller in\nrepo_parse_commit_internal() was unlucky enough to do so. But since that\nnew caller was calling repo_read_object_file() before, I'd think it\nwould have triggered the same thing.\n\n> > That might make things more verbose for other code paths, but that kind\n> > of seems like a good thing. If you have an object file that we can't\n> > open, we probably _should_ be complaining loudly about it.\n> > \n> > We may need to be a little more careful about preserving errno in\n> > map_loose_object_1(), though (gee, another place where the existing\n> > check could run into trouble).\n> \n> Besides needing to be careful in map_loose_object_1(), I'm not sure if this\n> fully solves the problem. This is non-fatal, so the EMFILE commit's work would\n> still remain undone. If this were made fatal, I think this would change the\n> behavior of too much code, especially those that can tolerate loose objects\n> being missing.\n\nTrue, in the sense that we'd still say \"X is corrupt\". But I think the\nreal sin prior to 3ba7a0655 is that we _only_ said \"hey, this looks\ncorrupt\". If the output is:\n\n error: unable to mmap .git/objects/12/3456abcd...\n fatal: object 123456abcd is corrupt or missing\n\nthen that is at least not actively misleading (and is broadly similar to\nother cases in Git, where higher-level code only knows \"I expected us to\nhave this object and for some reason we don't\", but without knowing\nwhether it was missing, or a system error, or corrupt).\n\nThat said I think all of these die() statements that you moved into\ndie_if_corrupt() are already doing the wrong thing. We should probably\nmention errors (besides \"missing object\") to the user at the lowest\nlevel where we can give the most detail, and then return errors up the\nstack. That makes Git more verbose, but remember we're talking about\ncorrupted or broken repositories here. You shouldn't see these under\nnormal circumstances.\n\n> What do you think of not putting any die_if_corrupt() calls in the commit\n> parsing code at all? The error message printed would then be different (just a\n> generic message about being unable to parse a commit, versus the specific one\n> here) but it does pass CI [1]. Also, I don't think that we should be doing errno\n> diagnostics separate from what causes the errno anyway.\n\nSo I think that's a lesser version of what I'm proposing above. ;) In\nthe sense that yes, I would say that repo_parse_commit_internal() does\nnot need to do anything more specific than its existing \"could not read\"\nmessage. But I would go further and say:\n\n - it would be nice the low-level code where errno _is_ valid said more\n about what happened\n\n - we should take read_object_file_extended() in the same direction\n\n-Peff\n"},{"id":"468655","messageId":"cover.1670373420.git.jonathantanmy@google.com","threadId":"58877","inReplyTo":"cover.1669839849.git.jonathantanmy@google.com","subject":"[PATCH v2 0/3] Don't lazy-fetch commits when parsing them","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-12-07T00:40:50Z","receivedAt":"2022-12-07T00:41:04Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Thanks everyone for taking a look. In the end, I thought it best to bite the\nbullet and move all the corruption diagnostics to where they are detected\ninstead of re-checking (and thus relying on errno to be preserved) after we\nhave found out that we couldn't read the object. This does mean a reworking\nof all the earlier patches, but overall I think that this puts the code in a\nbetter state.\n\nI have also verified that this passes CI [1].\n\n[1] https://github.com/jonathantanmy/git/actions/runs/3634359088\n\nJonathan Tan (3):\n object-file: don't exit early if skipping loose\n object-file: emit corruption errors when detected\n commit: don't lazy-fetch commits\n\n commit.c | 15 +++++++--\n object-file.c | 84 ++++++++++++++++++++++++++------------------------\n object-store.h | 3 ++\n 3 files changed, 59 insertions(+), 43 deletions(-)\n\nRange-diff against v1:\n1: 3a00bc45fd < -: ---------- object-file: reread object with exact same args\n2: 9999e127a0 < -: ---------- object-file: refactor corrupt object diagnosis\n3: 28c7ee2f8c < -: ---------- object-file: refactor replace object lookup\n-: ---------- > 1: 9ad34a1dce object-file: don't exit early if skipping loose\n-: ---------- > 2: 9ddfff3585 object-file: emit corruption errors when detected\n4: 0607fa67d1 ! 3: c5fe42deb0 commit: don't lazy-fetch commits\n @@ commit.c: int repo_parse_commit_internal(struct repository *r,\n \tenum object_type type;\n \tvoid *buffer;\n \tunsigned long size;\n -+\tconst struct object_id *real_oid;\n +\tstruct object_info oi = {\n +\t\t.typep = &type,\n +\t\t.sizep = &size,\n +\t\t.contentp = &buffer,\n -+\t\t.real_oidp = &real_oid,\n +\t};\n ++\t/*\n ++\t * Git does not support partial clones that exclude commits, so set\n ++\t * OBJECT_INFO_SKIP_FETCH_OBJECT to fail fast when an object is missing.\n ++\t */\n ++\tint flags = OBJECT_INFO_LOOKUP_REPLACE | OBJECT_INFO_SKIP_FETCH_OBJECT |\n ++\t\tOBJECT_INFO_DIE_IF_CORRUPT;\n \tint ret;\n \n \tif (!item)\n @@ commit.c: int repo_parse_commit_internal(struct repository *r,\n -\tbuffer = repo_read_object_file(r, &item->object.oid, &type, &size);\n -\tif (!buffer)\n +\n -+\t/*\n -+\t * Git does not support partial clones that exclude commits, so set\n -+\t * OBJECT_INFO_SKIP_FETCH_OBJECT to fail fast when an object is missing.\n -+\t */\n -+\tif (oid_object_info_extended(r, &item->object.oid, &oi,\n -+\t OBJECT_INFO_LOOKUP_REPLACE | OBJECT_INFO_SKIP_FETCH_OBJECT) < 0) {\n -+\t\tdie_if_corrupt(r, &item->object.oid, real_oid);\n ++\tif (oid_object_info_extended(r, &item->object.oid, &oi, flags) < 0)\n \t\treturn quiet_on_missing ? -1 :\n \t\t\terror(\"Could not read %s\",\n \t\t\t oid_to_hex(&item->object.oid));\n -+\t}\n - \tif (type != OBJ_COMMIT) {\n - \t\tfree(buffer);\n - \t\treturn error(\"Object %s not a commit\",\n-- \n2.39.0.rc0.267.gcb52ba06e7-goog\n\n"},{"id":"468656","messageId":"9ad34a1dce977044066de0bfa6e25977215e8dc9.1670373420.git.jonathantanmy@google.com","threadId":"58877","inReplyTo":"cover.1670373420.git.jonathantanmy@google.com","subject":"[PATCH v2 1/3] object-file: don't exit early if skipping loose","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-12-07T00:40:51Z","receivedAt":"2022-12-07T00:41:08Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Instead, also search the submodule object stores and promisor remotes.\n\nThis also centralizes what happens when the object is not found (the\n\"return -1\"), which is useful for a subsequent patch.\n\nSigned-off-by: Jonathan Tan \n---\n object-file.c | 23 +++++++++++------------\n 1 file changed, 11 insertions(+), 12 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex 26290554bb..596dd049fd 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1575,18 +1575,17 @@ static int do_oid_object_info_extended(struct repository *r,\n \t\tif (find_pack_entry(r, real, &e))\n \t\t\tbreak;\n \n-\t\tif (flags & OBJECT_INFO_IGNORE_LOOSE)\n-\t\t\treturn -1;\n-\n-\t\t/* Most likely it's a loose object. */\n-\t\tif (!loose_object_info(r, real, oi, flags))\n-\t\t\treturn 0;\n-\n-\t\t/* Not a loose object; someone else may have just packed it. */\n-\t\tif (!(flags & OBJECT_INFO_QUICK)) {\n-\t\t\treprepare_packed_git(r);\n-\t\t\tif (find_pack_entry(r, real, &e))\n-\t\t\t\tbreak;\n+\t\tif (!(flags & OBJECT_INFO_IGNORE_LOOSE)) {\n+\t\t\t/* Most likely it's a loose object. */\n+\t\t\tif (!loose_object_info(r, real, oi, flags))\n+\t\t\t\treturn 0;\n+\n+\t\t\t/* Not a loose object; someone else may have just packed it. */\n+\t\t\tif (!(flags & OBJECT_INFO_QUICK)) {\n+\t\t\t\treprepare_packed_git(r);\n+\t\t\t\tif (find_pack_entry(r, real, &e))\n+\t\t\t\t\tbreak;\n+\t\t\t}\n \t\t}\n \n \t\t/*\n-- \n2.39.0.rc0.267.gcb52ba06e7-goog\n\n"},{"id":"468657","messageId":"9ddfff3585c293c9801570e395b514505796a43f.1670373420.git.jonathantanmy@google.com","threadId":"58877","inReplyTo":"cover.1670373420.git.jonathantanmy@google.com","subject":"[PATCH v2 2/3] object-file: emit corruption errors when detected","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-12-07T00:40:52Z","receivedAt":"2022-12-07T00:41:16Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Instead of relying on errno being preserved across function calls, teach\ndo_oid_object_info_extended() to itself report object corruption when\nit first detects it. There are 3 types of corruption being detected:\n - when a replacement object is missing\n - when a loose object is corrupt\n - when a packed object is corrupt and the object cannot be read\n in another way\n\nNote that in the RHS of this patch's diff, a check for ENOENT that was\nintroduced in 3ba7a06552 (A loose object is not corrupt if it cannot\nbe read due to EMFILE, 2010-10-28) is also removed. The purpose of this\ncheck is to avoid a false report of corruption if the errno contains\nsomething like EMFILE (or anything that is not ENOENT), in which case\na more generic report is presented. Because, as of this patch, we no\nlonger rely on such a heuristic to determine corruption, but surface\nthe error message at the point when we read something that we did not\nexpect, this check is no longer necessary.\n\nBesides being more resilient, this also prepares for a future patch in\nwhich an indirect caller of do_oid_object_info_extended() will need\nsuch functionality.\n\nSigned-off-by: Jonathan Tan \n---\n object-file.c | 63 ++++++++++++++++++++++++++------------------------\n object-store.h | 3 +++\n 2 files changed, 36 insertions(+), 30 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex 596dd049fd..c7a513d123 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1215,7 +1215,8 @@ static int quick_has_loose(struct repository *r,\n * searching for a loose object named \"oid\".\n */\n static void *map_loose_object_1(struct repository *r, const char *path,\n-\t\t\t const struct object_id *oid, unsigned long *size)\n+\t\t\t\tconst struct object_id *oid, unsigned long *size,\n+\t\t\t\tchar **mapped_path)\n {\n \tvoid *map;\n \tint fd;\n@@ -1224,6 +1225,9 @@ static void *map_loose_object_1(struct repository *r, const char *path,\n \t\tfd = git_open(path);\n \telse\n \t\tfd = open_loose_object(r, oid, &path);\n+\tif (mapped_path)\n+\t\t*mapped_path = xstrdup(path);\n+\n \tmap = NULL;\n \tif (fd >= 0) {\n \t\tstruct stat st;\n@@ -1247,7 +1251,7 @@ void *map_loose_object(struct repository *r,\n \t\t const struct object_id *oid,\n \t\t unsigned long *size)\n {\n-\treturn map_loose_object_1(r, NULL, oid, size);\n+\treturn map_loose_object_1(r, NULL, oid, size, NULL);\n }\n \n enum unpack_loose_header_result unpack_loose_header(git_zstream *stream,\n@@ -1428,6 +1432,7 @@ static int loose_object_info(struct repository *r,\n {\n \tint status = 0;\n \tunsigned long mapsize;\n+\tchar *mapped_path = NULL;\n \tvoid *map;\n \tgit_zstream stream;\n \tchar hdr[MAX_HEADER_LEN];\n@@ -1459,9 +1464,11 @@ static int loose_object_info(struct repository *r,\n \t\treturn 0;\n \t}\n \n-\tmap = map_loose_object(r, oid, &mapsize);\n-\tif (!map)\n+\tmap = map_loose_object_1(r, NULL, oid, &mapsize, &mapped_path);\n+\tif (!map) {\n+\t\tfree(mapped_path);\n \t\treturn -1;\n+\t}\n \n \tif (!oi->sizep)\n \t\toi->sizep = &size_scratch;\n@@ -1497,8 +1504,13 @@ static int loose_object_info(struct repository *r,\n \t\tbreak;\n \t}\n \n+\tif (status && (flags & OBJECT_INFO_DIE_IF_CORRUPT))\n+\t\tdie(_(\"loose object %s (stored in %s) is corrupt\"),\n+\t\t oid_to_hex(oid), mapped_path);\n+\n \tgit_inflate_end(&stream);\n cleanup:\n+\tfree(mapped_path);\n \tmunmap(map, mapsize);\n \tif (oi->sizep == &size_scratch)\n \t\toi->sizep = NULL;\n@@ -1608,6 +1620,15 @@ static int do_oid_object_info_extended(struct repository *r,\n \t\t\tcontinue;\n \t\t}\n \n+\t\tif (flags & OBJECT_INFO_DIE_IF_CORRUPT) {\n+\t\t\tconst struct packed_git *p;\n+\t\t\tif ((flags & OBJECT_INFO_LOOKUP_REPLACE) && !oideq(real, oid))\n+\t\t\t\tdie(_(\"replacement %s not found for %s\"),\n+\t\t\t\t oid_to_hex(real), oid_to_hex(oid));\n+\t\t\tif ((p = has_packed_and_bad(r, real)))\n+\t\t\t\tdie(_(\"packed object %s (stored in %s) is corrupt\"),\n+\t\t\t\t oid_to_hex(real), p->pack_name);\n+\t\t}\n \t\treturn -1;\n \t}\n \n@@ -1660,7 +1681,8 @@ int oid_object_info(struct repository *r,\n \n static void *read_object(struct repository *r,\n \t\t\t const struct object_id *oid, enum object_type *type,\n-\t\t\t unsigned long *size)\n+\t\t\t unsigned long *size,\n+\t\t\t int die_if_corrupt)\n {\n \tstruct object_info oi = OBJECT_INFO_INIT;\n \tvoid *content;\n@@ -1668,7 +1690,9 @@ static void *read_object(struct repository *r,\n \toi.sizep = size;\n \toi.contentp = &content;\n \n-\tif (oid_object_info_extended(r, oid, &oi, 0) < 0)\n+\tif (oid_object_info_extended(r, oid, &oi,\n+\t\t\t\t die_if_corrupt ? OBJECT_INFO_DIE_IF_CORRUPT : 0)\n+\t < 0)\n \t\treturn NULL;\n \treturn content;\n }\n@@ -1704,35 +1728,14 @@ void *read_object_file_extended(struct repository *r,\n \t\t\t\tint lookup_replace)\n {\n \tvoid *data;\n-\tconst struct packed_git *p;\n-\tconst char *path;\n-\tstruct stat st;\n \tconst struct object_id *repl = lookup_replace ?\n \t\tlookup_replace_object(r, oid) : oid;\n \n \terrno = 0;\n-\tdata = read_object(r, repl, type, size);\n+\tdata = read_object(r, repl, type, size, 1);\n \tif (data)\n \t\treturn data;\n \n-\tobj_read_lock();\n-\tif (errno && errno != ENOENT)\n-\t\tdie_errno(_(\"failed to read object %s\"), oid_to_hex(oid));\n-\n-\t/* die if we replaced an object with one that does not exist */\n-\tif (repl != oid)\n-\t\tdie(_(\"replacement %s not found for %s\"),\n-\t\t oid_to_hex(repl), oid_to_hex(oid));\n-\n-\tif (!stat_loose_object(r, repl, &st, &path))\n-\t\tdie(_(\"loose object %s (stored in %s) is corrupt\"),\n-\t\t oid_to_hex(repl), path);\n-\n-\tif ((p = has_packed_and_bad(r, repl)))\n-\t\tdie(_(\"packed object %s (stored in %s) is corrupt\"),\n-\t\t oid_to_hex(repl), p->pack_name);\n-\tobj_read_unlock();\n-\n \treturn NULL;\n }\n \n@@ -2275,7 +2278,7 @@ int force_object_loose(const struct object_id *oid, time_t mtime)\n \n \tif (has_loose_object(oid))\n \t\treturn 0;\n-\tbuf = read_object(the_repository, oid, &type, &len);\n+\tbuf = read_object(the_repository, oid, &type, &len, 0);\n \tif (!buf)\n \t\treturn error(_(\"cannot read object for %s\"), oid_to_hex(oid));\n \thdrlen = format_object_header(hdr, sizeof(hdr), type, len);\n@@ -2797,7 +2800,7 @@ int read_loose_object(const char *path,\n \tchar hdr[MAX_HEADER_LEN];\n \tunsigned long *size = oi->sizep;\n \n-\tmap = map_loose_object_1(the_repository, path, NULL, &mapsize);\n+\tmap = map_loose_object_1(the_repository, path, NULL, &mapsize, NULL);\n \tif (!map) {\n \t\terror_errno(_(\"unable to mmap %s\"), path);\n \t\tgoto out;\ndiff --git a/object-store.h b/object-store.h\nindex 1be57abaf1..01134ab5ec 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -447,6 +447,9 @@ struct object_info {\n */\n #define OBJECT_INFO_FOR_PREFETCH (OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK)\n \n+/* Die if object corruption (not just an object being missing) was detected. */\n+#define OBJECT_INFO_DIE_IF_CORRUPT 64\n+\n int oid_object_info_extended(struct repository *r,\n \t\t\t const struct object_id *,\n \t\t\t struct object_info *, unsigned flags);\n-- \n2.39.0.rc0.267.gcb52ba06e7-goog\n\n"},{"id":"468658","messageId":"c5fe42deb04285b85d5354a57e90bf9410cc2420.1670373420.git.jonathantanmy@google.com","threadId":"58877","inReplyTo":"cover.1670373420.git.jonathantanmy@google.com","subject":"[PATCH v2 3/3] commit: don't lazy-fetch commits","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-12-07T00:40:53Z","receivedAt":"2022-12-07T00:41:18Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When parsing commits, fail fast when the commit is missing or\ncorrupt, instead of attempting to fetch them. This is done by inlining\nrepo_read_object_file() and setting the flag that prevents fetching.\n\nThis is motivated by a situation in which through a bug (not necessarily\nthrough Git), there was corruption in the object store of a partial\nclone. In this particular case, the problem was exposed when \"git gc\"\ntried to expire reflogs, which calls repo_parse_commit(), which triggers\nfetches of the missing commits.\n\n(There are other possible solutions to this problem including passing an\nargument from \"git gc\" to \"git reflog\" to inhibit all lazy fetches, but\nI think that this fix is at the wrong level - fixing \"git reflog\" means\nthat this particular command works fine, or so we think (it will fail if\nit somehow needs to read a legitimately missing blob, say, a .gitmodules\nfile), but fixing repo_parse_commit() will fix a whole class of bugs.)\n\nSigned-off-by: Jonathan Tan \nSigned-off-by: Junio C Hamano \n---\n commit.c | 15 +++++++++++++--\n 1 file changed, 13 insertions(+), 2 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex 572301b80a..a02723f06b 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -508,6 +508,17 @@ int repo_parse_commit_internal(struct repository *r,\n \tenum object_type type;\n \tvoid *buffer;\n \tunsigned long size;\n+\tstruct object_info oi = {\n+\t\t.typep = &type,\n+\t\t.sizep = &size,\n+\t\t.contentp = &buffer,\n+\t};\n+\t/*\n+\t * Git does not support partial clones that exclude commits, so set\n+\t * OBJECT_INFO_SKIP_FETCH_OBJECT to fail fast when an object is missing.\n+\t */\n+\tint flags = OBJECT_INFO_LOOKUP_REPLACE | OBJECT_INFO_SKIP_FETCH_OBJECT |\n+\t\tOBJECT_INFO_DIE_IF_CORRUPT;\n \tint ret;\n \n \tif (!item)\n@@ -516,8 +527,8 @@ int repo_parse_commit_internal(struct repository *r,\n \t\treturn 0;\n \tif (use_commit_graph && parse_commit_in_graph(r, item))\n \t\treturn 0;\n-\tbuffer = repo_read_object_file(r, &item->object.oid, &type, &size);\n-\tif (!buffer)\n+\n+\tif (oid_object_info_extended(r, &item->object.oid, &oi, flags) < 0)\n \t\treturn quiet_on_missing ? -1 :\n \t\t\terror(\"Could not read %s\",\n \t\t\t oid_to_hex(&item->object.oid));\n-- \n2.39.0.rc0.267.gcb52ba06e7-goog\n\n"},{"id":"468665","messageId":"xmqqy1rk6mqa.fsf@gitster.g","threadId":"58877","inReplyTo":"9ad34a1dce977044066de0bfa6e25977215e8dc9.1670373420.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 1/3] object-file: don't exit early if skipping loose","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-07T01:12:13Z","receivedAt":"2022-12-07T01:12:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> Instead, also search the submodule object stores and promisor remotes.\n>\n> This also centralizes what happens when the object is not found (the\n> \"return -1\"), which is useful for a subsequent patch.\n>\n> Signed-off-by: Jonathan Tan \n> ---\n> object-file.c | 23 +++++++++++------------\n> 1 file changed, 11 insertions(+), 12 deletions(-)\n>\n> diff --git a/object-file.c b/object-file.c\n> index 26290554bb..596dd049fd 100644\n> --- a/object-file.c\n> +++ b/object-file.c\n> @@ -1575,18 +1575,17 @@ static int do_oid_object_info_extended(struct repository *r,\n> \t\tif (find_pack_entry(r, real, &e))\n> \t\t\tbreak;\n> \n> -\t\tif (flags & OBJECT_INFO_IGNORE_LOOSE)\n> -\t\t\treturn -1;\n> -\n> -\t\t/* Most likely it's a loose object. */\n> -\t\tif (!loose_object_info(r, real, oi, flags))\n> -\t\t\treturn 0;\n> -\n> -\t\t/* Not a loose object; someone else may have just packed it. */\n> -\t\tif (!(flags & OBJECT_INFO_QUICK)) {\n> -\t\t\treprepare_packed_git(r);\n> -\t\t\tif (find_pack_entry(r, real, &e))\n> -\t\t\t\tbreak;\n> +\t\tif (!(flags & OBJECT_INFO_IGNORE_LOOSE)) {\n> +\t\t\t/* Most likely it's a loose object. */\n> +\t\t\tif (!loose_object_info(r, real, oi, flags))\n> +\t\t\t\treturn 0;\n> +\n> +\t\t\t/* Not a loose object; someone else may have just packed it. */\n> +\t\t\tif (!(flags & OBJECT_INFO_QUICK)) {\n> +\t\t\t\treprepare_packed_git(r);\n> +\t\t\t\tif (find_pack_entry(r, real, &e))\n> +\t\t\t\t\tbreak;\n> +\t\t\t}\n> \t\t}\n\nHmph, who passes IGNORE_LOOSE and why? Explaining the answer to\nthat question would give us confidence why this change is safe.\n\nIf the reason IGNORE_LOOSE is set by the callers is because they are\ninterested only in locally packed objects, then this change would\nbreak them because they end up triggering the lazy fetch in the\nupdated code, no? Or do all callers that set IGNORE_LOOSE drop the\nfetch_if_missing global before calling us?\n\nThanks.\n\n"},{"id":"468667","messageId":"xmqqtu286mjw.fsf@gitster.g","threadId":"58877","inReplyTo":"9ddfff3585c293c9801570e395b514505796a43f.1670373420.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 2/3] object-file: emit corruption errors when detected","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-07T01:16:03Z","receivedAt":"2022-12-07T01:16:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> Instead of relying on errno being preserved across function calls, teach\n> do_oid_object_info_extended() to itself report object corruption when\n> it first detects it. There are 3 types of corruption being detected:\n> - when a replacement object is missing\n> - when a loose object is corrupt\n> - when a packed object is corrupt and the object cannot be read\n> in another way\n>\n> Note that in the RHS of this patch's diff, a check for ENOENT that was\n> introduced in 3ba7a06552 (A loose object is not corrupt if it cannot\n> be read due to EMFILE, 2010-10-28) is also removed. The purpose of this\n> check is to avoid a false report of corruption if the errno contains\n> something like EMFILE (or anything that is not ENOENT), in which case\n> a more generic report is presented. Because, as of this patch, we no\n> longer rely on such a heuristic to determine corruption, but surface\n> the error message at the point when we read something that we did not\n> expect, this check is no longer necessary.\n>\n> Besides being more resilient, this also prepares for a future patch in\n> which an indirect caller of do_oid_object_info_extended() will need\n> such functionality.\n>\n> Signed-off-by: Jonathan Tan \n> ---\n> object-file.c | 63 ++++++++++++++++++++++++++------------------------\n> object-store.h | 3 +++\n> 2 files changed, 36 insertions(+), 30 deletions(-)\n\nThe implementation looks very straight-forward. Nicely done.\n"},{"id":"468668","messageId":"xmqqpmcw6mi9.fsf@gitster.g","threadId":"58877","inReplyTo":"c5fe42deb04285b85d5354a57e90bf9410cc2420.1670373420.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 3/3] commit: don't lazy-fetch commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-07T01:17:02Z","receivedAt":"2022-12-07T01:17:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> +\t/*\n> +\t * Git does not support partial clones that exclude commits, so set\n> +\t * OBJECT_INFO_SKIP_FETCH_OBJECT to fail fast when an object is missing.\n> +\t */\n> +\tint flags = OBJECT_INFO_LOOKUP_REPLACE | OBJECT_INFO_SKIP_FETCH_OBJECT |\n> +\t\tOBJECT_INFO_DIE_IF_CORRUPT;\n\nThat's a quite helpful comment.\n"},{"id":"468682","messageId":"221207.86359rc03e.gmgdl@evledraar.gmail.com","threadId":"58877","inReplyTo":"9ddfff3585c293c9801570e395b514505796a43f.1670373420.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 2/3] object-file: emit corruption errors when detected","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-07T04:05:47Z","receivedAt":"2022-12-07T04:26:32Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Dec 06 2022, Jonathan Tan wrote:\n\n> Instead of relying on errno being preserved across function calls, teach\n> do_oid_object_info_extended() to itself report object corruption when\n> it first detects it. There are 3 types of corruption being detected:\n> - when a replacement object is missing\n> - when a loose object is corrupt\n> - when a packed object is corrupt and the object cannot be read\n> in another way\n>\n> Note that in the RHS of this patch's diff, a check for ENOENT that was\n> introduced in 3ba7a06552 (A loose object is not corrupt if it cannot\n> be read due to EMFILE, 2010-10-28) is also removed. The purpose of this\n> check is to avoid a false report of corruption if the errno contains\n> something like EMFILE (or anything that is not ENOENT), in which case\n> a more generic report is presented. Because, as of this patch, we no\n> longer rely on such a heuristic to determine corruption, but surface\n> the error message at the point when we read something that we did not\n> expect, this check is no longer necessary.\n>\n> Besides being more resilient, this also prepares for a future patch in\n> which an indirect caller of do_oid_object_info_extended() will need\n> such functionality.\n>\n> Signed-off-by: Jonathan Tan \n> ---\n> object-file.c | 63 ++++++++++++++++++++++++++------------------------\n> object-store.h | 3 +++\n> 2 files changed, 36 insertions(+), 30 deletions(-)\n>\n> diff --git a/object-file.c b/object-file.c\n> index 596dd049fd..c7a513d123 100644\n> --- a/object-file.c\n> +++ b/object-file.c\n> @@ -1215,7 +1215,8 @@ static int quick_has_loose(struct repository *r,\n> * searching for a loose object named \"oid\".\n> */\n> static void *map_loose_object_1(struct repository *r, const char *path,\n> -\t\t\t const struct object_id *oid, unsigned long *size)\n> +\t\t\t\tconst struct object_id *oid, unsigned long *size,\n> +\t\t\t\tchar **mapped_path)\n> {\n> \tvoid *map;\n> \tint fd;\n> @@ -1224,6 +1225,9 @@ static void *map_loose_object_1(struct repository *r, const char *path,\n> \t\tfd = git_open(path);\n> \telse\n> \t\tfd = open_loose_object(r, oid, &path);\n> +\tif (mapped_path)\n> +\t\t*mapped_path = xstrdup(path);\n> +\n> \tmap = NULL;\n> \tif (fd >= 0) {\n> \t\tstruct stat st;\n\nI find this map_loose_object_1() function to be rather \"busy\". Part of\nit's the pre-image.\n\nCallers at the end of this series are:\n\n 1254: return map_loose_object_1(r, NULL, oid, size, NULL);\n 1467: map = map_loose_object_1(r, NULL, oid, &mapsize, &mapped_path);\n 2803: map = map_loose_object_1(the_repository, path, NULL, &mapsize, NULL);\n\nSo, either we know the path already, and we pass it in, or we don't know\nthe path, and may or may not be interested in what the path ends up\nbeing.\n\nWhich is why we pass in both a \"path\" and a \"mapped_path\".\n\nThen, somewhat confusingly (maybe I'm the only one who finds this odd\")\nthe \"path\" variable itself does double-duty within the function. If we\nhave a \"path\" already we leave it alone, but if we don't it's NULL, and\nthen we write our new path to it.\n\nWe *might* then have a path already, *and* write to the \"mapped_path\",\nbut in that case we'd be xstrdup() ing a string the user passed in. But\nthis API use would make no sense.\n\nSo shouldn't we at least have a:\n\n\tif (path && mapped_path)\n\t\tBUG(\"either tell me the path, or ask me, not both!\");\n\nBut I think it's better to just separate these concerns. Most of this\nrefactoring is good, but I think this bit went a step too far, and as a\nresult we now need to memory manage this \"mapped_path\". I.e. we'd get a\n\"struct strbuf\"'s \"buf\" before, but now loose_object_info() needs to\nhave it xstrdup()'d, just to free() it again.\n\nIsn't the below squashed in better? I.e. just always pass the \"path\",\nbut maybe pass a \"fd=0\", in which case the function might need to\ngit_open() it.\n\nThen have map_loose_object() and loose_object_info() call\nopen_loose_object(), and pass in the \"path\" and \"fd\".\n\ndiff --git a/object-file.c b/object-file.c\nindex c7a513d123e..24793e1b479 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1214,19 +1214,13 @@ static int quick_has_loose(struct repository *r,\n * Map the loose object at \"path\" if it is not NULL, or the path found by\n * searching for a loose object named \"oid\".\n */\n-static void *map_loose_object_1(struct repository *r, const char *path,\n-\t\t\t\tconst struct object_id *oid, unsigned long *size,\n-\t\t\t\tchar **mapped_path)\n+static void *map_loose_object_1(struct repository *r, const char *const path,\n+\t\t\t\tint fd, unsigned long *size)\n {\n \tvoid *map;\n-\tint fd;\n \n-\tif (path)\n+\tif (!fd)\n \t\tfd = git_open(path);\n-\telse\n-\t\tfd = open_loose_object(r, oid, &path);\n-\tif (mapped_path)\n-\t\t*mapped_path = xstrdup(path);\n \n \tmap = NULL;\n \tif (fd >= 0) {\n@@ -1251,7 +1245,10 @@ void *map_loose_object(struct repository *r,\n \t\t const struct object_id *oid,\n \t\t unsigned long *size)\n {\n-\treturn map_loose_object_1(r, NULL, oid, size, NULL);\n+\tconst char *path;\n+\tint fd = open_loose_object(r, oid, &path);\n+\n+\treturn map_loose_object_1(r, path,fd, size);\n }\n \n enum unpack_loose_header_result unpack_loose_header(git_zstream *stream,\n@@ -1432,7 +1429,6 @@ static int loose_object_info(struct repository *r,\n {\n \tint status = 0;\n \tunsigned long mapsize;\n-\tchar *mapped_path = NULL;\n \tvoid *map;\n \tgit_zstream stream;\n \tchar hdr[MAX_HEADER_LEN];\n@@ -1440,6 +1436,8 @@ static int loose_object_info(struct repository *r,\n \tunsigned long size_scratch;\n \tenum object_type type_scratch;\n \tint allow_unknown = flags & OBJECT_INFO_ALLOW_UNKNOWN_TYPE;\n+\tint fd;\n+\tconst char *path;\n \n \tif (oi->delta_base_oid)\n \t\toidclr(oi->delta_base_oid);\n@@ -1464,11 +1462,10 @@ static int loose_object_info(struct repository *r,\n \t\treturn 0;\n \t}\n \n-\tmap = map_loose_object_1(r, NULL, oid, &mapsize, &mapped_path);\n-\tif (!map) {\n-\t\tfree(mapped_path);\n+\tfd = open_loose_object(r, oid, &path);\n+\tmap = map_loose_object_1(r, path, fd, &mapsize);\n+\tif (!map)\n \t\treturn -1;\n-\t}\n \n \tif (!oi->sizep)\n \t\toi->sizep = &size_scratch;\n@@ -1506,11 +1503,10 @@ static int loose_object_info(struct repository *r,\n \n \tif (status && (flags & OBJECT_INFO_DIE_IF_CORRUPT))\n \t\tdie(_(\"loose object %s (stored in %s) is corrupt\"),\n-\t\t oid_to_hex(oid), mapped_path);\n+\t\t oid_to_hex(oid), path);\n \n \tgit_inflate_end(&stream);\n cleanup:\n-\tfree(mapped_path);\n \tmunmap(map, mapsize);\n \tif (oi->sizep == &size_scratch)\n \t\toi->sizep = NULL;\n@@ -2800,7 +2796,7 @@ int read_loose_object(const char *path,\n \tchar hdr[MAX_HEADER_LEN];\n \tunsigned long *size = oi->sizep;\n \n-\tmap = map_loose_object_1(the_repository, path, NULL, &mapsize, NULL);\n+\tmap = map_loose_object_1(the_repository, path, 0, &mapsize);\n \tif (!map) {\n \t\terror_errno(_(\"unable to mmap %s\"), path);\n \t\tgoto out;\n"},{"id":"468693","messageId":"Y5AvPjTjKPxq7vG8@coredump.intra.peff.net","threadId":"58877","inReplyTo":"xmqqy1rk6mqa.fsf@gitster.g","subject":"Re: [PATCH v2 1/3] object-file: don't exit early if skipping loose","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-12-07T06:14:22Z","receivedAt":"2022-12-07T06:14:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 07, 2022 at 10:12:13AM +0900, Junio C Hamano wrote:\n\n> Hmph, who passes IGNORE_LOOSE and why? Explaining the answer to\n> that question would give us confidence why this change is safe.\n> \n> If the reason IGNORE_LOOSE is set by the callers is because they are\n> interested only in locally packed objects, then this change would\n> break them because they end up triggering the lazy fetch in the\n> updated code, no? Or do all callers that set IGNORE_LOOSE drop the\n> fetch_if_missing global before calling us?\n\nI wondered who those callers might be, too, because it is such a weird\nthing for a caller to want to care about (usually we try to abstract the\nobject database).\n\nIt looks like the only user went away in 97b2fa08b6 (fetch-pack: drop\ncustom loose object cache, 2018-11-12). So I think we just want to drop\nit:\n\ndiff --git a/object-file.c b/object-file.c\nindex 26290554bb..cf724bc19b 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1575,9 +1575,6 @@ static int do_oid_object_info_extended(struct repository *r,\n \t\tif (find_pack_entry(r, real, &e))\n \t\t\tbreak;\n \n-\t\tif (flags & OBJECT_INFO_IGNORE_LOOSE)\n-\t\t\treturn -1;\n-\n \t\t/* Most likely it's a loose object. */\n \t\tif (!loose_object_info(r, real, oi, flags))\n \t\t\treturn 0;\ndiff --git a/object-store.h b/object-store.h\nindex 1be57abaf1..371629c1e1 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -434,8 +434,6 @@ struct object_info {\n #define OBJECT_INFO_ALLOW_UNKNOWN_TYPE 2\n /* Do not retry packed storage after checking packed and loose storage */\n #define OBJECT_INFO_QUICK 8\n-/* Do not check loose object */\n-#define OBJECT_INFO_IGNORE_LOOSE 16\n /*\n * Do not attempt to fetch the object if missing (even if fetch_is_missing is\n * nonzero).\n\n\nWe could also renumber the later flags to keep them compact, but I don't\nhave a strong opinion there.\n\n-Peff\n"},{"id":"468694","messageId":"Y5A11dOFgHP/ADcS@coredump.intra.peff.net","threadId":"58877","inReplyTo":"9ddfff3585c293c9801570e395b514505796a43f.1670373420.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 2/3] object-file: emit corruption errors when detected","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-12-07T06:42:29Z","receivedAt":"2022-12-07T06:42:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 06, 2022 at 04:40:52PM -0800, Jonathan Tan wrote:\n\n> Note that in the RHS of this patch's diff, a check for ENOENT that was\n> introduced in 3ba7a06552 (A loose object is not corrupt if it cannot\n> be read due to EMFILE, 2010-10-28) is also removed. The purpose of this\n> check is to avoid a false report of corruption if the errno contains\n> something like EMFILE (or anything that is not ENOENT), in which case\n> a more generic report is presented. Because, as of this patch, we no\n> longer rely on such a heuristic to determine corruption, but surface\n> the error message at the point when we read something that we did not\n> expect, this check is no longer necessary.\n\nYou're right that this will not say \"oops, the object is corrupted\" when\nwe get EMFILE, etc, which is what 3ba7a06552 was fixing. But I think\nafter your patch, we would also never actually say \"could not open\n$path: too many open descriptors\". I don't think that kicked in reliably\nwith the current code, but it seems like something we are losing in this\npatch.\n\nI.e., I think you'd want to also complain when map_loose_object()\nreturns anything but ENOENT. The errno value there is reliable-ish,\nthough it might be worth spending the extra work to preserve it across\nthe close() calls, just in case. Or if you split the open/mmap as Ævar\nsuggested, then it becomes:\n\n fd = open_loose_object(r, oid, &path);\n if (fd < 0) {\n\tif (errno != ENOENT)\n\t\terror_errno(\"unable to open loose object %s\", path);\n\treturn -1;\n }\n\n buf = map_loose_object_1(fd, path);\n if (!buf) {\n\t/* not much point in complaining here, as xmmap will die on\n\t * error. In theory this could catch fstat() problems, but\n\t * probably map_loose_object_1() itself should do so, since\n\t * it already is special-casing empty files.\n\t */\n close(fd);\n\treturn -1;\n }\n\n> diff --git a/object-file.c b/object-file.c\n> index 596dd049fd..c7a513d123 100644\n> --- a/object-file.c\n> +++ b/object-file.c\n> @@ -1215,7 +1215,8 @@ static int quick_has_loose(struct repository *r,\n> * searching for a loose object named \"oid\".\n> */\n> static void *map_loose_object_1(struct repository *r, const char *path,\n> -\t\t\t const struct object_id *oid, unsigned long *size)\n> +\t\t\t\tconst struct object_id *oid, unsigned long *size,\n> +\t\t\t\tchar **mapped_path)\n> {\n> \tvoid *map;\n> \tint fd;\n> @@ -1224,6 +1225,9 @@ static void *map_loose_object_1(struct repository *r, const char *path,\n> \t\tfd = git_open(path);\n> \telse\n> \t\tfd = open_loose_object(r, oid, &path);\n> +\tif (mapped_path)\n> +\t\t*mapped_path = xstrdup(path);\n> +\n\nThis introduces an extra malloc/free in every object lookup, even in the\nsuccess case where we don't even bother using the value. It's probably\nnot really noticeable, but it kind of feels wrong. Especially when\nopen_loose_object() already returns a pointer to long-ish term storage.\n\nOne solution is...\n\n> @@ -1497,8 +1504,13 @@ static int loose_object_info(struct repository *r,\n> \t\tbreak;\n> \t}\n> \n> +\tif (status && (flags & OBJECT_INFO_DIE_IF_CORRUPT))\n> +\t\tdie(_(\"loose object %s (stored in %s) is corrupt\"),\n> +\t\t oid_to_hex(oid), mapped_path);\n> +\n\n...we could just say \"loose object %s is corrupt\", which is generally\nsufficient (outside of alternates, you can only have one copy anyway).\n\nBut if we want to retain it, we could just redo the lookup in the error\npath, which is what the existing code (that you're deleting) does, via\nstat_loose_object(). That's even more wasteful than an extra\nmalloc/free, but you only pay the cost for a corrupted object. It's\nracy, of course, but probably not in a meaningful way in practice.\n\nAlternatively, I like Ævar's suggestion to just split\nmap_loose_object(). Then this code would naturally have the path, via\ncalling open_loose_object() itself.\n\n> diff --git a/object-store.h b/object-store.h\n> index 1be57abaf1..01134ab5ec 100644\n> --- a/object-store.h\n> +++ b/object-store.h\n> @@ -447,6 +447,9 @@ struct object_info {\n> */\n> #define OBJECT_INFO_FOR_PREFETCH (OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK)\n> \n> +/* Die if object corruption (not just an object being missing) was detected. */\n> +#define OBJECT_INFO_DIE_IF_CORRUPT 64\n\nI have a suspicion that the world would be a better place if these die()\ncalls simply went away, in favor of returning -1 up the stack. But I'm\nOK leaving it as-is for the sake of trying not to do too many things at\nonce (I probably wouldn't have even mentioned it, except that if we do\nwant to end up there in the long run, we'd eventually have to rip out\nthis new flag and its associated plumbing).\n\n-Peff\n"},{"id":"468695","messageId":"xmqqy1rj67dz.fsf@gitster.g","threadId":"58877","inReplyTo":"Y5AvPjTjKPxq7vG8@coredump.intra.peff.net","subject":"Re: [PATCH v2 1/3] object-file: don't exit early if skipping loose","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-07T06:43:36Z","receivedAt":"2022-12-07T06:43:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I wondered who those callers might be, too, because it is such a weird\n> thing for a caller to want to care about (usually we try to abstract the\n> object database).\n\nExactly.\n\n> It looks like the only user went away in 97b2fa08b6 (fetch-pack: drop\n> custom loose object cache, 2018-11-12).\n\nNice, very nice.\n\n> So I think we just want to drop it:\n>\n> diff --git a/object-file.c b/object-file.c\n> index 26290554bb..cf724bc19b 100644\n> --- a/object-file.c\n> +++ b/object-file.c\n> @@ -1575,9 +1575,6 @@ static int do_oid_object_info_extended(struct repository *r,\n> \t\tif (find_pack_entry(r, real, &e))\n> \t\t\tbreak;\n> \n> -\t\tif (flags & OBJECT_INFO_IGNORE_LOOSE)\n> -\t\t\treturn -1;\n> -\n> \t\t/* Most likely it's a loose object. */\n> \t\tif (!loose_object_info(r, real, oi, flags))\n> \t\t\treturn 0;\n> diff --git a/object-store.h b/object-store.h\n> index 1be57abaf1..371629c1e1 100644\n> --- a/object-store.h\n> +++ b/object-store.h\n> @@ -434,8 +434,6 @@ struct object_info {\n> #define OBJECT_INFO_ALLOW_UNKNOWN_TYPE 2\n> /* Do not retry packed storage after checking packed and loose storage */\n> #define OBJECT_INFO_QUICK 8\n> -/* Do not check loose object */\n> -#define OBJECT_INFO_IGNORE_LOOSE 16\n> /*\n> * Do not attempt to fetch the object if missing (even if fetch_is_missing is\n> * nonzero).\n\nThis would make Jonathan's change a lot transparent and intuitive if\nit is based on it, I would think.\n\nThanks for digging.\n"},{"id":"468696","messageId":"Y5A3EkxY8p6XptWt@coredump.intra.peff.net","threadId":"58877","inReplyTo":"c5fe42deb04285b85d5354a57e90bf9410cc2420.1670373420.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 3/3] commit: don't lazy-fetch commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-12-07T06:47:46Z","receivedAt":"2022-12-07T06:47:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 06, 2022 at 04:40:53PM -0800, Jonathan Tan wrote:\n\n> @@ -516,8 +527,8 @@ int repo_parse_commit_internal(struct repository *r,\n> \t\treturn 0;\n> \tif (use_commit_graph && parse_commit_in_graph(r, item))\n> \t\treturn 0;\n> -\tbuffer = repo_read_object_file(r, &item->object.oid, &type, &size);\n> -\tif (!buffer)\n> +\n> +\tif (oid_object_info_extended(r, &item->object.oid, &oi, flags) < 0)\n\nNice. And this swap-out is much more obviously correct in this version\nof the series because read_object_file_extended() is now clearly a thin\nwrapper around oid_object_info_extended(), after your patch 2.\n\nI actually think it would be beneficial to do a bit more refactoring\nthere to eliminate read_object() entirely (in favor of just having the\ntwo callers use oid_object_info_extended() directly), and having\nrepo_read_object_extended() pass OBJECT_INFO_LOOKUP_REPLACE instead of\ndoing its own lookup.\n\nBut as that's all orthogonal to your goal, I don't mind if we punt on it\nfor now. We can do it later on top.\n\n-Peff\n"},{"id":"468697","messageId":"Y5A7qOaxyWxHJiex@coredump.intra.peff.net","threadId":"58877","inReplyTo":"221207.86359rc03e.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 2/3] object-file: emit corruption errors when detected","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-12-07T07:07:20Z","receivedAt":"2022-12-07T07:07:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 07, 2022 at 05:05:47AM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> Isn't the below squashed in better? I.e. just always pass the \"path\",\n> but maybe pass a \"fd=0\", in which case the function might need to\n> git_open() it.\n> \n> Then have map_loose_object() and loose_object_info() call\n> open_loose_object(), and pass in the \"path\" and \"fd\".\n\nI like this direction, though I'd give a few small suggestions. One is\nto make it unconditional to pass in a valid \"fd\". These kind of magic\nsentinel values sometimes lead to confusion or bugs, and it's easy\nenough for the caller to use git_open() itself.\n\nIn fact, in the one caller who cares, it lets us produce a nicer\nerror message:\n\ndiff --git a/object-file.c b/object-file.c\nindex 24793e1b47..7c2a85132b 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1219,9 +1219,6 @@ static void *map_loose_object_1(struct repository *r, const char *const path,\n {\n \tvoid *map;\n \n-\tif (!fd)\n-\t\tfd = git_open(path);\n-\n \tmap = NULL;\n \tif (fd >= 0) {\n \t\tstruct stat st;\n@@ -2790,13 +2787,18 @@ int read_loose_object(const char *path,\n \t\t struct object_info *oi)\n {\n \tint ret = -1;\n+\tint fd;\n \tvoid *map = NULL;\n \tunsigned long mapsize;\n \tgit_zstream stream;\n \tchar hdr[MAX_HEADER_LEN];\n \tunsigned long *size = oi->sizep;\n \n-\tmap = map_loose_object_1(the_repository, path, 0, &mapsize);\n+\tfd = git_open(path);\n+\tif (fd < 0)\n+\t\terror_errno(_(\"unable to open %s\"), path);\n+\n+\tmap = map_loose_object_1(the_repository, path, fd, &mapsize);\n \tif (!map) {\n \t\terror_errno(_(\"unable to mmap %s\"), path);\n \t\tgoto out;\n\n> +static void *map_loose_object_1(struct repository *r, const char *const path,\n> +\t\t\t\tint fd, unsigned long *size)\n> {\n> \tvoid *map;\n> -\tint fd;\n> \n> -\tif (path)\n> +\tif (!fd)\n> \t\tfd = git_open(path);\n> -\telse\n> -\t\tfd = open_loose_object(r, oid, &path);\n> -\tif (mapped_path)\n> -\t\t*mapped_path = xstrdup(path);\n\nThe other weird thing here is ownership of \"fd\". Now some callers pass\nit in, but map_loose_object_1() always closes it. I think that's OK\n(since we want it closed even on success), but definitely surprising\nenough that we'd want to document that in a comment.\n\n> @@ -1251,7 +1245,10 @@ void *map_loose_object(struct repository *r,\n> \t\t const struct object_id *oid,\n> \t\t unsigned long *size)\n> {\n> -\treturn map_loose_object_1(r, NULL, oid, size, NULL);\n> +\tconst char *path;\n> +\tint fd = open_loose_object(r, oid, &path);\n> +\n> +\treturn map_loose_object_1(r, path,fd, size);\n> }\n\nIt's also kind of weird that map_loose_object_1() is a noop on a\nnegative descriptor. That technically makes this correct, but I think it\nwould be much less surprising to always take a valid descriptor, and\nthis code should do:\n\n if (fd)\n\treturn -1;\n return map_loose_object_1(r, path, fd, size);\n\nIf we are going to make map_loose_object_1() less confusing (and I think\nthat is worth doing), let's go all the way.\n\n-Peff\n"},{"id":"468702","messageId":"221207.86pmcva2s8.gmgdl@evledraar.gmail.com","threadId":"58877","inReplyTo":"Y5A7qOaxyWxHJiex@coredump.intra.peff.net","subject":"Re: [PATCH v2 2/3] object-file: emit corruption errors when detected","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-07T10:33:47Z","receivedAt":"2022-12-07T11:10:41Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Dec 07 2022, Jeff King wrote:\n\n> On Wed, Dec 07, 2022 at 05:05:47AM +0100, Ævar Arnfjörð Bjarmason wrote:\n>\n>> Isn't the below squashed in better? I.e. just always pass the \"path\",\n>> but maybe pass a \"fd=0\", in which case the function might need to\n>> git_open() it.\n>> \n>> Then have map_loose_object() and loose_object_info() call\n>> open_loose_object(), and pass in the \"path\" and \"fd\".\n>\n> I like this direction, though I'd give a few small suggestions. One is\n> to make it unconditional to pass in a valid \"fd\". These kind of magic\n> sentinel values sometimes lead to confusion or bugs, and it's easy\n> enough for the caller to use git_open() itself.\n>\n> In fact, in the one caller who cares, it lets us produce a nicer\n> error message:\n>\n> diff --git a/object-file.c b/object-file.c\n> index 24793e1b47..7c2a85132b 100644\n> --- a/object-file.c\n> +++ b/object-file.c\n> @@ -1219,9 +1219,6 @@ static void *map_loose_object_1(struct repository *r, const char *const path,\n> {\n> \tvoid *map;\n> \n> -\tif (!fd)\n> -\t\tfd = git_open(path);\n> -\n> \tmap = NULL;\n> \tif (fd >= 0) {\n> \t\tstruct stat st;\n> @@ -2790,13 +2787,18 @@ int read_loose_object(const char *path,\n> \t\t struct object_info *oi)\n> {\n> \tint ret = -1;\n> +\tint fd;\n> \tvoid *map = NULL;\n> \tunsigned long mapsize;\n> \tgit_zstream stream;\n> \tchar hdr[MAX_HEADER_LEN];\n> \tunsigned long *size = oi->sizep;\n> \n> -\tmap = map_loose_object_1(the_repository, path, 0, &mapsize);\n> +\tfd = git_open(path);\n> +\tif (fd < 0)\n> +\t\terror_errno(_(\"unable to open %s\"), path);\n> +\n> +\tmap = map_loose_object_1(the_repository, path, fd, &mapsize);\n> \tif (!map) {\n> \t\terror_errno(_(\"unable to mmap %s\"), path);\n> \t\tgoto out;\n\nYeah, I think that's even better, although...\n\n>> +static void *map_loose_object_1(struct repository *r, const char *const path,\n>> +\t\t\t\tint fd, unsigned long *size)\n>> {\n>> \tvoid *map;\n>> -\tint fd;\n>> \n>> -\tif (path)\n>> +\tif (!fd)\n>> \t\tfd = git_open(path);\n>> -\telse\n>> -\t\tfd = open_loose_object(r, oid, &path);\n>> -\tif (mapped_path)\n>> -\t\t*mapped_path = xstrdup(path);\n>\n> The other weird thing here is ownership of \"fd\". Now some callers pass\n> it in, but map_loose_object_1() always closes it. I think that's OK\n> (since we want it closed even on success), but definitely surprising\n> enough that we'd want to document that in a comment.\n>\n>> @@ -1251,7 +1245,10 @@ void *map_loose_object(struct repository *r,\n>> \t\t const struct object_id *oid,\n>> \t\t unsigned long *size)\n>> {\n>> -\treturn map_loose_object_1(r, NULL, oid, size, NULL);\n>> +\tconst char *path;\n>> +\tint fd = open_loose_object(r, oid, &path);\n>> +\n>> +\treturn map_loose_object_1(r, path,fd, size);\n>> }\n>\n> It's also kind of weird that map_loose_object_1() is a noop on a\n> negative descriptor. That technically makes this correct, but I think it\n> would be much less surprising to always take a valid descriptor, and\n> this code should do:\n>\n> if (fd)\n> \treturn -1;\n> return map_loose_object_1(r, path, fd, size);\n>\n> If we are going to make map_loose_object_1() less confusing (and I think\n> that is worth doing), let's go all the way.\n\n...maybe we should go further in the other direction. I.e. with my\nearlier suggestion we're left with the mess that the \"fd\" ownership\nisn't clear.\n\nBut what I was trying to do was fix up the ownership around the\n\"mapped_path\", but we don't need to xstrdup() it in the first place. We\nalready have the caller of open_loose_object() not doing that, we can\njust say that you're not going to open two loose objects at a time.\n\nThen this becomes easier, and we can just pass the maybe-NULL \"const\nchar **oid_path\" all the way to open_loose_object():\n\n\ndiff --git a/object-file.c b/object-file.c\nindex c7a513d123e..6e900737b76 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1176,7 +1176,7 @@ static int stat_loose_object(struct repository *r, const struct object_id *oid,\n * descriptor. See the caveats on the \"path\" parameter above.\n */\n static int open_loose_object(struct repository *r,\n-\t\t\t const struct object_id *oid, const char **path)\n+\t\t\t const struct object_id *oid, const char **oid_path)\n {\n \tint fd;\n \tstruct object_directory *odb;\n@@ -1185,8 +1185,12 @@ static int open_loose_object(struct repository *r,\n \n \tprepare_alt_odb(r);\n \tfor (odb = r->objects->odb; odb; odb = odb->next) {\n-\t\t*path = odb_loose_path(odb, &buf, oid);\n-\t\tfd = git_open(*path);\n+\t\tconst char *path;\n+\n+\t\tpath = odb_loose_path(odb, &buf, oid);\n+\t\tif (oid_path)\n+\t\t\t*oid_path = path;\n+\t\tfd = git_open(path);\n \t\tif (fd >= 0)\n \t\t\treturn fd;\n \n@@ -1214,19 +1218,22 @@ static int quick_has_loose(struct repository *r,\n * Map the loose object at \"path\" if it is not NULL, or the path found by\n * searching for a loose object named \"oid\".\n */\n-static void *map_loose_object_1(struct repository *r, const char *path,\n+static void *map_loose_object_1(struct repository *r, const char *const path,\n \t\t\t\tconst struct object_id *oid, unsigned long *size,\n-\t\t\t\tchar **mapped_path)\n+\t\t\t\tconst char **oid_path)\n {\n \tvoid *map;\n \tint fd;\n \n+\tif (path && oid_path)\n+\t\tBUG(\"don't tell me about the path, and ask me what it is!\");\n+\telse if (!(path || oid))\n+\t\tBUG(\"must get an OID or a path!\");\n+\n \tif (path)\n \t\tfd = git_open(path);\n \telse\n-\t\tfd = open_loose_object(r, oid, &path);\n-\tif (mapped_path)\n-\t\t*mapped_path = xstrdup(path);\n+\t\tfd = open_loose_object(r, oid, oid_path);\n \n \tmap = NULL;\n \tif (fd >= 0) {\n@@ -1236,7 +1243,8 @@ static void *map_loose_object_1(struct repository *r, const char *path,\n \t\t\t*size = xsize_t(st.st_size);\n \t\t\tif (!*size) {\n \t\t\t\t/* mmap() is forbidden on empty files */\n-\t\t\t\terror(_(\"object file %s is empty\"), path);\n+\t\t\t\terror(_(\"object file %s is empty\"),\n+\t\t\t\t path ? path : *oid_path);\n \t\t\t\tclose(fd);\n \t\t\t\treturn NULL;\n \t\t\t}\n@@ -1432,7 +1440,7 @@ static int loose_object_info(struct repository *r,\n {\n \tint status = 0;\n \tunsigned long mapsize;\n-\tchar *mapped_path = NULL;\n+\tconst char *oid_path;\n \tvoid *map;\n \tgit_zstream stream;\n \tchar hdr[MAX_HEADER_LEN];\n@@ -1464,11 +1472,9 @@ static int loose_object_info(struct repository *r,\n \t\treturn 0;\n \t}\n \n-\tmap = map_loose_object_1(r, NULL, oid, &mapsize, &mapped_path);\n-\tif (!map) {\n-\t\tfree(mapped_path);\n+\tmap = map_loose_object_1(r, NULL, oid, &mapsize, &oid_path);\n+\tif (!map)\n \t\treturn -1;\n-\t}\n \n \tif (!oi->sizep)\n \t\toi->sizep = &size_scratch;\n@@ -1506,11 +1512,10 @@ static int loose_object_info(struct repository *r,\n \n \tif (status && (flags & OBJECT_INFO_DIE_IF_CORRUPT))\n \t\tdie(_(\"loose object %s (stored in %s) is corrupt\"),\n-\t\t oid_to_hex(oid), mapped_path);\n+\t\t oid_to_hex(oid), oid_path);\n \n \tgit_inflate_end(&stream);\n cleanup:\n-\tfree(mapped_path);\n \tmunmap(map, mapsize);\n \tif (oi->sizep == &size_scratch)\n \t\toi->sizep = NULL;\n\n\n\n\n\n"},{"id":"468747","messageId":"20221207232035.1438092-1-jonathantanmy@google.com","threadId":"58877","inReplyTo":"xmqqy1rj67dz.fsf@gitster.g","subject":"Re: [PATCH v2 1/3] object-file: don't exit early if skipping loose","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-12-07T23:20:34Z","receivedAt":"2022-12-07T23:20:43Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Junio C Hamano writes:\n> > It looks like the only user went away in 97b2fa08b6 (fetch-pack: drop\n> > custom loose object cache, 2018-11-12).\n> \n> Nice, very nice.\n> \n> > So I think we just want to drop it:\n\n[snip]\n\n> This would make Jonathan's change a lot transparent and intuitive if\n> it is based on it, I would think.\n> \n> Thanks for digging.\n\nAh, thanks for finding this. I'll make this change.\n"},{"id":"468748","messageId":"20221207232623.1439026-1-jonathantanmy@google.com","threadId":"58877","inReplyTo":"221207.86pmcva2s8.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 2/3] object-file: emit corruption errors when detected","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-12-07T23:26:23Z","receivedAt":"2022-12-07T23:26:51Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Yeah, I think that's even better, although...\n\n[snip]\n \n> > It's also kind of weird that map_loose_object_1() is a noop on a\n> > negative descriptor. That technically makes this correct, but I think it\n> > would be much less surprising to always take a valid descriptor, and\n> > this code should do:\n> >\n> > if (fd)\n> > \treturn -1;\n> > return map_loose_object_1(r, path, fd, size);\n> >\n> > If we are going to make map_loose_object_1() less confusing (and I think\n> > that is worth doing), let's go all the way.\n> \n> ...maybe we should go further in the other direction. I.e. with my\n> earlier suggestion we're left with the mess that the \"fd\" ownership\n> isn't clear.\n\nWith Peff's suggestion I think we can make the function always close\nthe fd. If we find it not to be clear, we can rename the function\nto ..._close_fd() or something like that.\n\nThanks to both of you for your suggestions.\n"},{"id":"468752","messageId":"221208.86edta93e5.gmgdl@evledraar.gmail.com","threadId":"58877","inReplyTo":"20221207232623.1439026-1-jonathantanmy@google.com","subject":"Re: [PATCH v2 2/3] object-file: emit corruption errors when detected","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-07T23:50:27Z","receivedAt":"2022-12-07T23:54:00Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Dec 07 2022, Jonathan Tan wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> Yeah, I think that's even better, although...\n>\n> [snip]\n> \n>> > It's also kind of weird that map_loose_object_1() is a noop on a\n>> > negative descriptor. That technically makes this correct, but I think it\n>> > would be much less surprising to always take a valid descriptor, and\n>> > this code should do:\n>> >\n>> > if (fd)\n>> > \treturn -1;\n>> > return map_loose_object_1(r, path, fd, size);\n>> >\n>> > If we are going to make map_loose_object_1() less confusing (and I think\n>> > that is worth doing), let's go all the way.\n>> \n>> ...maybe we should go further in the other direction. I.e. with my\n>> earlier suggestion we're left with the mess that the \"fd\" ownership\n>> isn't clear.\n>\n> With Peff's suggestion I think we can make the function always close\n> the fd. If we find it not to be clear, we can rename the function\n> to ..._close_fd() or something like that.\n>\n> Thanks to both of you for your suggestions.\n\nI think that was my suggestion.\n\nPeff's on top of that was to never have it *open* the fd, but I'd left\none caller doing that.\n\nI.e. that the ownership would still be passed to it, but at least it\nwould always be passed, and wouldn't be the mixed affair that my initial\nsuggestion left it at.\n\nI'll leave it to you to pick through this.\n\nI have a mild preference for my latest suggestion as the ownership of\nall the variables seems cleanest in that iteration. I.e. we don't need\nto xstrdup(), and the \"fd\" is always contained within\nmap_loose_object_1().\n\nWe still have the \"sometimes a path, sometimes I make a path from an\noid\" semantics though, but that seems unavoidable.\n\n"},{"id":"468766","messageId":"Y5GFSqKG1org13lc@coredump.intra.peff.net","threadId":"58877","inReplyTo":"221208.86edta93e5.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 2/3] object-file: emit corruption errors when detected","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-12-08T06:33:46Z","receivedAt":"2022-12-08T06:33:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 08, 2022 at 12:50:27AM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> I have a mild preference for my latest suggestion as the ownership of\n> all the variables seems cleanest in that iteration. I.e. we don't need\n> to xstrdup(), and the \"fd\" is always contained within\n> map_loose_object_1().\n> \n> We still have the \"sometimes a path, sometimes I make a path from an\n> oid\" semantics though, but that seems unavoidable.\n\nOf the two warts, I think \"this function consume the fd\" is less weird\nthan the two path variables (one sometimes-in and one sometimes-out).\nIf the fd thing is too ugly, we could have the function _not_ consume\nthe fd, but I think that probably makes the callers worse.\n\nAt any rate, we can wait and see what Jonathan comes up with.\n\n(As an aside, thank you Jonathan for dealing with some of this\nlong-standing ugliness; it is not directly related to your goal, but I\nthink it's adjacent enough to merit doing it as part of the series).\n\n-Peff\n"},{"id":"468796","messageId":"cover.1670532905.git.jonathantanmy@google.com","threadId":"58877","inReplyTo":"cover.1669839849.git.jonathantanmy@google.com","subject":"[PATCH v3 0/4] Don't lazy-fetch commits when parsing them","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-12-08T20:57:04Z","receivedAt":"2022-12-08T20:57:27Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Thanks everyone for your review. map_loose_object_1() definitely looks\nless \"busy\" than before after following your suggestions.\n\nJonathan Tan (4):\n object-file: remove OBJECT_INFO_IGNORE_LOOSE\n object-file: refactor map_loose_object_1()\n object-file: emit corruption errors when detected\n commit: don't lazy-fetch commits\n\n commit.c | 15 ++++++-\n object-file.c | 111 +++++++++++++++++++++++++------------------------\n object-store.h | 7 ++--\n 3 files changed, 73 insertions(+), 60 deletions(-)\n\nRange-diff against v2:\n1: 9ad34a1dce < -: ---------- object-file: don't exit early if skipping loose\n-: ---------- > 1: be0b08cac2 object-file: remove OBJECT_INFO_IGNORE_LOOSE\n-: ---------- > 2: 7419e4ac70 object-file: refactor map_loose_object_1()\n2: 9ddfff3585 ! 3: 7c9ed861e7 object-file: emit corruption errors when detected\n @@ Commit message\n Signed-off-by: Jonathan Tan \n \n ## object-file.c ##\n -@@ object-file.c: static int quick_has_loose(struct repository *r,\n - * searching for a loose object named \"oid\".\n - */\n - static void *map_loose_object_1(struct repository *r, const char *path,\n --\t\t\t const struct object_id *oid, unsigned long *size)\n -+\t\t\t\tconst struct object_id *oid, unsigned long *size,\n -+\t\t\t\tchar **mapped_path)\n - {\n - \tvoid *map;\n - \tint fd;\n -@@ object-file.c: static void *map_loose_object_1(struct repository *r, const char *path,\n - \t\tfd = git_open(path);\n - \telse\n - \t\tfd = open_loose_object(r, oid, &path);\n -+\tif (mapped_path)\n -+\t\t*mapped_path = xstrdup(path);\n -+\n - \tmap = NULL;\n - \tif (fd >= 0) {\n - \t\tstruct stat st;\n -@@ object-file.c: void *map_loose_object(struct repository *r,\n - \t\t const struct object_id *oid,\n - \t\t unsigned long *size)\n - {\n --\treturn map_loose_object_1(r, NULL, oid, size);\n -+\treturn map_loose_object_1(r, NULL, oid, size, NULL);\n - }\n - \n - enum unpack_loose_header_result unpack_loose_header(git_zstream *stream,\n @@ object-file.c: static int loose_object_info(struct repository *r,\n {\n \tint status = 0;\n \tunsigned long mapsize;\n -+\tchar *mapped_path = NULL;\n ++\tconst char *path = NULL;\n \tvoid *map;\n \tgit_zstream stream;\n \tchar hdr[MAX_HEADER_LEN];\n @@ object-file.c: static int loose_object_info(struct repository *r,\n \t}\n \n -\tmap = map_loose_object(r, oid, &mapsize);\n --\tif (!map)\n -+\tmap = map_loose_object_1(r, NULL, oid, &mapsize, &mapped_path);\n -+\tif (!map) {\n -+\t\tfree(mapped_path);\n ++\tmap = map_loose_object_1(r, oid, &mapsize, &path);\n + \tif (!map)\n \t\treturn -1;\n -+\t}\n \n - \tif (!oi->sizep)\n - \t\toi->sizep = &size_scratch;\n @@ object-file.c: static int loose_object_info(struct repository *r,\n \t\tbreak;\n \t}\n \n +\tif (status && (flags & OBJECT_INFO_DIE_IF_CORRUPT))\n +\t\tdie(_(\"loose object %s (stored in %s) is corrupt\"),\n -+\t\t oid_to_hex(oid), mapped_path);\n ++\t\t oid_to_hex(oid), path);\n +\n \tgit_inflate_end(&stream);\n cleanup:\n -+\tfree(mapped_path);\n \tmunmap(map, mapsize);\n - \tif (oi->sizep == &size_scratch)\n - \t\toi->sizep = NULL;\n @@ object-file.c: static int do_oid_object_info_extended(struct repository *r,\n \t\t\tcontinue;\n \t\t}\n @@ object-file.c: int force_object_loose(const struct object_id *oid, time_t mtime)\n \tif (!buf)\n \t\treturn error(_(\"cannot read object for %s\"), oid_to_hex(oid));\n \thdrlen = format_object_header(hdr, sizeof(hdr), type, len);\n -@@ object-file.c: int read_loose_object(const char *path,\n - \tchar hdr[MAX_HEADER_LEN];\n - \tunsigned long *size = oi->sizep;\n - \n --\tmap = map_loose_object_1(the_repository, path, NULL, &mapsize);\n -+\tmap = map_loose_object_1(the_repository, path, NULL, &mapsize, NULL);\n - \tif (!map) {\n - \t\terror_errno(_(\"unable to mmap %s\"), path);\n - \t\tgoto out;\n \n ## object-store.h ##\n @@ object-store.h: struct object_info {\n @@ object-store.h: struct object_info {\n #define OBJECT_INFO_FOR_PREFETCH (OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK)\n \n +/* Die if object corruption (not just an object being missing) was detected. */\n -+#define OBJECT_INFO_DIE_IF_CORRUPT 64\n ++#define OBJECT_INFO_DIE_IF_CORRUPT 32\n +\n int oid_object_info_extended(struct repository *r,\n \t\t\t const struct object_id *,\n3: c5fe42deb0 = 4: 5924a5120b commit: don't lazy-fetch commits\n-- \n2.39.0.rc1.256.g54fd8350bd-goog\n\n"},{"id":"468797","messageId":"be0b08cac219357e1ce9cd46fd7ab5c13344699b.1670532905.git.jonathantanmy@google.com","threadId":"58877","inReplyTo":"cover.1670532905.git.jonathantanmy@google.com","subject":"[PATCH v3 1/4] object-file: remove OBJECT_INFO_IGNORE_LOOSE","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-12-08T20:57:05Z","receivedAt":"2022-12-08T20:57:29Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Its last user was removed in 97b2fa08b6 (fetch-pack: drop\ncustom loose object cache, 2018-11-12), so we can remove it.\n\nHelped-by: Jeff King \nSigned-off-by: Jonathan Tan \n---\n object-file.c | 3 ---\n object-store.h | 4 +---\n 2 files changed, 1 insertion(+), 6 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex 26290554bb..cf724bc19b 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1575,9 +1575,6 @@ static int do_oid_object_info_extended(struct repository *r,\n \t\tif (find_pack_entry(r, real, &e))\n \t\t\tbreak;\n \n-\t\tif (flags & OBJECT_INFO_IGNORE_LOOSE)\n-\t\t\treturn -1;\n-\n \t\t/* Most likely it's a loose object. */\n \t\tif (!loose_object_info(r, real, oi, flags))\n \t\t\treturn 0;\ndiff --git a/object-store.h b/object-store.h\nindex 1be57abaf1..b1ec0bde82 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -434,13 +434,11 @@ struct object_info {\n #define OBJECT_INFO_ALLOW_UNKNOWN_TYPE 2\n /* Do not retry packed storage after checking packed and loose storage */\n #define OBJECT_INFO_QUICK 8\n-/* Do not check loose object */\n-#define OBJECT_INFO_IGNORE_LOOSE 16\n /*\n * Do not attempt to fetch the object if missing (even if fetch_is_missing is\n * nonzero).\n */\n-#define OBJECT_INFO_SKIP_FETCH_OBJECT 32\n+#define OBJECT_INFO_SKIP_FETCH_OBJECT 16\n /*\n * This is meant for bulk prefetching of missing blobs in a partial\n * clone. Implies OBJECT_INFO_SKIP_FETCH_OBJECT and OBJECT_INFO_QUICK\n-- \n2.39.0.rc1.256.g54fd8350bd-goog\n\n"},{"id":"468798","messageId":"7419e4ac7053ab2d89a4cdc4612e5baeca48ce9f.1670532905.git.jonathantanmy@google.com","threadId":"58877","inReplyTo":"cover.1670532905.git.jonathantanmy@google.com","subject":"[PATCH v3 2/4] object-file: refactor map_loose_object_1()","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-12-08T20:57:06Z","receivedAt":"2022-12-08T20:57:38Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"This function can do 3 things:\n 1. Gets an fd given a path\n 2. Simultaneously gets a path and fd given an OID\n 3. Memory maps an fd\n\nSplit this function up. Only one caller needs 1, so inline that. As for\n2, a future patch will also need this functionality and, in addition,\nthe calculated path, so extract this into a separate function with an\nout parameter for the path.\n\nSigned-off-by: Jonathan Tan \n---\n object-file.c | 60 +++++++++++++++++++++++++++++----------------------\n 1 file changed, 34 insertions(+), 26 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex cf724bc19b..d99d05839f 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1211,43 +1211,48 @@ static int quick_has_loose(struct repository *r,\n }\n \n /*\n- * Map the loose object at \"path\" if it is not NULL, or the path found by\n- * searching for a loose object named \"oid\".\n+ * Map and close the given loose object fd. The path argument is used for\n+ * error reporting.\n */\n-static void *map_loose_object_1(struct repository *r, const char *path,\n-\t\t\t const struct object_id *oid, unsigned long *size)\n+static void *map_fd(int fd, const char *path, unsigned long *size)\n {\n-\tvoid *map;\n-\tint fd;\n-\n-\tif (path)\n-\t\tfd = git_open(path);\n-\telse\n-\t\tfd = open_loose_object(r, oid, &path);\n-\tmap = NULL;\n-\tif (fd >= 0) {\n-\t\tstruct stat st;\n+\tvoid *map = NULL;\n+\tstruct stat st;\n \n-\t\tif (!fstat(fd, &st)) {\n-\t\t\t*size = xsize_t(st.st_size);\n-\t\t\tif (!*size) {\n-\t\t\t\t/* mmap() is forbidden on empty files */\n-\t\t\t\terror(_(\"object file %s is empty\"), path);\n-\t\t\t\tclose(fd);\n-\t\t\t\treturn NULL;\n-\t\t\t}\n-\t\t\tmap = xmmap(NULL, *size, PROT_READ, MAP_PRIVATE, fd, 0);\n+\tif (!fstat(fd, &st)) {\n+\t\t*size = xsize_t(st.st_size);\n+\t\tif (!*size) {\n+\t\t\t/* mmap() is forbidden on empty files */\n+\t\t\terror(_(\"object file %s is empty\"), path);\n+\t\t\tclose(fd);\n+\t\t\treturn NULL;\n \t\t}\n-\t\tclose(fd);\n+\t\tmap = xmmap(NULL, *size, PROT_READ, MAP_PRIVATE, fd, 0);\n \t}\n+\tclose(fd);\n \treturn map;\n }\n \n+static void *map_loose_object_1(struct repository *r,\n+\t\t\t\tconst struct object_id *oid,\n+\t\t\t\tunsigned long *size,\n+\t\t\t\tconst char **path)\n+{\n+\tconst char *p;\n+\tint fd = open_loose_object(r, oid, &p);\n+\n+\tif (fd < 0)\n+\t\treturn NULL;\n+\tif (path)\n+\t\t*path = p;\n+\treturn map_fd(fd, p, size);\n+}\n+\n void *map_loose_object(struct repository *r,\n \t\t const struct object_id *oid,\n \t\t unsigned long *size)\n {\n-\treturn map_loose_object_1(r, NULL, oid, size);\n+\treturn map_loose_object_1(r, oid, size, NULL);\n }\n \n enum unpack_loose_header_result unpack_loose_header(git_zstream *stream,\n@@ -2789,13 +2794,16 @@ int read_loose_object(const char *path,\n \t\t struct object_info *oi)\n {\n \tint ret = -1;\n+\tint fd;\n \tvoid *map = NULL;\n \tunsigned long mapsize;\n \tgit_zstream stream;\n \tchar hdr[MAX_HEADER_LEN];\n \tunsigned long *size = oi->sizep;\n \n-\tmap = map_loose_object_1(the_repository, path, NULL, &mapsize);\n+\tfd = git_open(path);\n+\tif (fd >= 0)\n+\t\tmap = map_fd(fd, path, &mapsize);\n \tif (!map) {\n \t\terror_errno(_(\"unable to mmap %s\"), path);\n \t\tgoto out;\n-- \n2.39.0.rc1.256.g54fd8350bd-goog\n\n"},{"id":"468799","messageId":"7c9ed861e7431352df864c8d2c3bec7dee6e3905.1670532905.git.jonathantanmy@google.com","threadId":"58877","inReplyTo":"cover.1670532905.git.jonathantanmy@google.com","subject":"[PATCH v3 3/4] object-file: emit corruption errors when detected","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-12-08T20:57:07Z","receivedAt":"2022-12-08T20:57:44Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Instead of relying on errno being preserved across function calls, teach\ndo_oid_object_info_extended() to itself report object corruption when\nit first detects it. There are 3 types of corruption being detected:\n - when a replacement object is missing\n - when a loose object is corrupt\n - when a packed object is corrupt and the object cannot be read\n in another way\n\nNote that in the RHS of this patch's diff, a check for ENOENT that was\nintroduced in 3ba7a06552 (A loose object is not corrupt if it cannot\nbe read due to EMFILE, 2010-10-28) is also removed. The purpose of this\ncheck is to avoid a false report of corruption if the errno contains\nsomething like EMFILE (or anything that is not ENOENT), in which case\na more generic report is presented. Because, as of this patch, we no\nlonger rely on such a heuristic to determine corruption, but surface\nthe error message at the point when we read something that we did not\nexpect, this check is no longer necessary.\n\nBesides being more resilient, this also prepares for a future patch in\nwhich an indirect caller of do_oid_object_info_extended() will need\nsuch functionality.\n\nSigned-off-by: Jonathan Tan \n---\n object-file.c | 48 ++++++++++++++++++++++--------------------------\n object-store.h | 3 +++\n 2 files changed, 25 insertions(+), 26 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex d99d05839f..f166065f32 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1433,6 +1433,7 @@ static int loose_object_info(struct repository *r,\n {\n \tint status = 0;\n \tunsigned long mapsize;\n+\tconst char *path = NULL;\n \tvoid *map;\n \tgit_zstream stream;\n \tchar hdr[MAX_HEADER_LEN];\n@@ -1464,7 +1465,7 @@ static int loose_object_info(struct repository *r,\n \t\treturn 0;\n \t}\n \n-\tmap = map_loose_object(r, oid, &mapsize);\n+\tmap = map_loose_object_1(r, oid, &mapsize, &path);\n \tif (!map)\n \t\treturn -1;\n \n@@ -1502,6 +1503,10 @@ static int loose_object_info(struct repository *r,\n \t\tbreak;\n \t}\n \n+\tif (status && (flags & OBJECT_INFO_DIE_IF_CORRUPT))\n+\t\tdie(_(\"loose object %s (stored in %s) is corrupt\"),\n+\t\t oid_to_hex(oid), path);\n+\n \tgit_inflate_end(&stream);\n cleanup:\n \tmunmap(map, mapsize);\n@@ -1611,6 +1616,15 @@ static int do_oid_object_info_extended(struct repository *r,\n \t\t\tcontinue;\n \t\t}\n \n+\t\tif (flags & OBJECT_INFO_DIE_IF_CORRUPT) {\n+\t\t\tconst struct packed_git *p;\n+\t\t\tif ((flags & OBJECT_INFO_LOOKUP_REPLACE) && !oideq(real, oid))\n+\t\t\t\tdie(_(\"replacement %s not found for %s\"),\n+\t\t\t\t oid_to_hex(real), oid_to_hex(oid));\n+\t\t\tif ((p = has_packed_and_bad(r, real)))\n+\t\t\t\tdie(_(\"packed object %s (stored in %s) is corrupt\"),\n+\t\t\t\t oid_to_hex(real), p->pack_name);\n+\t\t}\n \t\treturn -1;\n \t}\n \n@@ -1663,7 +1677,8 @@ int oid_object_info(struct repository *r,\n \n static void *read_object(struct repository *r,\n \t\t\t const struct object_id *oid, enum object_type *type,\n-\t\t\t unsigned long *size)\n+\t\t\t unsigned long *size,\n+\t\t\t int die_if_corrupt)\n {\n \tstruct object_info oi = OBJECT_INFO_INIT;\n \tvoid *content;\n@@ -1671,7 +1686,9 @@ static void *read_object(struct repository *r,\n \toi.sizep = size;\n \toi.contentp = &content;\n \n-\tif (oid_object_info_extended(r, oid, &oi, 0) < 0)\n+\tif (oid_object_info_extended(r, oid, &oi,\n+\t\t\t\t die_if_corrupt ? OBJECT_INFO_DIE_IF_CORRUPT : 0)\n+\t < 0)\n \t\treturn NULL;\n \treturn content;\n }\n@@ -1707,35 +1724,14 @@ void *read_object_file_extended(struct repository *r,\n \t\t\t\tint lookup_replace)\n {\n \tvoid *data;\n-\tconst struct packed_git *p;\n-\tconst char *path;\n-\tstruct stat st;\n \tconst struct object_id *repl = lookup_replace ?\n \t\tlookup_replace_object(r, oid) : oid;\n \n \terrno = 0;\n-\tdata = read_object(r, repl, type, size);\n+\tdata = read_object(r, repl, type, size, 1);\n \tif (data)\n \t\treturn data;\n \n-\tobj_read_lock();\n-\tif (errno && errno != ENOENT)\n-\t\tdie_errno(_(\"failed to read object %s\"), oid_to_hex(oid));\n-\n-\t/* die if we replaced an object with one that does not exist */\n-\tif (repl != oid)\n-\t\tdie(_(\"replacement %s not found for %s\"),\n-\t\t oid_to_hex(repl), oid_to_hex(oid));\n-\n-\tif (!stat_loose_object(r, repl, &st, &path))\n-\t\tdie(_(\"loose object %s (stored in %s) is corrupt\"),\n-\t\t oid_to_hex(repl), path);\n-\n-\tif ((p = has_packed_and_bad(r, repl)))\n-\t\tdie(_(\"packed object %s (stored in %s) is corrupt\"),\n-\t\t oid_to_hex(repl), p->pack_name);\n-\tobj_read_unlock();\n-\n \treturn NULL;\n }\n \n@@ -2278,7 +2274,7 @@ int force_object_loose(const struct object_id *oid, time_t mtime)\n \n \tif (has_loose_object(oid))\n \t\treturn 0;\n-\tbuf = read_object(the_repository, oid, &type, &len);\n+\tbuf = read_object(the_repository, oid, &type, &len, 0);\n \tif (!buf)\n \t\treturn error(_(\"cannot read object for %s\"), oid_to_hex(oid));\n \thdrlen = format_object_header(hdr, sizeof(hdr), type, len);\ndiff --git a/object-store.h b/object-store.h\nindex b1ec0bde82..98c1d67946 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -445,6 +445,9 @@ struct object_info {\n */\n #define OBJECT_INFO_FOR_PREFETCH (OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK)\n \n+/* Die if object corruption (not just an object being missing) was detected. */\n+#define OBJECT_INFO_DIE_IF_CORRUPT 32\n+\n int oid_object_info_extended(struct repository *r,\n \t\t\t const struct object_id *,\n \t\t\t struct object_info *, unsigned flags);\n-- \n2.39.0.rc1.256.g54fd8350bd-goog\n\n"},{"id":"468800","messageId":"5924a5120bc8e0bf529fc1cde5c23724550f72a4.1670532905.git.jonathantanmy@google.com","threadId":"58877","inReplyTo":"cover.1670532905.git.jonathantanmy@google.com","subject":"[PATCH v3 4/4] commit: don't lazy-fetch commits","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-12-08T20:57:08Z","receivedAt":"2022-12-08T20:57:59Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When parsing commits, fail fast when the commit is missing or\ncorrupt, instead of attempting to fetch them. This is done by inlining\nrepo_read_object_file() and setting the flag that prevents fetching.\n\nThis is motivated by a situation in which through a bug (not necessarily\nthrough Git), there was corruption in the object store of a partial\nclone. In this particular case, the problem was exposed when \"git gc\"\ntried to expire reflogs, which calls repo_parse_commit(), which triggers\nfetches of the missing commits.\n\n(There are other possible solutions to this problem including passing an\nargument from \"git gc\" to \"git reflog\" to inhibit all lazy fetches, but\nI think that this fix is at the wrong level - fixing \"git reflog\" means\nthat this particular command works fine, or so we think (it will fail if\nit somehow needs to read a legitimately missing blob, say, a .gitmodules\nfile), but fixing repo_parse_commit() will fix a whole class of bugs.)\n\nSigned-off-by: Jonathan Tan \nSigned-off-by: Junio C Hamano \n---\n commit.c | 15 +++++++++++++--\n 1 file changed, 13 insertions(+), 2 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex 572301b80a..a02723f06b 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -508,6 +508,17 @@ int repo_parse_commit_internal(struct repository *r,\n \tenum object_type type;\n \tvoid *buffer;\n \tunsigned long size;\n+\tstruct object_info oi = {\n+\t\t.typep = &type,\n+\t\t.sizep = &size,\n+\t\t.contentp = &buffer,\n+\t};\n+\t/*\n+\t * Git does not support partial clones that exclude commits, so set\n+\t * OBJECT_INFO_SKIP_FETCH_OBJECT to fail fast when an object is missing.\n+\t */\n+\tint flags = OBJECT_INFO_LOOKUP_REPLACE | OBJECT_INFO_SKIP_FETCH_OBJECT |\n+\t\tOBJECT_INFO_DIE_IF_CORRUPT;\n \tint ret;\n \n \tif (!item)\n@@ -516,8 +527,8 @@ int repo_parse_commit_internal(struct repository *r,\n \t\treturn 0;\n \tif (use_commit_graph && parse_commit_in_graph(r, item))\n \t\treturn 0;\n-\tbuffer = repo_read_object_file(r, &item->object.oid, &type, &size);\n-\tif (!buffer)\n+\n+\tif (oid_object_info_extended(r, &item->object.oid, &oi, flags) < 0)\n \t\treturn quiet_on_missing ? -1 :\n \t\t\terror(\"Could not read %s\",\n \t\t\t oid_to_hex(&item->object.oid));\n-- \n2.39.0.rc1.256.g54fd8350bd-goog\n\n"},{"id":"468811","messageId":"Y5KV0vIkxyA95/xf@coredump.intra.peff.net","threadId":"58877","inReplyTo":"7c9ed861e7431352df864c8d2c3bec7dee6e3905.1670532905.git.jonathantanmy@google.com","subject":"Re: [PATCH v3 3/4] object-file: emit corruption errors when detected","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-12-09T01:56:34Z","receivedAt":"2022-12-09T01:58:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 08, 2022 at 12:57:07PM -0800, Jonathan Tan wrote:\n\n> Note that in the RHS of this patch's diff, a check for ENOENT that was\n> introduced in 3ba7a06552 (A loose object is not corrupt if it cannot\n> be read due to EMFILE, 2010-10-28) is also removed. The purpose of this\n> check is to avoid a false report of corruption if the errno contains\n> something like EMFILE (or anything that is not ENOENT), in which case\n> a more generic report is presented. Because, as of this patch, we no\n> longer rely on such a heuristic to determine corruption, but surface\n> the error message at the point when we read something that we did not\n> expect, this check is no longer necessary.\n\nI think this version still has the small issue that we'll _only_ surface\na generic error return in such a case, and never report EMFILE\nspecifically. I.e., I think we'd still want something like this on top:\n\ndiff --git a/object-file.c b/object-file.c\nindex dc7665d6fa..36082bc991 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1422,6 +1422,7 @@ static int loose_object_info(struct repository *r,\n \t\t\t struct object_info *oi, int flags)\n {\n \tint status = 0;\n+\tint fd;\n \tunsigned long mapsize;\n \tconst char *path = NULL;\n \tvoid *map;\n@@ -1455,7 +1456,13 @@ static int loose_object_info(struct repository *r,\n \t\treturn 0;\n \t}\n \n-\tmap = map_loose_object_1(r, oid, &mapsize, &path);\n+\tfd = open_loose_object(r, oid, &path);\n+\tif (fd < 0) {\n+\t\tif (errno != ENOENT)\n+\t\t\terror_errno(_(\"unable to open loose object %s\"), path);\n+\t\treturn -1;\n+\t}\n+\tmap = map_fd(fd, path, &mapsize);\n \tif (!map)\n \t\treturn -1;\n \n\nOtherwise ENOENT and EMFILE are indistinguishable from the user's\nperspective. And one is normal and routine, but the other points to\nsomething the user probably needs to fix.\n\n-Peff\n"},{"id":"468812","messageId":"Y5KWpXwxdRI4QPNl@coredump.intra.peff.net","threadId":"58877","inReplyTo":"7419e4ac7053ab2d89a4cdc4612e5baeca48ce9f.1670532905.git.jonathantanmy@google.com","subject":"Re: [PATCH v3 2/4] object-file: refactor map_loose_object_1()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-12-09T02:00:05Z","receivedAt":"2022-12-09T02:00:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 08, 2022 at 12:57:06PM -0800, Jonathan Tan wrote:\n\n> This function can do 3 things:\n> 1. Gets an fd given a path\n> 2. Simultaneously gets a path and fd given an OID\n> 3. Memory maps an fd\n> \n> Split this function up. Only one caller needs 1, so inline that. As for\n> 2, a future patch will also need this functionality and, in addition,\n> the calculated path, so extract this into a separate function with an\n> out parameter for the path.\n\nThis is moving in a good direction. I like the name \"map_fd\" for the\nhelper. Being able to give it a useful name like that is a good clue\nthat it is doing a more focused and understandable job than the generic\nmap_loose_object_1(). :)\n\nIn fact...\n\n> +static void *map_loose_object_1(struct repository *r,\n> +\t\t\t\tconst struct object_id *oid,\n> +\t\t\t\tunsigned long *size,\n> +\t\t\t\tconst char **path)\n> +{\n> +\tconst char *p;\n> +\tint fd = open_loose_object(r, oid, &p);\n> +\n> +\tif (fd < 0)\n> +\t\treturn NULL;\n> +\tif (path)\n> +\t\t*path = p;\n> +\treturn map_fd(fd, p, size);\n> +}\n> +\n> void *map_loose_object(struct repository *r,\n> \t\t const struct object_id *oid,\n> \t\t unsigned long *size)\n> {\n> -\treturn map_loose_object_1(r, NULL, oid, size);\n> +\treturn map_loose_object_1(r, oid, size, NULL);\n> }\n\nIf you take my suggestion on patch 3, then the only other caller of\nmap_loose_object_1() goes away, and we can fold it all into one\nreasonably-named function:\n\ndiff --git a/object-file.c b/object-file.c\nindex d99d05839f..429e3a746d 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1233,28 +1233,18 @@ static void *map_fd(int fd, const char *path, unsigned long *size)\n \treturn map;\n }\n \n-static void *map_loose_object_1(struct repository *r,\n-\t\t\t\tconst struct object_id *oid,\n-\t\t\t\tunsigned long *size,\n-\t\t\t\tconst char **path)\n+void *map_loose_object(struct repository *r,\n+\t\t const struct object_id *oid,\n+\t\t unsigned long *size)\n {\n \tconst char *p;\n \tint fd = open_loose_object(r, oid, &p);\n \n \tif (fd < 0)\n \t\treturn NULL;\n-\tif (path)\n-\t\t*path = p;\n \treturn map_fd(fd, p, size);\n }\n \n-void *map_loose_object(struct repository *r,\n-\t\t const struct object_id *oid,\n-\t\t unsigned long *size)\n-{\n-\treturn map_loose_object_1(r, oid, size, NULL);\n-}\n-\n enum unpack_loose_header_result unpack_loose_header(git_zstream *stream,\n \t\t\t\t\t\t unsigned char *map,\n \t\t\t\t\t\t unsigned long mapsize,\n\n-Peff\n"},{"id":"468828","messageId":"221209.86359o7jbd.gmgdl@evledraar.gmail.com","threadId":"58877","inReplyTo":"5924a5120bc8e0bf529fc1cde5c23724550f72a4.1670532905.git.jonathantanmy@google.com","subject":"Re: [PATCH v3 4/4] commit: don't lazy-fetch commits","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-09T14:14:47Z","receivedAt":"2022-12-09T14:18:17Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Dec 08 2022, Jonathan Tan wrote:\n\n\n> diff --git a/commit.c b/commit.c\n> index 572301b80a..a02723f06b 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -508,6 +508,17 @@ int repo_parse_commit_internal(struct repository *r,\n> \tenum object_type type;\n> \tvoid *buffer;\n> \tunsigned long size;\n> +\tstruct object_info oi = {\n> +\t\t.typep = &type,\n> +\t\t.sizep = &size,\n> +\t\t.contentp = &buffer,\n> +\t};\n> +\t/*\n> +\t * Git does not support partial clones that exclude commits, so set\n> +\t * OBJECT_INFO_SKIP_FETCH_OBJECT to fail fast when an object is missing.\n> +\t */\n> +\tint flags = OBJECT_INFO_LOOKUP_REPLACE | OBJECT_INFO_SKIP_FETCH_OBJECT |\n> +\t\tOBJECT_INFO_DIE_IF_CORRUPT;\n> \tint ret;\n> \n> \tif (!item)\n> @@ -516,8 +527,8 @@ int repo_parse_commit_internal(struct repository *r,\n> \t\treturn 0;\n> \tif (use_commit_graph && parse_commit_in_graph(r, item))\n> \t\treturn 0;\n> -\tbuffer = repo_read_object_file(r, &item->object.oid, &type, &size);\n> -\tif (!buffer)\n> +\n> +\tif (oid_object_info_extended(r, &item->object.oid, &oi, flags) < 0)\n\nStyle: you're adding another \\n here, usually we'd prefer it, but here\nthe function already has all these checks bundled together without a \\n,\nand this \"if\" is followed by one without it.\n\nBut then again those two \"if\"'s have to do with populating the \"oi\" and\nthen reading out the \"type\", so it's probably fine & OK.\n"},{"id":"468829","messageId":"221209.86y1rg63tt.gmgdl@evledraar.gmail.com","threadId":"58877","inReplyTo":"7c9ed861e7431352df864c8d2c3bec7dee6e3905.1670532905.git.jonathantanmy@google.com","subject":"Re: [PATCH v3 3/4] object-file: emit corruption errors when detected","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-09T14:19:44Z","receivedAt":"2022-12-09T14:37:28Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Dec 08 2022, Jonathan Tan wrote:\n\n> Instead of relying on errno being preserved across function calls, teach\n> do_oid_object_info_extended() to itself report object corruption when\n> it first detects it. There are 3 types of corruption being detected:\n> - when a replacement object is missing\n> - when a loose object is corrupt\n> - when a packed object is corrupt and the object cannot be read\n> in another way\n>\n> Note that in the RHS of this patch's diff, a check for ENOENT that was\n> introduced in 3ba7a06552 (A loose object is not corrupt if it cannot\n> be read due to EMFILE, 2010-10-28) is also removed. The purpose of this\n> check is to avoid a false report of corruption if the errno contains\n> something like EMFILE (or anything that is not ENOENT), in which case\n> a more generic report is presented. Because, as of this patch, we no\n> longer rely on such a heuristic to determine corruption, but surface\n> the error message at the point when we read something that we did not\n> expect, this check is no longer necessary.\n>\n> Besides being more resilient, this also prepares for a future patch in\n> which an indirect caller of do_oid_object_info_extended() will need\n> such functionality.\n>\n> Signed-off-by: Jonathan Tan \n> ---\n> object-file.c | 48 ++++++++++++++++++++++--------------------------\n> object-store.h | 3 +++\n> 2 files changed, 25 insertions(+), 26 deletions(-)\n>\n> diff --git a/object-file.c b/object-file.c\n> index d99d05839f..f166065f32 100644\n> --- a/object-file.c\n> +++ b/object-file.c\n> @@ -1433,6 +1433,7 @@ static int loose_object_info(struct repository *r,\n> {\n> \tint status = 0;\n> \tunsigned long mapsize;\n> +\tconst char *path = NULL;\n\nI think the NULL assignment here should either go, or it's incomplete.\n\nBelow you chechk the return value of \"map\", so let's either trust it to\npopulate \"path\" if it's returning success (which it does), *or* not\ntrust it, init this to NULL, and below add....\n\n> \tvoid *map;\n> \tgit_zstream stream;\n> \tchar hdr[MAX_HEADER_LEN];\n> @@ -1464,7 +1465,7 @@ static int loose_object_info(struct repository *r,\n> \t\treturn 0;\n> \t}\n> \n> -\tmap = map_loose_object(r, oid, &mapsize);\n> +\tmap = map_loose_object_1(r, oid, &mapsize, &path);\n> \tif (!map)\n> \t\treturn -1;\n\n....\n\n\tif (!path)\n\t\tBUG(\"map_loose_object_1 should have given us a path\");\n\nBut I don't think it's good to just hide the potential difference\nbetween the two, especially as....\n\n> @@ -1502,6 +1503,10 @@ static int loose_object_info(struct repository *r,\n> \t\tbreak;\n> \t}\n> \n> +\tif (status && (flags & OBJECT_INFO_DIE_IF_CORRUPT))\n> +\t\tdie(_(\"loose object %s (stored in %s) is corrupt\"),\n> +\t\t oid_to_hex(oid), path);\n\n...I think this is leftover from a previous round (or maybe not, I\ndidn't check) where we did a free(path), but here we'd end up\nsegfaulting (not on glibc, but some platforms) if we have a NULL path.\n\nSo init-ing it didn't help us, but just helps to hide that potential\n(and much worse) bug.\n\nI think this change should also remove the existing \"const char *path\"\nin this function from the \"if\"'d scope omitted in this context.\n\nThe C compiler won't care, but to the human reader it's easier to reason\nabout not shadowing the variable now, for as it turns out no reason, as\nthey're effectively independent.\n\n> \tgit_inflate_end(&stream);\n> cleanup:\n> \tmunmap(map, mapsize);\n> @@ -1611,6 +1616,15 @@ static int do_oid_object_info_extended(struct repository *r,\n> \t\t\tcontinue;\n> \t\t}\n> \n> +\t\tif (flags & OBJECT_INFO_DIE_IF_CORRUPT) {\n> +\t\t\tconst struct packed_git *p;\n\nNit: add an extra \\n here, between decls and code.\n\n> -\tif (oid_object_info_extended(r, oid, &oi, 0) < 0)\n> +\tif (oid_object_info_extended(r, oid, &oi,\n> +\t\t\t\t die_if_corrupt ? OBJECT_INFO_DIE_IF_CORRUPT : 0)\n> +\t < 0)\n> \t\treturn NULL;\n> \treturn content;\n> }\n\nThis is a very odd coding style/wrapping, to not even end up with a line\nshorter than 79 characters. You can instead do:\n\n\tif (oid_object_info_extended(r, oid, &oi, die_if_corrupt\n\t\t\t\t ? OBJECT_INFO_DIE_IF_CORRUPT : 0) < 0)\n\nWhich is in line with our usual style, and does wrap before 79 characters...\n\n> diff --git a/object-store.h b/object-store.h\n> index b1ec0bde82..98c1d67946 100644\n> --- a/object-store.h\n> +++ b/object-store.h\n> @@ -445,6 +445,9 @@ struct object_info {\n> */\n> #define OBJECT_INFO_FOR_PREFETCH (OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK)\n> \n> +/* Die if object corruption (not just an object being missing) was detected. */\n> +#define OBJECT_INFO_DIE_IF_CORRUPT 32\n\nPersonally I wouldn't mind a short cleanup step in this series to change\nthese to 1<<0, 1<<1 etc., as we do for almost everything els.\n\nI.e. in an earlier step you removed the \"16\", and changed that \"32\" to\n\"16\", now we're adding a \"32\" again.\n\nI also notice that you didn't just add a \"4\" here, which is an existing\ngap, which turns out to be a leftover bit from your 9c8a294a1ae\n(sha1-file: remove OBJECT_INFO_SKIP_CACHED, 2020-01-02) ~2 years ago :)\n\nMaybe it's too much, and we could do it later, but something like this\nas a first step:\n\ndiff --git a/object-file.c b/object-file.c\nindex 26290554bb4..48eff3850f5 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1534,7 +1534,8 @@ int fetch_if_missing = 1;\n \n static int do_oid_object_info_extended(struct repository *r,\n \t\t\t\t const struct object_id *oid,\n-\t\t\t\t struct object_info *oi, unsigned flags)\n+\t\t\t\t struct object_info *oi,\n+\t\t\t\t enum object_info_flags flags)\n {\n \tstatic struct object_info blank_oi = OBJECT_INFO_INIT;\n \tstruct cached_object *co;\n@@ -1633,7 +1634,7 @@ static int do_oid_object_info_extended(struct repository *r,\n }\n \n int oid_object_info_extended(struct repository *r, const struct object_id *oid,\n-\t\t\t struct object_info *oi, unsigned flags)\n+\t\t\t struct object_info *oi, enum object_info_flags flags)\n {\n \tint ret;\n \tobj_read_lock();\ndiff --git a/object-store.h b/object-store.h\nindex 1be57abaf10..a20e00395b9 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -428,28 +428,31 @@ struct object_info {\n */\n #define OBJECT_INFO_INIT { 0 }\n \n-/* Invoke lookup_replace_object() on the given hash */\n-#define OBJECT_INFO_LOOKUP_REPLACE 1\n-/* Allow reading from a loose object file of unknown/bogus type */\n-#define OBJECT_INFO_ALLOW_UNKNOWN_TYPE 2\n-/* Do not retry packed storage after checking packed and loose storage */\n-#define OBJECT_INFO_QUICK 8\n-/* Do not check loose object */\n-#define OBJECT_INFO_IGNORE_LOOSE 16\n-/*\n- * Do not attempt to fetch the object if missing (even if fetch_is_missing is\n- * nonzero).\n- */\n-#define OBJECT_INFO_SKIP_FETCH_OBJECT 32\n-/*\n- * This is meant for bulk prefetching of missing blobs in a partial\n- * clone. Implies OBJECT_INFO_SKIP_FETCH_OBJECT and OBJECT_INFO_QUICK\n- */\n-#define OBJECT_INFO_FOR_PREFETCH (OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK)\n+enum object_info_flags {\n+\t/* Invoke lookup_replace_object() on the given hash */\n+\tOBJECT_INFO_LOOKUP_REPLACE = 1<<0,\n+\t/* Allow reading from a loose object file of unknown/bogus type */\n+\tOBJECT_INFO_ALLOW_UNKNOWN_TYPE = 1<<1,\n+\t/* Do not retry packed storage after checking packed and loose storage */\n+\tOBJECT_INFO_QUICK = 1<<2,\n+\t/* Do not check loose object */\n+\tOBJECT_INFO_IGNORE_LOOSE = 1<<3,\n+\t/*\n+\t * Do not attempt to fetch the object if missing (even if fetch_is_missing is\n+\t * nonzero).\n+\t */\n+\tOBJECT_INFO_SKIP_FETCH_OBJECT = 1<<4,\n+\t/*\n+\t * This is meant for bulk prefetching of missing blobs in a partial\n+\t * clone. Implies OBJECT_INFO_SKIP_FETCH_OBJECT and OBJECT_INFO_QUICK\n+\t */\n+\tOBJECT_INFO_FOR_PREFETCH = (OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK),\n+};\n \n int oid_object_info_extended(struct repository *r,\n \t\t\t const struct object_id *,\n-\t\t\t struct object_info *, unsigned flags);\n+\t\t\t struct object_info *,\n+\t\t\t enum object_info_flags flags);\n \n /*\n * Iterate over the files in the loose-object parts of the object\n\n"},{"id":"468833","messageId":"20221209181704.106534-1-jonathantanmy@google.com","threadId":"58877","inReplyTo":"Y5KWpXwxdRI4QPNl@coredump.intra.peff.net","subject":"Re: [PATCH v3 2/4] object-file: refactor map_loose_object_1()","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-12-09T18:17:04Z","receivedAt":"2022-12-09T18:17:15Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Jeff King writes:\n> If you take my suggestion on patch 3, then the only other caller of\n> map_loose_object_1() goes away, and we can fold it all into one\n> reasonably-named function:\n\nAh, that is true as of this patch, but patch 3 introduces another caller\nof this function. I tried to allude to it in the commit message, but if\nthere is a clearer way to explain that, please let me know.\n"},{"id":"468834","messageId":"20221209182656.108137-1-jonathantanmy@google.com","threadId":"58877","inReplyTo":"Y5KV0vIkxyA95/xf@coredump.intra.peff.net","subject":"Re: [PATCH v3 3/4] object-file: emit corruption errors when detected","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-12-09T18:26:56Z","receivedAt":"2022-12-09T18:27:05Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Jeff King writes:\n> I think this version still has the small issue that we'll _only_ surface\n> a generic error return in such a case, and never report EMFILE\n> specifically. I.e., I think we'd still want something like this on top:\n\n[snip]\n\nOK, I'll do this. This also has the advantage of not using\nmap_loose_object_1, so I'll be able to inline it in the previous patch.\n"},{"id":"468835","messageId":"20221209183318.394294-1-jonathantanmy@google.com","threadId":"58877","inReplyTo":"221209.86y1rg63tt.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v3 3/4] object-file: emit corruption errors when detected","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-12-09T18:33:18Z","receivedAt":"2022-12-09T18:33:25Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Ævar Arnfjörð Bjarmason writes:\n> > @@ -1433,6 +1433,7 @@ static int loose_object_info(struct repository *r,\n> > {\n> > \tint status = 0;\n> > \tunsigned long mapsize;\n> > +\tconst char *path = NULL;\n> \n> I think the NULL assignment here should either go, or it's incomplete.\n\n[snip]\n\n> So init-ing it didn't help us, but just helps to hide that potential\n> (and much worse) bug.\n\nGood catch. I'll remove the assignment.\n\n \n> I think this change should also remove the existing \"const char *path\"\n> in this function from the \"if\"'d scope omitted in this context.\n> \n> The C compiler won't care, but to the human reader it's easier to reason\n> about not shadowing the variable now, for as it turns out no reason, as\n> they're effectively independent.\n\nMakes sense.\n\n> > \tgit_inflate_end(&stream);\n> > cleanup:\n> > \tmunmap(map, mapsize);\n> > @@ -1611,6 +1616,15 @@ static int do_oid_object_info_extended(struct repository *r,\n> > \t\t\tcontinue;\n> > \t\t}\n> > \n> > +\t\tif (flags & OBJECT_INFO_DIE_IF_CORRUPT) {\n> > +\t\t\tconst struct packed_git *p;\n> \n> Nit: add an extra \\n here, between decls and code.\n\nOK.\n\n> > -\tif (oid_object_info_extended(r, oid, &oi, 0) < 0)\n> > +\tif (oid_object_info_extended(r, oid, &oi,\n> > +\t\t\t\t die_if_corrupt ? OBJECT_INFO_DIE_IF_CORRUPT : 0)\n> > +\t < 0)\n> > \t\treturn NULL;\n> > \treturn content;\n> > }\n> \n> This is a very odd coding style/wrapping, to not even end up with a line\n> shorter than 79 characters. You can instead do:\n> \n> \tif (oid_object_info_extended(r, oid, &oi, die_if_corrupt\n> \t\t\t\t ? OBJECT_INFO_DIE_IF_CORRUPT : 0) < 0)\n> \n> Which is in line with our usual style, and does wrap before 79 characters...\n\nI didn't want to split the ternary expression, but OK, I'll follow your\nwrapping. \n\n> > diff --git a/object-store.h b/object-store.h\n> > index b1ec0bde82..98c1d67946 100644\n> > --- a/object-store.h\n> > +++ b/object-store.h\n> > @@ -445,6 +445,9 @@ struct object_info {\n> > */\n> > #define OBJECT_INFO_FOR_PREFETCH (OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK)\n> > \n> > +/* Die if object corruption (not just an object being missing) was detected. */\n> > +#define OBJECT_INFO_DIE_IF_CORRUPT 32\n> \n> Personally I wouldn't mind a short cleanup step in this series to change\n> these to 1<<0, 1<<1 etc., as we do for almost everything els.\n> \n> I.e. in an earlier step you removed the \"16\", and changed that \"32\" to\n> \"16\", now we're adding a \"32\" again.\n> \n> I also notice that you didn't just add a \"4\" here, which is an existing\n> gap, which turns out to be a leftover bit from your 9c8a294a1ae\n> (sha1-file: remove OBJECT_INFO_SKIP_CACHED, 2020-01-02) ~2 years ago :)\n\nAh...I didn't notice the 4 missing. But I think the series has gone over\nenough iterations now that I'd rather leave this as-is and maybe change\nthis in a future patch.\n"},{"id":"468839","messageId":"Y5OaLKmIXvbEI1bP@coredump.intra.peff.net","threadId":"58877","inReplyTo":"20221209181704.106534-1-jonathantanmy@google.com","subject":"Re: [PATCH v3 2/4] object-file: refactor map_loose_object_1()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-12-09T20:27:24Z","receivedAt":"2022-12-09T20:27:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 09, 2022 at 10:17:04AM -0800, Jonathan Tan wrote:\n\n> Jeff King writes:\n> > If you take my suggestion on patch 3, then the only other caller of\n> > map_loose_object_1() goes away, and we can fold it all into one\n> > reasonably-named function:\n> \n> Ah, that is true as of this patch, but patch 3 introduces another caller\n> of this function. I tried to allude to it in the commit message, but if\n> there is a clearer way to explain that, please let me know.\n\nYes, that's the \"other caller\" I was referring to. :) Hopefully it is\nmore clear after you read my comments on v3.\n\n-Peff\n"},{"id":"468840","messageId":"Y5OaScWCzzhIumtM@coredump.intra.peff.net","threadId":"58877","inReplyTo":"Y5OaLKmIXvbEI1bP@coredump.intra.peff.net","subject":"Re: [PATCH v3 2/4] object-file: refactor map_loose_object_1()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-12-09T20:27:53Z","receivedAt":"2022-12-09T20:28:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 09, 2022 at 03:27:24PM -0500, Jeff King wrote:\n\n> On Fri, Dec 09, 2022 at 10:17:04AM -0800, Jonathan Tan wrote:\n> \n> > Jeff King writes:\n> > > If you take my suggestion on patch 3, then the only other caller of\n> > > map_loose_object_1() goes away, and we can fold it all into one\n> > > reasonably-named function:\n> > \n> > Ah, that is true as of this patch, but patch 3 introduces another caller\n> > of this function. I tried to allude to it in the commit message, but if\n> > there is a clearer way to explain that, please let me know.\n> \n> Yes, that's the \"other caller\" I was referring to. :) Hopefully it is\n> more clear after you read my comments on v3.\n\nEr, that should be \"...comments on patch 3\", of course, not \"v3\".\n\n-Peff\n"},{"id":"468846","messageId":"cover.1670622176.git.jonathantanmy@google.com","threadId":"58877","inReplyTo":"cover.1669839849.git.jonathantanmy@google.com","subject":"[PATCH v4 0/4] Don't lazy-fetch commits when parsing them","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-12-09T21:44:21Z","receivedAt":"2022-12-09T21:44:34Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Thanks everyone for your comments. Here's a reroll.\n\nJonathan Tan (4):\n object-file: remove OBJECT_INFO_IGNORE_LOOSE\n object-file: refactor map_loose_object_1()\n object-file: emit corruption errors when detected\n commit: don't lazy-fetch commits\n\n commit.c | 15 ++++++-\n object-file.c | 108 ++++++++++++++++++++++++-------------------------\n object-store.h | 7 ++--\n 3 files changed, 69 insertions(+), 61 deletions(-)\n\nRange-diff against v3:\n1: be0b08cac2 = 1: be0b08cac2 object-file: remove OBJECT_INFO_IGNORE_LOOSE\n2: 7419e4ac70 ! 2: 4b2fb68743 object-file: refactor map_loose_object_1()\n @@ Commit message\n 2. Simultaneously gets a path and fd given an OID\n 3. Memory maps an fd\n \n - Split this function up. Only one caller needs 1, so inline that. As for\n - 2, a future patch will also need this functionality and, in addition,\n - the calculated path, so extract this into a separate function with an\n - out parameter for the path.\n + Keep 3 (renaming the function accordingly) and inline 1 and 2 into their\n + respective callers.\n \n Signed-off-by: Jonathan Tan \n \n @@ object-file.c: static int quick_has_loose(struct repository *r,\n \treturn map;\n }\n \n -+static void *map_loose_object_1(struct repository *r,\n -+\t\t\t\tconst struct object_id *oid,\n -+\t\t\t\tunsigned long *size,\n -+\t\t\t\tconst char **path)\n -+{\n +@@ object-file.c: void *map_loose_object(struct repository *r,\n + \t\t const struct object_id *oid,\n + \t\t unsigned long *size)\n + {\n +-\treturn map_loose_object_1(r, NULL, oid, size);\n +\tconst char *p;\n +\tint fd = open_loose_object(r, oid, &p);\n +\n +\tif (fd < 0)\n +\t\treturn NULL;\n -+\tif (path)\n -+\t\t*path = p;\n +\treturn map_fd(fd, p, size);\n -+}\n -+\n - void *map_loose_object(struct repository *r,\n - \t\t const struct object_id *oid,\n - \t\t unsigned long *size)\n - {\n --\treturn map_loose_object_1(r, NULL, oid, size);\n -+\treturn map_loose_object_1(r, oid, size, NULL);\n }\n \n enum unpack_loose_header_result unpack_loose_header(git_zstream *stream,\n3: 7c9ed861e7 ! 3: 07d28db92c object-file: emit corruption errors when detected\n @@ Commit message\n which an indirect caller of do_oid_object_info_extended() will need\n such functionality.\n \n + Helped-by: Jeff King \n Signed-off-by: Jonathan Tan \n \n ## object-file.c ##\n @@ object-file.c: static int loose_object_info(struct repository *r,\n + \t\t\t struct object_info *oi, int flags)\n {\n \tint status = 0;\n ++\tint fd;\n \tunsigned long mapsize;\n -+\tconst char *path = NULL;\n ++\tconst char *path;\n \tvoid *map;\n \tgit_zstream stream;\n \tchar hdr[MAX_HEADER_LEN];\n +@@ object-file.c: static int loose_object_info(struct repository *r,\n + \t * object even exists.\n + \t */\n + \tif (!oi->typep && !oi->type_name && !oi->sizep && !oi->contentp) {\n +-\t\tconst char *path;\n + \t\tstruct stat st;\n + \t\tif (!oi->disk_sizep && (flags & OBJECT_INFO_QUICK))\n + \t\t\treturn quick_has_loose(r, oid) ? 0 : -1;\n @@ object-file.c: static int loose_object_info(struct repository *r,\n \t\treturn 0;\n \t}\n \n -\tmap = map_loose_object(r, oid, &mapsize);\n -+\tmap = map_loose_object_1(r, oid, &mapsize, &path);\n ++\tfd = open_loose_object(r, oid, &path);\n ++\tif (fd < 0) {\n ++\t\tif (errno != ENOENT)\n ++\t\t\terror_errno(_(\"unable to open loose object %s\"), path);\n ++\t\treturn -1;\n ++\t}\n ++\tmap = map_fd(fd, path, &mapsize);\n \tif (!map)\n \t\treturn -1;\n \n @@ object-file.c: static void *read_object(struct repository *r,\n \toi.contentp = &content;\n \n -\tif (oid_object_info_extended(r, oid, &oi, 0) < 0)\n -+\tif (oid_object_info_extended(r, oid, &oi,\n -+\t\t\t\t die_if_corrupt ? OBJECT_INFO_DIE_IF_CORRUPT : 0)\n -+\t < 0)\n ++\tif (oid_object_info_extended(r, oid, &oi, die_if_corrupt\n ++\t\t\t\t ? OBJECT_INFO_DIE_IF_CORRUPT : 0) < 0)\n \t\treturn NULL;\n \treturn content;\n }\n4: 5924a5120b = 4: 1a0cd5b244 commit: don't lazy-fetch commits\n-- \n2.39.0.rc1.256.g54fd8350bd-goog\n\n"},{"id":"468847","messageId":"be0b08cac219357e1ce9cd46fd7ab5c13344699b.1670622176.git.jonathantanmy@google.com","threadId":"58877","inReplyTo":"cover.1670622176.git.jonathantanmy@google.com","subject":"[PATCH v4 1/4] object-file: remove OBJECT_INFO_IGNORE_LOOSE","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-12-09T21:44:22Z","receivedAt":"2022-12-09T21:44:37Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Its last user was removed in 97b2fa08b6 (fetch-pack: drop\ncustom loose object cache, 2018-11-12), so we can remove it.\n\nHelped-by: Jeff King \nSigned-off-by: Jonathan Tan \n---\n object-file.c | 3 ---\n object-store.h | 4 +---\n 2 files changed, 1 insertion(+), 6 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex 26290554bb..cf724bc19b 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1575,9 +1575,6 @@ static int do_oid_object_info_extended(struct repository *r,\n \t\tif (find_pack_entry(r, real, &e))\n \t\t\tbreak;\n \n-\t\tif (flags & OBJECT_INFO_IGNORE_LOOSE)\n-\t\t\treturn -1;\n-\n \t\t/* Most likely it's a loose object. */\n \t\tif (!loose_object_info(r, real, oi, flags))\n \t\t\treturn 0;\ndiff --git a/object-store.h b/object-store.h\nindex 1be57abaf1..b1ec0bde82 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -434,13 +434,11 @@ struct object_info {\n #define OBJECT_INFO_ALLOW_UNKNOWN_TYPE 2\n /* Do not retry packed storage after checking packed and loose storage */\n #define OBJECT_INFO_QUICK 8\n-/* Do not check loose object */\n-#define OBJECT_INFO_IGNORE_LOOSE 16\n /*\n * Do not attempt to fetch the object if missing (even if fetch_is_missing is\n * nonzero).\n */\n-#define OBJECT_INFO_SKIP_FETCH_OBJECT 32\n+#define OBJECT_INFO_SKIP_FETCH_OBJECT 16\n /*\n * This is meant for bulk prefetching of missing blobs in a partial\n * clone. Implies OBJECT_INFO_SKIP_FETCH_OBJECT and OBJECT_INFO_QUICK\n-- \n2.39.0.rc1.256.g54fd8350bd-goog\n\n"},{"id":"468848","messageId":"4b2fb687432c2ce1471d9eb02e86b3acc43cc953.1670622176.git.jonathantanmy@google.com","threadId":"58877","inReplyTo":"cover.1670622176.git.jonathantanmy@google.com","subject":"[PATCH v4 2/4] object-file: refactor map_loose_object_1()","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-12-09T21:44:23Z","receivedAt":"2022-12-09T21:44:42Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"This function can do 3 things:\n 1. Gets an fd given a path\n 2. Simultaneously gets a path and fd given an OID\n 3. Memory maps an fd\n\nKeep 3 (renaming the function accordingly) and inline 1 and 2 into their\nrespective callers.\n\nSigned-off-by: Jonathan Tan \n---\n object-file.c | 50 ++++++++++++++++++++++++--------------------------\n 1 file changed, 24 insertions(+), 26 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex cf724bc19b..429e3a746d 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1211,35 +1211,25 @@ static int quick_has_loose(struct repository *r,\n }\n \n /*\n- * Map the loose object at \"path\" if it is not NULL, or the path found by\n- * searching for a loose object named \"oid\".\n+ * Map and close the given loose object fd. The path argument is used for\n+ * error reporting.\n */\n-static void *map_loose_object_1(struct repository *r, const char *path,\n-\t\t\t const struct object_id *oid, unsigned long *size)\n+static void *map_fd(int fd, const char *path, unsigned long *size)\n {\n-\tvoid *map;\n-\tint fd;\n-\n-\tif (path)\n-\t\tfd = git_open(path);\n-\telse\n-\t\tfd = open_loose_object(r, oid, &path);\n-\tmap = NULL;\n-\tif (fd >= 0) {\n-\t\tstruct stat st;\n+\tvoid *map = NULL;\n+\tstruct stat st;\n \n-\t\tif (!fstat(fd, &st)) {\n-\t\t\t*size = xsize_t(st.st_size);\n-\t\t\tif (!*size) {\n-\t\t\t\t/* mmap() is forbidden on empty files */\n-\t\t\t\terror(_(\"object file %s is empty\"), path);\n-\t\t\t\tclose(fd);\n-\t\t\t\treturn NULL;\n-\t\t\t}\n-\t\t\tmap = xmmap(NULL, *size, PROT_READ, MAP_PRIVATE, fd, 0);\n+\tif (!fstat(fd, &st)) {\n+\t\t*size = xsize_t(st.st_size);\n+\t\tif (!*size) {\n+\t\t\t/* mmap() is forbidden on empty files */\n+\t\t\terror(_(\"object file %s is empty\"), path);\n+\t\t\tclose(fd);\n+\t\t\treturn NULL;\n \t\t}\n-\t\tclose(fd);\n+\t\tmap = xmmap(NULL, *size, PROT_READ, MAP_PRIVATE, fd, 0);\n \t}\n+\tclose(fd);\n \treturn map;\n }\n \n@@ -1247,7 +1237,12 @@ void *map_loose_object(struct repository *r,\n \t\t const struct object_id *oid,\n \t\t unsigned long *size)\n {\n-\treturn map_loose_object_1(r, NULL, oid, size);\n+\tconst char *p;\n+\tint fd = open_loose_object(r, oid, &p);\n+\n+\tif (fd < 0)\n+\t\treturn NULL;\n+\treturn map_fd(fd, p, size);\n }\n \n enum unpack_loose_header_result unpack_loose_header(git_zstream *stream,\n@@ -2789,13 +2784,16 @@ int read_loose_object(const char *path,\n \t\t struct object_info *oi)\n {\n \tint ret = -1;\n+\tint fd;\n \tvoid *map = NULL;\n \tunsigned long mapsize;\n \tgit_zstream stream;\n \tchar hdr[MAX_HEADER_LEN];\n \tunsigned long *size = oi->sizep;\n \n-\tmap = map_loose_object_1(the_repository, path, NULL, &mapsize);\n+\tfd = git_open(path);\n+\tif (fd >= 0)\n+\t\tmap = map_fd(fd, path, &mapsize);\n \tif (!map) {\n \t\terror_errno(_(\"unable to mmap %s\"), path);\n \t\tgoto out;\n-- \n2.39.0.rc1.256.g54fd8350bd-goog\n\n"},{"id":"468849","messageId":"07d28db92c2c61358755b3d501bc5bd35a760de1.1670622176.git.jonathantanmy@google.com","threadId":"58877","inReplyTo":"cover.1670622176.git.jonathantanmy@google.com","subject":"[PATCH v4 3/4] object-file: emit corruption errors when detected","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-12-09T21:44:24Z","receivedAt":"2022-12-09T21:44:50Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Instead of relying on errno being preserved across function calls, teach\ndo_oid_object_info_extended() to itself report object corruption when\nit first detects it. There are 3 types of corruption being detected:\n - when a replacement object is missing\n - when a loose object is corrupt\n - when a packed object is corrupt and the object cannot be read\n in another way\n\nNote that in the RHS of this patch's diff, a check for ENOENT that was\nintroduced in 3ba7a06552 (A loose object is not corrupt if it cannot\nbe read due to EMFILE, 2010-10-28) is also removed. The purpose of this\ncheck is to avoid a false report of corruption if the errno contains\nsomething like EMFILE (or anything that is not ENOENT), in which case\na more generic report is presented. Because, as of this patch, we no\nlonger rely on such a heuristic to determine corruption, but surface\nthe error message at the point when we read something that we did not\nexpect, this check is no longer necessary.\n\nBesides being more resilient, this also prepares for a future patch in\nwhich an indirect caller of do_oid_object_info_extended() will need\nsuch functionality.\n\nHelped-by: Jeff King \nSigned-off-by: Jonathan Tan \n---\n object-file.c | 55 +++++++++++++++++++++++++-------------------------\n object-store.h | 3 +++\n 2 files changed, 31 insertions(+), 27 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex 429e3a746d..2a0df39822 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1422,7 +1422,9 @@ static int loose_object_info(struct repository *r,\n \t\t\t struct object_info *oi, int flags)\n {\n \tint status = 0;\n+\tint fd;\n \tunsigned long mapsize;\n+\tconst char *path;\n \tvoid *map;\n \tgit_zstream stream;\n \tchar hdr[MAX_HEADER_LEN];\n@@ -1443,7 +1445,6 @@ static int loose_object_info(struct repository *r,\n \t * object even exists.\n \t */\n \tif (!oi->typep && !oi->type_name && !oi->sizep && !oi->contentp) {\n-\t\tconst char *path;\n \t\tstruct stat st;\n \t\tif (!oi->disk_sizep && (flags & OBJECT_INFO_QUICK))\n \t\t\treturn quick_has_loose(r, oid) ? 0 : -1;\n@@ -1454,7 +1455,13 @@ static int loose_object_info(struct repository *r,\n \t\treturn 0;\n \t}\n \n-\tmap = map_loose_object(r, oid, &mapsize);\n+\tfd = open_loose_object(r, oid, &path);\n+\tif (fd < 0) {\n+\t\tif (errno != ENOENT)\n+\t\t\terror_errno(_(\"unable to open loose object %s\"), path);\n+\t\treturn -1;\n+\t}\n+\tmap = map_fd(fd, path, &mapsize);\n \tif (!map)\n \t\treturn -1;\n \n@@ -1492,6 +1499,10 @@ static int loose_object_info(struct repository *r,\n \t\tbreak;\n \t}\n \n+\tif (status && (flags & OBJECT_INFO_DIE_IF_CORRUPT))\n+\t\tdie(_(\"loose object %s (stored in %s) is corrupt\"),\n+\t\t oid_to_hex(oid), path);\n+\n \tgit_inflate_end(&stream);\n cleanup:\n \tmunmap(map, mapsize);\n@@ -1601,6 +1612,15 @@ static int do_oid_object_info_extended(struct repository *r,\n \t\t\tcontinue;\n \t\t}\n \n+\t\tif (flags & OBJECT_INFO_DIE_IF_CORRUPT) {\n+\t\t\tconst struct packed_git *p;\n+\t\t\tif ((flags & OBJECT_INFO_LOOKUP_REPLACE) && !oideq(real, oid))\n+\t\t\t\tdie(_(\"replacement %s not found for %s\"),\n+\t\t\t\t oid_to_hex(real), oid_to_hex(oid));\n+\t\t\tif ((p = has_packed_and_bad(r, real)))\n+\t\t\t\tdie(_(\"packed object %s (stored in %s) is corrupt\"),\n+\t\t\t\t oid_to_hex(real), p->pack_name);\n+\t\t}\n \t\treturn -1;\n \t}\n \n@@ -1653,7 +1673,8 @@ int oid_object_info(struct repository *r,\n \n static void *read_object(struct repository *r,\n \t\t\t const struct object_id *oid, enum object_type *type,\n-\t\t\t unsigned long *size)\n+\t\t\t unsigned long *size,\n+\t\t\t int die_if_corrupt)\n {\n \tstruct object_info oi = OBJECT_INFO_INIT;\n \tvoid *content;\n@@ -1661,7 +1682,8 @@ static void *read_object(struct repository *r,\n \toi.sizep = size;\n \toi.contentp = &content;\n \n-\tif (oid_object_info_extended(r, oid, &oi, 0) < 0)\n+\tif (oid_object_info_extended(r, oid, &oi, die_if_corrupt\n+\t\t\t\t ? OBJECT_INFO_DIE_IF_CORRUPT : 0) < 0)\n \t\treturn NULL;\n \treturn content;\n }\n@@ -1697,35 +1719,14 @@ void *read_object_file_extended(struct repository *r,\n \t\t\t\tint lookup_replace)\n {\n \tvoid *data;\n-\tconst struct packed_git *p;\n-\tconst char *path;\n-\tstruct stat st;\n \tconst struct object_id *repl = lookup_replace ?\n \t\tlookup_replace_object(r, oid) : oid;\n \n \terrno = 0;\n-\tdata = read_object(r, repl, type, size);\n+\tdata = read_object(r, repl, type, size, 1);\n \tif (data)\n \t\treturn data;\n \n-\tobj_read_lock();\n-\tif (errno && errno != ENOENT)\n-\t\tdie_errno(_(\"failed to read object %s\"), oid_to_hex(oid));\n-\n-\t/* die if we replaced an object with one that does not exist */\n-\tif (repl != oid)\n-\t\tdie(_(\"replacement %s not found for %s\"),\n-\t\t oid_to_hex(repl), oid_to_hex(oid));\n-\n-\tif (!stat_loose_object(r, repl, &st, &path))\n-\t\tdie(_(\"loose object %s (stored in %s) is corrupt\"),\n-\t\t oid_to_hex(repl), path);\n-\n-\tif ((p = has_packed_and_bad(r, repl)))\n-\t\tdie(_(\"packed object %s (stored in %s) is corrupt\"),\n-\t\t oid_to_hex(repl), p->pack_name);\n-\tobj_read_unlock();\n-\n \treturn NULL;\n }\n \n@@ -2268,7 +2269,7 @@ int force_object_loose(const struct object_id *oid, time_t mtime)\n \n \tif (has_loose_object(oid))\n \t\treturn 0;\n-\tbuf = read_object(the_repository, oid, &type, &len);\n+\tbuf = read_object(the_repository, oid, &type, &len, 0);\n \tif (!buf)\n \t\treturn error(_(\"cannot read object for %s\"), oid_to_hex(oid));\n \thdrlen = format_object_header(hdr, sizeof(hdr), type, len);\ndiff --git a/object-store.h b/object-store.h\nindex b1ec0bde82..98c1d67946 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -445,6 +445,9 @@ struct object_info {\n */\n #define OBJECT_INFO_FOR_PREFETCH (OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK)\n \n+/* Die if object corruption (not just an object being missing) was detected. */\n+#define OBJECT_INFO_DIE_IF_CORRUPT 32\n+\n int oid_object_info_extended(struct repository *r,\n \t\t\t const struct object_id *,\n \t\t\t struct object_info *, unsigned flags);\n-- \n2.39.0.rc1.256.g54fd8350bd-goog\n\n"},{"id":"468850","messageId":"1a0cd5b244652fc821714380bfd3cb5425388c8b.1670622176.git.jonathantanmy@google.com","threadId":"58877","inReplyTo":"cover.1670622176.git.jonathantanmy@google.com","subject":"[PATCH v4 4/4] commit: don't lazy-fetch commits","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-12-09T21:44:25Z","receivedAt":"2022-12-09T21:44:52Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When parsing commits, fail fast when the commit is missing or\ncorrupt, instead of attempting to fetch them. This is done by inlining\nrepo_read_object_file() and setting the flag that prevents fetching.\n\nThis is motivated by a situation in which through a bug (not necessarily\nthrough Git), there was corruption in the object store of a partial\nclone. In this particular case, the problem was exposed when \"git gc\"\ntried to expire reflogs, which calls repo_parse_commit(), which triggers\nfetches of the missing commits.\n\n(There are other possible solutions to this problem including passing an\nargument from \"git gc\" to \"git reflog\" to inhibit all lazy fetches, but\nI think that this fix is at the wrong level - fixing \"git reflog\" means\nthat this particular command works fine, or so we think (it will fail if\nit somehow needs to read a legitimately missing blob, say, a .gitmodules\nfile), but fixing repo_parse_commit() will fix a whole class of bugs.)\n\nSigned-off-by: Jonathan Tan \nSigned-off-by: Junio C Hamano \n---\n commit.c | 15 +++++++++++++--\n 1 file changed, 13 insertions(+), 2 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex 572301b80a..a02723f06b 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -508,6 +508,17 @@ int repo_parse_commit_internal(struct repository *r,\n \tenum object_type type;\n \tvoid *buffer;\n \tunsigned long size;\n+\tstruct object_info oi = {\n+\t\t.typep = &type,\n+\t\t.sizep = &size,\n+\t\t.contentp = &buffer,\n+\t};\n+\t/*\n+\t * Git does not support partial clones that exclude commits, so set\n+\t * OBJECT_INFO_SKIP_FETCH_OBJECT to fail fast when an object is missing.\n+\t */\n+\tint flags = OBJECT_INFO_LOOKUP_REPLACE | OBJECT_INFO_SKIP_FETCH_OBJECT |\n+\t\tOBJECT_INFO_DIE_IF_CORRUPT;\n \tint ret;\n \n \tif (!item)\n@@ -516,8 +527,8 @@ int repo_parse_commit_internal(struct repository *r,\n \t\treturn 0;\n \tif (use_commit_graph && parse_commit_in_graph(r, item))\n \t\treturn 0;\n-\tbuffer = repo_read_object_file(r, &item->object.oid, &type, &size);\n-\tif (!buffer)\n+\n+\tif (oid_object_info_extended(r, &item->object.oid, &oi, flags) < 0)\n \t\treturn quiet_on_missing ? -1 :\n \t\t\terror(\"Could not read %s\",\n \t\t\t oid_to_hex(&item->object.oid));\n-- \n2.39.0.rc1.256.g54fd8350bd-goog\n\n"},{"id":"468854","messageId":"xmqqv8mkxgd1.fsf@gitster.g","threadId":"58877","inReplyTo":"07d28db92c2c61358755b3d501bc5bd35a760de1.1670622176.git.jonathantanmy@google.com","subject":"Re: [PATCH v4 3/4] object-file: emit corruption errors when detected","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-10T00:16:42Z","receivedAt":"2022-12-10T00:16:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> +\tfd = open_loose_object(r, oid, &path);\n> +\tif (fd < 0) {\n> +\t\tif (errno != ENOENT)\n> +\t\t\terror_errno(_(\"unable to open loose object %s\"), path);\n> +\t\treturn -1;\n> +\t}\n\nI know there was a discussion in the previous round, but is this use\nof path truly safe? Currently it may happen to be as long as there\nis at least one element on the odb list, but when thinking things\nthrough with future-proofing point of view, I do not think assuming\nthat path is always computable is a healthy thing to do in the\nlonger term.\n\nOur \"struct object_id\" may be extended in the future and allow us to\nexpress \"invalid\" object name, in which case the error return we get\nmay not even be about \"loose object file not openable\" but \"there\nwill never be a loose object file for such an invalid object name\",\nin which case there won't be any path returned from the function.\n\nOther than that, the series looks quite clearly written. Nicely\ndone.\n\nThanks.\n"},{"id":"468922","messageId":"20221212203811.77228-1-jonathantanmy@google.com","threadId":"58877","inReplyTo":"xmqqv8mkxgd1.fsf@gitster.g","subject":"Re: [PATCH v4 3/4] object-file: emit corruption errors when detected","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-12-12T20:38:11Z","receivedAt":"2022-12-12T20:38:20Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Junio C Hamano writes:\n> Jonathan Tan writes:\n> \n> > +\tfd = open_loose_object(r, oid, &path);\n> > +\tif (fd < 0) {\n> > +\t\tif (errno != ENOENT)\n> > +\t\t\terror_errno(_(\"unable to open loose object %s\"), path);\n> > +\t\treturn -1;\n> > +\t}\n> \n> I know there was a discussion in the previous round, but is this use\n> of path truly safe? Currently it may happen to be as long as there\n> is at least one element on the odb list, but when thinking things\n> through with future-proofing point of view, I do not think assuming\n> that path is always computable is a healthy thing to do in the\n> longer term.\n> \n> Our \"struct object_id\" may be extended in the future and allow us to\n> express \"invalid\" object name, in which case the error return we get\n> may not even be about \"loose object file not openable\" but \"there\n> will never be a loose object file for such an invalid object name\",\n> in which case there won't be any path returned from the function.\n\nAh, good point. I think what I can do is to document the function to\nonly return a path if a path was involved in the error somehow, and make\nanything that uses \"path\" in the caller check for NULL.\n \n> Other than that, the series looks quite clearly written. Nicely\n> done.\n> \n> Thanks.\n\nThanks for taking a look.\n"},{"id":"468923","messageId":"Y5eT6jodUdNr6hK6@coredump.intra.peff.net","threadId":"58877","inReplyTo":"xmqqv8mkxgd1.fsf@gitster.g","subject":"Re: [PATCH v4 3/4] object-file: emit corruption errors when detected","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-12-12T20:49:46Z","receivedAt":"2022-12-12T20:51:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 10, 2022 at 09:16:42AM +0900, Junio C Hamano wrote:\n\n> Jonathan Tan writes:\n> \n> > +\tfd = open_loose_object(r, oid, &path);\n> > +\tif (fd < 0) {\n> > +\t\tif (errno != ENOENT)\n> > +\t\t\terror_errno(_(\"unable to open loose object %s\"), path);\n> > +\t\treturn -1;\n> > +\t}\n> \n> I know there was a discussion in the previous round, but is this use\n> of path truly safe? Currently it may happen to be as long as there\n> is at least one element on the odb list, but when thinking things\n> through with future-proofing point of view, I do not think assuming\n> that path is always computable is a healthy thing to do in the\n> longer term.\n> \n> Our \"struct object_id\" may be extended in the future and allow us to\n> express \"invalid\" object name, in which case the error return we get\n> may not even be about \"loose object file not openable\" but \"there\n> will never be a loose object file for such an invalid object name\",\n> in which case there won't be any path returned from the function.\n\nActually, I think it is much worse than that. The code as written above\nis already buggy (which is my fault, as I suggested it).\n\nIn open_loose_object() we'll continue to iterate and pick out the \"most\ninteresting errno\". But we'll throw away the path that gave us that\nerrno. So we might well say:\n\n unable to open loose object /some/alternate/12/34abcd: permission denied\n\nwhen the actual problem is in /main/objdir/12/34abcd.\n\nIt's fixable, but with some pain in handling the allocations. I think it\nwould be sufficient to just say:\n\n error_errno(_(\"unable to open loose object %s\"), oid_to_hex(oid));\n\nhere. And possibly put a comment above open_loose_object() that \"path\"\nis only guaranteed to point to something sensible when a non-negative\nvalue is returned.\n\n-Peff\n"},{"id":"468927","messageId":"20221212205955.956380-1-jonathantanmy@google.com","threadId":"58877","inReplyTo":"Y5eT6jodUdNr6hK6@coredump.intra.peff.net","subject":"Re: [PATCH v4 3/4] object-file: emit corruption errors when detected","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-12-12T20:59:55Z","receivedAt":"2022-12-12T21:01:11Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Jeff King writes:\n> Actually, I think it is much worse than that. The code as written above\n> is already buggy (which is my fault, as I suggested it).\n> \n> In open_loose_object() we'll continue to iterate and pick out the \"most\n> interesting errno\". But we'll throw away the path that gave us that\n> errno. So we might well say:\n> \n> unable to open loose object /some/alternate/12/34abcd: permission denied\n> \n> when the actual problem is in /main/objdir/12/34abcd.\n> \n> It's fixable, but with some pain in handling the allocations. I think it\n> would be sufficient to just say:\n> \n> error_errno(_(\"unable to open loose object %s\"), oid_to_hex(oid));\n> \n> here. \n\nOK, let's go with this.\n\n> And possibly put a comment above open_loose_object() that \"path\"\n> is only guaranteed to point to something sensible when a non-negative\n> value is returned.\n\nJunio made a point that there could, for example, be no path when the\nodb list is empty (maybe in the future) so I don't think this would be\nsufficient. But there is already a comment there pointing to a comment\nin another function that states \"path ... (if any)\" so this is something\nthat callers should already take care of. In my changes, I'll initialize\nit to NULL and whenever I use it, I'll check for non-NULL first.\n"},{"id":"468930","messageId":"Y5ebC1qwJi5VwnCh@coredump.intra.peff.net","threadId":"58877","inReplyTo":"20221212205955.956380-1-jonathantanmy@google.com","subject":"Re: [PATCH v4 3/4] object-file: emit corruption errors when detected","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-12-12T21:20:11Z","receivedAt":"2022-12-12T21:21:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 12, 2022 at 12:59:55PM -0800, Jonathan Tan wrote:\n\n> > And possibly put a comment above open_loose_object() that \"path\"\n> > is only guaranteed to point to something sensible when a non-negative\n> > value is returned.\n> \n> Junio made a point that there could, for example, be no path when the\n> odb list is empty (maybe in the future) so I don't think this would be\n> sufficient. But there is already a comment there pointing to a comment\n> in another function that states \"path ... (if any)\" so this is something\n> that callers should already take care of. In my changes, I'll initialize\n> it to NULL and whenever I use it, I'll check for non-NULL first.\n\nIf we return a non-negative value, then we opened something, so by\ndefinition, don't we have a path of the thing we opened?\n\nI think the case Junio mentioned was if we for some reason didn't look\nat _any_ path. In which case we'd be returning an error.\n\n-Peff\n"},{"id":"468931","messageId":"20221212212947.1559820-1-jonathantanmy@google.com","threadId":"58877","inReplyTo":"Y5ebC1qwJi5VwnCh@coredump.intra.peff.net","subject":"Re: [PATCH v4 3/4] object-file: emit corruption errors when detected","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-12-12T21:29:47Z","receivedAt":"2022-12-12T21:29:54Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Jeff King writes:\n> On Mon, Dec 12, 2022 at 12:59:55PM -0800, Jonathan Tan wrote:\n> \n> > > And possibly put a comment above open_loose_object() that \"path\"\n> > > is only guaranteed to point to something sensible when a non-negative\n> > > value is returned.\n> > \n> > Junio made a point that there could, for example, be no path when the\n> > odb list is empty (maybe in the future) so I don't think this would be\n> > sufficient. But there is already a comment there pointing to a comment\n> > in another function that states \"path ... (if any)\" so this is something\n> > that callers should already take care of. In my changes, I'll initialize\n> > it to NULL and whenever I use it, I'll check for non-NULL first.\n> \n> If we return a non-negative value, then we opened something, so by\n> definition, don't we have a path of the thing we opened?\n> \n> I think the case Junio mentioned was if we for some reason didn't look\n> at _any_ path. In which case we'd be returning an error.\n\nAh, my reading comprehension is failing me, sorry. We do want \"path\"\nto point to something sensible (well, whenever we can) when an error\noccurs, though, since we want to include that path in our error message\nwhen DIE_IF_CORRUPT is used. So guaranteeing \"path\" when a non-negative\nvalue is returned (and hence, no error occurred) is not so useful.\n"},{"id":"468953","messageId":"Y5eoZV0nXVKbIPuP@coredump.intra.peff.net","threadId":"58877","inReplyTo":"20221212212947.1559820-1-jonathantanmy@google.com","subject":"Re: [PATCH v4 3/4] object-file: emit corruption errors when detected","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-12-12T22:17:09Z","receivedAt":"2022-12-12T22:18:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 12, 2022 at 01:29:47PM -0800, Jonathan Tan wrote:\n\n> > If we return a non-negative value, then we opened something, so by\n> > definition, don't we have a path of the thing we opened?\n> > \n> > I think the case Junio mentioned was if we for some reason didn't look\n> > at _any_ path. In which case we'd be returning an error.\n> \n> Ah, my reading comprehension is failing me, sorry. We do want \"path\"\n> to point to something sensible (well, whenever we can) when an error\n> occurs, though, since we want to include that path in our error message\n> when DIE_IF_CORRUPT is used. So guaranteeing \"path\" when a non-negative\n> value is returned (and hence, no error occurred) is not so useful.\n\nBut we only DIE_IF_CORRUPT when there is actual corruption, which means\nwe've opened an object file, and \"path\" is valid.\n\nThe only time \"path\" would be invalid is if open_loose_object() itself\nreturns an error, which is the message under discussion:\n\n\tfd = open_loose_object(r, oid, &path);\n\tif (fd < 0) {\n\t\tif (errno != ENOENT)\n\t\t\terror_errno(_(\"unable to open loose object %s\"), path);\n\t\treturn -1;\n\t}\n\nIf that stops expecting \"path\" to be valid (and just mentions the oid),\nthen the rest of loose_object_info() should be fine.\n\n-Peff\n"},{"id":"468959","messageId":"cover.1670885252.git.jonathantanmy@google.com","threadId":"58877","inReplyTo":"cover.1669839849.git.jonathantanmy@google.com","subject":"[PATCH v5 0/4] Don't lazy-fetch commits when parsing them","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-12-12T22:48:47Z","receivedAt":"2022-12-12T22:50:18Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Thanks everyone for taking a look. Here's a reroll with safer path\nhandling.\n\nJonathan Tan (4):\n object-file: remove OBJECT_INFO_IGNORE_LOOSE\n object-file: refactor map_loose_object_1()\n object-file: emit corruption errors when detected\n commit: don't lazy-fetch commits\n\n commit.c | 15 ++++++-\n object-file.c | 108 ++++++++++++++++++++++++-------------------------\n object-store.h | 7 ++--\n 3 files changed, 69 insertions(+), 61 deletions(-)\n\nRange-diff against v4:\n1: be0b08cac2 = 1: be0b08cac2 object-file: remove OBJECT_INFO_IGNORE_LOOSE\n2: 4b2fb68743 = 2: 4b2fb68743 object-file: refactor map_loose_object_1()\n3: 07d28db92c ! 3: a229ea0b11 object-file: emit corruption errors when detected\n @@ object-file.c: static int loose_object_info(struct repository *r,\n \tint status = 0;\n +\tint fd;\n \tunsigned long mapsize;\n -+\tconst char *path;\n ++\tconst char *path = NULL;\n \tvoid *map;\n \tgit_zstream stream;\n \tchar hdr[MAX_HEADER_LEN];\n @@ object-file.c: static int loose_object_info(struct repository *r,\n +\tfd = open_loose_object(r, oid, &path);\n +\tif (fd < 0) {\n +\t\tif (errno != ENOENT)\n -+\t\t\terror_errno(_(\"unable to open loose object %s\"), path);\n ++\t\t\terror_errno(_(\"unable to open loose object %s\"), oid_to_hex(oid));\n +\t\treturn -1;\n +\t}\n +\tmap = map_fd(fd, path, &mapsize);\n @@ object-file.c: static int loose_object_info(struct repository *r,\n \t\tbreak;\n \t}\n \n -+\tif (status && (flags & OBJECT_INFO_DIE_IF_CORRUPT))\n ++\tif (status && path && (flags & OBJECT_INFO_DIE_IF_CORRUPT))\n +\t\tdie(_(\"loose object %s (stored in %s) is corrupt\"),\n +\t\t oid_to_hex(oid), path);\n +\n4: 1a0cd5b244 = 4: b54972118a commit: don't lazy-fetch commits\n-- \n2.39.0.rc1.256.g54fd8350bd-goog\n\n"},{"id":"468960","messageId":"be0b08cac219357e1ce9cd46fd7ab5c13344699b.1670885252.git.jonathantanmy@google.com","threadId":"58877","inReplyTo":"cover.1670885252.git.jonathantanmy@google.com","subject":"[PATCH v5 1/4] object-file: remove OBJECT_INFO_IGNORE_LOOSE","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-12-12T22:48:48Z","receivedAt":"2022-12-12T22:50:20Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Its last user was removed in 97b2fa08b6 (fetch-pack: drop\ncustom loose object cache, 2018-11-12), so we can remove it.\n\nHelped-by: Jeff King \nSigned-off-by: Jonathan Tan \n---\n object-file.c | 3 ---\n object-store.h | 4 +---\n 2 files changed, 1 insertion(+), 6 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex 26290554bb..cf724bc19b 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1575,9 +1575,6 @@ static int do_oid_object_info_extended(struct repository *r,\n \t\tif (find_pack_entry(r, real, &e))\n \t\t\tbreak;\n \n-\t\tif (flags & OBJECT_INFO_IGNORE_LOOSE)\n-\t\t\treturn -1;\n-\n \t\t/* Most likely it's a loose object. */\n \t\tif (!loose_object_info(r, real, oi, flags))\n \t\t\treturn 0;\ndiff --git a/object-store.h b/object-store.h\nindex 1be57abaf1..b1ec0bde82 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -434,13 +434,11 @@ struct object_info {\n #define OBJECT_INFO_ALLOW_UNKNOWN_TYPE 2\n /* Do not retry packed storage after checking packed and loose storage */\n #define OBJECT_INFO_QUICK 8\n-/* Do not check loose object */\n-#define OBJECT_INFO_IGNORE_LOOSE 16\n /*\n * Do not attempt to fetch the object if missing (even if fetch_is_missing is\n * nonzero).\n */\n-#define OBJECT_INFO_SKIP_FETCH_OBJECT 32\n+#define OBJECT_INFO_SKIP_FETCH_OBJECT 16\n /*\n * This is meant for bulk prefetching of missing blobs in a partial\n * clone. Implies OBJECT_INFO_SKIP_FETCH_OBJECT and OBJECT_INFO_QUICK\n-- \n2.39.0.rc1.256.g54fd8350bd-goog\n\n"},{"id":"468961","messageId":"4b2fb687432c2ce1471d9eb02e86b3acc43cc953.1670885252.git.jonathantanmy@google.com","threadId":"58877","inReplyTo":"cover.1670885252.git.jonathantanmy@google.com","subject":"[PATCH v5 2/4] object-file: refactor map_loose_object_1()","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-12-12T22:48:49Z","receivedAt":"2022-12-12T22:50:24Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"This function can do 3 things:\n 1. Gets an fd given a path\n 2. Simultaneously gets a path and fd given an OID\n 3. Memory maps an fd\n\nKeep 3 (renaming the function accordingly) and inline 1 and 2 into their\nrespective callers.\n\nSigned-off-by: Jonathan Tan \n---\n object-file.c | 50 ++++++++++++++++++++++++--------------------------\n 1 file changed, 24 insertions(+), 26 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex cf724bc19b..429e3a746d 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1211,35 +1211,25 @@ static int quick_has_loose(struct repository *r,\n }\n \n /*\n- * Map the loose object at \"path\" if it is not NULL, or the path found by\n- * searching for a loose object named \"oid\".\n+ * Map and close the given loose object fd. The path argument is used for\n+ * error reporting.\n */\n-static void *map_loose_object_1(struct repository *r, const char *path,\n-\t\t\t const struct object_id *oid, unsigned long *size)\n+static void *map_fd(int fd, const char *path, unsigned long *size)\n {\n-\tvoid *map;\n-\tint fd;\n-\n-\tif (path)\n-\t\tfd = git_open(path);\n-\telse\n-\t\tfd = open_loose_object(r, oid, &path);\n-\tmap = NULL;\n-\tif (fd >= 0) {\n-\t\tstruct stat st;\n+\tvoid *map = NULL;\n+\tstruct stat st;\n \n-\t\tif (!fstat(fd, &st)) {\n-\t\t\t*size = xsize_t(st.st_size);\n-\t\t\tif (!*size) {\n-\t\t\t\t/* mmap() is forbidden on empty files */\n-\t\t\t\terror(_(\"object file %s is empty\"), path);\n-\t\t\t\tclose(fd);\n-\t\t\t\treturn NULL;\n-\t\t\t}\n-\t\t\tmap = xmmap(NULL, *size, PROT_READ, MAP_PRIVATE, fd, 0);\n+\tif (!fstat(fd, &st)) {\n+\t\t*size = xsize_t(st.st_size);\n+\t\tif (!*size) {\n+\t\t\t/* mmap() is forbidden on empty files */\n+\t\t\terror(_(\"object file %s is empty\"), path);\n+\t\t\tclose(fd);\n+\t\t\treturn NULL;\n \t\t}\n-\t\tclose(fd);\n+\t\tmap = xmmap(NULL, *size, PROT_READ, MAP_PRIVATE, fd, 0);\n \t}\n+\tclose(fd);\n \treturn map;\n }\n \n@@ -1247,7 +1237,12 @@ void *map_loose_object(struct repository *r,\n \t\t const struct object_id *oid,\n \t\t unsigned long *size)\n {\n-\treturn map_loose_object_1(r, NULL, oid, size);\n+\tconst char *p;\n+\tint fd = open_loose_object(r, oid, &p);\n+\n+\tif (fd < 0)\n+\t\treturn NULL;\n+\treturn map_fd(fd, p, size);\n }\n \n enum unpack_loose_header_result unpack_loose_header(git_zstream *stream,\n@@ -2789,13 +2784,16 @@ int read_loose_object(const char *path,\n \t\t struct object_info *oi)\n {\n \tint ret = -1;\n+\tint fd;\n \tvoid *map = NULL;\n \tunsigned long mapsize;\n \tgit_zstream stream;\n \tchar hdr[MAX_HEADER_LEN];\n \tunsigned long *size = oi->sizep;\n \n-\tmap = map_loose_object_1(the_repository, path, NULL, &mapsize);\n+\tfd = git_open(path);\n+\tif (fd >= 0)\n+\t\tmap = map_fd(fd, path, &mapsize);\n \tif (!map) {\n \t\terror_errno(_(\"unable to mmap %s\"), path);\n \t\tgoto out;\n-- \n2.39.0.rc1.256.g54fd8350bd-goog\n\n"},{"id":"468962","messageId":"b54972118acb0552c21e507035c550b704adc812.1670885252.git.jonathantanmy@google.com","threadId":"58877","inReplyTo":"cover.1670885252.git.jonathantanmy@google.com","subject":"[PATCH v5 4/4] commit: don't lazy-fetch commits","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-12-12T22:48:51Z","receivedAt":"2022-12-12T22:50:28Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When parsing commits, fail fast when the commit is missing or\ncorrupt, instead of attempting to fetch them. This is done by inlining\nrepo_read_object_file() and setting the flag that prevents fetching.\n\nThis is motivated by a situation in which through a bug (not necessarily\nthrough Git), there was corruption in the object store of a partial\nclone. In this particular case, the problem was exposed when \"git gc\"\ntried to expire reflogs, which calls repo_parse_commit(), which triggers\nfetches of the missing commits.\n\n(There are other possible solutions to this problem including passing an\nargument from \"git gc\" to \"git reflog\" to inhibit all lazy fetches, but\nI think that this fix is at the wrong level - fixing \"git reflog\" means\nthat this particular command works fine, or so we think (it will fail if\nit somehow needs to read a legitimately missing blob, say, a .gitmodules\nfile), but fixing repo_parse_commit() will fix a whole class of bugs.)\n\nSigned-off-by: Jonathan Tan \nSigned-off-by: Junio C Hamano \n---\n commit.c | 15 +++++++++++++--\n 1 file changed, 13 insertions(+), 2 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex 572301b80a..a02723f06b 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -508,6 +508,17 @@ int repo_parse_commit_internal(struct repository *r,\n \tenum object_type type;\n \tvoid *buffer;\n \tunsigned long size;\n+\tstruct object_info oi = {\n+\t\t.typep = &type,\n+\t\t.sizep = &size,\n+\t\t.contentp = &buffer,\n+\t};\n+\t/*\n+\t * Git does not support partial clones that exclude commits, so set\n+\t * OBJECT_INFO_SKIP_FETCH_OBJECT to fail fast when an object is missing.\n+\t */\n+\tint flags = OBJECT_INFO_LOOKUP_REPLACE | OBJECT_INFO_SKIP_FETCH_OBJECT |\n+\t\tOBJECT_INFO_DIE_IF_CORRUPT;\n \tint ret;\n \n \tif (!item)\n@@ -516,8 +527,8 @@ int repo_parse_commit_internal(struct repository *r,\n \t\treturn 0;\n \tif (use_commit_graph && parse_commit_in_graph(r, item))\n \t\treturn 0;\n-\tbuffer = repo_read_object_file(r, &item->object.oid, &type, &size);\n-\tif (!buffer)\n+\n+\tif (oid_object_info_extended(r, &item->object.oid, &oi, flags) < 0)\n \t\treturn quiet_on_missing ? -1 :\n \t\t\terror(\"Could not read %s\",\n \t\t\t oid_to_hex(&item->object.oid));\n-- \n2.39.0.rc1.256.g54fd8350bd-goog\n\n"},{"id":"468963","messageId":"a229ea0b1122f55e91f98475cd7e508f4dd8501a.1670885252.git.jonathantanmy@google.com","threadId":"58877","inReplyTo":"cover.1670885252.git.jonathantanmy@google.com","subject":"[PATCH v5 3/4] object-file: emit corruption errors when detected","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-12-12T22:48:50Z","receivedAt":"2022-12-12T22:50:30Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Instead of relying on errno being preserved across function calls, teach\ndo_oid_object_info_extended() to itself report object corruption when\nit first detects it. There are 3 types of corruption being detected:\n - when a replacement object is missing\n - when a loose object is corrupt\n - when a packed object is corrupt and the object cannot be read\n in another way\n\nNote that in the RHS of this patch's diff, a check for ENOENT that was\nintroduced in 3ba7a06552 (A loose object is not corrupt if it cannot\nbe read due to EMFILE, 2010-10-28) is also removed. The purpose of this\ncheck is to avoid a false report of corruption if the errno contains\nsomething like EMFILE (or anything that is not ENOENT), in which case\na more generic report is presented. Because, as of this patch, we no\nlonger rely on such a heuristic to determine corruption, but surface\nthe error message at the point when we read something that we did not\nexpect, this check is no longer necessary.\n\nBesides being more resilient, this also prepares for a future patch in\nwhich an indirect caller of do_oid_object_info_extended() will need\nsuch functionality.\n\nHelped-by: Jeff King \nSigned-off-by: Jonathan Tan \n---\n object-file.c | 55 +++++++++++++++++++++++++-------------------------\n object-store.h | 3 +++\n 2 files changed, 31 insertions(+), 27 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex 429e3a746d..e0cef8b906 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1422,7 +1422,9 @@ static int loose_object_info(struct repository *r,\n \t\t\t struct object_info *oi, int flags)\n {\n \tint status = 0;\n+\tint fd;\n \tunsigned long mapsize;\n+\tconst char *path = NULL;\n \tvoid *map;\n \tgit_zstream stream;\n \tchar hdr[MAX_HEADER_LEN];\n@@ -1443,7 +1445,6 @@ static int loose_object_info(struct repository *r,\n \t * object even exists.\n \t */\n \tif (!oi->typep && !oi->type_name && !oi->sizep && !oi->contentp) {\n-\t\tconst char *path;\n \t\tstruct stat st;\n \t\tif (!oi->disk_sizep && (flags & OBJECT_INFO_QUICK))\n \t\t\treturn quick_has_loose(r, oid) ? 0 : -1;\n@@ -1454,7 +1455,13 @@ static int loose_object_info(struct repository *r,\n \t\treturn 0;\n \t}\n \n-\tmap = map_loose_object(r, oid, &mapsize);\n+\tfd = open_loose_object(r, oid, &path);\n+\tif (fd < 0) {\n+\t\tif (errno != ENOENT)\n+\t\t\terror_errno(_(\"unable to open loose object %s\"), oid_to_hex(oid));\n+\t\treturn -1;\n+\t}\n+\tmap = map_fd(fd, path, &mapsize);\n \tif (!map)\n \t\treturn -1;\n \n@@ -1492,6 +1499,10 @@ static int loose_object_info(struct repository *r,\n \t\tbreak;\n \t}\n \n+\tif (status && path && (flags & OBJECT_INFO_DIE_IF_CORRUPT))\n+\t\tdie(_(\"loose object %s (stored in %s) is corrupt\"),\n+\t\t oid_to_hex(oid), path);\n+\n \tgit_inflate_end(&stream);\n cleanup:\n \tmunmap(map, mapsize);\n@@ -1601,6 +1612,15 @@ static int do_oid_object_info_extended(struct repository *r,\n \t\t\tcontinue;\n \t\t}\n \n+\t\tif (flags & OBJECT_INFO_DIE_IF_CORRUPT) {\n+\t\t\tconst struct packed_git *p;\n+\t\t\tif ((flags & OBJECT_INFO_LOOKUP_REPLACE) && !oideq(real, oid))\n+\t\t\t\tdie(_(\"replacement %s not found for %s\"),\n+\t\t\t\t oid_to_hex(real), oid_to_hex(oid));\n+\t\t\tif ((p = has_packed_and_bad(r, real)))\n+\t\t\t\tdie(_(\"packed object %s (stored in %s) is corrupt\"),\n+\t\t\t\t oid_to_hex(real), p->pack_name);\n+\t\t}\n \t\treturn -1;\n \t}\n \n@@ -1653,7 +1673,8 @@ int oid_object_info(struct repository *r,\n \n static void *read_object(struct repository *r,\n \t\t\t const struct object_id *oid, enum object_type *type,\n-\t\t\t unsigned long *size)\n+\t\t\t unsigned long *size,\n+\t\t\t int die_if_corrupt)\n {\n \tstruct object_info oi = OBJECT_INFO_INIT;\n \tvoid *content;\n@@ -1661,7 +1682,8 @@ static void *read_object(struct repository *r,\n \toi.sizep = size;\n \toi.contentp = &content;\n \n-\tif (oid_object_info_extended(r, oid, &oi, 0) < 0)\n+\tif (oid_object_info_extended(r, oid, &oi, die_if_corrupt\n+\t\t\t\t ? OBJECT_INFO_DIE_IF_CORRUPT : 0) < 0)\n \t\treturn NULL;\n \treturn content;\n }\n@@ -1697,35 +1719,14 @@ void *read_object_file_extended(struct repository *r,\n \t\t\t\tint lookup_replace)\n {\n \tvoid *data;\n-\tconst struct packed_git *p;\n-\tconst char *path;\n-\tstruct stat st;\n \tconst struct object_id *repl = lookup_replace ?\n \t\tlookup_replace_object(r, oid) : oid;\n \n \terrno = 0;\n-\tdata = read_object(r, repl, type, size);\n+\tdata = read_object(r, repl, type, size, 1);\n \tif (data)\n \t\treturn data;\n \n-\tobj_read_lock();\n-\tif (errno && errno != ENOENT)\n-\t\tdie_errno(_(\"failed to read object %s\"), oid_to_hex(oid));\n-\n-\t/* die if we replaced an object with one that does not exist */\n-\tif (repl != oid)\n-\t\tdie(_(\"replacement %s not found for %s\"),\n-\t\t oid_to_hex(repl), oid_to_hex(oid));\n-\n-\tif (!stat_loose_object(r, repl, &st, &path))\n-\t\tdie(_(\"loose object %s (stored in %s) is corrupt\"),\n-\t\t oid_to_hex(repl), path);\n-\n-\tif ((p = has_packed_and_bad(r, repl)))\n-\t\tdie(_(\"packed object %s (stored in %s) is corrupt\"),\n-\t\t oid_to_hex(repl), p->pack_name);\n-\tobj_read_unlock();\n-\n \treturn NULL;\n }\n \n@@ -2268,7 +2269,7 @@ int force_object_loose(const struct object_id *oid, time_t mtime)\n \n \tif (has_loose_object(oid))\n \t\treturn 0;\n-\tbuf = read_object(the_repository, oid, &type, &len);\n+\tbuf = read_object(the_repository, oid, &type, &len, 0);\n \tif (!buf)\n \t\treturn error(_(\"cannot read object for %s\"), oid_to_hex(oid));\n \thdrlen = format_object_header(hdr, sizeof(hdr), type, len);\ndiff --git a/object-store.h b/object-store.h\nindex b1ec0bde82..98c1d67946 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -445,6 +445,9 @@ struct object_info {\n */\n #define OBJECT_INFO_FOR_PREFETCH (OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK)\n \n+/* Die if object corruption (not just an object being missing) was detected. */\n+#define OBJECT_INFO_DIE_IF_CORRUPT 32\n+\n int oid_object_info_extended(struct repository *r,\n \t\t\t const struct object_id *,\n \t\t\t struct object_info *, unsigned flags);\n-- \n2.39.0.rc1.256.g54fd8350bd-goog\n\n"},{"id":"468964","messageId":"20221212225212.2556886-1-jonathantanmy@google.com","threadId":"58877","inReplyTo":"Y5ebC1qwJi5VwnCh@coredump.intra.peff.net","subject":"Re: [PATCH v4 3/4] object-file: emit corruption errors when detected","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-12-12T22:52:12Z","receivedAt":"2022-12-12T22:52:18Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Jeff King writes:\n> On Mon, Dec 12, 2022 at 12:59:55PM -0800, Jonathan Tan wrote:\n> \n> > > And possibly put a comment above open_loose_object() that \"path\"\n> > > is only guaranteed to point to something sensible when a non-negative\n> > > value is returned.\n> > \n> > Junio made a point that there could, for example, be no path when the\n> > odb list is empty (maybe in the future) so I don't think this would be\n> > sufficient. But there is already a comment there pointing to a comment\n> > in another function that states \"path ... (if any)\" so this is something\n> > that callers should already take care of. In my changes, I'll initialize\n> > it to NULL and whenever I use it, I'll check for non-NULL first.\n> \n> If we return a non-negative value, then we opened something, so by\n> definition, don't we have a path of the thing we opened?\n\nHmm...are you saying \"path is guaranteed when there is no error; when\nthere is an error, the caller must check\"? If yes, I think we are in\nagreement. In any case, to make things more concrete, I've just sent a\nnew version [1].\n\n[1] https://lore.kernel.org/git/cover.1670885252.git.jonathantanmy@google.com/\n"},{"id":"468969","messageId":"xmqqzgbsoyte.fsf@gitster.g","threadId":"58877","inReplyTo":"a229ea0b1122f55e91f98475cd7e508f4dd8501a.1670885252.git.jonathantanmy@google.com","subject":"Re: [PATCH v5 3/4] object-file: emit corruption errors when detected","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-13T01:51:57Z","receivedAt":"2022-12-13T01:52:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> diff --git a/object-file.c b/object-file.c\n> index 429e3a746d..e0cef8b906 100644\n> --- a/object-file.c\n> +++ b/object-file.c\n> @@ -1422,7 +1422,9 @@ static int loose_object_info(struct repository *r,\n> \t\t\t struct object_info *oi, int flags)\n> {\n> \tint status = 0;\n> +\tint fd;\n> \tunsigned long mapsize;\n> +\tconst char *path = NULL;\n\nIt may be OK to leave this uninitialized, as long as the contract of\nopen_loose_object() is that a successful opening will report the path\nto the loose object file that was opened. Because ...\n\n> @@ -1454,7 +1455,13 @@ static int loose_object_info(struct repository *r,\n> \t\treturn 0;\n> \t}\n> \n> -\tmap = map_loose_object(r, oid, &mapsize);\n> +\tfd = open_loose_object(r, oid, &path);\n> +\tif (fd < 0) {\n> +\t\tif (errno != ENOENT)\n> +\t\t\terror_errno(_(\"unable to open loose object %s\"), oid_to_hex(oid));\n> +\t\treturn -1;\n\n... we no longer refer to \"path\" which may not be populated here, and ...\n\n> +\t}\n\n... at this point, we know we successfully opened, and populated path.\n\n> +\tmap = map_fd(fd, path, &mapsize);\n> \tif (!map)\n> \t\treturn -1;\n> \n> @@ -1492,6 +1499,10 @@ static int loose_object_info(struct repository *r,\n> \t\tbreak;\n> \t}\n> \n> +\tif (status && path && (flags & OBJECT_INFO_DIE_IF_CORRUPT))\n\nAlso, here, \"path\" should be valid, as we have successfully opened\nthe loose object file (otherwise we would have hit the early return\nthat reports only the oid_to_hex(oid)).\n\n> +\t\tdie(_(\"loose object %s (stored in %s) is corrupt\"),\n> +\t\t oid_to_hex(oid), path);\n\nIf we didn't have path and did not die, we'd end up ignoring\nDIE_IF_CORRUPT request and force the caller to handle non-zero\nstatus. But I do not think that should happen, because path would\nbe valid here. No?\n"},{"id":"468975","messageId":"Y5hV1IC7c4bmEXnN@coredump.intra.peff.net","threadId":"58877","inReplyTo":"20221212225212.2556886-1-jonathantanmy@google.com","subject":"Re: [PATCH v4 3/4] object-file: emit corruption errors when detected","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-12-13T10:37:08Z","receivedAt":"2022-12-13T10:37:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 12, 2022 at 02:52:12PM -0800, Jonathan Tan wrote:\n\n> > > Junio made a point that there could, for example, be no path when the\n> > > odb list is empty (maybe in the future) so I don't think this would be\n> > > sufficient. But there is already a comment there pointing to a comment\n> > > in another function that states \"path ... (if any)\" so this is something\n> > > that callers should already take care of. In my changes, I'll initialize\n> > > it to NULL and whenever I use it, I'll check for non-NULL first.\n> > \n> > If we return a non-negative value, then we opened something, so by\n> > definition, don't we have a path of the thing we opened?\n> \n> Hmm...are you saying \"path is guaranteed when there is no error; when\n> there is an error, the caller must check\"? If yes, I think we are in\n> agreement. In any case, to make things more concrete, I've just sent a\n> new version [1].\n\nAlmost. I'm saying \"path is guaranteed when there is no error; when\nthere is an error, the value of path is meaningless and should not be\nlooked at\".\n\nIf you want to enforce that open_loose_object() sets \"path\" to NULL on\nerror, and then say \"the caller must check\", that would be valid. But\nwithout that, even checking it for NULL is pointless (because you may\nsee a path which got ENOENT, even though we got an interesting errno\nfrom an earlier path).\n\n-Peff\n"},{"id":"468977","messageId":"Y5hWCa31OVLOU3sK@coredump.intra.peff.net","threadId":"58877","inReplyTo":"xmqqzgbsoyte.fsf@gitster.g","subject":"Re: [PATCH v5 3/4] object-file: emit corruption errors when detected","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-12-13T10:38:01Z","receivedAt":"2022-12-13T10:38:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 13, 2022 at 10:51:57AM +0900, Junio C Hamano wrote:\n\n> Jonathan Tan writes:\n> \n> > diff --git a/object-file.c b/object-file.c\n> > index 429e3a746d..e0cef8b906 100644\n> > --- a/object-file.c\n> > +++ b/object-file.c\n> > @@ -1422,7 +1422,9 @@ static int loose_object_info(struct repository *r,\n> > \t\t\t struct object_info *oi, int flags)\n> > {\n> > \tint status = 0;\n> > +\tint fd;\n> > \tunsigned long mapsize;\n> > +\tconst char *path = NULL;\n> \n> It may be OK to leave this uninitialized, as long as the contract of\n> open_loose_object() is that a successful opening will report the path\n> to the loose object file that was opened. Because ...\n\nYeah, I'd agree that this initialization can be left off (and that the\nNULL checks later in the function are not needed).\n\n-Peff\n"},{"id":"469028","messageId":"cover.1671045259.git.jonathantanmy@google.com","threadId":"58877","inReplyTo":"cover.1669839849.git.jonathantanmy@google.com","subject":"[PATCH v6 0/4] Don't lazy-fetch commits when parsing them","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-12-14T19:17:39Z","receivedAt":"2022-12-14T19:17:53Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Thanks everyone once again and sorry for the churn. Hopefully I got it\nright this time.\n\nopen_loose_object() is documented to return the path of the object\nwe found, so I think we already have that covered (if we detect that\nan object is corrupt, it follows that we would already have found the\nobject in the first place).\n\nJonathan Tan (4):\n object-file: remove OBJECT_INFO_IGNORE_LOOSE\n object-file: refactor map_loose_object_1()\n object-file: emit corruption errors when detected\n commit: don't lazy-fetch commits\n\n commit.c | 15 ++++++-\n object-file.c | 108 ++++++++++++++++++++++++-------------------------\n object-store.h | 7 ++--\n 3 files changed, 69 insertions(+), 61 deletions(-)\n\nRange-diff against v5:\n1: be0b08cac2 = 1: be0b08cac2 object-file: remove OBJECT_INFO_IGNORE_LOOSE\n2: 4b2fb68743 = 2: 4b2fb68743 object-file: refactor map_loose_object_1()\n3: a229ea0b11 ! 3: 811620909a object-file: emit corruption errors when detected\n @@ object-file.c: static int loose_object_info(struct repository *r,\n \tint status = 0;\n +\tint fd;\n \tunsigned long mapsize;\n -+\tconst char *path = NULL;\n ++\tconst char *path;\n \tvoid *map;\n \tgit_zstream stream;\n \tchar hdr[MAX_HEADER_LEN];\n @@ object-file.c: static int loose_object_info(struct repository *r,\n \t\tbreak;\n \t}\n \n -+\tif (status && path && (flags & OBJECT_INFO_DIE_IF_CORRUPT))\n ++\tif (status && (flags & OBJECT_INFO_DIE_IF_CORRUPT))\n +\t\tdie(_(\"loose object %s (stored in %s) is corrupt\"),\n +\t\t oid_to_hex(oid), path);\n +\n4: b54972118a = 4: 8acf1a29e7 commit: don't lazy-fetch commits\n-- \n2.39.0.314.g84b9a713c41-goog\n\n"},{"id":"469029","messageId":"be0b08cac219357e1ce9cd46fd7ab5c13344699b.1671045259.git.jonathantanmy@google.com","threadId":"58877","inReplyTo":"cover.1671045259.git.jonathantanmy@google.com","subject":"[PATCH v6 1/4] object-file: remove OBJECT_INFO_IGNORE_LOOSE","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-12-14T19:17:40Z","receivedAt":"2022-12-14T19:17:56Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Its last user was removed in 97b2fa08b6 (fetch-pack: drop\ncustom loose object cache, 2018-11-12), so we can remove it.\n\nHelped-by: Jeff King \nSigned-off-by: Jonathan Tan \n---\n object-file.c | 3 ---\n object-store.h | 4 +---\n 2 files changed, 1 insertion(+), 6 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex 26290554bb..cf724bc19b 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1575,9 +1575,6 @@ static int do_oid_object_info_extended(struct repository *r,\n \t\tif (find_pack_entry(r, real, &e))\n \t\t\tbreak;\n \n-\t\tif (flags & OBJECT_INFO_IGNORE_LOOSE)\n-\t\t\treturn -1;\n-\n \t\t/* Most likely it's a loose object. */\n \t\tif (!loose_object_info(r, real, oi, flags))\n \t\t\treturn 0;\ndiff --git a/object-store.h b/object-store.h\nindex 1be57abaf1..b1ec0bde82 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -434,13 +434,11 @@ struct object_info {\n #define OBJECT_INFO_ALLOW_UNKNOWN_TYPE 2\n /* Do not retry packed storage after checking packed and loose storage */\n #define OBJECT_INFO_QUICK 8\n-/* Do not check loose object */\n-#define OBJECT_INFO_IGNORE_LOOSE 16\n /*\n * Do not attempt to fetch the object if missing (even if fetch_is_missing is\n * nonzero).\n */\n-#define OBJECT_INFO_SKIP_FETCH_OBJECT 32\n+#define OBJECT_INFO_SKIP_FETCH_OBJECT 16\n /*\n * This is meant for bulk prefetching of missing blobs in a partial\n * clone. Implies OBJECT_INFO_SKIP_FETCH_OBJECT and OBJECT_INFO_QUICK\n-- \n2.39.0.314.g84b9a713c41-goog\n\n"},{"id":"469030","messageId":"4b2fb687432c2ce1471d9eb02e86b3acc43cc953.1671045259.git.jonathantanmy@google.com","threadId":"58877","inReplyTo":"cover.1671045259.git.jonathantanmy@google.com","subject":"[PATCH v6 2/4] object-file: refactor map_loose_object_1()","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-12-14T19:17:41Z","receivedAt":"2022-12-14T19:17:58Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"This function can do 3 things:\n 1. Gets an fd given a path\n 2. Simultaneously gets a path and fd given an OID\n 3. Memory maps an fd\n\nKeep 3 (renaming the function accordingly) and inline 1 and 2 into their\nrespective callers.\n\nSigned-off-by: Jonathan Tan \n---\n object-file.c | 50 ++++++++++++++++++++++++--------------------------\n 1 file changed, 24 insertions(+), 26 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex cf724bc19b..429e3a746d 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1211,35 +1211,25 @@ static int quick_has_loose(struct repository *r,\n }\n \n /*\n- * Map the loose object at \"path\" if it is not NULL, or the path found by\n- * searching for a loose object named \"oid\".\n+ * Map and close the given loose object fd. The path argument is used for\n+ * error reporting.\n */\n-static void *map_loose_object_1(struct repository *r, const char *path,\n-\t\t\t const struct object_id *oid, unsigned long *size)\n+static void *map_fd(int fd, const char *path, unsigned long *size)\n {\n-\tvoid *map;\n-\tint fd;\n-\n-\tif (path)\n-\t\tfd = git_open(path);\n-\telse\n-\t\tfd = open_loose_object(r, oid, &path);\n-\tmap = NULL;\n-\tif (fd >= 0) {\n-\t\tstruct stat st;\n+\tvoid *map = NULL;\n+\tstruct stat st;\n \n-\t\tif (!fstat(fd, &st)) {\n-\t\t\t*size = xsize_t(st.st_size);\n-\t\t\tif (!*size) {\n-\t\t\t\t/* mmap() is forbidden on empty files */\n-\t\t\t\terror(_(\"object file %s is empty\"), path);\n-\t\t\t\tclose(fd);\n-\t\t\t\treturn NULL;\n-\t\t\t}\n-\t\t\tmap = xmmap(NULL, *size, PROT_READ, MAP_PRIVATE, fd, 0);\n+\tif (!fstat(fd, &st)) {\n+\t\t*size = xsize_t(st.st_size);\n+\t\tif (!*size) {\n+\t\t\t/* mmap() is forbidden on empty files */\n+\t\t\terror(_(\"object file %s is empty\"), path);\n+\t\t\tclose(fd);\n+\t\t\treturn NULL;\n \t\t}\n-\t\tclose(fd);\n+\t\tmap = xmmap(NULL, *size, PROT_READ, MAP_PRIVATE, fd, 0);\n \t}\n+\tclose(fd);\n \treturn map;\n }\n \n@@ -1247,7 +1237,12 @@ void *map_loose_object(struct repository *r,\n \t\t const struct object_id *oid,\n \t\t unsigned long *size)\n {\n-\treturn map_loose_object_1(r, NULL, oid, size);\n+\tconst char *p;\n+\tint fd = open_loose_object(r, oid, &p);\n+\n+\tif (fd < 0)\n+\t\treturn NULL;\n+\treturn map_fd(fd, p, size);\n }\n \n enum unpack_loose_header_result unpack_loose_header(git_zstream *stream,\n@@ -2789,13 +2784,16 @@ int read_loose_object(const char *path,\n \t\t struct object_info *oi)\n {\n \tint ret = -1;\n+\tint fd;\n \tvoid *map = NULL;\n \tunsigned long mapsize;\n \tgit_zstream stream;\n \tchar hdr[MAX_HEADER_LEN];\n \tunsigned long *size = oi->sizep;\n \n-\tmap = map_loose_object_1(the_repository, path, NULL, &mapsize);\n+\tfd = git_open(path);\n+\tif (fd >= 0)\n+\t\tmap = map_fd(fd, path, &mapsize);\n \tif (!map) {\n \t\terror_errno(_(\"unable to mmap %s\"), path);\n \t\tgoto out;\n-- \n2.39.0.314.g84b9a713c41-goog\n\n"},{"id":"469031","messageId":"811620909a9efff9de6cb5c3a1076da0f13e3875.1671045259.git.jonathantanmy@google.com","threadId":"58877","inReplyTo":"cover.1671045259.git.jonathantanmy@google.com","subject":"[PATCH v6 3/4] object-file: emit corruption errors when detected","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-12-14T19:17:42Z","receivedAt":"2022-12-14T19:18:12Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Instead of relying on errno being preserved across function calls, teach\ndo_oid_object_info_extended() to itself report object corruption when\nit first detects it. There are 3 types of corruption being detected:\n - when a replacement object is missing\n - when a loose object is corrupt\n - when a packed object is corrupt and the object cannot be read\n in another way\n\nNote that in the RHS of this patch's diff, a check for ENOENT that was\nintroduced in 3ba7a06552 (A loose object is not corrupt if it cannot\nbe read due to EMFILE, 2010-10-28) is also removed. The purpose of this\ncheck is to avoid a false report of corruption if the errno contains\nsomething like EMFILE (or anything that is not ENOENT), in which case\na more generic report is presented. Because, as of this patch, we no\nlonger rely on such a heuristic to determine corruption, but surface\nthe error message at the point when we read something that we did not\nexpect, this check is no longer necessary.\n\nBesides being more resilient, this also prepares for a future patch in\nwhich an indirect caller of do_oid_object_info_extended() will need\nsuch functionality.\n\nHelped-by: Jeff King \nSigned-off-by: Jonathan Tan \n---\n object-file.c | 55 +++++++++++++++++++++++++-------------------------\n object-store.h | 3 +++\n 2 files changed, 31 insertions(+), 27 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex 429e3a746d..e55697e378 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1422,7 +1422,9 @@ static int loose_object_info(struct repository *r,\n \t\t\t struct object_info *oi, int flags)\n {\n \tint status = 0;\n+\tint fd;\n \tunsigned long mapsize;\n+\tconst char *path;\n \tvoid *map;\n \tgit_zstream stream;\n \tchar hdr[MAX_HEADER_LEN];\n@@ -1443,7 +1445,6 @@ static int loose_object_info(struct repository *r,\n \t * object even exists.\n \t */\n \tif (!oi->typep && !oi->type_name && !oi->sizep && !oi->contentp) {\n-\t\tconst char *path;\n \t\tstruct stat st;\n \t\tif (!oi->disk_sizep && (flags & OBJECT_INFO_QUICK))\n \t\t\treturn quick_has_loose(r, oid) ? 0 : -1;\n@@ -1454,7 +1455,13 @@ static int loose_object_info(struct repository *r,\n \t\treturn 0;\n \t}\n \n-\tmap = map_loose_object(r, oid, &mapsize);\n+\tfd = open_loose_object(r, oid, &path);\n+\tif (fd < 0) {\n+\t\tif (errno != ENOENT)\n+\t\t\terror_errno(_(\"unable to open loose object %s\"), oid_to_hex(oid));\n+\t\treturn -1;\n+\t}\n+\tmap = map_fd(fd, path, &mapsize);\n \tif (!map)\n \t\treturn -1;\n \n@@ -1492,6 +1499,10 @@ static int loose_object_info(struct repository *r,\n \t\tbreak;\n \t}\n \n+\tif (status && (flags & OBJECT_INFO_DIE_IF_CORRUPT))\n+\t\tdie(_(\"loose object %s (stored in %s) is corrupt\"),\n+\t\t oid_to_hex(oid), path);\n+\n \tgit_inflate_end(&stream);\n cleanup:\n \tmunmap(map, mapsize);\n@@ -1601,6 +1612,15 @@ static int do_oid_object_info_extended(struct repository *r,\n \t\t\tcontinue;\n \t\t}\n \n+\t\tif (flags & OBJECT_INFO_DIE_IF_CORRUPT) {\n+\t\t\tconst struct packed_git *p;\n+\t\t\tif ((flags & OBJECT_INFO_LOOKUP_REPLACE) && !oideq(real, oid))\n+\t\t\t\tdie(_(\"replacement %s not found for %s\"),\n+\t\t\t\t oid_to_hex(real), oid_to_hex(oid));\n+\t\t\tif ((p = has_packed_and_bad(r, real)))\n+\t\t\t\tdie(_(\"packed object %s (stored in %s) is corrupt\"),\n+\t\t\t\t oid_to_hex(real), p->pack_name);\n+\t\t}\n \t\treturn -1;\n \t}\n \n@@ -1653,7 +1673,8 @@ int oid_object_info(struct repository *r,\n \n static void *read_object(struct repository *r,\n \t\t\t const struct object_id *oid, enum object_type *type,\n-\t\t\t unsigned long *size)\n+\t\t\t unsigned long *size,\n+\t\t\t int die_if_corrupt)\n {\n \tstruct object_info oi = OBJECT_INFO_INIT;\n \tvoid *content;\n@@ -1661,7 +1682,8 @@ static void *read_object(struct repository *r,\n \toi.sizep = size;\n \toi.contentp = &content;\n \n-\tif (oid_object_info_extended(r, oid, &oi, 0) < 0)\n+\tif (oid_object_info_extended(r, oid, &oi, die_if_corrupt\n+\t\t\t\t ? OBJECT_INFO_DIE_IF_CORRUPT : 0) < 0)\n \t\treturn NULL;\n \treturn content;\n }\n@@ -1697,35 +1719,14 @@ void *read_object_file_extended(struct repository *r,\n \t\t\t\tint lookup_replace)\n {\n \tvoid *data;\n-\tconst struct packed_git *p;\n-\tconst char *path;\n-\tstruct stat st;\n \tconst struct object_id *repl = lookup_replace ?\n \t\tlookup_replace_object(r, oid) : oid;\n \n \terrno = 0;\n-\tdata = read_object(r, repl, type, size);\n+\tdata = read_object(r, repl, type, size, 1);\n \tif (data)\n \t\treturn data;\n \n-\tobj_read_lock();\n-\tif (errno && errno != ENOENT)\n-\t\tdie_errno(_(\"failed to read object %s\"), oid_to_hex(oid));\n-\n-\t/* die if we replaced an object with one that does not exist */\n-\tif (repl != oid)\n-\t\tdie(_(\"replacement %s not found for %s\"),\n-\t\t oid_to_hex(repl), oid_to_hex(oid));\n-\n-\tif (!stat_loose_object(r, repl, &st, &path))\n-\t\tdie(_(\"loose object %s (stored in %s) is corrupt\"),\n-\t\t oid_to_hex(repl), path);\n-\n-\tif ((p = has_packed_and_bad(r, repl)))\n-\t\tdie(_(\"packed object %s (stored in %s) is corrupt\"),\n-\t\t oid_to_hex(repl), p->pack_name);\n-\tobj_read_unlock();\n-\n \treturn NULL;\n }\n \n@@ -2268,7 +2269,7 @@ int force_object_loose(const struct object_id *oid, time_t mtime)\n \n \tif (has_loose_object(oid))\n \t\treturn 0;\n-\tbuf = read_object(the_repository, oid, &type, &len);\n+\tbuf = read_object(the_repository, oid, &type, &len, 0);\n \tif (!buf)\n \t\treturn error(_(\"cannot read object for %s\"), oid_to_hex(oid));\n \thdrlen = format_object_header(hdr, sizeof(hdr), type, len);\ndiff --git a/object-store.h b/object-store.h\nindex b1ec0bde82..98c1d67946 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -445,6 +445,9 @@ struct object_info {\n */\n #define OBJECT_INFO_FOR_PREFETCH (OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK)\n \n+/* Die if object corruption (not just an object being missing) was detected. */\n+#define OBJECT_INFO_DIE_IF_CORRUPT 32\n+\n int oid_object_info_extended(struct repository *r,\n \t\t\t const struct object_id *,\n \t\t\t struct object_info *, unsigned flags);\n-- \n2.39.0.314.g84b9a713c41-goog\n\n"},{"id":"469032","messageId":"8acf1a29e7586354a800dcce1a6237448f914c7c.1671045259.git.jonathantanmy@google.com","threadId":"58877","inReplyTo":"cover.1671045259.git.jonathantanmy@google.com","subject":"[PATCH v6 4/4] commit: don't lazy-fetch commits","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-12-14T19:17:43Z","receivedAt":"2022-12-14T19:18:24Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When parsing commits, fail fast when the commit is missing or\ncorrupt, instead of attempting to fetch them. This is done by inlining\nrepo_read_object_file() and setting the flag that prevents fetching.\n\nThis is motivated by a situation in which through a bug (not necessarily\nthrough Git), there was corruption in the object store of a partial\nclone. In this particular case, the problem was exposed when \"git gc\"\ntried to expire reflogs, which calls repo_parse_commit(), which triggers\nfetches of the missing commits.\n\n(There are other possible solutions to this problem including passing an\nargument from \"git gc\" to \"git reflog\" to inhibit all lazy fetches, but\nI think that this fix is at the wrong level - fixing \"git reflog\" means\nthat this particular command works fine, or so we think (it will fail if\nit somehow needs to read a legitimately missing blob, say, a .gitmodules\nfile), but fixing repo_parse_commit() will fix a whole class of bugs.)\n\nSigned-off-by: Jonathan Tan \nSigned-off-by: Junio C Hamano \n---\n commit.c | 15 +++++++++++++--\n 1 file changed, 13 insertions(+), 2 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex 572301b80a..a02723f06b 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -508,6 +508,17 @@ int repo_parse_commit_internal(struct repository *r,\n \tenum object_type type;\n \tvoid *buffer;\n \tunsigned long size;\n+\tstruct object_info oi = {\n+\t\t.typep = &type,\n+\t\t.sizep = &size,\n+\t\t.contentp = &buffer,\n+\t};\n+\t/*\n+\t * Git does not support partial clones that exclude commits, so set\n+\t * OBJECT_INFO_SKIP_FETCH_OBJECT to fail fast when an object is missing.\n+\t */\n+\tint flags = OBJECT_INFO_LOOKUP_REPLACE | OBJECT_INFO_SKIP_FETCH_OBJECT |\n+\t\tOBJECT_INFO_DIE_IF_CORRUPT;\n \tint ret;\n \n \tif (!item)\n@@ -516,8 +527,8 @@ int repo_parse_commit_internal(struct repository *r,\n \t\treturn 0;\n \tif (use_commit_graph && parse_commit_in_graph(r, item))\n \t\treturn 0;\n-\tbuffer = repo_read_object_file(r, &item->object.oid, &type, &size);\n-\tif (!buffer)\n+\n+\tif (oid_object_info_extended(r, &item->object.oid, &oi, flags) < 0)\n \t\treturn quiet_on_missing ? -1 :\n \t\t\terror(\"Could not read %s\",\n \t\t\t oid_to_hex(&item->object.oid));\n-- \n2.39.0.314.g84b9a713c41-goog\n\n"},{"id":"469042","messageId":"Y5o1d6f2cepf7Vp6@coredump.intra.peff.net","threadId":"58877","inReplyTo":"cover.1671045259.git.jonathantanmy@google.com","subject":"Re: [PATCH v6 0/4] Don't lazy-fetch commits when parsing them","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-12-14T20:43:35Z","receivedAt":"2022-12-14T20:43:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 14, 2022 at 11:17:39AM -0800, Jonathan Tan wrote:\n\n> Thanks everyone once again and sorry for the churn. Hopefully I got it\n> right this time.\n> \n> open_loose_object() is documented to return the path of the object\n> we found, so I think we already have that covered (if we detect that\n> an object is corrupt, it follows that we would already have found the\n> object in the first place).\n\nThis version looks good to me. Thanks for your persistence. :) I think\nthe end result is very nicely done.\n\n-Peff\n"},{"id":"469061","messageId":"xmqqy1r9ttq9.fsf@gitster.g","threadId":"58877","inReplyTo":"Y5o1d6f2cepf7Vp6@coredump.intra.peff.net","subject":"Re: [PATCH v6 0/4] Don't lazy-fetch commits when parsing them","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-15T00:07:26Z","receivedAt":"2022-12-15T00:13:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Dec 14, 2022 at 11:17:39AM -0800, Jonathan Tan wrote:\n>\n>> Thanks everyone once again and sorry for the churn. Hopefully I got it\n>> right this time.\n>> \n>> open_loose_object() is documented to return the path of the object\n>> we found, so I think we already have that covered (if we detect that\n>> an object is corrupt, it follows that we would already have found the\n>> object in the first place).\n>\n> This version looks good to me. Thanks for your persistence. :) I think\n> the end result is very nicely done.\n\nYeah, this looks good. Nothing added to or removed from the\nprevious round other than what I found questionable during the\nreview of that round.\n\nThanks, both.\n"}]}