[PATCH 13/14] t4135-*.sh: Skip the "backslash" tests on cygwin

[PATCH 13/14] t4135-*.sh: Skip the "backslash" tests on cygwin

[Ramsay Jones]

The BSLASHPSPEC tests (11-13) fail on cygwin, since you can't
create files containing an backslash character in the name.
In order to skip these tests, we simply stop (incorrectly)
asserting the BSLASHPSPEC prerequisite in test-lib.sh.

Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
---

Note t3700-*.sh has a test protected by BSLASHSPEC which
previously passed on cygwin and will now be (unnecessarily)
skipped. This test needs to be skipped on MinGW, given how
it is written; if you remove the single quotes around the
filename, however, it will pass even on MinGW.

 t/test-lib.sh |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 9e74357..aee7d20 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1061,7 +1061,6 @@ case $(uname -s) in
 	;;
 *CYGWIN*)
 	test_set_prereq POSIXPERM
-	test_set_prereq BSLASHPSPEC
 	test_set_prereq EXECKEEPSPID
 	test_set_prereq NOT_MINGW
 	test_set_prereq SED_STRIPS_CR
-- 
1.7.3

Re: [PATCH 13/14] t4135-*.sh: Skip the "backslash" tests on cygwin

[Johannes Sixt]

On Dienstag, 14. Dezember 2010, Ramsay Jones wrote:
> Note t3700-*.sh has a test protected by BSLASHSPEC which
> previously passed on cygwin and will now be (unnecessarily)
> skipped. This test needs to be skipped on MinGW, given how
> it is written; if you remove the single quotes around the
> filename, however, it will pass even on MinGW.

That is suspicious. It would mean that git add does not do file globbing 
anymore. Should it or should it not do file globbing?

-- Hannes

Re: [PATCH 13/14] t4135-*.sh: Skip the "backslash" tests on cygwin

[Ramsay Jones]

Johannes Sixt wrote:
> On Dienstag, 14. Dezember 2010, Ramsay Jones wrote:
>> Note t3700-*.sh has a test protected by BSLASHSPEC which
>> previously passed on cygwin and will now be (unnecessarily)
>> skipped. This test needs to be skipped on MinGW, given how
>> it is written; if you remove the single quotes around the
>> filename, however, it will pass even on MinGW.
> 
> That is suspicious. It would mean that git add does not do file globbing 
> anymore. Should it or should it not do file globbing?

Hmm, something like "git add 'a.[ch]'" works just fine. The problems
occur when you back-quote the metachars like "git add 'a.\[ch\]'".

The test is skipped on MinGW, because of BSLASHSPEC. The test is now
skipped on cygwin, after this patch, even though it passes on cygwin.
BSLASHSPEC is, apparently, used for both a '\' in a filename and for
a "\-quoting". (Perhaps it should be split into two prerequisites)

The difference in behaviour between cygwin and MinGW (& msvc) is easy
to trace, thus:

cmd_add()
    =>validate_pathspec() builtin/add.c:435
        =>get_pathspec() builtin/add.c:216
            =>prefix_path() setup.c:147
                =>normalize_path_copy() setup.c:18
                    =>is_dir_sep()

on cygwin is_dir_sep() is defined thus:

    git-compat-util.h:208:#define is_dir_sep(c) ((c) == '/')

where on MinGW it is defined thus:

    compat/mingw.h:291:#define is_dir_sep(c) ((c) == '/' || (c) == '\\')

So, on entry to git-add the pathspec (in argv[1]) is
    fo\[ou\]bar
On return from validate_pathspec(), on cygwin it is *still*
    fo\[ou\]bar
but on MinGW (and msvc), it is now
    fo/[ou/]bar

and everything follows from there. (So for example, on cygwin, match_one()
matches fo\[ou\]bar with fo[ou]bar, but not with foobar.)

ATB,
Ramsay Jones

Re: [PATCH 13/14] t4135-*.sh: Skip the "backslash" tests on cygwin

[Johannes Sixt]

