Re: [PATCH v3 13/13] midx: implement writing incremental MIDX bitmaps

[Taylor Blau <me@ttaylorr.com>]

On Fri, Feb 28, 2025 at 11:01:39AM +0100, Patrick Steinhardt wrote:
> On Tue, Nov 19, 2024 at 05:07:56PM -0500, Taylor Blau wrote:
> > diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c
> > index 49758e2525f..1fbebe84479 100644
> > --- a/pack-bitmap-write.c
> > +++ b/pack-bitmap-write.c
> > @@ -25,6 +25,8 @@
> >  #include "alloc.h"
> >  #include "refs.h"
> >  #include "strmap.h"
> > +#include "midx.h"
> > +#include "pack-revindex.h"
>
> Nit: let's keep the headers sorted alphabetically.
>
> > @@ -206,19 +215,37 @@ void bitmap_writer_push_commit(struct bitmap_writer *writer,
> >  static uint32_t find_object_pos(struct bitmap_writer *writer,
> >  				const struct object_id *oid, int *found)
> >  {
> > -	struct object_entry *entry = packlist_find(writer->to_pack, oid);
> > +	struct object_entry *entry;
> > +
> > +	entry = packlist_find(writer->to_pack, oid);
> > +	if (entry) {
> > +		uint32_t base_objects = 0;
> > +		if (writer->midx)
> > +			base_objects = writer->midx->num_objects +
> > +				writer->midx->num_objects_in_base;
> > +
> > +		if (found)
> > +			*found = 1;
> > +		return oe_in_pack_pos(writer->to_pack, entry) + base_objects;
> > +	} else if (writer->midx) {
> > +		uint32_t at, pos;
> > +
> > +		if (!bsearch_midx(oid, writer->midx, &at))
> > +			goto missing;
> > +		if (midx_to_pack_pos(writer->midx, at, &pos) < 0)
> > +			goto missing;
> >
> > -	if (!entry) {
> >  		if (found)
> > -			*found = 0;
> > -		warning("Failed to write bitmap index. Packfile doesn't have full closure "
> > -			"(object %s is missing)", oid_to_hex(oid));
> > -		return 0;
> > +			*found = 1;
> > +		return pos;
> >  	}
> >
> > +missing:
> >  	if (found)
> > -		*found = 1;
> > -	return oe_in_pack_pos(writer->to_pack, entry);
> > +		*found = 0;
> > +	warning("Failed to write bitmap index. Packfile doesn't have full closure "
> > +		"(object %s is missing)", oid_to_hex(oid));
>
> Is this warning still accurate? I assume that in the MIDX case we don't
> have to have full closure in a single packfile, as that would otherwise
> make the whole thing rather moot.

Good question -- the warning is still "accurate" in the sense that the
thing we're generating the bitmap against has to have full object
closure under reachability.

It's important to note that even though in any individual layer we are
only selecting bitmaps from the set of commits in the union of packs in
the corresponding MIDX layer, they may well reference objects from
earlier layers. And there it isn't important that we find some reachable
object in the same MIDX layer, but rather that we find it in an earlier
layer or the current layer.

It's less than accurate in the sense that it says "Packfile doesn't have
full closure" and isn't marked for translation, but I punted on it here.
(This is definitely not the first time I've had the thought to change
it, so maybe I just should have done it here.)

Thanks,
Taylor