Re: [PATCH 7/9] pack-bitmap: introduce function to check whether a pack is bitmapped
From: Taylor Blau <hidden>
Date: 2025-02-27 23:33:47
On Fri, Feb 21, 2025 at 08:47:32AM +0100, Patrick Steinhardt wrote:
quoted hunk ↗ jump to hunk
Introduce a function that allows us to verify whether a pack is bitmapped or not. This functionality will be used in a subsequent commit. Signed-off-by: Patrick Steinhardt <redacted> --- pack-bitmap.c | 15 +++++++++++++++ pack-bitmap.h | 7 +++++++ 2 files changed, 22 insertions(+)diff --git a/pack-bitmap.c b/pack-bitmap.c index fc92e0aae65..3cbe5bfe909 100644 --- a/pack-bitmap.c +++ b/pack-bitmap.c@@ -658,6 +658,21 @@ struct bitmap_index *prepare_midx_bitmap_git(struct multi_pack_index *midx) return NULL; } +int bitmap_index_contains_pack(struct bitmap_index *bitmap, struct packed_git *pack) +{ + if (bitmap->pack) + return bitmap->pack == pack;
The bitmap_is_midx() function should be useful here. I don't think what you wrote is wrong per-se, but that function is supposed to "hide" what exactly constitutes a pack versus multi-pack bitmap.
+ if (!bitmap->midx->chunk_bitmapped_packs) + return 0;
What is the purpose of this check? The BTMP chunk was a relatively recent addition, but it came long after multi-pack bitmaps were first introduced. The BTMP chunk is necessary for multi-pack reuse, since it indicates what sections of the bitmap's object order correspond to what packs. With or without a BTMP chunk in the MIDX, a multi-pack bitmap is assumed to cover all of the packs in that MIDx. So I think the above check is at best not helpful, and at worst will return incorrect results for pre-BTMP MIDXs.
+ for (size_t i = 0; i < bitmap->midx->num_packs; i++) + if (bitmap->midx->packs[i] == pack) + return 1;
This part looks good to me. If you end up pulling in the incremental
MIDX bitmaps series in as a dependency of this one, this will have to be
rewritten something like:
for (; bitmap; bitmap = bitmap->base) {
if (bitmap_is_midx(bitmap)) {
for (size_t i = 0; i < bitmap->midx->num_packs; i++) {
if (bitmap->midx->packs[i] == pack)
return 1;
}
} else if (bitmap->pack == pack) {
return 1;
}
}
return 0;
Without pulling in that series as a dependency of this one, I think the
function would just contain the body of the above 'for' loop, but not
the loop itself.
Thanks,
Taylor