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
