From: Jeff King <peff@peff.net>
Date: Tue, 28 Oct 2025 23:19:45 GMT
Subject: Re: [PATCH] match_pathname(): give fnmatch one char of prefix context
Message-ID: <20251028231945.GA4128296@coredump.intra.peff.net>
In-Reply-To: <xmqq4irkl5ms.fsf@gitster.g>

On Mon, Oct 27, 2025 at 08:35:23AM -0700, Junio C Hamano wrote:

> > 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.

Yeah, if you do:

  git init
  echo 'foo\' >.gitignore
  git check-ignore --no-index -n -v 'foo\'

then we do have a nowildcard prefix of only 3. But I don't think we get
to match_pathname() and the prefix-matching optimization at all. In
match_basename(), we directly call the equivalent of (those are literal
backslashes in the strings):

  fnmatch("foo\", "foo\", 0);

and it claims there's no match. Now this is our own wildmatch-backed
implementation, but I wondered what POSIX has to say on a trailing
backslash like that. It's:

         If FNM_NOESCAPE is not set in flags, a <backslash> character in
         pattern followed by any other character shall match that second
         character in string.  In particular, "\\" shall match a
         <backslash> in string.  If pattern ends with an unescaped
         <backslash>, fnmatch() shall return a non-zero value (indicating
         either no match or an error).  If FNM_NOESCAPE is set, a
         <backslash> character shall be treated as an ordinary character.

So we are supposed to reject the match. Looking at the wildmatch
implementation, it does this:

        switch (p_ch) {
        case '\\':
                /* Literal match with following character.  Note that the test
                 * in "default" handles the p[1] == '\0' failure case. */
                p_ch = *++p;
                /* FALLTHROUGH */
        default:
                if (t_ch != p_ch)
                        return WM_NOMATCH;
                continue;

which matches what POSIX says.

So I think the input is really nonsense, and we're following POSIX here
in rejecting it. I can't fault an alternative implementation too much
for treating the "\" as a literal char, since that's the only other
sensible behavior. It's probably what I'd do if I hadn't read that bit
of POSIX. ;)

But to a certain degree, I think this is a case of "if it hurts, don't
do it". If you are trying to match "foo\", the correct pattern is
"foo\\".

-Peff