Re: [PATCH 5/6] pack-bitmap: save "have" bitmap from walk

From: Jeff King <peff@peff.net>
Date: Aug 31, 2018, 22:55 UTC
Message-ID: <20180831225558.GA22917@sigill.intra.peff.net>
In-Reply-To: <8736uud0gq.fsf@evledraar.gmail.com>

On Fri, Aug 31, 2018 at 05:23:17PM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 15 quoted lines

> 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.