From: Toon Claes Date: Wed, 07 Jan 2026 13:11:17 GMT Subject: Re: [PATCH v2 05/10] packfile: move packfile store into object source Message-ID: <87bjj5pnyi.fsf@iotcl.com> In-Reply-To: <20251218-b4-pks-pack-store-via-source-v2-5-62849007ce21@pks.im> Patrick Steinhardt writes: > The packfile store is a member of `struct object_database`, which means > that we have a single store per database. This doesn't really make much > sense though: each source connected to the database has its own set of > packfiles, so there is a conceptual mismatch here. This hasn't really > caused much of a problem in the past, but with the advent of pluggable > object databases this is becoming more of a problem because some of the > sources may not even use packfiles in the first place. > > Move the packfile store down by one level from the object database into > the object database source. This ensures that each source now has its > own packfile store, and we can eventually start to abstract it away > entirely so that the caller doesn't even know what kind of store it > uses. > > Note that we only need to adjust a relatively small number of callers, > way less than one might expect. This is because most callers are using > `repo_for_each_pack()`, which handles enumeration of all packfiles that > exist in the repository. So for now, none of these callers need to be > adapted. The remaining callers that iterate through the packfiles > directly and that need adjustment are those that are a bit more tangled > with packfiles. These will be adjusted over time. > > Note that this patch only moves the packfile store, and there is still a > bunch of functions that seemingly operate on a packfile store but that > end up iterating over all sources. These will be adjusted in subsequent > commits. > > Signed-off-by: Patrick Steinhardt > --- > builtin/fast-import.c | 37 ++++++++------ > builtin/grep.c | 6 ++- > builtin/index-pack.c | 2 +- > builtin/pack-objects.c | 96 +++++++++++++++++++------------------ > http.c | 2 +- > midx.c | 5 +- > odb.c | 36 +++++++------- > odb.h | 6 +-- > odb/streaming.c | 9 ++-- > packfile.c | 127 +++++++++++++++++++++++++++++++------------------ > packfile.h | 62 ++++++++++++++++++++---- > 11 files changed, 243 insertions(+), 145 deletions(-) > > diff --git a/builtin/fast-import.c b/builtin/fast-import.c > index 7849005ccb..b8a7757cfd 100644 > --- a/builtin/fast-import.c > +++ b/builtin/fast-import.c > @@ -900,7 +900,7 @@ static void end_packfile(void) > idx_name = keep_pack(create_index()); > > /* Register the packfile with core git's machinery. */ > - new_p = packfile_store_load_pack(pack_data->repo->objects->packfiles, > + new_p = packfile_store_load_pack(pack_data->repo->objects->sources->packfiles, > idx_name, 1); > if (!new_p) > die(_("core Git rejected index %s"), idx_name); > @@ -955,7 +955,7 @@ static int store_object( > struct object_id *oidout, > uintmax_t mark) > { > - struct packfile_store *packs = the_repository->objects->packfiles; > + struct odb_source *source; > void *out, *delta; > struct object_entry *e; > unsigned char hdr[96]; > @@ -979,7 +979,11 @@ static int store_object( > if (e->idx.offset) { > duplicate_count_by_type[type]++; > return 1; > - } else if (packfile_list_find_oid(packfile_store_get_packs(packs), &oid)) { > + } > + > + for (source = the_repository->objects->sources; source; source = source->next) { > + if (!packfile_list_find_oid(packfile_store_get_packs(source->packfiles), &oid)) > + continue; > e->type = type; > e->pack_id = MAX_PACK_ID; > e->idx.offset = 1; /* just not zero! */ > @@ -1096,10 +1100,10 @@ static void truncate_pack(struct hashfile_checkpoint *checkpoint) > > static void stream_blob(uintmax_t len, struct object_id *oidout, uintmax_t mark) > { > - struct packfile_store *packs = the_repository->objects->packfiles; > size_t in_sz = 64 * 1024, out_sz = 64 * 1024; > unsigned char *in_buf = xmalloc(in_sz); > unsigned char *out_buf = xmalloc(out_sz); > + struct odb_source *source; > struct object_entry *e; > struct object_id oid; > unsigned long hdrlen; > @@ -1179,24 +1183,29 @@ static void stream_blob(uintmax_t len, struct object_id *oidout, uintmax_t mark) > if (e->idx.offset) { > duplicate_count_by_type[OBJ_BLOB]++; > truncate_pack(&checkpoint); > + goto out; > + } > > - } else if (packfile_list_find_oid(packfile_store_get_packs(packs), &oid)) { > + for (source = the_repository->objects->sources; source; source = source->next) { > + if (!packfile_list_find_oid(packfile_store_get_packs(source->packfiles), &oid)) > + continue; > e->type = OBJ_BLOB; > e->pack_id = MAX_PACK_ID; > e->idx.offset = 1; /* just not zero! */ > duplicate_count_by_type[OBJ_BLOB]++; > truncate_pack(&checkpoint); > - > - } else { > - e->depth = 0; > - e->type = OBJ_BLOB; > - e->pack_id = pack_id; > - e->idx.offset = offset; > - e->idx.crc32 = crc32_end(pack_file); > - object_count++; > - object_count_by_type[OBJ_BLOB]++; > + goto out; > } > > + e->depth = 0; > + e->type = OBJ_BLOB; > + e->pack_id = pack_id; > + e->idx.offset = offset; > + e->idx.crc32 = crc32_end(pack_file); > + object_count++; > + object_count_by_type[OBJ_BLOB]++; > + > +out: > free(in_buf); > free(out_buf); > } > diff --git a/builtin/grep.c b/builtin/grep.c > index 53cccf2d25..4855b871dd 100644 > --- a/builtin/grep.c > +++ b/builtin/grep.c > @@ -1213,8 +1213,12 @@ int cmd_grep(int argc, > */ > if (recurse_submodules) > repo_read_gitmodules(the_repository, 1); > + /* > + * Note: `packfile_store_prepare()` prepares stores from all > + * sources. This will be fixed in a subsequent commit. I assume you mean the opposite. But no problem since this will be addressed in a later commit. -- Cheers, Toon