[PATCH 1/3] pathspec: catch prepending :(prefix) on pathspec with short magic

[PATCH 1/3] pathspec: catch prepending :(prefix) on pathspec with short magic

[Nguyễn Thái Ngọc Duy]

:(prefix) is in the long form. Suppose people pass :!foo with '!'
being the short form of magic 'bar', the code will happily turn it to
:(prefix..)!foo, which makes '!' part of the path and no longer a magic.

The correct form must be ':(prefix..,bar)foo', but as so far we
haven't had any magic in short form yet (*), the code to convert from
short form to long one will be inactive anyway. Let's postpone it
until a real short form magic appears.

(*) The short form magic '/' is a special case and won't be caught by
this die(), which is correct. When '/' magic is detected, prefixlen is
set back to 0 and the whole "if (prefixlen..)" block is skipped.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 fixes on top of nd/magic-pathspec.

 pathspec.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/pathspec.c b/pathspec.c
index d9f4143..62fde50 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -231,7 +231,9 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
 		const char *start = elt;
 		if (prefixlen && !literal_global) {
 			/* Preserve the actual prefix length of each pattern */
-			if (long_magic_end) {
+			if (short_magic)
+				die("BUG: prefixing on short magic is not supported");
+			else if (long_magic_end) {
 				strbuf_add(&sb, start, long_magic_end - start);
 				strbuf_addf(&sb, ",prefix:%d", prefixlen);
 				start = long_magic_end;
-- 
1.8.2.83.gc99314b

[PATCH 2/3] add: lift the pathspec magic restriction on "add -p"

[Nguyễn Thái Ngọc Duy]

Since 480ca64 (convert run_add_interactive to use struct pathspec -
2013-07-14), we have unconditionally passed :(prefix)xxx to
add-interactive.perl. It implies that all commands
add-interactive.perl calls must be aware of pathspec magic, or
:(prefix) is barfed. The restriction to :/ only becomes unnecessary.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/add.c      | 8 +-------
 builtin/checkout.c | 7 -------
 2 files changed, 1 insertion(+), 14 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 9d52fc7..b4035f6 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -270,13 +270,7 @@ int interactive_add(int argc, const char **argv, const char *prefix, int patch)
 {
 	struct pathspec pathspec;
 
-	/*
-	 * git-add--interactive itself does not parse pathspec. It
-	 * simply passes the pathspec to other builtin commands. Let's
-	 * hope all of them support all magic, or we'll need to limit
-	 * the magic here.
-	 */
-	parse_pathspec(&pathspec, PATHSPEC_ALL_MAGIC & ~PATHSPEC_FROMTOP,
+	parse_pathspec(&pathspec, 0,
 		       PATHSPEC_PREFER_FULL |
 		       PATHSPEC_SYMLINK_LEADING_PATH |
 		       PATHSPEC_PREFIX_ORIGIN,
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 7ea1100..b235e04 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1149,13 +1149,6 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	}
 
 	if (argc) {
-		/*
-		 * In patch mode (opts.patch_mode != 0), we pass the
-		 * pathspec to an external program, git-add--interactive.
-		 * Do not accept any kind of magic that that program
-		 * 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);
-- 
1.8.2.83.gc99314b

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

[Nguyễn Thái Ngọc Duy]

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>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 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.2.83.gc99314b

Re: [PATCH 1/3] pathspec: catch prepending :(prefix) on pathspec with short magic

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> :(prefix) is in the long form. Suppose people pass :!foo with '!'
> being the short form of magic 'bar', the code will happily turn it to
> :(prefix..)!foo, which makes '!' part of the path and no longer a magic.
>
> The correct form must be ':(prefix..,bar)foo', but as so far we
> haven't had any magic in short form yet (*), the code to convert from
> short form to long one will be inactive anyway. Let's postpone it
> until a real short form magic appears.
>
> (*) The short form magic '/' is a special case and won't be caught by
> this die(), which is correct. When '/' magic is detected, prefixlen is
> set back to 0 and the whole "if (prefixlen..)" block is skipped.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  fixes on top of nd/magic-pathspec.
>
>  pathspec.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/pathspec.c b/pathspec.c
> index d9f4143..62fde50 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -231,7 +231,9 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
>  		const char *start = elt;
>  		if (prefixlen && !literal_global) {
>  			/* Preserve the actual prefix length of each pattern */
> -			if (long_magic_end) {
> +			if (short_magic)
> +				die("BUG: prefixing on short magic is not supported");
> +			else if (long_magic_end) {
>  				strbuf_add(&sb, start, long_magic_end - start);
>  				strbuf_addf(&sb, ",prefix:%d", prefixlen);
>  				start = long_magic_end;

Good.

I wonder if we should start thinking about removing the big
"NEEDSWORK" comment in front of this function.

Also the pathspec_magic[] array was the table-driven way that was
meant to be enhanced to fit future needs to parse all supported
types of pathspec magic, but it seems that "prefix:12" magic is
implemented using a custom/special case code.  We may want to see if
we want to enrich the parser to fold this to match the table-driven
approach better someday---this is not urgent as we are not adding
any new pathspec magic now.

Will queue.  Thanks.