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

PATCH In review Johannes Sixt 3 participants 10 messages Jun 15, 2016

lore

Share & tools

lore

Johannes Sixt

author

2016-06-15 22:58 UTC #0 lore raw

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

Duy Nguyen

2016-06-15 22:58 UTC #1 lore raw

Quoted 37 lines · 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.

Quoted 6 lines

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 ':'

Quoted 8 lines

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

Signature · 1 line

Duy

Alex Riesen

2016-06-15 22:58 UTC #2 lore raw

In reply to Johannes Sixt

Quoted 4 lines · Johannes Sixt

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?

Nguyễn Thái Ngọc Duy

2016-06-15 22:58 UTC #3 lore raw

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

In reply to Johannes Sixt

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 <[email protected]>

---
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(-)

builtin/add.c +13 -0

index 9d52fc7..3402239 100644
@@ -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,

builtin/checkout.c +20 -3

index 7ea1100..e12330f 100644
@@ -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"));

builtin/reset.c +20 -4

index 86150d1..65f7390 100644
@@ -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)

Signature · 1 line

1.8.2.83.gc99314b

Johannes Sixt

author

2016-06-15 22:58 UTC #4 lore raw

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

Am 9/1/2013 4:08, schrieb Nguyễn Thái Ngọc Duy:

Quoted 6 lines

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.

Quoted 7 lines

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>
---
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

Quoted 20 lines

builtin/add.c +13 -0

+#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

Quoted 1 line

[etc.]

-- Hannes

Duy Nguyen

2016-06-15 22:58 UTC #5 lore raw

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

Quoted 25 lines · Johannes Sixt

Am 9/1/2013 4:08, schrieb Nguyễn Thái Ngọc Duy:
Quoted 6 lines
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.
Quoted 7 lines
Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>
---
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

Quoted 4 lines

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

builtin/add.c +6 -0

index 3402239..a138360 100644
@@ -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< --

Johannes Sixt

author

2016-06-15 22:58 UTC #6 lore raw

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

Am 9/2/2013 11:30, schrieb Duy Nguyen:

Quoted 46 lines · Johannes Sixt

Quoted 25 lines
Am 9/1/2013 4:08, schrieb Nguyễn Thái Ngọc Duy:
Quoted 6 lines
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.
Quoted 7 lines
Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>
---
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
Quoted 4 lines
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< --
builtin/add.c +3 -0
index 3402239..a138360 100644
@@ -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++)

builtin/add.c int run_add_interactive(const char *revision, const char *patch_mode,

Quoted 10 lines

builtin/add.c +3 -1

+		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

Duy Nguyen

2016-06-15 22:58 UTC #7 lore raw

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

Quoted 1 line · Johannes Sixt

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.

Signature · 1 line

Duy

Johannes Sixt

author

2016-06-15 22:58 UTC #8 lore raw

PATCH 1/1 add--interactive: fix external command invocation on Windows

From: Johannes Sixt <[email protected]>

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 <[email protected]>

---
Am 9/2/2013 13:56, schrieb Duy Nguyen:

Quoted 6 lines · Johannes Sixt

Quoted 1 line
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(-)

git-add--interactive.perl +1 -1

index 75a991f..5156384 100755
@@ -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 ? "\"$_\"": $_ } @_;

Signature · 21 lines

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 [email protected]
To unsubscribe from this group, send email to
[email protected]
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 [email protected].
For more options, visit https://groups.google.com/groups/opt_out.

Duy Nguyen

2016-06-15 22:58 UTC #9 lore raw

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

Quoted 17 lines · Johannes Sixt

From: Johannes Sixt <[email protected]>
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 <[email protected]>
---
Am 9/2/2013 13:56, schrieb Duy Nguyen:
Quoted 3 lines · Johannes Sixt
Quoted 1 line
Which features do we lose on Windows with the previous patch and this fixup?

Quoted 8 lines

Quoted 3 lines
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.

Quoted 19 lines

 git-add--interactive.perl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

git-add--interactive.perl +1 -2

index 75a991f..5156384 100755
@@ -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

Signature · 1 line

Duy