Re: [PATCH 2/4] revision: expose check for paths maybe changed in Bloom filter
From: Junio C Hamano <hidden>
Date: 2026-07-17 20:47:06
Toon Claes [off-list ref] writes:
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
@@ -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;