On Fri, Mar 7, 2025 at 3:37 PM Taylor Blau <me@xxxxxxxxxxxx> wrote: > > 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...)
