bitmap_has_sha1_in_uninteresting(): drop BUG check

Commit 30cdc33fba (pack-bitmap: save "have" bitmap from walk, 2018-08-21) introduced a new function for looking at the "have" side of a bitmap walk. Because it only makes sense to do so after we've finished the walk, we added an extra safety assertion, making sure that bitmap_git->result is non-NULL. However, this safety is misguided. It was trying to catch the case where we had called prepare_bitmap_walk() to give us a "struct bitmap_index", but had not yet called traverse_bitmap_commit_list() to walk it. But all of the interesting computation (including setting up the result and "have" bitmaps) happens in the first function! The latter function only delivers the result to a callback function. So the case we were worried about is impossible; if you get a non-NULL result from prepare_bitmap_walk(), then its "have" field will be fully formed. But much worse, traverse_bitmap_commit_list() actually frees the result field as it finishes. Which means that this assertion is worse than useless: it's almost guaranteed to trigger! Our test suite didn't catch this because the function isn't actually exercised at all. The only caller comes from 6a1e32d532 (pack-objects: reuse on-disk deltas for thin "have" objects, 2018-08-21), and that's triggered only when you fetch or push history that contains an object with a base that is found deep in history. Our test suite fetches and pushes either don't use bitmaps, or use too-small example repositories. But any reasonably-sized real-world push or fetch (with bitmaps) would trigger this. This patch drops the harmful assertion and tweaks the docstring for the function to make the precondition clear. The tests need to be improved to exercise this new pack-objects feature, but we'll do that in a separate commit. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Sep 1, 2018 at 03:44 UTC 5476fb07ebcf389752acef0a894a1eea1ff13ea9
2 files changed +1 -3
pack-bitmap.c
-2
@@ -1128,8 +1128,6 @@ int bitmap_has_sha1_in_uninteresting(struct bitmap_index *bitmap_git,
1128
1129 if (!bitmap_git)
1130 return 0; /* no bitmap loaded */
1131 - if (!bitmap_git->result)
1132 - BUG("failed to perform bitmap walk before querying");
1131 if (!bitmap_git->haves)
1132 return 0; /* walk had no "haves" */
1133
pack-bitmap.h
+1 -1
@@ -54,7 +54,7 @@ int rebuild_existing_bitmaps(struct bitmap_index *, struct packing_data *mapping
54 void free_bitmap_index(struct bitmap_index *);
55
56 /*
57 - * After a traversal has been performed on the bitmap_index, this can be
57 + * After a traversal has been performed by prepare_bitmap_walk(), this can be
58 * queried to see if a particular object was reachable from any of the
59 * objects flagged as UNINTERESTING.
60 */