Re: [PATCH v2 03/11] bloom: get_bloom_filter() cleanups
From: René Scharfe <hidden>
Date: 2020-06-25 07:25:23
Am 23.06.20 um 19:47 schrieb Derrick Stolee via GitGitGadget:
From: Derrick Stolee <redacted> 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.
So this is actually a logic fix, not just a cleanup as the subject says?
quoted hunk ↗ jump to hunk
Also clean up some style issues while we are here. Signed-off-by: Derrick Stolee <redacted> --- bloom.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)diff --git a/bloom.c b/bloom.c index c38d1cff0c..7291506564 100644 --- a/bloom.c +++ b/bloom.c@@ -186,7 +186,7 @@ struct bloom_filter *get_bloom_filter(struct repository *r, struct diff_options diffopt; int max_changes = 512; - if (bloom_filters.slab_size == 0) + if (!bloom_filters.slab_size) return NULL; filter = bloom_filter_slab_at(&bloom_filters, c);@@ -194,16 +194,15 @@ struct bloom_filter *get_bloom_filter(struct repository *r, if (!filter->data) { load_commit_graph_info(r, c); if (c->graph_pos != COMMIT_NOT_FROM_GRAPH && - r->objects->commit_graph->chunk_bloom_indexes) { - if (load_bloom_filter_from_graph(r->objects->commit_graph, filter, c)) - return filter; - else - return NULL;
... and the fix is that this else branch should not be taken if compute_if_not_present is set.
quoted hunk ↗ jump to hunk
- } + r->objects->commit_graph->chunk_bloom_indexes && + load_bloom_filter_from_graph(r->objects->commit_graph, filter, c)) + return filter;
You could even drop this return as well and have the check below handle the successful load case.
quoted hunk ↗ jump to hunk
} - if (filter->data || !compute_if_not_present) + if (filter->data) return filter; + if (!filter->data && !compute_if_not_present)
^^^^^^^^^^^^^ The first condition is always true, as the check two lines above makes sure. Removing it would be cleaner IMHO.
quoted hunk ↗ jump to hunk
+ return NULL; repo_diff_setup(r, &diffopt); diffopt.flags.recursive = 1;