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

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

Lidong Yan <yldhome2d2@gmail.com> writes:

Show 5 quoted lines

> Remove `if (spec->nr > 1)` to enable bloom filter given multiple
> pathspec. Wrapped for loop around code in prepare_to_use_bloom_filter()
> to initialize each pathspec's struct bloom_keyvec. Add for loop
> in check_maybe_different_in_bloom_filter() to find if at least one
> pathspec's bloom_keyvec is contained in bloom filter.

Oy. That's too dense enumeration but I suspect are all "what the patch does" that can be read from the diff. The first sentence gives "why", which is excellent.

You'd need to check in forbid_bloom_filters() that none of the pathspec items have magic (other than literal), not just the first one, no?

Totally outside the topic, but I wonder if we can further optimize by adding an early rejection using .nowildcard_len? Instead of allowing a wildcarded "dir/*" pathspec element from disabling the Bloom filter altogether, we could say "dir/ is not possibly altered, so there may be dir/A, dir/B, etc., in the directory, nothing that would match dir/* wildcard would have been modified", couldn't we?

Show 13 quoted lines

> diff --git a/revision.c b/revision.c
> index cf7dc3b3fa..8818f017f3 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -675,8 +675,6 @@ static int forbid_bloom_filters(struct pathspec *spec)
>  {
>  	if (spec->has_wildcard)
>  		return 1;
> -	if (spec->nr > 1)
> -		return 1;
>  	if (spec->magic & ~PATHSPEC_LITERAL)
>  		return 1;
>  	if (spec->nr && (spec->items[0].magic & ~PATHSPEC_LITERAL))

This last check only looks at the first item. It used to be OK because we didn't look at a pathspec with more than one element, but now shouldn't we care?

Thanks.