{"thread":{"id":"34792","subject":"nd/magic-pathspec exposes breakage in git-add--interactive on Windows","startedAt":"2013-08-29T06:54:34Z","lastAt":"2013-09-04T12:02:17Z","messageCount":10,"participants":["Johannes Sixt","Duy Nguyen","Alex Riesen","Nguyễn Thái Ngọc Duy"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"226139","messageId":"521EF02A.2020300@viscovery.net","threadId":"34792","inReplyTo":null,"subject":"nd/magic-pathspec exposes breakage in git-add--interactive on Windows","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2013-08-29T06:54:34Z","receivedAt":"2013-08-29T06:54:34Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"With nd/magic-pathspec I get the following failure on Windows in\nt2016-checkout-patch.sh:\n\nexpecting success:\n        set_state dir/foo work head &&\n        # the third n is to get out in case it mistakenly does not apply\n        (echo y; echo n; echo n) | (cd dir && git checkout -p foo) &&\n        verify_saved_state bar &&\n        verify_state dir/foo head head\n\n==== xx.\n==== 1d.\n==== 10e.\nmsys does not support: :(prefix:4)dir/foo\nnot ok 13 - path limiting works: foo inside dir\n\nThe error message 'msys does not support...' originates from this function\nin git-add--interactive.perl (which is invoked from checkout -p):\n\n  sub run_cmd_pipe {\n\tif ($^O eq 'MSWin32' || $^O eq 'msys') {\n\t\tmy @invalid = grep {m/[\":*]/} @_;\n\t\tdie \"$^O does not support: @invalid\\n\" if @invalid;\n\t\tmy @args = map { m/ /o ? \"\\\"$_\\\"\": $_ } @_;\n\t\treturn qx{@args};\n\t} else {\n\t\tmy $fh = undef;\n\t\topen($fh, '-|', @_) or die;\n\t\treturn <$fh>;\n\t}\n  }\n\nIt looks like on Windows we disallow arguments that contain double-quote,\ncolon, or asterisk, and otherwise wrap arguments in double-quotes if they\ncontain space. Then pass them through qx{}, which I can only guess what it\ndoes.\n\nThis code was introduced in 21e9757e (Hack git-add--interactive to make it\nwork with ActiveState Perl, 2007-08-01). The commit message has the\ngeneral statement \"It wont work for arguments with special characters\n(like \", : or *)), which I do not know how to interpret: Does ActiveState\nPerl not work with special charactoers, or Windows? Because the latter is\ndefinitely not true.\n\nCan we be more permissive in the 'my @invalid' check without breaking\nActiveState Perl?\n\nBTW, there is a similar failure in t7105-reset-patch.sh, which invokes\n'git reset -p', but I haven't investigate further.\n\n-- Hannes\n"},{"id":"226146","messageId":"CACsJy8B50Xb=S7wp=2H7NHPoM52XXZgBbygTJahFyRzuqT-=HQ@mail.gmail.com","threadId":"34792","inReplyTo":"521EF02A.2020300@viscovery.net","subject":"Re: nd/magic-pathspec exposes breakage in git-add--interactive on Windows","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-08-29T09:47:26Z","receivedAt":"2013-08-29T09:47:26Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Aug 29, 2013 at 1:54 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:\n> With nd/magic-pathspec I get the following failure on Windows in\n> t2016-checkout-patch.sh:\n>\n> expecting success:\n>         set_state dir/foo work head &&\n>         # the third n is to get out in case it mistakenly does not apply\n>         (echo y; echo n; echo n) | (cd dir && git checkout -p foo) &&\n>         verify_saved_state bar &&\n>         verify_state dir/foo head head\n>\n> ==== xx.\n> ==== 1d.\n> ==== 10e.\n> msys does not support: :(prefix:4)dir/foo\n> not ok 13 - path limiting works: foo inside dir\n>\n> The error message 'msys does not support...' originates from this function\n> in git-add--interactive.perl (which is invoked from checkout -p):\n>\n>   sub run_cmd_pipe {\n>         if ($^O eq 'MSWin32' || $^O eq 'msys') {\n>                 my @invalid = grep {m/[\":*]/} @_;\n>                 die \"$^O does not support: @invalid\\n\" if @invalid;\n>                 my @args = map { m/ /o ? \"\\\"$_\\\"\": $_ } @_;\n>                 return qx{@args};\n>         } else {\n>                 my $fh = undef;\n>                 open($fh, '-|', @_) or die;\n>                 return <$fh>;\n>         }\n>   }\n>\n> It looks like on Windows we disallow arguments that contain double-quote,\n> colon, or asterisk, and otherwise wrap arguments in double-quotes if they\n> contain space. Then pass them through qx{}, which I can only guess what it\n> does.\n>\n> This code was introduced in 21e9757e (Hack git-add--interactive to make it\n> work with ActiveState Perl, 2007-08-01). The commit message has the\n> general statement \"It wont work for arguments with special characters\n> (like \", : or *)), which I do not know how to interpret: Does ActiveState\n> Perl not work with special charactoers, or Windows? Because the latter is\n> definitely not true.\n\nIf it indeed does not work with ActiveState Perl, we still have a\nchance to make it :(prefix=4)dir/foo, assuming '=' does not fall to\nthe same category as ':'\n\n>\n> Can we be more permissive in the 'my @invalid' check without breaking\n> ActiveState Perl?\n>\n> BTW, there is a similar failure in t7105-reset-patch.sh, which invokes\n> 'git reset -p', but I haven't investigate further.\n>\n> -- Hannes\n-- \nDuy\n"},{"id":"226152","messageId":"CALxABCZaeS9eC50CxeqHLrKm530YXH1TRKtSUQEsd81L3Jxwyg@mail.gmail.com","threadId":"34792","inReplyTo":"521EF02A.2020300@viscovery.net","subject":"Re: nd/magic-pathspec exposes breakage in git-add--interactive on Windows","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2013-08-29T13:01:38Z","receivedAt":"2013-08-29T13:01:38Z","isPatch":false,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On Thu, Aug 29, 2013 at 8:54 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:\n> It looks like on Windows we disallow arguments that contain double-quote,\n> colon, or asterisk, and otherwise wrap arguments in double-quotes if they\n> contain space. Then pass them through qx{}, which I can only guess what it\n> does.\n\nWell, the command interpreter of the platform implementation caused\nthis, I believe.\nQuoting of the double quotes for it was too cumbersome for rarely used\ncombination\n(windows, ActiveState Perl, and Git). I guess it is more popular now?\n"},{"id":"226457","messageId":"1378001284-18426-1-git-send-email-pclouds@gmail.com","threadId":"34792","inReplyTo":"521EF02A.2020300@viscovery.net","subject":"[PATCH] Turn off pathspec magic on \"{checkout,reset,add} -p\" on native Windows builds","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-09-01T02:08:04Z","receivedAt":"2013-09-01T02:08:04Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"git-add--interactive.perl rejects arguments with colons in 21e9757\n(Hack git-add--interactive to make it work with ActiveState Perl -\n2007-08-01). Pathspec magic starts with a colon, so it won't work if\nthese pathspecs are passed to git-add--interactive.perl running with\nActiveState Perl. Make sure we only pass plain paths in this case.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n Johannes, can you check the test suite passes for you with this\n patch? I assume that Cygwin Perl behaves differently and does not hit\n this limit. So I keep the special case to GIT_WINDOWS_NATIVE only.\n I'll resend the patch with a few others on the same topic if it works\n for you.\n\n builtin/add.c      | 13 +++++++++++++\n builtin/checkout.c | 23 ++++++++++++++++++++---\n builtin/reset.c    | 24 ++++++++++++++++++++----\n 3 files changed, 53 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 9d52fc7..3402239 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -270,6 +270,18 @@ int interactive_add(int argc, const char **argv, const char *prefix, int patch)\n {\n \tstruct pathspec pathspec;\n \n+#ifdef GIT_WINDOWS_NATIVE\n+\t/*\n+\t * Pathspec magic is completely turned off on native Windows\n+\t * builds because git-add-interactive.perl won't accept\n+\t * arguments with colons in them. :/foo is an exception\n+\t * because no colons remain after parsing.\n+\t */\n+\tparse_pathspec(&pathspec, PATHSPEC_ALL_MAGIC & ~PATHSPEC_FROMTOP,\n+\t\t       PATHSPEC_PREFER_FULL |\n+\t\t       PATHSPEC_SYMLINK_LEADING_PATH,\n+\t\t       prefix, argv);\n+#else\n \t/*\n \t * git-add--interactive itself does not parse pathspec. It\n \t * simply passes the pathspec to other builtin commands. Let's\n@@ -281,6 +293,7 @@ int interactive_add(int argc, const char **argv, const char *prefix, int patch)\n \t\t       PATHSPEC_SYMLINK_LEADING_PATH |\n \t\t       PATHSPEC_PREFIX_ORIGIN,\n \t\t       prefix, argv);\n+#endif\n \n \treturn run_add_interactive(NULL,\n \t\t\t\t   patch ? \"--patch\" : NULL,\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 7ea1100..e12330f 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1156,9 +1156,26 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \t\t * cannot handle. Magic mask is pretty safe to be\n \t\t * lifted for new magic when opts.patch_mode == 0.\n \t\t */\n-\t\tparse_pathspec(&opts.pathspec, 0,\n-\t\t\t       opts.patch_mode ? PATHSPEC_PREFIX_ORIGIN : 0,\n-\t\t\t       prefix, argv);\n+\t\tif (opts.patch_mode) {\n+#ifdef GIT_WINDOWS_NATIVE\n+\t\t\t/*\n+\t\t\t * Pathspec magic is completely turned off on\n+\t\t\t * native Windows builds because\n+\t\t\t * git-add-interactive.perl won't accept\n+\t\t\t * arguments with colons in them. :/foo is an\n+\t\t\t * exception because no colons remain after\n+\t\t\t * parsing.\n+\t\t\t */\n+\t\t\tparse_pathspec(&opts.pathspec,\n+\t\t\t\t       PATHSPEC_ALL_MAGIC & ~PATHSPEC_FROMTOP,\n+\t\t\t\t       0, prefix, argv);\n+#else\n+\t\t\tparse_pathspec(&opts.pathspec, 0,\n+\t\t\t\t       PATHSPEC_PREFIX_ORIGIN,\n+\t\t\t\t       prefix, argv);\n+#endif\n+\t\t} else\n+\t\t\tparse_pathspec(&opts.pathspec, 0, 0, prefix, argv);\n \n \t\tif (!opts.pathspec.nr)\n \t\t\tdie(_(\"invalid path specification\"));\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 86150d1..65f7390 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -220,10 +220,26 @@ static void parse_args(struct pathspec *pathspec,\n \t\t}\n \t}\n \t*rev_ret = rev;\n-\tparse_pathspec(pathspec, 0,\n-\t\t       PATHSPEC_PREFER_FULL |\n-\t\t       (patch_mode ? PATHSPEC_PREFIX_ORIGIN : 0),\n-\t\t       prefix, argv);\n+\tif (patch_mode) {\n+#ifdef GIT_WINDOWS_NATIVE\n+\t\t/*\n+\t\t * Pathspec magic is completely turned off on native\n+\t\t * Windows builds because git-add-interactive.perl\n+\t\t * won't accept arguments with colons in them. :/foo\n+\t\t * is an exception because no colons remain after\n+\t\t * parsing.\n+\t\t */\n+\t\tparse_pathspec(pathspec,\n+\t\t\t       PATHSPEC_ALL_MAGIC & ~PATHSPEC_FROMTOP,\n+\t\t\t       PATHSPEC_PREFER_FULL, prefix, argv);\n+#else\n+\t\tparse_pathspec(pathspec, 0,\n+\t\t\t       PATHSPEC_PREFER_FULL | PATHSPEC_PREFIX_ORIGIN,\n+\t\t\t       prefix, argv);\n+#endif\n+\t} else\n+\t\tparse_pathspec(pathspec, 0, PATHSPEC_PREFER_FULL,\n+\t\t\t       prefix, argv);\n }\n \n static int update_refs(const char *rev, const unsigned char *sha1)\n-- \n1.8.2.83.gc99314b\n"},{"id":"226514","messageId":"5224334A.2090300@viscovery.net","threadId":"34792","inReplyTo":"1378001284-18426-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] Turn off pathspec magic on \"{checkout,reset,add} -p\" on native Windows builds","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2013-09-02T06:42:18Z","receivedAt":"2013-09-02T06:42:18Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 9/1/2013 4:08, schrieb Nguyễn Thái Ngọc Duy:\n> git-add--interactive.perl rejects arguments with colons in 21e9757\n> (Hack git-add--interactive to make it work with ActiveState Perl -\n> 2007-08-01). Pathspec magic starts with a colon, so it won't work if\n> these pathspecs are passed to git-add--interactive.perl running with\n> ActiveState Perl. Make sure we only pass plain paths in this case.\n> \n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>  Johannes, can you check the test suite passes for you with this\n>  patch? I assume that Cygwin Perl behaves differently and does not hit\n>  this limit. So I keep the special case to GIT_WINDOWS_NATIVE only.\n>  I'll resend the patch with a few others on the same topic if it works\n>  for you.\n\nIt does not help. The error in git-add--interactive is avoided, but the\nfailure in t2016-checkout-patch.sh is now:\n\nexpecting success:\n        set_state dir/foo work head &&\n        # the third n is to get out in case it mistakenly does not apply\n        (echo y; echo n; echo n) | (cd dir && git checkout -p foo) &&\n        verify_saved_state bar &&\n        verify_state dir/foo head head\n\nNo changes.\nnot ok 13 - path limiting works: foo inside dir\n\nand the same \"No changes.\" happens in t7105-reset-patch.sh\n\n> +#ifdef GIT_WINDOWS_NATIVE\n> +\t/*\n> +\t * Pathspec magic is completely turned off on native Windows\n> +\t * builds because git-add-interactive.perl won't accept\n> +\t * arguments with colons in them. :/foo is an exception\n> +\t * because no colons remain after parsing.\n> +\t */\n> +\tparse_pathspec(&pathspec, PATHSPEC_ALL_MAGIC & ~PATHSPEC_FROMTOP,\n> +\t\t       PATHSPEC_PREFER_FULL |\n> +\t\t       PATHSPEC_SYMLINK_LEADING_PATH,\n> +\t\t       prefix, argv);\n> +#else\n>  \t/*\n>  \t * git-add--interactive itself does not parse pathspec. It\n>  \t * simply passes the pathspec to other builtin commands. Let's\n> @@ -281,6 +293,7 @@ int interactive_add(int argc, const char **argv, const char *prefix, int patch)\n>  \t\t       PATHSPEC_SYMLINK_LEADING_PATH |\n>  \t\t       PATHSPEC_PREFIX_ORIGIN,\n>  \t\t       prefix, argv);\n> +#endif\n> [etc.]\n\n-- Hannes\n"},{"id":"226527","messageId":"20130902092959.GA17306@lanh","threadId":"34792","inReplyTo":"5224334A.2090300@viscovery.net","subject":"Re: [PATCH] Turn off pathspec magic on \"{checkout,reset,add} -p\" on native Windows builds","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-09-02T09:30:00Z","receivedAt":"2013-09-02T09:30:00Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Sep 02, 2013 at 08:42:18AM +0200, Johannes Sixt wrote:\n> Am 9/1/2013 4:08, schrieb Nguyễn Thái Ngọc Duy:\n> > git-add--interactive.perl rejects arguments with colons in 21e9757\n> > (Hack git-add--interactive to make it work with ActiveState Perl -\n> > 2007-08-01). Pathspec magic starts with a colon, so it won't work if\n> > these pathspecs are passed to git-add--interactive.perl running with\n> > ActiveState Perl. Make sure we only pass plain paths in this case.\n> > \n> > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> > ---\n> >  Johannes, can you check the test suite passes for you with this\n> >  patch? I assume that Cygwin Perl behaves differently and does not hit\n> >  this limit. So I keep the special case to GIT_WINDOWS_NATIVE only.\n> >  I'll resend the patch with a few others on the same topic if it works\n> >  for you.\n> \n> It does not help. The error in git-add--interactive is avoided, but the\n> failure in t2016-checkout-patch.sh is now:\n> \n> expecting success:\n>         set_state dir/foo work head &&\n>         # the third n is to get out in case it mistakenly does not apply\n>         (echo y; echo n; echo n) | (cd dir && git checkout -p foo) &&\n>         verify_saved_state bar &&\n>         verify_state dir/foo head head\n> \n> No changes.\n> not ok 13 - path limiting works: foo inside dir\n> \n> and the same \"No changes.\" happens in t7105-reset-patch.sh\n\nRight. Because I got rid of ':(prefix)foo' form but I passed 'foo'\ninstead of 'dir/foo'. How about this on top?\n\n-- 8< --\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 3402239..a138360 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -257,9 +257,15 @@ int run_add_interactive(const char *revision, const char *patch_mode,\n \tif (revision)\n \t\targs[ac++] = revision;\n \targs[ac++] = \"--\";\n+#ifdef GIT_WINDOWS_NATIVE\n+\tGUARD_PATHSPEC(pathspec, PATHSPEC_FROMTOP);\n+\tfor (i = 0; i < pathspec->nr; i++)\n+\t\targs[ac++] = pathspec->items[i].match;\n+#else\n \tfor (i = 0; i < pathspec->nr; i++)\n \t\t/* pass original pathspec, to be re-parsed */\n \t\targs[ac++] = pathspec->items[i].original;\n+#endif\n \n \tstatus = run_command_v_opt(args, RUN_GIT_CMD);\n \tfree(args);\n-- 8< --\n"},{"id":"226528","messageId":"52246B70.3080200@viscovery.net","threadId":"34792","inReplyTo":"20130902092959.GA17306@lanh","subject":"Re: [PATCH] Turn off pathspec magic on \"{checkout,reset,add} -p\" on native Windows builds","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2013-09-02T10:41:52Z","receivedAt":"2013-09-02T10:41:52Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 9/2/2013 11:30, schrieb Duy Nguyen:\n> On Mon, Sep 02, 2013 at 08:42:18AM +0200, Johannes Sixt wrote:\n>> Am 9/1/2013 4:08, schrieb Nguyễn Thái Ngọc Duy:\n>>> git-add--interactive.perl rejects arguments with colons in 21e9757\n>>> (Hack git-add--interactive to make it work with ActiveState Perl -\n>>> 2007-08-01). Pathspec magic starts with a colon, so it won't work if\n>>> these pathspecs are passed to git-add--interactive.perl running with\n>>> ActiveState Perl. Make sure we only pass plain paths in this case.\n>>>\n>>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n>>> ---\n>>>  Johannes, can you check the test suite passes for you with this\n>>>  patch? I assume that Cygwin Perl behaves differently and does not hit\n>>>  this limit. So I keep the special case to GIT_WINDOWS_NATIVE only.\n>>>  I'll resend the patch with a few others on the same topic if it works\n>>>  for you.\n>>\n>> It does not help. The error in git-add--interactive is avoided, but the\n>> failure in t2016-checkout-patch.sh is now:\n>>\n>> expecting success:\n>>         set_state dir/foo work head &&\n>>         # the third n is to get out in case it mistakenly does not apply\n>>         (echo y; echo n; echo n) | (cd dir && git checkout -p foo) &&\n>>         verify_saved_state bar &&\n>>         verify_state dir/foo head head\n>>\n>> No changes.\n>> not ok 13 - path limiting works: foo inside dir\n>>\n>> and the same \"No changes.\" happens in t7105-reset-patch.sh\n> \n> Right. Because I got rid of ':(prefix)foo' form but I passed 'foo'\n> instead of 'dir/foo'. How about this on top?\n> \n> -- 8< --\n> diff --git a/builtin/add.c b/builtin/add.c\n> index 3402239..a138360 100644\n> --- a/builtin/add.c\n> +++ b/builtin/add.c\n> @@ -257,9 +257,15 @@ int run_add_interactive(const char *revision, const char *patch_mode,\n>  \tif (revision)\n>  \t\targs[ac++] = revision;\n>  \targs[ac++] = \"--\";\n> +#ifdef GIT_WINDOWS_NATIVE\n> +\tGUARD_PATHSPEC(pathspec, PATHSPEC_FROMTOP);\n> +\tfor (i = 0; i < pathspec->nr; i++)\n> +\t\targs[ac++] = pathspec->items[i].match;\n> +#else\n>  \tfor (i = 0; i < pathspec->nr; i++)\n>  \t\t/* pass original pathspec, to be re-parsed */\n>  \t\targs[ac++] = pathspec->items[i].original;\n> +#endif\n>  \n>  \tstatus = run_command_v_opt(args, RUN_GIT_CMD);\n>  \tfree(args);\n> -- 8< --\n\nWith this patch, the two tests pass.\n\nWhich features do we lose on Windows with the previous patch and this fixup?\n\n-- Hannes\n"},{"id":"226529","messageId":"CACsJy8BxGVyVsOV=XyWRqSnNjna0LQOAk6rgJ_jRkVox1jUSxQ@mail.gmail.com","threadId":"34792","inReplyTo":"52246B70.3080200@viscovery.net","subject":"Re: [PATCH] Turn off pathspec magic on \"{checkout,reset,add} -p\" on native Windows builds","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-09-02T11:56:13Z","receivedAt":"2013-09-02T11:56:13Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Sep 2, 2013 at 5:41 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:\n> Which features do we lose on Windows with the previous patch and this fixup?\n\nNew pathspec magic :(glob), :(literal) and :(icase). You can still use\nthem via --*-pathspecs or equivalent env variables. You just can't\nenable them per individual pathspec.\n-- \nDuy\n"},{"id":"226705","messageId":"5226E03F.208@viscovery.net","threadId":"34792","inReplyTo":"CACsJy8BxGVyVsOV=XyWRqSnNjna0LQOAk6rgJ_jRkVox1jUSxQ@mail.gmail.com","subject":"[PATCH] add--interactive: fix external command invocation on Windows","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2013-09-04T07:24:47Z","receivedAt":"2013-09-04T07:24:47Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"From: Johannes Sixt <j6t@kdbg.org>\n\nBack in 21e9757e (Hack git-add--interactive to make it work with\nActiveState Perl, 2007-08-01), the invocation of external commands was\nchanged to use qx{} on Windows. The rationale was that the command\ninterpreter on Windows is not a POSIX shell, but rather Windows's CMD.\nThat patch was wrong to include 'msys' in the check whether to use qx{}\nor not: 'msys' identifies MSYS perl as shipped with Git for Windows,\nwhich does not need the special treatment; qx{} should be used only with\nActiveState perl, which is identified by 'MSWin32'.\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\nAm 9/2/2013 13:56, schrieb Duy Nguyen:\n> On Mon, Sep 2, 2013 at 5:41 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:\n>> Which features do we lose on Windows with the previous patch and this fixup?\n> \n> New pathspec magic :(glob), :(literal) and :(icase). You can still use\n> them via --*-pathspecs or equivalent env variables. You just can't\n> enable them per individual pathspec.\n\nI think this here is the correct \"solution\" rather than the special cases\nfor Windows that you proposed. ActiveState perl users would still suffer,\nbut that is a problem of the run_cmd_pipe implementation, which would\nneed a smarter method to quote the arguments before it feeds them to qx{}.\n\n git-add--interactive.perl | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/git-add--interactive.perl b/git-add--interactive.perl\nindex 75a991f..5156384 100755\n--- a/git-add--interactive.perl\n+++ b/git-add--interactive.perl\n@@ -169,7 +169,7 @@ my %patch_modes = (\n my %patch_mode_flavour = %{$patch_modes{stage}};\n \n sub run_cmd_pipe {\n-\tif ($^O eq 'MSWin32' || $^O eq 'msys') {\n+\tif ($^O eq 'MSWin32') {\n \t\tmy @invalid = grep {m/[\":*]/} @_;\n \t\tdie \"$^O does not support: @invalid\\n\" if @invalid;\n \t\tmy @args = map { m/ /o ? \"\\\"$_\\\"\": $_ } @_;\n-- \n1.8.4.1549.gc059550.dirty\n\n-- \n-- \n*** Please reply-to-all at all times ***\n*** (do not pretend to know who is subscribed and who is not) ***\n*** Please avoid top-posting. ***\nThe msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.\n\nYou received this message because you are subscribed to the Google\nGroups \"msysGit\" group.\nTo post to this group, send email to msysgit@googlegroups.com\nTo unsubscribe from this group, send email to\nmsysgit+unsubscribe@googlegroups.com\nFor more options, and view previous threads, visit this group at\nhttp://groups.google.com/group/msysgit?hl=en_US?hl=en\n\n--- \nYou received this message because you are subscribed to the Google Groups \"msysGit\" group.\nTo unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.\nFor more options, visit https://groups.google.com/groups/opt_out.\n"},{"id":"226725","messageId":"CACsJy8DbkNz8rTFDV3u9j3+mmzF0JVdO+TNVBX6uRQPCCfis_g@mail.gmail.com","threadId":"34792","inReplyTo":"5226E03F.208@viscovery.net","subject":"Re: [PATCH] add--interactive: fix external command invocation on Windows","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-09-04T12:02:17Z","receivedAt":"2013-09-04T12:02:17Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Sep 4, 2013 at 2:24 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:\n> From: Johannes Sixt <j6t@kdbg.org>\n>\n> Back in 21e9757e (Hack git-add--interactive to make it work with\n> ActiveState Perl, 2007-08-01), the invocation of external commands was\n> changed to use qx{} on Windows. The rationale was that the command\n> interpreter on Windows is not a POSIX shell, but rather Windows's CMD.\n> That patch was wrong to include 'msys' in the check whether to use qx{}\n> or not: 'msys' identifies MSYS perl as shipped with Git for Windows,\n> which does not need the special treatment; qx{} should be used only with\n> ActiveState perl, which is identified by 'MSWin32'.\n>\n> Signed-off-by: Johannes Sixt <j6t@kdbg.org>\n> ---\n> Am 9/2/2013 13:56, schrieb Duy Nguyen:\n>> On Mon, Sep 2, 2013 at 5:41 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:\n>>> Which features do we lose on Windows with the previous patch and this fixup?\n>>\n>> New pathspec magic :(glob), :(literal) and :(icase). You can still use\n>> them via --*-pathspecs or equivalent env variables. You just can't\n>> enable them per individual pathspec.\n>\n> I think this here is the correct \"solution\" rather than the special cases\n> for Windows that you proposed. ActiveState perl users would still suffer,\n> but that is a problem of the run_cmd_pipe implementation, which would\n> need a smarter method to quote the arguments before it feeds them to qx{}.\n\nI admit this is cleaner than my patch. Let's see any users using\nActiveState Perl yell up, then we'll deal with the problem.\n\n>  git-add--interactive.perl | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/git-add--interactive.perl b/git-add--interactive.perl\n> index 75a991f..5156384 100755\n> --- a/git-add--interactive.perl\n> +++ b/git-add--interactive.perl\n> @@ -169,7 +169,7 @@ my %patch_modes = (\n>  my %patch_mode_flavour = %{$patch_modes{stage}};\n>\n>  sub run_cmd_pipe {\n> -       if ($^O eq 'MSWin32' || $^O eq 'msys') {\n> +       if ($^O eq 'MSWin32') {\n>                 my @invalid = grep {m/[\":*]/} @_;\n>                 die \"$^O does not support: @invalid\\n\" if @invalid;\n>                 my @args = map { m/ /o ? \"\\\"$_\\\"\": $_ } @_;\n> --\n> 1.8.4.1549.gc059550.dirty\n>\n-- \nDuy\n"}]}