Re: [PATCH 2/2] refs.c: unify '--exclude' behavior between files and packed backends

[Patrick Steinhardt <ps@pks.im>]

On Wed, Mar 05, 2025 at 08:19:53PM -0500, Taylor Blau wrote:

Nit: I'd reword the commit subject to not only talk about "files" and
"packed" backends. Or maybe not mention the backend at all, as it is the
same bug for every backend that implements excludes. Something like
"refs: stop matching non-directory prefixes in exclude patterns", for
example.

[snip]
> But since the same problem exists in reftable, we can fix both at once
> by performing this pre-processing step one layer up in refs.c at the
> common entrypoint for the two, which is 'refs_ref_iterator_begin()'.
> 
> Since that solution is both the simplest and only requires modification
> in one spot, let's normalize exclude patterns so that they end with a
> trailing slash. This causes us to unify the behavior between all three
> backends.

Nice.

> There is some minor test fallout in the "overlapping excluded regions"
> test, which happens to use 'refs/ba' as an exclude pattern, and expects
> references under the "refs/heads/bar/*" and "refs/heads/baz/*"
> hierarchies to be excluded from the results.

Yup, I noticed that this test is asserting the broken behaviour.

> diff --git a/refs.c b/refs.c
> index 17d3840aff..2d9a1b51f4 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1708,7 +1708,11 @@ struct ref_iterator *refs_ref_iterator_begin(
>  			if (!len)
>  				continue;
>  
> -			strvec_push(&normalized_exclude_patterns, pattern);
> +			if (pattern[len - 1] == '/')
> +				strvec_push(&normalized_exclude_patterns, pattern);
> +			else
> +				strvec_pushf(&normalized_exclude_patterns, "%s/",
> +					     pattern);
>  		}
>  
>  		exclude_patterns = normalized_exclude_patterns.v;

This looks exactly as expected.

> diff --git a/t/t1419-exclude-refs.sh b/t/t1419-exclude-refs.sh
> index fd58260a24..d955cf9541 100755
> --- a/t/t1419-exclude-refs.sh
> +++ b/t/t1419-exclude-refs.sh
> @@ -101,7 +101,7 @@ test_expect_success 'adjacent, non-overlapping excluded regions' '
>  
>  test_expect_success 'overlapping excluded regions' '
>  	for_each_ref__exclude refs/heads refs/heads/ba refs/heads/baz >actual 2>perf &&
> -	for_each_ref refs/heads/foo refs/heads/quux >expect &&
> +	for_each_ref refs/heads/bar refs/heads/foo refs/heads/quux >expect &&
>  
>  	test_cmp expect actual &&
>  	assert_jumps 1 perf

I was wondering whether this still tests the right thing. But the ranges
still are overlapping, as "refs/heads" and "refs/heads/baz" are. So
judging by the test description it seems to still do what's advertised.

Patrick