Re: [PATCH v4 02/13] pack-revindex: prepare for incremental MIDX bitmaps

From: Jeff King <peff@peff.net>
Date: Mar 18, 2025, 01:27 UTC
Message-ID: <20250318012726.GB1471939@coredump.intra.peff.net>
In-Reply-To: <f2a232e556a066c1a5083f02584ddc3637ecfc48.1741983492.git.me@ttaylorr.com>

On Fri, Mar 14, 2025 at 04:18:24PM -0400, Taylor Blau wrote:

Show 16 quoted lines

> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index 6406953d32..c26d85b5db 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -170,6 +170,15 @@ static struct ewah_bitmap *read_bitmap_1(struct bitmap_index *index)
>  	return read_bitmap(index->map, index->map_size, &index->map_pos);
>  }
>  
> +static uint32_t bitmap_non_extended_bits(struct bitmap_index *index)
> +{
> +	if (index->midx) {
> +		struct multi_pack_index *m = index->midx;
> +		return m->num_objects + m->num_objects_in_base;
> +	}
> +	return index->pack->num_objects;
> +}

I understand why we need to account for the objects in the base to offset our total size.

Similar to Patrick's comments on v3, I wondered about why we couldn't just modify bitmap_num_objects() here, and why some callers would be left with the other.

I guess sometimes we still need to consider a single layer. We can't quite just access m->num_objects there, because we still need the midx vs pack abstraction layer. I just thought there'd be more discussion here, but it looks the same as v3.

I wonder if it is worth renaming bitmap_num_objects() to indicate that it is a single layer (and make sure other callers are examined). I dunno.

I also suspect from previous forays into bitmap indexing that it will be easy to mix up positions in various units (local to the layer vs in the global pseudo-pack ordering, for example). In theory we could use types to help us with this, but they're kind of weak in C (unless we wrap all of the ints in structs). Maybe not worth it.

-Peff