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

> 
> Lidong Yan <yldhome2d2@gmail.com> writes:
> 
>> [jc: avoid allocating zero length path in
>> convert_pathspec_to_bloom_keyvec()]
> 
> This is different from what I did, though.

> 
>> @@ -693,19 +698,31 @@ 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
>> + * bloom filter
>> + */
> 
> A missing full-stop.

> 
>> + while (len > 0 && pi->match[len - 1] != '/')
>> + len--;
>> + }
>> /* 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;
>> + if (len > 0 && pi->match[len - 1] == '/')
>> + len--;
>> 
>> - len = strlen(path);
>> if (!len) {
>> res = -1;
>> goto cleanup;
>> }
>> 
>> + if (len != pi->len) {
>> + path_alloc = xmemdupz(pi->match, len);
>> + path = path_alloc;
>> + } else
>> + path = pi->match;
>> +
>> *out = bloom_keyvec_new(path, len, settings);
>> 
>> cleanup:
> 
> Two comments.
> 
> * For a function that finds an error condition in the middle and
>   jumps to the "cleanup:" label at the end, it is more future-proof
>   to start pessimistic (i.e. initialize 'res' to error(-1)) and
>   flip 'res' to success(0) at the very end when everything went
>   well.  It would simplify the change necessary when we need to add
>   _more_ early error return code paths to the function in the
>   future.
> 
>   But this flip from "assume success" to "assume failure" is
>   something that should be not be done as part of this patch;
>   perhaps doing it a separate preliminary clean-up patch is a
>   better way to do so.

> 
> * I think the change from v3 (this one) to v4 makes the function
>   worse; we found that it is a good practice to have a single place
>   to release any resources we temporarily acquired and arrange
>   exception handling code to just jump there during the course of
>   this project.
> 
>   The current implementation may happen to have only one such early
>   return (i.e. "len has become 0; we realize that we cannot use the
>   Bloom filter"), but adding a new early return in the future would
>   be easier if you kept the original arrangement.  The new early
>   return condition may have to be computed after we have acquired
>   resources we need to release, so it may need more than a simple
>   "return -1”.