threads / patch / 35362

patchdiff: restrict pathspec limitations to diff b/f case only

Subject: [PATCH] diff: restrict pathspec limitations to diff b/f case only

## tl;dr

8 messages between Nov 20, 2013 and Nov 22, 2013. Diffs are folded; open one to read it.

replies: 7people: 2as markdown or json

Nguyễn Thái Ngọc Duy· Nov 20, 2013, 01:26 UTC · lore

builtin_diff_b_f() needs a path, not pathspec. Other modes in diff can deal with pathspec just fine. But because of the current GUARD_PATHSPEC() location, other modes also reject :(glob) and :(icase).

Move GUARD_PATHSPEC(), and the "path" assignment statement, which is the reason of this GUARD_PATHSPEC(), inside builtin_diff_b_f().

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/diff.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)
Show changes to builtin/diff.c +7 −10
diff --git a/builtin/diff.c b/builtin/diff.c
index adb93a9..fe0cc7f 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -64,15 +64,18 @@ static void stuff_change(struct diff_options *opt,
 
 static int builtin_diff_b_f(struct rev_info *revs,
 			    int argc, const char **argv,
-			    struct blobinfo *blob,
-			    const char *path)
+			    struct blobinfo *blob)
 {
 	/* Blob vs file in the working tree*/
 	struct stat st;
+	const char *path;
 
 	if (argc > 1)
 		usage(builtin_diff_usage);
 
+	GUARD_PATHSPEC(&revs->prune_data, PATHSPEC_FROMTOP | PATHSPEC_LITERAL);
+	path = revs->prune_data.items[0].match;
+
 	if (lstat(path, &st))
 		die_errno(_("failed to stat '%s'"), path);
 	if (!(S_ISREG(st.st_mode) || S_ISLNK(st.st_mode)))
@@ -255,7 +258,6 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 	struct rev_info rev;
 	struct object_array ent = OBJECT_ARRAY_INIT;
 	int blobs = 0, paths = 0;
-	const char *path = NULL;
 	struct blobinfo blob[2];
 	int nongit;
 	int result = 0;
@@ -366,13 +368,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 			die(_("unhandled object '%s' given."), name);
 		}
 	}
