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". 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.

-Peff