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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 9, 2025, 23:51 UTC
Message-ID: <xmqqpld45a0h.fsf@gitster.g>
In-Reply-To: <20250809021642.22195-1-yldhome2d2@gmail.com>

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.

Show 11 quoted lines

> @@ -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.

Show 27 quoted lines

> +		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".