-	if (rev.prune_data.nr) {
-		/* builtin_diff_b_f() */
-		GUARD_PATHSPEC(&rev.prune_data, PATHSPEC_FROMTOP | PATHSPEC_LITERAL);
-		if (!path)
-			path = rev.prune_data.items[0].match;
+	if (rev.prune_data.nr)
 		paths += rev.prune_data.nr;
-	}
 
 	/*
 	 * Now, do the arguments look reasonable?
@@ -385,7 +382,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 		case 1:
 			if (paths != 1)
 				usage(builtin_diff_usage);
-			result = builtin_diff_b_f(&rev, argc, argv, blob, path);
+			result = builtin_diff_b_f(&rev, argc, argv, blob);
 			break;
 		case 2:
 			if (paths)

-- 
1.8.2.82.gc24b958
Junio C Hamano· Nov 20, 2013, 22:43 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH] diff: restrict pathspec limitations to diff b/f case only

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
Show 10 quoted lines
> builtin_diff_b_f() needs a path, not pathspec. Other modes in diff
> can deal with pathspec just fine. But because of the current
> GUARD_PATHSPEC() location, other modes also reject :(glob) and
> :(icase).
>
> Move GUARD_PATHSPEC(), and the "path" assignment statement, which is
> the reason of this GUARD_PATHSPEC(), inside builtin_diff_b_f().
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
Makes sense.
Let me squash this in.
Show changes to t/t6131-pathspec-icase.sh +6 −0
diff --git a/t/t6131-pathspec-icase.sh b/t/t6131-pathspec-icase.sh
index 8d4a7fc..a7c7ff5 100755
--- a/t/t6131-pathspec-icase.sh
+++ b/t/t6131-pathspec-icase.sh
@@ -100,4 +100,10 @@ test_expect_success 'match_pathspec_depth matches :(icase)bar with empty prefix'
 	test_cmp expect actual
 '
 
+test_expect_success '"git diff" can take magic :(icase) pathspec' '
+	echo FOO/BAR >expect &&
+	git diff --name-only HEAD^ HEAD -- ":(icase)foo/bar" >actual &&
+	test_cmp expect actual
+'
+
 test_done

Thanks.
Nguyễn Thái Ngọc Duy· Nov 21, 2013, 06:44 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 1/2] glossary-content.txt: remove an obsolete paragrah

With the introduction of :(literal), :(glob) and :(icase), :(top) is no longer the only recognized magic signature.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 .. on top of nd/magic-pathspec..
 Documentation/glossary-content.txt | 4 ----
 1 file changed, 4 deletions(-)
Show changes to Documentation/glossary-content.txt +0 −4
diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt
index e470661..e22b524 100644
--- a/Documentation/glossary-content.txt
+++ b/Documentation/glossary-content.txt
@@ -379,10 +379,6 @@ full pathname may have special meaning:
 Glob magic is incompatible with literal magic.
 --
 +
-Currently only the slash `/` is recognized as the "magic signature",
-but it is envisioned that we will support more types of magic in later
-versions of Git.
-+
 A pathspec with only a colon means "there is no pathspec". This form
 should not be combined with other pathspec.
 

-- 
1.8.2.82.gc24b958
Nguyễn Thái Ngọc Duy· Nov 21, 2013, 06:44 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 2/2] glossary-content.txt: fix documentation of "**" patterns

"**" means bold in ASCIIDOC, so we need to escape it. This is similar to 8447dc8 (gitignore.txt: fix documentation of "**" patterns - 2013-11-07)

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 There is another instance of '**' in RelNotes/1.8.5.txt. Junio you
 may want to fix it too.
 Documentation/glossary-content.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
Show changes to Documentation/glossary-content.txt +3 −3
diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt
index e22b524..1b56ba4 100644
--- a/Documentation/glossary-content.txt
+++ b/Documentation/glossary-content.txt
@@ -362,12 +362,12 @@ full pathname may have special meaning:
 
  - A leading "`**`" followed by a slash means match in all
    directories. For example, "`**/foo`" matches file or directory
-   "`foo`" anywhere, the same as pattern "`foo`". "**/foo/bar"
+   "`foo`" anywhere, the same as pattern "`foo`". "`**/foo/bar`"
    matches file or directory "`bar`" anywhere that is directly
    under directory "`foo`".
 
- - A trailing "/**" matches everything inside. For example,
-   "abc/**" matches all files inside directory "abc", relative
+ - A trailing "`/**`" matches everything inside. For example,
+   "`abc/**`" matches all files inside directory "abc", relative
    to the location of the `.gitignore` file, with infinite depth.
 
  - A slash followed by two consecutive asterisks then a slash

-- 
1.8.2.82.gc24b958
Junio C Hamano· Nov 21, 2013, 18:13 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 1/2] glossary-content.txt: remove an obsolete paragrah

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
> With the introduction of :(literal), :(glob) and :(icase), :(top) is
> no longer the only recognized magic signature.

You need to re-read the message you took hints that led to this patch from (or, rather, the existing document around the area you are patching, which led to the wording in that message from me).

The above is a list of magic words, but until you add the magic signature ":!not_in_this_directory" for the ":(exclude)" magic word, ":/" still is the only magic signature.

Another moderately related tangent is this phrase in the existing document:

    In the short form, the leading colon `:` is followed by zero or
    more "magic signature" letters (which optionally is terminated
    by another colon `:`), and the remainder is the pattern to match
    against the path. The optional colon that terminates the "magic
    signature" can be omitted if the pattern begins with a character
    that cannot be a "magic signature" and is not a colon.

While it is technically correct, the phrase "a character that cannot be" is somewhat misleading, and I think it needs to be clarified by rephrasing.

As we can see in a later paragaph:
    The "magic signature" consists of an ASCII symbol that is not
    alphanumeric. Currently only the slash `/` is recognized as a
    "magic signature"...

the correct way to read that "a character that cannot be a magic signature" is "a character that is not 'an ASCII symbol that is not alphanumeric'", which means you can do:

	:!/foo	- with magic signatures ! and /, pattern begins at 'f'
	:/#abc  - with magic signatures # and /, pattern begins at 'a',
	          even if in a particular version of Git, '#' magic
		  signature may be invalid and produce an error
	:/:#abc - with magic signature /, pattern is "#abc".

but because the definition of "magic signature" syntax comes later than "cannot be", it is prone to be misinterpreted as "anything that is not currently defined as a magic signature (namely, '/')".

Show 21 quoted lines
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  .. on top of nd/magic-pathspec..
>
>  Documentation/glossary-content.txt | 4 ----
>  1 file changed, 4 deletions(-)
>
> diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt
> index e470661..e22b524 100644
> --- a/Documentation/glossary-content.txt
> +++ b/Documentation/glossary-content.txt
> @@ -379,10 +379,6 @@ full pathname may have special meaning:
>  Glob magic is incompatible with literal magic.
>  --
>  +
> -Currently only the slash `/` is recognized as the "magic signature",
> -but it is envisioned that we will support more types of magic in later
> -versions of Git.
> -+
>  A pathspec with only a colon means "there is no pathspec". This form
>  should not be combined with other pathspec.
Junio C Hamano· Nov 21, 2013, 23:55 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] glossary-content.txt: remove an obsolete paragrah

Junio C Hamano <gitster@pobox.com> writes:
An addendum.
Show 21 quoted lines
> As we can see in a later paragaph:
>
>     The "magic signature" consists of an ASCII symbol that is not
>     alphanumeric. Currently only the slash `/` is recognized as a
>     "magic signature"...
>
> the correct way to read that "a character that cannot be a magic
> signature" is "a character that is not 'an ASCII symbol that is not
> alphanumeric'", which means you can do:
>
> 	:!/foo	- with magic signatures ! and /, pattern begins at 'f'
>
> 	:/#abc  - with magic signatures # and /, pattern begins at 'a',
> 	          even if in a particular version of Git, '#' magic
> 		  signature may be invalid and produce an error
>
> 	:/:#abc - with magic signature /, pattern is "#abc".
>
> but because the definition of "magic signature" syntax comes later
> than "cannot be", it is prone to be misinterpreted as "anything that
> is not currently defined as a magic signature (namely, '/')".
... and that misinterpretation may give a false impression that
	":/#abc"

is with the magic signature '/' (i.e. match from top) for a pattern "#abc".

Doing so would mean that a version of Git that does not support a magic signature '#' will allow people and scripts to use ":/#abc" with such a meaning, and will prevent us from using '#' as a new magic signature with some useful meaning in the future. Rather, we need to force them to always spell it as ":/:#abc" even in the version of Git before the magic signature '#'.

And the phrasing 'if the pattern begins with a character that cannot be a "magic signature" and is not a colon' needs to be clarified to avoid that misinterpretation for that reason.

Duy Nguyen· Nov 22, 2013, 01:36 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] glossary-content.txt: remove an obsolete paragrah

On Fri, Nov 22, 2013 at 6:55 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 39 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
> An addendum.
>
>> As we can see in a later paragaph:
>>
>>     The "magic signature" consists of an ASCII symbol that is not
>>     alphanumeric. Currently only the slash `/` is recognized as a
>>     "magic signature"...
>>
>> the correct way to read that "a character that cannot be a magic
>> signature" is "a character that is not 'an ASCII symbol that is not
>> alphanumeric'", which means you can do:
>>
>>       :!/foo  - with magic signatures ! and /, pattern begins at 'f'
>>
>>       :/#abc  - with magic signatures # and /, pattern begins at 'a',
>>                 even if in a particular version of Git, '#' magic
>>                 signature may be invalid and produce an error
>>
>>       :/:#abc - with magic signature /, pattern is "#abc".
>>
>> but because the definition of "magic signature" syntax comes later
>> than "cannot be", it is prone to be misinterpreted as "anything that
>> is not currently defined as a magic signature (namely, '/')".
>
> ... and that misinterpretation may give a false impression that
>
>         ":/#abc"
>
> is with the magic signature '/' (i.e. match from top) for a pattern
> "#abc".
>
> Doing so would mean that a version of Git that does not support a
> magic signature '#' will allow people and scripts to use ":/#abc"
> with such a meaning, and will prevent us from using '#' as a new
> magic signature with some useful meaning in the future.  Rather, we
> need to force them to always spell it as ":/:#abc" even in the
> version of Git before the magic signature '#'.
Current version does force that.

$ git log -- ':/#aa' fatal: Unimplemented pathspec magic '#' in ':/#aa'

Not sure if there are tests for it though. I'll add an advice to do ":/:#aa".
> And the phrasing 'if the pattern begins with a character that cannot
> be a "magic signature" and is not a colon' needs to be clarified to
> avoid that misinterpretation for that reason.
-- 
Duy
Junio C Hamano· Nov 22, 2013, 05:40 UTC · re: Duy Nguyen · lore

Re: [PATCH 1/2] glossary-content.txt: remove an obsolete paragrah

Duy Nguyen <pclouds@gmail.com> writes:
Show 6 quoted lines
> Current version does force that.
>
> $ git log -- ':/#aa'
> fatal: Unimplemented pathspec magic '#' in ':/#aa'
>
> Not sure if there are tests for it though. I'll add an advice to do ":/:#aa".

It is good that we already do the right thing, but actually I was not questioning the implementation. I was talking about the fact that the documentation is open to misinterpretation that contradicts the correct behaviour, as if it were buggy.

← back to recent threads