Lidong Yan <yldhome2d2@xxxxxxxxx> 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".
