Re: [PATCH 01/16] midx-write: handle noop writes when converting incremental chains

[Jeff King <peff@peff.net>]

On Sun, Mar 29, 2026 at 05:40:53PM -0400, Taylor Blau wrote:

> This comparison fails when we have an incremental MIDX chain with
> at least two layers, since we do not recursively peel through earlier
> layers, instead treating the `->pack_names` array of the tip MIDX layer
> as containing all `m->num_packs + m->num_packs_in_base` packs.
> 
> Adjust this to instead look through the MIDX layers one by one when
> comparing pack names. While we're at it, fix a typo above in the same
> function.

Makes sense.

> @@ -1208,14 +1208,16 @@ static bool midx_needs_update(struct multi_pack_index *midx, struct write_midx_c
>  			BUG("same pack added twice?");
>  	}
>  
> -	for (uint32_t i = 0; i < ctx->nr; i++) {
> -		strbuf_reset(&buf);
> -		strbuf_addstr(&buf, midx->pack_names[i]);
> -		strbuf_strip_suffix(&buf, ".idx");
> +	for (struct multi_pack_index *m = midx; m; m = m->base_midx) {
> +		for (uint32_t i = 0; i < m->num_packs; i++) {
> +			strbuf_reset(&buf);
> +			strbuf_addstr(&buf, m->pack_names[i]);
> +			strbuf_strip_suffix(&buf, ".idx");
>  
> -		if (!strset_contains(&packs, buf.buf))
> -			goto out;
> -		strset_remove(&packs, buf.buf);
> +			if (!strset_contains(&packs, buf.buf))
> +				goto out;
> +			strset_remove(&packs, buf.buf);
> +		}

I wondered here if we would also need to adjust the check of the count,
but it is already OK. Just above the context is:

          if (ctx->nr != midx->num_packs + midx->num_packs_in_base)
                  goto out;

which handles the layered midx. Since we were counting up to ctx->nr
before, which includes packs in base midx layers, I guess these old
lines:

> -	for (uint32_t i = 0; i < ctx->nr; i++) {
> [...]
> -		strbuf_addstr(&buf, midx->pack_names[i]);

was probably reading garbage off the end of the array.

Now we iterate through each layer individually. We assume that the total
count matches ctx->nr, otherwise the midx struct is totally corrupt. We
could count them and double-check, I guess, but I don't think there's
much point (it would be a BUG() if they were mismatched).

So looks good to me.

-Peff