Derrick Stolee <stolee@xxxxxxxxx> wrote:
>
> This diff is still bigger than I was hoping, so I'm sending a couple
> of patches that simplify this code movement. Feel free to ignore
> them as being too nit-picky.
>
> --- >8 ---
>
> From 69fa36dc615e140ae842b536f7da792beaebb272 Mon Sep 17 00:00:00 2001
> From: Derrick Stolee <stolee@xxxxxxxxx>
> Date: Thu, 10 Jul 2025 08:06:29 -0400
> Subject: [PATCH v5.1 3.5/4] revision: make helper for pathspec to bloom key
>
> When preparing to use bloom filters in a revision walk, Git populates a
> boom_keyvec with an array of bloom keys for the components of a path.
> Before we create the ability to map multiple pathspecs to multiple
> bloom_keyvecs, extract the conversion from a pathspec to a bloom_keyvec
> into its own helper method. This simplifies the state that persists in
> prepare_to_use_bloom_filter() as well as makes the next change much
> simpler.
>
> Signed-off-by: Derrick Stolee <stolee@xxxxxxxxx>
> ---
> revision.c | 50 +++++++++++++++++++++++++++++++-------------------
> 1 file changed, 31 insertions(+), 19 deletions(-)
>
> diff --git a/revision.c b/revision.c
> index 22bcfab7f93..4c09b594c55 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -687,14 +687,37 @@ static int forbid_bloom_filters(struct pathspec *spec)
>
> static void release_revisions_bloom_keyvecs(struct rev_info *revs);
>
> -static void prepare_to_use_bloom_filter(struct rev_info *revs)
> +static int convert_pathspec_to_filter(const struct pathspec_item *pi,
> + struct bloom_keyvec **bloom_keyvec,
> + const struct bloom_filter_settings *settings)
> {
> - struct pathspec_item *pi;
> - struct bloom_keyvec *bloom_keyvec;
> - char *path_alloc = NULL;
> - const char *path, *p;
> size_t len;
> + const char *path;
> + char *path_alloc = NULL;
> + int res = 0;
> +
> + /* remove single trailing slash from path, if needed */
> + if (pi->len > 0 && pi->match[pi->len - 1] == '/') {
> + path_alloc = xmemdupz(pi->match, pi->len - 1);
> + path = path_alloc;
> + } else
> + path = pi->match;
> +
> + len = strlen(path);
> + if (!len) {
> + res = -1;
> + goto cleanup;
> + }
> +
> + *bloom_keyvec = bloom_keyvec_new(path, len, settings);
>
> +cleanup:
> + FREE_AND_NULL(path_alloc);
I think we don’t need to NULL path_alloc here, but it doesn’t hurt.
> + return res;
> +}
> +
> +static void prepare_to_use_bloom_filter(struct rev_info *revs)
> +{
> if (!revs->commits)
> return;
>
> @@ -712,22 +735,12 @@ static void prepare_to_use_bloom_filter(struct rev_info *revs)
>
> revs->bloom_keyvecs_nr = 1;
> CALLOC_ARRAY(revs->bloom_keyvecs, 1);
> - pi = &revs->pruning.pathspec.items[0];
>
> - /* remove single trailing slash from path, if needed */
> - if (pi->len > 0 && pi->match[pi->len - 1] == '/') {
> - path_alloc = xmemdupz(pi->match, pi->len - 1);
> - path = path_alloc;
> - } else
> - path = pi->match;
> -
> - len = strlen(path);
> - if (!len)
> + if (convert_pathspec_to_filter(&revs->pruning.pathspec.items[0],
> + &revs->bloom_keyvecs[0],
> + revs->bloom_filter_settings))
> goto fail;
>
> - revs->bloom_keyvecs[0] =
> - bloom_keyvec_new(path, len, revs->bloom_filter_settings);
> -
> if (trace2_is_enabled() && !bloom_filter_atexit_registered) {
> atexit(trace2_bloom_filter_statistics_atexit);
> bloom_filter_atexit_registered = 1;
> @@ -737,7 +750,6 @@ static void prepare_to_use_bloom_filter(struct rev_info *revs)
>
> fail:
> revs->bloom_filter_settings = NULL;
> - free(path_alloc);
> release_revisions_bloom_keyvecs(revs);
> }
>
> --
> 2.47.2.vfs.0.2
This looks perfect to me. I would squash patch 3.5 to patch 3 in v6.
Thanks for your patch,
Lidong
