From: Karthik Nayak Date: Thu, 30 Oct 2025 12:19:28 GMT Subject: Re: [PATCH 09/13] object-file: read objects via the loose object source Message-ID: In-Reply-To: <20251024-b4-pks-odb-loose-backend-v1-9-1a4202273c38@pks.im> Patrick Steinhardt writes: > When reading an object via `loose_object_info()` or `map_loose_object()` > we hand in the whole repository. We then iterate through each of the > object sources to figure out whether that source has the object in > question. > Okay so earlier these two functions would call `open_loose_object()` and that function would iterate over all object sources like you mentioned. > This logic is reversing responsibility though: a specific backend should > only care about one specific source, where the object sources themselves > are then managed by the object database. > > Refactor the code accordingly by passing an object source to both of > these functions instead. The different sources are then handled by > either `do_oid_object_info_extended()`, which sits on the object > database level, and by `open_istream_loose()`. The latter function > arguably is still at the wrong level, but this will be cleaned up at a > later point in time. And we move this check to a layer above, which can check if any of the sources contain the object. > > Signed-off-by: Patrick Steinhardt > --- > object-file.c | 68 ++++++++++++++++++++++------------------------------------- > object-file.h | 15 +++++++------ > odb.c | 9 ++++++-- > streaming.c | 11 +++++++++- > 4 files changed, 50 insertions(+), 53 deletions(-) > > diff --git a/object-file.c b/object-file.c > index 14daa2bdd90..d9724e3105f 100644 > --- a/object-file.c > +++ b/object-file.c > @@ -167,25 +167,22 @@ int stream_object_signature(struct repository *r, const struct object_id *oid) > } > > /* > - * Find "oid" as a loose object in the local repository or in an alternate. > + * Find "oid" as a loose object in given source. > * Returns 0 on success, negative on failure. > * > * The "path" out-parameter will give the path of the object we found (if any). > * Note that it may point to static storage and is only valid until another > * call to stat_loose_object(). > */ > -static int stat_loose_object(struct repository *r, const struct object_id *oid, > +static int stat_loose_object(struct odb_loose_source *source, > + const struct object_id *oid, > struct stat *st, const char **path) > { > - struct odb_source *source; > static struct strbuf buf = STRBUF_INIT; > > - odb_prepare_alternates(r->objects); > - for (source = r->objects->sources; source; source = source->next) { > - *path = odb_loose_path(source, &buf, oid); > - if (!lstat(*path, st)) > - return 0; > - } > + *path = odb_loose_path(source->source, &buf, oid); > + if (!lstat(*path, st)) > + return 0; > > return -1; > } > @@ -194,39 +191,24 @@ static int stat_loose_object(struct repository *r, const struct object_id *oid, > * Like stat_loose_object(), but actually open the object and return the > * descriptor. See the caveats on the "path" parameter above. > */ > -static int open_loose_object(struct repository *r, > +static int open_loose_object(struct odb_loose_source *source, > const struct object_id *oid, const char **path) > { > - int fd; > - struct odb_source *source; > - int most_interesting_errno = ENOENT; > static struct strbuf buf = STRBUF_INIT; > + int fd; > > - odb_prepare_alternates(r->objects); > - for (source = r->objects->sources; source; source = source->next) { > - *path = odb_loose_path(source, &buf, oid); > - fd = git_open(*path); > - if (fd >= 0) > - return fd; > + *path = odb_loose_path(source->source, &buf, oid); > + fd = git_open(*path); > + if (fd >= 0) > + return fd; > > - if (most_interesting_errno == ENOENT) > - most_interesting_errno = errno; > - } > - errno = most_interesting_errno; > return -1; > } > > -static int quick_has_loose(struct repository *r, > +static int quick_has_loose(struct odb_loose_source *source, > const struct object_id *oid) > { > - struct odb_source *source; > - > - odb_prepare_alternates(r->objects); > - for (source = r->objects->sources; source; source = source->next) { > - if (oidtree_contains(odb_loose_source_cache(source, oid), oid)) > - return 1; > - } > - return 0; > + return !!oidtree_contains(odb_loose_source_cache(source->source, oid), oid); > } > These above functions are much simpler now that they have to only worry about the source level and not the repository level. [snip] > diff --git a/odb.c b/odb.c > index 96059456f20..5dc1e2c7eb0 100644 > --- a/odb.c > +++ b/odb.c > @@ -697,13 +697,18 @@ static int do_oid_object_info_extended(struct object_database *odb, > return 0; > } > > + odb_prepare_alternates(odb); > + > while (1) { > + struct odb_source *source; > + > if (find_pack_entry(odb->repo, real, &e)) > break; > > /* Most likely it's a loose object. */ > - if (!loose_object_info(odb->repo, real, oi, flags)) > - return 0; > + for (source = odb->sources; source; source = source->next) > + if (!odb_loose_source_read_object_info(source, real, oi, flags)) > + return 0; > The iteration over all sources is now moved here, makes sense. > /* Not a loose object; someone else may have just packed it. */ > if (!(flags & OBJECT_INFO_QUICK)) { > diff --git a/streaming.c b/streaming.c > index 4b13827668e..8e554abd084 100644 > --- a/streaming.c > +++ b/streaming.c > @@ -230,12 +230,21 @@ static int open_istream_loose(struct git_istream *st, struct repository *r, > enum object_type *type) > { > struct object_info oi = OBJECT_INFO_INIT; > + struct odb_source *source; > + > oi.sizep = &st->size; > oi.typep = type; > > - st->u.loose.mapped = map_loose_object(r, oid, &st->u.loose.mapsize); > + odb_prepare_alternates(r->objects); > + for (source = r->objects->sources; source; source = source->next) { > + st->u.loose.mapped = odb_loose_source_map_object(source, oid, > + &st->u.loose.mapsize); > + if (st->u.loose.mapped) > + break; > + } > if (!st->u.loose.mapped) > return -1; > + > switch (unpack_loose_header(&st->z, st->u.loose.mapped, > st->u.loose.mapsize, st->u.loose.hdr, > sizeof(st->u.loose.hdr))) { > > -- > 2.51.1.930.gacf6e81ea2.dirty Same here. The patch looks good.