From: Jeff King <peff@peff•net>
To: Junio C Hamano <gitster@pobox•com>
Cc: Sruteesh Kumar <sruteesh.oss@protonmail•com>,
"git@vger•kernel.org" <git@vger•kernel.org>
Subject: Re: [PATCH] match_pathname(): give fnmatch one char of prefix context
Date: Tue, 28 Oct 2025 19:19:45 -0400 [thread overview]
Message-ID: <20251028231945.GA4128296@coredump.intra.peff.net> (raw)
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
next prev parent reply other threads:[~2025-10-28 23:19 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-10-10 14:57 Probable issue with code/documentation Sruteesh Kumar
2025-10-14 0:34 ` [PATCH] match_pathname(): give fnmatch one char of prefix context Jeff King
2025-10-14 3:09 ` Jeff King
2025-10-22 16:19 ` Sruteesh Kumar
2025-10-23 20:28 ` Junio C Hamano
2025-10-24 18:28 ` Sruteesh Kumar
2025-10-26 15:18 ` Jeff King
2025-10-26 15:26 ` Jeff King
2025-10-26 15:40 ` [PATCH v2 0/2] fix "foo**/bar" matching "foobar" Jeff King
2025-10-26 15:41 ` [PATCH v2 1/2] match_pathname(): reorder prefix-match check Jeff King
2025-10-26 15:42 ` [PATCH v2 2/2] match_pathname(): give fnmatch one char of prefix context Jeff King
2025-10-26 23:29 ` [PATCH] " Junio C Hamano
2025-10-27 14:29 ` Jeff King
2025-10-27 15:35 ` Junio C Hamano
2025-10-28 23:19 ` Jeff King [this message]
2025-10-29 15:32 ` [PATCH] doc: document backslash in gitignore patterns Jeff King
2025-10-29 15:55 ` Jeff King
2025-10-30 13:40 ` D. Ben Knoble
2025-10-30 15:08 ` Jeff King
2025-10-30 16:05 ` Ben Knoble
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20251028231945.GA4128296@coredump.intra.peff.net \
--to=peff@peff$(echo .)net \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=sruteesh.oss@protonmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox