Re: [PATCH 7/9] pack-bitmap: introduce function to check whether a pack is bitmapped

[Taylor Blau <me@ttaylorr.com>]

On Fri, Feb 21, 2025 at 08:47:32AM +0100, Patrick Steinhardt wrote:
> 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 <ps@pks.im>
> ---
>  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