On Thu, Feb 13, 2020 at 02:22:07PM -0800, Junio C Hamano wrote:
> Jeff King <peff@xxxxxxxx> writes:
>
> > This just passes the filter-options struct to prepare_bitmap_walk().
> > Since the bitmap code doesn't actually support any filters yet, it will
> > fallback to the non-bitmap code if any --filter is specified. But this
> > lets us exercise that rejection code path, as well as getting us ready
> > to test filters via rev-list when we _do_ support them.
>
> So we used to look at filter_options.choice and declared any filter
> is incompatible with use_bitmap_index quite early, but now we let
> each of the try_bitmap_*() helpers check what is in the filter and
> make their own decisions.
>
> Of course, the prepare_bitmap_walk() call at the beginning of these
> helpers does not know how to work with any filter at this point in
> the series, so all of the above cancel out :-).
>
> Makes sense.
>
> I wonder if the "revs.prune" thing that forces use_bitmap_index off
> should also move to prepare_bitmap_walk() at some point in the
> series (or after the current series is done). After all, the point
> of introducing try_bitmap_*() helpers was to let these bitmap
> specific logic to know what is and is not compatible with the bitmap
> routines.
Ah, interesting thought. Yeah, we could push it down to that level to
avoid rev-list having to know details about how the bitmap code works.
That could replace the earlier patch to consolidate the filter/prune
logic. And then in this patch, this hunk:
> > @@ -612,7 +614,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
> > (revs.left_right || revs.cherry_mark))
> > die(_("marked counting is incompatible with --objects"));
> >
> > - if (filter_options.choice || revs.prune)
> > + if (revs.prune)
> > use_bitmap_index = 0;
would just drop this conditional entirely. I like it.
-Peff
