On Mon, Jun 21, 2021 at 06:25:01PM -0400, Taylor Blau wrote:
> The set of objects covered by a bitmap must be closed under
> reachability, since it must be the case that there is a valid bit
> position assigned for every possible reachable object (otherwise the
> bitmaps would be incomplete).
>
> Pack bitmaps are never written from 'git repack' unless repacking
> all-into-one, and so we never write non-closed bitmaps (except in the
> case of partial clones where we aren't guaranteed to have all objects).
>
> But multi-pack bitmaps change this, since it isn't known whether the
> set of objects in the MIDX is closed under reachability until walking
> them. Plumb through a bit that is set when a reachable object isn't
> found.
>
> As soon as a reachable object isn't found in the set of objects to
> include in the bitmap, bitmap_writer_build() knows that the set is not
> closed, and so it now fails gracefully.
Leaving aside your intended use here, I think it's nice to get rid of a
deep-buried die() like this in general.
The amount of error-plumbing you had to do is a little unpleasant, but I
think is unavoidable. The only non-obvious part was this hunk:
> @@ -463,8 +488,11 @@ void bitmap_writer_build(struct packing_data *to_pack)
> struct commit *child;
> int reused = 0;
>
> - fill_bitmap_commit(ent, commit, &queue, &tree_queue,
> - old_bitmap, mapping);
> + if (fill_bitmap_commit(ent, commit, &queue, &tree_queue,
> + old_bitmap, mapping) < 0) {
> + closed = 0;
> + break;
> + }
>
> if (ent->selected) {
> store_selected(ent, commit);
This is the right thing to do because we still want to free memory, stop
progress, etc. I gave a look over what will run after breaking out of
the loop, and compute_xor_offsets(), which you already handled, is the
only thing we'd want to avoid running. Good.
-Peff
