From: Patrick Steinhardt <ps@pks.im>
Date: Mon, 26 May 2025 07:24:03 GMT
Subject: Re: [PATCH 5/5] midx: return a `packed_git` pointer from `prepare_midx_pack()`
Message-ID: <aDQXEz-RtrcUAwic@pks.im>
In-Reply-To: <80699bb3ee35f8c4728e7efe8dc53d17134a5fe5.1748198489.git.me@ttaylorr.com>

On Sun, May 25, 2025 at 02:42:03PM -0400, Taylor Blau wrote:
> diff --git a/midx.c b/midx.c
> index fbce88bd46..f7f509cf46 100644
> --- a/midx.c
> +++ b/midx.c
> @@ -449,50 +449,48 @@ static uint32_t midx_for_pack(struct multi_pack_index **_m,
>  	return pack_int_id - m->num_packs_in_base;
>  }
>  
> -int prepare_midx_pack(struct repository *r, struct multi_pack_index *m,
> -		      uint32_t pack_int_id)
> +struct packed_git *prepare_midx_pack(struct repository *r,
> +				     struct multi_pack_index *m,
> +				     uint32_t pack_int_id)
>  {
> -	struct strbuf pack_name = STRBUF_INIT;
> -	struct strbuf key = STRBUF_INIT;
> -	struct packed_git *p;
> +	uint32_t pack_pos = midx_for_pack(&m, pack_int_id);
>  
> -	pack_int_id = midx_for_pack(&m, pack_int_id);
> +	if (!m->packs[pack_pos]) {
> +		struct strbuf pack_name = STRBUF_INIT;
> +		struct strbuf key = STRBUF_INIT;
> +		struct packed_git *p;
>  
> -	if (m->packs[pack_int_id] == (void *)(intptr_t)-1)
> -		return 1;

Ah, so this series builds on top of my patch that introduces the
negative lookup cache? That wasn't quite clear to me and makes it a bit
hard to iterate on my patch now. Could I suggest that you maybe include
that patch as part of this series so that those can be iterated on in
tandem?

Overall I think that this change is quite sensible and hides away at
least some of the complexity. Thanks!

Patrick