Re: [PATCH v2 7/8] pack-bitmap: implement combined filter

From: Jeff King <peff@peff.net>
To: Patrick Steinhardt <ps@pks.im>
Cc: git@vger.kernel.org,
	Christian Couder <christian.couder@gmail.com>,
	Taylor Blau <me@ttaylorr.com>
Subject: Re: [PATCH v2 7/8] pack-bitmap: implement combined filter
Date: Fri, 9 Apr 2021 11:53:11 -0400	[thread overview]
Message-ID: <YHB4ZytlI8XI+4N1@coredump.intra.peff.net> (raw)
In-Reply-To: <YHAtDp11xFzdUSQl@ncase>

On Fri, Apr 09, 2021 at 12:31:42PM +0200, Patrick Steinhardt wrote:

> > Hmm. This is essentially reproducing the list in filter_bitmap() of
> > what's OK for bitmaps. So when adding a new filter, it would have to be
> > added in both places.
> > 
> > Can we preserve that property of the original code? I'd think that just
> > adding LOFC_COMBINE to filter_bitmap() would be sufficient. I.e., this
> > hunk:
> > 
> > > +	if (filter->choice == LOFC_COMBINE) {
> > > +		int i;
> > > +		for (i = 0; i < filter->sub_nr; i++) {
> > > +			filter_bitmap(bitmap_git, tip_objects, to_filter,
> > > +				      &filter->sub[i]);
> > > +		}
> > > +		return 0;
> > > +	}
> > 
> > ...except that we need to see if filter_bitmap() returns "-1" for any of
> > the recursive calls. Which we probably should be doing anyway to
> > propagate any errors (though I think the only "errors" we'd return are
> > "not supported", at least for now).
> > 
> > -Peff
> 
> But wouldn't that mean that we're now needlessly filtering via bitmaps
> all the way down the combined filters only to realize at the end that it
> cannot work because we've got a tree filter with non-zero tree depth?
> Granted, this will not be the common case. But it still feels like we're
> doing needless work for cases where we know that bitmaps cannot answer
> the query.

I don't think so. We first call can_filter_bitmap(filter), which passes
NULL for bitmap_git. And then in filter_bitmap(), we only do actual work
if bitmap_git is non-NULL.

This is the same thing that saves us from even loading the bitmaps
(which is itself a non-trivial amount of work) if the filter cannot be
satisfied by bitmaps.

-Peff