Junio C Hamano <gitster@xxxxxxxxx> writes:
>
> 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.
Sorry, I don’t fully understand what you mean — should I remove
this line or rewrite it?
>
>> @@ -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.
Will fix.
>
>> + 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.
Ah, I’ve always thought that `ret = -1; goto cleanup;` was a kind of the
everybody-should-use pattern. So when I saw you write `int ret = -1;` first,
I was a bit puzzled. Now I understand what you mean, and I’ll add a
cleanup patch.
>
> * 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”.
Understand. I was thinking about less code is better. I will add the cleanup
part back.
Thanks,
Lidong
