Re: [PATCH v3 03/13] pack-bitmap.c: open and store incremental bitmap layers

[Patrick Steinhardt <ps@pks.im>]

On Tue, Nov 19, 2024 at 05:07:26PM -0500, Taylor Blau wrote:
> 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 &/

> 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?

> @@ -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.

> @@ -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