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

From: Taylor Blau <me@ttaylorr.com>
Date: Mar 6, 2025, 14:54 UTC
Message-ID: <Z8m3Mo+3l0IxzyOH@nand.local>
In-Reply-To: <Z8lhKT2KuM3VJ7d-@pks.im>

On Thu, Mar 06, 2025 at 09:47:37AM +0100, Patrick Steinhardt wrote:

Show 7 quoted lines

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

Fair enough.

Show 17 quoted lines

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

That's not quite true. for_each_ref__exclude treats its first argument as the positive half of the query, and the remaining arguments as exclusions. So the only excluded regions here are "refs/heads/ba" and "refs/heads/baz", which were overlapping prior to this patch but aren't anymore.

So this test isn't quite doing what it says it is anymore, and should really be called "non-directory excluded regions" or similar. But we should still have a separate test that does cover excluded regions, which needs a new layer underneath some hirearchy, e.g., having "refs/heads/bar/x/..." and "refs/heads/bar" as the excluded regions.

I adjusted those and will push out a new version with the results shortly. Thanks for reading closely.

Thanks, Taylor