Re: [PATCH] match_pathname(): give fnmatch one char of prefix context

From: Jeff King <peff@peff.net>
Date: Oct 28, 2025, 23:19 UTC
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:

Show 13 quoted lines

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