Re: [PATCH v2 02/24] pack-bitmap-write.c: gracefully fail to write non-closed bitmaps
From: Jeff King <hidden>
Date: 2021-07-21 10:04:03
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:
quoted hunk ↗ jump to 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