On Donnerstag, 16. Dezember 2010, Ramsay Jones wrote:
> Johannes Sixt wrote:
> > On Dienstag, 14. Dezember 2010, Ramsay Jones wrote:
> >> Note t3700-*.sh has a test protected by BSLASHSPEC which
> >> previously passed on cygwin and will now be (unnecessarily)
> >> skipped. This test needs to be skipped on MinGW, given how
> >> it is written; if you remove the single quotes around the
> >> filename, however, it will pass even on MinGW.
> >
> > That is suspicious. It would mean that git add does not do file globbing
> > anymore. Should it or should it not do file globbing?
>
> Hmm, something like "git add 'a.[ch]'" works just fine. The problems
> occur when you back-quote the metachars like "git add 'a.\[ch\]'".
>
> The test is skipped on MinGW, because of BSLASHSPEC. The test is now
> skipped on cygwin, after this patch, even though it passes on cygwin.
> BSLASHSPEC is, apparently, used for both a '\' in a filename and for
> a "\-quoting". (Perhaps it should be split into two prerequisites)
>
> The difference in behaviour between cygwin and MinGW (& msvc) is easy
> to trace, thus:
>
> cmd_add()
>     =>validate_pathspec() builtin/add.c:435
>         =>get_pathspec() builtin/add.c:216
>             =>prefix_path() setup.c:147
>                 =>normalize_path_copy() setup.c:18
>                     =>is_dir_sep()
>
> on cygwin is_dir_sep() is defined thus:
>
>     git-compat-util.h:208:#define is_dir_sep(c) ((c) == '/')
>
> where on MinGW it is defined thus:
>
>     compat/mingw.h:291:#define is_dir_sep(c) ((c) == '/' || (c) == '\\')
>
> So, on entry to git-add the pathspec (in argv[1]) is
>     fo\[ou\]bar
> On return from validate_pathspec(), on cygwin it is *still*
>     fo\[ou\]bar
> but on MinGW (and msvc), it is now
>     fo/[ou/]bar
>
> and everything follows from there. (So for example, on cygwin, match_one()
> matches fo\[ou\]bar with fo[ou]bar, but not with foobar.)

Yes, that's clear, and this is the reason that we must skip this test on 
MinGW.

But you said that when the single quotes are removed, the test passes (and you 
are right). Then git-add sees this pathspec/pattern:

    fo[ou]bar

and it matches 'foobar' when it interprets that as a pattern, but 'fo[ou]bar' 
when it interprets that as straight file name. Even on Linux, the latter 
happens, and *that* is suspicious. What am I missing?

-- Hannes

Re: [PATCH 13/14] t4135-*.sh: Skip the "backslash" tests on cygwin

[Ramsay Jones]

Johannes Sixt wrote:
> Yes, that's clear, and this is the reason that we must skip this test on 
> MinGW.
> 
> But you said that when the single quotes are removed, the test passes (and you 
> are right). Then git-add sees this pathspec/pattern:
> 
>     fo[ou]bar
> 
> and it matches 'foobar' when it interprets that as a pattern, but 'fo[ou]bar' 
> when it interprets that as straight file name. Even on Linux, the latter 
> happens, and *that* is suspicious. What am I missing?

