On Fri, Aug 31, 2018 at 05:23:17PM +0200, Ævar Arnfjörð Bjarmason wrote:
> On Tue, Aug 21 2018, Jeff King wrote:
>
> > +int bitmap_has_sha1_in_uninteresting(struct bitmap_index *bitmap_git,
> > + const unsigned char *sha1)
> > +{
> > + int pos;
> > +
> > + if (!bitmap_git)
> > + return 0; /* no bitmap loaded */
> > + if (!bitmap_git->result)
> > + BUG("failed to perform bitmap walk before querying");
>
> Some part of what calls this completely breaks pushing from the "next"
> branch when you have local bitmaps (we *really* should have some tests
> for this...).
Yikes, thanks for reporting. I agree we need better tests here.
This assertion is totally bogus, as traverse_bitmap_commit_list() will
actually free (and NULL) the result field! This is a holdover from the
original bitmap code, where bitmap_git was a static, and this is how you
might prepare a second walk (though AFAIK only ever do one walk per
process). These days prepare_bitmap_walk() actually returns a fresh
bitmap_index struct.
So there's no way to know if traverse_bitmap_commit_list() was called.
But as it turns out, we don't care. We want to know if
"bitmap_git->have" was set up, which is done in prepare_bitmap_walk().
So there's no way[1] to get a bitmap_git that hasn't been properly set
up. This BUG() check can just go away.
I'll prepare a patch and some tests later tonight.
-Peff
[1] Actually, there is also prepare_bitmap_git(), but it is not really
for general use by callers. It should be made static, or better yet,
I suspect it can be folded into its callers.
