Re: [PATCH v3 04/13] pack-bitmap.c: teach `bitmap_for_commit()` about incremental MIDXs

[Taylor Blau <me@ttaylorr.com>]

On Fri, Feb 28, 2025 at 11:01:19AM +0100, Patrick Steinhardt wrote:
> On Tue, Nov 19, 2024 at 05:07:29PM -0500, Taylor Blau wrote:
> > diff --git a/pack-bitmap.c b/pack-bitmap.c
> > index 41675a69f68..e3fdcf8a01a 100644
> > --- a/pack-bitmap.c
> > +++ b/pack-bitmap.c
> > @@ -946,18 +946,21 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_
> >  struct ewah_bitmap *bitmap_for_commit(struct bitmap_index *bitmap_git,
> >  				      struct commit *commit)
> >  {
> > -	khiter_t hash_pos = kh_get_oid_map(bitmap_git->bitmaps,
> > -					   commit->object.oid);
> > +	khiter_t hash_pos;
> > +	if (!bitmap_git)
> > +		return NULL;
> > +
> > +	hash_pos = kh_get_oid_map(bitmap_git->bitmaps, commit->object.oid);
> >  	if (hash_pos >= kh_end(bitmap_git->bitmaps)) {
> >  		struct stored_bitmap *bitmap = NULL;
> >  		if (!bitmap_git->table_lookup)
> > -			return NULL;
> > +			return bitmap_for_commit(bitmap_git->base, commit);
> >
> >  		/* this is a fairly hot codepath - no trace2_region please */
> >  		/* NEEDSWORK: cache misses aren't recorded */
> >  		bitmap = lazy_bitmap_for_commit(bitmap_git, commit);
> >  		if (!bitmap)
> > -			return NULL;
> > +			return bitmap_for_commit(bitmap_git->base, commit);
> >  		return lookup_stored_bitmap(bitmap);
> >  	}
> >  	return lookup_stored_bitmap(kh_value(bitmap_git->bitmaps, hash_pos));
>
> One of the things that worries me a bit is that by recursing, we
> essentially are bound in the depth of MIDX layers as we may otherwise
> bust the stack. Not that I expect us to typically have thousands of
> layers, but if there ever was a bug this may fail in bad ways.

I think it's a valid concern, but in practice I suspect we are unlikely
to run into it. If we have enough MIDX layers to blow the stack, we
probably have much bigger problems to worry about ;-).

> I already asked this for a previous commit, but what is the current
> state regarding compaction of the layers? Do we need to be worried about
> this or do we already know to keep things limited in general?

I mentioned upthread, but briefly: we don't compact MIDX layers today,
but the design is such that it is possible (and planned) to do in the
future (part three of this multi-series thing).

Thanks,
Taylor