Re: [PATCH v2 2/2] midx: stop repeatedly looking up nonexistent packfiles
From: Jeff King <hidden>
Date: 2025-05-22 05:32:37
Subsystem:
the rest · Maintainer:
Linus Torvalds
On Tue, May 20, 2025 at 11:53:10AM +0200, Patrick Steinhardt wrote:
quoted hunk ↗ jump to hunk
@@ -458,6 +458,8 @@ int prepare_midx_pack(struct repository *r, struct multi_pack_index *m, pack_int_id = midx_for_pack(&m, pack_int_id); + if (m->packs[pack_int_id] == (void *)(intptr_t)-1) + return 1; if (m->packs[pack_int_id]) return 0;
I did wonder while writing this if we might be able to hide the magic number and gross casting inside a constant or macro. I think just: #define MIDX_PACK_ERROR ((void *)(intptr_t)-1) would be enough? Though...
quoted hunk ↗ jump to hunk
@@ -495,6 +499,8 @@ struct packed_git *nth_midxed_pack(struct multi_pack_index *m, uint32_t pack_int_id) { uint32_t local_pack_int_id = midx_for_pack(&m, pack_int_id); + if (m->packs[local_pack_int_id] == (void *)(intptr_t)-1) + return NULL; return m->packs[local_pack_int_id];
Yuck, yet another spot that needs to be aware of the new tri-state value. One alternative is using an auxiliary array to cache the errors, and then only the lookup function needs to care. Like:
diff --git a/midx.c b/midx.c
index c1adff4404..df71ead50b 100644
--- a/midx.c
+++ b/midx.c@@ -186,6 +186,7 @@ static struct multi_pack_index *load_multi_pack_index_one(struct repository *r, CALLOC_ARRAY(m->pack_names, m->num_packs); CALLOC_ARRAY(m->packs, m->num_packs); + CALLOC_ARRAY(m->pack_err, m->num_packs); cur_pack_name = (const char *)m->chunk_pack_names; for (i = 0; i < m->num_packs; i++) {
@@ -408,6 +409,7 @@ void close_midx(struct multi_pack_index *m) if (m->packs[i]) m->packs[i]->multi_pack_index = 0; } + FREE_AND_NULL(m->pack_errs); FREE_AND_NULL(m->packs); FREE_AND_NULL(m->pack_names); free(m);
@@ -460,6 +462,8 @@ int prepare_midx_pack(struct repository *r, struct multi_pack_index *m, if (m->packs[pack_int_id]) return 0; + if (m->pack_errs[pack_int_id]) + return 1; strbuf_addf(&pack_name, "%s/pack/%s", m->object_dir, m->pack_names[pack_int_id]);
@@ -482,8 +486,10 @@ int prepare_midx_pack(struct repository *r, struct multi_pack_index *m, strbuf_release(&pack_name); strbuf_release(&key); - if (!p) + if (!p) { + m->pack_errs[pack_int_id] = 1; return 1; + } p->multi_pack_index = 1; m->packs[pack_int_id] = p;
You could even lazy-malloc the extra array if you wanted to optimize the common no-errors case, but I'm not sure it's a big deal. -Peff