Re: [PATCH v2 08/10] pack-bitmap: introduce function to check whether a pack is bitmapped

[Patrick Steinhardt <ps@pks.im>]

On Tue, Apr 01, 2025 at 01:46:09PM +0200, Toon Claes wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > Introduce a function that allows us to verify whether a pack is
> > bitmapped or not. This functionality will be used in a subsequent
> > commit.
> >
> > Helped-by: Taylor Blau <me@ttaylorr.com>
> > 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 6adb8aaa1c2..edc8f42122d 100644
> > --- a/pack-bitmap.c
> > +++ b/pack-bitmap.c
> > @@ -745,6 +745,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)
> > +{
> > +	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) {
> 
> Here, and two lines above, we compare packs by their pointer address,
> this doesn't seem to be common practice to me. Or is it in the Git
> codebase? Do we expect any problems with this, for example when we stop
> using `the_repository`?

I don't expect any problems unless we have multiple `struct repository`
instances pointing to the same underlying repository. We never do that
to the best of my knowledge though, and it would feel somewhat broken if
we ever started to do that.

If we had structs pointing to different repositories though this will do
the right thing as a packfile from repository A shouldn't be indexed by
repository B.

Patrick