[BUG] "git --literal-pathspecs blame" broken in master

[BUG] "git --literal-pathspecs blame" broken in master

[Jeff King]

There seems to be a bad interaction with --literal-pathspecs and the
pathspec magic that is in master. Here's an example:

  [setup]
  $ git init
  $ echo content >':(foo)bar'
  $ git add . && git commit -m foo

  [with git v1.8.4]
  $ git blame ':(foo)bar'
  ^6b07eb4 (Jeff King 2013-10-24 22:59:02 -0400 1) content
  $ git --literal-pathspecs blame ':(foo)bar'
  ^6b07eb4 (Jeff King 2013-10-24 22:59:02 -0400 1) content

So originally "blame" did not handle pathspec magic at all, and
--literal-pathspecs did nothing. So the former is arguably buggy, but
the latter did the right thing (if only by accident :) ).

Then along came 9a08727 (remove init_pathspec() in favor of
parse_pathspec(), 2013-07-14), and we get:

  $ git blame ':(foo)bar'
  fatal: Invalid pathspec magic 'foo' in ':(foo)bar'
  $ git --literal-pathspecs blame ':(foo)bar'
  fatal: Invalid pathspec magic 'foo' in ':(foo)bar'

The first is an improvement, because we are now consistent with other
pathspec handling in git. But the latter is a regression; we are not
respecting the literal pathspec flag.

We get another change with a16bf9d (pathspec: make --literal-pathspecs
disable pathspec magic, 2013-07-14), which I would think would fix
things, but doesn't.

  $ git blame ':(foo)bar'
  fatal: Invalid pathspec magic 'foo' in ':(foo)bar'
  $ git --literal-pathspecs blame ':(foo)bar'
  fatal: :(foo)bar: pathspec magic not supported by this command: 'literal'

The first one remains good, but the second one is still broken. I
haven't dug further yet, but I thought it might be a bit more obvious to
you.

-Peff

Re: [BUG] "git --literal-pathspecs blame" broken in master

[Jeff King]

On Thu, Oct 24, 2013 at 11:49:47PM -0400, Jeff King wrote:

> We get another change with a16bf9d (pathspec: make --literal-pathspecs
> disable pathspec magic, 2013-07-14), which I would think would fix
> things, but doesn't.
> 
>   $ git blame ':(foo)bar'
>   fatal: Invalid pathspec magic 'foo' in ':(foo)bar'
>   $ git --literal-pathspecs blame ':(foo)bar'
>   fatal: :(foo)bar: pathspec magic not supported by this command: 'literal'
> 
> The first one remains good, but the second one is still broken. I
> haven't dug further yet, but I thought it might be a bit more obvious to
> you.

Hmm. Is the fix as simple as:

