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

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

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

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

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

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

[Junio C Hamano]

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

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


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.

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

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

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

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

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

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

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

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

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

[Junio C Hamano]

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, '/')".



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

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

[Junio C Hamano]

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 '#'.

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.

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

[Duy Nguyen]

On Fri, Nov 22, 2013 at 6:55 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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

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

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

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