Subject: Re: [PATCH 07/17] midx-write.c: don't use `pack_perm` when assigning `bitmap_pos`

From: Patrick Steinhardt <[email-masked]>
Date: Mon, 8 Dec 2025 19:26:27 +0100

On Sat, Dec 06, 2025 at 03:31:19PM -0500, Taylor Blau wrote:
> In midx_pack_order(), we compute for each bitampped pack the first bit

s/bitampped/bitmapped/

> to correspond to an object in that pack, along with how many bits were
> assigned to object(s) in that pack.
> 
> Initially, each bitmap_nr value is set to zero, and each bitmap_pos

I assume `bitmap_nr` is the number of bits, whereas `bitmap_pos` is the
position of the first bit?

> value is set to the sentinel BITMAP_POS_UNKNOWN. This is done to ensure
> that there are no packs who have an unknown bit position but a somehow
> non-zero number of objects (cf. `write_midx_bitmapped_packs()` in
> midx-write.c).
> 
> Once the pack order is fully determined, midx_pack_order() sets the
> bitmap_pos field for any bitmapped packs to zero if they are still
> listed as BITMAP_POS_UNKNOWN.

If so, it feels somewhat weird that we'd set the `bitmap_pos` to zero.
But I guess it doesn't matter anyway, as I assume that the `bitmap_nr`
would be zero anyway?

Anyway, reading on.

> However, we enumerate the bitmapped packs in order of `ctx->pack_perm`.

Which is the "permutation between pack-int-ids from the previous
multi-pack-index to the new one we are writing"'. So it's basically
tracking which new packs correspond to the old packs.

> This is fine for existing cases, since the only time the
> `ctx->pack_perm` array holds a value outside of the addressable range of
> `ctx->info` is when there are expired packs, which only occurs via 'git
> multi-pack-index expire', which does not support writing MIDX bitmaps.
> As a result, the range of ctx->pack_perm covers all values in [0,
> `ctx->nr`), so enumerating in this order isn't an issue.
> 
> A future change necessary for compaction will complicate this further by
> introducing a wrapper around the `ctx->pack_perm` array, which turns the
> given `pack_int_id` into one that is relative to the lower end of the
> compaction range. As a result, indexing into `ctx->pack_perm` through
> this helper, say, with "0" will produce a crash when the lower end of
> the compaction range has >0 pack(s) in its base layer, since the
> subtraction will wrap around the 32-bit unsigned range, resulting in an
> uninitialized read.
> 
> But the process is completely unnecessary in the first place: we are
> enumerating all values of `ctx->info`, and there is no reason to process
> them in a different order than they appear in memory. Index `ctx->info`
> directly to reflect that.

Fair. We do initialize the permutations like this:

	ALLOC_ARRAY(ctx.pack_perm, ctx.nr);
	for (size_t i = 0; i < ctx.nr; i++) {
		if (ctx.info[i].expired) {
			dropped_packs++;
			ctx.pack_perm[ctx.info[i].orig_pack_int_id] = PACK_EXPIRED;
		} else {
			ctx.pack_perm[ctx.info[i].orig_pack_int_id] = i - dropped_packs;
		}
	}

So obviously, the permutation will only ever be different in case we've
got at least one dropped pack, and that only happens when we expire any
packs. So the explanation matches.

Of course it may be a bit more fragile now if we ever added a caller
of this function that _does_ expire data. But we don't have any, so that
enters the territory of overthinking things.

> diff --git a/midx-write.c b/midx-write.c
> index 73d24fabbc6..c30f6a70d37 100644
> --- a/midx-write.c
> +++ b/midx-write.c
> @@ -637,7 +637,7 @@ static uint32_t *midx_pack_order(struct write_midx_context *ctx)
>  		pack_order[i] = data[i].nr;
>  	}
>  	for (i = 0; i < ctx->nr; i++) {
> -		struct pack_info *pack = &ctx->info[ctx->pack_perm[i]];
> +		struct pack_info *pack = &ctx->info[i];
>  		if (pack->bitmap_pos == BITMAP_POS_UNKNOWN)
>  			pack->bitmap_pos = 0;
>  	}

The change looks simple enough.

Patrick