diff --git a/builtin/blame.c b/builtin/blame.c
index 56e3d6b..1c2b303 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -408,7 +408,7 @@ static struct origin *find_origin(struct scoreboard *sb,
 	paths[0] = origin->path;
 	paths[1] = NULL;
 
-	parse_pathspec(&diff_opts.pathspec, PATHSPEC_ALL_MAGIC, 0, "", paths);
+	parse_pathspec(&diff_opts.pathspec, PATHSPEC_ALL_MAGIC & ~PATHSPEC_LITERAL, 0, "", paths);
 	diff_setup_done(&diff_opts);
 
 	if (is_null_sha1(origin->commit->object.sha1))

All of the GUARD_PATHSPEC calls indicate that everybody understands
PATHSPEC_LITERAL. It is not technically true that git-blame understands
the literal pathspec magic:

  $ git blame -- ':(literal)revision.c'
  fatal: no such path ':(literal)revision.c' in HEAD

but that is a separate bug (that blame considers the argument as a path
first before feeding it to the pathspec machinery). The patch above does
not fix that, but AFAICT it does not make anything worse.

-Peff

Re: [BUG] "git --literal-pathspecs blame" broken in master

[Duy Nguyen]

On Fri, Oct 25, 2013 at 11:04 AM, Jeff King <peff@peff.net> wrote:
> On Thu, Oct 24, 2013 at 11:49:47PM -0400, Jeff King wrote:
>
>> We get another change with a16bf9d (pathspec: make --literal-pathspecs
>> disable pathspec magic, 2013-07-14), which I would think would fix
>> things, but doesn't.
>>
>>   $ git blame ':(foo)bar'
>>   fatal: Invalid pathspec magic 'foo' in ':(foo)bar'
>>   $ git --literal-pathspecs blame ':(foo)bar'
>>   fatal: :(foo)bar: pathspec magic not supported by this command: 'literal'
>>
>> The first one remains good, but the second one is still broken. I
>> haven't dug further yet, but I thought it might be a bit more obvious to
>> you.
>
> Hmm. Is the fix as simple as:
>
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 56e3d6b..1c2b303 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -408,7 +408,7 @@ static struct origin *find_origin(struct scoreboard *sb,
>         paths[0] = origin->path;
>         paths[1] = NULL;
>
> -       parse_pathspec(&diff_opts.pathspec, PATHSPEC_ALL_MAGIC, 0, "", paths);
> +       parse_pathspec(&diff_opts.pathspec, PATHSPEC_ALL_MAGIC & ~PATHSPEC_LITERAL, 0, "", paths);
>         diff_setup_done(&diff_opts);
>
>         if (is_null_sha1(origin->commit->object.sha1))
>
> All of the GUARD_PATHSPEC calls indicate that everybody understands
> PATHSPEC_LITERAL. It is not technically true that git-blame understands
> the literal pathspec magic:
>
>   $ git blame -- ':(literal)revision.c'
>   fatal: no such path ':(literal)revision.c' in HEAD
>
> but that is a separate bug (that blame considers the argument as a path
> first before feeding it to the pathspec machinery). The patch above does
> not fix that, but AFAICT it does not make anything worse.

I did consider this change but dropped it because there are more
parse_pathspec() calls with PATHSPEC_ALL_MAGIC as mask. Thanks for
bringing up ":(literal)".  I guess we need to change prefix_pathspec()
to set PATHSPEC_LITERAL only when :(literal) is present, not when
--literal-pathspecs is used.
-- 
Duy

Re: [BUG] "git --literal-pathspecs blame" broken in master

[Jeff King]

On Fri, Oct 25, 2013 at 11:16:08AM +0700, Nguyen Thai Ngoc Duy wrote:

> > All of the GUARD_PATHSPEC calls indicate that everybody understands
> > PATHSPEC_LITERAL. It is not technically true that git-blame understands
> > the literal pathspec magic:
> >
> >   $ git blame -- ':(literal)revision.c'
> >   fatal: no such path ':(literal)revision.c' in HEAD
> >
> > but that is a separate bug (that blame considers the argument as a path
> > first before feeding it to the pathspec machinery). The patch above does
> > not fix that, but AFAICT it does not make anything worse.
> 
> I did consider this change but dropped it because there are more
> parse_pathspec() calls with PATHSPEC_ALL_MAGIC as mask. Thanks for
> bringing up ":(literal)".  I guess we need to change prefix_pathspec()
> to set PATHSPEC_LITERAL only when :(literal) is present, not when
> --literal-pathspecs is used.

I considered suggesting that, too, but it means that everywhere that
checks for PATHSPEC_LITERAL must _also_ check for literal_global (e.g.,
if they were deciding to feed the result to fnmatch). Whereas if we
catch it at the parse_pathspec layer, then the consumers of the pathspec
just need to check the one flag.

I dunno. I haven't kept up very well with your work in this area, so you
probably have a better sense than I do of what would be the most
elegant.

-Peff

Re: [BUG] "git --literal-pathspecs blame" broken in master

[Duy Nguyen]

On Fri, Oct 25, 2013 at 10:49 AM, Jeff King <peff@peff.net> wrote:
>   $ git --literal-pathspecs blame ':(foo)bar'
>   fatal: :(foo)bar: pathspec magic not supported by this command: 'literal'
>
> The first one remains good, but the second one is still broken. I
> haven't dug further yet, but I thought it might be a bit more obvious to
> you.

I checked it too strict. Something like this should fix it. I'll post
a patch with tests later

diff --git a/pathspec.c b/pathspec.c
index ad1a9f5..69e4fdb 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -405,7 +405,7 @@ void parse_pathspec(struct pathspec *pathspec,
                item[i].magic = prefix_pathspec(item + i, &short_magic,
                                                argv + i, flags,
                                                prefix, prefixlen, entry);
-               if (item[i].magic & magic_mask)
+               if (item[i].magic & (magic_mask & ~PATHSPEC_LITERAL))
                        unsupported_magic(entry,
                                          item[i].magic & magic_mask,
                                          short_magic);

-- 
Duy

[PATCH] pathspec: stop --*-pathspecs impact on internal parse_pathspec() uses

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

Normally parse_pathspec() is used on command line arguments where it
can do fancy thing like parsing magic on each argument or adding magic
for all pathspecs based on --*-pathspecs options.

There's another use of parse_pathspec(), where pathspec is needed, but
the input is known to be pure paths. In this case we usually don't
want --*-pathspecs to interfere. And we definitely do not want to
parse magic in these paths, regardless of --literal-pathspecs.

Add new flag PATHSPEC_LITERAL_PATH for this purpose. When it's set,
--*-pathspecs are ignored, no magic is parsed. And if the caller
allows PATHSPEC_LITERAL (i.e. the next calls can take literal magic),
then PATHSPEC_LITERAL will be set.

This fixes cases where git chokes when GIT_*_PATHSPECS are set because
parse_pathspec() indicates it won't take any magic. But
GIT_*_PATHSPECS add them anyway. These are

   export GIT_LITERAL_PATHSPECS=1
   git blame -- something
   git log --follow something
   git log --merge

"git ls-files --with-tree=path" (aka parse_pathspec() in
overlay_tree_on_cache()) is safe because the input is empty, and
producing one pathspec due to PATHSPEC_PREFER_CWD does not take any
magic into account.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Jeff, how about this?
 
 It's similar to your last suggestion (i.e.  relaxing the magic mask
 about literal magic). In addition, it forces literal magic
 unconditionally in this case, which I think is the right thing to do.
 And it will fix other --*-pathspecs as well.

 builtin/blame.c            | 4 +++-
 pathspec.c                 | 9 ++++++++-
 pathspec.h                 | 7 +++++++
 revision.c                 | 3 ++-
 t/t6130-pathspec-noglob.sh | 7 +++++++
 tree-diff.c                | 4 +++-
 6 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 6da7233..1407ae7 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -409,7 +409,9 @@ static struct origin *find_origin(struct scoreboard *sb,
 	paths[0] = origin->path;
 	paths[1] = NULL;
 
-	parse_pathspec(&diff_opts.pathspec, PATHSPEC_ALL_MAGIC, 0, "", paths);
+	parse_pathspec(&diff_opts.pathspec,
+		       PATHSPEC_ALL_MAGIC & ~PATHSPEC_LITERAL,
+		       PATHSPEC_LITERAL_PATH, "", paths);
 	diff_setup_done(&diff_opts);
 
 	if (is_null_sha1(origin->commit->object.sha1))
diff --git a/pathspec.c b/pathspec.c
index ad1a9f5..4cf2bd3 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -128,7 +128,11 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
 		die(_("global 'literal' pathspec setting is incompatible "
 		      "with all other global pathspec settings"));
 
-	if (elt[0] != ':' || literal_global) {
+	if (flags & PATHSPEC_LITERAL_PATH)
+		global_magic = 0;
+
+	if (elt[0] != ':' || literal_global ||
+	    (flags & PATHSPEC_LITERAL_PATH)) {
 		; /* nothing to do */
 	} else if (elt[1] == '(') {
 		/* longhand */
@@ -405,6 +409,9 @@ void parse_pathspec(struct pathspec *pathspec,
 		item[i].magic = prefix_pathspec(item + i, &short_magic,
 						argv + i, flags,
 						prefix, prefixlen, entry);
+		if ((flags & PATHSPEC_LITERAL_PATH) &&
+		    !(magic_mask & PATHSPEC_LITERAL))
+			item[i].magic |= PATHSPEC_LITERAL;
 		if (item[i].magic & magic_mask)
 			unsupported_magic(entry,
 					  item[i].magic & magic_mask,
diff --git a/pathspec.h b/pathspec.h
index 944baeb..a75e924 100644
--- a/pathspec.h
+++ b/pathspec.h
@@ -58,6 +58,13 @@ struct pathspec {
 #define PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE (1<<5)
 #define PATHSPEC_PREFIX_ORIGIN (1<<6)
 #define PATHSPEC_KEEP_ORDER (1<<7)
+/*
+ * For the callers that just need pure paths from somewhere else, not
+ * from command line. Global --*-pathspecs options are ignored. No
+ * magic is parsed in each pathspec either. If PATHSPEC_LITERAL is
+ * allowed, then it will automatically set for every pathspec.
+ */
+#define PATHSPEC_LITERAL_PATH (1<<8)
 
 extern void parse_pathspec(struct pathspec *pathspec,
 			   unsigned magic_mask,
diff --git a/revision.c b/revision.c
index 0173e01..9b9e22e 100644
--- a/revision.c
+++ b/revision.c
@@ -1372,7 +1372,8 @@ static void prepare_show_merge(struct rev_info *revs)
 			i++;
 	}
 	free_pathspec(&revs->prune_data);
-	parse_pathspec(&revs->prune_data, PATHSPEC_ALL_MAGIC, 0, "", prune);
+	parse_pathspec(&revs->prune_data, PATHSPEC_ALL_MAGIC & ~PATHSPEC_LITERAL,
+		       PATHSPEC_LITERAL_PATH, "", prune);
 	revs->limited = 1;
 }
 
diff --git a/t/t6130-pathspec-noglob.sh b/t/t6130-pathspec-noglob.sh
index ea00d71..6583532 100755
--- a/t/t6130-pathspec-noglob.sh
+++ b/t/t6130-pathspec-noglob.sh
@@ -108,6 +108,13 @@ test_expect_success 'no-glob environment variable works' '
 	test_cmp expect actual
 '
 
+test_expect_success 'blame takes global pathspec flags' '
+	git --literal-pathspecs blame -- foo &&
+	git --icase-pathspecs   blame -- foo &&
+	git --glob-pathspecs    blame -- foo &&
+	git --noglob-pathspecs  blame -- foo
+'
+
 test_expect_success 'setup xxx/bar' '
 	mkdir xxx &&
 	test_commit xxx xxx/bar
diff --git a/tree-diff.c b/tree-diff.c
index ccf9d7c..456660c 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -254,7 +254,9 @@ static void try_to_follow_renames(struct tree_desc *t1, struct tree_desc *t2, co
 			path[0] = p->one->path;
 			path[1] = NULL;
 			free_pathspec(&opt->pathspec);
-			parse_pathspec(&opt->pathspec, PATHSPEC_ALL_MAGIC, 0, "", path);
+			parse_pathspec(&opt->pathspec,
+				       PATHSPEC_ALL_MAGIC & ~PATHSPEC_LITERAL,
+				       PATHSPEC_LITERAL_PATH, "", path);
 
 			/*
 			 * The caller expects us to return a set of vanilla
-- 
1.8.2.83.gc99314b

Re: [PATCH] pathspec: stop --*-pathspecs impact on internal parse_pathspec() uses

[Jeff King]

On Sat, Oct 26, 2013 at 09:09:20AM +0700, Nguyen Thai Ngoc Duy wrote:

>  Jeff, how about this?
>  
>  It's similar to your last suggestion (i.e.  relaxing the magic mask
>  about literal magic). In addition, it forces literal magic
>  unconditionally in this case, which I think is the right thing to do.
>  And it will fix other --*-pathspecs as well.

Yeah, I think I follow your reasoning. The problem I saw was just about
--literal-pathspec, but the real issue is that "blame" does not want
magic pathspecs at all, and your new flag turns that off.

So it fixes both my problem, as well as "git blame -- :(foo)bar".

-Peff

[PATCH] pathspec: stop --*-pathspecs impact on internal parse_pathspec() uses

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

Normally parse_pathspec() is used on command line arguments where it
can do fancy thing like parsing magic on each argument or adding magic
for all pathspecs based on --*-pathspecs options.

There's another use of parse_pathspec(), where pathspec is needed, but
the input is known to be pure paths. In this case we usually don't
want --*-pathspecs to interfere. And we definitely do not want to
parse magic in these paths, regardless of --literal-pathspecs.

Add new flag PATHSPEC_LITERAL_PATH for this purpose. When it's set,
--*-pathspecs are ignored, no magic is parsed. And if the caller
allows PATHSPEC_LITERAL (i.e. the next calls can take literal magic),
then PATHSPEC_LITERAL will be set.

This fixes cases where git chokes when GIT_*_PATHSPECS are set because
parse_pathspec() indicates it won't take any magic. But
GIT_*_PATHSPECS add them anyway. These are

   export GIT_LITERAL_PATHSPECS=1
   git blame -- something
   git log --follow something
   git log --merge

"git ls-files --with-tree=path" (aka parse_pathspec() in
overlay_tree_on_cache()) is safe because the input is empty, and
producing one pathspec due to PATHSPEC_PREFER_CWD does not take any
magic into account.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Jeff, how about this?
 
 It's similar to your last suggestion (i.e.  relaxing the magic mask
 about literal magic). In addition, it forces literal magic
 unconditionally in this case, which I think is the right thing to do.
 And it will fix other --*-pathspecs as well.

 builtin/blame.c            | 4 +++-
 pathspec.c                 | 9 ++++++++-
 pathspec.h                 | 7 +++++++
 revision.c                 | 3 ++-
 t/t6130-pathspec-noglob.sh | 7 +++++++
 tree-diff.c                | 4 +++-
 6 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 6da7233..1407ae7 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -409,7 +409,9 @@ static struct origin *find_origin(struct scoreboard *sb,
 	paths[0] = origin->path;
 	paths[1] = NULL;
 
-	parse_pathspec(&diff_opts.pathspec, PATHSPEC_ALL_MAGIC, 0, "", paths);
+	parse_pathspec(&diff_opts.pathspec,
+		       PATHSPEC_ALL_MAGIC & ~PATHSPEC_LITERAL,
+		       PATHSPEC_LITERAL_PATH, "", paths);
 	diff_setup_done(&diff_opts);
 
 	if (is_null_sha1(origin->commit->object.sha1))
diff --git a/pathspec.c b/pathspec.c
index ad1a9f5..4cf2bd3 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -128,7 +128,11 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
 		die(_("global 'literal' pathspec setting is incompatible "
 		      "with all other global pathspec settings"));
 
-	if (elt[0] != ':' || literal_global) {
+	if (flags & PATHSPEC_LITERAL_PATH)
+		global_magic = 0;
+
+	if (elt[0] != ':' || literal_global ||
+	    (flags & PATHSPEC_LITERAL_PATH)) {
 		; /* nothing to do */
 	} else if (elt[1] == '(') {
 		/* longhand */
@@ -405,6 +409,9 @@ void parse_pathspec(struct pathspec *pathspec,
 		item[i].magic = prefix_pathspec(item + i, &short_magic,
 						argv + i, flags,
 						prefix, prefixlen, entry);
+		if ((flags & PATHSPEC_LITERAL_PATH) &&
+		    !(magic_mask & PATHSPEC_LITERAL))
+			item[i].magic |= PATHSPEC_LITERAL;
 		if (item[i].magic & magic_mask)
 			unsupported_magic(entry,
 					  item[i].magic & magic_mask,
diff --git a/pathspec.h b/pathspec.h
index 944baeb..a75e924 100644
--- a/pathspec.h
+++ b/pathspec.h
@@ -58,6 +58,13 @@ struct pathspec {
 #define PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE (1<<5)
 #define PATHSPEC_PREFIX_ORIGIN (1<<6)
 #define PATHSPEC_KEEP_ORDER (1<<7)
+/*
+ * For the callers that just need pure paths from somewhere else, not
+ * from command line. Global --*-pathspecs options are ignored. No
+ * magic is parsed in each pathspec either. If PATHSPEC_LITERAL is
+ * allowed, then it will automatically set for every pathspec.
+ */
+#define PATHSPEC_LITERAL_PATH (1<<8)
 
 extern void parse_pathspec(struct pathspec *pathspec,
 			   unsigned magic_mask,
diff --git a/revision.c b/revision.c
index 0173e01..9b9e22e 100644
--- a/revision.c
+++ b/revision.c
@@ -1372,7 +1372,8 @@ static void prepare_show_merge(struct rev_info *revs)
 			i++;
 	}
 	free_pathspec(&revs->prune_data);
-	parse_pathspec(&revs->prune_data, PATHSPEC_ALL_MAGIC, 0, "", prune);
+	parse_pathspec(&revs->prune_data, PATHSPEC_ALL_MAGIC & ~PATHSPEC_LITERAL,
+		       PATHSPEC_LITERAL_PATH, "", prune);
 	revs->limited = 1;
 }
 
diff --git a/t/t6130-pathspec-noglob.sh b/t/t6130-pathspec-noglob.sh
index ea00d71..6583532 100755
--- a/t/t6130-pathspec-noglob.sh
+++ b/t/t6130-pathspec-noglob.sh
@@ -108,6 +108,13 @@ test_expect_success 'no-glob environment variable works' '
 	test_cmp expect actual
 '
 
+test_expect_success 'blame takes global pathspec flags' '
+	git --literal-pathspecs blame -- foo &&
+	git --icase-pathspecs   blame -- foo &&
+	git --glob-pathspecs    blame -- foo &&
+	git --noglob-pathspecs  blame -- foo
+'
+
 test_expect_success 'setup xxx/bar' '
 	mkdir xxx &&
 	test_commit xxx xxx/bar
diff --git a/tree-diff.c b/tree-diff.c
index ccf9d7c..456660c 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -254,7 +254,9 @@ static void try_to_follow_renames(struct tree_desc *t1, struct tree_desc *t2, co
 			path[0] = p->one->path;
 			path[1] = NULL;
 			free_pathspec(&opt->pathspec);
-			parse_pathspec(&opt->pathspec, PATHSPEC_ALL_MAGIC, 0, "", path);
+			parse_pathspec(&opt->pathspec,
+				       PATHSPEC_ALL_MAGIC & ~PATHSPEC_LITERAL,
+				       PATHSPEC_LITERAL_PATH, "", path);
 
 			/*
 			 * The caller expects us to return a set of vanilla
-- 
1.8.2.83.gc99314b