[03/13] rev-list: fallback to non-bitmap traversal when filtering
diff mbox series

Message ID 20200213021730.GC1126038@coredump.intra.peff.net
State New
Headers show
Series
  • combining object filters and bitmaps
Related show

Commit Message

Jeff King Feb. 13, 2020, 2:17 a.m. UTC
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.

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

Comments

Junio C Hamano Feb. 13, 2020, 6:19 p.m. UTC | #1
Jeff King <peff@peff.net> writes:

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

Makes sense.  

Perhaps the option should have been called allow-bitmap-index or
something along that line, but it is too late ;-)

> +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
> +'

OK.
Jeff King Feb. 13, 2020, 6:40 p.m. UTC | #2
On Thu, Feb 13, 2020 at 10:19:19AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > 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.
> 
> Makes sense.  
> 
> Perhaps the option should have been called allow-bitmap-index or
> something along that line, but it is too late ;-)

Yeah. It's also annoyingly long to type, and makes for long lines in the
test scripts. ;)

There are also some weird semantics with the fallback, because the
output may differ depending on whether we use bitmaps (see one of the
later patches). I wouldn't be opposed to cleaning this up and giving it
a new option ("--allow-bitmaps" or something) to keep compatibility, but
it's out of scope here.

The existing option (and my suggestion, as well as most of the internal
code) are guilty of equating "bitmap" with "object reachability bitmap".
There's lots of things one might use bitmaps for, and at some point we
might even expose such a thing via rev-list.

Anyway, that concludes my rant. I don't think any of these are urgent to
fix, and definitely shouldn't be part of this series.

-Peff

Patch
diff mbox series

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