Re: [PATCH v2 03/11] bloom: get_bloom_filter() cleanups

From: René Scharfe <l.s.r@web.de>
Date: Jun 25, 2020, 07:24 UTC
Message-ID: <2d64707c-a538-118c-d7c3-7a38bf08fa79@web.de>
In-Reply-To: <492deaf916464abedc7fc2d3de41fe690a558d9b.1592934430.git.gitgitgadget@gmail.com>

Am 23.06.20 um 19:47 schrieb Derrick Stolee via GitGitGadget:

Show 9 quoted lines

> 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?

Show 30 quoted lines

>
> 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.

Show 6 quoted lines

>  	}
>
> -	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.

Show 5 quoted lines

> +		return NULL;
>
>  	repo_diff_setup(r, &diffopt);
>  	diffopt.flags.recursive = 1;
>