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

From: Taylor Blau <me@ttaylorr.com>
Date: Feb 27, 2025, 23:26 UTC
Message-ID: <Z8D0p8oKEmMqmzrJ@nand.local>
In-Reply-To: <Z71qQvlqtoOwDabu@pks.im>

On Tue, Feb 25, 2025 at 07:59:14AM +0100, Patrick Steinhardt wrote:

Show 28 quoted lines

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

Show 5 quoted lines

> 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