Re: [PATCH v2 03/15] rev-list: fallback to non-bitmap traversal when filtering

[Taylor Blau <me@ttaylorr.com>]

On Fri, Feb 14, 2020 at 01:22:13PM -0500, Jeff King wrote:
> The "--use-bitmap-index" option is usually aspirational: if we have
> bitmaps and the request can be fulfilled more quickly using them we'll
> do so, but otherwise fall back to a non-bitmap traversal.
>
> The exception is object filtering, which explicitly dies if the two
> options are combined. Let's convert this to the usual fallback behavior.
> This is a minor convenience for now (since the caller can easily know
> that --filter and --use-bitmap-index don't combine), but will become
> much more useful as we start to support _some_ filters with bitmaps, but
> not others.

Yeah, I think that this convenience is worth it early on instead of
lumping in these changes in a future patch.

> The test infrastructure here is bigger than necessary for checking this
> one small feature. But it will serve as the basis for more filtering
> bitmap tests in future patches.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/rev-list.c                 |  4 ++--
>  t/t6113-rev-list-bitmap-filters.sh | 24 ++++++++++++++++++++++++
>  2 files changed, 26 insertions(+), 2 deletions(-)
>  create mode 100755 t/t6113-rev-list-bitmap-filters.sh
>
> diff --git a/builtin/rev-list.c b/builtin/rev-list.c
> index e28d62ec64..bce406bd1e 100644
> --- a/builtin/rev-list.c
> +++ b/builtin/rev-list.c
> @@ -521,8 +521,8 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
>  	if (revs.show_notes)
>  		die(_("rev-list does not support display of notes"));
>
> -	if (filter_options.choice && use_bitmap_index)
> -		die(_("cannot combine --use-bitmap-index with object filtering"));
> +	if (filter_options.choice)
> +		use_bitmap_index = 0;
>
>  	save_commit_buffer = (revs.verbose_header ||
>  			      revs.grep_filter.pattern_list ||
> diff --git a/t/t6113-rev-list-bitmap-filters.sh b/t/t6113-rev-list-bitmap-filters.sh
> new file mode 100755
> index 0000000000..977f8d0930
> --- /dev/null
> +++ b/t/t6113-rev-list-bitmap-filters.sh
> @@ -0,0 +1,24 @@
> +#!/bin/sh
> +
> +test_description='rev-list combining bitmaps and filters'
> +. ./test-lib.sh
> +
> +test_expect_success 'set up bitmapped repo' '
> +	# one commit will have bitmaps, the other will not
> +	test_commit one &&
> +	git repack -adb &&
> +	test_commit two
> +'
> +
> +test_expect_success 'filters fallback to non-bitmap traversal' '
> +	# use a path-based filter, since they are inherently incompatible with
> +	# bitmaps (i.e., this test will never get confused by later code to
> +	# combine the features)
> +	filter=$(echo "!one" | git hash-object -w --stdin) &&
> +	git rev-list --objects --filter=sparse:oid=$filter HEAD >expect &&
> +	git rev-list --use-bitmap-index \
> +		     --objects --filter=sparse:oid=$filter HEAD >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_done
> --
> 2.25.0.796.gcc29325708

Makes sense.

Thanks,
Taylor