[PATCH v2 0/8] packfiles: track pack lists via the packfile store
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Oct 30, 2025, 10:38 UTC
- Message-ID
- <20251030-pks-packfiles-store-drop-list-v2-0-84654f080cc0@pks.im>
- In-Reply-To
- <20251028-pks-packfiles-store-drop-list-v1-0-1a3b82030a7a@pks.im>
Hi,
while the recently-introduced packfile store tracks the head of the pack lists, the actual lists themselves are still stored in a globally linked list via the `struct packed_git::next` pointer. This makes it quite hard to split up that list into per-object-source lists, as the assumption is embedded in many places that one packfile will identify all the others.
This patch series thus moves the ownership of the lists into the packfile store. This prepares us for a subsequent change where we can push the packfile store one level down, from the object database into the object source. So this is the second-last series before I'm done refactoring the packfile subsystem.
Note: I'd like to have some extra careful eyes on the last patch. This patch merges the two packfile lists we currently have (MRU and mtime-sorted). It is not needed to achieve my goal in this series, but there was some discussion around whether we really need both lists. I don't think we do, and in fact I think it causes confusion which of these one should really use.
The default is to use the mtime-sorted list, which I think is the wrong choice in many cases, but that is only by gut feeling. So I'm dropping that list in favor of the MRU list, but there is one gotcha here: when iterating through packfiles and then reading their respective objects, we end up in an infinite loop because we end up moving the respective packfile to the front of the list again. I'm fixing that with a new field that skips the MRU update, but I'm not quite sure wheter I think that this is too fragile or not.
The series is built on top of 419c72cb8a (Sync with Git 2.51.2, 2025-10-26) with ps/remove-packfile-store-get-packs at ecad863c12 (packfile: rename `packfile_store_get_all_packs()`, 2025-10-09) merged into it.
Changes in v2:
- A couple of commit message typo fixes.
- Avoid opening the pack index in `repo_approximate_object_count()` in
case we don't want to access the packfile in the first place.
- Further simplifications for `has_sha1_pack_kept_or_nonlocal()`.
Also, fix how we skip over the last-found pack.
- Completely reword the motivation why we unconditionally start to add
packfiles to the MRU list.
- Link to v1: https://lore.kernel.org/r/20251028-pks-packfiles-store-drop-list-v1-0-1a3b82030a7a@pks.imThanks!
Patrick
---
Patrick Steinhardt (8):
packfile: use a `strmap` to store packs by name
packfile: move the MRU list into the packfile store
http: refactor subsystem to use `packfile_list`s
packfile: fix approximation of object counts
builtin/pack-objects: simplify logic to find kept or nonlocal objects
packfile: move list of packs into the packfile store
packfile: always add packfiles to MRU when adding a pack
packfile: track packs via the MRU list exclusivelybuiltin/fast-import.c | 4 +- builtin/pack-objects.c | 37 ++++---- http-push.c | 6 +- http-walker.c | 26 ++---- http.c | 21 ++--- http.h | 5 +- midx.c | 2 - packfile.c | 224 +++++++++++++++++++++++++++++-------------------- packfile.h | 70 ++++++++++------ 9 files changed, 223 insertions(+), 172 deletions(-)
Range-diff versus v1:
1: 49bc9f8c9aa = 1: 56660c77d40 packfile: use a `strmap` to store packs by name
2: 4cea16b704e ! 2: d2e003b44ca packfile: move the MRU list into the packfile store
@@ Commit message
object. Consequently, we need to break up the global lists of packfiles
into per-object-source lists.
- A first step towards this goal is to move those lists ouf of `struct
+ A first step towards this goal is to move those lists out of `struct
packed_git` and into the packfile store. While the packfile store is
currently sitting on the `struct object_database` level, the intent is
to push it down one level into the `struct odb_source` in a subsequent
3: 140fc5add46 = 3: 9523423446d http: refactor subsystem to use `packfile_list`s
4: 3a0a29e80de ! 4: 3be216ddfb5 packfile: fix approximation of object counts
@@ packfile.c: unsigned long repo_approximate_object_count(struct repository *r)
- for (p = r->objects->packfiles->packs; p; p = p->next) {
- if (open_pack_index(p))
+ repo_for_each_pack(r, p) {
-+ if (open_pack_index(p) || p->multi_pack_index)
++ if (p->multi_pack_index || open_pack_index(p))
continue;
count += p->num_objects;
}
5: 324d3d29234 ! 5: 867c1d5315a builtin/pack-objects: simplify logic to find kept or nonlocal objects
@@ Commit message
check whether the pack contains the object ID, and to skip the cached
pack in the loop so that we don't search it twice.
+ Furthermore, stop using the `(void *)1` sentinel value and instead use a
+ simple `NULL` pointer to indicate that we don't have a last-found pack
+ yet.
+
This refactoring significantly simplifies the logic and makes it much
easier to follow.
@@ builtin/pack-objects.c: static void add_unreachable_loose_objects(struct rev_inf
static int has_sha1_pack_kept_or_nonlocal(const struct object_id *oid)
{
- struct packfile_store *packs = the_repository->objects->packfiles;
- static struct packed_git *last_found = (void *)1;
+- static struct packed_git *last_found = (void *)1;
++ static struct packed_git *last_found = NULL;
struct packed_git *p;
- p = (last_found != (void *)1) ? last_found :
- packfile_store_get_packs(packs);
-+ if (last_found != (void *)1 && find_pack_entry_one(oid, last_found))
++ if (last_found && find_pack_entry_one(oid, last_found))
+ return 1;
- while (p) {
@@ builtin/pack-objects.c: static void add_unreachable_loose_objects(struct rev_inf
- p->pack_keep_in_core) &&
- find_pack_entry_one(oid, p)) {
+ repo_for_each_pack(the_repository, p) {
++ /*
++ * We have already checked `last_found`, so there is no need to
++ * re-check here.
++ */
++ if (p == last_found)
++ continue;
++
+ if ((!p->pack_local || p->pack_keep || p->pack_keep_in_core) &&
+ find_pack_entry_one(oid, p)) {
last_found = p;
@@ builtin/pack-objects.c: static void add_unreachable_loose_objects(struct rev_inf
- p = p->next;
- if (p == last_found)
- p = p->next;
-+
-+ /*
-+ * We have already checked `last_found`, so there is no need to
-+ * re-check here.
-+ */
-+ if (p == last_found && last_found != (void *)1)
-+ continue;
}
+
return 0;
6: 92c7d5ab273 = 6: 21dd33b22ef packfile: move list of packs into the packfile store
7: df86cc9f650 ! 7: 1bf0880cce8 packfile: always add packfiles to MRU when adding a pack
@@ Metadata
## Commit message ##
packfile: always add packfiles to MRU when adding a pack
- When adding a packfile to it store we add it both to the list and map of
- packfiles, but we don't append it to the most-recently-used list of
- packs. We do know to add the packfile to the MRU list as soon as we
- access any of its objects, but in between we're being inconistent. It
- doesn't help that there are some subsystems that _do_ add the packfile
- to the MRU after having added it, which only adds to the confusion.
+ When preparing the packfile store we know to also prepare the MRU list
+ of packfiles with all packs that are currently loaded in the store via
+ `packfile_store_prepare_mru()`. So we know that the list of packs in the
+ MRU list should match the list of packs in the non-MRU list.
+
+ But there are some direct or indirect callsites that add a packfile to
+ the store via `packfile_store_add_pack()` without adding the pack to the
+ MRU. And while functions that access the MRU (e.g. `find_pack_entry()`)
+ know to call `packfile_store_prepare()`, which knows to prepare the MRU
+ via `packfile_store_prepare_mru()`, that operation will be turned into a
+ no-op because the packfile store is already prepared. So this will not
+ cause us to add the packfile to the MRU, and consequently we won't be
+ able to find the packfile in our MRU list.
+
+ There are only a handful of callers outside of "packfile.c" that add a
+ packfile to the store:
+
+ - "builtin/fast-import.c" adds multiple packs of imported objects, but
+ it knows to look up objects via `packfile_store_get_packs()`. This
+ function does not use the MRU, so we're good.
+
+ - "builtin/index-pack.c" adds the indexed pack to the store in case it
+ needs to perform consistency checks on its objects.
+
+ - "http.c" adds the fetched pack to the store so that we can access
+ its objects.
+
+ In all of these cases we actually want to access the contained objects.
+ And luckily, reading these objects works as expected:
+
+ 1. We eventually end up in `do_oid_object_info_extended()`.
+
+ 2. Calling `find_pack_entry()` fails because the MRU list doesn't
+ contain the newly added packfile.
+
+ 3. The callers don't pass `OBJECT_INFO_QUICK`, so we end up
+ repreparing the object database. This will also cause us to
+ reprepare the MRU list.
+
+ 4. We now retry reading the object via `find_pack_entry()`, and now we
+ succeed because the MRU list got populated.
+
+ This logic feels quite fragile: we intentionally add the packfile to the
+ store, but we then ultimately rely on repreparing the entire store only
+ to make the packfile accessible. While we do the correct thing in
+ `do_oid_object_info_extended()`, other sites that access the MRU may not
+ know to reprepare.
+
+ But besides being fragile it's also a waste of resources: repreparing
+ the object database requires us to re-read the alternates file and
+ discard any caches.
Refactor the code so that we unconditionally add packfiles to the MRU
- when adding them to a packfile store.
+ when adding them to a packfile store. This makes the logic less fragile
+ and ensures that we don't have to reprepare the store to make the pack
+ accessible.
Note that this does not allow us to drop `packfile_store_prepare_mru()`
just yet: while the MRU list is already populated with all packs now,
8: cc9d35a4b09 = 8: 64571e61fac packfile: track packs via the MRU list exclusively--- base-commit: cad6ef1d7514e7450c04c2fe624a55b28d99ac88 change-id: 20251010-pks-packfiles-store-drop-list-64ea0a4c9a3b