From: Jeff King <firstname.lastname@example.org> To: "Ævar Arnfjörð Bjarmason" <email@example.com> Cc: Stefan Beller <firstname.lastname@example.org>, email@example.com, Junio C Hamano <firstname.lastname@example.org> Subject: [PATCH 1/4] bitmap_has_sha1_in_uninteresting(): drop BUG check Date: Sat, 1 Sep 2018 03:44:48 -0400 [thread overview] Message-ID: <20180901074448.GA25461@sigill.intra.peff.net> (raw) In-Reply-To: <20180901074145.GA24023@sigill.intra.peff.net> 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 <email@example.com> --- pack-bitmap.c | 2 -- pack-bitmap.h | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/pack-bitmap.c b/pack-bitmap.c index c3231ef9ef..76fd93a3de 100644 --- a/pack-bitmap.c +++ b/pack-bitmap.c @@ -1128,8 +1128,6 @@ int bitmap_has_sha1_in_uninteresting(struct bitmap_index *bitmap_git, if (!bitmap_git) return 0; /* no bitmap loaded */ - if (!bitmap_git->result) - BUG("failed to perform bitmap walk before querying"); if (!bitmap_git->haves) return 0; /* walk had no "haves" */ diff --git a/pack-bitmap.h b/pack-bitmap.h index c633bf5238..189dd68ad3 100644 --- a/pack-bitmap.h +++ b/pack-bitmap.h @@ -54,7 +54,7 @@ int rebuild_existing_bitmaps(struct bitmap_index *, struct packing_data *mapping void free_bitmap_index(struct bitmap_index *); /* - * After a traversal has been performed on the bitmap_index, this can be + * After a traversal has been performed by prepare_bitmap_walk(), this can be * queried to see if a particular object was reachable from any of the * objects flagged as UNINTERESTING. */ -- 2.19.0.rc1.549.g6ce4dc0f08
next prev parent reply other threads:[~2018-09-01 7:44 UTC|newest] Thread overview: 53+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-08-21 18:41 [PATCH] test-tool.h: include git-compat-util.h Jeff King 2018-08-21 19:03 ` Junio C Hamano 2018-08-21 19:06 ` [PATCH 1/6] t/perf: factor boilerplate out of test_perf Jeff King 2018-08-21 19:06 ` [PATCH 2/6] t/perf: factor out percent calculations Jeff King 2018-08-21 19:06 ` [PATCH 3/6] t/perf: add infrastructure for measuring sizes Jeff King 2018-08-22 13:40 ` Derrick Stolee 2018-08-22 15:31 ` Jeff King 2018-08-21 19:06 ` [PATCH 4/6] t/perf: add perf tests for fetches from a bitmapped server Jeff King 2018-08-21 19:07 ` [PATCH 5/6] pack-bitmap: save "have" bitmap from walk Jeff King 2018-08-21 19:47 ` Derrick Stolee 2018-08-21 19:54 ` Jeff King 2018-08-31 15:23 ` Ævar Arnfjörð Bjarmason 2018-08-31 22:55 ` Jeff King 2018-09-01 7:41 ` [PATCH 0/4] un-breaking pack-objects with bitmaps Jeff King 2018-09-01 7:44 ` Jeff King [this message] 2018-09-01 7:48 ` [PATCH 2/4] t5310: test delta reuse " Jeff King 2018-09-01 8:03 ` Jeff King 2018-09-01 20:29 ` Ævar Arnfjörð Bjarmason 2018-09-01 22:46 ` Ben Peart 2018-09-02 5:51 ` Jeff King 2018-09-04 19:05 ` Stefan Beller 2018-09-04 19:45 ` Junio C Hamano 2018-09-04 20:02 ` Jeff King 2018-09-01 7:49 ` [PATCH 3/4] traverse_bitmap_commit_list(): don't free result Jeff King 2018-09-01 7:50 ` [PATCH 4/4] pack-bitmap: drop "loaded" flag Jeff King 2018-09-04 19:30 ` [PATCH 0/4] un-breaking pack-objects with bitmaps Stefan Beller 2018-09-04 20:03 ` Jeff King 2018-09-08 6:43 ` Ævar Arnfjörð Bjarmason 2018-09-10 16:53 ` Junio C Hamano 2018-09-10 18:48 ` Jeff King 2018-09-10 19:23 ` Junio C Hamano 2018-08-21 19:07 ` [PATCH 6/6] pack-objects: reuse on-disk deltas for thin "have" objects Jeff King 2018-08-21 19:43 ` Junio C Hamano 2018-08-21 19:50 ` Junio C Hamano 2018-08-21 20:07 ` Jeff King 2018-08-21 20:14 ` Jeff King 2018-08-21 20:52 ` Junio C Hamano 2018-08-21 21:30 ` Jeff King 2018-08-21 20:57 ` Junio C Hamano 2018-08-21 21:32 ` Jeff King 2018-08-23 0:43 ` [PATCH 0/9] trailer-parsing false positives Jeff King 2018-08-23 0:44 ` [PATCH 1/9] trailer: use size_t for string offsets Jeff King 2018-08-23 0:45 ` [PATCH 2/9] trailer: use size_t for iterating trailer list Jeff King 2018-08-23 0:46 ` [PATCH 3/9] trailer: pass process_trailer_opts to trailer_info_get() Jeff King 2018-08-23 0:48 ` [PATCH 4/9] interpret-trailers: tighten check for "---" patch boundary Jeff King 2018-08-23 0:49 ` [PATCH 5/9] interpret-trailers: allow suppressing "---" divider Jeff King 2018-08-23 0:50 ` [PATCH 6/9] pretty, ref-filter: format %(trailers) with no_divider option Jeff King 2018-08-23 0:50 ` [PATCH 7/9] sequencer: ignore "---" divider when parsing trailers Jeff King 2018-08-23 0:50 ` [PATCH 8/9] append_signoff: use size_t for string offsets Jeff King 2018-08-23 0:51 ` [PATCH 9/9] sequencer: handle ignore_footer when parsing trailers Jeff King 2018-08-23 18:30 ` [PATCH 0/9] trailer-parsing false positives Junio C Hamano 2018-08-24 7:26 ` Jeff King 2018-08-21 20:00 ` [PATCH 6/6] pack-objects: reuse on-disk deltas for thin "have" objects Jeff King
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20180901074448.GA25461@sigill.intra.peff.net \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --subject='Re: [PATCH 1/4] bitmap_has_sha1_in_uninteresting(): drop BUG check' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).