Re: [PATCH 4/4] last-modified: keep per-path Bloom filters for wildcard pathspecs

[Toon Claes <toon@iotcl.com>]

Toon Claes <toon@iotcl.com> writes:

> The last-modified builtin expands the pathspec to a set of literal paths
> and builds a Bloom key for each. During the walk it looks those keys up
> in the commit's filter to decide whether the commit is worth diffing.
> These lookups need `bloom_filter_settings` for the key hashing.
>
> prepare_revision_walk() runs prepare_to_use_bloom_filter() to build the
> pathspec key vectors. For a pathspec that cannot be turned into a Bloom
> key, such as a top-level wildcard like "*.c", that function gives up and
> clears `bloom_filter_settings`.
>
> Restore `bloom_filter_settings` after prepare_revision_walk() so the
> per-path check keeps working for wildcard pathspecs.
>
> Signed-off-by: Toon Claes <toon@iotcl.com>
> ---
>  builtin/last-modified.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/builtin/last-modified.c b/builtin/last-modified.c
> index e8ee610404..adc7cd8c74 100644
> --- a/builtin/last-modified.c
> +++ b/builtin/last-modified.c
> @@ -360,6 +360,14 @@ static int last_modified_run(struct last_modified *lm)
>  
>  	prepare_revision_walk(&lm->rev);
>  
> +	/*
> +	 * prepare_revision_walk() clears bloom_filter_settings for pathspecs
> +	 * without a Bloom key. Restore it so the per-path check keeps working.
> +	 */
> +	if (!lm->rev.bloom_filter_settings)
> +		lm->rev.bloom_filter_settings =
> +			get_bloom_filter_settings(lm->rev.repo);
> +

@Peff, as far I could tell:

* This change was not needed to be able to use the Bloom filters with
  the pathspec.

* Only restoring bloom_filter_settings was needed. In your patch you're
  calling prepare_to_use_bloom_filter(), but that is being called by
  prepare_revision_walk(). Thus the restoring of the filter settings
  I've added after that function.

>  	max_count = lm->rev.max_count;
>  
>  	init_active_paths_for_commit(&lm->active_paths);
>
> -- 
> 2.53.0.1323.g189a785ab5
>

-- 
Cheers,
Toon