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

On Tue, Apr 06, 2021 at 01:54:31PM -0400, Jeff King wrote:

On Mon, Mar 15, 2021 at 02:14:59PM +0100, Patrick Steinhardt wrote:

quoted

When the user has multiple objects filters specified, then this is
internally represented by having a "combined" filter. These combined
filters aren't yet supported by bitmap indices and can thus not be
accelerated.

Fix this by implementing support for these combined filters. The
implementation is quite trivial: when there's a combined filter, we
simply recurse into `filter_bitmap()` for all of the sub-filters.

The goal makes sense.

Before this patch, I think your test:

quoted

+test_expect_success 'combine filter' '
+	git rev-list --objects --filter=blob:limit=1000 --filter=object:type=blob tag >expect &&
+	git rev-list --use-bitmap-index \
+		     --objects --filter=blob:limit=1000 --filter=object:type=blob tag >actual &&
+	test_bitmap_traversal expect actual
+'

would pass anyway, because we'd just skip using bitmaps. Is there a way
we can tell that the bitmap code actually kicked in? Maybe a perf test
would make it clear (those aren't always run, but hopefully we'd
eventually notice a regression there).

I think that's not actually true. Note that we're using
`test_bitmap_traversal`:

    test_bitmap_traversal () {
        if test "$1" = "--no-confirm-bitmaps"
        then
            shift
        elif cmp "$1" "$2"
        then
            echo >&2 "identical raw outputs; are you sure bitmaps were used?"
            return 1
        fi &&
        cut -d' ' -f1 "$1" | sort >"$1.normalized" &&
        sort "$2" >"$2.normalized" &&
        test_cmp "$1.normalized" "$2.normalized" &&
        rm -f "$1.normalized" "$2.normalized"
    }

The output is different when using bitmap indices, which is why the
function knows to fail in case output is the same in both cases. So we
know that it cannot be the same here and thus we also know that the
bitmap case kicked in.

Patrick