nd/magic-pathspec exposes breakage in git-add--interactive on Windows

nd/magic-pathspec exposes breakage in git-add--interactive on Windows

[Johannes Sixt]

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

Re: nd/magic-pathspec exposes breakage in git-add--interactive on Windows

[Duy Nguyen]

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

Re: nd/magic-pathspec exposes breakage in git-add--interactive on Windows

[Alex Riesen]

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?

[PATCH] Turn off pathspec magic on "{checkout,reset,add} -p" on native Windows builds

[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

Re: [PATCH] Turn off pathspec magic on "{checkout,reset,add} -p" on native Windows builds

[Johannes Sixt]

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

Re: [PATCH] Turn off pathspec magic on "{checkout,reset,add} -p" on native Windows builds

[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< --

Re: [PATCH] Turn off pathspec magic on "{checkout,reset,add} -p" on native Windows builds

[Johannes Sixt]

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

Re: [PATCH] Turn off pathspec magic on "{checkout,reset,add} -p" on native Windows builds

[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.
-- 
Duy

[PATCH] add--interactive: fix external command invocation on Windows

[Johannes Sixt]

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.

Re: [PATCH] add--interactive: fix external command invocation on Windows

[Duy Nguyen]

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