On Fri, Mar 07, 2025 at 01:31:31PM -0800, Elijah Newren wrote: > > 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
