Re: [PATCH v2] bloom: enable bloom filter with wildcard pathspec in revision traversal

[Lidong Yan <yldhome2d2@gmail.com>]

Junio C Hamano <gitster@pobox.com> writes:
> 
>> @@ -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.

> 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