Ah, sorry, I mis-understood your point. :(

So, I decided to have a quick look and ... yeah, something is not quite right!

Hmm, I *think* I have a fix, see patch attached below. This patch provoked a
failure of the unicode tests in t0050-filesystem.sh for me on Linux. However,
these tests are borked when run by the dash shell, but work fine when run by
bash. (see separate e-mail) So, all (non svn) tests pass for me when I run
the tests thus:

    $ SHELL_PATH=/bin/bash make NO_SVN_TESTS=1 test

Of course, this is no guarantee that I haven't messed up all git commands that
use match_one() to process pathspecs, but is at least promising.

The problem boils down to the call to strncmp_icase() suppressing the call to
fnmatch() when the pattern contains glob chars, but the (remaining) string is
equal to the name; thus returning an exact match (MATCHED_EXACTLY) rather than
calling fnmatch (and returning either no-match or MATCHED_FNMATCH).

Note that the test itself is not correct; the argument to git-ls-files should
be quoted the same as the git-add before it ... (well you could pass
fo\\[ou\\]bar instead!).

[BTW, I started looking at the history of this function and I think this
problem has been there for a long time!]

Hmm, I think this is all being rewritten, at the moment (in branch
nd/struct-pathspec) isn't it?

Anyway, let me know what you think...

ATB,
Ramsay Jones

--- 8< ---
From: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
Date: Sun, 19 Dec 2010 19:47:39 +0000
Subject: [PATCH] dir.c: Fix handling of filespecs containing glob-ing chars


Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
---
 dir.c          |    2 +-
 t/t3700-add.sh |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/dir.c b/dir.c
index 852e60f..4e8c7bf 100644
--- a/dir.c
+++ b/dir.c
@@ -139,7 +139,7 @@ static int match_one(const char *match, const char *name, int namelen)
 	 * we need to match by fnmatch
 	 */
 	matchlen = strlen(match);
-	if (strncmp_icase(match, name, matchlen))
+	if (is_glob_special(*match) || strncmp_icase(match, name, matchlen))
 		return !fnmatch_icase(match, name, 0) ? MATCHED_FNMATCH : 0;
 
 	if (namelen == matchlen)
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index ec71083..9140164 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -239,7 +239,7 @@ test_expect_success BSLASHPSPEC "git add 'fo\\[ou\\]bar' ignores foobar" '
 	git reset --hard &&
 	touch fo\[ou\]bar foobar &&
 	git add '\''fo\[ou\]bar'\'' &&
-	git ls-files fo\[ou\]bar | fgrep fo\[ou\]bar &&
+	git ls-files '\''fo\[ou\]bar'\'' | fgrep fo\[ou\]bar &&
 	! ( git ls-files foobar | grep foobar )
 '
 
-- 
1.7.3

Re: [PATCH 13/14] t4135-*.sh: Skip the "backslash" tests on cygwin

[Nguyen Thai Ngoc Duy]

On Wed, Dec 22, 2010 at 2:31 AM, Ramsay Jones
<ramsay@ramsay1.demon.co.uk> wrote:
> The problem boils down to the call to strncmp_icase() suppressing the call to
> fnmatch() when the pattern contains glob chars, but the (remaining) string is
> equal to the name; thus returning an exact match (MATCHED_EXACTLY) rather than
> calling fnmatch (and returning either no-match or MATCHED_FNMATCH).

I think that's expected behavior. Wildcard pathspecs are fixed
pathspecs will additional wildcard matching support and can match both
ways. See 186d604 (glossary: define pathspec)

> [BTW, I started looking at the history of this function and I think this
> problem has been there for a long time!]

Not only in this function. pathspec_matches() in builtin/grep.c
behaves the same (I think).

> Hmm, I think this is all being rewritten, at the moment (in branch
> nd/struct-pathspec) isn't it?

Yes. Thanks for pulling me in. I didn't know recent match_one() has
case-insensitive support.
-- 
Duy

Re: [PATCH 13/14] t4135-*.sh: Skip the "backslash" tests on cygwin

[Ramsay Jones]

Nguyen Thai Ngoc Duy wrote:
> On Wed, Dec 22, 2010 at 2:31 AM, Ramsay Jones
> <ramsay@ramsay1.demon.co.uk> wrote:
>> The problem boils down to the call to strncmp_icase() suppressing the call to
>> fnmatch() when the pattern contains glob chars, but the (remaining) string is
>> equal to the name; thus returning an exact match (MATCHED_EXACTLY) rather than
>> calling fnmatch (and returning either no-match or MATCHED_FNMATCH).
> 
> I think that's expected behavior. Wildcard pathspecs are fixed
> pathspecs will additional wildcard matching support and can match both
> ways. See 186d604 (glossary: define pathspec)

Really? Hmm, that seems ... odd! (To be clear: you are saying that an exact
match, *even if the pattern contains glob chars*, takes precedence over the
glob match! - again *odd*)
Well, if you are happy with that ...

ATB,
Ramsay Jones

Re: [PATCH 13/14] t4135-*.sh: Skip the "backslash" tests on cygwin

[Nguyen Thai Ngoc Duy]

On Wed, Dec 29, 2010 at 1:10 AM, Ramsay Jones
<ramsay@ramsay1.demon.co.uk> wrote:
> Nguyen Thai Ngoc Duy wrote:
>> On Wed, Dec 22, 2010 at 2:31 AM, Ramsay Jones
>> <ramsay@ramsay1.demon.co.uk> wrote:
>>> The problem boils down to the call to strncmp_icase() suppressing the call to
>>> fnmatch() when the pattern contains glob chars, but the (remaining) string is
>>> equal to the name; thus returning an exact match (MATCHED_EXACTLY) rather than
>>> calling fnmatch (and returning either no-match or MATCHED_FNMATCH).
>>
>> I think that's expected behavior. Wildcard pathspecs are fixed
>> pathspecs will additional wildcard matching support and can match both
>> ways. See 186d604 (glossary: define pathspec)
>
> Really? Hmm, that seems ... odd! (To be clear: you are saying that an exact
> match, *even if the pattern contains glob chars*, takes precedence over the
> glob match! - again *odd*)

not exactly taking precedence. I would say it's "normal pathspecs with
extra capability", so it can match more

> Well, if you are happy with that ...

Well, we have two options here: either that, or declare it a day
(near) zero bug [1] with potentially massive impact. Personally I'd go
with which ever way that is less work :) as long as it does not annoy
me (much).

[1] Exerpt from 56fc510 ([PATCH] git-ls-files: generalized pathspecs -
2005-08-21): "the "matching" criteria is a combination of "exact path
component match" (the same as the git-diff-* family), and
"fnmatch()"." Git makes digging history fun.
-- 
Duy