Fwd: Possibly nicer pathspec syntax?

[ Duh, I sent this just to Junio initially due to a brainfart. Here
goes the list also ]

Most of the time when I use pathspecs, I just use the bog-standard
normal ones, and everything works wonderfully.

But then occasionally I want to exclude a directory (usually
"drivers/"), and it all works fine, but the syntax for that is just
really cumbersome.

That's due to two issues:

 - the magic characters seem to have been chosen to be annoying on purpose

 - there's an extra "you can't exclude things without having positive
patterns" check

and both of those are rather nasty.

So to explain where I come from, during releases I do things like this:

    git diff -M --dirstat=2,cumulative v4.10-rc6..v4.10-rc7

and this is literaly why that "dirstat" diff exists - I find this very
useful for a project like the kernel that has a good hierarchical
directory structure. So the whole dirstat option came about from my
statistics gathering (see more explanations in my original commit
7df7c019c ("Add "--dirstat" for some directory statistics").

However, what often happens for the kernel is that a few big
subsystems end up dominating the discussion (usually drivers and
architecture), and then you want to drill down into everything else to
get that part. Long ago, that used to be painful, and I did things
like

    git diff -M --dirstat ... -- $(ls | egrep -v '(drivers)|(arch)')

which works, and gives me the dirstat for stuff that isn't arch or
driver updates.

However, git actually added exclude patterns, and I don't need to do
that crazy thing with shell expansion any more. Now I can do this
crazy thing with git natively instead:

    git diff -M --dirstat .. -- ':!drivers' ':!arch' .

but honestly, the git native interface really isn't much simpler than
what I used to do.

Is there really any reason for requiring the '.'?

[ Clarification from original message, since Junio asked: I didn't
actually want the semantics of '.' at all, since in a subdirectory it
limits to the current subdirectory. So I'd suggest that in the absense
of any positive pattern, there is simply no filtering at all, so
whenever I say '.' as a pattern, I really meant ":(top)." which is
even more of a cumbersom syntax that the current model really
encourages. Crazy. Since I tend to always work in the top directory,
the two are the same for me ]

And did we really have to pick such annoying characters that we need
the shell escaping?

(I never use the other ones with long forms, but they have the same
issue: parenthesis need escaping too, so you have to write them as

    ':(exclude,icase)drivers'

and you have to remember that a final colon is *not* allowed, and they
still need the escaping).

It really isn't all that wonderful to use from the command line.

In revisions, we use "^" for negation, partly exactly because '!' is
such a nasty character for shell users. With exclusion being the only
case I particularly use, I'd like that for pathspecs too, but maybe
others use icase etc?

IOW, what I'd like to do is just

    git diff -M --dirstat .. ^drivers ^arch

without needing the ugly quoting, and without needing the pointless
positive 'match everything else' marker.

Or even just allowing ^ in addition to ! for negation, and otherwise
keeping the current syntax.

[ Clarification from original message, since Junio asked: yes, this
suggestion still assumes the "don't need to specify the positive
pattern", so you could just do

    git diff :^drivers

  to avoid the drivers directory ]

Comments? Other ideas?

This is certainly not a high priority, but I hit it once again when
doing the 4.10-rc7 statistics, which is why I bring up the
discussion..

                 Linus

On Tue, 7 Feb 2017, Linus Torvalds wrote:

[ Clarification from original message, since Junio asked: I didn't
  actually want the semantics of '.' at all, since in a subdirectory it
  limits to the current subdirectory. So I'd suggest that in the absence
  of any positive pattern, there is simply no filtering at all, so
  whenever I say '.' as a pattern, I really meant ":(top)." which is
  even more of a cumbersom syntax that the current model really
  encourages. Crazy. Since I tend to always work in the top directory,
  the two are the same for me ]

So here's an RFC patch, and I'm quoting the above part of my thinking 
because it's what the patch does, but it turns out that it's probably not 
what we want, and I suspect the "." behavior (as opposed to "no filtering 
at all") is actually better.

Now _I_ don't much care, since I only work from the top level, but without 
the "." behavior, you get into an odd situation that the negative match 
will be relative to the current directory, but then the positive matches 
will be everywhere else. 

Obviously, a negative match that has "top" set would change that logic. So 
this patch is purely a request for further discussion.

When I wrote the patch, I actually also removed the now stale entries from 
the 'po' files, but I'm not including that part here because it just 
distracts from the meat of it all. So this diff was actually generated 
with the new syntax:

	git diff -p --stat -- :^po/

and the only thing even remotely subtle here is that it changes our ctype 
array to make '^' be both a regex and a pathspec magic character.

Everything else should be pretty darn obvious.

The code *could* just track all the 'relative to top or not' bits in the 
exclusion pattern, and then use whatever top-ness the exclusion patterns 
have (and maybe fall back to the old warning if it had a mixture of 
exclusionary patterns). I'll happily change it to act that way if people 
think that makes sense.

Comments?

                Linus

---
 ctype.c    |  3 ++-
 pathspec.c | 15 ++++++++++-----
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/ctype.c b/ctype.c
index fc0225ceb..250e2ce15 100644
--- a/ctype.c
+++ b/ctype.c

@@ -14,6 +14,7 @@ enum {
 	P = GIT_PATHSPEC_MAGIC, /* other non-alnum, except for ] and } */
 	X = GIT_CNTRL,
 	U = GIT_PUNCT,
+	Y = GIT_REGEX_SPECIAL | GIT_PATHSPEC_MAGIC,
 	Z = GIT_CNTRL | GIT_SPACE
 };
 

@@ -23,7 +24,7 @@ const unsigned char sane_ctype[256] = {
 	S, P, P, P, R, P, P, P, R, R, G, R, P, P, R, P,		/*  32.. 47 */
 	D, D, D, D, D, D, D, D, D, D, P, P, P, P, P, G,		/*  48.. 63 */
 	P, A, A, A, A, A, A, A, A, A, A, A, A, A, A, A,		/*  64.. 79 */
-	A, A, A, A, A, A, A, A, A, A, A, G, G, U, R, P,		/*  80.. 95 */
+	A, A, A, A, A, A, A, A, A, A, A, G, G, U, Y, P,		/*  80.. 95 */
 	P, A, A, A, A, A, A, A, A, A, A, A, A, A, A, A,		/*  96..111 */
 	A, A, A, A, A, A, A, A, A, A, A, R, R, U, P, X,		/* 112..127 */
 	/* Nothing in the 128.. range */

diff --git a/pathspec.c b/pathspec.c
index 7ababb315..ef59d080d 100644
--- a/pathspec.c
+++ b/pathspec.c

@@ -72,6 +72,7 @@ static struct pathspec_magic {
 	{ PATHSPEC_GLOB,    '\0', "glob" },
 	{ PATHSPEC_ICASE,   '\0', "icase" },
 	{ PATHSPEC_EXCLUDE,  '!', "exclude" },
+	{ PATHSPEC_EXCLUDE,  '^', "exclude" },
 };
 
 static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)

@@ -516,7 +517,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 +541,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)

On Tue, Feb 7, 2017 at 6:40 PM, Mike Hommey [off-list ref] wrote:

As such, the default positive match should be ':/' (which is shorter and
less cumbersome than ':(top)', btw)

So that's what my patch does.

However, it's actually very counter-intuitive in a subdirectory.

Git doesn't do much of that, but let me give you an example from the
kernel. Again, this is not an example of anything I would do (because
I'm always at the top), but:

  [torvalds@i7 linux]$ cd drivers/
  [torvalds@i7 drivers]$ ll

  .. whee, *lots* of diorectories ..
  .. lets see what happened in net/ ..

  [torvalds@i7 drivers]$ git diff -M --dirstat=1,cumulative
v4.10-rc6..v4.10-rc7 -- net/
     7.4% drivers/net/ethernet/adaptec/
    47.9% drivers/net/ethernet/cadence/
     7.1% drivers/net/ethernet/emulex/benet/
     1.1% drivers/net/ethernet/freescale/
     3.6% drivers/net/ethernet/mellanox/mlx4/
    23.5% drivers/net/ethernet/mellanox/mlx5/core/
    27.2% drivers/net/ethernet/mellanox/
    92.5% drivers/net/ethernet/
     5.3% drivers/net/wireless/intel/iwlwifi/mvm/
     5.9% drivers/net/wireless/intel/iwlwifi/
   100.0% drivers/net/

  .. let's see what happened *outside* of net/ ..

[torvalds@i7 drivers]$ git diff -M --dirstat=1,cumulative
v4.10-rc6..v4.10-rc7 -- :^net/
   2.4% arch/arm64/crypto/
   2.1% arch/powerpc/include/asm/
   1.5% arch/powerpc/kernel/
   3.9% arch/powerpc/
   3.5% arch/sparc/kernel/
   4.1% arch/sparc/
   8.3% arch/x86/events/intel/
   1.7% arch/x86/kernel/cpu/mcheck/
   1.6% arch/x86/kernel/cpu/microcode/
   3.3% arch/x86/kernel/cpu/
   3.8% arch/x86/kernel/
   1.0% arch/x86/platform/efi/
  13.3% arch/x86/
  24.0% arch/
   1.1% drivers/base/
   2.9% drivers/dma/
  12.3% drivers/gpu/drm/i915/
   1.0% drivers/gpu/drm/nouveau/
  16.2% drivers/gpu/drm/
   3.9% drivers/hid/
   1.6% drivers/iio/
   2.3% drivers/regulator/
   ...

Notice? When you say "show only the net subdirectory" it does the
obvious thing relative to the current working directory, but if you
say "show everything _but_ the net subdirectory" it suddenly starts
showing other things.

Now, it would be easy enough to say "if you don't give a positive
path, we'll just use the empty path that matches the negative paths".
So if you ask for a negative relative "net" directory, we'll use the
relative empty path. And if you ask for a negative absolute path,
we'll use the empty absolute path.

It's a couple of lines more, and I think it might avoid some confusion.

And I suspect almost nobody has ever done any of this before,. because
the syntax was/is so cumbersome.

                Linus

On Tue, Feb 07, 2017 at 06:49:24PM -0800, Linus Torvalds wrote:

On Tue, Feb 7, 2017 at 6:40 PM, Mike Hommey [off-list ref] wrote:

quoted

As such, the default positive match should be ':/' (which is shorter and
less cumbersome than ':(top)', btw)

So that's what my patch does.

However, it's actually very counter-intuitive in a subdirectory.

Git doesn't do much of that, but let me give you an example from the
kernel. Again, this is not an example of anything I would do (because
I'm always at the top), but:

  [torvalds@i7 linux]$ cd drivers/
  [torvalds@i7 drivers]$ ll

  .. whee, *lots* of diorectories ..
  .. lets see what happened in net/ ..

  [torvalds@i7 drivers]$ git diff -M --dirstat=1,cumulative
v4.10-rc6..v4.10-rc7 -- net/
     7.4% drivers/net/ethernet/adaptec/
    47.9% drivers/net/ethernet/cadence/
     7.1% drivers/net/ethernet/emulex/benet/
     1.1% drivers/net/ethernet/freescale/
     3.6% drivers/net/ethernet/mellanox/mlx4/
    23.5% drivers/net/ethernet/mellanox/mlx5/core/
    27.2% drivers/net/ethernet/mellanox/
    92.5% drivers/net/ethernet/
     5.3% drivers/net/wireless/intel/iwlwifi/mvm/
     5.9% drivers/net/wireless/intel/iwlwifi/
   100.0% drivers/net/

  .. let's see what happened *outside* of net/ ..

[torvalds@i7 drivers]$ git diff -M --dirstat=1,cumulative
v4.10-rc6..v4.10-rc7 -- :^net/
   2.4% arch/arm64/crypto/
   2.1% arch/powerpc/include/asm/
   1.5% arch/powerpc/kernel/
   3.9% arch/powerpc/
   3.5% arch/sparc/kernel/
   4.1% arch/sparc/
   8.3% arch/x86/events/intel/
   1.7% arch/x86/kernel/cpu/mcheck/
   1.6% arch/x86/kernel/cpu/microcode/
   3.3% arch/x86/kernel/cpu/
   3.8% arch/x86/kernel/
   1.0% arch/x86/platform/efi/
  13.3% arch/x86/
  24.0% arch/
   1.1% drivers/base/
   2.9% drivers/dma/
  12.3% drivers/gpu/drm/i915/
   1.0% drivers/gpu/drm/nouveau/
  16.2% drivers/gpu/drm/
   3.9% drivers/hid/
   1.6% drivers/iio/
   2.3% drivers/regulator/
   ...

Notice? When you say "show only the net subdirectory" it does the
obvious thing relative to the current working directory, but if you
say "show everything _but_ the net subdirectory" it suddenly starts
showing other things.

I can totally see how this can be confusing, but the case can be made
that the problem is that `git diff` would be showing everything if you
didn't pass an exclusion list. Now, what you're suggesting introduces
some inconsistency, which, in itself, can cause confusion.

I'm not sure which confusion is worse.

Mike

On Tue, Feb 07, 2017 at 05:48:26PM -0800, Linus Torvalds wrote:

On Tue, 7 Feb 2017, Linus Torvalds wrote:

quoted

[ Clarification from original message, since Junio asked: I didn't
  actually want the semantics of '.' at all, since in a subdirectory it
  limits to the current subdirectory. So I'd suggest that in the absence
  of any positive pattern, there is simply no filtering at all, so
  whenever I say '.' as a pattern, I really meant ":(top)." which is
  even more of a cumbersom syntax that the current model really
  encourages. Crazy. Since I tend to always work in the top directory,
  the two are the same for me ]

So here's an RFC patch, and I'm quoting the above part of my thinking 
because it's what the patch does, but it turns out that it's probably not 
what we want, and I suspect the "." behavior (as opposed to "no filtering 
at all") is actually better.

Now _I_ don't much care, since I only work from the top level, but without 
the "." behavior, you get into an odd situation that the negative match 
will be relative to the current directory, but then the positive matches 
will be everywhere else. 

Obviously, a negative match that has "top" set would change that logic. So 
this patch is purely a request for further discussion.

When I wrote the patch, I actually also removed the now stale entries from 
the 'po' files, but I'm not including that part here because it just 
distracts from the meat of it all. So this diff was actually generated 
with the new syntax:

	git diff -p --stat -- :^po/

and the only thing even remotely subtle here is that it changes our ctype 
array to make '^' be both a regex and a pathspec magic character.

Everything else should be pretty darn obvious.

The code *could* just track all the 'relative to top or not' bits in the 
exclusion pattern, and then use whatever top-ness the exclusion patterns 
have (and maybe fall back to the old warning if it had a mixture of 
exclusionary patterns). I'll happily change it to act that way if people 
think that makes sense.

Comments?

It seems to me that `git diff` and `git diff -- :^stuff` should have the
same output if there aren't changes in stuff, and `git diff` does the
same as `git diff -- :/` if you are in a subdirectory, not the same as
`git diff .`.

As such, the default positive match should be ':/' (which is shorter and
less cumbersome than ':(top)', btw)

Mike