* nd/magic-pathspec exposes breakage in git-add--interactive on Windows
@ 2013-08-29 6:54 Johannes Sixt
2013-08-29 9:47 ` Duy Nguyen
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Johannes Sixt @ 2013-08-29 6:54 UTC (permalink / raw)
To: Alex Riesen; +Cc: Git Mailing List, Nguyễn Thái Ngọc Duy
With nd/magic-pathspec I get the following failure on Windows in
t2016-checkout-patch.sh:
expecting success:
set_state dir/foo work head &&
# the third n is to get out in case it mistakenly does not apply
(echo y; echo n; echo n) | (cd dir && git checkout -p foo) &&
verify_saved_state bar &&
verify_state dir/foo head head
==== xx.
==== 1d.
==== 10e.
msys does not support: :(prefix:4)dir/foo
not ok 13 - path limiting works: foo inside dir
The error message 'msys does not support...' originates from this function
in git-add--interactive.perl (which is invoked from checkout -p):
sub run_cmd_pipe {
if ($^O eq 'MSWin32' || $^O eq 'msys') {
my @invalid = grep {m/[":*]/} @_;
die "$^O does not support: @invalid\n" if @invalid;
my @args = map { m/ /o ? "\"$_\"": $_ } @_;
return qx{@args};
} else {
my $fh = undef;
open($fh, '-|', @_) or die;
return <$fh>;
}
}
It looks like on Windows we disallow arguments that contain double-quote,
colon, or asterisk, and otherwise wrap arguments in double-quotes if they
contain space. Then pass them through qx{}, which I can only guess what it
does.
This code was introduced in 21e9757e (Hack git-add--interactive to make it
work with ActiveState Perl, 2007-08-01). The commit message has the
general statement "It wont work for arguments with special characters
(like ", : or *)), which I do not know how to interpret: Does ActiveState
Perl not work with special charactoers, or Windows? Because the latter is
definitely not true.
Can we be more permissive in the 'my @invalid' check without breaking
ActiveState Perl?
BTW, there is a similar failure in t7105-reset-patch.sh, which invokes
'git reset -p', but I haven't investigate further.
-- Hannes
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: nd/magic-pathspec exposes breakage in git-add--interactive on Windows 2013-08-29 6:54 nd/magic-pathspec exposes breakage in git-add--interactive on Windows Johannes Sixt @ 2013-08-29 9:47 ` Duy Nguyen 2013-08-29 13:01 ` Alex Riesen 2013-09-01 2:08 ` [PATCH] Turn off pathspec magic on "{checkout,reset,add} -p" on native Windows builds Nguyễn Thái Ngọc Duy 2 siblings, 0 replies; 10+ messages in thread From: Duy Nguyen @ 2013-08-29 9:47 UTC (permalink / raw) To: Johannes Sixt; +Cc: Alex Riesen, Git Mailing List On Thu, Aug 29, 2013 at 1:54 PM, Johannes Sixt <j.sixt@viscovery.net> wrote: > With nd/magic-pathspec I get the following failure on Windows in > t2016-checkout-patch.sh: > > expecting success: > set_state dir/foo work head && > # the third n is to get out in case it mistakenly does not apply > (echo y; echo n; echo n) | (cd dir && git checkout -p foo) && > verify_saved_state bar && > verify_state dir/foo head head > > ==== xx. > ==== 1d. > ==== 10e. > msys does not support: :(prefix:4)dir/foo > not ok 13 - path limiting works: foo inside dir > > The error message 'msys does not support...' originates from this function > in git-add--interactive.perl (which is invoked from checkout -p): > > sub run_cmd_pipe { > if ($^O eq 'MSWin32' || $^O eq 'msys') { > my @invalid = grep {m/[":*]/} @_; > die "$^O does not support: @invalid\n" if @invalid; > my @args = map { m/ /o ? "\"$_\"": $_ } @_; > return qx{@args}; > } else { > my $fh = undef; > open($fh, '-|', @_) or die; > return <$fh>; > } > } > > It looks like on Windows we disallow arguments that contain double-quote, > colon, or asterisk, and otherwise wrap arguments in double-quotes if they > contain space. Then pass them through qx{}, which I can only guess what it > does. > > This code was introduced in 21e9757e (Hack git-add--interactive to make it > work with ActiveState Perl, 2007-08-01). The commit message has the > general statement "It wont work for arguments with special characters > (like ", : or *)), which I do not know how to interpret: Does ActiveState > Perl not work with special charactoers, or Windows? Because the latter is > definitely not true. If it indeed does not work with ActiveState Perl, we still have a chance to make it :(prefix=4)dir/foo, assuming '=' does not fall to the same category as ':' > > Can we be more permissive in the 'my @invalid' check without breaking > ActiveState Perl? > > BTW, there is a similar failure in t7105-reset-patch.sh, which invokes > 'git reset -p', but I haven't investigate further. > > -- Hannes -- Duy ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: nd/magic-pathspec exposes breakage in git-add--interactive on Windows 2013-08-29 6:54 nd/magic-pathspec exposes breakage in git-add--interactive on Windows Johannes Sixt 2013-08-29 9:47 ` Duy Nguyen @ 2013-08-29 13:01 ` Alex Riesen 2013-09-01 2:08 ` [PATCH] Turn off pathspec magic on "{checkout,reset,add} -p" on native Windows builds Nguyễn Thái Ngọc Duy 2 siblings, 0 replies; 10+ messages in thread From: Alex Riesen @ 2013-08-29 13:01 UTC (permalink / raw) To: Johannes Sixt; +Cc: Git Mailing List, Nguyễn Thái Ngọc Duy On Thu, Aug 29, 2013 at 8:54 AM, Johannes Sixt <j.sixt@viscovery.net> wrote: > It looks like on Windows we disallow arguments that contain double-quote, > colon, or asterisk, and otherwise wrap arguments in double-quotes if they > contain space. Then pass them through qx{}, which I can only guess what it > does. Well, the command interpreter of the platform implementation caused this, I believe. Quoting of the double quotes for it was too cumbersome for rarely used combination (windows, ActiveState Perl, and Git). I guess it is more popular now? ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] Turn off pathspec magic on "{checkout,reset,add} -p" on native Windows builds 2013-08-29 6:54 nd/magic-pathspec exposes breakage in git-add--interactive on Windows Johannes Sixt 2013-08-29 9:47 ` Duy Nguyen 2013-08-29 13:01 ` Alex Riesen @ 2013-09-01 2:08 ` Nguyễn Thái Ngọc Duy 2013-09-02 6:42 ` Johannes Sixt 2 siblings, 1 reply; 10+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2013-09-01 2:08 UTC (permalink / raw) To: Johannes Sixt; +Cc: git, Alex Riesen, Nguyễn Thái Ngọc Duy git-add--interactive.perl rejects arguments with colons in 21e9757 (Hack git-add--interactive to make it work with ActiveState Perl - 2007-08-01). Pathspec magic starts with a colon, so it won't work if these pathspecs are passed to git-add--interactive.perl running with ActiveState Perl. Make sure we only pass plain paths in this case. Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- Johannes, can you check the test suite passes for you with this patch? I assume that Cygwin Perl behaves differently and does not hit this limit. So I keep the special case to GIT_WINDOWS_NATIVE only. I'll resend the patch with a few others on the same topic if it works for you. builtin/add.c | 13 +++++++++++++ builtin/checkout.c | 23 ++++++++++++++++++++--- builtin/reset.c | 24 ++++++++++++++++++++---- 3 files changed, 53 insertions(+), 7 deletions(-) diff --git a/builtin/add.c b/builtin/add.c index 9d52fc7..3402239 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -270,6 +270,18 @@ int interactive_add(int argc, const char **argv, const char *prefix, int patch) { struct pathspec pathspec; +#ifdef GIT_WINDOWS_NATIVE + /* + * Pathspec magic is completely turned off on native Windows + * builds because git-add-interactive.perl won't accept + * arguments with colons in them. :/foo is an exception + * because no colons remain after parsing. + */ + parse_pathspec(&pathspec, PATHSPEC_ALL_MAGIC & ~PATHSPEC_FROMTOP, + PATHSPEC_PREFER_FULL | + PATHSPEC_SYMLINK_LEADING_PATH, + prefix, argv); +#else /* * git-add--interactive itself does not parse pathspec. It * simply passes the pathspec to other builtin commands. Let's @@ -281,6 +293,7 @@ int interactive_add(int argc, const char **argv, const char *prefix, int patch) PATHSPEC_SYMLINK_LEADING_PATH | PATHSPEC_PREFIX_ORIGIN, prefix, argv); +#endif return run_add_interactive(NULL, patch ? "--patch" : NULL, diff --git a/builtin/checkout.c b/builtin/checkout.c index 7ea1100..e12330f 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -1156,9 +1156,26 @@ int cmd_checkout(int argc, const char **argv, const char *prefix) * cannot handle. Magic mask is pretty safe to be * lifted for new magic when opts.patch_mode == 0. */ - parse_pathspec(&opts.pathspec, 0, - opts.patch_mode ? PATHSPEC_PREFIX_ORIGIN : 0, - prefix, argv); + if (opts.patch_mode) { +#ifdef GIT_WINDOWS_NATIVE + /* + * Pathspec magic is completely turned off on + * native Windows builds because + * git-add-interactive.perl won't accept + * arguments with colons in them. :/foo is an + * exception because no colons remain after + * parsing. + */ + parse_pathspec(&opts.pathspec, + PATHSPEC_ALL_MAGIC & ~PATHSPEC_FROMTOP, + 0, prefix, argv); +#else + parse_pathspec(&opts.pathspec, 0, + PATHSPEC_PREFIX_ORIGIN, + prefix, argv); +#endif + } else + parse_pathspec(&opts.pathspec, 0, 0, prefix, argv); if (!opts.pathspec.nr) die(_("invalid path specification")); diff --git a/builtin/reset.c b/builtin/reset.c index 86150d1..65f7390 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -220,10 +220,26 @@ static void parse_args(struct pathspec *pathspec, } } *rev_ret = rev; - parse_pathspec(pathspec, 0, - PATHSPEC_PREFER_FULL | - (patch_mode ? PATHSPEC_PREFIX_ORIGIN : 0), - prefix, argv); + if (patch_mode) { +#ifdef GIT_WINDOWS_NATIVE + /* + * Pathspec magic is completely turned off on native + * Windows builds because git-add-interactive.perl + * won't accept arguments with colons in them. :/foo + * is an exception because no colons remain after + * parsing. + */ + parse_pathspec(pathspec, + PATHSPEC_ALL_MAGIC & ~PATHSPEC_FROMTOP, + PATHSPEC_PREFER_FULL, prefix, argv); +#else + parse_pathspec(pathspec, 0, + PATHSPEC_PREFER_FULL | PATHSPEC_PREFIX_ORIGIN, + prefix, argv); +#endif + } else + parse_pathspec(pathspec, 0, PATHSPEC_PREFER_FULL, + prefix, argv); } static int update_refs(const char *rev, const unsigned char *sha1) -- 1.8.2.83.gc99314b ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] Turn off pathspec magic on "{checkout,reset,add} -p" on native Windows builds 2013-09-01 2:08 ` [PATCH] Turn off pathspec magic on "{checkout,reset,add} -p" on native Windows builds Nguyễn Thái Ngọc Duy @ 2013-09-02 6:42 ` Johannes Sixt 2013-09-02 9:30 ` Duy Nguyen 0 siblings, 1 reply; 10+ messages in thread From: Johannes Sixt @ 2013-09-02 6:42 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy; +Cc: git, Alex Riesen Am 9/1/2013 4:08, schrieb Nguyễn Thái Ngọc Duy: > git-add--interactive.perl rejects arguments with colons in 21e9757 > (Hack git-add--interactive to make it work with ActiveState Perl - > 2007-08-01). Pathspec magic starts with a colon, so it won't work if > these pathspecs are passed to git-add--interactive.perl running with > ActiveState Perl. Make sure we only pass plain paths in this case. > > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> > --- > Johannes, can you check the test suite passes for you with this > patch? I assume that Cygwin Perl behaves differently and does not hit > this limit. So I keep the special case to GIT_WINDOWS_NATIVE only. > I'll resend the patch with a few others on the same topic if it works > for you. It does not help. The error in git-add--interactive is avoided, but the failure in t2016-checkout-patch.sh is now: expecting success: set_state dir/foo work head && # the third n is to get out in case it mistakenly does not apply (echo y; echo n; echo n) | (cd dir && git checkout -p foo) && verify_saved_state bar && verify_state dir/foo head head No changes. not ok 13 - path limiting works: foo inside dir and the same "No changes." happens in t7105-reset-patch.sh > +#ifdef GIT_WINDOWS_NATIVE > + /* > + * Pathspec magic is completely turned off on native Windows > + * builds because git-add-interactive.perl won't accept > + * arguments with colons in them. :/foo is an exception > + * because no colons remain after parsing. > + */ > + parse_pathspec(&pathspec, PATHSPEC_ALL_MAGIC & ~PATHSPEC_FROMTOP, > + PATHSPEC_PREFER_FULL | > + PATHSPEC_SYMLINK_LEADING_PATH, > + prefix, argv); > +#else > /* > * git-add--interactive itself does not parse pathspec. It > * simply passes the pathspec to other builtin commands. Let's > @@ -281,6 +293,7 @@ int interactive_add(int argc, const char **argv, const char *prefix, int patch) > PATHSPEC_SYMLINK_LEADING_PATH | > PATHSPEC_PREFIX_ORIGIN, > prefix, argv); > +#endif > [etc.] -- Hannes ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Turn off pathspec magic on "{checkout,reset,add} -p" on native Windows builds 2013-09-02 6:42 ` Johannes Sixt @ 2013-09-02 9:30 ` Duy Nguyen 2013-09-02 10:41 ` Johannes Sixt 0 siblings, 1 reply; 10+ messages in thread From: Duy Nguyen @ 2013-09-02 9:30 UTC (permalink / raw) To: Johannes Sixt; +Cc: git, Alex Riesen On Mon, Sep 02, 2013 at 08:42:18AM +0200, Johannes Sixt wrote: > Am 9/1/2013 4:08, schrieb Nguyễn Thái Ngọc Duy: > > git-add--interactive.perl rejects arguments with colons in 21e9757 > > (Hack git-add--interactive to make it work with ActiveState Perl - > > 2007-08-01). Pathspec magic starts with a colon, so it won't work if > > these pathspecs are passed to git-add--interactive.perl running with > > ActiveState Perl. Make sure we only pass plain paths in this case. > > > > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> > > --- > > Johannes, can you check the test suite passes for you with this > > patch? I assume that Cygwin Perl behaves differently and does not hit > > this limit. So I keep the special case to GIT_WINDOWS_NATIVE only. > > I'll resend the patch with a few others on the same topic if it works > > for you. > > It does not help. The error in git-add--interactive is avoided, but the > failure in t2016-checkout-patch.sh is now: > > expecting success: > set_state dir/foo work head && > # the third n is to get out in case it mistakenly does not apply > (echo y; echo n; echo n) | (cd dir && git checkout -p foo) && > verify_saved_state bar && > verify_state dir/foo head head > > No changes. > not ok 13 - path limiting works: foo inside dir > > and the same "No changes." happens in t7105-reset-patch.sh Right. Because I got rid of ':(prefix)foo' form but I passed 'foo' instead of 'dir/foo'. How about this on top? -- 8< -- diff --git a/builtin/add.c b/builtin/add.c index 3402239..a138360 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -257,9 +257,15 @@ int run_add_interactive(const char *revision, const char *patch_mode, if (revision) args[ac++] = revision; args[ac++] = "--"; +#ifdef GIT_WINDOWS_NATIVE + GUARD_PATHSPEC(pathspec, PATHSPEC_FROMTOP); + for (i = 0; i < pathspec->nr; i++) + args[ac++] = pathspec->items[i].match; +#else for (i = 0; i < pathspec->nr; i++) /* pass original pathspec, to be re-parsed */ args[ac++] = pathspec->items[i].original; +#endif status = run_command_v_opt(args, RUN_GIT_CMD); free(args); -- 8< -- ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] Turn off pathspec magic on "{checkout,reset,add} -p" on native Windows builds 2013-09-02 9:30 ` Duy Nguyen @ 2013-09-02 10:41 ` Johannes Sixt 2013-09-02 11:56 ` Duy Nguyen 0 siblings, 1 reply; 10+ messages in thread From: Johannes Sixt @ 2013-09-02 10:41 UTC (permalink / raw) To: Duy Nguyen; +Cc: git, Alex Riesen Am 9/2/2013 11:30, schrieb Duy Nguyen: > On Mon, Sep 02, 2013 at 08:42:18AM +0200, Johannes Sixt wrote: >> Am 9/1/2013 4:08, schrieb Nguyễn Thái Ngọc Duy: >>> git-add--interactive.perl rejects arguments with colons in 21e9757 >>> (Hack git-add--interactive to make it work with ActiveState Perl - >>> 2007-08-01). Pathspec magic starts with a colon, so it won't work if >>> these pathspecs are passed to git-add--interactive.perl running with >>> ActiveState Perl. Make sure we only pass plain paths in this case. >>> >>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> >>> --- >>> Johannes, can you check the test suite passes for you with this >>> patch? I assume that Cygwin Perl behaves differently and does not hit >>> this limit. So I keep the special case to GIT_WINDOWS_NATIVE only. >>> I'll resend the patch with a few others on the same topic if it works >>> for you. >> >> It does not help. The error in git-add--interactive is avoided, but the >> failure in t2016-checkout-patch.sh is now: >> >> expecting success: >> set_state dir/foo work head && >> # the third n is to get out in case it mistakenly does not apply >> (echo y; echo n; echo n) | (cd dir && git checkout -p foo) && >> verify_saved_state bar && >> verify_state dir/foo head head >> >> No changes. >> not ok 13 - path limiting works: foo inside dir >> >> and the same "No changes." happens in t7105-reset-patch.sh > > Right. Because I got rid of ':(prefix)foo' form but I passed 'foo' > instead of 'dir/foo'. How about this on top? > > -- 8< -- > diff --git a/builtin/add.c b/builtin/add.c > index 3402239..a138360 100644 > --- a/builtin/add.c > +++ b/builtin/add.c > @@ -257,9 +257,15 @@ int run_add_interactive(const char *revision, const char *patch_mode, > if (revision) > args[ac++] = revision; > args[ac++] = "--"; > +#ifdef GIT_WINDOWS_NATIVE > + GUARD_PATHSPEC(pathspec, PATHSPEC_FROMTOP); > + for (i = 0; i < pathspec->nr; i++) > + args[ac++] = pathspec->items[i].match; > +#else > for (i = 0; i < pathspec->nr; i++) > /* pass original pathspec, to be re-parsed */ > args[ac++] = pathspec->items[i].original; > +#endif > > status = run_command_v_opt(args, RUN_GIT_CMD); > free(args); > -- 8< -- With this patch, the two tests pass. Which features do we lose on Windows with the previous patch and this fixup? -- Hannes ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Turn off pathspec magic on "{checkout,reset,add} -p" on native Windows builds 2013-09-02 10:41 ` Johannes Sixt @ 2013-09-02 11:56 ` Duy Nguyen 2013-09-04 7:24 ` [PATCH] add--interactive: fix external command invocation on Windows Johannes Sixt 0 siblings, 1 reply; 10+ messages in thread From: Duy Nguyen @ 2013-09-02 11:56 UTC (permalink / raw) To: Johannes Sixt; +Cc: Git Mailing List, Alex Riesen On Mon, Sep 2, 2013 at 5:41 PM, Johannes Sixt <j.sixt@viscovery.net> wrote: > Which features do we lose on Windows with the previous patch and this fixup? New pathspec magic :(glob), :(literal) and :(icase). You can still use them via --*-pathspecs or equivalent env variables. You just can't enable them per individual pathspec. -- Duy ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] add--interactive: fix external command invocation on Windows 2013-09-02 11:56 ` Duy Nguyen @ 2013-09-04 7:24 ` Johannes Sixt 2013-09-04 12:02 ` Duy Nguyen 0 siblings, 1 reply; 10+ messages in thread From: Johannes Sixt @ 2013-09-04 7:24 UTC (permalink / raw) To: Duy Nguyen; +Cc: Git Mailing List, Alex Riesen, msysGit From: Johannes Sixt <j6t@kdbg.org> Back in 21e9757e (Hack git-add--interactive to make it work with ActiveState Perl, 2007-08-01), the invocation of external commands was changed to use qx{} on Windows. The rationale was that the command interpreter on Windows is not a POSIX shell, but rather Windows's CMD. That patch was wrong to include 'msys' in the check whether to use qx{} or not: 'msys' identifies MSYS perl as shipped with Git for Windows, which does not need the special treatment; qx{} should be used only with ActiveState perl, which is identified by 'MSWin32'. Signed-off-by: Johannes Sixt <j6t@kdbg.org> --- Am 9/2/2013 13:56, schrieb Duy Nguyen: > On Mon, Sep 2, 2013 at 5:41 PM, Johannes Sixt <j.sixt@viscovery.net> wrote: >> Which features do we lose on Windows with the previous patch and this fixup? > > New pathspec magic :(glob), :(literal) and :(icase). You can still use > them via --*-pathspecs or equivalent env variables. You just can't > enable them per individual pathspec. I think this here is the correct "solution" rather than the special cases for Windows that you proposed. ActiveState perl users would still suffer, but that is a problem of the run_cmd_pipe implementation, which would need a smarter method to quote the arguments before it feeds them to qx{}. git-add--interactive.perl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/git-add--interactive.perl b/git-add--interactive.perl index 75a991f..5156384 100755 --- a/git-add--interactive.perl +++ b/git-add--interactive.perl @@ -169,7 +169,7 @@ my %patch_modes = ( my %patch_mode_flavour = %{$patch_modes{stage}}; sub run_cmd_pipe { - if ($^O eq 'MSWin32' || $^O eq 'msys') { + if ($^O eq 'MSWin32') { my @invalid = grep {m/[":*]/} @_; die "$^O does not support: @invalid\n" if @invalid; my @args = map { m/ /o ? "\"$_\"": $_ } @_; -- 1.8.4.1549.gc059550.dirty -- -- *** Please reply-to-all at all times *** *** (do not pretend to know who is subscribed and who is not) *** *** Please avoid top-posting. *** The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free. You received this message because you are subscribed to the Google Groups "msysGit" group. To post to this group, send email to msysgit@googlegroups.com To unsubscribe from this group, send email to msysgit+unsubscribe@googlegroups.com For more options, and view previous threads, visit this group at http://groups.google.com/group/msysgit?hl=en_US?hl=en --- You received this message because you are subscribed to the Google Groups "msysGit" group. To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com. For more options, visit https://groups.google.com/groups/opt_out. ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] add--interactive: fix external command invocation on Windows 2013-09-04 7:24 ` [PATCH] add--interactive: fix external command invocation on Windows Johannes Sixt @ 2013-09-04 12:02 ` Duy Nguyen 0 siblings, 0 replies; 10+ messages in thread From: Duy Nguyen @ 2013-09-04 12:02 UTC (permalink / raw) To: Johannes Sixt; +Cc: Git Mailing List, Alex Riesen, msysGit On Wed, Sep 4, 2013 at 2:24 PM, Johannes Sixt <j.sixt@viscovery.net> wrote: > From: Johannes Sixt <j6t@kdbg.org> > > Back in 21e9757e (Hack git-add--interactive to make it work with > ActiveState Perl, 2007-08-01), the invocation of external commands was > changed to use qx{} on Windows. The rationale was that the command > interpreter on Windows is not a POSIX shell, but rather Windows's CMD. > That patch was wrong to include 'msys' in the check whether to use qx{} > or not: 'msys' identifies MSYS perl as shipped with Git for Windows, > which does not need the special treatment; qx{} should be used only with > ActiveState perl, which is identified by 'MSWin32'. > > Signed-off-by: Johannes Sixt <j6t@kdbg.org> > --- > Am 9/2/2013 13:56, schrieb Duy Nguyen: >> On Mon, Sep 2, 2013 at 5:41 PM, Johannes Sixt <j.sixt@viscovery.net> wrote: >>> Which features do we lose on Windows with the previous patch and this fixup? >> >> New pathspec magic :(glob), :(literal) and :(icase). You can still use >> them via --*-pathspecs or equivalent env variables. You just can't >> enable them per individual pathspec. > > I think this here is the correct "solution" rather than the special cases > for Windows that you proposed. ActiveState perl users would still suffer, > but that is a problem of the run_cmd_pipe implementation, which would > need a smarter method to quote the arguments before it feeds them to qx{}. I admit this is cleaner than my patch. Let's see any users using ActiveState Perl yell up, then we'll deal with the problem. > git-add--interactive.perl | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/git-add--interactive.perl b/git-add--interactive.perl > index 75a991f..5156384 100755 > --- a/git-add--interactive.perl > +++ b/git-add--interactive.perl > @@ -169,7 +169,7 @@ my %patch_modes = ( > my %patch_mode_flavour = %{$patch_modes{stage}}; > > sub run_cmd_pipe { > - if ($^O eq 'MSWin32' || $^O eq 'msys') { > + if ($^O eq 'MSWin32') { > my @invalid = grep {m/[":*]/} @_; > die "$^O does not support: @invalid\n" if @invalid; > my @args = map { m/ /o ? "\"$_\"": $_ } @_; > -- > 1.8.4.1549.gc059550.dirty > -- Duy ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2013-09-04 12:02 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-08-29 6:54 nd/magic-pathspec exposes breakage in git-add--interactive on Windows Johannes Sixt
2013-08-29 9:47 ` Duy Nguyen
2013-08-29 13:01 ` Alex Riesen
2013-09-01 2:08 ` [PATCH] Turn off pathspec magic on "{checkout,reset,add} -p" on native Windows builds Nguyễn Thái Ngọc Duy
2013-09-02 6:42 ` Johannes Sixt
2013-09-02 9:30 ` Duy Nguyen
2013-09-02 10:41 ` Johannes Sixt
2013-09-02 11:56 ` Duy Nguyen
2013-09-04 7:24 ` [PATCH] add--interactive: fix external command invocation on Windows Johannes Sixt
2013-09-04 12:02 ` Duy Nguyen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).