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

From: Lidong Yan <502024330056@smail.nju.edu.cn>
Date: Jun 27, 2025, 14:24 UTC
Message-ID: <4A56A595-55B1-4EEB-9B9E-3E9F7A9A74D4@smail.nju.edu.cn>
In-Reply-To: <xmqqy0td8fa9.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 11 quoted lines

> Lidong Yan <yldhome2d2@gmail.com> writes:
> 
>> 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.

Got it. I will simply this log message in next version.

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

Yeah, I never notice that. I would add checks in forbid_bloom_filters(). And add test to ensure we don’t use bloom filter if any pathspec item is not literal.

Show 7 quoted lines

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

I think it's feasible. In that case, we would need to add a condition .nowildcard_len > 0 to forbid_bloom_filter. I'm happy to write a new patch to address this issue.

Show 17 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?

Definitely, I would fix that in 3rd version.

Thanks, Lidong