On Thu, Mar 06, 2025 at 09:47:37AM +0100, Patrick Steinhardt wrote: > 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. > > 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
