From: Patrick Steinhardt <ps@pks•im>
To: Taylor Blau <me@ttaylorr•com>
Cc: git@vger•kernel.org, Elijah Newren <newren@gmail•com>,
Jeff King <peff@peff•net>, Junio C Hamano <gitster@pobox•com>
Subject: Re: [PATCH 07/17] midx-write.c: don't use `pack_perm` when assigning `bitmap_pos`
Date: Mon, 8 Dec 2025 19:26:27 +0100 [thread overview]
Message-ID: <aTcYU_yVYyXL9TXv@pks.im> (raw)
In-Reply-To: <1fc359d9a98cdd61f6986fa1d9a1190f57dacb9b.1765053054.git.me@ttaylorr.com>
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
next prev parent reply other threads:[~2025-12-08 18:26 UTC|newest]
Thread overview: 99+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-12-06 20:30 [PATCH 00/17] midx: incremental MIDX/bitmap layer compaction Taylor Blau
2025-12-06 20:31 ` [PATCH 01/17] midx: mark `get_midx_checksum()` arguments as const Taylor Blau
2025-12-08 18:26 ` Patrick Steinhardt
2025-12-09 1:41 ` Taylor Blau
2025-12-06 20:31 ` [PATCH 02/17] midx: split `get_midx_checksum()` by adding `get_midx_hash()` Taylor Blau
2025-12-08 18:25 ` Patrick Steinhardt
2025-12-09 1:42 ` Taylor Blau
2025-12-09 1:50 ` Taylor Blau
2025-12-09 6:27 ` Patrick Steinhardt
2026-01-13 22:46 ` Taylor Blau
2025-12-06 20:31 ` [PATCH 03/17] builtin/multi-pack-index.c: make '--progress' a common option Taylor Blau
2025-12-06 20:31 ` [PATCH 04/17] git-multi-pack-index(1): remove non-existent incompatibility Taylor Blau
2025-12-06 20:31 ` [PATCH 05/17] git-multi-pack-index(1): align SYNOPSIS with 'git multi-pack-index -h' Taylor Blau
2025-12-06 20:31 ` [PATCH 06/17] t/t5319-multi-pack-index.sh: fix copy-and-paste error in t5319.39 Taylor Blau
2025-12-06 20:31 ` [PATCH 07/17] midx-write.c: don't use `pack_perm` when assigning `bitmap_pos` Taylor Blau
2025-12-08 18:26 ` Patrick Steinhardt [this message]
2025-12-09 1:59 ` Taylor Blau
2025-12-06 20:31 ` [PATCH 08/17] midx-write.c: introduce `struct write_midx_opts` Taylor Blau
2025-12-08 18:26 ` Patrick Steinhardt
2025-12-09 2:04 ` Taylor Blau
2025-12-06 20:31 ` [PATCH 09/17] midx: do not require packs to be sorted in lexicographic order Taylor Blau
2025-12-08 18:26 ` Patrick Steinhardt
2025-12-09 2:07 ` Taylor Blau
2025-12-09 2:11 ` Taylor Blau
2025-12-06 20:31 ` [PATCH 10/17] git-compat-util.h: introduce `u32_add()` Taylor Blau
2025-12-08 18:27 ` Patrick Steinhardt
2025-12-09 2:13 ` Taylor Blau
2025-12-06 20:31 ` [PATCH 11/17] midx-write.c: introduce `midx_pack_perm()` helper Taylor Blau
2025-12-06 20:31 ` [PATCH 12/17] midx-write.c: extract `fill_pack_from_midx()` Taylor Blau
2025-12-06 20:31 ` [PATCH 13/17] midx-write.c: enumerate `pack_int_id` values directly Taylor Blau
2025-12-08 18:27 ` Patrick Steinhardt
2025-12-09 2:14 ` Taylor Blau
2025-12-06 20:31 ` [PATCH 14/17] midx-write.c: factor fanout layering from `compute_sorted_entries()` Taylor Blau
2025-12-06 20:31 ` [PATCH 15/17] t/helper/test-read-midx.c: plug memory leak when selecting layer Taylor Blau
2025-12-08 18:27 ` Patrick Steinhardt
2025-12-09 2:16 ` Taylor Blau
2025-12-06 20:31 ` [PATCH 16/17] midx: implement MIDX compaction Taylor Blau
2025-12-09 7:21 ` Patrick Steinhardt
2026-01-13 23:32 ` Taylor Blau
2025-12-06 20:31 ` [PATCH 17/17] midx: enable reachability bitmaps during " Taylor Blau
2025-12-09 7:21 ` Patrick Steinhardt
2026-01-13 23:47 ` Taylor Blau
2026-01-14 19:54 ` [PATCH v2 00/18] midx: incremental MIDX/bitmap layer compaction Taylor Blau
2026-01-14 19:54 ` [PATCH v2 01/18] midx: mark `get_midx_checksum()` arguments as const Taylor Blau
2026-01-14 19:54 ` [PATCH v2 02/18] midx: rename `get_midx_checksum()` to `midx_get_checksum_hash()` Taylor Blau
2026-01-14 19:54 ` [PATCH v2 03/18] midx: introduce `midx_get_checksum_hex()` Taylor Blau
2026-01-14 19:54 ` [PATCH v2 04/18] builtin/multi-pack-index.c: make '--progress' a common option Taylor Blau
2026-01-14 19:54 ` [PATCH v2 05/18] git-multi-pack-index(1): remove non-existent incompatibility Taylor Blau
2026-01-14 19:54 ` [PATCH v2 06/18] git-multi-pack-index(1): align SYNOPSIS with 'git multi-pack-index -h' Taylor Blau
2026-01-14 19:54 ` [PATCH v2 07/18] t/t5319-multi-pack-index.sh: fix copy-and-paste error in t5319.39 Taylor Blau
2026-01-14 19:54 ` [PATCH v2 08/18] midx-write.c: don't use `pack_perm` when assigning `bitmap_pos` Taylor Blau
2026-01-14 21:13 ` Junio C Hamano
2026-01-14 21:40 ` Taylor Blau
2026-01-14 19:54 ` [PATCH v2 09/18] midx-write.c: introduce `struct write_midx_opts` Taylor Blau
2026-01-14 19:54 ` [PATCH v2 10/18] midx: do not require packs to be sorted in lexicographic order Taylor Blau
2026-01-14 21:28 ` Junio C Hamano
2026-01-14 21:44 ` Taylor Blau
2026-01-27 7:34 ` Patrick Steinhardt
2026-02-24 18:47 ` Taylor Blau
2026-01-14 19:54 ` [PATCH v2 11/18] git-compat-util.h: introduce `u32_add()` Taylor Blau
2026-01-14 21:49 ` Junio C Hamano
2026-01-14 22:03 ` Taylor Blau
2026-01-15 0:11 ` Taylor Blau
2026-01-21 8:51 ` Patrick Steinhardt
2026-01-21 23:55 ` Taylor Blau
2026-01-22 2:26 ` rsbecker
2026-01-22 17:07 ` Junio C Hamano
2026-02-23 13:49 ` Jeff King
2026-02-24 18:53 ` Taylor Blau
2026-01-14 19:54 ` [PATCH v2 12/18] midx-write.c: introduce `midx_pack_perm()` helper Taylor Blau
2026-01-14 19:54 ` [PATCH v2 13/18] midx-write.c: extract `fill_pack_from_midx()` Taylor Blau
2026-01-14 19:54 ` [PATCH v2 14/18] midx-write.c: enumerate `pack_int_id` values directly Taylor Blau
2026-01-14 19:55 ` [PATCH v2 15/18] midx-write.c: factor fanout layering from `compute_sorted_entries()` Taylor Blau
2026-01-14 19:55 ` [PATCH v2 16/18] t/helper/test-read-midx.c: plug memory leak when selecting layer Taylor Blau
2026-01-14 19:55 ` [PATCH v2 17/18] midx: implement MIDX compaction Taylor Blau
2026-01-27 7:35 ` Patrick Steinhardt
2026-01-27 22:13 ` Taylor Blau
2026-01-14 19:55 ` [PATCH v2 18/18] midx: enable reachability bitmaps during " Taylor Blau
2026-02-20 22:24 ` [PATCH v2 00/18] midx: incremental MIDX/bitmap layer compaction Junio C Hamano
2026-02-23 14:08 ` Jeff King
2026-02-24 5:25 ` Taylor Blau
2026-02-24 18:59 ` [PATCH v3 00/17] " Taylor Blau
2026-02-24 18:59 ` [PATCH v3 01/17] midx: mark `get_midx_checksum()` arguments as const Taylor Blau
2026-02-24 18:59 ` [PATCH v3 02/17] midx: rename `get_midx_checksum()` to `midx_get_checksum_hash()` Taylor Blau
2026-02-24 18:59 ` [PATCH v3 03/17] midx: introduce `midx_get_checksum_hex()` Taylor Blau
2026-02-24 18:59 ` [PATCH v3 04/17] builtin/multi-pack-index.c: make '--progress' a common option Taylor Blau
2026-02-24 18:59 ` [PATCH v3 05/17] git-multi-pack-index(1): remove non-existent incompatibility Taylor Blau
2026-02-24 18:59 ` [PATCH v3 06/17] git-multi-pack-index(1): align SYNOPSIS with 'git multi-pack-index -h' Taylor Blau
2026-02-24 19:00 ` [PATCH v3 07/17] t/t5319-multi-pack-index.sh: fix copy-and-paste error in t5319.39 Taylor Blau
2026-02-24 19:00 ` [PATCH v3 08/17] midx-write.c: don't use `pack_perm` when assigning `bitmap_pos` Taylor Blau
2026-02-24 19:00 ` [PATCH v3 09/17] midx-write.c: introduce `struct write_midx_opts` Taylor Blau
2026-02-24 19:00 ` [PATCH v3 10/17] midx: do not require packs to be sorted in lexicographic order Taylor Blau
2026-02-24 19:00 ` [PATCH v3 11/17] midx-write.c: introduce `midx_pack_perm()` helper Taylor Blau
2026-02-24 19:00 ` [PATCH v3 12/17] midx-write.c: extract `fill_pack_from_midx()` Taylor Blau
2026-02-24 19:00 ` [PATCH v3 13/17] midx-write.c: enumerate `pack_int_id` values directly Taylor Blau
2026-02-24 19:00 ` [PATCH v3 14/17] midx-write.c: factor fanout layering from `compute_sorted_entries()` Taylor Blau
2026-02-24 19:00 ` [PATCH v3 15/17] t/helper/test-read-midx.c: plug memory leak when selecting layer Taylor Blau
2026-02-24 19:00 ` [PATCH v3 16/17] midx: implement MIDX compaction Taylor Blau
2026-02-24 19:00 ` [PATCH v3 17/17] midx: enable reachability bitmaps during " Taylor Blau
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=aTcYU_yVYyXL9TXv@pks.im \
--to=ps@pks$(echo .)im \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=me@ttaylorr$(echo .)com \
--cc=newren@gmail$(echo .)com \
--cc=peff@peff$(echo .)net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox