| Message ID | 9ad34a1dce977044066de0bfa6e25977215e8dc9.1670373420.git.jonathantanmy@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Don't lazy-fetch commits when parsing them | expand |
Jonathan Tan <jonathantanmy@google.com> writes: > Instead, also search the submodule object stores and promisor remotes. > > This also centralizes what happens when the object is not found (the > "return -1"), which is useful for a subsequent patch. > > Signed-off-by: Jonathan Tan <jonathantanmy@google.com> > --- > object-file.c | 23 +++++++++++------------ > 1 file changed, 11 insertions(+), 12 deletions(-) > > diff --git a/object-file.c b/object-file.c > index 26290554bb..596dd049fd 100644 > --- a/object-file.c > +++ b/object-file.c > @@ -1575,18 +1575,17 @@ static int do_oid_object_info_extended(struct repository *r, > if (find_pack_entry(r, real, &e)) > break; > > - if (flags & OBJECT_INFO_IGNORE_LOOSE) > - return -1; > - > - /* Most likely it's a loose object. */ > - if (!loose_object_info(r, real, oi, flags)) > - return 0; > - > - /* Not a loose object; someone else may have just packed it. */ > - if (!(flags & OBJECT_INFO_QUICK)) { > - reprepare_packed_git(r); > - if (find_pack_entry(r, real, &e)) > - break; > + if (!(flags & OBJECT_INFO_IGNORE_LOOSE)) { > + /* Most likely it's a loose object. */ > + if (!loose_object_info(r, real, oi, flags)) > + return 0; > + > + /* Not a loose object; someone else may have just packed it. */ > + if (!(flags & OBJECT_INFO_QUICK)) { > + reprepare_packed_git(r); > + if (find_pack_entry(r, real, &e)) > + break; > + } > } Hmph, who passes IGNORE_LOOSE and why? Explaining the answer to that question would give us confidence why this change is safe. If the reason IGNORE_LOOSE is set by the callers is because they are interested only in locally packed objects, then this change would break them because they end up triggering the lazy fetch in the updated code, no? Or do all callers that set IGNORE_LOOSE drop the fetch_if_missing global before calling us? Thanks.
On Wed, Dec 07, 2022 at 10:12:13AM +0900, Junio C Hamano wrote: > Hmph, who passes IGNORE_LOOSE and why? Explaining the answer to > that question would give us confidence why this change is safe. > > If the reason IGNORE_LOOSE is set by the callers is because they are > interested only in locally packed objects, then this change would > break them because they end up triggering the lazy fetch in the > updated code, no? Or do all callers that set IGNORE_LOOSE drop the > fetch_if_missing global before calling us? I wondered who those callers might be, too, because it is such a weird thing for a caller to want to care about (usually we try to abstract the object database). It looks like the only user went away in 97b2fa08b6 (fetch-pack: drop custom loose object cache, 2018-11-12). So I think we just want to drop it: diff --git a/object-file.c b/object-file.c index 26290554bb..cf724bc19b 100644 --- a/object-file.c +++ b/object-file.c @@ -1575,9 +1575,6 @@ static int do_oid_object_info_extended(struct repository *r, if (find_pack_entry(r, real, &e)) break; - if (flags & OBJECT_INFO_IGNORE_LOOSE) - return -1; - /* Most likely it's a loose object. */ if (!loose_object_info(r, real, oi, flags)) return 0; diff --git a/object-store.h b/object-store.h index 1be57abaf1..371629c1e1 100644 --- a/object-store.h +++ b/object-store.h @@ -434,8 +434,6 @@ struct object_info { #define OBJECT_INFO_ALLOW_UNKNOWN_TYPE 2 /* Do not retry packed storage after checking packed and loose storage */ #define OBJECT_INFO_QUICK 8 -/* Do not check loose object */ -#define OBJECT_INFO_IGNORE_LOOSE 16 /* * Do not attempt to fetch the object if missing (even if fetch_is_missing is * nonzero). We could also renumber the later flags to keep them compact, but I don't have a strong opinion there. -Peff
Jeff King <peff@peff.net> writes: > I wondered who those callers might be, too, because it is such a weird > thing for a caller to want to care about (usually we try to abstract the > object database). Exactly. > It looks like the only user went away in 97b2fa08b6 (fetch-pack: drop > custom loose object cache, 2018-11-12). Nice, very nice. > So I think we just want to drop it: > > diff --git a/object-file.c b/object-file.c > index 26290554bb..cf724bc19b 100644 > --- a/object-file.c > +++ b/object-file.c > @@ -1575,9 +1575,6 @@ static int do_oid_object_info_extended(struct repository *r, > if (find_pack_entry(r, real, &e)) > break; > > - if (flags & OBJECT_INFO_IGNORE_LOOSE) > - return -1; > - > /* Most likely it's a loose object. */ > if (!loose_object_info(r, real, oi, flags)) > return 0; > diff --git a/object-store.h b/object-store.h > index 1be57abaf1..371629c1e1 100644 > --- a/object-store.h > +++ b/object-store.h > @@ -434,8 +434,6 @@ struct object_info { > #define OBJECT_INFO_ALLOW_UNKNOWN_TYPE 2 > /* Do not retry packed storage after checking packed and loose storage */ > #define OBJECT_INFO_QUICK 8 > -/* Do not check loose object */ > -#define OBJECT_INFO_IGNORE_LOOSE 16 > /* > * Do not attempt to fetch the object if missing (even if fetch_is_missing is > * nonzero). This would make Jonathan's change a lot transparent and intuitive if it is based on it, I would think. Thanks for digging.
Junio C Hamano <gitster@pobox.com> writes: > > It looks like the only user went away in 97b2fa08b6 (fetch-pack: drop > > custom loose object cache, 2018-11-12). > > Nice, very nice. > > > So I think we just want to drop it: [snip] > This would make Jonathan's change a lot transparent and intuitive if > it is based on it, I would think. > > Thanks for digging. Ah, thanks for finding this. I'll make this change.
diff --git a/object-file.c b/object-file.c index 26290554bb..596dd049fd 100644 --- a/object-file.c +++ b/object-file.c @@ -1575,18 +1575,17 @@ static int do_oid_object_info_extended(struct repository *r, if (find_pack_entry(r, real, &e)) break; - if (flags & OBJECT_INFO_IGNORE_LOOSE) - return -1; - - /* Most likely it's a loose object. */ - if (!loose_object_info(r, real, oi, flags)) - return 0; - - /* Not a loose object; someone else may have just packed it. */ - if (!(flags & OBJECT_INFO_QUICK)) { - reprepare_packed_git(r); - if (find_pack_entry(r, real, &e)) - break; + if (!(flags & OBJECT_INFO_IGNORE_LOOSE)) { + /* Most likely it's a loose object. */ + if (!loose_object_info(r, real, oi, flags)) + return 0; + + /* Not a loose object; someone else may have just packed it. */ + if (!(flags & OBJECT_INFO_QUICK)) { + reprepare_packed_git(r); + if (find_pack_entry(r, real, &e)) + break; + } } /*
Instead, also search the submodule object stores and promisor remotes. This also centralizes what happens when the object is not found (the "return -1"), which is useful for a subsequent patch. Signed-off-by: Jonathan Tan <jonathantanmy@google.com> --- object-file.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-)