{"thread":{"id":"64398","subject":"[PATCH 0/8] packfiles: track pack lists via the packfile store","startedAt":"2025-10-28T11:08:42Z","lastAt":"2025-10-30T10:39:10Z","messageCount":34,"participants":["Patrick Steinhardt","Toon Claes","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":8},"messages":[{"id":"529779","messageId":"20251028-pks-packfiles-store-drop-list-v1-0-1a3b82030a7a@pks.im","threadId":"64398","inReplyTo":null,"subject":"[PATCH 0/8] packfiles: track pack lists via the packfile store","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-10-28T11:08:30Z","receivedAt":"2025-10-28T11:08:42Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nwhile the recently-introduced packfile store tracks the head of the pack\nlists, the actual lists themselves are still stored in a globally linked\nlist via the `struct packed_git::next` pointer. This makes it quite hard\nto split up that list into per-object-source lists, as the assumption is\nembedded in many places that one packfile will identify all the others.\n\nThis patch series thus moves the ownership of the lists into the\npackfile store. This prepares us for a subsequent change where we can\npush the packfile store one level down, from the object database into\nthe object source. So this is the second-last series before I'm done\nrefactoring the packfile subsystem.\n\nNote: I'd like to have some extra careful eyes on the last patch. This\npatch merges the two packfile lists we currently have (MRU and\nmtime-sorted). It is not needed to achieve my goal in this series, but\nthere was some discussion around whether we really need both lists. I\ndon't think we do, and in fact I think it causes confusion which of\nthese one should really use.\n\nThe default is to use the mtime-sorted list, which I think is the wrong\nchoice in many cases, but that is only by gut feeling. So I'm dropping\nthat list in favor of the MRU list, but there is one gotcha here: when\niterating through packfiles and then reading their respective objects,\nwe end up in an infinite loop because we end up moving the respective\npackfile to the front of the list again. I'm fixing that with a new\nfield that skips the MRU update, but I'm not quite sure wheter I think\nthat this is too fragile or not.\n\nThe series is built on top of 419c72cb8a (Sync with Git 2.51.2,\n2025-10-26) with ps/remove-packfile-store-get-packs at ecad863c12\n(packfile: rename `packfile_store_get_all_packs()`, 2025-10-09) merged\ninto it.\n\nThanks!\n\nPatrick\n\n---\nPatrick Steinhardt (8):\n packfile: use a `strmap` to store packs by name\n packfile: move the MRU list into the packfile store\n http: refactor subsystem to use `packfile_list`s\n packfile: fix approximation of object counts\n builtin/pack-objects: simplify logic to find kept or nonlocal objects\n packfile: move list of packs into the packfile store\n packfile: always add packfiles to MRU when adding a pack\n packfile: track packs via the MRU list exclusively\n\n builtin/fast-import.c | 4 +-\n builtin/pack-objects.c | 35 ++++----\n http-push.c | 6 +-\n http-walker.c | 26 ++----\n http.c | 21 ++---\n http.h | 5 +-\n midx.c | 2 -\n packfile.c | 224 +++++++++++++++++++++++++++++--------------------\n packfile.h | 70 ++++++++++------\n 9 files changed, 222 insertions(+), 171 deletions(-)\n\n\n---\nbase-commit: cad6ef1d7514e7450c04c2fe624a55b28d99ac88\nchange-id: 20251010-pks-packfiles-store-drop-list-64ea0a4c9a3b\n\n"},{"id":"529780","messageId":"20251028-pks-packfiles-store-drop-list-v1-1-1a3b82030a7a@pks.im","threadId":"64398","inReplyTo":"20251028-pks-packfiles-store-drop-list-v1-0-1a3b82030a7a@pks.im","subject":"[PATCH 1/8] packfile: use a `strmap` to store packs by name","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-10-28T11:08:31Z","receivedAt":"2025-10-28T11:08:44Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"To allow fast lookups of a packfile by name we use a hashmap that has\nthe packfile name as key and the pack itself as value. But while this is\nthe perfect use case for a `strmap`, we instead use `struct hashmap` and\nstore the hashmap entry in the packfile itself.\n\nSimplify the code by using a `strmap` instead.\n\nSigned-off-by: Patrick Steinhardt \n---\n packfile.c | 24 ++++--------------------\n packfile.h | 4 ++--\n 2 files changed, 6 insertions(+), 22 deletions(-)\n\ndiff --git a/packfile.c b/packfile.c\nindex 1ae2b2fe1ed..04649e52920 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -788,8 +788,7 @@ void packfile_store_add_pack(struct packfile_store *store,\n \tpack->next = store->packs;\n \tstore->packs = pack;\n \n-\thashmap_entry_init(&pack->packmap_ent, strhash(pack->pack_name));\n-\thashmap_add(&store->map, &pack->packmap_ent);\n+\tstrmap_put(&store->packs_by_path, pack->pack_name, pack);\n }\n \n struct packed_git *packfile_store_load_pack(struct packfile_store *store,\n@@ -806,8 +805,7 @@ struct packed_git *packfile_store_load_pack(struct packfile_store *store,\n \tstrbuf_strip_suffix(&key, \".idx\");\n \tstrbuf_addstr(&key, \".pack\");\n \n-\tp = hashmap_get_entry_from_hash(&store->map, strhash(key.buf), key.buf,\n-\t\t\t\t\tstruct packed_git, packmap_ent);\n+\tp = strmap_get(&store->packs_by_path, key.buf);\n \tif (!p) {\n \t\tp = add_packed_git(store->odb->repo, idx_path,\n \t\t\t\t strlen(idx_path), local);\n@@ -2311,27 +2309,13 @@ int parse_pack_header_option(const char *in, unsigned char *out, unsigned int *l\n \treturn 0;\n }\n \n-static int pack_map_entry_cmp(const void *cmp_data UNUSED,\n-\t\t\t const struct hashmap_entry *entry,\n-\t\t\t const struct hashmap_entry *entry2,\n-\t\t\t const void *keydata)\n-{\n-\tconst char *key = keydata;\n-\tconst struct packed_git *pg1, *pg2;\n-\n-\tpg1 = container_of(entry, const struct packed_git, packmap_ent);\n-\tpg2 = container_of(entry2, const struct packed_git, packmap_ent);\n-\n-\treturn strcmp(pg1->pack_name, key ? key : pg2->pack_name);\n-}\n-\n struct packfile_store *packfile_store_new(struct object_database *odb)\n {\n \tstruct packfile_store *store;\n \tCALLOC_ARRAY(store, 1);\n \tstore->odb = odb;\n \tINIT_LIST_HEAD(&store->mru);\n-\thashmap_init(&store->map, pack_map_entry_cmp, NULL, 0);\n+\tstrmap_init(&store->packs_by_path);\n \treturn store;\n }\n \n@@ -2341,7 +2325,7 @@ void packfile_store_free(struct packfile_store *store)\n \t\tnext = p->next;\n \t\tfree(p);\n \t}\n-\thashmap_clear(&store->map);\n+\tstrmap_clear(&store->packs_by_path, 0);\n \tfree(store);\n }\n \ndiff --git a/packfile.h b/packfile.h\nindex c9d0b93446b..9da7f14317b 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -5,12 +5,12 @@\n #include \"object.h\"\n #include \"odb.h\"\n #include \"oidset.h\"\n+#include \"strmap.h\"\n \n /* in odb.h */\n struct object_info;\n \n struct packed_git {\n-\tstruct hashmap_entry packmap_ent;\n \tstruct packed_git *next;\n \tstruct list_head mru;\n \tstruct pack_window *windows;\n@@ -85,7 +85,7 @@ struct packfile_store {\n \t * A map of packfile names to packed_git structs for tracking which\n \t * packs have been loaded already.\n \t */\n-\tstruct hashmap map;\n+\tstruct strmap packs_by_path;\n \n \t/*\n \t * Whether packfiles have already been populated with this store's\n\n-- \n2.51.2.997.g839fc31de9.dirty\n\n"},{"id":"529781","messageId":"20251028-pks-packfiles-store-drop-list-v1-2-1a3b82030a7a@pks.im","threadId":"64398","inReplyTo":"20251028-pks-packfiles-store-drop-list-v1-0-1a3b82030a7a@pks.im","subject":"[PATCH 2/8] packfile: move the MRU list into the packfile store","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-10-28T11:08:32Z","receivedAt":"2025-10-28T11:08:46Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Packfiles have two lists associated to them:\n\n - A list that keeps track of packfiles in the order that they were\n added to a packfile store.\n\n - A list that keeps track of packfiles in most-recently-used order so\n that packfiles that are more likely to contain a specific object are\n ordered towards the front.\n\nBoth of these lists are hosted by `struct packed_git` itself, So to\nidentify all packfiles in a repository you simply need to grab the first\npackfile and then iterate the `->next` pointers or the MRU list. This\npattern has the problem that all packfiles are part of the same list,\nregardless of whether or not they belong to the same object source.\n\nWith the upcoming pluggable object database effort this needs to change:\npackfiles should be contained by a single object source, and reading an\nobject from any such packfile should use that source to look up the\nobject. Consequently, we need to break up the global lists of packfiles\ninto per-object-source lists.\n\nA first step towards this goal is to move those lists ouf of `struct\npacked_git` and into the packfile store. While the packfile store is\ncurrently sitting on the `struct object_database` level, the intent is\nto push it down one level into the `struct odb_source` in a subsequent\npatch series.\n\nIntroduce a new `struct packfile_list` that is used to manage lists of\npackfiles and use it to store the list of most-recently-used packfiles\nin `struct packfile_store`. For now, the new list type is only used in a\nsingle spot, but we'll expand its usage in subsequent patches.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/pack-objects.c | 9 +++--\n midx.c | 2 +-\n packfile.c | 92 +++++++++++++++++++++++++++++++++++++++++++++-----\n packfile.h | 19 +++++++++--\n 4 files changed, 104 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex b5454e5df13..5348aebbe9f 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -1706,8 +1706,8 @@ static int want_object_in_pack_mtime(const struct object_id *oid,\n \t\t\t\t uint32_t found_mtime)\n {\n \tint want;\n+\tstruct packfile_list_entry *e;\n \tstruct odb_source *source;\n-\tstruct list_head *pos;\n \n \tif (!exclude && local) {\n \t\t/*\n@@ -1748,12 +1748,11 @@ static int want_object_in_pack_mtime(const struct object_id *oid,\n \t\t}\n \t}\n \n-\tlist_for_each(pos, packfile_store_get_packs_mru(the_repository->objects->packfiles)) {\n-\t\tstruct packed_git *p = list_entry(pos, struct packed_git, mru);\n+\tfor (e = the_repository->objects->packfiles->mru.head; e; e = e->next) {\n+\t\tstruct packed_git *p = e->pack;\n \t\twant = want_object_in_pack_one(p, oid, exclude, found_pack, found_offset, found_mtime);\n \t\tif (!exclude && want > 0)\n-\t\t\tlist_move(&p->mru,\n-\t\t\t\t packfile_store_get_packs_mru(the_repository->objects->packfiles));\n+\t\t\tpackfile_list_prepend(&the_repository->objects->packfiles->mru, p);\n \t\tif (want != -1)\n \t\t\treturn want;\n \t}\ndiff --git a/midx.c b/midx.c\nindex 1d6269f957e..8022be9a45e 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -463,7 +463,7 @@ int prepare_midx_pack(struct multi_pack_index *m,\n \tp = packfile_store_load_pack(r->objects->packfiles,\n \t\t\t\t pack_name.buf, m->source->local);\n \tif (p)\n-\t\tlist_add_tail(&p->mru, &r->objects->packfiles->mru);\n+\t\tpackfile_list_append(&m->source->odb->packfiles->mru, p);\n \tstrbuf_release(&pack_name);\n \n \tif (!p) {\ndiff --git a/packfile.c b/packfile.c\nindex 04649e52920..4d2d3b674f3 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -47,6 +47,80 @@ static size_t pack_mapped;\n #define SZ_FMT PRIuMAX\n static inline uintmax_t sz_fmt(size_t s) { return s; }\n \n+void packfile_list_clear(struct packfile_list *list)\n+{\n+\tstruct packfile_list_entry *e, *next;\n+\n+\tfor (e = list->head; e; e = next) {\n+\t\tnext = e->next;\n+\t\tfree(e);\n+\t}\n+\n+\tlist->head = list->tail = NULL;\n+}\n+\n+static struct packfile_list_entry *packfile_list_remove_internal(struct packfile_list *list,\n+\t\t\t\t\t\t\t\t struct packed_git *pack)\n+{\n+\tstruct packfile_list_entry *e, *prev;\n+\n+\tfor (e = list->head, prev = NULL; e; prev = e, e = e->next) {\n+\t\tif (e->pack != pack)\n+\t\t\tcontinue;\n+\n+\t\tif (prev)\n+\t\t\tprev->next = e->next;\n+\t\tif (list->head == e)\n+\t\t\tlist->head = e->next;\n+\t\tif (list->tail == e)\n+\t\t\tlist->tail = prev;\n+\n+\t\treturn e;\n+\t}\n+\n+\treturn NULL;\n+}\n+\n+void packfile_list_remove(struct packfile_list *list, struct packed_git *pack)\n+{\n+\tfree(packfile_list_remove_internal(list, pack));\n+}\n+\n+void packfile_list_prepend(struct packfile_list *list, struct packed_git *pack)\n+{\n+\tstruct packfile_list_entry *entry;\n+\n+\tentry = packfile_list_remove_internal(list, pack);\n+\tif (!entry) {\n+\t\tentry = xmalloc(sizeof(*entry));\n+\t\tentry->pack = pack;\n+\t}\n+\tentry->next = list->head;\n+\n+\tlist->head = entry;\n+\tif (!list->tail)\n+\t\tlist->tail = entry;\n+}\n+\n+void packfile_list_append(struct packfile_list *list, struct packed_git *pack)\n+{\n+\tstruct packfile_list_entry *entry;\n+\n+\tentry = packfile_list_remove_internal(list, pack);\n+\tif (!entry) {\n+\t\tentry = xmalloc(sizeof(*entry));\n+\t\tentry->pack = pack;\n+\t}\n+\tentry->next = NULL;\n+\n+\tif (list->tail) {\n+\t\tlist->tail->next = entry;\n+\t\tlist->tail = entry;\n+\t} else {\n+\t\tlist->head = list->tail = entry;\n+\t}\n+}\n+\n void pack_report(struct repository *repo)\n {\n \tfprintf(stderr,\n@@ -995,10 +1069,10 @@ static void packfile_store_prepare_mru(struct packfile_store *store)\n {\n \tstruct packed_git *p;\n \n-\tINIT_LIST_HEAD(&store->mru);\n+\tpackfile_list_clear(&store->mru);\n \n \tfor (p = store->packs; p; p = p->next)\n-\t\tlist_add_tail(&p->mru, &store->mru);\n+\t\tpackfile_list_append(&store->mru, p);\n }\n \n void packfile_store_prepare(struct packfile_store *store)\n@@ -1040,10 +1114,10 @@ struct packed_git *packfile_store_get_packs(struct packfile_store *store)\n \treturn store->packs;\n }\n \n-struct list_head *packfile_store_get_packs_mru(struct packfile_store *store)\n+struct packfile_list_entry *packfile_store_get_packs_mru(struct packfile_store *store)\n {\n \tpackfile_store_prepare(store);\n-\treturn &store->mru;\n+\treturn store->mru.head;\n }\n \n /*\n@@ -2048,7 +2122,7 @@ static int fill_pack_entry(const struct object_id *oid,\n \n int find_pack_entry(struct repository *r, const struct object_id *oid, struct pack_entry *e)\n {\n-\tstruct list_head *pos;\n+\tstruct packfile_list_entry *l;\n \n \tpackfile_store_prepare(r->objects->packfiles);\n \n@@ -2059,10 +2133,11 @@ int find_pack_entry(struct repository *r, const struct object_id *oid, struct pa\n \tif (!r->objects->packfiles->packs)\n \t\treturn 0;\n \n-\tlist_for_each(pos, &r->objects->packfiles->mru) {\n-\t\tstruct packed_git *p = list_entry(pos, struct packed_git, mru);\n+\tfor (l = r->objects->packfiles->mru.head; l; l = l->next) {\n+\t\tstruct packed_git *p = l->pack;\n+\n \t\tif (!p->multi_pack_index && fill_pack_entry(oid, e, p)) {\n-\t\t\tlist_move(&p->mru, &r->objects->packfiles->mru);\n+\t\t\tpackfile_list_prepend(&r->objects->packfiles->mru, p);\n \t\t\treturn 1;\n \t\t}\n \t}\n@@ -2314,7 +2389,6 @@ struct packfile_store *packfile_store_new(struct object_database *odb)\n \tstruct packfile_store *store;\n \tCALLOC_ARRAY(store, 1);\n \tstore->odb = odb;\n-\tINIT_LIST_HEAD(&store->mru);\n \tstrmap_init(&store->packs_by_path);\n \treturn store;\n }\ndiff --git a/packfile.h b/packfile.h\nindex 9da7f14317b..39ed1073e4a 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -12,7 +12,6 @@ struct object_info;\n \n struct packed_git {\n \tstruct packed_git *next;\n-\tstruct list_head mru;\n \tstruct pack_window *windows;\n \toff_t pack_size;\n \tconst void *index_data;\n@@ -52,6 +51,20 @@ struct packed_git {\n \tchar pack_name[FLEX_ARRAY]; /* more */\n };\n \n+struct packfile_list {\n+\tstruct packfile_list_entry *head, *tail;\n+};\n+\n+struct packfile_list_entry {\n+\tstruct packfile_list_entry *next;\n+\tstruct packed_git *pack;\n+};\n+\n+void packfile_list_clear(struct packfile_list *list);\n+void packfile_list_remove(struct packfile_list *list, struct packed_git *pack);\n+void packfile_list_prepend(struct packfile_list *list, struct packed_git *pack);\n+void packfile_list_append(struct packfile_list *list, struct packed_git *pack);\n+\n /*\n * A store that manages packfiles for a given object database.\n */\n@@ -79,7 +92,7 @@ struct packfile_store {\n \t} kept_cache;\n \n \t/* A most-recently-used ordered version of the packs list. */\n-\tstruct list_head mru;\n+\tstruct packfile_list mru;\n \n \t/*\n \t * A map of packfile names to packed_git structs for tracking which\n@@ -153,7 +166,7 @@ struct packed_git *packfile_store_get_packs(struct packfile_store *store);\n /*\n * Get all packs in most-recently-used order.\n */\n-struct list_head *packfile_store_get_packs_mru(struct packfile_store *store);\n+struct packfile_list_entry *packfile_store_get_packs_mru(struct packfile_store *store);\n \n /*\n * Open the packfile and add it to the store if it isn't yet known. Returns\n\n-- \n2.51.2.997.g839fc31de9.dirty\n\n"},{"id":"529782","messageId":"20251028-pks-packfiles-store-drop-list-v1-3-1a3b82030a7a@pks.im","threadId":"64398","inReplyTo":"20251028-pks-packfiles-store-drop-list-v1-0-1a3b82030a7a@pks.im","subject":"[PATCH 3/8] http: refactor subsystem to use `packfile_list`s","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-10-28T11:08:33Z","receivedAt":"2025-10-28T11:08:49Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The dumb HTTP protocol directly fetches packfiles from the remote server\nand temporarily stores them in a list of packfiles. Those packfiles are\nnot yet added to the repository's packfile store until we finalize the\nwhole fetch.\n\nRefactor the code to instead use a `struct packfile_list` to store those\npacks. This prepares us for a subsequent change where the `->next`\npointer of `struct packed_git` will go away.\n\nNote that this refactoring creates some temporary duplication of code,\nas we now have both `packfile_list_find_oid()` and `find_oid_pack()`.\nThe latter function will be removed in a subsequent commit though.\n\nSigned-off-by: Patrick Steinhardt \n---\n http-push.c | 6 +++---\n http-walker.c | 26 +++++++++-----------------\n http.c | 21 ++++++++-------------\n http.h | 5 +++--\n packfile.c | 9 +++++++++\n packfile.h | 8 ++++++++\n 6 files changed, 40 insertions(+), 35 deletions(-)\n\ndiff --git a/http-push.c b/http-push.c\nindex a1c01e3b9b9..d86ce771198 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -104,7 +104,7 @@ struct repo {\n \tint has_info_refs;\n \tint can_update_info_refs;\n \tint has_info_packs;\n-\tstruct packed_git *packs;\n+\tstruct packfile_list packs;\n \tstruct remote_lock *locks;\n };\n \n@@ -311,7 +311,7 @@ static void start_fetch_packed(struct transfer_request *request)\n \tstruct transfer_request *check_request = request_queue_head;\n \tstruct http_pack_request *preq;\n \n-\ttarget = find_oid_pack(&request->obj->oid, repo->packs);\n+\ttarget = packfile_list_find_oid(repo->packs.head, &request->obj->oid);\n \tif (!target) {\n \t\tfprintf(stderr, \"Unable to fetch %s, will not be able to update server info refs\\n\", oid_to_hex(&request->obj->oid));\n \t\trepo->can_update_info_refs = 0;\n@@ -683,7 +683,7 @@ static int add_send_request(struct object *obj, struct remote_lock *lock)\n \t\tget_remote_object_list(obj->oid.hash[0]);\n \tif (obj->flags & (REMOTE | PUSHING))\n \t\treturn 0;\n-\ttarget = find_oid_pack(&obj->oid, repo->packs);\n+\ttarget = packfile_list_find_oid(repo->packs.head, &obj->oid);\n \tif (target) {\n \t\tobj->flags |= REMOTE;\n \t\treturn 0;\ndiff --git a/http-walker.c b/http-walker.c\nindex 0f7ae46d7f1..e886e648664 100644\n--- a/http-walker.c\n+++ b/http-walker.c\n@@ -15,7 +15,7 @@\n struct alt_base {\n \tchar *base;\n \tint got_indices;\n-\tstruct packed_git *packs;\n+\tstruct packfile_list packs;\n \tstruct alt_base *next;\n };\n \n@@ -324,11 +324,8 @@ static void process_alternates_response(void *callback_data)\n \t\t\t\t} else if (is_alternate_allowed(target.buf)) {\n \t\t\t\t\twarning(\"adding alternate object store: %s\",\n \t\t\t\t\t\ttarget.buf);\n-\t\t\t\t\tnewalt = xmalloc(sizeof(*newalt));\n-\t\t\t\t\tnewalt->next = NULL;\n+\t\t\t\t\tCALLOC_ARRAY(newalt, 1);\n \t\t\t\t\tnewalt->base = strbuf_detach(&target, NULL);\n-\t\t\t\t\tnewalt->got_indices = 0;\n-\t\t\t\t\tnewalt->packs = NULL;\n \n \t\t\t\t\twhile (tail->next != NULL)\n \t\t\t\t\t\ttail = tail->next;\n@@ -435,7 +432,7 @@ static int http_fetch_pack(struct walker *walker, struct alt_base *repo,\n \n \tif (fetch_indices(walker, repo))\n \t\treturn -1;\n-\ttarget = find_oid_pack(oid, repo->packs);\n+\ttarget = packfile_list_find_oid(repo->packs.head, oid);\n \tif (!target)\n \t\treturn -1;\n \tclose_pack_index(target);\n@@ -584,17 +581,15 @@ static void cleanup(struct walker *walker)\n \tif (data) {\n \t\talt = data->alt;\n \t\twhile (alt) {\n-\t\t\tstruct packed_git *pack;\n+\t\t\tstruct packfile_list_entry *e;\n \n \t\t\talt_next = alt->next;\n \n-\t\t\tpack = alt->packs;\n-\t\t\twhile (pack) {\n-\t\t\t\tstruct packed_git *pack_next = pack->next;\n-\t\t\t\tclose_pack(pack);\n-\t\t\t\tfree(pack);\n-\t\t\t\tpack = pack_next;\n+\t\t\tfor (e = alt->packs.head; e; e = e->next) {\n+\t\t\t\tclose_pack(e->pack);\n+\t\t\t\tfree(e->pack);\n \t\t\t}\n+\t\t\tpackfile_list_clear(&alt->packs);\n \n \t\t\tfree(alt->base);\n \t\t\tfree(alt);\n@@ -612,14 +607,11 @@ struct walker *get_http_walker(const char *url)\n \tstruct walker_data *data = xmalloc(sizeof(struct walker_data));\n \tstruct walker *walker = xmalloc(sizeof(struct walker));\n \n-\tdata->alt = xmalloc(sizeof(*data->alt));\n+\tCALLOC_ARRAY(data->alt, 1);\n \tdata->alt->base = xstrdup(url);\n \tfor (s = data->alt->base + strlen(data->alt->base) - 1; *s == '/'; --s)\n \t\t*s = 0;\n \n-\tdata->alt->got_indices = 0;\n-\tdata->alt->packs = NULL;\n-\tdata->alt->next = NULL;\n \tdata->got_alternates = -1;\n \n \twalker->corrupt_object_found = 0;\ndiff --git a/http.c b/http.c\nindex 17130823f00..41f850db16d 100644\n--- a/http.c\n+++ b/http.c\n@@ -2413,8 +2413,9 @@ static char *fetch_pack_index(unsigned char *hash, const char *base_url)\n \treturn tmp;\n }\n \n-static int fetch_and_setup_pack_index(struct packed_git **packs_head,\n-\tunsigned char *sha1, const char *base_url)\n+static int fetch_and_setup_pack_index(struct packfile_list *packs,\n+\t\t\t\t unsigned char *sha1,\n+\t\t\t\t const char *base_url)\n {\n \tstruct packed_git *new_pack, *p;\n \tchar *tmp_idx = NULL;\n@@ -2448,12 +2449,11 @@ static int fetch_and_setup_pack_index(struct packed_git **packs_head,\n \tif (ret)\n \t\treturn -1;\n \n-\tnew_pack->next = *packs_head;\n-\t*packs_head = new_pack;\n+\tpackfile_list_prepend(packs, new_pack);\n \treturn 0;\n }\n \n-int http_get_info_packs(const char *base_url, struct packed_git **packs_head)\n+int http_get_info_packs(const char *base_url, struct packfile_list *packs)\n {\n \tstruct http_get_options options = {0};\n \tint ret = 0;\n@@ -2477,7 +2477,7 @@ int http_get_info_packs(const char *base_url, struct packed_git **packs_head)\n \t\t !parse_oid_hex(data, &oid, &data) &&\n \t\t skip_prefix(data, \".pack\", &data) &&\n \t\t (*data == '\\n' || *data == '\\0')) {\n-\t\t\tfetch_and_setup_pack_index(packs_head, oid.hash, base_url);\n+\t\t\tfetch_and_setup_pack_index(packs, oid.hash, base_url);\n \t\t} else {\n \t\t\tdata = strchrnul(data, '\\n');\n \t\t}\n@@ -2541,14 +2541,9 @@ int finish_http_pack_request(struct http_pack_request *preq)\n }\n \n void http_install_packfile(struct packed_git *p,\n-\t\t\t struct packed_git **list_to_remove_from)\n+\t\t\t struct packfile_list *list_to_remove_from)\n {\n-\tstruct packed_git **lst = list_to_remove_from;\n-\n-\twhile (*lst != p)\n-\t\tlst = &((*lst)->next);\n-\t*lst = (*lst)->next;\n-\n+\tpackfile_list_remove(list_to_remove_from, p);\n \tpackfile_store_add_pack(the_repository->objects->packfiles, p);\n }\n \ndiff --git a/http.h b/http.h\nindex 553e16205ce..f9d45934047 100644\n--- a/http.h\n+++ b/http.h\n@@ -2,6 +2,7 @@\n #define HTTP_H\n \n struct packed_git;\n+struct packfile_list;\n \n #include \"git-zlib.h\"\n \n@@ -190,7 +191,7 @@ struct curl_slist *http_append_auth_header(const struct credential *c,\n \n /* Helpers for fetching packs */\n int http_get_info_packs(const char *base_url,\n-\t\t\tstruct packed_git **packs_head);\n+\t\t\tstruct packfile_list *packs);\n \n /* Helper for getting Accept-Language header */\n const char *http_get_accept_language_header(void);\n@@ -226,7 +227,7 @@ void release_http_pack_request(struct http_pack_request *preq);\n * from http_get_info_packs() and have chosen a specific pack to fetch.\n */\n void http_install_packfile(struct packed_git *p,\n-\t\t\t struct packed_git **list_to_remove_from);\n+\t\t\t struct packfile_list *list_to_remove_from);\n \n /* Helpers for fetching object */\n struct http_object_request {\ndiff --git a/packfile.c b/packfile.c\nindex 4d2d3b674f3..6aa2ca8ac9e 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -121,6 +121,15 @@ void packfile_list_append(struct packfile_list *list, struct packed_git *pack)\n \t}\n }\n \n+struct packed_git *packfile_list_find_oid(struct packfile_list_entry *packs,\n+\t\t\t\t\t const struct object_id *oid)\n+{\n+\tfor (; packs; packs = packs->next)\n+\t\tif (find_pack_entry_one(oid, packs->pack))\n+\t\t\treturn packs->pack;\n+\treturn NULL;\n+}\n+\n void pack_report(struct repository *repo)\n {\n \tfprintf(stderr,\ndiff --git a/packfile.h b/packfile.h\nindex 39ed1073e4a..a53336d722a 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -65,6 +65,14 @@ void packfile_list_remove(struct packfile_list *list, struct packed_git *pack);\n void packfile_list_prepend(struct packfile_list *list, struct packed_git *pack);\n void packfile_list_append(struct packfile_list *list, struct packed_git *pack);\n \n+/*\n+ * Find the pack within the \"packs\" list whose index contains the object\n+ * \"oid\". For general object lookups, you probably don't want this; use\n+ * find_pack_entry() instead.\n+ */\n+struct packed_git *packfile_list_find_oid(struct packfile_list_entry *packs,\n+\t\t\t\t\t const struct object_id *oid);\n+\n /*\n * A store that manages packfiles for a given object database.\n */\n\n-- \n2.51.2.997.g839fc31de9.dirty\n\n"},{"id":"529783","messageId":"20251028-pks-packfiles-store-drop-list-v1-4-1a3b82030a7a@pks.im","threadId":"64398","inReplyTo":"20251028-pks-packfiles-store-drop-list-v1-0-1a3b82030a7a@pks.im","subject":"[PATCH 4/8] packfile: fix approximation of object counts","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-10-28T11:08:34Z","receivedAt":"2025-10-28T11:08:52Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When approximating the number of objects in a repository we only take\ninto account two data sources, the multi-pack index and the packfile\nindices, as both of these data structures allow us to easily figure out\nhow many objects they contain.\n\nBut the way we currently approximate the number of objects is broken in\npresence of a multi-pack index. This is due to two separate reasons:\n\n - We have recently introduced initial infrastructure for incremental\n multi-pack indices. Starting with that series, `num_objects` only\n counts the number of objects of a specific layer of the MIDX chain,\n so we do not take into account objects from parent layers.\n\n This issue is fixed by adding `num_objects_in_base`, which contains\n the sum of all objects in previous layers.\n\n - When using the multi-pack index we may count objects contained in\n packfiles twice: once via the multi-pack index, but then we again\n count them via the packfile itself.\n\n This issue is fixed by skipping any packfiles that have an MIDX.\n\nOverall, given that we _always_ count the packs, we can only end up\noverestimating the number of objects, and the overestimation is limited\nto a factor of two at most.\n\nThe consequences of those issues are very limited though, as we only\napproximate object counts in a small number of cases:\n\n - When writing a commit-graph we use the approximate object count to\n display the upper limit of a progress display.\n\n - In `repo_find_unique_abbrev_r()` we use it to specify a lower limit\n of how many hex digits we want to abbreviate to. Given that we use\n power-of-two here to derive the lower limit we may end up with an\n abbreviated hash that is one digit longer than required.\n\n - In `estimate_repack_memory()` we may end up overestimating how much\n memory a repack needs to pack objects. Conseuqently, we may end up\n dropping some packfiles from a repack.\n\nNone of these are really game-changing. But it's nice to fix those\nissues regardless.\n\nWhile at it, convert the code to use `repo_for_each_pack()`.\nFurthermore, use `odb_prepare_alternates()` instead of explicitly\npreparing the packfile store. We really only want to prepare the object\ndatabase sources, and `get_multi_pack_index()` already knows to prepare\nthe packfile store for us.\n\nHelped-by: Taylor Blau \nSigned-off-by: Patrick Steinhardt \n---\n packfile.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/packfile.c b/packfile.c\nindex 6aa2ca8ac9e..6722c3b2b88 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1143,16 +1143,16 @@ unsigned long repo_approximate_object_count(struct repository *r)\n \t\tunsigned long count = 0;\n \t\tstruct packed_git *p;\n \n-\t\tpackfile_store_prepare(r->objects->packfiles);\n+\t\todb_prepare_alternates(r->objects);\n \n \t\tfor (source = r->objects->sources; source; source = source->next) {\n \t\t\tstruct multi_pack_index *m = get_multi_pack_index(source);\n \t\t\tif (m)\n-\t\t\t\tcount += m->num_objects;\n+\t\t\t\tcount += m->num_objects + m->num_objects_in_base;\n \t\t}\n \n-\t\tfor (p = r->objects->packfiles->packs; p; p = p->next) {\n-\t\t\tif (open_pack_index(p))\n+\t\trepo_for_each_pack(r, p) {\n+\t\t\tif (open_pack_index(p) || p->multi_pack_index)\n \t\t\t\tcontinue;\n \t\t\tcount += p->num_objects;\n \t\t}\n\n-- \n2.51.2.997.g839fc31de9.dirty\n\n"},{"id":"529784","messageId":"20251028-pks-packfiles-store-drop-list-v1-5-1a3b82030a7a@pks.im","threadId":"64398","inReplyTo":"20251028-pks-packfiles-store-drop-list-v1-0-1a3b82030a7a@pks.im","subject":"[PATCH 5/8] builtin/pack-objects: simplify logic to find kept or nonlocal objects","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-10-28T11:08:35Z","receivedAt":"2025-10-28T11:08:55Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The function `has_sha1_pack_kept_or_nonlocal()` takes an object ID and\nthen searches through packed objects to figure out whether the object\nexists in a kept or non-local pack. As a performance optimization we\nremember the packfile that contains a given object ID so that the next\ncall to the function first checks that same packfile again.\n\nThe way this is written is rather hard to follow though, as the caching\nmechanism is intertwined with the loop that iterates through the packs.\nConsequently, we need to do some gymnastics to re-start the iteration if\nthe cached pack does not contain the objects.\n\nRefactor this so that we check the cached packfile at the beginning. We\ndon't have to re-verify whether the packfile meets the properties as we\nhave already verified those when storing the pack in `last_found` in the\nfirst place. So all we need to do is to use `find_pack_entry_one()` to\ncheck whether the pack contains the object ID, and to skip the cached\npack in the loop so that we don't search it twice.\n\nThis refactoring significantly simplifies the logic and makes it much\neasier to follow.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/pack-objects.c | 26 +++++++++++++-------------\n 1 file changed, 13 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 5348aebbe9f..861fef3f38a 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -4388,27 +4388,27 @@ static void add_unreachable_loose_objects(struct rev_info *revs)\n \n static int has_sha1_pack_kept_or_nonlocal(const struct object_id *oid)\n {\n-\tstruct packfile_store *packs = the_repository->objects->packfiles;\n \tstatic struct packed_git *last_found = (void *)1;\n \tstruct packed_git *p;\n \n-\tp = (last_found != (void *)1) ? last_found :\n-\t\t\t\t\tpackfile_store_get_packs(packs);\n+\tif (last_found != (void *)1 && find_pack_entry_one(oid, last_found))\n+\t\treturn 1;\n \n-\twhile (p) {\n-\t\tif ((!p->pack_local || p->pack_keep ||\n-\t\t\t\tp->pack_keep_in_core) &&\n-\t\t\tfind_pack_entry_one(oid, p)) {\n+\trepo_for_each_pack(the_repository, p) {\n+\t\tif ((!p->pack_local || p->pack_keep || p->pack_keep_in_core) &&\n+\t\t find_pack_entry_one(oid, p)) {\n \t\t\tlast_found = p;\n \t\t\treturn 1;\n \t\t}\n-\t\tif (p == last_found)\n-\t\t\tp = packfile_store_get_packs(packs);\n-\t\telse\n-\t\t\tp = p->next;\n-\t\tif (p == last_found)\n-\t\t\tp = p->next;\n+\n+\t\t/*\n+\t\t * We have already checked `last_found`, so there is no need to\n+\t\t * re-check here.\n+\t\t */\n+\t\tif (p == last_found && last_found != (void *)1)\n+\t\t\tcontinue;\n \t}\n+\n \treturn 0;\n }\n \n\n-- \n2.51.2.997.g839fc31de9.dirty\n\n"},{"id":"529785","messageId":"20251028-pks-packfiles-store-drop-list-v1-6-1a3b82030a7a@pks.im","threadId":"64398","inReplyTo":"20251028-pks-packfiles-store-drop-list-v1-0-1a3b82030a7a@pks.im","subject":"[PATCH 6/8] packfile: move list of packs into the packfile store","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-10-28T11:08:36Z","receivedAt":"2025-10-28T11:08:58Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Move the list of packs into the packfile store. This follows the same\nlogic as in a previous commit, where we moved the most-recently-used\nlist of packs, as well.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/fast-import.c | 4 +--\n packfile.c | 83 +++++++++++++++++++++++----------------------------\n packfile.h | 16 +++-------\n 3 files changed, 43 insertions(+), 60 deletions(-)\n\ndiff --git a/builtin/fast-import.c b/builtin/fast-import.c\nindex 215295c1561..6fe6e9bc61d 100644\n--- a/builtin/fast-import.c\n+++ b/builtin/fast-import.c\n@@ -978,7 +978,7 @@ static int store_object(\n \tif (e->idx.offset) {\n \t\tduplicate_count_by_type[type]++;\n \t\treturn 1;\n-\t} else if (find_oid_pack(&oid, packfile_store_get_packs(packs))) {\n+\t} else if (packfile_list_find_oid(packfile_store_get_packs(packs), &oid)) {\n \t\te->type = type;\n \t\te->pack_id = MAX_PACK_ID;\n \t\te->idx.offset = 1; /* just not zero! */\n@@ -1179,7 +1179,7 @@ static void stream_blob(uintmax_t len, struct object_id *oidout, uintmax_t mark)\n \t\tduplicate_count_by_type[OBJ_BLOB]++;\n \t\ttruncate_pack(&checkpoint);\n \n-\t} else if (find_oid_pack(&oid, packfile_store_get_packs(packs))) {\n+\t} else if (packfile_list_find_oid(packfile_store_get_packs(packs), &oid)) {\n \t\te->type = OBJ_BLOB;\n \t\te->pack_id = MAX_PACK_ID;\n \t\te->idx.offset = 1; /* just not zero! */\ndiff --git a/packfile.c b/packfile.c\nindex 6722c3b2b88..f8158c1aa52 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -356,13 +356,14 @@ static void scan_windows(struct packed_git *p,\n \n static int unuse_one_window(struct packed_git *current)\n {\n-\tstruct packed_git *p, *lru_p = NULL;\n+\tstruct packfile_list_entry *e;\n+\tstruct packed_git *lru_p = NULL;\n \tstruct pack_window *lru_w = NULL, *lru_l = NULL;\n \n \tif (current)\n \t\tscan_windows(current, &lru_p, &lru_w, &lru_l);\n-\tfor (p = current->repo->objects->packfiles->packs; p; p = p->next)\n-\t\tscan_windows(p, &lru_p, &lru_w, &lru_l);\n+\tfor (e = current->repo->objects->packfiles->packs.head; e; e = e->next)\n+\t\tscan_windows(e->pack, &lru_p, &lru_w, &lru_l);\n \tif (lru_p) {\n \t\tmunmap(lru_w->base, lru_w->len);\n \t\tpack_mapped -= lru_w->len;\n@@ -542,14 +543,15 @@ static void find_lru_pack(struct packed_git *p, struct packed_git **lru_p, struc\n \n static int close_one_pack(struct repository *r)\n {\n-\tstruct packed_git *p, *lru_p = NULL;\n+\tstruct packfile_list_entry *e;\n+\tstruct packed_git *lru_p = NULL;\n \tstruct pack_window *mru_w = NULL;\n \tint accept_windows_inuse = 1;\n \n-\tfor (p = r->objects->packfiles->packs; p; p = p->next) {\n-\t\tif (p->pack_fd == -1)\n+\tfor (e = r->objects->packfiles->packs.head; e; e = e->next) {\n+\t\tif (e->pack->pack_fd == -1)\n \t\t\tcontinue;\n-\t\tfind_lru_pack(p, &lru_p, &mru_w, &accept_windows_inuse);\n+\t\tfind_lru_pack(e->pack, &lru_p, &mru_w, &accept_windows_inuse);\n \t}\n \n \tif (lru_p)\n@@ -868,8 +870,7 @@ void packfile_store_add_pack(struct packfile_store *store,\n \tif (pack->pack_fd != -1)\n \t\tpack_open_fds++;\n \n-\tpack->next = store->packs;\n-\tstore->packs = pack;\n+\tpackfile_list_prepend(&store->packs, pack);\n \n \tstrmap_put(&store->packs_by_path, pack->pack_name, pack);\n }\n@@ -1046,9 +1047,10 @@ static void prepare_packed_git_one(struct odb_source *source)\n \tstring_list_clear(data.garbage, 0);\n }\n \n-DEFINE_LIST_SORT(static, sort_packs, struct packed_git, next);\n+DEFINE_LIST_SORT(static, sort_packs, struct packfile_list_entry, next);\n \n-static int sort_pack(const struct packed_git *a, const struct packed_git *b)\n+static int sort_pack(const struct packfile_list_entry *a,\n+\t\t const struct packfile_list_entry *b)\n {\n \tint st;\n \n@@ -1058,7 +1060,7 @@ static int sort_pack(const struct packed_git *a, const struct packed_git *b)\n \t * remote ones could be on a network mounted filesystem.\n \t * Favor local ones for these reasons.\n \t */\n-\tst = a->pack_local - b->pack_local;\n+\tst = a->pack->pack_local - b->pack->pack_local;\n \tif (st)\n \t\treturn -st;\n \n@@ -1067,21 +1069,19 @@ static int sort_pack(const struct packed_git *a, const struct packed_git *b)\n \t * and more recent objects tend to get accessed more\n \t * often.\n \t */\n-\tif (a->mtime < b->mtime)\n+\tif (a->pack->mtime < b->pack->mtime)\n \t\treturn 1;\n-\telse if (a->mtime == b->mtime)\n+\telse if (a->pack->mtime == b->pack->mtime)\n \t\treturn 0;\n \treturn -1;\n }\n \n static void packfile_store_prepare_mru(struct packfile_store *store)\n {\n-\tstruct packed_git *p;\n-\n \tpackfile_list_clear(&store->mru);\n \n-\tfor (p = store->packs; p; p = p->next)\n-\t\tpackfile_list_append(&store->mru, p);\n+\tfor (struct packfile_list_entry *e = store->packs.head; e; e = e->next)\n+\t\tpackfile_list_append(&store->mru, e->pack);\n }\n \n void packfile_store_prepare(struct packfile_store *store)\n@@ -1096,7 +1096,11 @@ void packfile_store_prepare(struct packfile_store *store)\n \t\tprepare_multi_pack_index_one(source);\n \t\tprepare_packed_git_one(source);\n \t}\n-\tsort_packs(&store->packs, sort_pack);\n+\n+\tsort_packs(&store->packs.head, sort_pack);\n+\tfor (struct packfile_list_entry *e = store->packs.head; e; e = e->next)\n+\t\tif (!e->next)\n+\t\t\tstore->packs.tail = e;\n \n \tpackfile_store_prepare_mru(store);\n \tstore->initialized = true;\n@@ -1108,7 +1112,7 @@ void packfile_store_reprepare(struct packfile_store *store)\n \tpackfile_store_prepare(store);\n }\n \n-struct packed_git *packfile_store_get_packs(struct packfile_store *store)\n+struct packfile_list_entry *packfile_store_get_packs(struct packfile_store *store)\n {\n \tpackfile_store_prepare(store);\n \n@@ -1120,7 +1124,7 @@ struct packed_git *packfile_store_get_packs(struct packfile_store *store)\n \t\t\tprepare_midx_pack(m, i);\n \t}\n \n-\treturn store->packs;\n+\treturn store->packs.head;\n }\n \n struct packfile_list_entry *packfile_store_get_packs_mru(struct packfile_store *store)\n@@ -1276,11 +1280,11 @@ void mark_bad_packed_object(struct packed_git *p, const struct object_id *oid)\n const struct packed_git *has_packed_and_bad(struct repository *r,\n \t\t\t\t\t const struct object_id *oid)\n {\n-\tstruct packed_git *p;\n+\tstruct packfile_list_entry *e;\n \n-\tfor (p = r->objects->packfiles->packs; p; p = p->next)\n-\t\tif (oidset_contains(&p->bad_objects, oid))\n-\t\t\treturn p;\n+\tfor (e = r->objects->packfiles->packs.head; e; e = e->next)\n+\t\tif (oidset_contains(&e->pack->bad_objects, oid))\n+\t\t\treturn e->pack;\n \treturn NULL;\n }\n \n@@ -2088,19 +2092,6 @@ int is_pack_valid(struct packed_git *p)\n \treturn !open_packed_git(p);\n }\n \n-struct packed_git *find_oid_pack(const struct object_id *oid,\n-\t\t\t\t struct packed_git *packs)\n-{\n-\tstruct packed_git *p;\n-\n-\tfor (p = packs; p; p = p->next) {\n-\t\tif (find_pack_entry_one(oid, p))\n-\t\t\treturn p;\n-\t}\n-\treturn NULL;\n-\n-}\n-\n static int fill_pack_entry(const struct object_id *oid,\n \t\t\t struct pack_entry *e,\n \t\t\t struct packed_git *p)\n@@ -2139,7 +2130,7 @@ int find_pack_entry(struct repository *r, const struct object_id *oid, struct pa\n \t\tif (source->midx && fill_midx_entry(source->midx, oid, e))\n \t\t\treturn 1;\n \n-\tif (!r->objects->packfiles->packs)\n+\tif (!r->objects->packfiles->packs.head)\n \t\treturn 0;\n \n \tfor (l = r->objects->packfiles->mru.head; l; l = l->next) {\n@@ -2404,19 +2395,19 @@ struct packfile_store *packfile_store_new(struct object_database *odb)\n \n void packfile_store_free(struct packfile_store *store)\n {\n-\tfor (struct packed_git *p = store->packs, *next; p; p = next) {\n-\t\tnext = p->next;\n-\t\tfree(p);\n-\t}\n+\tfor (struct packfile_list_entry *e = store->packs.head; e; e = e->next)\n+\t\tfree(e->pack);\n+\tpackfile_list_clear(&store->packs);\n+\n \tstrmap_clear(&store->packs_by_path, 0);\n \tfree(store);\n }\n \n void packfile_store_close(struct packfile_store *store)\n {\n-\tfor (struct packed_git *p = store->packs; p; p = p->next) {\n-\t\tif (p->do_not_close)\n+\tfor (struct packfile_list_entry *e = store->packs.head; e; e = e->next) {\n+\t\tif (e->pack->do_not_close)\n \t\t\tBUG(\"want to close pack marked 'do-not-close'\");\n-\t\tclose_pack(p);\n+\t\tclose_pack(e->pack);\n \t}\n }\ndiff --git a/packfile.h b/packfile.h\nindex a53336d722a..d95275e666c 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -11,7 +11,6 @@\n struct object_info;\n \n struct packed_git {\n-\tstruct packed_git *next;\n \tstruct pack_window *windows;\n \toff_t pack_size;\n \tconst void *index_data;\n@@ -83,7 +82,7 @@ struct packfile_store {\n \t * The list of packfiles in the order in which they are being added to\n \t * the store.\n \t */\n-\tstruct packed_git *packs;\n+\tstruct packfile_list packs;\n \n \t/*\n \t * Cache of packfiles which are marked as \"kept\", either because there\n@@ -163,13 +162,14 @@ void packfile_store_add_pack(struct packfile_store *store,\n * repository.\n */\n #define repo_for_each_pack(repo, p) \\\n-\tfor (p = packfile_store_get_packs(repo->objects->packfiles); p; p = p->next)\n+\tfor (struct packfile_list_entry *e = packfile_store_get_packs(repo->objects->packfiles); \\\n+\t ((p) = (e ? e->pack : NULL)); e = e->next)\n \n /*\n * Get all packs managed by the given store, including packfiles that are\n * referenced by multi-pack indices.\n */\n-struct packed_git *packfile_store_get_packs(struct packfile_store *store);\n+struct packfile_list_entry *packfile_store_get_packs(struct packfile_store *store);\n \n /*\n * Get all packs in most-recently-used order.\n@@ -266,14 +266,6 @@ extern void (*report_garbage)(unsigned seen_bits, const char *path);\n */\n unsigned long repo_approximate_object_count(struct repository *r);\n \n-/*\n- * Find the pack within the \"packs\" list whose index contains the object \"oid\".\n- * For general object lookups, you probably don't want this; use\n- * find_pack_entry() instead.\n- */\n-struct packed_git *find_oid_pack(const struct object_id *oid,\n-\t\t\t\t struct packed_git *packs);\n-\n void pack_report(struct repository *repo);\n \n /*\n\n-- \n2.51.2.997.g839fc31de9.dirty\n\n"},{"id":"529786","messageId":"20251028-pks-packfiles-store-drop-list-v1-7-1a3b82030a7a@pks.im","threadId":"64398","inReplyTo":"20251028-pks-packfiles-store-drop-list-v1-0-1a3b82030a7a@pks.im","subject":"[PATCH 7/8] packfile: always add packfiles to MRU when adding a pack","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-10-28T11:08:37Z","receivedAt":"2025-10-28T11:09:00Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When adding a packfile to it store we add it both to the list and map of\npackfiles, but we don't append it to the most-recently-used list of\npacks. We do know to add the packfile to the MRU list as soon as we\naccess any of its objects, but in between we're being inconistent. It\ndoesn't help that there are some subsystems that _do_ add the packfile\nto the MRU after having added it, which only adds to the confusion.\n\nRefactor the code so that we unconditionally add packfiles to the MRU\nwhen adding them to a packfile store.\n\nNote that this does not allow us to drop `packfile_store_prepare_mru()`\njust yet: while the MRU list is already populated with all packs now,\nthe order in which we add these packs is indeterministic for most of the\npart. So by first calling `sort_pack()` on the other packfile list and\nthen re-preparing the MRU list we inherit its sorting.\n\nSigned-off-by: Patrick Steinhardt \n---\n midx.c | 2 --\n packfile.c | 1 +\n 2 files changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 8022be9a45e..24e1e721754 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -462,8 +462,6 @@ int prepare_midx_pack(struct multi_pack_index *m,\n \t\t m->pack_names[pack_int_id]);\n \tp = packfile_store_load_pack(r->objects->packfiles,\n \t\t\t\t pack_name.buf, m->source->local);\n-\tif (p)\n-\t\tpackfile_list_append(&m->source->odb->packfiles->mru, p);\n \tstrbuf_release(&pack_name);\n \n \tif (!p) {\ndiff --git a/packfile.c b/packfile.c\nindex f8158c1aa52..79d2b27c42c 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -871,6 +871,7 @@ void packfile_store_add_pack(struct packfile_store *store,\n \t\tpack_open_fds++;\n \n \tpackfile_list_prepend(&store->packs, pack);\n+\tpackfile_list_append(&store->mru, pack);\n \n \tstrmap_put(&store->packs_by_path, pack->pack_name, pack);\n }\n\n-- \n2.51.2.997.g839fc31de9.dirty\n\n"},{"id":"529787","messageId":"20251028-pks-packfiles-store-drop-list-v1-8-1a3b82030a7a@pks.im","threadId":"64398","inReplyTo":"20251028-pks-packfiles-store-drop-list-v1-0-1a3b82030a7a@pks.im","subject":"[PATCH 8/8] packfile: track packs via the MRU list exclusively","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-10-28T11:08:38Z","receivedAt":"2025-10-28T11:09:03Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We track packfiles via two different lists:\n\n - `struct packfile_store::packs` is a list that sorts local packs\n first. In addition, these packs are sorted so that younger packs are\n sorted towards the front.\n\n - `struct packfile_store::mru` is a list that sorts packs so that\n most-recently used packs are at the front.\n\nThe reasoning behind the ordering in the `packs` list is that younger\nobjects stored in the local object store tend to be accessed more\nfrequently, and that is certainly true for some cases. But there are\ngoing to be lots of cases where that isn't true. Especially when\ntraversing history it is likely that one needs to access many older\nobjects, and due to our housekeeping it is very likely that almost all\nof those older objects will be contained in one large pack that is\noldest.\n\nSo whether or not the ordering makes sense really depends on the use\ncase at hand. A flexible approach like our MRU list addresses that need,\nas it will sort packs towards the front that are accessed all the time.\nIntuitively, this approach is thus able to satisfy more use cases more\nefficiently.\n\nThis reasoning casts some doubt on whether or not it really makes sense\nto track packs via two different lists. It causes confusion, and it is\nnot clear whether there are use cases where the `packs` list really is\nsuch an obvious choice.\n\nMerge these two lists into one most-recently-used list.\n\nNote that there is one important edge case: `for_each_packed_object()`\nuses the MRU list to iterate through packs, and then it lists each\nobject in those packs. This would have the effect that we now sort the\ncurrent pack towards the front, thus modifying the list of packfiles we\nare iterating over, with the consequence that we'll see an infinite\nloop. This edge case is worked around by introducing a new field that\nallows us to skip updating the MRU.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/pack-objects.c | 4 ++--\n packfile.c | 27 +++++++--------------------\n packfile.h | 27 +++++++++++++++++----------\n 3 files changed, 26 insertions(+), 32 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 861fef3f38a..3b73ab9f614 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -1748,11 +1748,11 @@ static int want_object_in_pack_mtime(const struct object_id *oid,\n \t\t}\n \t}\n \n-\tfor (e = the_repository->objects->packfiles->mru.head; e; e = e->next) {\n+\tfor (e = the_repository->objects->packfiles->packs.head; e; e = e->next) {\n \t\tstruct packed_git *p = e->pack;\n \t\twant = want_object_in_pack_one(p, oid, exclude, found_pack, found_offset, found_mtime);\n \t\tif (!exclude && want > 0)\n-\t\t\tpackfile_list_prepend(&the_repository->objects->packfiles->mru, p);\n+\t\t\tpackfile_list_prepend(&the_repository->objects->packfiles->packs, p);\n \t\tif (want != -1)\n \t\t\treturn want;\n \t}\ndiff --git a/packfile.c b/packfile.c\nindex 79d2b27c42c..8785e397104 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -870,9 +870,7 @@ void packfile_store_add_pack(struct packfile_store *store,\n \tif (pack->pack_fd != -1)\n \t\tpack_open_fds++;\n \n-\tpackfile_list_prepend(&store->packs, pack);\n-\tpackfile_list_append(&store->mru, pack);\n-\n+\tpackfile_list_append(&store->packs, pack);\n \tstrmap_put(&store->packs_by_path, pack->pack_name, pack);\n }\n \n@@ -1077,14 +1075,6 @@ static int sort_pack(const struct packfile_list_entry *a,\n \treturn -1;\n }\n \n-static void packfile_store_prepare_mru(struct packfile_store *store)\n-{\n-\tpackfile_list_clear(&store->mru);\n-\n-\tfor (struct packfile_list_entry *e = store->packs.head; e; e = e->next)\n-\t\tpackfile_list_append(&store->mru, e->pack);\n-}\n-\n void packfile_store_prepare(struct packfile_store *store)\n {\n \tstruct odb_source *source;\n@@ -1103,7 +1093,6 @@ void packfile_store_prepare(struct packfile_store *store)\n \t\tif (!e->next)\n \t\t\tstore->packs.tail = e;\n \n-\tpackfile_store_prepare_mru(store);\n \tstore->initialized = true;\n }\n \n@@ -1128,12 +1117,6 @@ struct packfile_list_entry *packfile_store_get_packs(struct packfile_store *stor\n \treturn store->packs.head;\n }\n \n-struct packfile_list_entry *packfile_store_get_packs_mru(struct packfile_store *store)\n-{\n-\tpackfile_store_prepare(store);\n-\treturn store->mru.head;\n-}\n-\n /*\n * Give a fast, rough count of the number of objects in the repository. This\n * ignores loose objects completely. If you have a lot of them, then either\n@@ -2134,11 +2117,12 @@ int find_pack_entry(struct repository *r, const struct object_id *oid, struct pa\n \tif (!r->objects->packfiles->packs.head)\n \t\treturn 0;\n \n-\tfor (l = r->objects->packfiles->mru.head; l; l = l->next) {\n+\tfor (l = r->objects->packfiles->packs.head; l; l = l->next) {\n \t\tstruct packed_git *p = l->pack;\n \n \t\tif (!p->multi_pack_index && fill_pack_entry(oid, e, p)) {\n-\t\t\tpackfile_list_prepend(&r->objects->packfiles->mru, p);\n+\t\t\tif (!r->objects->packfiles->skip_mru_updates)\n+\t\t\t\tpackfile_list_prepend(&r->objects->packfiles->packs, p);\n \t\t\treturn 1;\n \t\t}\n \t}\n@@ -2270,6 +2254,7 @@ int for_each_packed_object(struct repository *repo, each_packed_object_fn cb,\n \tint r = 0;\n \tint pack_errors = 0;\n \n+\trepo->objects->packfiles->skip_mru_updates = true;\n \trepo_for_each_pack(repo, p) {\n \t\tif ((flags & FOR_EACH_OBJECT_LOCAL_ONLY) && !p->pack_local)\n \t\t\tcontinue;\n@@ -2290,6 +2275,8 @@ int for_each_packed_object(struct repository *repo, each_packed_object_fn cb,\n \t\tif (r)\n \t\t\tbreak;\n \t}\n+\trepo->objects->packfiles->skip_mru_updates = false;\n+\n \treturn r ? r : pack_errors;\n }\n \ndiff --git a/packfile.h b/packfile.h\nindex d95275e666c..27ba607e7c5 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -79,8 +79,8 @@ struct packfile_store {\n \tstruct object_database *odb;\n \n \t/*\n-\t * The list of packfiles in the order in which they are being added to\n-\t * the store.\n+\t * The list of packfiles in the order in which they have been most\n+\t * recently used.\n \t */\n \tstruct packfile_list packs;\n \n@@ -98,9 +98,6 @@ struct packfile_store {\n \t\tunsigned flags;\n \t} kept_cache;\n \n-\t/* A most-recently-used ordered version of the packs list. */\n-\tstruct packfile_list mru;\n-\n \t/*\n \t * A map of packfile names to packed_git structs for tracking which\n \t * packs have been loaded already.\n@@ -112,6 +109,21 @@ struct packfile_store {\n \t * packs.\n \t */\n \tbool initialized;\n+\n+\t/*\n+\t * Usually, packfiles will be reordered to the front of the `packs`\n+\t * list whenever an object is looked up via them. This has the effect\n+\t * that packs that contain a lot of accessed objects will be located\n+\t * towards the front.\n+\t *\n+\t * This is usually desireable, but there are exceptions. One exception\n+\t * is when the looking up multiple objects in a loop for each packfile.\n+\t * In that case, we may easily end up with an infinite loop as the\n+\t * packfiles get reordered to the front repeatedly.\n+\t *\n+\t * Setting this field to `true` thus disables these reorderings.\n+\t */\n+\tbool skip_mru_updates;\n };\n \n /*\n@@ -171,11 +183,6 @@ void packfile_store_add_pack(struct packfile_store *store,\n */\n struct packfile_list_entry *packfile_store_get_packs(struct packfile_store *store);\n \n-/*\n- * Get all packs in most-recently-used order.\n- */\n-struct packfile_list_entry *packfile_store_get_packs_mru(struct packfile_store *store);\n-\n /*\n * Open the packfile and add it to the store if it isn't yet known. Returns\n * either the newly opened packfile or the preexisting packfile. Returns a\n\n-- \n2.51.2.997.g839fc31de9.dirty\n\n"},{"id":"529866","messageId":"87h5vhrdjq.fsf@iotcl.com","threadId":"64398","inReplyTo":"20251028-pks-packfiles-store-drop-list-v1-3-1a3b82030a7a@pks.im","subject":"Re: [PATCH 3/8] http: refactor subsystem to use `packfile_list`s","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2025-10-29T14:24:41Z","receivedAt":"2025-10-29T14:24:59Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"Patrick Steinhardt writes:\n\n> The dumb HTTP protocol directly fetches packfiles from the remote server\n> and temporarily stores them in a list of packfiles. Those packfiles are\n> not yet added to the repository's packfile store until we finalize the\n> whole fetch.\n>\n> Refactor the code to instead use a `struct packfile_list` to store those\n> packs. This prepares us for a subsequent change where the `->next`\n> pointer of `struct packed_git` will go away.\n>\n> Note that this refactoring creates some temporary duplication of code,\n> as we now have both `packfile_list_find_oid()` and `find_oid_pack()`.\n> The latter function will be removed in a subsequent commit though.\n>\n> Signed-off-by: Patrick Steinhardt \n> ---\n> http-push.c | 6 +++---\n> http-walker.c | 26 +++++++++-----------------\n> http.c | 21 ++++++++-------------\n> http.h | 5 +++--\n> packfile.c | 9 +++++++++\n> packfile.h | 8 ++++++++\n> 6 files changed, 40 insertions(+), 35 deletions(-)\n>\n> [snip]\n>\n> diff --git a/packfile.c b/packfile.c\n> index 4d2d3b674f3..6aa2ca8ac9e 100644\n> --- a/packfile.c\n> +++ b/packfile.c\n> @@ -121,6 +121,15 @@ void packfile_list_append(struct packfile_list *list, struct packed_git *pack)\n> \t}\n> }\n> \n> +struct packed_git *packfile_list_find_oid(struct packfile_list_entry *packs,\n> +\t\t\t\t\t const struct object_id *oid)\n> +{\n\nWhy does it take a `struct packfile_list_entry` and not a `struct\npackfile_list` ?\n\n-- \nCheers,\nToon\n"},{"id":"529870","messageId":"875xbxrc4q.fsf@iotcl.com","threadId":"64398","inReplyTo":"20251028-pks-packfiles-store-drop-list-v1-5-1a3b82030a7a@pks.im","subject":"Re: [PATCH 5/8] builtin/pack-objects: simplify logic to find kept or nonlocal objects","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2025-10-29T14:55:17Z","receivedAt":"2025-10-29T14:55:45Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"Patrick Steinhardt writes:\n\n> The function `has_sha1_pack_kept_or_nonlocal()` takes an object ID and\n> then searches through packed objects to figure out whether the object\n> exists in a kept or non-local pack. As a performance optimization we\n> remember the packfile that contains a given object ID so that the next\n> call to the function first checks that same packfile again.\n>\n> The way this is written is rather hard to follow though, as the caching\n> mechanism is intertwined with the loop that iterates through the packs.\n> Consequently, we need to do some gymnastics to re-start the iteration if\n> the cached pack does not contain the objects.\n\nOkay, this took me while, but yes this function was really hard to\nunderstand. Thanks for simplifying.\n\nNaive question, what's the point of keeping a \"last_found\"? We have one\nglobal \"last_found\" for the last time this function was called, and we\nhave no control which OIDs get passed to this function. Why look into\n\"last_found\" first?\n\n> Refactor this so that we check the cached packfile at the beginning. We\n> don't have to re-verify whether the packfile meets the properties as we\n> have already verified those when storing the pack in `last_found` in the\n> first place. So all we need to do is to use `find_pack_entry_one()` to\n> check whether the pack contains the object ID, and to skip the cached\n> pack in the loop so that we don't search it twice.\n>\n> This refactoring significantly simplifies the logic and makes it much\n> easier to follow.\n>\n> Signed-off-by: Patrick Steinhardt \n> ---\n> builtin/pack-objects.c | 26 +++++++++++++-------------\n> 1 file changed, 13 insertions(+), 13 deletions(-)\n>\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index 5348aebbe9f..861fef3f38a 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -4388,27 +4388,27 @@ static void add_unreachable_loose_objects(struct rev_info *revs)\n> \n> static int has_sha1_pack_kept_or_nonlocal(const struct object_id *oid)\n> {\n> -\tstruct packfile_store *packs = the_repository->objects->packfiles;\n> \tstatic struct packed_git *last_found = (void *)1;\n> \tstruct packed_git *p;\n> \n> -\tp = (last_found != (void *)1) ? last_found :\n> -\t\t\t\t\tpackfile_store_get_packs(packs);\n> +\tif (last_found != (void *)1 && find_pack_entry_one(oid, last_found))\n> +\t\treturn 1;\n> \n> -\twhile (p) {\n> -\t\tif ((!p->pack_local || p->pack_keep ||\n> -\t\t\t\tp->pack_keep_in_core) &&\n> -\t\t\tfind_pack_entry_one(oid, p)) {\n> +\trepo_for_each_pack(the_repository, p) {\n> +\t\tif ((!p->pack_local || p->pack_keep || p->pack_keep_in_core) &&\n> +\t\t find_pack_entry_one(oid, p)) {\n> \t\t\tlast_found = p;\n> \t\t\treturn 1;\n> \t\t}\n> -\t\tif (p == last_found)\n> -\t\t\tp = packfile_store_get_packs(packs);\n> -\t\telse\n> -\t\t\tp = p->next;\n> -\t\tif (p == last_found)\n> -\t\t\tp = p->next;\n> +\n> +\t\t/*\n> +\t\t * We have already checked `last_found`, so there is no need to\n> +\t\t * re-check here.\n> +\t\t */\n\nI had to reason with myself why you need to extra `(void *)1` check,\nmaybe you can extend the comment a bit:\n\n\t\t/*\n\t\t * When `last_found` was set to something else then\n\t\t * `(void *)1` we have already checked it,\n\t\t * so there is no need to re-check here.\n\t\t */\n\n> +\t\tif (p == last_found && last_found != (void *)1)\n> +\t\t\tcontinue;\n\n-- \nCheers,\nToon\n"},{"id":"529893","messageId":"aQKSMk6nFIk6Xomh@nand.local","threadId":"64398","inReplyTo":"20251028-pks-packfiles-store-drop-list-v1-1-1a3b82030a7a@pks.im","subject":"Re: [PATCH 1/8] packfile: use a `strmap` to store packs by name","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-10-29T22:16:18Z","receivedAt":"2025-10-29T22:16:22Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Oct 28, 2025 at 12:08:31PM +0100, Patrick Steinhardt wrote:\n> ---\n> packfile.c | 24 ++++--------------------\n> packfile.h | 4 ++--\n> 2 files changed, 6 insertions(+), 22 deletions(-)\n\nNice; well explained and the change below looks obviously correct to me.\nI much prefer the strmap API here and agree that this is a good use-case\nfor it.\n\nThanks,\nTaylor\n"},{"id":"529906","messageId":"aQKXpM8g3Oy3DVAa@nand.local","threadId":"64398","inReplyTo":"20251028-pks-packfiles-store-drop-list-v1-2-1a3b82030a7a@pks.im","subject":"Re: [PATCH 2/8] packfile: move the MRU list into the packfile store","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-10-29T22:39:32Z","receivedAt":"2025-10-29T22:39:35Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Oct 28, 2025 at 12:08:32PM +0100, Patrick Steinhardt wrote:\n> Packfiles have two lists associated to them:\n>\n> - A list that keeps track of packfiles in the order that they were\n> added to a packfile store.\n>\n> - A list that keeps track of packfiles in most-recently-used order so\n> that packfiles that are more likely to contain a specific object are\n> ordered towards the front.\n>\n> Both of these lists are hosted by `struct packed_git` itself, So to\n> identify all packfiles in a repository you simply need to grab the first\n> packfile and then iterate the `->next` pointers or the MRU list. This\n> pattern has the problem that all packfiles are part of the same list,\n> regardless of whether or not they belong to the same object source.\n>\n> With the upcoming pluggable object database effort this needs to change:\n> packfiles should be contained by a single object source, and reading an\n> object from any such packfile should use that source to look up the\n> object. Consequently, we need to break up the global lists of packfiles\n\ns/lists/list/\n\n> into per-object-source lists.\n\nHow does this work for alternates? My understanding is that each\nalternate now has its own object source. So to perform an object lookup\nin a repository with alternate(s), I am assuming that at some layer we\nneed to iterate over those sources to then enumerate the packs in that\nsource looking for some object.\n\nI would have imagined that packfile.c::find_pack_entry() would have to\nbe adjusted in a similar way as above, but I couldn't find the changes\nin this series, so I feel like I must be missing something in my\nunderstanding of how this all works together :-).\n\nAre packs from different sources still connected somehow such that\niterating over the list of packs from one source will enumerate the list\nof packs from all sources?\n\n> A first step towards this goal is to move those lists ouf of `struct\n\ns/ouf/out/\n\n> packed_git` and into the packfile store. While the packfile store is\n> currently sitting on the `struct object_database` level, the intent is\n> to push it down one level into the `struct odb_source` in a subsequent\n> patch series.\n\nBefore sending, I was confused by \"Consequently, we need to break up the\nglobal lists of packfiles [...]\", since it wasn't clear whether or not\nthis series realizes that goal, or pushes us in the direction towards\nit.\n\nBut this clarifies things, and I think is the reason that we do not see\nmore invasive changes like needing to enumerate the MRU cache of each\nstore in order to find an object like I mentioned above.\n\n> Introduce a new `struct packfile_list` that is used to manage lists of\n> packfiles and use it to store the list of most-recently-used packfiles\n> in `struct packfile_store`. For now, the new list type is only used in a\n> single spot, but we'll expand its usage in subsequent patches.\n\nI am a little curious why we need a new list type and implementation\nhere. Is it to avoid exposing the list as part of struct packed_git like\nwe are forced to do with list_head?\n\nI could imagine that you might want to avoid exposing the \"struct\nlist_head mru\" part of packed_git to avoid the suggestion that all\npackfiles (including those from different sources) are part of the same\nlist. But if that's the case, I wonder if we couldn't have kept the same\nmru list and clarified via comment that it is per-store, not global.\n\nI suppose that is a bit of a foot-gun, and perhaps that is what you are\ntrying to do here, but after reading the patch message a few times I\nwasn't clear on what the motivation for the new type was.\n\nThanks,\nTaylor\n"},{"id":"529908","messageId":"aQKZ7FW925zvscgh@nand.local","threadId":"64398","inReplyTo":"20251028-pks-packfiles-store-drop-list-v1-4-1a3b82030a7a@pks.im","subject":"Re: [PATCH 4/8] packfile: fix approximation of object counts","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-10-29T22:49:16Z","receivedAt":"2025-10-29T22:49:19Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Oct 28, 2025 at 12:08:34PM +0100, Patrick Steinhardt wrote:\n> None of these are really game-changing. But it's nice to fix those\n> issues regardless.\n\nWell explained, thank you.\n\n> While at it, convert the code to use `repo_for_each_pack()`.\n> Furthermore, use `odb_prepare_alternates()` instead of explicitly\n> preparing the packfile store. We really only want to prepare the object\n> database sources, and `get_multi_pack_index()` already knows to prepare\n> the packfile store for us.\n>\n> Helped-by: Taylor Blau \n> Signed-off-by: Patrick Steinhardt \n> ---\n> packfile.c | 8 ++++----\n> 1 file changed, 4 insertions(+), 4 deletions(-)\n>\n> diff --git a/packfile.c b/packfile.c\n> index 6aa2ca8ac9e..6722c3b2b88 100644\n> --- a/packfile.c\n> +++ b/packfile.c\n> @@ -1143,16 +1143,16 @@ unsigned long repo_approximate_object_count(struct repository *r)\n> \t\tunsigned long count = 0;\n> \t\tstruct packed_git *p;\n>\n> -\t\tpackfile_store_prepare(r->objects->packfiles);\n> +\t\todb_prepare_alternates(r->objects);\n\nI was wondering how this worked, since odb_prepare_alternates() does not\neagerly load the packs belonging to a MIDX, but get_multi_pack_index()\ndoes, so this makes sense.\n\n(Writing this out, I realized that you wrote this as the last sentence\nin your patch, which is helpful and I think worth doing.)\n\n> \t\tfor (source = r->objects->sources; source; source = source->next) {\n> \t\t\tstruct multi_pack_index *m = get_multi_pack_index(source);\n> \t\t\tif (m)\n> -\t\t\t\tcount += m->num_objects;\n> +\t\t\t\tcount += m->num_objects + m->num_objects_in_base;\n\nOops. This fix is definitely right, thanks for spotting and fixing it.\n\nAs a general aside, I expect that we're going to find some more of\nthese. I tried my best to audit all the places where we use\nm->num_objects and m->num_packs, but without having great infrastructure\nthat encourages the use of MIDX chains, most of this code is all dead\nanyway.\n\nHopefully soon we will see some more usage of MIDX chains with the\nincremental repacking work that I've been sending patches for recently.\nI'm sure that will flush out more of these issues.\n\n> -\t\tfor (p = r->objects->packfiles->packs; p; p = p->next) {\n> -\t\t\tif (open_pack_index(p))\n> +\t\trepo_for_each_pack(r, p) {\n> +\t\t\tif (open_pack_index(p) || p->multi_pack_index)\n\nDo we care about opening the pack index if we already accounted for it\nvia the MIDX path above? My guess is not, so I would probably suggest\nwriting this conditional as:\n\n if (p->multi_pack_index || open_pack_index(p))\n continue;\n\nto avoid loading pack indexes unless we have to.\n\nThanks,\nTaylor\n"},{"id":"529911","messageId":"aQKfnd2gWS2T9GaD@nand.local","threadId":"64398","inReplyTo":"20251028-pks-packfiles-store-drop-list-v1-5-1a3b82030a7a@pks.im","subject":"Re: [PATCH 5/8] builtin/pack-objects: simplify logic to find kept or nonlocal objects","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-10-29T23:13:33Z","receivedAt":"2025-10-29T23:13:36Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Oct 28, 2025 at 12:08:35PM +0100, Patrick Steinhardt wrote:\n> @@ -4388,27 +4388,27 @@ static void add_unreachable_loose_objects(struct rev_info *revs)\n>\n> static int has_sha1_pack_kept_or_nonlocal(const struct object_id *oid)\n> {\n> -\tstruct packfile_store *packs = the_repository->objects->packfiles;\n> \tstatic struct packed_git *last_found = (void *)1;\n> \tstruct packed_git *p;\n>\n> -\tp = (last_found != (void *)1) ? last_found :\n> -\t\t\t\t\tpackfile_store_get_packs(packs);\n> +\tif (last_found != (void *)1 && find_pack_entry_one(oid, last_found))\n> +\t\treturn 1;\n\nThis all looks right to me, since we only assign last_found when a pack\nmeets the kept-or-non-local criteria, which is good.\n\n>\n> -\twhile (p) {\n> -\t\tif ((!p->pack_local || p->pack_keep ||\n> -\t\t\t\tp->pack_keep_in_core) &&\n> -\t\t\tfind_pack_entry_one(oid, p)) {\n> +\trepo_for_each_pack(the_repository, p) {\n> +\t\tif ((!p->pack_local || p->pack_keep || p->pack_keep_in_core) &&\n> +\t\t find_pack_entry_one(oid, p)) {\n> \t\t\tlast_found = p;\n> \t\t\treturn 1;\n> \t\t}\n> -\t\tif (p == last_found)\n> -\t\t\tp = packfile_store_get_packs(packs);\n> -\t\telse\n> -\t\t\tp = p->next;\n> -\t\tif (p == last_found)\n> -\t\t\tp = p->next;\n> +\n> +\t\t/*\n> +\t\t * We have already checked `last_found`, so there is no need to\n> +\t\t * re-check here.\n> +\t\t */\n> +\t\tif (p == last_found && last_found != (void *)1)\n> +\t\t\tcontinue;\n\nCan 'p' ever be (void *)1 here? I would imagine not since this is coming\nfrom repo_for_each_pack(), so I think it would suffice to limit this\nconditional to just \"if (p == last_found)\".\n\nOtherwise looks good. I think you could make use of the kept_cache here\nat least for the local-but-kept packs, but what you wrote is definitely\nan improvement in readability.\n\n static int has_sha1_pack_kept_or_nonlocal(const struct object_id *oid)\n {\n static struct packed_git *last_found = (void *)1;\n uint32_t kept_flags = ON_DISK_KEEP_PACKS | IN_CORE_KEEP_PACKS;\n struct packed_git *p;\n struct pack_entry entry;\n\n if (last_found != (void *)1 && find_pack_entry_one(oid, last_found))\n return 1;\n\n if (find_kept_pack_entry(the_repository, oid, flags, &entry)) {\n last_found = entry.p;\n return 1;\n }\n\n repo_for_each_pack(the_repository, p) {\n if (p->pack_local || p == last_found)\n continue;\n if (find_pack_entry_one(oid, p)) {\n last_found = p;\n return 1;\n }\n }\n return 0;\n }\n\nYou still end up looping over all of the packs in the worst case, but in\nthe case where you are going to pick up a copy of the object via a kept\npack, the above should be faster since it will focus the search on those\npacks first.\n\nI'm not sure that it matters all that much, since at worst we're\ninterspersing the search over kept packs with some non-local ones, and\nskipping over the rest. But certainly you could imagine cases where the\nnumber of non-local packs greatly outnumbers the local, kept ones, so in\na situation like that I think the above would help.\n\nProbably micro-optimizing this case is not all that useful, since this\nonly comes up when we are unpacking unreachable objects like with 'git\nrepack -A', which should be using cruft packs anyway.\n\nThanks,\nTaylor\n"},{"id":"529912","messageId":"aQKf9vNkAkm6m216@nand.local","threadId":"64398","inReplyTo":"875xbxrc4q.fsf@iotcl.com","subject":"Re: [PATCH 5/8] builtin/pack-objects: simplify logic to find kept or nonlocal objects","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-10-29T23:15:02Z","receivedAt":"2025-10-29T23:15:04Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Oct 29, 2025 at 03:55:17PM +0100, Toon Claes wrote:\n> > +\t\t/*\n> > +\t\t * We have already checked `last_found`, so there is no need to\n> > +\t\t * re-check here.\n> > +\t\t */\n>\n> I had to reason with myself why you need to extra `(void *)1` check,\n> maybe you can extend the comment a bit:\n>\n> \t\t/*\n> \t\t * When `last_found` was set to something else then\n> \t\t * `(void *)1` we have already checked it,\n> \t\t * so there is no need to re-check here.\n> \t\t */\n>\n> > +\t\tif (p == last_found && last_found != (void *)1)\n> > +\t\t\tcontinue;\n\nI wrote above to Patrick that I think the \"&& last_found != (void *)1\"\npart can be dropped, since repo_for_each_pack() should never hand us\nsuch a pointer to begin with.\n\nThanks,\nTaylor\n"},{"id":"529914","messageId":"aQKiT9JA+3zF4DHA@nand.local","threadId":"64398","inReplyTo":"20251028-pks-packfiles-store-drop-list-v1-7-1a3b82030a7a@pks.im","subject":"Re: [PATCH 7/8] packfile: always add packfiles to MRU when adding a pack","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-10-29T23:25:03Z","receivedAt":"2025-10-29T23:25:06Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Oct 28, 2025 at 12:08:37PM +0100, Patrick Steinhardt wrote:\n> When adding a packfile to it store we add it both to the list and map of\n> packfiles, but we don't append it to the most-recently-used list of\n> packs. We do know to add the packfile to the MRU list as soon as we\n> access any of its objects, but in between we're being inconistent. It\n> doesn't help that there are some subsystems that _do_ add the packfile\n> to the MRU after having added it, which only adds to the confusion.\n>\n> Refactor the code so that we unconditionally add packfiles to the MRU\n> when adding them to a packfile store.\n\nReading this, I thought that the MRU cache lazily added packs only upon\na successful object lookup, but looking more closely,\npackfile_store_prepare_mru() adds all of the known packs to the MRU\ncache eagerly.\n\nI think I would probably advocate in the long term that we go the other\nway here, which would be to avoid adding packs to the MRU cache until we\nhave found an object within them. But that is a larger change, since we\ndon't add packs outside of the MRU cache to them, only move packs which\nare already in the MRU cache around.\n\nBut I think in the immediate term what you wrote here makes sense, and\nit makes the behavior consistent in the meantime.\n\nThanks,\nTaylor\n"},{"id":"529924","messageId":"aQMotPUNIPUfa6U-@pks.im","threadId":"64398","inReplyTo":"87h5vhrdjq.fsf@iotcl.com","subject":"Re: [PATCH 3/8] http: refactor subsystem to use `packfile_list`s","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-10-30T08:58:28Z","receivedAt":"2025-10-30T08:58:39Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Oct 29, 2025 at 03:24:41PM +0100, Toon Claes wrote:\n> Patrick Steinhardt writes:\n> diff --git a/packfile.c b/packfile.c\n> > index 4d2d3b674f3..6aa2ca8ac9e 100644\n> > --- a/packfile.c\n> > +++ b/packfile.c\n> > @@ -121,6 +121,15 @@ void packfile_list_append(struct packfile_list *list, struct packed_git *pack)\n> > \t}\n> > }\n> > \n> > +struct packed_git *packfile_list_find_oid(struct packfile_list_entry *packs,\n> > +\t\t\t\t\t const struct object_id *oid)\n> > +{\n> \n> Why does it take a `struct packfile_list_entry` and not a `struct\n> packfile_list` ?\n\nThis is because `packfile_store_get_packs()` returns the first entry and\nnot head itself. It makes the interface a bit easier to use going\nforward.\n\nPatrick\n"},{"id":"529925","messageId":"aQMov9F59ZFXSqAG@pks.im","threadId":"64398","inReplyTo":"aQKZ7FW925zvscgh@nand.local","subject":"Re: [PATCH 4/8] packfile: fix approximation of object counts","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-10-30T08:58:39Z","receivedAt":"2025-10-30T08:58:45Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Oct 29, 2025 at 06:49:16PM -0400, Taylor Blau wrote:\n> On Tue, Oct 28, 2025 at 12:08:34PM +0100, Patrick Steinhardt wrote:\n> > diff --git a/packfile.c b/packfile.c\n> > index 6aa2ca8ac9e..6722c3b2b88 100644\n> > --- a/packfile.c\n> > +++ b/packfile.c\n[snip]\n> > -\t\tfor (p = r->objects->packfiles->packs; p; p = p->next) {\n> > -\t\t\tif (open_pack_index(p))\n> > +\t\trepo_for_each_pack(r, p) {\n> > +\t\t\tif (open_pack_index(p) || p->multi_pack_index)\n> \n> Do we care about opening the pack index if we already accounted for it\n> via the MIDX path above? My guess is not, so I would probably suggest\n> writing this conditional as:\n> \n> if (p->multi_pack_index || open_pack_index(p))\n> continue;\n> \n> to avoid loading pack indexes unless we have to.\n\nMakes sense indeed. We don't need it to have the MIDX prepared, so we\ncan avoid the function call if we don't have any.\n\nPatrick\n"},{"id":"529926","messageId":"aQMoyQAAeMD5WPxu@pks.im","threadId":"64398","inReplyTo":"aQKiT9JA+3zF4DHA@nand.local","subject":"Re: [PATCH 7/8] packfile: always add packfiles to MRU when adding a pack","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-10-30T08:58:49Z","receivedAt":"2025-10-30T08:58:54Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Oct 29, 2025 at 07:25:03PM -0400, Taylor Blau wrote:\n> On Tue, Oct 28, 2025 at 12:08:37PM +0100, Patrick Steinhardt wrote:\n> > When adding a packfile to it store we add it both to the list and map of\n> > packfiles, but we don't append it to the most-recently-used list of\n> > packs. We do know to add the packfile to the MRU list as soon as we\n> > access any of its objects, but in between we're being inconistent. It\n> > doesn't help that there are some subsystems that _do_ add the packfile\n> > to the MRU after having added it, which only adds to the confusion.\n> >\n> > Refactor the code so that we unconditionally add packfiles to the MRU\n> > when adding them to a packfile store.\n> \n> Reading this, I thought that the MRU cache lazily added packs only upon\n> a successful object lookup, but looking more closely,\n> packfile_store_prepare_mru() adds all of the known packs to the MRU\n> cache eagerly.\n\nHm, you're right, this description is quite misleading.\n\nOverall it's a mixed bag. We do add packfiles to the MRU initially via\n`packfile_store_prepare_mru()` as you mention. But there are direct and\nindirect callers of `packfile_store_add_pack()` that don't:\n\n - \"builtin/index-pack.c\"\n - \"builtin/fast-import.c\"\n - \"http.c\"\n\nIn all of these cases we expect that the objects will be read, so there\nis no reason to not have them in the MRU as far as I can see.\n\nWill rewrite the commit message, thanks!\n\nPatrick\n"},{"id":"529927","messageId":"aQMo0QJUPWqkbRNC@pks.im","threadId":"64398","inReplyTo":"aQKfnd2gWS2T9GaD@nand.local","subject":"Re: [PATCH 5/8] builtin/pack-objects: simplify logic to find kept or nonlocal objects","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-10-30T08:58:57Z","receivedAt":"2025-10-30T08:59:03Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Oct 29, 2025 at 07:13:33PM -0400, Taylor Blau wrote:\n> On Tue, Oct 28, 2025 at 12:08:35PM +0100, Patrick Steinhardt wrote:\n> > @@ -4388,27 +4388,27 @@ static void add_unreachable_loose_objects(struct rev_info *revs)\n> > -\twhile (p) {\n> > -\t\tif ((!p->pack_local || p->pack_keep ||\n> > -\t\t\t\tp->pack_keep_in_core) &&\n> > -\t\t\tfind_pack_entry_one(oid, p)) {\n> > +\trepo_for_each_pack(the_repository, p) {\n> > +\t\tif ((!p->pack_local || p->pack_keep || p->pack_keep_in_core) &&\n> > +\t\t find_pack_entry_one(oid, p)) {\n> > \t\t\tlast_found = p;\n> > \t\t\treturn 1;\n> > \t\t}\n> > -\t\tif (p == last_found)\n> > -\t\t\tp = packfile_store_get_packs(packs);\n> > -\t\telse\n> > -\t\t\tp = p->next;\n> > -\t\tif (p == last_found)\n> > -\t\t\tp = p->next;\n> > +\n> > +\t\t/*\n> > +\t\t * We have already checked `last_found`, so there is no need to\n> > +\t\t * re-check here.\n> > +\t\t */\n> > +\t\tif (p == last_found && last_found != (void *)1)\n> > +\t\t\tcontinue;\n> \n> Can 'p' ever be (void *)1 here? I would imagine not since this is coming\n> from repo_for_each_pack(), so I think it would suffice to limit this\n> conditional to just \"if (p == last_found)\".\n\nOh, you're right of course, will adapt.\n\nFurthermore, do we even need the `(void *)1` thingy? I think it should\nbe perfectly fine to instead use a `NULL` pointer here. A valid pack\nobviously cannot be a `NULL` pointer, so the sentinel feels kind of\npointless to me.\n\n> Otherwise looks good. I think you could make use of the kept_cache here\n> at least for the local-but-kept packs, but what you wrote is definitely\n> an improvement in readability.\n\nMakes sense. I'll leave this out of this series though as a #leftoverbit\nfor a future patch series :)\n\nThanks!\n\nPatrick\n"},{"id":"529928","messageId":"aQMo4PqG_U6JIkOt@pks.im","threadId":"64398","inReplyTo":"875xbxrc4q.fsf@iotcl.com","subject":"Re: [PATCH 5/8] builtin/pack-objects: simplify logic to find kept or nonlocal objects","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-10-30T08:59:12Z","receivedAt":"2025-10-30T08:59:18Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Oct 29, 2025 at 03:55:17PM +0100, Toon Claes wrote:\n> Patrick Steinhardt writes:\n> \n> > The function `has_sha1_pack_kept_or_nonlocal()` takes an object ID and\n> > then searches through packed objects to figure out whether the object\n> > exists in a kept or non-local pack. As a performance optimization we\n> > remember the packfile that contains a given object ID so that the next\n> > call to the function first checks that same packfile again.\n> >\n> > The way this is written is rather hard to follow though, as the caching\n> > mechanism is intertwined with the loop that iterates through the packs.\n> > Consequently, we need to do some gymnastics to re-start the iteration if\n> > the cached pack does not contain the objects.\n> \n> Okay, this took me while, but yes this function was really hard to\n> understand. Thanks for simplifying.\n> \n> Naive question, what's the point of keeping a \"last_found\"? We have one\n> global \"last_found\" for the last time this function was called, and we\n> have no control which OIDs get passed to this function. Why look into\n> \"last_found\" first?\n\nI guess it's just a micro-optimization. I'm sure it exists for a reason,\nbut honestly I didn't feel like opening that can of worms. The caching\njust made me scratch my head in subsequent refactorings, so I cared more\nabout making it maintainable than questioning its existence.\n\nPatrick\n"},{"id":"529929","messageId":"aQMo6DxZqYc6gEjQ@pks.im","threadId":"64398","inReplyTo":"aQKXpM8g3Oy3DVAa@nand.local","subject":"Re: [PATCH 2/8] packfile: move the MRU list into the packfile store","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-10-30T08:59:20Z","receivedAt":"2025-10-30T08:59:25Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Oct 29, 2025 at 06:39:32PM -0400, Taylor Blau wrote:\n> On Tue, Oct 28, 2025 at 12:08:32PM +0100, Patrick Steinhardt wrote:\n> > Packfiles have two lists associated to them:\n> >\n> > - A list that keeps track of packfiles in the order that they were\n> > added to a packfile store.\n> >\n> > - A list that keeps track of packfiles in most-recently-used order so\n> > that packfiles that are more likely to contain a specific object are\n> > ordered towards the front.\n> >\n> > Both of these lists are hosted by `struct packed_git` itself, So to\n> > identify all packfiles in a repository you simply need to grab the first\n> > packfile and then iterate the `->next` pointers or the MRU list. This\n> > pattern has the problem that all packfiles are part of the same list,\n> > regardless of whether or not they belong to the same object source.\n> >\n> > With the upcoming pluggable object database effort this needs to change:\n> > packfiles should be contained by a single object source, and reading an\n> > object from any such packfile should use that source to look up the\n> > object. Consequently, we need to break up the global lists of packfiles\n> \n> s/lists/list/\n\nIt's actually two: the MRU-ordered and the mtime-ordered one.\n\n> > into per-object-source lists.\n> \n> How does this work for alternates? My understanding is that each\n> alternate now has its own object source. So to perform an object lookup\n> in a repository with alternate(s), I am assuming that at some layer we\n> need to iterate over those sources to then enumerate the packs in that\n> source looking for some object.\n> \n> I would have imagined that packfile.c::find_pack_entry() would have to\n> be adjusted in a similar way as above, but I couldn't find the changes\n> in this series, so I feel like I must be missing something in my\n> understanding of how this all works together :-).\n> \n> Are packs from different sources still connected somehow such that\n> iterating over the list of packs from one source will enumerate the list\n> of packs from all sources?\n\nThis whole mechanism isn't yet part of this series :) So right now we\ndon't really change anything yet, and the list of packfiles is still a\nglobal list across all alternates. This series is thus basically only\npaving the path towards having per-alternate packfile stores.\n\nMoving the packfile store into the sources is going to be part of the\nnext series.\n\n> > A first step towards this goal is to move those lists ouf of `struct\n> \n> s/ouf/out/\n> \n> > packed_git` and into the packfile store. While the packfile store is\n> > currently sitting on the `struct object_database` level, the intent is\n> > to push it down one level into the `struct odb_source` in a subsequent\n> > patch series.\n> \n> Before sending, I was confused by \"Consequently, we need to break up the\n> global lists of packfiles [...]\", since it wasn't clear whether or not\n> this series realizes that goal, or pushes us in the direction towards\n> it.\n> \n> But this clarifies things, and I think is the reason that we do not see\n> more invasive changes like needing to enumerate the MRU cache of each\n> store in order to find an object like I mentioned above.\n\nYup, exactly.\n\n> > Introduce a new `struct packfile_list` that is used to manage lists of\n> > packfiles and use it to store the list of most-recently-used packfiles\n> > in `struct packfile_store`. For now, the new list type is only used in a\n> > single spot, but we'll expand its usage in subsequent patches.\n> \n> I am a little curious why we need a new list type and implementation\n> here. Is it to avoid exposing the list as part of struct packed_git like\n> we are forced to do with list_head?\n> \n> I could imagine that you might want to avoid exposing the \"struct\n> list_head mru\" part of packed_git to avoid the suggestion that all\n> packfiles (including those from different sources) are part of the same\n> list. But if that's the case, I wonder if we couldn't have kept the same\n> mru list and clarified via comment that it is per-store, not global.\n> \n> I suppose that is a bit of a foot-gun, and perhaps that is what you are\n> trying to do here, but after reading the patch message a few times I\n> wasn't clear on what the motivation for the new type was.\n\nYes, this is one of the reasons, it very much feels like a foot-gun to\nme. I found it significantly harder to work with the list embedded into\nthe packfiles themselves, and it made it significantly harder to check\nwhether the split really is done correctly. So by moving the list into\nthe packfile store it now becomes obvious in our code's layout, and it\nbecomes much easier to build an implicitly-correct mental model.\n\nThe second reason though is that packfiles aren't only used in the\ncontext of our ODB, but also by other layers like our transport. I want\nto have a clean split so that a packfile is a completely separate entity\nthat can exist without an object store. But if the list pointers used by\nthe store are embedded into the packfile itself, then that boundary gets\na lot more fuzzy.\n\nSo it's basically separation of concerns: `struct packed_git` should\nonly ever be concerned about a singular packfile. And the packfile store\nis then concerned with managing a set of packfiles.\n\nThanks!\n\nPatrick\n"},{"id":"529930","messageId":"87wm4cu462.fsf@iotcl.com","threadId":"64398","inReplyTo":"20251028-pks-packfiles-store-drop-list-v1-5-1a3b82030a7a@pks.im","subject":"Re: [PATCH 5/8] builtin/pack-objects: simplify logic to find kept or nonlocal objects","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2025-10-30T09:31:17Z","receivedAt":"2025-10-30T09:31:27Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"Patrick Steinhardt writes:\n\n> +\t\t/*\n> +\t\t * We have already checked `last_found`, so there is no need to\n> +\t\t * re-check here.\n> +\t\t */\n> +\t\tif (p == last_found && last_found != (void *)1)\n> +\t\t\tcontinue;\n\nUnrelated to the (void *)1 check, shouldn't this be in the beginning of\nthe loop?\n\n-- \nCheers,\nToon\n"},{"id":"529938","messageId":"aQM1TJF97kpTygBE@pks.im","threadId":"64398","inReplyTo":"87wm4cu462.fsf@iotcl.com","subject":"Re: [PATCH 5/8] builtin/pack-objects: simplify logic to find kept or nonlocal objects","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-10-30T09:52:12Z","receivedAt":"2025-10-30T09:52:18Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Oct 30, 2025 at 10:31:17AM +0100, Toon Claes wrote:\n> Patrick Steinhardt writes:\n> \n> > +\t\t/*\n> > +\t\t * We have already checked `last_found`, so there is no need to\n> > +\t\t * re-check here.\n> > +\t\t */\n> > +\t\tif (p == last_found && last_found != (void *)1)\n> > +\t\t\tcontinue;\n> \n> Unrelated to the (void *)1 check, shouldn't this be in the beginning of\n> the loop?\n\nOh, good catch. Yes, it of course should be, thanks!\n\nPatrick\n"},{"id":"529940","messageId":"20251030-pks-packfiles-store-drop-list-v2-0-84654f080cc0@pks.im","threadId":"64398","inReplyTo":"20251028-pks-packfiles-store-drop-list-v1-0-1a3b82030a7a@pks.im","subject":"[PATCH v2 0/8] packfiles: track pack lists via the packfile store","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-10-30T10:38:37Z","receivedAt":"2025-10-30T10:38:47Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nwhile the recently-introduced packfile store tracks the head of the pack\nlists, the actual lists themselves are still stored in a globally linked\nlist via the `struct packed_git::next` pointer. This makes it quite hard\nto split up that list into per-object-source lists, as the assumption is\nembedded in many places that one packfile will identify all the others.\n\nThis patch series thus moves the ownership of the lists into the\npackfile store. This prepares us for a subsequent change where we can\npush the packfile store one level down, from the object database into\nthe object source. So this is the second-last series before I'm done\nrefactoring the packfile subsystem.\n\nNote: I'd like to have some extra careful eyes on the last patch. This\npatch merges the two packfile lists we currently have (MRU and\nmtime-sorted). It is not needed to achieve my goal in this series, but\nthere was some discussion around whether we really need both lists. I\ndon't think we do, and in fact I think it causes confusion which of\nthese one should really use.\n\nThe default is to use the mtime-sorted list, which I think is the wrong\nchoice in many cases, but that is only by gut feeling. So I'm dropping\nthat list in favor of the MRU list, but there is one gotcha here: when\niterating through packfiles and then reading their respective objects,\nwe end up in an infinite loop because we end up moving the respective\npackfile to the front of the list again. I'm fixing that with a new\nfield that skips the MRU update, but I'm not quite sure wheter I think\nthat this is too fragile or not.\n\nThe series is built on top of 419c72cb8a (Sync with Git 2.51.2,\n2025-10-26) with ps/remove-packfile-store-get-packs at ecad863c12\n(packfile: rename `packfile_store_get_all_packs()`, 2025-10-09) merged\ninto it.\n\nChanges in v2:\n - A couple of commit message typo fixes.\n - Avoid opening the pack index in `repo_approximate_object_count()` in\n case we don't want to access the packfile in the first place.\n - Further simplifications for `has_sha1_pack_kept_or_nonlocal()`.\n Also, fix how we skip over the last-found pack.\n - Completely reword the motivation why we unconditionally start to add\n packfiles to the MRU list.\n - Link to v1: https://lore.kernel.org/r/20251028-pks-packfiles-store-drop-list-v1-0-1a3b82030a7a@pks.im\n\nThanks!\n\nPatrick\n\n---\nPatrick Steinhardt (8):\n packfile: use a `strmap` to store packs by name\n packfile: move the MRU list into the packfile store\n http: refactor subsystem to use `packfile_list`s\n packfile: fix approximation of object counts\n builtin/pack-objects: simplify logic to find kept or nonlocal objects\n packfile: move list of packs into the packfile store\n packfile: always add packfiles to MRU when adding a pack\n packfile: track packs via the MRU list exclusively\n\n builtin/fast-import.c | 4 +-\n builtin/pack-objects.c | 37 ++++----\n http-push.c | 6 +-\n http-walker.c | 26 ++----\n http.c | 21 ++---\n http.h | 5 +-\n midx.c | 2 -\n packfile.c | 224 +++++++++++++++++++++++++++++--------------------\n packfile.h | 70 ++++++++++------\n 9 files changed, 223 insertions(+), 172 deletions(-)\n\nRange-diff versus v1:\n\n1: 49bc9f8c9aa = 1: 56660c77d40 packfile: use a `strmap` to store packs by name\n2: 4cea16b704e ! 2: d2e003b44ca packfile: move the MRU list into the packfile store\n @@ Commit message\n object. Consequently, we need to break up the global lists of packfiles\n into per-object-source lists.\n \n - A first step towards this goal is to move those lists ouf of `struct\n + A first step towards this goal is to move those lists out of `struct\n packed_git` and into the packfile store. While the packfile store is\n currently sitting on the `struct object_database` level, the intent is\n to push it down one level into the `struct odb_source` in a subsequent\n3: 140fc5add46 = 3: 9523423446d http: refactor subsystem to use `packfile_list`s\n4: 3a0a29e80de ! 4: 3be216ddfb5 packfile: fix approximation of object counts\n @@ packfile.c: unsigned long repo_approximate_object_count(struct repository *r)\n -\t\tfor (p = r->objects->packfiles->packs; p; p = p->next) {\n -\t\t\tif (open_pack_index(p))\n +\t\trepo_for_each_pack(r, p) {\n -+\t\t\tif (open_pack_index(p) || p->multi_pack_index)\n ++\t\t\tif (p->multi_pack_index || open_pack_index(p))\n \t\t\t\tcontinue;\n \t\t\tcount += p->num_objects;\n \t\t}\n5: 324d3d29234 ! 5: 867c1d5315a builtin/pack-objects: simplify logic to find kept or nonlocal objects\n @@ Commit message\n check whether the pack contains the object ID, and to skip the cached\n pack in the loop so that we don't search it twice.\n \n + Furthermore, stop using the `(void *)1` sentinel value and instead use a\n + simple `NULL` pointer to indicate that we don't have a last-found pack\n + yet.\n +\n This refactoring significantly simplifies the logic and makes it much\n easier to follow.\n \n @@ builtin/pack-objects.c: static void add_unreachable_loose_objects(struct rev_inf\n static int has_sha1_pack_kept_or_nonlocal(const struct object_id *oid)\n {\n -\tstruct packfile_store *packs = the_repository->objects->packfiles;\n - \tstatic struct packed_git *last_found = (void *)1;\n +-\tstatic struct packed_git *last_found = (void *)1;\n ++\tstatic struct packed_git *last_found = NULL;\n \tstruct packed_git *p;\n \n -\tp = (last_found != (void *)1) ? last_found :\n -\t\t\t\t\tpackfile_store_get_packs(packs);\n -+\tif (last_found != (void *)1 && find_pack_entry_one(oid, last_found))\n ++\tif (last_found && find_pack_entry_one(oid, last_found))\n +\t\treturn 1;\n \n -\twhile (p) {\n @@ builtin/pack-objects.c: static void add_unreachable_loose_objects(struct rev_inf\n -\t\t\t\tp->pack_keep_in_core) &&\n -\t\t\tfind_pack_entry_one(oid, p)) {\n +\trepo_for_each_pack(the_repository, p) {\n ++\t\t/*\n ++\t\t * We have already checked `last_found`, so there is no need to\n ++\t\t * re-check here.\n ++\t\t */\n ++\t\tif (p == last_found)\n ++\t\t\tcontinue;\n ++\n +\t\tif ((!p->pack_local || p->pack_keep || p->pack_keep_in_core) &&\n +\t\t find_pack_entry_one(oid, p)) {\n \t\t\tlast_found = p;\n @@ builtin/pack-objects.c: static void add_unreachable_loose_objects(struct rev_inf\n -\t\t\tp = p->next;\n -\t\tif (p == last_found)\n -\t\t\tp = p->next;\n -+\n -+\t\t/*\n -+\t\t * We have already checked `last_found`, so there is no need to\n -+\t\t * re-check here.\n -+\t\t */\n -+\t\tif (p == last_found && last_found != (void *)1)\n -+\t\t\tcontinue;\n \t}\n +\n \treturn 0;\n6: 92c7d5ab273 = 6: 21dd33b22ef packfile: move list of packs into the packfile store\n7: df86cc9f650 ! 7: 1bf0880cce8 packfile: always add packfiles to MRU when adding a pack\n @@ Metadata\n ## Commit message ##\n packfile: always add packfiles to MRU when adding a pack\n \n - When adding a packfile to it store we add it both to the list and map of\n - packfiles, but we don't append it to the most-recently-used list of\n - packs. We do know to add the packfile to the MRU list as soon as we\n - access any of its objects, but in between we're being inconistent. It\n - doesn't help that there are some subsystems that _do_ add the packfile\n - to the MRU after having added it, which only adds to the confusion.\n + When preparing the packfile store we know to also prepare the MRU list\n + of packfiles with all packs that are currently loaded in the store via\n + `packfile_store_prepare_mru()`. So we know that the list of packs in the\n + MRU list should match the list of packs in the non-MRU list.\n +\n + But there are some direct or indirect callsites that add a packfile to\n + the store via `packfile_store_add_pack()` without adding the pack to the\n + MRU. And while functions that access the MRU (e.g. `find_pack_entry()`)\n + know to call `packfile_store_prepare()`, which knows to prepare the MRU\n + via `packfile_store_prepare_mru()`, that operation will be turned into a\n + no-op because the packfile store is already prepared. So this will not\n + cause us to add the packfile to the MRU, and consequently we won't be\n + able to find the packfile in our MRU list.\n +\n + There are only a handful of callers outside of \"packfile.c\" that add a\n + packfile to the store:\n +\n + - \"builtin/fast-import.c\" adds multiple packs of imported objects, but\n + it knows to look up objects via `packfile_store_get_packs()`. This\n + function does not use the MRU, so we're good.\n +\n + - \"builtin/index-pack.c\" adds the indexed pack to the store in case it\n + needs to perform consistency checks on its objects.\n +\n + - \"http.c\" adds the fetched pack to the store so that we can access\n + its objects.\n +\n + In all of these cases we actually want to access the contained objects.\n + And luckily, reading these objects works as expected:\n +\n + 1. We eventually end up in `do_oid_object_info_extended()`.\n +\n + 2. Calling `find_pack_entry()` fails because the MRU list doesn't\n + contain the newly added packfile.\n +\n + 3. The callers don't pass `OBJECT_INFO_QUICK`, so we end up\n + repreparing the object database. This will also cause us to\n + reprepare the MRU list.\n +\n + 4. We now retry reading the object via `find_pack_entry()`, and now we\n + succeed because the MRU list got populated.\n +\n + This logic feels quite fragile: we intentionally add the packfile to the\n + store, but we then ultimately rely on repreparing the entire store only\n + to make the packfile accessible. While we do the correct thing in\n + `do_oid_object_info_extended()`, other sites that access the MRU may not\n + know to reprepare.\n +\n + But besides being fragile it's also a waste of resources: repreparing\n + the object database requires us to re-read the alternates file and\n + discard any caches.\n \n Refactor the code so that we unconditionally add packfiles to the MRU\n - when adding them to a packfile store.\n + when adding them to a packfile store. This makes the logic less fragile\n + and ensures that we don't have to reprepare the store to make the pack\n + accessible.\n \n Note that this does not allow us to drop `packfile_store_prepare_mru()`\n just yet: while the MRU list is already populated with all packs now,\n8: cc9d35a4b09 = 8: 64571e61fac packfile: track packs via the MRU list exclusively\n\n---\nbase-commit: cad6ef1d7514e7450c04c2fe624a55b28d99ac88\nchange-id: 20251010-pks-packfiles-store-drop-list-64ea0a4c9a3b\n\n"},{"id":"529941","messageId":"20251030-pks-packfiles-store-drop-list-v2-1-84654f080cc0@pks.im","threadId":"64398","inReplyTo":"20251030-pks-packfiles-store-drop-list-v2-0-84654f080cc0@pks.im","subject":"[PATCH v2 1/8] packfile: use a `strmap` to store packs by name","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-10-30T10:38:38Z","receivedAt":"2025-10-30T10:38:49Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"To allow fast lookups of a packfile by name we use a hashmap that has\nthe packfile name as key and the pack itself as value. But while this is\nthe perfect use case for a `strmap`, we instead use `struct hashmap` and\nstore the hashmap entry in the packfile itself.\n\nSimplify the code by using a `strmap` instead.\n\nSigned-off-by: Patrick Steinhardt \n---\n packfile.c | 24 ++++--------------------\n packfile.h | 4 ++--\n 2 files changed, 6 insertions(+), 22 deletions(-)\n\ndiff --git a/packfile.c b/packfile.c\nindex 1ae2b2fe1ed..04649e52920 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -788,8 +788,7 @@ void packfile_store_add_pack(struct packfile_store *store,\n \tpack->next = store->packs;\n \tstore->packs = pack;\n \n-\thashmap_entry_init(&pack->packmap_ent, strhash(pack->pack_name));\n-\thashmap_add(&store->map, &pack->packmap_ent);\n+\tstrmap_put(&store->packs_by_path, pack->pack_name, pack);\n }\n \n struct packed_git *packfile_store_load_pack(struct packfile_store *store,\n@@ -806,8 +805,7 @@ struct packed_git *packfile_store_load_pack(struct packfile_store *store,\n \tstrbuf_strip_suffix(&key, \".idx\");\n \tstrbuf_addstr(&key, \".pack\");\n \n-\tp = hashmap_get_entry_from_hash(&store->map, strhash(key.buf), key.buf,\n-\t\t\t\t\tstruct packed_git, packmap_ent);\n+\tp = strmap_get(&store->packs_by_path, key.buf);\n \tif (!p) {\n \t\tp = add_packed_git(store->odb->repo, idx_path,\n \t\t\t\t strlen(idx_path), local);\n@@ -2311,27 +2309,13 @@ int parse_pack_header_option(const char *in, unsigned char *out, unsigned int *l\n \treturn 0;\n }\n \n-static int pack_map_entry_cmp(const void *cmp_data UNUSED,\n-\t\t\t const struct hashmap_entry *entry,\n-\t\t\t const struct hashmap_entry *entry2,\n-\t\t\t const void *keydata)\n-{\n-\tconst char *key = keydata;\n-\tconst struct packed_git *pg1, *pg2;\n-\n-\tpg1 = container_of(entry, const struct packed_git, packmap_ent);\n-\tpg2 = container_of(entry2, const struct packed_git, packmap_ent);\n-\n-\treturn strcmp(pg1->pack_name, key ? key : pg2->pack_name);\n-}\n-\n struct packfile_store *packfile_store_new(struct object_database *odb)\n {\n \tstruct packfile_store *store;\n \tCALLOC_ARRAY(store, 1);\n \tstore->odb = odb;\n \tINIT_LIST_HEAD(&store->mru);\n-\thashmap_init(&store->map, pack_map_entry_cmp, NULL, 0);\n+\tstrmap_init(&store->packs_by_path);\n \treturn store;\n }\n \n@@ -2341,7 +2325,7 @@ void packfile_store_free(struct packfile_store *store)\n \t\tnext = p->next;\n \t\tfree(p);\n \t}\n-\thashmap_clear(&store->map);\n+\tstrmap_clear(&store->packs_by_path, 0);\n \tfree(store);\n }\n \ndiff --git a/packfile.h b/packfile.h\nindex c9d0b93446b..9da7f14317b 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -5,12 +5,12 @@\n #include \"object.h\"\n #include \"odb.h\"\n #include \"oidset.h\"\n+#include \"strmap.h\"\n \n /* in odb.h */\n struct object_info;\n \n struct packed_git {\n-\tstruct hashmap_entry packmap_ent;\n \tstruct packed_git *next;\n \tstruct list_head mru;\n \tstruct pack_window *windows;\n@@ -85,7 +85,7 @@ struct packfile_store {\n \t * A map of packfile names to packed_git structs for tracking which\n \t * packs have been loaded already.\n \t */\n-\tstruct hashmap map;\n+\tstruct strmap packs_by_path;\n \n \t/*\n \t * Whether packfiles have already been populated with this store's\n\n-- \n2.51.2.997.g839fc31de9.dirty\n\n"},{"id":"529942","messageId":"20251030-pks-packfiles-store-drop-list-v2-2-84654f080cc0@pks.im","threadId":"64398","inReplyTo":"20251030-pks-packfiles-store-drop-list-v2-0-84654f080cc0@pks.im","subject":"[PATCH v2 2/8] packfile: move the MRU list into the packfile store","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-10-30T10:38:39Z","receivedAt":"2025-10-30T10:38:52Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Packfiles have two lists associated to them:\n\n - A list that keeps track of packfiles in the order that they were\n added to a packfile store.\n\n - A list that keeps track of packfiles in most-recently-used order so\n that packfiles that are more likely to contain a specific object are\n ordered towards the front.\n\nBoth of these lists are hosted by `struct packed_git` itself, So to\nidentify all packfiles in a repository you simply need to grab the first\npackfile and then iterate the `->next` pointers or the MRU list. This\npattern has the problem that all packfiles are part of the same list,\nregardless of whether or not they belong to the same object source.\n\nWith the upcoming pluggable object database effort this needs to change:\npackfiles should be contained by a single object source, and reading an\nobject from any such packfile should use that source to look up the\nobject. Consequently, we need to break up the global lists of packfiles\ninto per-object-source lists.\n\nA first step towards this goal is to move those lists out of `struct\npacked_git` and into the packfile store. While the packfile store is\ncurrently sitting on the `struct object_database` level, the intent is\nto push it down one level into the `struct odb_source` in a subsequent\npatch series.\n\nIntroduce a new `struct packfile_list` that is used to manage lists of\npackfiles and use it to store the list of most-recently-used packfiles\nin `struct packfile_store`. For now, the new list type is only used in a\nsingle spot, but we'll expand its usage in subsequent patches.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/pack-objects.c | 9 +++--\n midx.c | 2 +-\n packfile.c | 92 +++++++++++++++++++++++++++++++++++++++++++++-----\n packfile.h | 19 +++++++++--\n 4 files changed, 104 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex b5454e5df13..5348aebbe9f 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -1706,8 +1706,8 @@ static int want_object_in_pack_mtime(const struct object_id *oid,\n \t\t\t\t uint32_t found_mtime)\n {\n \tint want;\n+\tstruct packfile_list_entry *e;\n \tstruct odb_source *source;\n-\tstruct list_head *pos;\n \n \tif (!exclude && local) {\n \t\t/*\n@@ -1748,12 +1748,11 @@ static int want_object_in_pack_mtime(const struct object_id *oid,\n \t\t}\n \t}\n \n-\tlist_for_each(pos, packfile_store_get_packs_mru(the_repository->objects->packfiles)) {\n-\t\tstruct packed_git *p = list_entry(pos, struct packed_git, mru);\n+\tfor (e = the_repository->objects->packfiles->mru.head; e; e = e->next) {\n+\t\tstruct packed_git *p = e->pack;\n \t\twant = want_object_in_pack_one(p, oid, exclude, found_pack, found_offset, found_mtime);\n \t\tif (!exclude && want > 0)\n-\t\t\tlist_move(&p->mru,\n-\t\t\t\t packfile_store_get_packs_mru(the_repository->objects->packfiles));\n+\t\t\tpackfile_list_prepend(&the_repository->objects->packfiles->mru, p);\n \t\tif (want != -1)\n \t\t\treturn want;\n \t}\ndiff --git a/midx.c b/midx.c\nindex 1d6269f957e..8022be9a45e 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -463,7 +463,7 @@ int prepare_midx_pack(struct multi_pack_index *m,\n \tp = packfile_store_load_pack(r->objects->packfiles,\n \t\t\t\t pack_name.buf, m->source->local);\n \tif (p)\n-\t\tlist_add_tail(&p->mru, &r->objects->packfiles->mru);\n+\t\tpackfile_list_append(&m->source->odb->packfiles->mru, p);\n \tstrbuf_release(&pack_name);\n \n \tif (!p) {\ndiff --git a/packfile.c b/packfile.c\nindex 04649e52920..4d2d3b674f3 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -47,6 +47,80 @@ static size_t pack_mapped;\n #define SZ_FMT PRIuMAX\n static inline uintmax_t sz_fmt(size_t s) { return s; }\n \n+void packfile_list_clear(struct packfile_list *list)\n+{\n+\tstruct packfile_list_entry *e, *next;\n+\n+\tfor (e = list->head; e; e = next) {\n+\t\tnext = e->next;\n+\t\tfree(e);\n+\t}\n+\n+\tlist->head = list->tail = NULL;\n+}\n+\n+static struct packfile_list_entry *packfile_list_remove_internal(struct packfile_list *list,\n+\t\t\t\t\t\t\t\t struct packed_git *pack)\n+{\n+\tstruct packfile_list_entry *e, *prev;\n+\n+\tfor (e = list->head, prev = NULL; e; prev = e, e = e->next) {\n+\t\tif (e->pack != pack)\n+\t\t\tcontinue;\n+\n+\t\tif (prev)\n+\t\t\tprev->next = e->next;\n+\t\tif (list->head == e)\n+\t\t\tlist->head = e->next;\n+\t\tif (list->tail == e)\n+\t\t\tlist->tail = prev;\n+\n+\t\treturn e;\n+\t}\n+\n+\treturn NULL;\n+}\n+\n+void packfile_list_remove(struct packfile_list *list, struct packed_git *pack)\n+{\n+\tfree(packfile_list_remove_internal(list, pack));\n+}\n+\n+void packfile_list_prepend(struct packfile_list *list, struct packed_git *pack)\n+{\n+\tstruct packfile_list_entry *entry;\n+\n+\tentry = packfile_list_remove_internal(list, pack);\n+\tif (!entry) {\n+\t\tentry = xmalloc(sizeof(*entry));\n+\t\tentry->pack = pack;\n+\t}\n+\tentry->next = list->head;\n+\n+\tlist->head = entry;\n+\tif (!list->tail)\n+\t\tlist->tail = entry;\n+}\n+\n+void packfile_list_append(struct packfile_list *list, struct packed_git *pack)\n+{\n+\tstruct packfile_list_entry *entry;\n+\n+\tentry = packfile_list_remove_internal(list, pack);\n+\tif (!entry) {\n+\t\tentry = xmalloc(sizeof(*entry));\n+\t\tentry->pack = pack;\n+\t}\n+\tentry->next = NULL;\n+\n+\tif (list->tail) {\n+\t\tlist->tail->next = entry;\n+\t\tlist->tail = entry;\n+\t} else {\n+\t\tlist->head = list->tail = entry;\n+\t}\n+}\n+\n void pack_report(struct repository *repo)\n {\n \tfprintf(stderr,\n@@ -995,10 +1069,10 @@ static void packfile_store_prepare_mru(struct packfile_store *store)\n {\n \tstruct packed_git *p;\n \n-\tINIT_LIST_HEAD(&store->mru);\n+\tpackfile_list_clear(&store->mru);\n \n \tfor (p = store->packs; p; p = p->next)\n-\t\tlist_add_tail(&p->mru, &store->mru);\n+\t\tpackfile_list_append(&store->mru, p);\n }\n \n void packfile_store_prepare(struct packfile_store *store)\n@@ -1040,10 +1114,10 @@ struct packed_git *packfile_store_get_packs(struct packfile_store *store)\n \treturn store->packs;\n }\n \n-struct list_head *packfile_store_get_packs_mru(struct packfile_store *store)\n+struct packfile_list_entry *packfile_store_get_packs_mru(struct packfile_store *store)\n {\n \tpackfile_store_prepare(store);\n-\treturn &store->mru;\n+\treturn store->mru.head;\n }\n \n /*\n@@ -2048,7 +2122,7 @@ static int fill_pack_entry(const struct object_id *oid,\n \n int find_pack_entry(struct repository *r, const struct object_id *oid, struct pack_entry *e)\n {\n-\tstruct list_head *pos;\n+\tstruct packfile_list_entry *l;\n \n \tpackfile_store_prepare(r->objects->packfiles);\n \n@@ -2059,10 +2133,11 @@ int find_pack_entry(struct repository *r, const struct object_id *oid, struct pa\n \tif (!r->objects->packfiles->packs)\n \t\treturn 0;\n \n-\tlist_for_each(pos, &r->objects->packfiles->mru) {\n-\t\tstruct packed_git *p = list_entry(pos, struct packed_git, mru);\n+\tfor (l = r->objects->packfiles->mru.head; l; l = l->next) {\n+\t\tstruct packed_git *p = l->pack;\n+\n \t\tif (!p->multi_pack_index && fill_pack_entry(oid, e, p)) {\n-\t\t\tlist_move(&p->mru, &r->objects->packfiles->mru);\n+\t\t\tpackfile_list_prepend(&r->objects->packfiles->mru, p);\n \t\t\treturn 1;\n \t\t}\n \t}\n@@ -2314,7 +2389,6 @@ struct packfile_store *packfile_store_new(struct object_database *odb)\n \tstruct packfile_store *store;\n \tCALLOC_ARRAY(store, 1);\n \tstore->odb = odb;\n-\tINIT_LIST_HEAD(&store->mru);\n \tstrmap_init(&store->packs_by_path);\n \treturn store;\n }\ndiff --git a/packfile.h b/packfile.h\nindex 9da7f14317b..39ed1073e4a 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -12,7 +12,6 @@ struct object_info;\n \n struct packed_git {\n \tstruct packed_git *next;\n-\tstruct list_head mru;\n \tstruct pack_window *windows;\n \toff_t pack_size;\n \tconst void *index_data;\n@@ -52,6 +51,20 @@ struct packed_git {\n \tchar pack_name[FLEX_ARRAY]; /* more */\n };\n \n+struct packfile_list {\n+\tstruct packfile_list_entry *head, *tail;\n+};\n+\n+struct packfile_list_entry {\n+\tstruct packfile_list_entry *next;\n+\tstruct packed_git *pack;\n+};\n+\n+void packfile_list_clear(struct packfile_list *list);\n+void packfile_list_remove(struct packfile_list *list, struct packed_git *pack);\n+void packfile_list_prepend(struct packfile_list *list, struct packed_git *pack);\n+void packfile_list_append(struct packfile_list *list, struct packed_git *pack);\n+\n /*\n * A store that manages packfiles for a given object database.\n */\n@@ -79,7 +92,7 @@ struct packfile_store {\n \t} kept_cache;\n \n \t/* A most-recently-used ordered version of the packs list. */\n-\tstruct list_head mru;\n+\tstruct packfile_list mru;\n \n \t/*\n \t * A map of packfile names to packed_git structs for tracking which\n@@ -153,7 +166,7 @@ struct packed_git *packfile_store_get_packs(struct packfile_store *store);\n /*\n * Get all packs in most-recently-used order.\n */\n-struct list_head *packfile_store_get_packs_mru(struct packfile_store *store);\n+struct packfile_list_entry *packfile_store_get_packs_mru(struct packfile_store *store);\n \n /*\n * Open the packfile and add it to the store if it isn't yet known. Returns\n\n-- \n2.51.2.997.g839fc31de9.dirty\n\n"},{"id":"529943","messageId":"20251030-pks-packfiles-store-drop-list-v2-3-84654f080cc0@pks.im","threadId":"64398","inReplyTo":"20251030-pks-packfiles-store-drop-list-v2-0-84654f080cc0@pks.im","subject":"[PATCH v2 3/8] http: refactor subsystem to use `packfile_list`s","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-10-30T10:38:40Z","receivedAt":"2025-10-30T10:38:55Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The dumb HTTP protocol directly fetches packfiles from the remote server\nand temporarily stores them in a list of packfiles. Those packfiles are\nnot yet added to the repository's packfile store until we finalize the\nwhole fetch.\n\nRefactor the code to instead use a `struct packfile_list` to store those\npacks. This prepares us for a subsequent change where the `->next`\npointer of `struct packed_git` will go away.\n\nNote that this refactoring creates some temporary duplication of code,\nas we now have both `packfile_list_find_oid()` and `find_oid_pack()`.\nThe latter function will be removed in a subsequent commit though.\n\nSigned-off-by: Patrick Steinhardt \n---\n http-push.c | 6 +++---\n http-walker.c | 26 +++++++++-----------------\n http.c | 21 ++++++++-------------\n http.h | 5 +++--\n packfile.c | 9 +++++++++\n packfile.h | 8 ++++++++\n 6 files changed, 40 insertions(+), 35 deletions(-)\n\ndiff --git a/http-push.c b/http-push.c\nindex a1c01e3b9b9..d86ce771198 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -104,7 +104,7 @@ struct repo {\n \tint has_info_refs;\n \tint can_update_info_refs;\n \tint has_info_packs;\n-\tstruct packed_git *packs;\n+\tstruct packfile_list packs;\n \tstruct remote_lock *locks;\n };\n \n@@ -311,7 +311,7 @@ static void start_fetch_packed(struct transfer_request *request)\n \tstruct transfer_request *check_request = request_queue_head;\n \tstruct http_pack_request *preq;\n \n-\ttarget = find_oid_pack(&request->obj->oid, repo->packs);\n+\ttarget = packfile_list_find_oid(repo->packs.head, &request->obj->oid);\n \tif (!target) {\n \t\tfprintf(stderr, \"Unable to fetch %s, will not be able to update server info refs\\n\", oid_to_hex(&request->obj->oid));\n \t\trepo->can_update_info_refs = 0;\n@@ -683,7 +683,7 @@ static int add_send_request(struct object *obj, struct remote_lock *lock)\n \t\tget_remote_object_list(obj->oid.hash[0]);\n \tif (obj->flags & (REMOTE | PUSHING))\n \t\treturn 0;\n-\ttarget = find_oid_pack(&obj->oid, repo->packs);\n+\ttarget = packfile_list_find_oid(repo->packs.head, &obj->oid);\n \tif (target) {\n \t\tobj->flags |= REMOTE;\n \t\treturn 0;\ndiff --git a/http-walker.c b/http-walker.c\nindex 0f7ae46d7f1..e886e648664 100644\n--- a/http-walker.c\n+++ b/http-walker.c\n@@ -15,7 +15,7 @@\n struct alt_base {\n \tchar *base;\n \tint got_indices;\n-\tstruct packed_git *packs;\n+\tstruct packfile_list packs;\n \tstruct alt_base *next;\n };\n \n@@ -324,11 +324,8 @@ static void process_alternates_response(void *callback_data)\n \t\t\t\t} else if (is_alternate_allowed(target.buf)) {\n \t\t\t\t\twarning(\"adding alternate object store: %s\",\n \t\t\t\t\t\ttarget.buf);\n-\t\t\t\t\tnewalt = xmalloc(sizeof(*newalt));\n-\t\t\t\t\tnewalt->next = NULL;\n+\t\t\t\t\tCALLOC_ARRAY(newalt, 1);\n \t\t\t\t\tnewalt->base = strbuf_detach(&target, NULL);\n-\t\t\t\t\tnewalt->got_indices = 0;\n-\t\t\t\t\tnewalt->packs = NULL;\n \n \t\t\t\t\twhile (tail->next != NULL)\n \t\t\t\t\t\ttail = tail->next;\n@@ -435,7 +432,7 @@ static int http_fetch_pack(struct walker *walker, struct alt_base *repo,\n \n \tif (fetch_indices(walker, repo))\n \t\treturn -1;\n-\ttarget = find_oid_pack(oid, repo->packs);\n+\ttarget = packfile_list_find_oid(repo->packs.head, oid);\n \tif (!target)\n \t\treturn -1;\n \tclose_pack_index(target);\n@@ -584,17 +581,15 @@ static void cleanup(struct walker *walker)\n \tif (data) {\n \t\talt = data->alt;\n \t\twhile (alt) {\n-\t\t\tstruct packed_git *pack;\n+\t\t\tstruct packfile_list_entry *e;\n \n \t\t\talt_next = alt->next;\n \n-\t\t\tpack = alt->packs;\n-\t\t\twhile (pack) {\n-\t\t\t\tstruct packed_git *pack_next = pack->next;\n-\t\t\t\tclose_pack(pack);\n-\t\t\t\tfree(pack);\n-\t\t\t\tpack = pack_next;\n+\t\t\tfor (e = alt->packs.head; e; e = e->next) {\n+\t\t\t\tclose_pack(e->pack);\n+\t\t\t\tfree(e->pack);\n \t\t\t}\n+\t\t\tpackfile_list_clear(&alt->packs);\n \n \t\t\tfree(alt->base);\n \t\t\tfree(alt);\n@@ -612,14 +607,11 @@ struct walker *get_http_walker(const char *url)\n \tstruct walker_data *data = xmalloc(sizeof(struct walker_data));\n \tstruct walker *walker = xmalloc(sizeof(struct walker));\n \n-\tdata->alt = xmalloc(sizeof(*data->alt));\n+\tCALLOC_ARRAY(data->alt, 1);\n \tdata->alt->base = xstrdup(url);\n \tfor (s = data->alt->base + strlen(data->alt->base) - 1; *s == '/'; --s)\n \t\t*s = 0;\n \n-\tdata->alt->got_indices = 0;\n-\tdata->alt->packs = NULL;\n-\tdata->alt->next = NULL;\n \tdata->got_alternates = -1;\n \n \twalker->corrupt_object_found = 0;\ndiff --git a/http.c b/http.c\nindex 17130823f00..41f850db16d 100644\n--- a/http.c\n+++ b/http.c\n@@ -2413,8 +2413,9 @@ static char *fetch_pack_index(unsigned char *hash, const char *base_url)\n \treturn tmp;\n }\n \n-static int fetch_and_setup_pack_index(struct packed_git **packs_head,\n-\tunsigned char *sha1, const char *base_url)\n+static int fetch_and_setup_pack_index(struct packfile_list *packs,\n+\t\t\t\t unsigned char *sha1,\n+\t\t\t\t const char *base_url)\n {\n \tstruct packed_git *new_pack, *p;\n \tchar *tmp_idx = NULL;\n@@ -2448,12 +2449,11 @@ static int fetch_and_setup_pack_index(struct packed_git **packs_head,\n \tif (ret)\n \t\treturn -1;\n \n-\tnew_pack->next = *packs_head;\n-\t*packs_head = new_pack;\n+\tpackfile_list_prepend(packs, new_pack);\n \treturn 0;\n }\n \n-int http_get_info_packs(const char *base_url, struct packed_git **packs_head)\n+int http_get_info_packs(const char *base_url, struct packfile_list *packs)\n {\n \tstruct http_get_options options = {0};\n \tint ret = 0;\n@@ -2477,7 +2477,7 @@ int http_get_info_packs(const char *base_url, struct packed_git **packs_head)\n \t\t !parse_oid_hex(data, &oid, &data) &&\n \t\t skip_prefix(data, \".pack\", &data) &&\n \t\t (*data == '\\n' || *data == '\\0')) {\n-\t\t\tfetch_and_setup_pack_index(packs_head, oid.hash, base_url);\n+\t\t\tfetch_and_setup_pack_index(packs, oid.hash, base_url);\n \t\t} else {\n \t\t\tdata = strchrnul(data, '\\n');\n \t\t}\n@@ -2541,14 +2541,9 @@ int finish_http_pack_request(struct http_pack_request *preq)\n }\n \n void http_install_packfile(struct packed_git *p,\n-\t\t\t struct packed_git **list_to_remove_from)\n+\t\t\t struct packfile_list *list_to_remove_from)\n {\n-\tstruct packed_git **lst = list_to_remove_from;\n-\n-\twhile (*lst != p)\n-\t\tlst = &((*lst)->next);\n-\t*lst = (*lst)->next;\n-\n+\tpackfile_list_remove(list_to_remove_from, p);\n \tpackfile_store_add_pack(the_repository->objects->packfiles, p);\n }\n \ndiff --git a/http.h b/http.h\nindex 553e16205ce..f9d45934047 100644\n--- a/http.h\n+++ b/http.h\n@@ -2,6 +2,7 @@\n #define HTTP_H\n \n struct packed_git;\n+struct packfile_list;\n \n #include \"git-zlib.h\"\n \n@@ -190,7 +191,7 @@ struct curl_slist *http_append_auth_header(const struct credential *c,\n \n /* Helpers for fetching packs */\n int http_get_info_packs(const char *base_url,\n-\t\t\tstruct packed_git **packs_head);\n+\t\t\tstruct packfile_list *packs);\n \n /* Helper for getting Accept-Language header */\n const char *http_get_accept_language_header(void);\n@@ -226,7 +227,7 @@ void release_http_pack_request(struct http_pack_request *preq);\n * from http_get_info_packs() and have chosen a specific pack to fetch.\n */\n void http_install_packfile(struct packed_git *p,\n-\t\t\t struct packed_git **list_to_remove_from);\n+\t\t\t struct packfile_list *list_to_remove_from);\n \n /* Helpers for fetching object */\n struct http_object_request {\ndiff --git a/packfile.c b/packfile.c\nindex 4d2d3b674f3..6aa2ca8ac9e 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -121,6 +121,15 @@ void packfile_list_append(struct packfile_list *list, struct packed_git *pack)\n \t}\n }\n \n+struct packed_git *packfile_list_find_oid(struct packfile_list_entry *packs,\n+\t\t\t\t\t const struct object_id *oid)\n+{\n+\tfor (; packs; packs = packs->next)\n+\t\tif (find_pack_entry_one(oid, packs->pack))\n+\t\t\treturn packs->pack;\n+\treturn NULL;\n+}\n+\n void pack_report(struct repository *repo)\n {\n \tfprintf(stderr,\ndiff --git a/packfile.h b/packfile.h\nindex 39ed1073e4a..a53336d722a 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -65,6 +65,14 @@ void packfile_list_remove(struct packfile_list *list, struct packed_git *pack);\n void packfile_list_prepend(struct packfile_list *list, struct packed_git *pack);\n void packfile_list_append(struct packfile_list *list, struct packed_git *pack);\n \n+/*\n+ * Find the pack within the \"packs\" list whose index contains the object\n+ * \"oid\". For general object lookups, you probably don't want this; use\n+ * find_pack_entry() instead.\n+ */\n+struct packed_git *packfile_list_find_oid(struct packfile_list_entry *packs,\n+\t\t\t\t\t const struct object_id *oid);\n+\n /*\n * A store that manages packfiles for a given object database.\n */\n\n-- \n2.51.2.997.g839fc31de9.dirty\n\n"},{"id":"529944","messageId":"20251030-pks-packfiles-store-drop-list-v2-4-84654f080cc0@pks.im","threadId":"64398","inReplyTo":"20251030-pks-packfiles-store-drop-list-v2-0-84654f080cc0@pks.im","subject":"[PATCH v2 4/8] packfile: fix approximation of object counts","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-10-30T10:38:41Z","receivedAt":"2025-10-30T10:38:58Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When approximating the number of objects in a repository we only take\ninto account two data sources, the multi-pack index and the packfile\nindices, as both of these data structures allow us to easily figure out\nhow many objects they contain.\n\nBut the way we currently approximate the number of objects is broken in\npresence of a multi-pack index. This is due to two separate reasons:\n\n - We have recently introduced initial infrastructure for incremental\n multi-pack indices. Starting with that series, `num_objects` only\n counts the number of objects of a specific layer of the MIDX chain,\n so we do not take into account objects from parent layers.\n\n This issue is fixed by adding `num_objects_in_base`, which contains\n the sum of all objects in previous layers.\n\n - When using the multi-pack index we may count objects contained in\n packfiles twice: once via the multi-pack index, but then we again\n count them via the packfile itself.\n\n This issue is fixed by skipping any packfiles that have an MIDX.\n\nOverall, given that we _always_ count the packs, we can only end up\noverestimating the number of objects, and the overestimation is limited\nto a factor of two at most.\n\nThe consequences of those issues are very limited though, as we only\napproximate object counts in a small number of cases:\n\n - When writing a commit-graph we use the approximate object count to\n display the upper limit of a progress display.\n\n - In `repo_find_unique_abbrev_r()` we use it to specify a lower limit\n of how many hex digits we want to abbreviate to. Given that we use\n power-of-two here to derive the lower limit we may end up with an\n abbreviated hash that is one digit longer than required.\n\n - In `estimate_repack_memory()` we may end up overestimating how much\n memory a repack needs to pack objects. Conseuqently, we may end up\n dropping some packfiles from a repack.\n\nNone of these are really game-changing. But it's nice to fix those\nissues regardless.\n\nWhile at it, convert the code to use `repo_for_each_pack()`.\nFurthermore, use `odb_prepare_alternates()` instead of explicitly\npreparing the packfile store. We really only want to prepare the object\ndatabase sources, and `get_multi_pack_index()` already knows to prepare\nthe packfile store for us.\n\nHelped-by: Taylor Blau \nSigned-off-by: Patrick Steinhardt \n---\n packfile.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/packfile.c b/packfile.c\nindex 6aa2ca8ac9e..b07509b69bd 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1143,16 +1143,16 @@ unsigned long repo_approximate_object_count(struct repository *r)\n \t\tunsigned long count = 0;\n \t\tstruct packed_git *p;\n \n-\t\tpackfile_store_prepare(r->objects->packfiles);\n+\t\todb_prepare_alternates(r->objects);\n \n \t\tfor (source = r->objects->sources; source; source = source->next) {\n \t\t\tstruct multi_pack_index *m = get_multi_pack_index(source);\n \t\t\tif (m)\n-\t\t\t\tcount += m->num_objects;\n+\t\t\t\tcount += m->num_objects + m->num_objects_in_base;\n \t\t}\n \n-\t\tfor (p = r->objects->packfiles->packs; p; p = p->next) {\n-\t\t\tif (open_pack_index(p))\n+\t\trepo_for_each_pack(r, p) {\n+\t\t\tif (p->multi_pack_index || open_pack_index(p))\n \t\t\t\tcontinue;\n \t\t\tcount += p->num_objects;\n \t\t}\n\n-- \n2.51.2.997.g839fc31de9.dirty\n\n"},{"id":"529945","messageId":"20251030-pks-packfiles-store-drop-list-v2-5-84654f080cc0@pks.im","threadId":"64398","inReplyTo":"20251030-pks-packfiles-store-drop-list-v2-0-84654f080cc0@pks.im","subject":"[PATCH v2 5/8] builtin/pack-objects: simplify logic to find kept or nonlocal objects","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-10-30T10:38:42Z","receivedAt":"2025-10-30T10:39:01Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The function `has_sha1_pack_kept_or_nonlocal()` takes an object ID and\nthen searches through packed objects to figure out whether the object\nexists in a kept or non-local pack. As a performance optimization we\nremember the packfile that contains a given object ID so that the next\ncall to the function first checks that same packfile again.\n\nThe way this is written is rather hard to follow though, as the caching\nmechanism is intertwined with the loop that iterates through the packs.\nConsequently, we need to do some gymnastics to re-start the iteration if\nthe cached pack does not contain the objects.\n\nRefactor this so that we check the cached packfile at the beginning. We\ndon't have to re-verify whether the packfile meets the properties as we\nhave already verified those when storing the pack in `last_found` in the\nfirst place. So all we need to do is to use `find_pack_entry_one()` to\ncheck whether the pack contains the object ID, and to skip the cached\npack in the loop so that we don't search it twice.\n\nFurthermore, stop using the `(void *)1` sentinel value and instead use a\nsimple `NULL` pointer to indicate that we don't have a last-found pack\nyet.\n\nThis refactoring significantly simplifies the logic and makes it much\neasier to follow.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/pack-objects.c | 28 ++++++++++++++--------------\n 1 file changed, 14 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 5348aebbe9f..b83eb8ead14 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -4388,27 +4388,27 @@ static void add_unreachable_loose_objects(struct rev_info *revs)\n \n static int has_sha1_pack_kept_or_nonlocal(const struct object_id *oid)\n {\n-\tstruct packfile_store *packs = the_repository->objects->packfiles;\n-\tstatic struct packed_git *last_found = (void *)1;\n+\tstatic struct packed_git *last_found = NULL;\n \tstruct packed_git *p;\n \n-\tp = (last_found != (void *)1) ? last_found :\n-\t\t\t\t\tpackfile_store_get_packs(packs);\n+\tif (last_found && find_pack_entry_one(oid, last_found))\n+\t\treturn 1;\n \n-\twhile (p) {\n-\t\tif ((!p->pack_local || p->pack_keep ||\n-\t\t\t\tp->pack_keep_in_core) &&\n-\t\t\tfind_pack_entry_one(oid, p)) {\n+\trepo_for_each_pack(the_repository, p) {\n+\t\t/*\n+\t\t * We have already checked `last_found`, so there is no need to\n+\t\t * re-check here.\n+\t\t */\n+\t\tif (p == last_found)\n+\t\t\tcontinue;\n+\n+\t\tif ((!p->pack_local || p->pack_keep || p->pack_keep_in_core) &&\n+\t\t find_pack_entry_one(oid, p)) {\n \t\t\tlast_found = p;\n \t\t\treturn 1;\n \t\t}\n-\t\tif (p == last_found)\n-\t\t\tp = packfile_store_get_packs(packs);\n-\t\telse\n-\t\t\tp = p->next;\n-\t\tif (p == last_found)\n-\t\t\tp = p->next;\n \t}\n+\n \treturn 0;\n }\n \n\n-- \n2.51.2.997.g839fc31de9.dirty\n\n"},{"id":"529946","messageId":"20251030-pks-packfiles-store-drop-list-v2-6-84654f080cc0@pks.im","threadId":"64398","inReplyTo":"20251030-pks-packfiles-store-drop-list-v2-0-84654f080cc0@pks.im","subject":"[PATCH v2 6/8] packfile: move list of packs into the packfile store","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-10-30T10:38:43Z","receivedAt":"2025-10-30T10:39:04Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Move the list of packs into the packfile store. This follows the same\nlogic as in a previous commit, where we moved the most-recently-used\nlist of packs, as well.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/fast-import.c | 4 +--\n packfile.c | 83 +++++++++++++++++++++++----------------------------\n packfile.h | 16 +++-------\n 3 files changed, 43 insertions(+), 60 deletions(-)\n\ndiff --git a/builtin/fast-import.c b/builtin/fast-import.c\nindex 215295c1561..6fe6e9bc61d 100644\n--- a/builtin/fast-import.c\n+++ b/builtin/fast-import.c\n@@ -978,7 +978,7 @@ static int store_object(\n \tif (e->idx.offset) {\n \t\tduplicate_count_by_type[type]++;\n \t\treturn 1;\n-\t} else if (find_oid_pack(&oid, packfile_store_get_packs(packs))) {\n+\t} else if (packfile_list_find_oid(packfile_store_get_packs(packs), &oid)) {\n \t\te->type = type;\n \t\te->pack_id = MAX_PACK_ID;\n \t\te->idx.offset = 1; /* just not zero! */\n@@ -1179,7 +1179,7 @@ static void stream_blob(uintmax_t len, struct object_id *oidout, uintmax_t mark)\n \t\tduplicate_count_by_type[OBJ_BLOB]++;\n \t\ttruncate_pack(&checkpoint);\n \n-\t} else if (find_oid_pack(&oid, packfile_store_get_packs(packs))) {\n+\t} else if (packfile_list_find_oid(packfile_store_get_packs(packs), &oid)) {\n \t\te->type = OBJ_BLOB;\n \t\te->pack_id = MAX_PACK_ID;\n \t\te->idx.offset = 1; /* just not zero! */\ndiff --git a/packfile.c b/packfile.c\nindex b07509b69bd..71e95ae11c5 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -356,13 +356,14 @@ static void scan_windows(struct packed_git *p,\n \n static int unuse_one_window(struct packed_git *current)\n {\n-\tstruct packed_git *p, *lru_p = NULL;\n+\tstruct packfile_list_entry *e;\n+\tstruct packed_git *lru_p = NULL;\n \tstruct pack_window *lru_w = NULL, *lru_l = NULL;\n \n \tif (current)\n \t\tscan_windows(current, &lru_p, &lru_w, &lru_l);\n-\tfor (p = current->repo->objects->packfiles->packs; p; p = p->next)\n-\t\tscan_windows(p, &lru_p, &lru_w, &lru_l);\n+\tfor (e = current->repo->objects->packfiles->packs.head; e; e = e->next)\n+\t\tscan_windows(e->pack, &lru_p, &lru_w, &lru_l);\n \tif (lru_p) {\n \t\tmunmap(lru_w->base, lru_w->len);\n \t\tpack_mapped -= lru_w->len;\n@@ -542,14 +543,15 @@ static void find_lru_pack(struct packed_git *p, struct packed_git **lru_p, struc\n \n static int close_one_pack(struct repository *r)\n {\n-\tstruct packed_git *p, *lru_p = NULL;\n+\tstruct packfile_list_entry *e;\n+\tstruct packed_git *lru_p = NULL;\n \tstruct pack_window *mru_w = NULL;\n \tint accept_windows_inuse = 1;\n \n-\tfor (p = r->objects->packfiles->packs; p; p = p->next) {\n-\t\tif (p->pack_fd == -1)\n+\tfor (e = r->objects->packfiles->packs.head; e; e = e->next) {\n+\t\tif (e->pack->pack_fd == -1)\n \t\t\tcontinue;\n-\t\tfind_lru_pack(p, &lru_p, &mru_w, &accept_windows_inuse);\n+\t\tfind_lru_pack(e->pack, &lru_p, &mru_w, &accept_windows_inuse);\n \t}\n \n \tif (lru_p)\n@@ -868,8 +870,7 @@ void packfile_store_add_pack(struct packfile_store *store,\n \tif (pack->pack_fd != -1)\n \t\tpack_open_fds++;\n \n-\tpack->next = store->packs;\n-\tstore->packs = pack;\n+\tpackfile_list_prepend(&store->packs, pack);\n \n \tstrmap_put(&store->packs_by_path, pack->pack_name, pack);\n }\n@@ -1046,9 +1047,10 @@ static void prepare_packed_git_one(struct odb_source *source)\n \tstring_list_clear(data.garbage, 0);\n }\n \n-DEFINE_LIST_SORT(static, sort_packs, struct packed_git, next);\n+DEFINE_LIST_SORT(static, sort_packs, struct packfile_list_entry, next);\n \n-static int sort_pack(const struct packed_git *a, const struct packed_git *b)\n+static int sort_pack(const struct packfile_list_entry *a,\n+\t\t const struct packfile_list_entry *b)\n {\n \tint st;\n \n@@ -1058,7 +1060,7 @@ static int sort_pack(const struct packed_git *a, const struct packed_git *b)\n \t * remote ones could be on a network mounted filesystem.\n \t * Favor local ones for these reasons.\n \t */\n-\tst = a->pack_local - b->pack_local;\n+\tst = a->pack->pack_local - b->pack->pack_local;\n \tif (st)\n \t\treturn -st;\n \n@@ -1067,21 +1069,19 @@ static int sort_pack(const struct packed_git *a, const struct packed_git *b)\n \t * and more recent objects tend to get accessed more\n \t * often.\n \t */\n-\tif (a->mtime < b->mtime)\n+\tif (a->pack->mtime < b->pack->mtime)\n \t\treturn 1;\n-\telse if (a->mtime == b->mtime)\n+\telse if (a->pack->mtime == b->pack->mtime)\n \t\treturn 0;\n \treturn -1;\n }\n \n static void packfile_store_prepare_mru(struct packfile_store *store)\n {\n-\tstruct packed_git *p;\n-\n \tpackfile_list_clear(&store->mru);\n \n-\tfor (p = store->packs; p; p = p->next)\n-\t\tpackfile_list_append(&store->mru, p);\n+\tfor (struct packfile_list_entry *e = store->packs.head; e; e = e->next)\n+\t\tpackfile_list_append(&store->mru, e->pack);\n }\n \n void packfile_store_prepare(struct packfile_store *store)\n@@ -1096,7 +1096,11 @@ void packfile_store_prepare(struct packfile_store *store)\n \t\tprepare_multi_pack_index_one(source);\n \t\tprepare_packed_git_one(source);\n \t}\n-\tsort_packs(&store->packs, sort_pack);\n+\n+\tsort_packs(&store->packs.head, sort_pack);\n+\tfor (struct packfile_list_entry *e = store->packs.head; e; e = e->next)\n+\t\tif (!e->next)\n+\t\t\tstore->packs.tail = e;\n \n \tpackfile_store_prepare_mru(store);\n \tstore->initialized = true;\n@@ -1108,7 +1112,7 @@ void packfile_store_reprepare(struct packfile_store *store)\n \tpackfile_store_prepare(store);\n }\n \n-struct packed_git *packfile_store_get_packs(struct packfile_store *store)\n+struct packfile_list_entry *packfile_store_get_packs(struct packfile_store *store)\n {\n \tpackfile_store_prepare(store);\n \n@@ -1120,7 +1124,7 @@ struct packed_git *packfile_store_get_packs(struct packfile_store *store)\n \t\t\tprepare_midx_pack(m, i);\n \t}\n \n-\treturn store->packs;\n+\treturn store->packs.head;\n }\n \n struct packfile_list_entry *packfile_store_get_packs_mru(struct packfile_store *store)\n@@ -1276,11 +1280,11 @@ void mark_bad_packed_object(struct packed_git *p, const struct object_id *oid)\n const struct packed_git *has_packed_and_bad(struct repository *r,\n \t\t\t\t\t const struct object_id *oid)\n {\n-\tstruct packed_git *p;\n+\tstruct packfile_list_entry *e;\n \n-\tfor (p = r->objects->packfiles->packs; p; p = p->next)\n-\t\tif (oidset_contains(&p->bad_objects, oid))\n-\t\t\treturn p;\n+\tfor (e = r->objects->packfiles->packs.head; e; e = e->next)\n+\t\tif (oidset_contains(&e->pack->bad_objects, oid))\n+\t\t\treturn e->pack;\n \treturn NULL;\n }\n \n@@ -2088,19 +2092,6 @@ int is_pack_valid(struct packed_git *p)\n \treturn !open_packed_git(p);\n }\n \n-struct packed_git *find_oid_pack(const struct object_id *oid,\n-\t\t\t\t struct packed_git *packs)\n-{\n-\tstruct packed_git *p;\n-\n-\tfor (p = packs; p; p = p->next) {\n-\t\tif (find_pack_entry_one(oid, p))\n-\t\t\treturn p;\n-\t}\n-\treturn NULL;\n-\n-}\n-\n static int fill_pack_entry(const struct object_id *oid,\n \t\t\t struct pack_entry *e,\n \t\t\t struct packed_git *p)\n@@ -2139,7 +2130,7 @@ int find_pack_entry(struct repository *r, const struct object_id *oid, struct pa\n \t\tif (source->midx && fill_midx_entry(source->midx, oid, e))\n \t\t\treturn 1;\n \n-\tif (!r->objects->packfiles->packs)\n+\tif (!r->objects->packfiles->packs.head)\n \t\treturn 0;\n \n \tfor (l = r->objects->packfiles->mru.head; l; l = l->next) {\n@@ -2404,19 +2395,19 @@ struct packfile_store *packfile_store_new(struct object_database *odb)\n \n void packfile_store_free(struct packfile_store *store)\n {\n-\tfor (struct packed_git *p = store->packs, *next; p; p = next) {\n-\t\tnext = p->next;\n-\t\tfree(p);\n-\t}\n+\tfor (struct packfile_list_entry *e = store->packs.head; e; e = e->next)\n+\t\tfree(e->pack);\n+\tpackfile_list_clear(&store->packs);\n+\n \tstrmap_clear(&store->packs_by_path, 0);\n \tfree(store);\n }\n \n void packfile_store_close(struct packfile_store *store)\n {\n-\tfor (struct packed_git *p = store->packs; p; p = p->next) {\n-\t\tif (p->do_not_close)\n+\tfor (struct packfile_list_entry *e = store->packs.head; e; e = e->next) {\n+\t\tif (e->pack->do_not_close)\n \t\t\tBUG(\"want to close pack marked 'do-not-close'\");\n-\t\tclose_pack(p);\n+\t\tclose_pack(e->pack);\n \t}\n }\ndiff --git a/packfile.h b/packfile.h\nindex a53336d722a..d95275e666c 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -11,7 +11,6 @@\n struct object_info;\n \n struct packed_git {\n-\tstruct packed_git *next;\n \tstruct pack_window *windows;\n \toff_t pack_size;\n \tconst void *index_data;\n@@ -83,7 +82,7 @@ struct packfile_store {\n \t * The list of packfiles in the order in which they are being added to\n \t * the store.\n \t */\n-\tstruct packed_git *packs;\n+\tstruct packfile_list packs;\n \n \t/*\n \t * Cache of packfiles which are marked as \"kept\", either because there\n@@ -163,13 +162,14 @@ void packfile_store_add_pack(struct packfile_store *store,\n * repository.\n */\n #define repo_for_each_pack(repo, p) \\\n-\tfor (p = packfile_store_get_packs(repo->objects->packfiles); p; p = p->next)\n+\tfor (struct packfile_list_entry *e = packfile_store_get_packs(repo->objects->packfiles); \\\n+\t ((p) = (e ? e->pack : NULL)); e = e->next)\n \n /*\n * Get all packs managed by the given store, including packfiles that are\n * referenced by multi-pack indices.\n */\n-struct packed_git *packfile_store_get_packs(struct packfile_store *store);\n+struct packfile_list_entry *packfile_store_get_packs(struct packfile_store *store);\n \n /*\n * Get all packs in most-recently-used order.\n@@ -266,14 +266,6 @@ extern void (*report_garbage)(unsigned seen_bits, const char *path);\n */\n unsigned long repo_approximate_object_count(struct repository *r);\n \n-/*\n- * Find the pack within the \"packs\" list whose index contains the object \"oid\".\n- * For general object lookups, you probably don't want this; use\n- * find_pack_entry() instead.\n- */\n-struct packed_git *find_oid_pack(const struct object_id *oid,\n-\t\t\t\t struct packed_git *packs);\n-\n void pack_report(struct repository *repo);\n \n /*\n\n-- \n2.51.2.997.g839fc31de9.dirty\n\n"},{"id":"529947","messageId":"20251030-pks-packfiles-store-drop-list-v2-7-84654f080cc0@pks.im","threadId":"64398","inReplyTo":"20251030-pks-packfiles-store-drop-list-v2-0-84654f080cc0@pks.im","subject":"[PATCH v2 7/8] packfile: always add packfiles to MRU when adding a pack","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-10-30T10:38:44Z","receivedAt":"2025-10-30T10:39:07Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When preparing the packfile store we know to also prepare the MRU list\nof packfiles with all packs that are currently loaded in the store via\n`packfile_store_prepare_mru()`. So we know that the list of packs in the\nMRU list should match the list of packs in the non-MRU list.\n\nBut there are some direct or indirect callsites that add a packfile to\nthe store via `packfile_store_add_pack()` without adding the pack to the\nMRU. And while functions that access the MRU (e.g. `find_pack_entry()`)\nknow to call `packfile_store_prepare()`, which knows to prepare the MRU\nvia `packfile_store_prepare_mru()`, that operation will be turned into a\nno-op because the packfile store is already prepared. So this will not\ncause us to add the packfile to the MRU, and consequently we won't be\nable to find the packfile in our MRU list.\n\nThere are only a handful of callers outside of \"packfile.c\" that add a\npackfile to the store:\n\n - \"builtin/fast-import.c\" adds multiple packs of imported objects, but\n it knows to look up objects via `packfile_store_get_packs()`. This\n function does not use the MRU, so we're good.\n\n - \"builtin/index-pack.c\" adds the indexed pack to the store in case it\n needs to perform consistency checks on its objects.\n\n - \"http.c\" adds the fetched pack to the store so that we can access\n its objects.\n\nIn all of these cases we actually want to access the contained objects.\nAnd luckily, reading these objects works as expected:\n\n 1. We eventually end up in `do_oid_object_info_extended()`.\n\n 2. Calling `find_pack_entry()` fails because the MRU list doesn't\n contain the newly added packfile.\n\n 3. The callers don't pass `OBJECT_INFO_QUICK`, so we end up\n repreparing the object database. This will also cause us to\n reprepare the MRU list.\n\n 4. We now retry reading the object via `find_pack_entry()`, and now we\n succeed because the MRU list got populated.\n\nThis logic feels quite fragile: we intentionally add the packfile to the\nstore, but we then ultimately rely on repreparing the entire store only\nto make the packfile accessible. While we do the correct thing in\n`do_oid_object_info_extended()`, other sites that access the MRU may not\nknow to reprepare.\n\nBut besides being fragile it's also a waste of resources: repreparing\nthe object database requires us to re-read the alternates file and\ndiscard any caches.\n\nRefactor the code so that we unconditionally add packfiles to the MRU\nwhen adding them to a packfile store. This makes the logic less fragile\nand ensures that we don't have to reprepare the store to make the pack\naccessible.\n\nNote that this does not allow us to drop `packfile_store_prepare_mru()`\njust yet: while the MRU list is already populated with all packs now,\nthe order in which we add these packs is indeterministic for most of the\npart. So by first calling `sort_pack()` on the other packfile list and\nthen re-preparing the MRU list we inherit its sorting.\n\nSigned-off-by: Patrick Steinhardt \n---\n midx.c | 2 --\n packfile.c | 1 +\n 2 files changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 8022be9a45e..24e1e721754 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -462,8 +462,6 @@ int prepare_midx_pack(struct multi_pack_index *m,\n \t\t m->pack_names[pack_int_id]);\n \tp = packfile_store_load_pack(r->objects->packfiles,\n \t\t\t\t pack_name.buf, m->source->local);\n-\tif (p)\n-\t\tpackfile_list_append(&m->source->odb->packfiles->mru, p);\n \tstrbuf_release(&pack_name);\n \n \tif (!p) {\ndiff --git a/packfile.c b/packfile.c\nindex 71e95ae11c5..60f2e42876a 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -871,6 +871,7 @@ void packfile_store_add_pack(struct packfile_store *store,\n \t\tpack_open_fds++;\n \n \tpackfile_list_prepend(&store->packs, pack);\n+\tpackfile_list_append(&store->mru, pack);\n \n \tstrmap_put(&store->packs_by_path, pack->pack_name, pack);\n }\n\n-- \n2.51.2.997.g839fc31de9.dirty\n\n"},{"id":"529948","messageId":"20251030-pks-packfiles-store-drop-list-v2-8-84654f080cc0@pks.im","threadId":"64398","inReplyTo":"20251030-pks-packfiles-store-drop-list-v2-0-84654f080cc0@pks.im","subject":"[PATCH v2 8/8] packfile: track packs via the MRU list exclusively","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-10-30T10:38:45Z","receivedAt":"2025-10-30T10:39:10Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We track packfiles via two different lists:\n\n - `struct packfile_store::packs` is a list that sorts local packs\n first. In addition, these packs are sorted so that younger packs are\n sorted towards the front.\n\n - `struct packfile_store::mru` is a list that sorts packs so that\n most-recently used packs are at the front.\n\nThe reasoning behind the ordering in the `packs` list is that younger\nobjects stored in the local object store tend to be accessed more\nfrequently, and that is certainly true for some cases. But there are\ngoing to be lots of cases where that isn't true. Especially when\ntraversing history it is likely that one needs to access many older\nobjects, and due to our housekeeping it is very likely that almost all\nof those older objects will be contained in one large pack that is\noldest.\n\nSo whether or not the ordering makes sense really depends on the use\ncase at hand. A flexible approach like our MRU list addresses that need,\nas it will sort packs towards the front that are accessed all the time.\nIntuitively, this approach is thus able to satisfy more use cases more\nefficiently.\n\nThis reasoning casts some doubt on whether or not it really makes sense\nto track packs via two different lists. It causes confusion, and it is\nnot clear whether there are use cases where the `packs` list really is\nsuch an obvious choice.\n\nMerge these two lists into one most-recently-used list.\n\nNote that there is one important edge case: `for_each_packed_object()`\nuses the MRU list to iterate through packs, and then it lists each\nobject in those packs. This would have the effect that we now sort the\ncurrent pack towards the front, thus modifying the list of packfiles we\nare iterating over, with the consequence that we'll see an infinite\nloop. This edge case is worked around by introducing a new field that\nallows us to skip updating the MRU.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/pack-objects.c | 4 ++--\n packfile.c | 27 +++++++--------------------\n packfile.h | 27 +++++++++++++++++----------\n 3 files changed, 26 insertions(+), 32 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex b83eb8ead14..0e4e9f80682 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -1748,11 +1748,11 @@ static int want_object_in_pack_mtime(const struct object_id *oid,\n \t\t}\n \t}\n \n-\tfor (e = the_repository->objects->packfiles->mru.head; e; e = e->next) {\n+\tfor (e = the_repository->objects->packfiles->packs.head; e; e = e->next) {\n \t\tstruct packed_git *p = e->pack;\n \t\twant = want_object_in_pack_one(p, oid, exclude, found_pack, found_offset, found_mtime);\n \t\tif (!exclude && want > 0)\n-\t\t\tpackfile_list_prepend(&the_repository->objects->packfiles->mru, p);\n+\t\t\tpackfile_list_prepend(&the_repository->objects->packfiles->packs, p);\n \t\tif (want != -1)\n \t\t\treturn want;\n \t}\ndiff --git a/packfile.c b/packfile.c\nindex 60f2e42876a..378b0b1920d 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -870,9 +870,7 @@ void packfile_store_add_pack(struct packfile_store *store,\n \tif (pack->pack_fd != -1)\n \t\tpack_open_fds++;\n \n-\tpackfile_list_prepend(&store->packs, pack);\n-\tpackfile_list_append(&store->mru, pack);\n-\n+\tpackfile_list_append(&store->packs, pack);\n \tstrmap_put(&store->packs_by_path, pack->pack_name, pack);\n }\n \n@@ -1077,14 +1075,6 @@ static int sort_pack(const struct packfile_list_entry *a,\n \treturn -1;\n }\n \n-static void packfile_store_prepare_mru(struct packfile_store *store)\n-{\n-\tpackfile_list_clear(&store->mru);\n-\n-\tfor (struct packfile_list_entry *e = store->packs.head; e; e = e->next)\n-\t\tpackfile_list_append(&store->mru, e->pack);\n-}\n-\n void packfile_store_prepare(struct packfile_store *store)\n {\n \tstruct odb_source *source;\n@@ -1103,7 +1093,6 @@ void packfile_store_prepare(struct packfile_store *store)\n \t\tif (!e->next)\n \t\t\tstore->packs.tail = e;\n \n-\tpackfile_store_prepare_mru(store);\n \tstore->initialized = true;\n }\n \n@@ -1128,12 +1117,6 @@ struct packfile_list_entry *packfile_store_get_packs(struct packfile_store *stor\n \treturn store->packs.head;\n }\n \n-struct packfile_list_entry *packfile_store_get_packs_mru(struct packfile_store *store)\n-{\n-\tpackfile_store_prepare(store);\n-\treturn store->mru.head;\n-}\n-\n /*\n * Give a fast, rough count of the number of objects in the repository. This\n * ignores loose objects completely. If you have a lot of them, then either\n@@ -2134,11 +2117,12 @@ int find_pack_entry(struct repository *r, const struct object_id *oid, struct pa\n \tif (!r->objects->packfiles->packs.head)\n \t\treturn 0;\n \n-\tfor (l = r->objects->packfiles->mru.head; l; l = l->next) {\n+\tfor (l = r->objects->packfiles->packs.head; l; l = l->next) {\n \t\tstruct packed_git *p = l->pack;\n \n \t\tif (!p->multi_pack_index && fill_pack_entry(oid, e, p)) {\n-\t\t\tpackfile_list_prepend(&r->objects->packfiles->mru, p);\n+\t\t\tif (!r->objects->packfiles->skip_mru_updates)\n+\t\t\t\tpackfile_list_prepend(&r->objects->packfiles->packs, p);\n \t\t\treturn 1;\n \t\t}\n \t}\n@@ -2270,6 +2254,7 @@ int for_each_packed_object(struct repository *repo, each_packed_object_fn cb,\n \tint r = 0;\n \tint pack_errors = 0;\n \n+\trepo->objects->packfiles->skip_mru_updates = true;\n \trepo_for_each_pack(repo, p) {\n \t\tif ((flags & FOR_EACH_OBJECT_LOCAL_ONLY) && !p->pack_local)\n \t\t\tcontinue;\n@@ -2290,6 +2275,8 @@ int for_each_packed_object(struct repository *repo, each_packed_object_fn cb,\n \t\tif (r)\n \t\t\tbreak;\n \t}\n+\trepo->objects->packfiles->skip_mru_updates = false;\n+\n \treturn r ? r : pack_errors;\n }\n \ndiff --git a/packfile.h b/packfile.h\nindex d95275e666c..27ba607e7c5 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -79,8 +79,8 @@ struct packfile_store {\n \tstruct object_database *odb;\n \n \t/*\n-\t * The list of packfiles in the order in which they are being added to\n-\t * the store.\n+\t * The list of packfiles in the order in which they have been most\n+\t * recently used.\n \t */\n \tstruct packfile_list packs;\n \n@@ -98,9 +98,6 @@ struct packfile_store {\n \t\tunsigned flags;\n \t} kept_cache;\n \n-\t/* A most-recently-used ordered version of the packs list. */\n-\tstruct packfile_list mru;\n-\n \t/*\n \t * A map of packfile names to packed_git structs for tracking which\n \t * packs have been loaded already.\n@@ -112,6 +109,21 @@ struct packfile_store {\n \t * packs.\n \t */\n \tbool initialized;\n+\n+\t/*\n+\t * Usually, packfiles will be reordered to the front of the `packs`\n+\t * list whenever an object is looked up via them. This has the effect\n+\t * that packs that contain a lot of accessed objects will be located\n+\t * towards the front.\n+\t *\n+\t * This is usually desireable, but there are exceptions. One exception\n+\t * is when the looking up multiple objects in a loop for each packfile.\n+\t * In that case, we may easily end up with an infinite loop as the\n+\t * packfiles get reordered to the front repeatedly.\n+\t *\n+\t * Setting this field to `true` thus disables these reorderings.\n+\t */\n+\tbool skip_mru_updates;\n };\n \n /*\n@@ -171,11 +183,6 @@ void packfile_store_add_pack(struct packfile_store *store,\n */\n struct packfile_list_entry *packfile_store_get_packs(struct packfile_store *store);\n \n-/*\n- * Get all packs in most-recently-used order.\n- */\n-struct packfile_list_entry *packfile_store_get_packs_mru(struct packfile_store *store);\n-\n /*\n * Open the packfile and add it to the store if it isn't yet known. Returns\n * either the newly opened packfile or the preexisting packfile. Returns a\n\n-- \n2.51.2.997.g839fc31de9.dirty\n\n"}]}