Re: [PATCH v2 1/2] refs.c: remove empty '--exclude' patterns

From: Elijah Newren <newren@gmail.com>
Date: Mar 7, 2025, 23:58 UTC
Message-ID: <CABPp-BG_mXT1fv0G2QXJg4WJuVZcWbPHG_9VQ8Szf8Ges4fGJw@mail.gmail.com>
In-Reply-To: <Z8uDMHclfE+KqBMz@nand.local>

On Fri, Mar 7, 2025 at 3:37 PM Taylor Blau <me@ttaylorr.com> wrote:

Show 8 quoted lines

>
> On Fri, Mar 07, 2025 at 01:32:49PM -0800, Elijah Newren wrote:
> > Makes sense...but doesn't the second patch also fix this issue without
> > the first patch being needed?
>
> It does, but the mechanism is pretty round-about. (From a quick glance
> we'll turn the empty pattern "" into "/" which won't match anything, and
> thus won't contribute to the jump list).

How is that round-about? The whole point of patch 2 is to stop matching on excludes as a prefix unless that prefix is a directory name, right? To me, patch 1 merely looks like a special case of patch 2.

> But there are a couple of reasons to keep this patch. Most importantly,
> it hardens us against potential future regressions here with the empty
> pattern.

I'm fine with leaving the patch in place, since it doesn't hurt anything. And if the empty pattern is especially problematic, I can see this logic.

> And it makes dealing with that case much more explicit by
> throwing those patterns out before they make their way to the backends
> instead of the quirk above.

I don't understand this reason, though. It feels to me like the design behind patch 2 rather than a "quirk"...but maybe the fact that patch 2 doesn't exclude "refs/heads/bar" (at the low-level) even when that exact string is given as an exclude was unintentional or something? If it was an intentional part of patch 2 (as I assumed while reading it), then I don't see how patch 2 excluding the empty string exclude is a "quirk". Am I missing something?

(Not that it matters, since I'm fine with keeping the patch for your first reason, I'm just curious...)