Re: [PATCH 6/9] pack-bitmap: expose function to iterate over bitmapped objects

[Taylor Blau <me@ttaylorr.com>]

On Mon, Feb 24, 2025 at 10:05:27AM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > Expose a function that allows the caller to iterate over all bitmapped
> > objects of a specific type. This mechanism allows us to use the object
> > type-specific bitmaps to enumerate all objects of that type without
> > having to scan through a complete packfile.
> >
> > This functionality will be used in a subsequent commit.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  builtin/pack-objects.c |  3 ++-
> >  builtin/rev-list.c     |  3 ++-
> >  pack-bitmap.c          | 65 +++++++++++++++++++++++++++++++-------------------
> >  pack-bitmap.h          | 12 +++++++++-
> >  reachable.c            |  3 ++-
> >  5 files changed, 57 insertions(+), 29 deletions(-)
>
> After 2189649b (pack-bitmap.c: keep track of each layer's type
> bitmaps, 2024-11-19) added <type>_all bitmaps to the bitmap_index
> struct, this step would need some adjustment, I am afraid.
>
> Taylor Cc'ed.

Thanks, I was going to respond with the same thing.

I was going to suggest leaving that function as-is to prevent future
breakage and/or a messy integration into 'seen'. But stepping back I am
not sure I understand the purpose of this commit in the first place.

It looks like the aim here is to introduce a function which executes a
callback for each object of some type in a bitmap. That's a thin wrapper
over the ewah_iterator, but it's not clear why we need a wrapper around
that function since it is internal to pack-bitmap.c. Likewise, this is a
performance critical area, so I am not sure I'm in favor of adding a
function pointer to a hot path which executes once per object for some
object type.

Thanks,
Taylor