{"thread":{"id":"26062","subject":"[PATCH 13/14] t4135-*.sh: Skip the \"backslash\" tests on cygwin","startedAt":"2010-12-14T18:37:43Z","lastAt":"2010-12-29T13:56:55Z","messageCount":8,"participants":["Ramsay Jones","Johannes Sixt","Nguyen Thai Ngoc Duy"],"isPatch":true,"patchVersion":1,"patchTotal":14},"messages":[{"id":"158074","messageId":"4D07B977.9010502@ramsay1.demon.co.uk","threadId":"26062","inReplyTo":null,"subject":"[PATCH 13/14] t4135-*.sh: Skip the \"backslash\" tests on cygwin","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2010-12-14T18:37:43Z","receivedAt":"2010-12-14T18:37:43Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nThe BSLASHPSPEC tests (11-13) fail on cygwin, since you can't\ncreate files containing an backslash character in the name.\nIn order to skip these tests, we simply stop (incorrectly)\nasserting the BSLASHPSPEC prerequisite in test-lib.sh.\n\nSigned-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\n---\n\nNote t3700-*.sh has a test protected by BSLASHSPEC which\npreviously passed on cygwin and will now be (unnecessarily)\nskipped. This test needs to be skipped on MinGW, given how\nit is written; if you remove the single quotes around the\nfilename, however, it will pass even on MinGW.\n\n t/test-lib.sh |    1 -\n 1 files changed, 0 insertions(+), 1 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 9e74357..aee7d20 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1061,7 +1061,6 @@ case $(uname -s) in\n \t;;\n *CYGWIN*)\n \ttest_set_prereq POSIXPERM\n-\ttest_set_prereq BSLASHPSPEC\n \ttest_set_prereq EXECKEEPSPID\n \ttest_set_prereq NOT_MINGW\n \ttest_set_prereq SED_STRIPS_CR\n-- \n1.7.3\n"},{"id":"158090","messageId":"201012142149.33725.j6t@kdbg.org","threadId":"26062","inReplyTo":"4D07B977.9010502@ramsay1.demon.co.uk","subject":"Re: [PATCH 13/14] t4135-*.sh: Skip the \"backslash\" tests on cygwin","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-12-14T20:49:33Z","receivedAt":"2010-12-14T20:49:33Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Dienstag, 14. Dezember 2010, Ramsay Jones wrote:\n> Note t3700-*.sh has a test protected by BSLASHSPEC which\n> previously passed on cygwin and will now be (unnecessarily)\n> skipped. This test needs to be skipped on MinGW, given how\n> it is written; if you remove the single quotes around the\n> filename, however, it will pass even on MinGW.\n\nThat is suspicious. It would mean that git add does not do file globbing \nanymore. Should it or should it not do file globbing?\n\n-- Hannes\n"},{"id":"158236","messageId":"4D0A94D8.6090206@ramsay1.demon.co.uk","threadId":"26062","inReplyTo":"201012142149.33725.j6t@kdbg.org","subject":"Re: [PATCH 13/14] t4135-*.sh: Skip the \"backslash\" tests on cygwin","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2010-12-16T22:38:16Z","receivedAt":"2010-12-16T22:38:16Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Johannes Sixt wrote:\n> On Dienstag, 14. Dezember 2010, Ramsay Jones wrote:\n>> Note t3700-*.sh has a test protected by BSLASHSPEC which\n>> previously passed on cygwin and will now be (unnecessarily)\n>> skipped. This test needs to be skipped on MinGW, given how\n>> it is written; if you remove the single quotes around the\n>> filename, however, it will pass even on MinGW.\n> \n> That is suspicious. It would mean that git add does not do file globbing \n> anymore. Should it or should it not do file globbing?\n\nHmm, something like \"git add 'a.[ch]'\" works just fine. The problems\noccur when you back-quote the metachars like \"git add 'a.\\[ch\\]'\".\n\nThe test is skipped on MinGW, because of BSLASHSPEC. The test is now\nskipped on cygwin, after this patch, even though it passes on cygwin.\nBSLASHSPEC is, apparently, used for both a '\\' in a filename and for\na \"\\-quoting\". (Perhaps it should be split into two prerequisites)\n\nThe difference in behaviour between cygwin and MinGW (& msvc) is easy\nto trace, thus:\n\ncmd_add()\n    =>validate_pathspec() builtin/add.c:435\n        =>get_pathspec() builtin/add.c:216\n            =>prefix_path() setup.c:147\n                =>normalize_path_copy() setup.c:18\n                    =>is_dir_sep()\n\non cygwin is_dir_sep() is defined thus:\n\n    git-compat-util.h:208:#define is_dir_sep(c) ((c) == '/')\n\nwhere on MinGW it is defined thus:\n\n    compat/mingw.h:291:#define is_dir_sep(c) ((c) == '/' || (c) == '\\\\')\n\nSo, on entry to git-add the pathspec (in argv[1]) is\n    fo\\[ou\\]bar\nOn return from validate_pathspec(), on cygwin it is *still*\n    fo\\[ou\\]bar\nbut on MinGW (and msvc), it is now\n    fo/[ou/]bar\n\nand everything follows from there. (So for example, on cygwin, match_one()\nmatches fo\\[ou\\]bar with fo[ou]bar, but not with foobar.)\n\nATB,\nRamsay Jones\n"},{"id":"158297","messageId":"201012172254.31242.j6t@kdbg.org","threadId":"26062","inReplyTo":"4D0A94D8.6090206@ramsay1.demon.co.uk","subject":"Re: [PATCH 13/14] t4135-*.sh: Skip the \"backslash\" tests on cygwin","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-12-17T21:54:31Z","receivedAt":"2010-12-17T21:54:31Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Donnerstag, 16. Dezember 2010, Ramsay Jones wrote:\n> Johannes Sixt wrote:\n> > On Dienstag, 14. Dezember 2010, Ramsay Jones wrote:\n> >> Note t3700-*.sh has a test protected by BSLASHSPEC which\n> >> previously passed on cygwin and will now be (unnecessarily)\n> >> skipped. This test needs to be skipped on MinGW, given how\n> >> it is written; if you remove the single quotes around the\n> >> filename, however, it will pass even on MinGW.\n> >\n> > That is suspicious. It would mean that git add does not do file globbing\n> > anymore. Should it or should it not do file globbing?\n>\n> Hmm, something like \"git add 'a.[ch]'\" works just fine. The problems\n> occur when you back-quote the metachars like \"git add 'a.\\[ch\\]'\".\n>\n> The test is skipped on MinGW, because of BSLASHSPEC. The test is now\n> skipped on cygwin, after this patch, even though it passes on cygwin.\n> BSLASHSPEC is, apparently, used for both a '\\' in a filename and for\n> a \"\\-quoting\". (Perhaps it should be split into two prerequisites)\n>\n> The difference in behaviour between cygwin and MinGW (& msvc) is easy\n> to trace, thus:\n>\n> cmd_add()\n>     =>validate_pathspec() builtin/add.c:435\n>         =>get_pathspec() builtin/add.c:216\n>             =>prefix_path() setup.c:147\n>                 =>normalize_path_copy() setup.c:18\n>                     =>is_dir_sep()\n>\n> on cygwin is_dir_sep() is defined thus:\n>\n>     git-compat-util.h:208:#define is_dir_sep(c) ((c) == '/')\n>\n> where on MinGW it is defined thus:\n>\n>     compat/mingw.h:291:#define is_dir_sep(c) ((c) == '/' || (c) == '\\\\')\n>\n> So, on entry to git-add the pathspec (in argv[1]) is\n>     fo\\[ou\\]bar\n> On return from validate_pathspec(), on cygwin it is *still*\n>     fo\\[ou\\]bar\n> but on MinGW (and msvc), it is now\n>     fo/[ou/]bar\n>\n> and everything follows from there. (So for example, on cygwin, match_one()\n> matches fo\\[ou\\]bar with fo[ou]bar, but not with foobar.)\n\nYes, that's clear, and this is the reason that we must skip this test on \nMinGW.\n\nBut you said that when the single quotes are removed, the test passes (and you \nare right). Then git-add sees this pathspec/pattern:\n\n    fo[ou]bar\n\nand it matches 'foobar' when it interprets that as a pattern, but 'fo[ou]bar' \nwhen it interprets that as straight file name. Even on Linux, the latter \nhappens, and *that* is suspicious. What am I missing?\n\n-- Hannes\n"},{"id":"158452","messageId":"4D1100A3.2010309@ramsay1.demon.co.uk","threadId":"26062","inReplyTo":"201012172254.31242.j6t@kdbg.org","subject":"Re: [PATCH 13/14] t4135-*.sh: Skip the \"backslash\" tests on cygwin","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2010-12-21T19:31:47Z","receivedAt":"2010-12-21T19:31:47Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Johannes Sixt wrote:\n> Yes, that's clear, and this is the reason that we must skip this test on \n> MinGW.\n> \n> But you said that when the single quotes are removed, the test passes (and you \n> are right). Then git-add sees this pathspec/pattern:\n> \n>     fo[ou]bar\n> \n> and it matches 'foobar' when it interprets that as a pattern, but 'fo[ou]bar' \n> when it interprets that as straight file name. Even on Linux, the latter \n> happens, and *that* is suspicious. What am I missing?\n\nAh, sorry, I mis-understood your point. :(\n\nSo, I decided to have a quick look and ... yeah, something is not quite right!\n\nHmm, I *think* I have a fix, see patch attached below. This patch provoked a\nfailure of the unicode tests in t0050-filesystem.sh for me on Linux. However,\nthese tests are borked when run by the dash shell, but work fine when run by\nbash. (see separate e-mail) So, all (non svn) tests pass for me when I run\nthe tests thus:\n\n    $ SHELL_PATH=/bin/bash make NO_SVN_TESTS=1 test\n\nOf course, this is no guarantee that I haven't messed up all git commands that\nuse match_one() to process pathspecs, but is at least promising.\n\nThe problem boils down to the call to strncmp_icase() suppressing the call to\nfnmatch() when the pattern contains glob chars, but the (remaining) string is\nequal to the name; thus returning an exact match (MATCHED_EXACTLY) rather than\ncalling fnmatch (and returning either no-match or MATCHED_FNMATCH).\n\nNote that the test itself is not correct; the argument to git-ls-files should\nbe quoted the same as the git-add before it ... (well you could pass\nfo\\\\[ou\\\\]bar instead!).\n\n[BTW, I started looking at the history of this function and I think this\nproblem has been there for a long time!]\n\nHmm, I think this is all being rewritten, at the moment (in branch\nnd/struct-pathspec) isn't it?\n\nAnyway, let me know what you think...\n\nATB,\nRamsay Jones\n\n--- 8< ---\nFrom: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\nDate: Sun, 19 Dec 2010 19:47:39 +0000\nSubject: [PATCH] dir.c: Fix handling of filespecs containing glob-ing chars\n\n\nSigned-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\n---\n dir.c          |    2 +-\n t/t3700-add.sh |    2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 852e60f..4e8c7bf 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -139,7 +139,7 @@ static int match_one(const char *match, const char *name, int namelen)\n \t * we need to match by fnmatch\n \t */\n \tmatchlen = strlen(match);\n-\tif (strncmp_icase(match, name, matchlen))\n+\tif (is_glob_special(*match) || strncmp_icase(match, name, matchlen))\n \t\treturn !fnmatch_icase(match, name, 0) ? MATCHED_FNMATCH : 0;\n \n \tif (namelen == matchlen)\ndiff --git a/t/t3700-add.sh b/t/t3700-add.sh\nindex ec71083..9140164 100755\n--- a/t/t3700-add.sh\n+++ b/t/t3700-add.sh\n@@ -239,7 +239,7 @@ test_expect_success BSLASHPSPEC \"git add 'fo\\\\[ou\\\\]bar' ignores foobar\" '\n \tgit reset --hard &&\n \ttouch fo\\[ou\\]bar foobar &&\n \tgit add '\\''fo\\[ou\\]bar'\\'' &&\n-\tgit ls-files fo\\[ou\\]bar | fgrep fo\\[ou\\]bar &&\n+\tgit ls-files '\\''fo\\[ou\\]bar'\\'' | fgrep fo\\[ou\\]bar &&\n \t! ( git ls-files foobar | grep foobar )\n '\n \n-- \n1.7.3\n"},{"id":"158469","messageId":"AANLkTi=-7ihdW_cMiAqnyjbUE_d35OFrSSnQQw4mi64G@mail.gmail.com","threadId":"26062","inReplyTo":"4D1100A3.2010309@ramsay1.demon.co.uk","subject":"Re: [PATCH 13/14] t4135-*.sh: Skip the \"backslash\" tests on cygwin","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-12-22T01:44:32Z","receivedAt":"2010-12-22T01:44:32Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Dec 22, 2010 at 2:31 AM, Ramsay Jones\n<ramsay@ramsay1.demon.co.uk> wrote:\n> The problem boils down to the call to strncmp_icase() suppressing the call to\n> fnmatch() when the pattern contains glob chars, but the (remaining) string is\n> equal to the name; thus returning an exact match (MATCHED_EXACTLY) rather than\n> calling fnmatch (and returning either no-match or MATCHED_FNMATCH).\n\nI think that's expected behavior. Wildcard pathspecs are fixed\npathspecs will additional wildcard matching support and can match both\nways. See 186d604 (glossary: define pathspec)\n\n> [BTW, I started looking at the history of this function and I think this\n> problem has been there for a long time!]\n\nNot only in this function. pathspec_matches() in builtin/grep.c\nbehaves the same (I think).\n\n> Hmm, I think this is all being rewritten, at the moment (in branch\n> nd/struct-pathspec) isn't it?\n\nYes. Thanks for pulling me in. I didn't know recent match_one() has\ncase-insensitive support.\n-- \nDuy\n"},{"id":"158682","messageId":"4D1A2827.6070503@ramsay1.demon.co.uk","threadId":"26062","inReplyTo":"AANLkTi=-7ihdW_cMiAqnyjbUE_d35OFrSSnQQw4mi64G@mail.gmail.com","subject":"Re: [PATCH 13/14] t4135-*.sh: Skip the \"backslash\" tests on cygwin","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2010-12-28T18:10:47Z","receivedAt":"2010-12-28T18:10:47Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Nguyen Thai Ngoc Duy wrote:\n> On Wed, Dec 22, 2010 at 2:31 AM, Ramsay Jones\n> <ramsay@ramsay1.demon.co.uk> wrote:\n>> The problem boils down to the call to strncmp_icase() suppressing the call to\n>> fnmatch() when the pattern contains glob chars, but the (remaining) string is\n>> equal to the name; thus returning an exact match (MATCHED_EXACTLY) rather than\n>> calling fnmatch (and returning either no-match or MATCHED_FNMATCH).\n> \n> I think that's expected behavior. Wildcard pathspecs are fixed\n> pathspecs will additional wildcard matching support and can match both\n> ways. See 186d604 (glossary: define pathspec)\n\nReally? Hmm, that seems ... odd! (To be clear: you are saying that an exact\nmatch, *even if the pattern contains glob chars*, takes precedence over the\nglob match! - again *odd*)\nWell, if you are happy with that ...\n\nATB,\nRamsay Jones\n"},{"id":"158709","messageId":"AANLkTiknOthWgmZxEpit9qA--54NPKq=SqPV7hv9SOPJ@mail.gmail.com","threadId":"26062","inReplyTo":"4D1A2827.6070503@ramsay1.demon.co.uk","subject":"Re: [PATCH 13/14] t4135-*.sh: Skip the \"backslash\" tests on cygwin","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-12-29T13:56:55Z","receivedAt":"2010-12-29T13:56:55Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Dec 29, 2010 at 1:10 AM, Ramsay Jones\n<ramsay@ramsay1.demon.co.uk> wrote:\n> Nguyen Thai Ngoc Duy wrote:\n>> On Wed, Dec 22, 2010 at 2:31 AM, Ramsay Jones\n>> <ramsay@ramsay1.demon.co.uk> wrote:\n>>> The problem boils down to the call to strncmp_icase() suppressing the call to\n>>> fnmatch() when the pattern contains glob chars, but the (remaining) string is\n>>> equal to the name; thus returning an exact match (MATCHED_EXACTLY) rather than\n>>> calling fnmatch (and returning either no-match or MATCHED_FNMATCH).\n>>\n>> I think that's expected behavior. Wildcard pathspecs are fixed\n>> pathspecs will additional wildcard matching support and can match both\n>> ways. See 186d604 (glossary: define pathspec)\n>\n> Really? Hmm, that seems ... odd! (To be clear: you are saying that an exact\n> match, *even if the pattern contains glob chars*, takes precedence over the\n> glob match! - again *odd*)\n\nnot exactly taking precedence. I would say it's \"normal pathspecs with\nextra capability\", so it can match more\n\n> Well, if you are happy with that ...\n\nWell, we have two options here: either that, or declare it a day\n(near) zero bug [1] with potentially massive impact. Personally I'd go\nwith which ever way that is less work :) as long as it does not annoy\nme (much).\n\n[1] Exerpt from 56fc510 ([PATCH] git-ls-files: generalized pathspecs -\n2005-08-21): \"the \"matching\" criteria is a combination of \"exact path\ncomponent match\" (the same as the git-diff-* family), and\n\"fnmatch()\".\" Git makes digging history fun.\n-- \nDuy\n"}]}