On Wed, Nov 30, 2022 at 12:30:49PM -0800, Jonathan Tan wrote: > When parsing commits, fail fast when the commit is missing or > corrupt, instead of attempting to fetch them. This is done by inlining > repo_read_object_file() and setting the flag that prevents fetching. > > This is motivated by a situation in which through a bug (not necessarily > through Git), there was corruption in the object store of a partial > clone. In this particular case, the problem was exposed when "git gc" > tried to expire reflogs, which calls repo_parse_commit(), which triggers > fetches of the missing commits. Makes sense. > diff --git a/commit.c b/commit.c > index 572301b80a..17e71f5be4 100644 > --- a/commit.c > +++ b/commit.c > @@ -508,6 +508,13 @@ int repo_parse_commit_internal(struct repository *r, > enum object_type type; > void *buffer; > unsigned long size; > + const struct object_id *real_oid; > + struct object_info oi = { > + .typep = &type, > + .sizep = &size, > + .contentp = &buffer, > + .real_oidp = &real_oid, > + }; > int ret; > > if (!item) > @@ -516,11 +523,18 @@ int repo_parse_commit_internal(struct repository *r, > return 0; > if (use_commit_graph && parse_commit_in_graph(r, item)) > return 0; > - buffer = repo_read_object_file(r, &item->object.oid, &type, &size); > - if (!buffer) > + > + /* > + * Git does not support partial clones that exclude commits, so set > + * OBJECT_INFO_SKIP_FETCH_OBJECT to fail fast when an object is missing. > + */ > + if (oid_object_info_extended(r, &item->object.oid, &oi, > + OBJECT_INFO_LOOKUP_REPLACE | OBJECT_INFO_SKIP_FETCH_OBJECT) < 0) { > + die_if_corrupt(r, &item->object.oid, real_oid); > return quiet_on_missing ? -1 : > error("Could not read %s", > oid_to_hex(&item->object.oid)); > + } OK, so we know we want a commit object because we're in the commit-parsing function, so we just ask to disable fetching. Two devil's advocate thoughts: 1. What if we're wrong that it's a commit? If somebody references a blob in a commit "parent" header, the "right" outcome is for us to say "oops, the type is wrong" when we try to parse it as a commit. But now in a partial clone, we might avoid fetching that supposed commit and say "you don't have this object", even though we could get it. I think I'm OK with that. Either way, the repo is corrupt, and we'll have informed the user of that. The fact that this bizarre and specific sequence of corruptions might not go as far as it can to deduce the root cause is probably fine. 2. Are there other places where we'd want to do the same thing? E.g., in parse_object() we might ask for an object (not knowing its type) only to find out that it is a commit. But we have no idea if we lazy-fetched it or not! I had somehow imagined your series would be hooking in at the level of the lazy-fetch code, and complaining about fetching commits. But that may be tricky to do, because we really don't know the type until after we fetch it, and selectively removing an object from the odb is quite hard. We'd probably have to tell the other side "please, don't send me any commits", which requires a protocol extension, and...yuck. By comparison, your approach is an easy win that may catch problems in practice (and is certainly better than the status quo). -Peff