Re: [PATCH 1/4] revision: move bloom keyvec precondition into function

[Toon Claes <toon@iotcl.com>]

Jeff King <peff@peff.net> writes:

> On Fri, Jul 17, 2026 at 05:46:59PM +0200, Toon Claes wrote:
>
>> There are currently two callsites calling
>> check_maybe_different_in_bloom_filter(). They both check if
>> revs->bloom_keyvecs_nr is not zero before they call that function.
>> 
>> Move bloom_keyvecs_nr precondition into
>> check_maybe_different_in_bloom_filter() to simplify the code.
>
> Makes sense, but...
>
>> Note that this changes `bloom_ret` to become -1 when there are no Bloom
>> key vectors, which results in `count_bloom_filter_false_positive` not
>> being incremented. This is unobservable, as the Bloom statistics are
>> only reported when key vectors were set up.
>
> This "-1" return is kind of subtle. The function is really a tristate
> returning one of:
>
>   0: no, it's definitely not in the filter
>   1: yes, it's (probably) in the filter
>   -1: we could not even check the filter
>
> But nobody ever cares about the difference between "1" and "-1", because
> the probabilistic data structure means "we could not check" must err on
> the side of "it might be in the filter".

That's not entirely true. The `count_bloom_filter_false_positive`
depends on knowing whether the filter said "maybe" or if no filter was
used at all.

> But that leads to code like:
>
>   if (!bloom_ret)
>
> that _looks_ wrong at first glance (as in "oops, we are not catching -1
> and accidentally treating it the same as 1"). But it's is actually
> correct for the reason above.
>
> The "return -1" you are adding here is not the first (we'd do a similar
> thing if the commit was not found in the graph file). So it is not
> really adding to the confusion.
>
> But as we prepare to make this function public, should we consider
> changing that tristate to a boolean, like:
>
>   false: no, the path is definitely not touched by this commit
>   true: the path could be touched by this commit
>
> It's a minor point, but I think this makes the interface much more
> obvious.

That said, the public function might have a boolean interface, while
the private wrapper still uses the tristate. I'll address in the next
version.


-- 
Cheers,
Toon