From: Junio C Hamano Date: Mon, 27 Oct 2025 15:35:23 GMT Subject: Re: [PATCH] match_pathname(): give fnmatch one char of prefix context Message-ID: In-Reply-To: <20251027142902.GB2758515@coredump.intra.peff.net> Jeff King writes: >> I added the "limit to known bad case" in the illustration not for >> performance but for correctness. This was because just like we >> weren't convinced that the "**/" may be the only case that breaks >> the existing optimization, I was worried if stepping back by one >> byte may somehow make a pattern that should not match mistakenly >> match. > > Hmm, I hadn't really considered that. This is mostly hand-waving, but it > feels unlikely to cause issues because we're giving fnmatch() more > context, and never less. Yup. The above was an explanation of what I was thinking when I wrote it, not a justification. I haven't thought of a single case that it would hurt to step back by one byte. > BTW, there was another bug mentioned in that original issue around > backslash handling. I didn't investigate it at all, though. It didn't > look like it would be related to this optimization, so I think we can > just consider this fix independently. Hmph, backslash is GIT_GLOB_SPECIAL so nowildcard prefix would stop before it. Could it be that we mistake it as a directory separator? I _think_ we are still cleanly distinguish paths from the filesystem (which could use backslash as directory separator on some platforms) and the pathspec (which defines the slash as the sole directory separator), and have platform-specific fspathncmp() to absorb the differences when matching one with the other. And nowildcard_len is all about the pathspec, so it probably is something else. \Thanks.