Re: [PATCH v2] bloom: enable bloom filter with wildcard pathspec in revision traversal
- From
Lidong Yan <yldhome2d2@gmail.com>
- Date
- Aug 9, 2025, 02:06 UTC
- Message-ID
- <B2F0FE14-AA88-490D-989C-3D93BF972DCF@gmail.com>
- In-Reply-To
- <xmqqsei1izhs.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
Show 13 quoted lines
>
>> @@ -693,9 +698,22 @@ static int convert_pathspec_to_bloom_keyvec(struct bloom_keyvec **out,
>> size_t len;
>> int res = 0;
>>
>> + len = pi->nowildcard_len;
>> + if (len != pi->len) {
>> + /*
>> + * for path like "/dir/file*", nowildcard part would be
>> + * "/dir/file", but only "/dir" should be used for the
>
> Leading "/" makes it look as if the pathspec element can begin with
> a slash, but it can not, can it?Yes, seems like if we pass a absolute path "/path/to/repository/dir/file”, git will automatically move "/path/to/repository” (in setup.c abspath_part_inside_repo()) So I should remove leading slash in my comment.
Show 52 quoted lines
> Taking what I suggested above, here is a possible improvement.
>
> revision.c | 18 ++++++++----------
> 1 file changed, 8 insertions(+), 10 deletions(-)
>
> diff --git i/revision.c w/revision.c
> index 2a5b98390e..2a92bdda84 100644
> --- i/revision.c
> +++ w/revision.c
> @@ -696,14 +696,14 @@ static int convert_pathspec_to_bloom_keyvec(struct bloom_keyvec **out,
> char *path_alloc = NULL;
> const char *path;
> size_t len;
> - int res = 0;
> + int res = -1; /* be pessimistic */
>
> len = pi->nowildcard_len;
> if (len != pi->len) {
> /*
> - * for path like "/dir/file*", nowildcard part would be
> - * "/dir/file", but only "/dir" should be used for the
> - * bloom filter
> + * for path like "dir/file*", nowildcard part would be
> + * "dir/file", but only "dir" should be used for the
> + * bloom filter.
> */
> while (len > 0 && pi->match[len - 1] != '/')
> len--;
> @@ -712,19 +712,17 @@ static int convert_pathspec_to_bloom_keyvec(struct bloom_keyvec **out,
> if (len > 0 && pi->match[len - 1] == '/')
> len--;
>
> + if (!len)
> + goto cleanup;
> +
> if (len != pi->len) {
> path_alloc = xmemdupz(pi->match, len);
> path = path_alloc;
> } else
> path = pi->match;
>
> - len = strlen(path);
> - if (!len) {
> - res = -1;
> - goto cleanup;
> - }
> -
> *out = bloom_keyvec_new(path, len, settings);
> + res = 0;
>
> cleanup:
> free(path_alloc);Thanks, I will apply this and add your signed-off. Lidong