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

From: Taylor Blau <me@ttaylorr.com>
Date: Mar 7, 2025, 23:46 UTC
Message-ID: <Z8uFcvHRcBExhjNS@nand.local>
In-Reply-To: <CABPp-BFn0wht71rM1bE1ABpa6Rn6QRrcVrbN0fhcwprbm+T39Q@mail.gmail.com>

On Fri, Mar 07, 2025 at 01:31:31PM -0800, Elijah Newren wrote:

Show 34 quoted lines

> > 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);
>
> Doesn't this mean that if the user requested to exclude
> "refs/heads/bar" and "refs/heads/bar" exists, that we won't exclude it
> because it doesn't have a trailing slash?
>
> >From reading other comments in this thread, I guess that ends up being
> okay, because we only promise to filter out what we can cheaply
> filter, and we rely on our caller to double-check everything and do
> the real filtering.
>
> ...but it gives me some ugly dir.c vibes, reminding me of 95c11ecc73f2
> (Fix error-prone fill_directory() API; make it only return matches,
> 2020-04-01) and a slew of related bugs preceding it.  Granted, dir.c
> had this tri-state to deal with (tracked, untracked-but-ignored,
> untracked-and-not-ignored) and simplifying of whole directories, which
> don't apply here, so maybe the similarity of
> "fast-filtering-only-and-rely-on-caller" won't be a problem since the
> upper level filtering is so much more straightforward.
>
> Should this at least be called out in the commit message, though?

Yeah, I think that we don't have a tri-state here to deal with as was the case in 95c11ecc732 makes this a little easier to reason about.

And you're right: if we have a pattern like "refs/heads/bar" and we see a leaf in our reference hierarchy called "refs/heads/bar", the packed backend will not exclude it.

This is OK because the exclude pattern stuff is all considered "best-effort" and callers are expected to do their own filtering. Note that the exclude patterns (at least in the packed backend) don't know how to handle meta-characters (there's a big comment in refs/packed-backend.c explaining why). So we can't guarantee the absence of false positives without performing the same post-processing as the caller would.

Even prior to this commit, a literal match in the excluded patterns would result in a region whose start- and end-points are the same, and we'd throw it out before it made its way into the jump list.

Thanks, Taylor