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

From: Junio C Hamano <gitster@pobox.com>
Date: May 22, 2025, 15:47 UTC
Message-ID: <xmqqbjrk7ikx.fsf@gitster.g>
In-Reply-To: <20250522053235.GB1134267@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 24 quoted lines

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

So the idea is to leave m->packs[] for unused pack NULL instead of magic value, so that users of that array do not need to check?

I think that is a lot safer than the magic "we know this fails" value that existing callers that have long trusted that a non-NULL .packs[] element is a valid pack.

By the way, I suspect I am not reading the code correctly, but I am not sure what fill_midx_entry() does with a failed case.

	midx_for_object(&m, pos);
	pack_int_id = nth_midxed_pack_int_id(m, pos);

	if (prepare_midx_pack(r, m, pack_int_id))
		return 0;

With or without cached failure, this should return 0 when and only when m->packs[pack_int_id] is a usable pack. But what about the access on the next line?

	p = m->packs[pack_int_id - m->num_packs_in_base];

Do we have any guarantee that we called prepare_midx_pack() for the pack at (pack_int_id - m->num_packs_in_base)th slot? Can p be NULL here? And with the magic "we know this fails" value, can p be that magic value?

	/*
	* We are about to tell the caller where they can locate the
	* requested object.  We better make sure the packfile is
	* still here and can be accessed before supplying that
	* answer, as it may have been deleted since the MIDX was
	* loaded!
	*/
	if (!is_pack_valid(p))
		return 0;

That value is passed to is_pack_valid(), which I do not think even takes NULL.

Puzzled.