Re: [PATCH 2/2] bloom: enable multiple pathspec bloom keys

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 27, 2025, 20:39 UTC
Message-ID: <xmqqqzz47wd3.fsf@gitster.g>
In-Reply-To: <20250625125541.3048632-3-502024330056@smail.nju.edu.cn>

Lidong Yan <yldhome2d2@gmail.com> writes:

This is a tangent, but I have to say that whoever wrote the original test does not understand shells very well. When you have files A and B in your working tree, to your $command, the following two does not make any difference:

	$command ?
	$command A B

In fact it cannot even tell which form was used when composing the command line. So this original test ...

> -test_expect_success 'git log with wildcard that resolves to a multiple paths does not uses Bloom filters' '
> -	test_bloom_filters_not_used "-- *" &&
> -	test_bloom_filters_not_used "-- file*"

... is misleading to say the least.

> +test_expect_success 'git log with wildcard that resolves to a multiple paths uses Bloom filters' '
> +	test_bloom_filters_used "-- *" &&
> +	test_bloom_filters_used "-- file*"
>  '

I think you should just retitle this to say

	git log with multiple literal paths use Bloom filter

or something.

Also the setup helper test_bloom_filters_{not_,}used helpers call is written in a way to make it impossible to pass a real wildcard and see how "$git log" would behave, because it does this:

	git -c core.commitGraph=false log --pretty="format:%s" $1 >log_wo_bloom &&

It probably should use 'eval' so that the caller can pass a quoted wildcard, perhaps like

    eval git -c core.commitgraph=false \
	     log --pretty=format:%s "$1" >log_wo_bloom &&

Then a test we can add to see how wildcards prevent Bloom from kicking in would look like

	test_bloom_filters_used "-- file*" &&
	test_bloom_filters_not_used "-- file4 file\*" &&

The former lets the shell expand file* when the above "eval" evaluates its (concatenated) strings, while the latter leaves the backslash before the asterisk in the strings fed to "eval", so the "log" will see a pathspec with wildcard.

If we were to fix that setup() thing, we of course need to be a bit careful about existing tests.

Thanks.