| Message ID | 492deaf916464abedc7fc2d3de41fe690a558d9b.1592934430.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | More commit-graph/Bloom filter improvements | expand |
Am 23.06.20 um 19:47 schrieb Derrick Stolee via GitGitGadget: > From: Derrick Stolee <dstolee@microsoft.com> > > 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? > > Also clean up some style issues while we are here. > > Signed-off-by: Derrick Stolee <dstolee@microsoft.com> > --- > 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. > - } > + 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. > } > > - 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. > + return NULL; > > repo_diff_setup(r, &diffopt); > diffopt.flags.recursive = 1; >
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; - } + r->objects->commit_graph->chunk_bloom_indexes && + load_bloom_filter_from_graph(r->objects->commit_graph, filter, c)) + return filter; } - if (filter->data || !compute_if_not_present) + if (filter->data) return filter; + if (!filter->data && !compute_if_not_present) + return NULL; repo_diff_setup(r, &diffopt); diffopt.flags.recursive = 1;