bloom: fix logic in get_bloom_filter()
The get_bloom_filter() method is a bit complicated in some parts where it does not need to be. In particular, it needs to return a NULL filter only when compute_if_not_present is zero AND the filter data cannot be loaded from a commit-graph file. This currently happens by accident because the commit-graph does not load changed-path Bloom filters from an existing commit-graph when writing a new one. This will change in a later patch. Also clean up some style issues while we are here. One side-effect of returning a NULL filter is that the filters that are reported as "too large" will now be reported as NULL insead of length zero. This case was not properly covered before, so add a test. Further, remote the counting of the zero-length filters from revision.c and the trace2 logs. Helped-by: René Scharfe <l.s.r@web.de> Helped-by: SZEDER Gábor <szeder.dev@gmail.com> Signed-off-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Derrick Stolee committed
Jul 1, 2020 at 13:27 UTC
949197420e3da13c06c6a15fd4b4ed3120753c42
4 files changed
+34
-19
bloom.c
+6
-8
@@ -186,7 +186,7 @@ struct bloom_filter *get_bloom_filter(struct repository *r,
186
struct diff_options diffopt;
187
int max_changes = 512;
188
189
- if (bloom_filters.slab_size == 0)
189
+ if (!bloom_filters.slab_size)
190
return NULL;
191
192
filter = bloom_filter_slab_at(&bloom_filters, c);
@@ -194,16 +194,14 @@ struct bloom_filter *get_bloom_filter(struct repository *r,
194
if (!filter->data) {
195
load_commit_graph_info(r, c);
196
if (c->graph_pos != COMMIT_NOT_FROM_GRAPH &&
197
- r->objects->commit_graph->chunk_bloom_indexes) {
198
- if (load_bloom_filter_from_graph(r->objects->commit_graph, filter, c))
199
- return filter;
200
- else
201
- return NULL;
202
- }
197
+ r->objects->commit_graph->chunk_bloom_indexes)
198
+ load_bloom_filter_from_graph(r->objects->commit_graph, filter, c);
199
}
200
205
- if (filter->data || !compute_if_not_present)
201
+ if (filter->data)
202
return filter;
203
+ if (!compute_if_not_present)
204
+ return NULL;
205
206
repo_diff_setup(r, &diffopt);
207
diffopt.flags.recursive = 1;
commit-graph.c
+6
-2
@@ -1098,7 +1098,8 @@ static void write_graph_chunk_bloom_indexes(struct hashfile *f,
1098
1099
while (list < last) {
1100
struct bloom_filter *filter = get_bloom_filter(ctx->r, *list, 0);
1101
- cur_pos += filter->len;
1101
+ size_t len = filter ? filter->len : 0;
1102
+ cur_pos += len;
1103
display_progress(progress, ++i);
1104
hashwrite_be32(f, cur_pos);
1105
list++;
@@ -1126,8 +1127,11 @@ static void write_graph_chunk_bloom_data(struct hashfile *f,
1127
1128
while (list < last) {
1129
struct bloom_filter *filter = get_bloom_filter(ctx->r, *list, 0);
1130
+ size_t len = filter ? filter->len : 0;
1131
display_progress(progress, ++i);
1130
- hashwrite(f, filter->data, filter->len * sizeof(unsigned char));
1132
+
1133
+ if (len)
1134
+ hashwrite(f, filter->data, len * sizeof(unsigned char));
1135
list++;
1136
}
1137
revision.c
-7
@@ -633,7 +633,6 @@ static unsigned int count_bloom_filter_maybe;
633
static unsigned int count_bloom_filter_definitely_not;
634
static unsigned int count_bloom_filter_false_positive;
635
static unsigned int count_bloom_filter_not_present;
636
-static unsigned int count_bloom_filter_length_zero;
636
637
static void trace2_bloom_filter_statistics_atexit(void)
638
{
@@ -641,7 +640,6 @@ static void trace2_bloom_filter_statistics_atexit(void)
640
641
jw_object_begin(&jw, 0);
642
jw_object_intmax(&jw, "filter_not_present", count_bloom_filter_not_present);
644
- jw_object_intmax(&jw, "zero_length_filter", count_bloom_filter_length_zero);
643
jw_object_intmax(&jw, "maybe", count_bloom_filter_maybe);
644
jw_object_intmax(&jw, "definitely_not", count_bloom_filter_definitely_not);
645
jw_object_intmax(&jw, "false_positive", count_bloom_filter_false_positive);
@@ -735,11 +733,6 @@ static int check_maybe_different_in_bloom_filter(struct rev_info *revs,
733
return -1;
734
}
735
738
- if (!filter->len) {
739
- count_bloom_filter_length_zero++;
740
- return -1;
741
- }
742
-
736
result = bloom_filter_contains(filter,
737
revs->bloom_key,
738
revs->bloom_filter_settings);
t/t4216-log-bloom.sh
+22
-2
@@ -60,7 +60,7 @@ setup () {
60
61
test_bloom_filters_used () {
62
log_args=$1
63
- bloom_trace_prefix="statistics:{\"filter_not_present\":0,\"zero_length_filter\":0,\"maybe\""
63
+ bloom_trace_prefix="statistics:{\"filter_not_present\":0,\"maybe\""
64
setup "$log_args" &&
65
grep -q "$bloom_trace_prefix" "$TRASH_DIRECTORY/trace.perf" &&
66
test_cmp log_wo_bloom log_w_bloom &&
@@ -142,7 +142,7 @@ test_expect_success 'setup - add commit-graph to the chain with Bloom filters' '
142
143
test_bloom_filters_used_when_some_filters_are_missing () {
144
log_args=$1
145
- bloom_trace_prefix="statistics:{\"filter_not_present\":3,\"zero_length_filter\":0,\"maybe\":8,\"definitely_not\":6"
145
+ bloom_trace_prefix="statistics:{\"filter_not_present\":3,\"maybe\":8,\"definitely_not\":6"
146
setup "$log_args" &&
147
grep -q "$bloom_trace_prefix" "$TRASH_DIRECTORY/trace.perf" &&
148
test_cmp log_wo_bloom log_w_bloom
@@ -152,4 +152,24 @@ test_expect_success 'Use Bloom filters if they exist in the latest but not all c
152
test_bloom_filters_used_when_some_filters_are_missing "-- A/B"
153
'
154
155
+test_expect_success 'correctly report changes over limit' '
156
+ git init 513changes &&
157
+ (
158
+ cd 513changes &&
159
+ for i in $(test_seq 1 513)
160
+ do
161
+ echo $i >file$i.txt || return 1
162
+ done &&
163
+ git add . &&
164
+ git commit -m "files" &&
165
+ git commit-graph write --reachable --changed-paths &&
166
+ for i in $(test_seq 1 513)
167
+ do
168
+ git -c core.commitGraph=false log -- file$i.txt >expect &&
169
+ git log -- file$i.txt >actual &&
170
+ test_cmp expect actual || return 1
171
+ done
172
+ )
173
+'
174
+
175
test_done
\ No newline at end of file