Re: [PATCH v2 2/2] refs.c: stop matching non-directory prefixes in exclude patterns

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 6, 2025, 17:27 UTC
Message-ID: <xmqqjz92hxxi.fsf@gitster.g>
In-Reply-To: <67c8c5f797833a9a35f4805059d7e759020f54bd.1741275245.git.me@ttaylorr.com>

Taylor Blau <me@ttaylorr.com> writes:

Show 14 quoted lines

> So there is a subtle bug with '--exclude' which is that in the
> packed-refs backend we will consider "refs/heads/bar" to be a pattern
> match against "refs/heads/ba" when we shouldn't. Likewise, the reftable
> backend (which in this case is bug-compatible with the packed backend)
> exhibits the same broken behavior.
> ...
> 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.
>
> ... test (since the range is no longer
> overlapping under the stricter interpretation of --exclude patterns
> presented here).

The code change, reasoning, and the tests look all good. It just leaves a bit awkward aftertaste.

In general, I think our "we have a tree-like structure with patterns to match paths" code paths, like pathspec matching, are structured in such a way that the low level is expected to merely cull candidates early as a performance optimization measure (in other words, they are allowed false positives and say something matches when they do not, but not allowed false negatives) and leave the upper level to further reject the ones that do not match the pattern. If packed-refs backend was too loose in its matching and erroneously considered that refs/heads/bar matched refs/heads/ba pattern, I would naïvely expect that the upper layer would catch and reject that refs/heads/bar as not matching.

Apparently that was not happening and that is why we need this fix?

Is the excluded region optimization expected to be powerful enough to cover all our needs so that we do not need to post-process what it passes?

Thanks.