Re: [PATCH 2/4] revision: expose check for paths maybe changed in Bloom filter

[Junio C Hamano <gitster@pobox.com>]

Toon Claes <toon@iotcl.com> writes:

> @@ -748,26 +748,20 @@ static int check_maybe_different_in_bloom_filter(struct rev_info *revs,
>  						 struct commit *commit)
>  {
>  	struct bloom_filter *filter;
> -	int result = 0;
> -
> -	if (!revs->bloom_keyvecs_nr)
> -		return -1;
> +	int result;
>  
>  	if (commit_graph_generation(commit) == GENERATION_NUMBER_INFINITY)
>  		return -1;
>  
>  	filter = get_bloom_filter(revs->repo, commit);
> -
>  	if (!filter) {
>  		count_bloom_filter_not_present++;
>  		return -1;
>  	}
>  
> -	for (size_t nr = 0; !result && nr < revs->bloom_keyvecs_nr; nr++) {
> -		result = bloom_filter_contains_vec(filter,
> -						   revs->bloom_keyvecs[nr],
> -						   revs->bloom_filter_settings);
> -	}
> +	result = revs_maybe_changed_in_bloom(revs, filter);
> +	if (result < 0)
> +		return result;
>  
>  	if (result)
>  		count_bloom_filter_maybe++;

Doesn't this change skew the stats?

In today's code, revs->bloom_keyvecs_nr == 0 results in an early
return, without touching count_bloom_filter_not_present.  In the
updated code, we would not notice revs->bloom_keyvecs_nr being zero
and call get_bloom_filter() first.  If that yields NULL, we increment
_not_present variable.

Also an error return -1 from bloom_filter_contains_vec() breaks the
loop in today's code, increments count_bloom_filter_maybe (even
though the result is -1, not positive) and returns.  In updated
code, an error return would return from this function but neither
_maybe nor _definitely_not is incremented.

It could be that these two are intended "while at it we fix it too"
improvements, but then they deserve to be mentioned in the proposed
log message.  Personally, I think the first one that increments the
_not_present statistics when keyvecs is empty a bug, though.

> @@ -777,6 +771,23 @@ static int check_maybe_different_in_bloom_filter(struct rev_info *revs,
>  	return result;
>  }
>  
> +int revs_maybe_changed_in_bloom(struct rev_info *revs,
> +				struct bloom_filter *filter)
> +{
> +	int result = 0;
> +
> +	if (!revs->bloom_keyvecs_nr)
> +		return -1;
> +
> +	for (size_t nr = 0; !result && nr < revs->bloom_keyvecs_nr; nr++) {
> +		result = bloom_filter_contains_vec(filter,
> +						   revs->bloom_keyvecs[nr],
> +						   revs->bloom_filter_settings);
> +	}
> +
> +	return result;
> +}

This is inherited from the original, but I think it would be easier
to follow if it were written like this:

	for (size_t nr = 0; nr < revs->bloom_keyvecs_nr; nr++) {
		if ((result = bloom_filter_contains_vec(filter,
					revs->bloom_keyvecs[nr],
					revs->bloom_filter_settings)))
			return result;
	}
	return 0;