Re: [PATCH v3 03/13] pack-bitmap.c: open and store incremental bitmap layers
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Feb 28, 2025, 10:01 UTC
- Message-ID
- <Z8GJbOoY9Z3VMSEM@pks.im>
- In-Reply-To
- <5b5d625cbe02560a20c12b7dd20aeda4979017bb.1732054032.git.me@ttaylorr.com>
On Tue, Nov 19, 2024 at 05:07:26PM -0500, Taylor Blau wrote:
Show 6 quoted lines
> Prepare the pack-bitmap machinery to work with incremental MIDXs by > adding a new "base" field to keep track of the bitmap index associated > with the previous MIDX layer. > > The changes in this commit are mostly boilerplate to open the correct > bitmap(s), add them to the chain bitmap layers along the "base" pointer,
s/bitmap layers/of &/
Show 15 quoted lines
> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index bba9c6a905a..41675a69f68 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -54,6 +54,13 @@ struct bitmap_index {
> struct packed_git *pack;
> struct multi_pack_index *midx;
>
> + /*
> + * If using a multi-pack index chain, 'base' points to the
> + * bitmap index corresponding to this bitmap's midx->base_midx.
> + */
> + struct bitmap_index *base;
> + uint32_t base_nr;
> +It would be nice to point out that `base_nr` is not 0-indexed, but 1-indexed, which is rather uncommon. Is there any particular reason why you made it 1-indexed?
Show 16 quoted lines
> @@ -377,8 +384,13 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)
> char *midx_bitmap_filename(struct multi_pack_index *midx)
> {
> struct strbuf buf = STRBUF_INIT;
> - get_midx_filename_ext(&buf, midx->object_dir, get_midx_checksum(midx),
> - MIDX_EXT_BITMAP);
> + if (midx->has_chain)
> + get_split_midx_filename_ext(&buf, midx->object_dir,
> + get_midx_checksum(midx),
> + MIDX_EXT_BITMAP);
> + else
> + get_midx_filename_ext(&buf, midx->object_dir,
> + get_midx_checksum(midx), MIDX_EXT_BITMAP);
>
> return strbuf_detach(&buf, NULL);
> }Okay, this is mostly the same change as in the preceding commit, but for bitmaps instead of reverse indices.
Show 16 quoted lines
> @@ -397,10 +409,17 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
> {
> struct stat st;
> char *bitmap_name = midx_bitmap_filename(midx);
> - int fd = git_open(bitmap_name);
> + int fd;
> uint32_t i, preferred_pack;
> struct packed_git *preferred;
>
> + fd = git_open(bitmap_name);
> + if (fd < 0 && errno == ENOENT) {
> + FREE_AND_NULL(bitmap_name);
> + bitmap_name = midx_bitmap_filename(midx);
> + fd = git_open(bitmap_name);
> + }
> +Wait, this looks weird to me. `bitmap_name` already contains the result of `midx_bitmap_filename()`, so you're essentially retrying the exact same operation as before?
Patrick