Re: [PATCH v2 2/2] midx: stop repeatedly looking up nonexistent packfiles

[Jeff King <peff@xxxxxxxx>]

On Tue, May 20, 2025 at 11:53:10AM +0200, Patrick Steinhardt wrote:

> @@ -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...

> @@ -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