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

From: Taylor Blau <[email-masked]>
Date: Thu, 27 Feb 2025 18:26:31 -0500

On Tue, Feb 25, 2025 at 07:59:14AM +0100, Patrick Steinhardt wrote:
> On Mon, Feb 24, 2025 at 10:05:27AM -0800, Junio C Hamano wrote:
> > Patrick Steinhardt <[email-masked]> 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 <[email-masked]>
> > > ---
> > >  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.
>
> Hm, does it? I understand that this commit only makes the bitmaps
> accessible individually per bitmapped packfile, but the bitmap indices
> part of `struct bitmap_index` would continue to be the union of all of
> those bitmaps. Oh, but that changes in the subsequent commits indeed,
> where we start to use an `ewah_or_iterator`.

That's right; the ewah_or_iterator is the mechanism by which we can
combine multiple "layers" of the bitmaps into a single iterator.

(As an aside, that was not the first approach I pursued. Initially the
caller was supposed to chase the 'next' pointer of each bitmap and
enumerate through whatever type iterator they're interested in at each
layer. But that was too error-prone, since you have to remember and
update the offset into the pseudo-pack order across multiple layers.)

> I see that Taylor's series has been sitting in an unreviewed state for a
> couple months already. I can review it with the hope of moving it
> forward and can then pull it in as a dependency of this series. But I'll
> wait for him to chime in first to see whether anything changed about its
> current state.

It would be great to get some review from you on that series. I know
that it has been on Peff's (CC'd) radar for a while, but that he has
likewise had a few off-list things to deal with lately as well.

I am still not sold on introducing a callback here, though, and would
much rather see callers interact with the iterator directly.

Thanks,
Taylor