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

[Patrick Steinhardt <ps@pks.im>]

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 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?

Patrick