Re: Fwd: Possibly nicer pathspec syntax?

[Linus Torvalds <torvalds@linux-foundation.org>]

[-- Attachment #1: Type: text/plain, Size: 2644 bytes --]

On Tue, Feb 7, 2017 at 7:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> But that is not what I was talking about.  Let's simplify.  I'd say
> for any command that acts on "everything" when pathspec is not
> given, the two sets of actual paths affected by these two:
>
>         git cmd -- "net/"
>         git cmd -- ":!net/"
>
> should have no overlap (obviously) and when you take union of the
> two sets, that should equal to
>
>         git cmd --
>
> i.e. no pathspecs.

Well, as mentioned, I won't ever care. I'm certainly ok with the "make
the default positive entry be everything".

I just suspect that from a user perspective that actually delves into
the subdirectories, the much bigger question will be: "I gave you a
pathspec, and suddenly you start giving me stuff from outside the area
entirely".

And then you can say "well, just add '.' to the pathspec", and you'd
be right, but I still think it's not what a naive user would expect.

People don't expect set theory from their pathspecs. They expect their
pathspecs to limit the output. They've learnt that within a
subdirectory, the pathspec limits to that subdirectory. And now it
suddenly starts showing things outside the subdirectory?

At that point no amount of "but but think about set theory" will
matter, methinks.

But I really don't feel strongly about it. The path I sent out (and
the slightly modified version attached in this email) actually acts
the way you suggest. It's certainly the simplest implementation. I
just suspect it's not the implementation people who go down into
subdirectories would want/expect.

>>>  2. I am not sure what ctype.c change is about.  Care to elaborate?
>>
>> I didn't see the need for it either until I made the rest of the
>> patch, and it didn't work at all.
>>
>> The pathspec.c code uses "if (is_pathspec_magic(..))" to test whether
>> a character is a short magiic pathspec character.  But '^' wasn't in
>> that set, because it was already marked as being (only) in the regex
>> set.
>>
>> Does that whole is_pathspec_magic() thing make any sense when we have
>> an array that specifies the special characters we react to? No it does
>> not.
>>
>> But it is what the code does, and I just made that code work.
>
> Ah, OK.

Side note: here's an alternative patch that avoids that issue
entirely, and also avoids a problem with prefix_magic() being uphappy
when one bit shows up multiple times in the array.

It's slightly hacky in parse_short_magic(), but it's smaller and
simpler, and avoids the two subtle cases. No need for ctype changes,
and no issues with prefix_magic() being somewhat stupid.

               Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/plain, Size: 1293 bytes --]

 pathspec.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/pathspec.c b/pathspec.c
index 7ababb315..2a91247bc 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -224,6 +224,12 @@ static const char *parse_short_magic(unsigned *magic, const char *elem)
 		char ch = *pos;
 		int i;
 
+		// Special case alias for '!'
+		if (ch == '^') {
+			*magic |= PATHSPEC_EXCLUDE;
+			continue;
+		}
+
 		if (!is_pathspec_magic(ch))
 			break;
 
@@ -516,7 +522,7 @@ void parse_pathspec(struct pathspec *pathspec,
 	}
 
 	pathspec->nr = n;
-	ALLOC_ARRAY(pathspec->items, n);
+	ALLOC_ARRAY(pathspec->items, n+1);
 	item = pathspec->items;
 	prefixlen = prefix ? strlen(prefix) : 0;
 
@@ -540,10 +546,14 @@ void parse_pathspec(struct pathspec *pathspec,
 		pathspec->magic |= item[i].magic;
 	}
 
-	if (nr_exclude == n)
-		die(_("There is nothing to exclude from by :(exclude) patterns.\n"
-		      "Perhaps you forgot to add either ':/' or '.' ?"));
-
+	/*
+	 * If everything is an exclude pattern, add one positive pattern
+	 * that matches everyting. We allocated an extra one for this.
+	 */
+	if (nr_exclude == n) {
+		init_pathspec_item(item + n, 0, "", 0, "");
+		pathspec->nr++;
+	}
 
 	if (pathspec->magic & PATHSPEC_MAXDEPTH) {
 		if (flags & PATHSPEC_KEEP_ORDER)