Pathspec does not accept / does not warn for specific pathspecs

Pathspec does not accept / does not warn for specific pathspecs

[Σταύρος Ντέντος]

Hello there,

as I was trying to use git-archive to ... well, archive some
artifacts. So I called:
git archive HEAD -o p.zip --prefix=p/

Then, I noticed that .gitignore files from the repo were packed (and a
couple of others, but we don't care about that right now).
I wanted to exclude all .gitignore files (there is `/.gitignore` and
`/resources/.gitignore`). So I did:
git archive HEAD -o p.zip --prefix=p/ -- ':!.gitignore'

I noticed that the second-level .gitignore file was still included. I
started thinking that this is not a .gitignore pathspec, but it
matches a literal file.
I don't know why at-the-time I accepted that, since, if I write
`.ignoreFile` at a `.gitignore`, it would exclude any `.ignoreFile` at
any level. So I did:
git archive HEAD -o p.zip --prefix=p/ -- ':!/.gitignore'

Which, as expected, did not change anything; and I followed up with:
git archive HEAD -o p.zip --prefix=p/ -- ':!**/.gitignore'

Then, I noticed some weird behavior. The second-level `.gitignore` was
getting ignored, but not the first one!
Someone at the chat indicated that, the `**` was _magically_
interpreted as `*`, which is obvious to me why it won't help. So, they
suggested:
git archive HEAD -o p.zip --prefix=p/ -- ':!(glob)**/.gitignore'

Which lead to **both of the  .gitignores** to be included. And then I
was baffled.
Now I know (I guess) that the pathspec magic was `:!`, and I tried to
match against a `(glob)**/.gitignore`, i.e. `(glob)*/.gitignore`,
which, of course, it doesn't exist in any form.

And I wonder:
Why doesn't `git archive ... -- ` understand `:!.gitignore` as a
.gitignore file would do (minus the `:`)?
Why doesn't `git archive ... -- ` understand `:!**/.gitignore` as a
.gitignore file would do (minus the `:`)?
Is there some reason that `git archive ... -- ` doesn't understand
`:!(glob)**/.gitignore`? It doesn't sound awfully complicated, or
risking a lot of regression (a folder starting with `(`, and
containing valid pathspec long forms IMHO is rare), or
Is there some reason that `git archive ... -- ` doesn't warn me that
`:!(glob)**/.gitignore` is invalid (and maybe I meant
`(exclude,glob)`)?

With regards,
Ntentos Stavros

Re: Pathspec does not accept / does not warn for specific pathspecs

[Junio C Hamano]

Σταύρος Ντέντος  <stdedos@gmail.com> writes:

> git archive HEAD -o p.zip --prefix=p/ -- ':!(glob)**/.gitignore'
>
> Which lead to **both of the  .gitignores** to be included. And then I
> was baffled.
> Now I know (I guess) that the pathspec magic was `:!`, and I tried to
> match against a `(glob)**/.gitignore`, i.e. `(glob)*/.gitignore`,
> which, of course, it doesn't exist in any form.

You correctly guessed.

The section on pathspec in "git help glossary" may need to be
clarified so that ":!(exclude,glob)" is the right way to spell what
you meant, I think.

> And I wonder:
> Why doesn't `git archive ... -- ` understand `:!.gitignore` as a
> .gitignore file would do (minus the `:`)?

At this point, asking "why" is fruitless.  They are spelled
differently, because pathspecs and .gitignore patterns are simply
different.

> Is there some reason that `git archive ... -- ` doesn't understand
> `:!(glob)**/.gitignore`? It doesn't sound awfully complicated, or
> risking a lot of regression (a folder starting with `(`, and
> containing valid pathspec long forms IMHO is rare),

It was probably OK back in 2006 when the number of users and
projects that are using Git were smaller, but these days, whatever
anybody would think "rare" are used by somebody and it would hurt
many people (in absolute terms---they may be a tiny minority of the
population) if you make a backward incompatible change that breaks
these "rare" cases.

> Is there some reason that `git archive ... -- ` doesn't warn me that
> `:!(glob)**/.gitignore` is invalid (and maybe I meant
> `(exclude,glob)`)?

This one does have merit.  Patches are very much welcomed.

Thanks.

[PATCH v1 0/1] pathspec: warn for a no-glob entry that contains `**`

[Σταύρος Ντέντος]

From: Stavros Ntentos <133706+stdedos@users.noreply.github.com>

This is my first contribution to the git project.

It is the first of the second takes that came from the
https://lore.kernel.org/git/xmqqft1iquka.fsf@gitster.g/ e-mail
(hopefully I linked it correctly using the git-magic commands).

For the review, I pulled in the latest _meaningful_ contributer,
as well as one big contributer to the file.

hopefully, all is in order and correctly done.

Please be gentle with "protocol violations". :-D

Stavros Ntentos (1):
  pathspec: warn for a no-glob entry that contains `**`

 pathspec.c                 | 13 +++++++++++++
 pathspec.h                 |  1 +
 t/t6130-pathspec-noglob.sh | 13 +++++++++++++
 3 files changed, 27 insertions(+)

--
2.31.0

[PATCH v1 1/1] pathspec: warn for a no-glob entry that contains `**`

[Σταύρος Ντέντος]

From: Stavros Ntentos <133706+stdedos@users.noreply.github.com>

If a pathspec is given that contains `**`, chances are that someone is
naively expecting that it will do what the manual has told him that `**`
will match (i.e. 0-or-more directories).

However, without an explicit `:(glob)` magic, that will fall out the sky:
the two `**` will merge into one star, which surrounded by slashes, will
match any directory name.

These changes attempt to bring awareness to this issue.

Signed-off-by: Stavros Ntentos <133706+stdedos@users.noreply.github.com>
---
 pathspec.c                 | 13 +++++++++++++
 pathspec.h                 |  1 +
 t/t6130-pathspec-noglob.sh | 13 +++++++++++++
 3 files changed, 27 insertions(+)

diff --git a/pathspec.c b/pathspec.c
index 7a229d8d22..9b5066d9d9 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -1,3 +1,4 @@
+#include <string.h>
 #include "cache.h"
 #include "config.h"
 #include "dir.h"
@@ -588,6 +589,8 @@ void parse_pathspec(struct pathspec *pathspec,
 
 		init_pathspec_item(item + i, flags, prefix, prefixlen, entry);
 
+		check_missing_glob(entry, item[i].magic);
+
 		if (item[i].magic & PATHSPEC_EXCLUDE)
 			nr_exclude++;
 		if (item[i].magic & magic_mask)
@@ -739,3 +742,13 @@ int match_pathspec_attrs(const struct index_state *istate,
 
 	return 1;
 }
+
+void check_missing_glob(const char *entry, int flags) {
+	if (flags & (PATHSPEC_GLOB | PATHSPEC_LITERAL)) {
+		return;
+	}
+
+	if (strstr(entry, "**")) {
+		warning(_("Pathspec provided contains `**`, but no :(glob) magic.\n\tIt will not match 0 or more directories!"));
+	}
+}
diff --git a/pathspec.h b/pathspec.h
index 454ce364fa..913518ebd3 100644
--- a/pathspec.h
+++ b/pathspec.h
@@ -157,5 +157,6 @@ char *find_pathspecs_matching_against_index(const struct pathspec *pathspec,
 int match_pathspec_attrs(const struct index_state *istate,
 			 const char *name, int namelen,
 			 const struct pathspec_item *item);
+void check_missing_glob(const char* pathspec_entry, int flags);
 
 #endif /* PATHSPEC_H */
diff --git a/t/t6130-pathspec-noglob.sh b/t/t6130-pathspec-noglob.sh
index ba7902c9cd..1cd5efef5a 100755
--- a/t/t6130-pathspec-noglob.sh
+++ b/t/t6130-pathspec-noglob.sh
@@ -157,4 +157,17 @@ test_expect_success '**/ does not work with :(literal) and --glob-pathspecs' '
 	test_must_be_empty actual
 '
 
+cat > expected <<"EOF"
+warning: Pathspec provided contains `**`, but no glob magic.
+EOF
+test_expect_success '** without :(glob) warns of lacking glob magic' '
+	test_might_fail git stash -- "**/bar" 2>warns &&
+	grep -Ff expected warns
+'
+
+test_expect_success '** with    :(literal) does not warn of lacking glob magic' '
+	test_might_fail git stash -- ":(literal)**/bar" 2>warns &&
+	! grep -Ff expected warns
+'
+
 test_done
-- 
2.31.0

Re: [PATCH v1 1/1] pathspec: warn for a no-glob entry that contains `**`

[Bagas Sanjaya]

On 25/03/21 17.22, Σταύρος Ντέντος wrote:
> From: Stavros Ntentos <133706+stdedos@users.noreply.github.com>
> 
> If a pathspec is given that contains `**`, chances are that someone is
> naively expecting that it will do what the manual has told him that `**`
> will match (i.e. 0-or-more directories).
> 
> However, without an explicit `:(glob)` magic, that will fall out the sky:
> the two `**` will merge into one star, which surrounded by slashes, will
> match any directory name.
> 
> These changes attempt to bring awareness to this issue.
> 
> Signed-off-by: Stavros Ntentos <133706+stdedos@users.noreply.github.com>
> ---
>   pathspec.c                 | 13 +++++++++++++
>   pathspec.h                 |  1 +
>   t/t6130-pathspec-noglob.sh | 13 +++++++++++++
>   3 files changed, 27 insertions(+)
> 
> diff --git a/pathspec.c b/pathspec.c
> index 7a229d8d22..9b5066d9d9 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -1,3 +1,4 @@
> +#include <string.h>
>   #include "cache.h"
>   #include "config.h"
>   #include "dir.h"
> @@ -588,6 +589,8 @@ void parse_pathspec(struct pathspec *pathspec,
>   
>   		init_pathspec_item(item + i, flags, prefix, prefixlen, entry);
>   
> +		check_missing_glob(entry, item[i].magic);
> +
>   		if (item[i].magic & PATHSPEC_EXCLUDE)
>   			nr_exclude++;
>   		if (item[i].magic & magic_mask)
> @@ -739,3 +742,13 @@ int match_pathspec_attrs(const struct index_state *istate,
>   
>   	return 1;
>   }
> +
> +void check_missing_glob(const char *entry, int flags) {
> +	if (flags & (PATHSPEC_GLOB | PATHSPEC_LITERAL)) {
> +		return;
> +	}
> +
> +	if (strstr(entry, "**")) {
> +		warning(_("Pathspec provided contains `**`, but no :(glob) magic.\n\tIt will not match 0 or more directories!"));
> +	}
Why did you add an extra \t? I think it is unnecessary indentation.
> +}
> diff --git a/pathspec.h b/pathspec.h
> index 454ce364fa..913518ebd3 100644
> --- a/pathspec.h
> +++ b/pathspec.h
> @@ -157,5 +157,6 @@ char *find_pathspecs_matching_against_index(const struct pathspec *pathspec,
>   int match_pathspec_attrs(const struct index_state *istate,
>   			 const char *name, int namelen,
>   			 const struct pathspec_item *item);
> +void check_missing_glob(const char* pathspec_entry, int flags);
>   
>   #endif /* PATHSPEC_H */
> diff --git a/t/t6130-pathspec-noglob.sh b/t/t6130-pathspec-noglob.sh
> index ba7902c9cd..1cd5efef5a 100755
> --- a/t/t6130-pathspec-noglob.sh
> +++ b/t/t6130-pathspec-noglob.sh
> @@ -157,4 +157,17 @@ test_expect_success '**/ does not work with :(literal) and --glob-pathspecs' '
>   	test_must_be_empty actual
>   '
>   
> +cat > expected <<"EOF"
> +warning: Pathspec provided contains `**`, but no glob magic.
> +EOF
> +test_expect_success '** without :(glob) warns of lacking glob magic' '
> +	test_might_fail git stash -- "**/bar" 2>warns &&
> +	grep -Ff expected warns
> +'
> +
> +test_expect_success '** with    :(literal) does not warn of lacking glob magic' '
Padding maybe?
> +	test_might_fail git stash -- ":(literal)**/bar" 2>warns &&
> +	! grep -Ff expected warns
> +'
> +
>   test_done
> 

-- 
An old man doll... just what I always wanted! - Clara

Re: [PATCH v1 1/1] pathspec: warn for a no-glob entry that contains `**`

[Bagas Sanjaya]

On 25/03/21 17.22, Σταύρος Ντέντος wrote:
> From: Stavros Ntentos <133706+stdedos@users.noreply.github.com>
> 
> If a pathspec is given that contains `**`, chances are that someone is
> naively expecting that it will do what the manual has told him that `**`
> will match (i.e. 0-or-more directories).
> 
> However, without an explicit `:(glob)` magic, that will fall out the sky:
> the two `**` will merge into one star, which surrounded by slashes, will
> match any directory name.
> 
> These changes attempt to bring awareness to this issue.
> 
> Signed-off-by: Stavros Ntentos <133706+stdedos@users.noreply.github.com>
> ---
>   pathspec.c                 | 13 +++++++++++++
>   pathspec.h                 |  1 +
>   t/t6130-pathspec-noglob.sh | 13 +++++++++++++
>   3 files changed, 27 insertions(+)
> 
> diff --git a/pathspec.c b/pathspec.c
> index 7a229d8d22..9b5066d9d9 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -1,3 +1,4 @@
> +#include <string.h>
>   #include "cache.h"
>   #include "config.h"
>   #include "dir.h"
> @@ -588,6 +589,8 @@ void parse_pathspec(struct pathspec *pathspec,
>   
>   		init_pathspec_item(item + i, flags, prefix, prefixlen, entry);
>   
> +		check_missing_glob(entry, item[i].magic);
> +
>   		if (item[i].magic & PATHSPEC_EXCLUDE)
>   			nr_exclude++;
>   		if (item[i].magic & magic_mask)
> @@ -739,3 +742,13 @@ int match_pathspec_attrs(const struct index_state *istate,
>   
>   	return 1;
>   }
> +
> +void check_missing_glob(const char *entry, int flags) {
> +	if (flags & (PATHSPEC_GLOB | PATHSPEC_LITERAL)) {
> +		return;
> +	}
> +
> +	if (strstr(entry, "**")) {
> +		warning(_("Pathspec provided contains `**`, but no :(glob) magic.\n\tIt will not match 0 or more directories!"));
> +	}
Why an extra \t? Unnecessary indentation?
> +}
> diff --git a/pathspec.h b/pathspec.h
> index 454ce364fa..913518ebd3 100644
> --- a/pathspec.h
> +++ b/pathspec.h
> @@ -157,5 +157,6 @@ char *find_pathspecs_matching_against_index(const struct pathspec *pathspec,
>   int match_pathspec_attrs(const struct index_state *istate,
>   			 const char *name, int namelen,
>   			 const struct pathspec_item *item);
> +void check_missing_glob(const char* pathspec_entry, int flags);
>   
>   #endif /* PATHSPEC_H */
> diff --git a/t/t6130-pathspec-noglob.sh b/t/t6130-pathspec-noglob.sh
> index ba7902c9cd..1cd5efef5a 100755
> --- a/t/t6130-pathspec-noglob.sh
> +++ b/t/t6130-pathspec-noglob.sh
> @@ -157,4 +157,17 @@ test_expect_success '**/ does not work with :(literal) and --glob-pathspecs' '
>   	test_must_be_empty actual
>   '
>   
> +cat > expected <<"EOF"
> +warning: Pathspec provided contains `**`, but no glob magic.
> +EOF
> +test_expect_success '** without :(glob) warns of lacking glob magic' '
> +	test_might_fail git stash -- "**/bar" 2>warns &&
> +	grep -Ff expected warns
> +'
> +
> +test_expect_success '** with    :(literal) does not warn of lacking glob magic' '
Padding with without test above?
> +	test_might_fail git stash -- ":(literal)**/bar" 2>warns &&
> +	! grep -Ff expected warns
> +'
> +
>   test_done
> 

-- 
An old man doll... just what I always wanted! - Clara

[PATCH v1 1/1] pathspec: warn for a no-glob entry that contains `**`

[Stavros Ntentos]

> > +	if (strstr(entry, "**")) {
> > +		warning(_("Pathspec provided contains `**`, but no :(glob) magic.\n\tIt will not match 0 or more directories!"));
> > +	}
> Why an extra \t? Unnecessary indentation?

It brings out the warning label:

root@30f6bde171fe:/usr/src/git/t# ../git stash -- **/bar
warning: Pathspec provided contains `**`, but no :(glob) magic.
	It will not match 0 or more directories!
error: pathspec ':(,prefix:2)t/**/bar' did not match any file(s) known to git
Did you forget to 'git add'?

and "makes the user feel" that these two lines are in reality one
(but this is way over any sensible line limit to present as such).

I would've padded exactly, but:
* `\t` expansion is terminal-specific (usually set at 8, but not guaranteed)
* ` `-only would've been too long of a padding to an already long line

Ofc, if someone really wanted to solve this, someone
could rework the `void vreportf` split and auto-pad
prefix at newline, but sounds like a project on its own ...

> > +test_expect_success '** with    :(literal) does not warn of lacking glob magic' '
> Padding with without test above?

Yes; it somewhat aligns individual test output:

root@30f6bde171fe:/usr/src/git/t# ./t6130-pathspec-noglob.sh
...
ok 22 - ** without :(glob) warns of lacking glob magic
ok 23 - ** with    :(literal) does not warn of lacking glob magic
# passed all 23 test(s)

to emphasize they are a positive/negative test pair.

I don't know how well I feel about this anyway; I can undo it.

Re: [PATCH v1 1/1] pathspec: warn for a no-glob entry that contains `**`

[Junio C Hamano]

Stavros Ntentos <stdedos@gmail.com> writes:

> Ofc, if someone really wanted to solve this, someone
> could rework the `void vreportf` split and auto-pad
> prefix at newline, but sounds like a project on its own ...

But when it happens, this extra "\t" would get in the way
and needs to be adjusted ;-)

It has been on the radar for quite some time to apply the same
principle for die(), error(), and warning() as how advise()
shows its "hint:" prefix.

The format strings given to these functions are end-user-facing and
subject to localization, so there is no huge compatibility issues we
need to be worried about.

The test suite have fixed expectations and such a change must be
accompanied by adjustment to the affected tests, though.

[PATCH v2 0/1] pathspec: warn for a no-glob entry that contains `**`

[Σταύρος Ντέντος]

From: Stavros Ntentos <133706+stdedos@users.noreply.github.com>

Small amends, here and there, and fixed/reverted the whitespaces

Stavros Ntentos (1):
  pathspec: warn for a no-glob entry that contains `**`

 pathspec.c                 | 13 +++++++++++++
 pathspec.h                 |  1 +
 t/t6130-pathspec-noglob.sh | 13 +++++++++++++
 3 files changed, 27 insertions(+)

--
2.31.0

[PATCH v2 1/1] pathspec: warn for a no-glob entry that contains `**`

[Σταύρος Ντέντος]

From: Stavros Ntentos <133706+stdedos@users.noreply.github.com>

If a pathspec given that contains `**`, chances are that someone is
naively expecting that it will do what the manual has told him
(i.e. that `**` will match 0-or-more directories).

However, without an explicit `:(glob)` magic, that will fall out the sky:
the two `**` will merge into one star, which surrounded by slashes, will
match any directory name.

These changes attempt to bring awareness to this issue.

Signed-off-by: Stavros Ntentos <133706+stdedos@users.noreply.github.com>
---
 pathspec.c                 | 13 +++++++++++++
 pathspec.h                 |  1 +
 t/t6130-pathspec-noglob.sh | 13 +++++++++++++
 3 files changed, 27 insertions(+)

diff --git a/pathspec.c b/pathspec.c
index 7a229d8d22..d5b9c0d792 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -1,3 +1,4 @@
+#include <string.h>
 #include "cache.h"
 #include "config.h"
 #include "dir.h"
@@ -588,6 +589,8 @@ void parse_pathspec(struct pathspec *pathspec,
 
 		init_pathspec_item(item + i, flags, prefix, prefixlen, entry);
 
+		check_missing_glob(entry, item[i].magic);
+
 		if (item[i].magic & PATHSPEC_EXCLUDE)
 			nr_exclude++;
 		if (item[i].magic & magic_mask)
@@ -739,3 +742,13 @@ int match_pathspec_attrs(const struct index_state *istate,
 
 	return 1;
 }
+
+void check_missing_glob(const char *pathspec_entry, int flags) {
+	if (flags & (PATHSPEC_GLOB | PATHSPEC_LITERAL)) {
+		return;
+	}
+
+	if (strstr(pathspec_entry, "**")) {
+		warning(_("Pathspec provided contains `**`, but no :(glob) magic.\nIt will not match 0 or more directories!"));
+	}
+}
diff --git a/pathspec.h b/pathspec.h
index 454ce364fa..913518ebd3 100644
--- a/pathspec.h
+++ b/pathspec.h
@@ -157,5 +157,6 @@ char *find_pathspecs_matching_against_index(const struct pathspec *pathspec,
 int match_pathspec_attrs(const struct index_state *istate,
 			 const char *name, int namelen,
 			 const struct pathspec_item *item);
+void check_missing_glob(const char* pathspec_entry, int flags);
 
 #endif /* PATHSPEC_H */
diff --git a/t/t6130-pathspec-noglob.sh b/t/t6130-pathspec-noglob.sh
index ba7902c9cd..af6cd16f76 100755
--- a/t/t6130-pathspec-noglob.sh
+++ b/t/t6130-pathspec-noglob.sh
@@ -157,4 +157,17 @@ test_expect_success '**/ does not work with :(literal) and --glob-pathspecs' '
 	test_must_be_empty actual
 '
 
+cat > expected <<"EOF"
+warning: Pathspec provided contains `**`, but no :(glob) magic.
+EOF
+test_expect_success '** without :(glob) warns of lacking glob magic' '
+	test_might_fail git stash -- "**/bar" 2>warns &&
+	grep -Ff expected warns
+'
+
+test_expect_success '** with :(literal) does not warn of lacking glob magic' '
+	test_might_fail git stash -- ":(literal)**/bar" 2>warns &&
+	! grep -Ff expected warns
+'
+
 test_done
-- 
2.31.0

Re: [PATCH v2 1/1] pathspec: warn for a no-glob entry that contains `**`

[Junio C Hamano]

"Σταύρος Ντέντος"  <stdedos@gmail.com> writes:

> From: Stavros Ntentos <133706+stdedos@users.noreply.github.com>
>
> If a pathspec given that contains `**`, chances are that someone is
> naively expecting that it will do what the manual has told him
> (i.e. that `**` will match 0-or-more directories).

OK. this is a well written introduction of the problem being solved.

But chances are that the user may have meant to give double-asterisk
just out of habit, very well knowing that it would not be an error
to give ** when a single * suffices.

Which leads us to suspect that it is a bad change to make this a
warning that unconditionally fires and cannot squelch.  It may be
better to implement it as an advice, where the user who knowingly
uses that construct can say "yes, I know what I am doing" and
squelch it.

> However, without an explicit `:(glob)` magic, that will fall out the sky:
> the two `**` will merge into one star, which surrounded by slashes, will
> match any directory name.

I am not sure everything after the "the sky:" you wrote is what you
meant to write.  Without being marked with a "glob" magic, a
wildcard character in a pattern matches even a slash, so these two

	git ls-files 'Documentation**v2.txt'
	git ls-files 'Documentation*v2.txt'

give the identical result and there is nothing about "surrounded by
slashes" involved in it.

So, perhaps taking the first two paragraphs together and rewriting:

	When '/**/' appears in the pathspec, the user may be
	expecting that it would be matched using the "wildmatch"
	semantics, matching 0 or more directories.  But that is
	not what happens without ":(glob)" magic.

or did you want to warn about "foo**bar" as if it were "foo/**/bar"?

The output from these commands:

	git ls-files ':(glob)Documentation/**/*stash.txt'
	git ls-files ':(glob)Documentation***stash.txt'
	git ls-files ':(glob)Documentation**stash.txt'

seems to tell me that the "zero-or-more directories" magic happens
only when /**/ form is used, not when two asterisks are placed next
to each other in a random context.

> These changes attempt to bring awareness to this issue.

In any case, after presenting what problem we are trying to address,
we present our approach / solution in a form as if we are giving
orders to the codebase to "become like so".

	Teach the pathspec parser to emit an advice message when a
	substring "/**/" appears in a pathspec element that does not
	have a ":(glob)" magic.  Make sure we don't disturb users
	who use ":(literal)" magic with such a substring, as it is
	clear they want to find these strings literally.

perhaps.

> Signed-off-by: Stavros Ntentos <133706+stdedos@users.noreply.github.com>
> ---
>  pathspec.c                 | 13 +++++++++++++
>  pathspec.h                 |  1 +
>  t/t6130-pathspec-noglob.sh | 13 +++++++++++++
>  3 files changed, 27 insertions(+)
>
> diff --git a/pathspec.c b/pathspec.c
> index 7a229d8d22..d5b9c0d792 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -1,3 +1,4 @@
> +#include <string.h>

Never include any header file before including <git-compat-util.h>.
Including <git-compat-util.h> indirectly by including <cache.h> or
<builtin.h> counts as including it as the first thing.

As all necessary standard system headers are supposed to be given by
including <git-compat-util.h>, if you needed to explicitly include
the above, that may mean <git-compat-util.h>, which should cause the
header that lists necessary function, is not working properly on
your platform and needs to be adjusted. Are you on a minority
platform we haven't adjusted the header inclusion order for, and
what function are you trying to use that does not work without
adding this extra #include here?  We already use strstr() in many
places, so that is not the function we are missing system includes
for.

Or perhaps this is a remnant of what you added while you were
experimenting, without realizing that "#include <cache.h>" that is
already there already gives you anything you need.  If that is the
case, iow, if the code works without the extra include, please
remove that line.

> @@ -588,6 +589,8 @@ void parse_pathspec(struct pathspec *pathspec,
>  
>  		init_pathspec_item(item + i, flags, prefix, prefixlen, entry);
>  
> +		check_missing_glob(entry, item[i].magic);
> +

We have only one caller of the helper (i.e. this one) and nowhere
else, and the helper is small enough ...

> @@ -739,3 +742,13 @@ int match_pathspec_attrs(const struct index_state *istate,
>  
>  	return 1;
>  }
> +
> +void check_missing_glob(const char *pathspec_entry, int flags) {
> +	if (flags & (PATHSPEC_GLOB | PATHSPEC_LITERAL)) {
> +		return;
> +	}
> +
> +	if (strstr(pathspec_entry, "**")) {
> +		warning(_("Pathspec provided contains `**`, but no :(glob) magic.\nIt will not match 0 or more directories!"));
> +	}
> +}

... hence, there is no reason why this helper should exist, let
alone be a public function.  Also, there is no reason to split the
conditional into two.  Just "it is OK if we have GLOB or LITERAL,
otherwise see if there is /**/" is sufficient.

It would be more sensible to just add the check to parse_pathspec()
directly.

Also, our warning messages do not begin with an uppercase.

See attached patch for all of the above, but it is for illustration
purposes only; not even compile tested.  I am not illustrating how
to turn this into an advice message that the user can squelch with
the illustration patch.

> diff --git a/t/t6130-pathspec-noglob.sh b/t/t6130-pathspec-noglob.sh
> index ba7902c9cd..af6cd16f76 100755
> --- a/t/t6130-pathspec-noglob.sh
> +++ b/t/t6130-pathspec-noglob.sh
> @@ -157,4 +157,17 @@ test_expect_success '**/ does not work with :(literal) and --glob-pathspecs' '
>  	test_must_be_empty actual
>  '
>  

> +cat > expected <<"EOF"
> +warning: Pathspec provided contains `**`, but no :(glob) magic.
> +EOF

Please don't imitate ancient tests.  These days, all preparations
for individual tests, including the expected outcome, are to be done
inside the test itself.  Study nearby tests in the same script for
better examples.

> +test_expect_success '** without :(glob) warns of lacking glob magic' '
> +	test_might_fail git stash -- "**/bar" 2>warns &&
> +	grep -Ff expected warns

Same comment.  Nearby examples all set up expeced output and never
uses "grep" to see if the expectation is satisfied.  Imitate them.

> +'
> +
> +test_expect_success '** with :(literal) does not warn of lacking glob magic' '
> +	test_might_fail git stash -- ":(literal)**/bar" 2>warns &&
> +	! grep -Ff expected warns

Ditto.

Thanks.


 pathspec.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git i/pathspec.c w/pathspec.c
index 18b3be362a..5b0eed5a65 100644
--- i/pathspec.c
+++ w/pathspec.c
@@ -598,6 +598,10 @@ void parse_pathspec(struct pathspec *pathspec,
 			die(_("pathspec '%s' is beyond a symbolic link"), entry);
 		}
 
+		if (!(item[i].magic & (PATHSPEC_GLOB | PATHSPEC_LITERAL)) &&
+		    strstr(entry, "**"))
+			warning(_("** in '%s'without :(glob) magic:"), entry);
+
 		if (item[i].nowildcard_len < item[i].len)
 			pathspec->has_wildcard = 1;
 		pathspec->magic |= item[i].magic;

Re: [PATCH v2 1/1] pathspec: warn for a no-glob entry that contains `**`

[Eric Sunshine]

On Thu, Mar 25, 2021 at 8:43 PM Junio C Hamano <gitster@pobox.com> wrote:
> So, perhaps taking the first two paragraphs together and rewriting:
>
>         When '/**/' appears in the pathspec, the user may be
>         expecting that it would be matched using the "wildmatch"
>         semantics, matching 0 or more directories.  But that is
>         not what happens without ":(glob)" magic.
>
>         Teach the pathspec parser to emit an advice message when a
>         substring "/**/" appears in a pathspec element that does not
>         have a ":(glob)" magic.  Make sure we don't disturb users
>         who use ":(literal)" magic with such a substring, as it is
>         clear they want to find these strings literally.

I haven't been following the discussion, but is there a reason we need
to penalize the user with a warning rather than helping, for instance
by inferring ":(glob)" in the presence of `/**/` if not otherwise
countermanded by ":(literal)" or whatnot?

Re: [PATCH v2 1/1] pathspec: warn for a no-glob entry that contains `**`

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> I haven't been following the discussion, but is there a reason we need
> to penalize the user with a warning rather than helping, for instance
> by inferring ":(glob)" in the presence of `/**/` if not otherwise
> countermanded by ":(literal)" or whatnot?

Two reasons I can think of offhand are

 - How /**/ is interpreted is not the only thing that is different
   between the normal mode and the glob magic mode.  IIRC, an
   asterisk * or a question mark ? matches slash in normal mode (it
   started out as fnmatch() without FNM_PATHNAME).  Should we warn
   about ":(glob)" if somebody asks for "foo*", "*foo", or
   "foo*bar".  If not, why shouldn't?

 - Thers is no explicit magic that says "there is no magic" to
   countermand such a DWIM.

Re: [PATCH v2 1/1] pathspec: warn for a no-glob entry that contains `**`

[Jeff King]

On Thu, Mar 25, 2021 at 08:02:31PM -0700, Junio C Hamano wrote:

> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
> > I haven't been following the discussion, but is there a reason we need
> > to penalize the user with a warning rather than helping, for instance
> > by inferring ":(glob)" in the presence of `/**/` if not otherwise
> > countermanded by ":(literal)" or whatnot?
> 
> Two reasons I can think of offhand are
> 
>  - How /**/ is interpreted is not the only thing that is different
>    between the normal mode and the glob magic mode.  IIRC, an
>    asterisk * or a question mark ? matches slash in normal mode (it
>    started out as fnmatch() without FNM_PATHNAME).  Should we warn
>    about ":(glob)" if somebody asks for "foo*", "*foo", or
>    "foo*bar".  If not, why shouldn't?
> 
>  - Thers is no explicit magic that says "there is no magic" to
>    countermand such a DWIM.

I do wonder if this distinction creates more harm than good.

As somebody who has never used ":(glob)" myself, I was confused about
what it even does (and it was not easy to find the documentation; I
ended up finding the original commit in the history first!).

We have three modes:

  - no globbing

  - globbing with fnmatch(), with FNM_PATHNAME according to the docs

  - globbing with wildmatch

You may notice that I would call both of those latter two "globbing",
but only one of them is triggered by the ":(glob)" magic. :)

This just seems really confusing, and I wonder if anybody would be that
sad if we just used wildmatch everywhere. The original bd30c2e484
(pathspec: support :(glob) syntax, 2013-07-14) even says:

  The old fnmatch behavior is considered deprecated and discouraged to
  use.

but I guess it would be backwards-incompatible.

Maybe it would be less confusing if we named the three states
explicitly:

  :(literal)
  :(fnmatch)
  :(wildmatch)

(and keeping :(glob) as a synonym for compatibility).

-Peff

Re: [PATCH v2 1/1] pathspec: warn for a no-glob entry that contains `**`

[Stavros Ntentos]

I think I have managed to address everyone's questions in this thread.
Hopefully everything will be addressed by this, and the patch that will soon follow:

> You may notice that I would call both of those latter two "globbing",
> but only one of them is triggered by the ":(glob)" magic. :)

And that's why I argued a DWIM was warranted here
(https://lore.kernel.org/git/xmqqft1iquka.fsf@gitster.g/; it's more clear
in Junio's quote, but you could of course read the original).

I would want to be considered as an above-average git user, and I still was
oblivious to the fact that `**` required a `:(glob)` from the command line.
Especially since `.gitignore` files are treated differently
(i.e. don't require `:(glob)`)

I cannot / don't want to argue to "do it" or not, as I think my experience
is not substantial enough to navigate such argument, and go from concept
to materialization.

That being said, if there was a cli.pathspec.wildmatch flag,
I would've had it set to true in my global gitconfig.
Ofc I could set `GIT_GLOB_PATHSPECS=1`, but I am not that happy to
force it by setting it in a shell rc, instead of where it belongs.

> I am not sure everything after the "the sky:" you wrote is what you
> meant to write.  Without being marked with a "glob" magic, a
> wildcard character in a pattern matches even a slash, so these two
>
> 	git ls-files 'Documentation**v2.txt'
> 	git ls-files 'Documentation*v2.txt'
>
> give the identical result and there is nothing about "surrounded by
> slashes" involved in it.

It seems you are right - in `fnmatch` mode, `:!*.gitignore` would've
served me right (and avoided the whole thread).

If you think that it's just my (i.e. few people's) lacking of understanding
`fnmatch` glob, then we can drop this.
However, given the disparity between `.gitignore` syntax and pathspec given
from the command line (from my limited POV meaningless/confusing), and your
(plural) arguments of backwards compatibility, I think we can draw the line
at an advice been acceptable.

Unless I see other points (like the warn vs advice), or pure C code-review
points, I am getting the impression that this thread will not move forward.
As I don't know how reviews usually happen here, and lacking some other medium
of discussion, I would appreciate (at some point) an explicit go/no-go
decision - to save everyone's time.

Being unfamiliar to the project, and being who I am, I prefer explicit vs implicit points.
Navigating an unknown process from top to bottom is pressure enough to me :-D

> seems to tell me that the "zero-or-more directories" magic happens
> only when /**/ form is used, not when two asterisks are placed next
> to each other in a random context.

Not exactly: ** needs to either start or end with a slash (or both) for its
wildmatch behavior. I can try to make the code more explicit, but of course
the code (and tests) will increase - and then maybe disproportionately to
the otherwise intended-to-be small change (since DWIM is not wanted).

[PATCH v3 1/2] pathspec: warn for a no-glob entry that contains `**`

[Stavros Ntentos]

If a pathspec given that contains `**`, chances are that someone is
naively expecting that it will do what the manual has told him
(i.e. that `**` will match 0-or-more directories).

When `**` appears in the pathspec, the user may be expecting that
it would be matched using the "wildmatch" semantics,
matching 0 or more directories.
That is not what happens without ":(glob)" magic.

Teach the pathspec parser to emit an advice message when a substring
`**` appears in a pathspec element that does not have a `:(glob)` magic.
Make sure we don't disturb users who use ":(literal)" magic
with such a substring, as it is clear they want to find these strings literally.

Signed-off-by: Stavros Ntentos <133706+stdedos@users.noreply.github.com>
---
 pathspec.c                 | 13 +++++++++++++
 pathspec.h                 |  1 +
 t/t6130-pathspec-noglob.sh | 13 +++++++++++++
 3 files changed, 27 insertions(+)

diff --git a/pathspec.c b/pathspec.c
index 7a229d8d22..d5b9c0d792 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -1,3 +1,4 @@
+#include <string.h>
 #include "cache.h"
 #include "config.h"
 #include "dir.h"
@@ -588,6 +589,8 @@ void parse_pathspec(struct pathspec *pathspec,
 
 		init_pathspec_item(item + i, flags, prefix, prefixlen, entry);
 
+		check_missing_glob(entry, item[i].magic);
+
 		if (item[i].magic & PATHSPEC_EXCLUDE)
 			nr_exclude++;
 		if (item[i].magic & magic_mask)
@@ -739,3 +742,13 @@ int match_pathspec_attrs(const struct index_state *istate,
 
 	return 1;
 }
+
+void check_missing_glob(const char *pathspec_entry, int flags) {
+	if (flags & (PATHSPEC_GLOB | PATHSPEC_LITERAL)) {
+		return;
+	}
+
+	if (strstr(pathspec_entry, "**")) {
+		warning(_("Pathspec provided contains `**`, but no :(glob) magic.\nIt will not match 0 or more directories!"));
+	}
+}
diff --git a/pathspec.h b/pathspec.h
index 454ce364fa..913518ebd3 100644
--- a/pathspec.h
+++ b/pathspec.h
@@ -157,5 +157,6 @@ char *find_pathspecs_matching_against_index(const struct pathspec *pathspec,
 int match_pathspec_attrs(const struct index_state *istate,
 			 const char *name, int namelen,
 			 const struct pathspec_item *item);
+void check_missing_glob(const char* pathspec_entry, int flags);
 
 #endif /* PATHSPEC_H */
diff --git a/t/t6130-pathspec-noglob.sh b/t/t6130-pathspec-noglob.sh
index ba7902c9cd..af6cd16f76 100755
--- a/t/t6130-pathspec-noglob.sh
+++ b/t/t6130-pathspec-noglob.sh
@@ -157,4 +157,17 @@ test_expect_success '**/ does not work with :(literal) and --glob-pathspecs' '
 	test_must_be_empty actual
 '
 
+cat > expected <<"EOF"
+warning: Pathspec provided contains `**`, but no :(glob) magic.
+EOF
+test_expect_success '** without :(glob) warns of lacking glob magic' '
+	test_might_fail git stash -- "**/bar" 2>warns &&
+	grep -Ff expected warns
+'
+
+test_expect_success '** with :(literal) does not warn of lacking glob magic' '
+	test_might_fail git stash -- ":(literal)**/bar" 2>warns &&
+	! grep -Ff expected warns
+'
+
 test_done
-- 
2.31.0

[PATCH v3 2/2] pathspec: convert no-glob warn to advice

[Stavros Ntentos]

Teach git that, if anyone wants to squelch such warning,
can do so via `git config advice.starStarNoGlobPathspec false`
---
 Documentation/config/advice.txt | 4 ++++
 advice.c                        | 3 +++
 advice.h                        | 2 ++
 pathspec.c                      | 6 ++++--
 t/t6130-pathspec-noglob.sh      | 4 +++-
 5 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index acbd0c09aa..35aa36cfa1 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -119,4 +119,8 @@ advice.*::
 	addEmptyPathspec::
 		Advice shown if a user runs the add command without providing
 		the pathspec parameter.
+	starStarNoGlobPathspec::
+		Advice shown if a user provides a pathspec via the terminal
+		that contains `**`, anticipating that the pattern will be
+		matched using wildmatch (aka `:(glob)` magic).
 --
diff --git a/advice.c b/advice.c
index 164742305f..5703bfba86 100644
--- a/advice.c
+++ b/advice.c
@@ -33,6 +33,7 @@ int advice_checkout_ambiguous_remote_branch_name = 1;
 int advice_submodule_alternate_error_strategy_die = 1;
 int advice_add_ignored_file = 1;
 int advice_add_empty_pathspec = 1;
+int advice_star_star_no_glob_pathspec = 1;
 
 static int advice_use_color = -1;
 static char advice_colors[][COLOR_MAXLEN] = {
@@ -95,6 +96,7 @@ static struct {
 	{ "submoduleAlternateErrorStrategyDie", &advice_submodule_alternate_error_strategy_die },
 	{ "addIgnoredFile", &advice_add_ignored_file },
 	{ "addEmptyPathspec", &advice_add_empty_pathspec },
+	{ "starStarNoGlobPathspec", &advice_star_star_no_glob_pathspec },
 
 	/* make this an alias for backward compatibility */
 	{ "pushNonFastForward", &advice_push_update_rejected }
@@ -137,6 +139,7 @@ static struct {
 	[ADVICE_STATUS_U_OPTION]			= { "statusUoption", 1 },
 	[ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = { "submoduleAlternateErrorStrategyDie", 1 },
 	[ADVICE_WAITING_FOR_EDITOR]			= { "waitingForEditor", 1 },
+	[ADVICE_STAR_STAR_NO_GLOB_PATHSPEC]		= { "starStarNoGlobPathspec", 1 },
 };
 
 static const char turn_off_instructions[] =
diff --git a/advice.h b/advice.h
index bc2432980a..c62fb47434 100644
--- a/advice.h
+++ b/advice.h
@@ -33,6 +33,7 @@ extern int advice_checkout_ambiguous_remote_branch_name;
 extern int advice_submodule_alternate_error_strategy_die;
 extern int advice_add_ignored_file;
 extern int advice_add_empty_pathspec;
+extern int advice_star_star_no_glob_pathspec;
 
 /*
  * To add a new advice, you need to:
@@ -72,6 +73,7 @@ extern int advice_add_empty_pathspec;
 	ADVICE_STATUS_U_OPTION,
 	ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE,
 	ADVICE_WAITING_FOR_EDITOR,
+	ADVICE_STAR_STAR_NO_GLOB_PATHSPEC,
 };
 
 int git_default_advice_config(const char *var, const char *value);
diff --git a/pathspec.c b/pathspec.c
index d5b9c0d792..879d87260d 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -1,4 +1,4 @@
-#include <string.h>
+#include "git-compat-util.h"
 #include "cache.h"
 #include "config.h"
 #include "dir.h"
@@ -744,11 +744,13 @@ int match_pathspec_attrs(const struct index_state *istate,
 }
 
 void check_missing_glob(const char *pathspec_entry, int flags) {
+	const char *advice = NULL;
 	if (flags & (PATHSPEC_GLOB | PATHSPEC_LITERAL)) {
 		return;
 	}
 
+	advice = _("Pathspec provided contains `**`, but no :(glob) magic.\nIt will not match 0 or more directories!");
 	if (strstr(pathspec_entry, "**")) {
-		warning(_("Pathspec provided contains `**`, but no :(glob) magic.\nIt will not match 0 or more directories!"));
+		advise_if_enabled(ADVICE_STAR_STAR_NO_GLOB_PATHSPEC, advice);
 	}
 }
diff --git a/t/t6130-pathspec-noglob.sh b/t/t6130-pathspec-noglob.sh
index af6cd16f76..0482030243 100755
--- a/t/t6130-pathspec-noglob.sh
+++ b/t/t6130-pathspec-noglob.sh
@@ -158,7 +158,9 @@ test_expect_success '**/ does not work with :(literal) and --glob-pathspecs' '
 '
 
 cat > expected <<"EOF"
-warning: Pathspec provided contains `**`, but no :(glob) magic.
+hint: Pathspec provided contains `**`, but no :(glob) magic.
+hint: It will not match 0 or more directories!
+hint: Disable this message with "git config advice.starStarNoGlobPathspec false"
 EOF
 test_expect_success '** without :(glob) warns of lacking glob magic' '
 	test_might_fail git stash -- "**/bar" 2>warns &&
-- 
2.31.0

[RFC PATCH v1 0/1] pathspec: warn: long and short forms are incompatible

[Σταύρος Ντέντος]

From: Stavros Ntentos <133706+stdedos@users.noreply.github.com>

The second of the two issues identified in this thread.

Both patches work as expected.

There is a short solution, which is mostly okay.
It does not compile with -Werror, but:
It avoids malloc/free, and keeps the logic/sloc low.

All of this could be my lack of C experience.

Stavros Ntentos (1):
  pathspec: warn: long and short forms are incompatible

 pathspec.c                  | 30 ++++++++++++++++++++++++++++++
 pathspec.h                  |  1 +
 t/t6132-pathspec-exclude.sh | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 64 insertions(+)

--
2.31.0

[RFC PATCH v1 1/2] pathspec: warn: long and short forms are incompatible

[Σταύρος Ντέντος]

From: Stavros Ntentos <133706+stdedos@users.noreply.github.com>

Namely, `!` and any other long magic form (e.g. `glob`)
cannot be combined to one entry.

Issue a warning when such thing happens, and hint to the solution.

Signed-off-by: Stavros Ntentos <133706+stdedos@users.noreply.github.com>
---
 pathspec.c                  | 19 +++++++++++++++++++
 pathspec.h                  |  1 +
 t/t6132-pathspec-exclude.sh | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 53 insertions(+)

diff --git a/pathspec.c b/pathspec.c
index 7a229d8d22..374c529569 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -1,3 +1,5 @@
+#include <stdio.h>
+#include <string.h>
 #include "cache.h"
 #include "config.h"
 #include "dir.h"
@@ -586,6 +588,8 @@ void parse_pathspec(struct pathspec *pathspec,
 	for (i = 0; i < n; i++) {
 		entry = argv[i];
 
+		check_mishandled_exclude(entry);
+
 		init_pathspec_item(item + i, flags, prefix, prefixlen, entry);
 
 		if (item[i].magic & PATHSPEC_EXCLUDE)
@@ -739,3 +743,18 @@ int match_pathspec_attrs(const struct index_state *istate,
 
 	return 1;
 }
+
+void check_mishandled_exclude(const char *entry) {
+	size_t entry_len = strlen(entry);
+	char flags[entry_len];
+	char path[entry_len];
+
+	if (sscanf(entry, ":!(%4096[^)])%4096s", &flags, &path) != 2) {
+		return;
+	}
+	if (count_slashes(flags) > 0) {
+		return;
+	}
+
+	warning(_("Pathspec provided matches `:!(...)`\n\tDid you mean `:(exclude,...)`?"));
+}
diff --git a/pathspec.h b/pathspec.h
index 454ce364fa..879d4e82c6 100644
--- a/pathspec.h
+++ b/pathspec.h
@@ -157,5 +157,6 @@ char *find_pathspecs_matching_against_index(const struct pathspec *pathspec,
 int match_pathspec_attrs(const struct index_state *istate,
 			 const char *name, int namelen,
 			 const struct pathspec_item *item);
+void check_mishandled_exclude(const char* pathspec_entry);
 
 #endif /* PATHSPEC_H */
diff --git a/t/t6132-pathspec-exclude.sh b/t/t6132-pathspec-exclude.sh
index 30328b87f0..b32ddb2a56 100755
--- a/t/t6132-pathspec-exclude.sh
+++ b/t/t6132-pathspec-exclude.sh
@@ -244,4 +244,37 @@ test_expect_success 'grep --untracked PATTERN :(exclude)*FILE' '
 	test_cmp expect-grep actual-grep
 '
 
+cat > expected_warn <<"EOF"
+Pathspec provided matches `:!(...)`
+EOF
+test_expect_success 'warn pathspec :!(...) skips the parenthesized magics' '
+	git log --oneline --format=%s -- '"'"':!(glob)**/file'"'"' >actual 2>warn &&
+	cat <<EOF >expect &&
+sub2/file
+sub/sub/sub/file
+sub/file2
+sub/sub/file
+sub/file
+file
+EOF
+	cat actual &&
+	cat warn &&
+	test_cmp expect actual &&
+	grep -Ff expected_warn warn
+'
+
+test_expect_success 'do not warn that pathspec :!(...) skips the parenthesized magics (if parenthesis would not be part of the magic)' '
+	git log --oneline --format=%s -- '"'"':!(gl/ob)/file'"'"' >actual 2>warn &&
+	cat <<EOF >expect &&
+sub2/file
+sub/sub/sub/file
+sub/file2
+sub/sub/file
+sub/file
+file
+EOF
+	test_cmp expect actual &&
+	! grep -Ff expected_warn warn
+'
+
 test_done
-- 
2.31.0

Re: [RFC PATCH v1 1/2] pathspec: warn: long and short forms are incompatible

[Jeff King]

On Fri, Mar 26, 2021 at 04:40:04AM +0200, Σταύρος Ντέντος wrote:

> +void check_mishandled_exclude(const char *entry) {
> +	size_t entry_len = strlen(entry);
> +	char flags[entry_len];
> +	char path[entry_len];

We avoid using variable-length arrays in our codebase. For one thing,
they were not historically supported by all platforms (we are slowly
using more C99 features, but we are introducing them slowly and
intentionally).

But two, they are limited in size and the failure mode is not graceful.
If "entry" is larger than the available stack, then we'll get a segfault
with no option to handle it better.

> +	if (sscanf(entry, ":!(%4096[^)])%4096s", &flags, &path) != 2) {
> +		return;

We also generally avoid using scanf, because it's error-prone. The
"4096" is scary here, but I don't _think_ it's a buffer overflow,
because "path" is already the same size as "entry" (not including the
NUL terminator, but that is negated by the fact that we'll have skipped
at least ":!").

Is this "%4096[^)]" actually valid? I don't think scanf understands
regular expressions.

We'd want to avoid making an extra copy of the string anyway. So you'd
probably want to just parse left-to-right in the original string, like:

  const char *p = entry;

  /* skip past stuff we know must be there */
  if (!skip_prefix(p, ":!(", &p))
        return;

  /* this checks count_slashes() > 0 in the flags section, though I'm
   * not sure I understand what that is looking for... */
  for (; *p && *p != ')'; p++) {
        if (*p == '/')
	        return;
  }
  if (*p++ != ')')
        return;

  /* now p is pointing at "path", though we don't seem to do anything
   * with it... */

-Peff

Re: [RFC PATCH v1 1/2] pathspec: warn: long and short forms are incompatible

[Stavros Ntentos]

> We avoid using variable-length arrays in our codebase. ...

Hear hear, however, I wanted to avoid the "small mess"
that allocate/free would cause (one or more of ++sloc, labels, if-nesting); ...

> But two, they are limited in size and the failure mode is not graceful. ...
... however, my main issue is that - I don't know what's a sane allocation size.

> ... The "4096" is scary here ...
While scary, it is "a safe" upper high.
The first time the string ends up in pathspec.c for processing it's here:
	entry = argv[i];
which comes from here
	parse_pathspec(pathspec, magic_mask, flags, prefix, parsed_file.v);

and I don't know what's the maximum size of `parsed_file.v[0]`

> Is this "%4096[^)]" actually valid? I don't think scanf understands
> regular expressions.

I was suprised too - but it's not regex.

see: https://www.cplusplus.com/reference/cstdio/scanf/#parameters - 4th/3rd row from the end

%s is greedy and there would be no other way to limit `sscanf` to not include `)` on its first variable.

> ... though I'm not sure I understand what that is looking for ...

I think it will help you see what I am trying to achieve if you read at the warning message / testcase
https://lore.kernel.org/git/20210326024005.26962-2-stdedos+git@gmail.com/#iZ30t:t6132-pathspec-exclude.sh

And, to clean up the testcase:
	git log --oneline --format=%s -- ':!(glob)**/file'

I guess it should be now obvious what am I targetting:

If someone naively mixes short and long pathspec magics (`!`, and `(glob)`),
short form takes precedence and ignores long magic / assumes long magic as part of path.

(If it's not obvious, all the more reason to include such warning)

Re: [RFC PATCH v1 1/2] pathspec: warn: long and short forms are incompatible

[Jeff King]

On Fri, Mar 26, 2021 at 06:16:26PM +0200, Stavros Ntentos wrote:

> > We avoid using variable-length arrays in our codebase. ...
> 
> Hear hear, however, I wanted to avoid the "small mess"
> that allocate/free would cause (one or more of ++sloc, labels, if-nesting); ...
> 
> > But two, they are limited in size and the failure mode is not graceful. ...
> ... however, my main issue is that - I don't know what's a sane allocation size.

I don't think a VLA gets you out of knowing the allocation size. Either
way, you are using strlen(entry).

But I do think avoiding allocating altogether is better (as I showed in
my previous response).

> > ... The "4096" is scary here ...
> While scary, it is "a safe" upper high.
> The first time the string ends up in pathspec.c for processing it's here:
> 	entry = argv[i];
> which comes from here
> 	parse_pathspec(pathspec, magic_mask, flags, prefix, parsed_file.v);
> 
> and I don't know what's the maximum size of `parsed_file.v[0]`

There is no reasonable maximum size you can assume. Using 4096 is most
definitely not a safe upper bound.

However, as I said, I don't think it is doing anything useful in the
first place. You have sized the destination buffers as large as the
original string, so they must be large enough to hold any subset of the
original. Dropping them would be equally correct, but less distracting
to a reader.

> > Is this "%4096[^)]" actually valid? I don't think scanf understands
> > regular expressions.
> 
> I was suprised too - but it's not regex.
> 
> see: https://www.cplusplus.com/reference/cstdio/scanf/#parameters - 4th/3rd row from the end

Thanks, this is a corner of scanf I haven't looked at (mostly because
again, we generally avoid scanf in our code base entirely).

> I think it will help you see what I am trying to achieve if you read at the warning message / testcase
> https://lore.kernel.org/git/20210326024005.26962-2-stdedos+git@gmail.com/#iZ30t:t6132-pathspec-exclude.sh
> 
> And, to clean up the testcase:
> 	git log --oneline --format=%s -- ':!(glob)**/file'
> 
> I guess it should be now obvious what am I targetting:
> 
> If someone naively mixes short and long pathspec magics (`!`, and `(glob)`),
> short form takes precedence and ignores long magic / assumes long magic as part of path.
> 
> (If it's not obvious, all the more reason to include such warning)

I understand the overall goal. I am not sure why slashes in the flags
section are a reliable indicator that this mixing is not happening and
we should not show the warning.

It also feels like any checks like this should be relying on the
existing pathspec-magic parser a bit more. I don't know the pathspec
code that well, but surely at some point it has a notion of which parts
are magic flags (e.g., after parse_element_magic in init_pathspec_item).

-Peff

Re: [RFC PATCH v1 1/2] pathspec: warn: long and short forms are incompatible

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> It also feels like any checks like this should be relying on the
> existing pathspec-magic parser a bit more. I don't know the pathspec
> code that well, but surely at some point it has a notion of which parts
> are magic flags (e.g., after parse_element_magic in init_pathspec_item).

Absolutely.  parse_element_magic() decides, by looking at the
character after the initial ':', the we are looking at the longhand
form when magic sequence is introduced by ":(".

Otherwise, : must be followed by shorthand form, where the only
usable ones are '/' (synonym for top), '!' and '^' (synonym for
exclude), and ':' (end of short-form marker), but most importantly,
'(' will *never* be a valid shorthand form (because it is not part
of the GIT_PATHSPEC_MAGIC class of bytes).

So, presumably, inside parse_short_magic(), you could detect '('
and complain it as an attempt to switch to longform.

Here is to illustrate where to hook the check.  With this, I can get
something like this:

    $ git show -s --oneline -- ':!/(foobar)frotz' ':/(bar)nitfol' .
    warning: :!/: cannot mix shortform magic with longform like (exclude)
    warning: :/: cannot mix shortform magic with longform like (exclude)
    84d06cdc06 Sync with v2.31.1

Note that this illustration does not show the best warning message.
To improve it to a bit more useful level, I think two things need to
happen on top.

 * Use advice to allow users sequelch.

 * Show a few letters after '(' (but pay attention to the end of the
   string, which ends with either '\0' or ':'), and lose the
   substring "like (exclude)" from the message.

That would have given

    hint: ":!/(foo...": cannot mix shortform and longform magic

for the above example.

I initially thought it might make it even better if we looked ahead
of what comes after '(', took the substring before ',' or ')', and
looked up pathspec_magic[] array, BUT I do not think it is a good
idea.  It would be confusing if we do not give the same advice to
":!(literol)" when the user does get one for ":!(literal)".  So the
above "two things need to happen" list deliberately excludes an
"improvement" doing so.


 pathspec.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git i/pathspec.c w/pathspec.c
index 18b3be362a..cd343d5b54 100644
--- i/pathspec.c
+++ w/pathspec.c
@@ -336,6 +336,9 @@ static const char *parse_long_magic(unsigned *magic, int *prefix_len,
 	return pos;
 }
 
+static const char mixed_pathspec_magic[] =
+	N_("%.*s: cannot mix shortform magic with longform like (exclude)");
+
 /*
  * Parse the pathspec element looking for short magic
  *
@@ -356,6 +359,16 @@ static const char *parse_short_magic(unsigned *magic, const char *elem)
 			continue;
 		}
 
+		if (ch == '(') {
+			/*
+			 * a possible common mistake: once you started
+			 * a shortform you cannot add (longform), e.g.
+			 * ":!(top)"
+			 */
+			warning(_(mixed_pathspec_magic),
+				(int)(pos-elem), elem);
+		}
+
 		if (!is_pathspec_magic(ch))
 			break;
 

Re: [RFC PATCH v1 1/2] pathspec: warn: long and short forms are incompatible

[Stavros Ntentos]

Junio C Hamano <gitster@pobox.com> writes:
> Jeff King <peff@peff.net> writes:
>
> > It also feels like any checks like this should be relying on the
> > existing pathspec-magic parser a bit more. I don't know the pathspec
> > code that well, but surely at some point it has a notion of which parts
> > are magic flags (e.g., after parse_element_magic in init_pathspec_item).
>
> Absolutely.  parse_element_magic() decides, by looking at the
> character after the initial ':', the we are looking at the longhand
> form when magic sequence is introduced by ":(".

Absolutely from me too! I would want to re-use more of the existing code.
In response to that, patch
https://lore.kernel.org/git/20210328152629.16486-1-133706+stdedos@users.noreply.github.com/
clearly shows that it is possible to re-use the de-facto long form magic parsing
code to attempt to parse the string that continues as a long form magic.

This could theoretically pave the road for mixed short/long form parsing
(in that order), if would be so desired.

However, as probably suspected, it suffers from one big problem:
	if (ARRAY_SIZE(pathspec_magic) <= i)
		die(_("Invalid pathspec magic '%.*s' in '%s'"),
		    (int) len, pos, elem);

which is an unconditional `exit(128)`.

I don't know if it's possible to rescue (or redirect) that `die` call.
In any case, considering that, this again deviates from the
"small informative" change this commit was supposed to be.

Having said that, I have cooked a working patch with the proposed code.

[RFC PATCH v1 2/2] fixup! pathspec: warn: long and short forms are incompatible

[Σταύρος Ντέντος]

From: Stavros Ntentos <133706+stdedos@users.noreply.github.com>

malloc and stuff
---
 pathspec.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/pathspec.c b/pathspec.c
index 374c529569..4ac8bfdc06 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -7,6 +7,7 @@
 #include "attr.h"
 #include "strvec.h"
 #include "quote.h"
+#include "git-compat-util.h"
 
 /*
  * Finds which of the given pathspecs match items in the index.
@@ -745,16 +746,20 @@ int match_pathspec_attrs(const struct index_state *istate,
 }
 
 void check_mishandled_exclude(const char *entry) {
+	char *flags, *path;
 	size_t entry_len = strlen(entry);
-	char flags[entry_len];
-	char path[entry_len];
 
-	if (sscanf(entry, ":!(%4096[^)])%4096s", &flags, &path) != 2) {
-		return;
-	}
-	if (count_slashes(flags) > 0) {
-		return;
+	flags = xstrdup(entry);
+	memset(flags, '\0', entry_len);
+	path = xstrdup(entry);
+	memset(path, '\0', entry_len);
+
+	if (sscanf(entry, ":!(%4096[^)])%4096s", flags, path) == 2) {
+		if (count_slashes(flags) == 0) {
+			warning(_("Pathspec provided matches `:!(...)`\n\tDid you mean `:(exclude,...)`?"));
+		}
 	}
 
-	warning(_("Pathspec provided matches `:!(...)`\n\tDid you mean `:(exclude,...)`?"));
+	FREE_AND_NULL(flags);
+	FREE_AND_NULL(path);
 }
-- 
2.31.0

Re: [RFC PATCH v1 2/2] fixup! pathspec: warn: long and short forms are incompatible

[Bagas Sanjaya]

On 26/03/21 09.40, Σταύρος Ντέντος wrote:
> From: Stavros Ntentos <133706+stdedos@users.noreply.github.com>
> 
> malloc and stuff
> ---
>   pathspec.c | 21 +++++++++++++--------
>   1 file changed, 13 insertions(+), 8 deletions(-)
What does this fixup patch try to accomplish? Give more details.
> diff --git a/pathspec.c b/pathspec.c
> index 374c529569..4ac8bfdc06 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -7,6 +7,7 @@
>   #include "attr.h"
>   #include "strvec.h"
>   #include "quote.h"
> +#include "git-compat-util.h"
>   
>   /*
>    * Finds which of the given pathspecs match items in the index.
> @@ -745,16 +746,20 @@ int match_pathspec_attrs(const struct index_state *istate,
>   }
>   
>   void check_mishandled_exclude(const char *entry) {
> +	char *flags, *path;
>   	size_t entry_len = strlen(entry);
> -	char flags[entry_len];
> -	char path[entry_len];
>   
> -	if (sscanf(entry, ":!(%4096[^)])%4096s", &flags, &path) != 2) {
> -		return;
> -	}
> -	if (count_slashes(flags) > 0) {
> -		return;
> +	flags = xstrdup(entry);
> +	memset(flags, '\0', entry_len);
> +	path = xstrdup(entry);
> +	memset(path, '\0', entry_len);
> +
> +	if (sscanf(entry, ":!(%4096[^)])%4096s", flags, path) == 2) {
> +		if (count_slashes(flags) == 0) {
> +			warning(_("Pathspec provided matches `:!(...)`\n\tDid you mean `:(exclude,...)`?"));
> +		}
>   	}
>   
> -	warning(_("Pathspec provided matches `:!(...)`\n\tDid you mean `:(exclude,...)`?"));
Message looks ok.
> +	FREE_AND_NULL(flags);
> +	FREE_AND_NULL(path);
>   }
> 

-- 
An old man doll... just what I always wanted! - Clara

Re: [RFC PATCH v1 2/2] fixup! pathspec: warn: long and short forms are incompatible

[Stavros Ntentos]

I merely wanted to give visibility on the "resulting" patch
https://lore.kernel.org/git/20210326024411.28615-2-stdedos+git@gmail.com/,
by giving the two versions of the implementation
(as discussed in the https://lore.kernel.org/git/20210326024005.26962-1-stdedos+git@gmail.com/ cover)

[RFC PATCH v1 3/3] squash! fixup! pathspec: warn: long and short forms are incompatible

[Stavros Ntentos]

Attempt to force parsing long magic values to detect if
there is actually long magic present or not.

Signed-off-by: Stavros Ntentos <133706+stdedos@users.noreply.github.com>
---
 pathspec.c                  | 35 +++++++++++++++++++----------------
 pathspec.h                  |  2 +-
 t/t6132-pathspec-exclude.sh |  4 ++--
 3 files changed, 22 insertions(+), 19 deletions(-)

diff --git a/pathspec.c b/pathspec.c
index 857519fda4..447d765112 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -389,9 +389,11 @@ static const char *parse_element_magic(unsigned *magic, int *prefix_len,
 	else if (elem[1] == '(')
 		/* longhand */
 		return parse_long_magic(magic, prefix_len, item, elem);
-	else
+	else {
 		/* shorthand */
+		check_mixed_short_and_long_magic(elem);
 		return parse_short_magic(magic, elem);
+	}
 }
 
 /*
@@ -589,8 +591,6 @@ void parse_pathspec(struct pathspec *pathspec,
 	for (i = 0; i < n; i++) {
 		entry = argv[i];
 
-		check_mishandled_exclude(entry);
-
 		init_pathspec_item(item + i, flags, prefix, prefixlen, entry);
 
 		if (item[i].magic & PATHSPEC_EXCLUDE)
@@ -745,21 +745,24 @@ int match_pathspec_attrs(const struct index_state *istate,
 	return 1;
 }
 
-void check_mishandled_exclude(const char *entry) {
-	char *flags, *path;
-	size_t entry_len = strlen(entry);
+void check_mixed_short_and_long_magic(const char *entry) {
+	const char *parsed_magic;
 
-	flags = xstrdup(entry);
-	memset(flags, '\0', entry_len);
-	path = xstrdup(entry);
-	memset(path, '\0', entry_len);
+	/* skip past stuff we know must be there */
+	if (!skip_prefix(entry, ":", &entry)) {
+		return;
+	}
+
+	/* Throwaway allocations */
+	unsigned magic = 0;
+	int prefix_len = -1;
+	struct pathspec_item *item;
+	item = xmallocz(sizeof(&item));
 
-	if (sscanf(entry, ":!(%4096[^)])%4096s", flags, path) == 2) {
-		if (count_slashes(flags) == 0) {
-			warning(_("Pathspec provided matches `:!(...)`\n\tDid you mean `:(exclude,...)`?"));
-		}
+	parsed_magic = parse_long_magic(&magic, &prefix_len, item, entry);
+	if (entry != parsed_magic) {
+		warning(_("Pathspec provided matches both short and long forms.\nShort forms take presedence over long forms!"));
 	}
 
-	FREE_AND_NULL(flags);
-	FREE_AND_NULL(path);
+	FREE_AND_NULL(item);
 }
diff --git a/pathspec.h b/pathspec.h
index 879d4e82c6..af6c458a06 100644
--- a/pathspec.h
+++ b/pathspec.h
@@ -157,6 +157,6 @@ char *find_pathspecs_matching_against_index(const struct pathspec *pathspec,
 int match_pathspec_attrs(const struct index_state *istate,
 			 const char *name, int namelen,
 			 const struct pathspec_item *item);
-void check_mishandled_exclude(const char* pathspec_entry);
+void check_mixed_short_and_long_magic(const char* pathspec_entry);
 
 #endif /* PATHSPEC_H */
diff --git a/t/t6132-pathspec-exclude.sh b/t/t6132-pathspec-exclude.sh
index b32ddb2a56..a1580a89ae 100755
--- a/t/t6132-pathspec-exclude.sh
+++ b/t/t6132-pathspec-exclude.sh
@@ -245,7 +245,7 @@ test_expect_success 'grep --untracked PATTERN :(exclude)*FILE' '
 '
 
 cat > expected_warn <<"EOF"
-Pathspec provided matches `:!(...)`
+Pathspec provided matches both short and long forms.
 EOF
 test_expect_success 'warn pathspec :!(...) skips the parenthesized magics' '
 	git log --oneline --format=%s -- '"'"':!(glob)**/file'"'"' >actual 2>warn &&
@@ -263,7 +263,7 @@ EOF
 	grep -Ff expected_warn warn
 '
 
-test_expect_success 'do not warn that pathspec :!(...) skips the parenthesized magics (if parenthesis would not be part of the magic)' '
+test_expect_success 'do not warn that pathspec :!(...) skips the parenthesized magics (if parenthesized text would not be magic)' '
 	git log --oneline --format=%s -- '"'"':!(gl/ob)/file'"'"' >actual 2>warn &&
 	cat <<EOF >expect &&
 sub2/file
-- 
2.31.0

[RFC PATCH v1 0/1] pathspec: warn: long and short forms are incompatible

[Σταύρος Ντέντος]

From: Stavros Ntentos <133706+stdedos@users.noreply.github.com>

The second of the two issues identified in this thread.

Both patches work as expected.

There is a short solution, which is mostly okay.
It does not compile with -Werror, but:
It avoids malloc/free, and keeps the logic/sloc low.

All of this could be my lack of C experience.

Stavros Ntentos (1):
  pathspec: warn: long and short forms are incompatible

 pathspec.c                  | 30 ++++++++++++++++++++++++++++++
 pathspec.h                  |  1 +
 t/t6132-pathspec-exclude.sh | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 64 insertions(+)

--
2.31.0

[RFC PATCH v1] pathspec: warn: long and short forms are incompatible

[Σταύρος Ντέντος]

From: Stavros Ntentos <133706+stdedos@users.noreply.github.com>

Namely, `!` and any other long magic form (e.g. `glob`)
cannot be combined to one entry.

Issue a warning when such thing happens, and hint to the solution.

Signed-off-by: Stavros Ntentos <133706+stdedos@users.noreply.github.com>
---
 pathspec.c                  | 24 ++++++++++++++++++++++++
 pathspec.h                  |  1 +
 t/t6132-pathspec-exclude.sh | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 58 insertions(+)

diff --git a/pathspec.c b/pathspec.c
index 7a229d8d22..4ac8bfdc06 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -1,3 +1,5 @@
+#include <stdio.h>
+#include <string.h>
 #include "cache.h"
 #include "config.h"
 #include "dir.h"
@@ -5,6 +7,7 @@
 #include "attr.h"
 #include "strvec.h"
 #include "quote.h"
+#include "git-compat-util.h"
 
 /*
  * Finds which of the given pathspecs match items in the index.
@@ -586,6 +589,8 @@ void parse_pathspec(struct pathspec *pathspec,
 	for (i = 0; i < n; i++) {
 		entry = argv[i];
 
+		check_mishandled_exclude(entry);
+
 		init_pathspec_item(item + i, flags, prefix, prefixlen, entry);
 
 		if (item[i].magic & PATHSPEC_EXCLUDE)
@@ -739,3 +744,22 @@ int match_pathspec_attrs(const struct index_state *istate,
 
 	return 1;
 }
+
+void check_mishandled_exclude(const char *entry) {
+	char *flags, *path;
+	size_t entry_len = strlen(entry);
+
+	flags = xstrdup(entry);
+	memset(flags, '\0', entry_len);
+	path = xstrdup(entry);
+	memset(path, '\0', entry_len);
+
+	if (sscanf(entry, ":!(%4096[^)])%4096s", flags, path) == 2) {
+		if (count_slashes(flags) == 0) {
+			warning(_("Pathspec provided matches `:!(...)`\n\tDid you mean `:(exclude,...)`?"));
+		}
+	}
+
+	FREE_AND_NULL(flags);
+	FREE_AND_NULL(path);
+}
diff --git a/pathspec.h b/pathspec.h
index 454ce364fa..879d4e82c6 100644
--- a/pathspec.h
+++ b/pathspec.h
@@ -157,5 +157,6 @@ char *find_pathspecs_matching_against_index(const struct pathspec *pathspec,
 int match_pathspec_attrs(const struct index_state *istate,
 			 const char *name, int namelen,
 			 const struct pathspec_item *item);
+void check_mishandled_exclude(const char* pathspec_entry);
 
 #endif /* PATHSPEC_H */
diff --git a/t/t6132-pathspec-exclude.sh b/t/t6132-pathspec-exclude.sh
index 30328b87f0..b32ddb2a56 100755
--- a/t/t6132-pathspec-exclude.sh
+++ b/t/t6132-pathspec-exclude.sh
@@ -244,4 +244,37 @@ test_expect_success 'grep --untracked PATTERN :(exclude)*FILE' '
 	test_cmp expect-grep actual-grep
 '
 
+cat > expected_warn <<"EOF"
+Pathspec provided matches `:!(...)`
+EOF
+test_expect_success 'warn pathspec :!(...) skips the parenthesized magics' '
+	git log --oneline --format=%s -- '"'"':!(glob)**/file'"'"' >actual 2>warn &&
+	cat <<EOF >expect &&
+sub2/file
+sub/sub/sub/file
+sub/file2
+sub/sub/file
+sub/file
+file
+EOF
+	cat actual &&
+	cat warn &&
+	test_cmp expect actual &&
+	grep -Ff expected_warn warn
+'
+
+test_expect_success 'do not warn that pathspec :!(...) skips the parenthesized magics (if parenthesis would not be part of the magic)' '
+	git log --oneline --format=%s -- '"'"':!(gl/ob)/file'"'"' >actual 2>warn &&
+	cat <<EOF >expect &&
+sub2/file
+sub/sub/sub/file
+sub/file2
+sub/sub/file
+sub/file
+file
+EOF
+	test_cmp expect actual &&
+	! grep -Ff expected_warn warn
+'
+
 test_done
-- 
2.31.0

[PATCH v3] pathspec: advice: long and short forms are incompatible

[Stavros Ntentos]

It can be a "reasonable" mistake to mix short and long forms,
e.g. `:!(glob)`, instead of the (correct) `:(exclude,glob)`.

Teach git to issue an advice when such a pathspec is given.
        i.e.: While in short form parsing:
        * if the string contains an open parenthesis [`(`], and
        * without having explicitly terminated magic parsing (`:`)
        issue an advice hinting to that fact.

Based on "Junio C Hamano <gitster@pobox.com>"'s code suggestions:
* https://lore.kernel.org/git/xmqqa6qoqw9n.fsf@gitster.g/
* https://lore.kernel.org/git/xmqqo8f0cr9z.fsf@gitster.g/

Signed-off-by: Stavros Ntentos <133706+stdedos@users.noreply.github.com>
---
 Documentation/config/advice.txt |  3 +++
 advice.c                        |  3 +++
 advice.h                        |  2 ++
 pathspec.c                      | 33 ++++++++++++++++++++++++++++
 t/t6132-pathspec-exclude.sh     | 38 +++++++++++++++++++++++++++++++++
 5 files changed, 79 insertions(+)

diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index acbd0c09aa..05a3cbc164 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -119,4 +119,7 @@ advice.*::
 	addEmptyPathspec::
 		Advice shown if a user runs the add command without providing
 		the pathspec parameter.
+	mixedPathspecMagic::
+		Advice shown if a user tries to mix short- and
+		longform pathspec magic.
 --
diff --git a/advice.c b/advice.c
index 164742305f..f94505e0d6 100644
--- a/advice.c
+++ b/advice.c
@@ -33,6 +33,7 @@ int advice_checkout_ambiguous_remote_branch_name = 1;
 int advice_submodule_alternate_error_strategy_die = 1;
 int advice_add_ignored_file = 1;
 int advice_add_empty_pathspec = 1;
+int advice_mixed_pathspec_magic = 1;
 
 static int advice_use_color = -1;
 static char advice_colors[][COLOR_MAXLEN] = {
@@ -95,6 +96,7 @@ static struct {
 	{ "submoduleAlternateErrorStrategyDie", &advice_submodule_alternate_error_strategy_die },
 	{ "addIgnoredFile", &advice_add_ignored_file },
 	{ "addEmptyPathspec", &advice_add_empty_pathspec },
+	{ "mixedPathspecMagic", &advice_mixed_pathspec_magic },
 
 	/* make this an alias for backward compatibility */
 	{ "pushNonFastForward", &advice_push_update_rejected }
@@ -137,6 +139,7 @@ static struct {
 	[ADVICE_STATUS_U_OPTION]			= { "statusUoption", 1 },
 	[ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = { "submoduleAlternateErrorStrategyDie", 1 },
 	[ADVICE_WAITING_FOR_EDITOR]			= { "waitingForEditor", 1 },
+	[ADVICE_MIXED_PATHSPEC_MAGIC]	= { "mixedPathspecMagic", 1 },
 };
 
 static const char turn_off_instructions[] =
diff --git a/advice.h b/advice.h
index bc2432980a..aa229fded8 100644
--- a/advice.h
+++ b/advice.h
@@ -33,6 +33,7 @@ extern int advice_checkout_ambiguous_remote_branch_name;
 extern int advice_submodule_alternate_error_strategy_die;
 extern int advice_add_ignored_file;
 extern int advice_add_empty_pathspec;
+extern int advice_mixed_pathspec_magic;
 
 /*
  * To add a new advice, you need to:
@@ -72,6 +73,7 @@ extern int advice_add_empty_pathspec;
 	ADVICE_STATUS_U_OPTION,
 	ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE,
 	ADVICE_WAITING_FOR_EDITOR,
+	ADVICE_MIXED_PATHSPEC_MAGIC,
 };
 
 int git_default_advice_config(const char *var, const char *value);
diff --git a/pathspec.c b/pathspec.c
index 18b3be362a..5f77e2e8d8 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -336,6 +336,36 @@ static const char *parse_long_magic(unsigned *magic, int *prefix_len,
 	return pos;
 }
 
+
+/*
+ * Give hint for a common mistake of mixing short and long
+ * form of pathspec magic, as well as possible corrections
+ */
+static void warn_mixed_magic(unsigned magic, const char *elem, const char *pos)
+{
+	struct strbuf longform = STRBUF_INIT;
+	int i;
+
+	if (!advice_enabled(ADVICE_MIXED_PATHSPEC_MAGIC))
+		return;
+
+	for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
+		if (pathspec_magic[i].bit & magic) {
+			if (longform.len)
+				strbuf_addch(&longform, ',');
+			strbuf_addstr(&longform, pathspec_magic[i].name);
+		}
+	}
+	advise_if_enabled(ADVICE_MIXED_PATHSPEC_MAGIC,
+	                  _("'%.*s(...': cannot mix short- and longform pathspec magic!\n"
+	                    "Either spell the shortform magic '%.*s' as ':(%s,...'\n"
+	                    "or end magic pathspec matching with '%.*s:'."),
+	                  (int)(pos - elem), elem,
+	                  (int)(pos - (elem + 1)), elem + 1,
+	                  longform.buf,
+	                  (int)(pos - elem), elem);
+}
+
 /*
  * Parse the pathspec element looking for short magic
  *
@@ -356,6 +386,9 @@ static const char *parse_short_magic(unsigned *magic, const char *elem)
 			continue;
 		}
 
+		if (ch == '(')
+			warn_mixed_magic(*magic, elem, pos);
+
 		if (!is_pathspec_magic(ch))
 			break;
 
diff --git a/t/t6132-pathspec-exclude.sh b/t/t6132-pathspec-exclude.sh
index 30328b87f0..8b9d543e1e 100755
--- a/t/t6132-pathspec-exclude.sh
+++ b/t/t6132-pathspec-exclude.sh
@@ -244,4 +244,42 @@ test_expect_success 'grep --untracked PATTERN :(exclude)*FILE' '
 	test_cmp expect-grep actual-grep
 '
 
+cat > expected_warn <<"EOF"
+hint: ':!(...': cannot mix short- and longform pathspec magic!
+hint: Either spell the shortform magic '!' as ':(exclude,...'
+hint: or end magic pathspec matching with ':!:'.
+hint: Disable this message with "git config advice.mixedPathspecMagic false"
+EOF
+test_expect_success 'warn pathspec not matching longform magic in :!(...)' '
+	git log --oneline --format=%s -- '"'"':!(glob)**/file'"'"' >actual 2>warn &&
+	cat <<EOF >expect &&
+sub2/file
+sub/sub/sub/file
+sub/file2
+sub/sub/file
+sub/file
+file
+EOF
+	cat actual &&
+	cat warn &&
+	test_cmp expect actual &&
+	test_cmp expected_warn warn
+'
+
+test_expect_success 'do not warn pathspec not matching longform magic in :!:(...) (i.e. if magic parsing is explicitly stopped)' '
+	git log --oneline --format=%s -- '"'"':!:(glob)**/file'"'"' >actual 2>warn &&
+	cat <<EOF >expect &&
+sub2/file
+sub/sub/sub/file
+sub/file2
+sub/sub/file
+sub/file
+file
+EOF
+	cat actual &&
+	cat warn &&
+	test_cmp expect actual &&
+	! test_cmp expected_warn warn
+'
+
 test_done
-- 
2.31.1

Re: [PATCH v3] pathspec: advice: long and short forms are incompatible

[Junio C Hamano]

Stavros Ntentos <stdedos@gmail.com> writes:

> It can be a "reasonable" mistake to mix short and long forms,
> e.g. `:!(glob)`, instead of the (correct) `:(exclude,glob)`.

The word "form" may be sufficient for today's us because we have
been so focused on this particular pathspec magic issue.  

But reviewers are not, and those who read the "git log" output later
are not, either.  Let's be friendly and helpful to them by saying
"form of pathspec magic" or somesuch.

The same comment applies to the patch title.

It also might be more friendly to readers what the mistaken form
would do, too.

Here is my attempt, taking all of the above into account.

    It is a hard-to-notice mistake to try mixing short and long
    forms of pathspec magic, e.g. instead of ':(exclude,glob)', it
    may be tempting to write ':!(glob)', which stops at ":!",
    i.e. the end of the short-form pathspec magic, and the "(glob)"
    is taken as the beginning part of the pathspec, wanting to match
    a file or a directory whose name begins with that literal string.

> Teach git to issue an advice when such a pathspec is given.
>         i.e.: While in short form parsing:
>         * if the string contains an open parenthesis [`(`], and
>         * without having explicitly terminated magic parsing (`:`)
>         issue an advice hinting to that fact.

OK.

> Based on "Junio C Hamano <gitster@pobox.com>"'s code suggestions:
> * https://lore.kernel.org/git/xmqqa6qoqw9n.fsf@gitster.g/
> * https://lore.kernel.org/git/xmqqo8f0cr9z.fsf@gitster.g/

It is sufficient to just write a single line

	Helped-by: Junio C Hamano <gitster@pobox.com>

immediately before your sign-off, I would think.

> Signed-off-by: Stavros Ntentos <133706+stdedos@users.noreply.github.com>

The sign-off name and address must match the name and address of the
patch author (i.e. "Stavros Ntentos <stdedos@gmail.com>").

> +	mixedPathspecMagic::
> +		Advice shown if a user tries to mix short- and
> +		longform pathspec magic.

Good.  Here the phrase "pathspec magic" is used.

> +/*
> + * Give hint for a common mistake of mixing short and long
> + * form of pathspec magic, as well as possible corrections
> + */
> +static void warn_mixed_magic(unsigned magic, const char *elem, const char *pos)
> +{
> +	struct strbuf longform = STRBUF_INIT;
> +	int i;
> +
> +	if (!advice_enabled(ADVICE_MIXED_PATHSPEC_MAGIC))
> +		return;
> +
> +	for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
> +		if (pathspec_magic[i].bit & magic) {
> +			if (longform.len)
> +				strbuf_addch(&longform, ',');
> +			strbuf_addstr(&longform, pathspec_magic[i].name);
> +		}
> +	}

OK, so we are collecting the ones given by the short form so far.
For e.g. ":!(glob)", we write "exclude" in the longform buffer.  If
we had more than one, then before adding the second one, we add a
comma, so we may see "top,exclude" for ":/!(glob)".  Good.

> +	advise_if_enabled(ADVICE_MIXED_PATHSPEC_MAGIC,
> +	                  _("'%.*s(...': cannot mix short- and longform pathspec magic!\n"

Do we need to "shout!"?  I think a normal full-stop would be sufficient.

'elem' is pointing at ':', 'pos' is where we read '(' from, so
the above gives us "':/!(...': cannot..." for "':/!(glob)".  OK.

> +	                    "Either spell the shortform magic '%.*s' as ':(%s,...'\n"

Here, the shortform %.*s excludes the ':' that introduced the magic,
so we would see "shortform magic '/!' as ':(top,exclude,...'".  Good.

> +	                    "or end magic pathspec matching with '%.*s:'."),

This one I am not sure about.  Something like

    $ git add -- ":!(0) preface.txt" \*.txt

may be plausible, albeit rare, and it may be a good advice to
explicitly terminate the shortform pathspec magic before the '(' in
such a case.

But presumably it is much rarer for '(' to be a part of a pathspec
element than an attempt to introduce a longform magic, it might be
worth spending an extra line to explain in what narrow cases the
latter choice may make sense.  Here is my attempt.

    or if '(...' is indeed the beginning of a pathname, end the shortform
    magic sequence explicitly with another ':' before it, e.g. '%.*s:(...'


> +	                  (int)(pos - elem), elem,
> +	                  (int)(pos - (elem + 1)), elem + 1,
> +	                  longform.buf,
> +	                  (int)(pos - elem), elem);
> +}
>  /*
>   * Parse the pathspec element looking for short magic
>   *
> @@ -356,6 +386,9 @@ static const char *parse_short_magic(unsigned *magic, const char *elem)
>  			continue;
>  		}
>  
> +		if (ch == '(')
> +			warn_mixed_magic(*magic, elem, pos);
> +
>  		if (!is_pathspec_magic(ch))
>  			break;
>  
> diff --git a/t/t6132-pathspec-exclude.sh b/t/t6132-pathspec-exclude.sh
> index 30328b87f0..8b9d543e1e 100755
> --- a/t/t6132-pathspec-exclude.sh
> +++ b/t/t6132-pathspec-exclude.sh
> @@ -244,4 +244,42 @@ test_expect_success 'grep --untracked PATTERN :(exclude)*FILE' '
>  	test_cmp expect-grep actual-grep
>  '
>  
> +cat > expected_warn <<"EOF"
> +hint: ':!(...': cannot mix short- and longform pathspec magic!
> +hint: Either spell the shortform magic '!' as ':(exclude,...'
> +hint: or end magic pathspec matching with ':!:'.
> +hint: Disable this message with "git config advice.mixedPathspecMagic false"
> +EOF
> +test_expect_success 'warn pathspec not matching longform magic in :!(...)' '
> +	git log --oneline --format=%s -- '"'"':!(glob)**/file'"'"' >actual 2>warn &&
> +	cat <<EOF >expect &&
> +sub2/file
> +sub/sub/sub/file
> +sub/file2
> +sub/sub/file
> +sub/file
> +file
> +EOF
> +	cat actual &&
> +	cat warn &&

Are these leftover debugging aid?

> +	test_cmp expect actual &&
> +	test_cmp expected_warn warn
> +'
> +
> +test_expect_success 'do not warn pathspec not matching longform magic in :!:(...) (i.e. if magic parsing is explicitly stopped)' '
> +	git log --oneline --format=%s -- '"'"':!:(glob)**/file'"'"' >actual 2>warn &&
> +	cat <<EOF >expect &&
> +sub2/file
> +sub/sub/sub/file
> +sub/file2
> +sub/sub/file
> +sub/file
> +file
> +EOF
> +	cat actual &&
> +	cat warn &&

Are these leftover debugging aid?

> +	test_cmp expect actual &&
> +	! test_cmp expected_warn warn
> +'
> +
>  test_done


Thanks.

Re: [PATCH v3] pathspec: advice: long and short forms are incompatible

[Σταύρος Ντέντος]

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

Hello there,

Implemented the last fixes

> > Signed-off-by: Stavros Ntentos <133706+stdedos@users.noreply.github.com>
>
> The sign-off name and address must match the name and address of the
> patch author (i.e. "Stavros Ntentos <stdedos@gmail.com>").

The author *is* Stavros Ntentos <133706+stdedos@users.noreply.github.com>;
I don't know why it is messed up. Maybe if I send the patch as an
attachment instead.

With regards,
Ntentos Stavros

On Sun, 4 Apr 2021 at 10:19, Junio C Hamano <gitster@pobox.com> wrote:
>
> Stavros Ntentos <stdedos@gmail.com> writes:
>
> > It can be a "reasonable" mistake to mix short and long forms,
> > e.g. `:!(glob)`, instead of the (correct) `:(exclude,glob)`.
>
> The word "form" may be sufficient for today's us because we have
> been so focused on this particular pathspec magic issue.
>
> But reviewers are not, and those who read the "git log" output later
> are not, either.  Let's be friendly and helpful to them by saying
> "form of pathspec magic" or somesuch.
>
> The same comment applies to the patch title.
>
> It also might be more friendly to readers what the mistaken form
> would do, too.
>
> Here is my attempt, taking all of the above into account.
>
>     It is a hard-to-notice mistake to try mixing short and long
>     forms of pathspec magic, e.g. instead of ':(exclude,glob)', it
>     may be tempting to write ':!(glob)', which stops at ":!",
>     i.e. the end of the short-form pathspec magic, and the "(glob)"
>     is taken as the beginning part of the pathspec, wanting to match
>     a file or a directory whose name begins with that literal string.
>
> > Teach git to issue an advice when such a pathspec is given.
> >         i.e.: While in short form parsing:
> >         * if the string contains an open parenthesis [`(`], and
> >         * without having explicitly terminated magic parsing (`:`)
> >         issue an advice hinting to that fact.
>
> OK.
>
> > Based on "Junio C Hamano <gitster@pobox.com>"'s code suggestions:
> > * https://lore.kernel.org/git/xmqqa6qoqw9n.fsf@gitster.g/
> > * https://lore.kernel.org/git/xmqqo8f0cr9z.fsf@gitster.g/
>
> It is sufficient to just write a single line
>
>         Helped-by: Junio C Hamano <gitster@pobox.com>
>
> immediately before your sign-off, I would think.
>
> > Signed-off-by: Stavros Ntentos <133706+stdedos@users.noreply.github.com>
>
> The sign-off name and address must match the name and address of the
> patch author (i.e. "Stavros Ntentos <stdedos@gmail.com>").
>
> > +     mixedPathspecMagic::
> > +             Advice shown if a user tries to mix short- and
> > +             longform pathspec magic.
>
> Good.  Here the phrase "pathspec magic" is used.
>
> > +/*
> > + * Give hint for a common mistake of mixing short and long
> > + * form of pathspec magic, as well as possible corrections
> > + */
> > +static void warn_mixed_magic(unsigned magic, const char *elem, const char *pos)
> > +{
> > +     struct strbuf longform = STRBUF_INIT;
> > +     int i;
> > +
> > +     if (!advice_enabled(ADVICE_MIXED_PATHSPEC_MAGIC))
> > +             return;
> > +
> > +     for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
> > +             if (pathspec_magic[i].bit & magic) {
> > +                     if (longform.len)
> > +                             strbuf_addch(&longform, ',');
> > +                     strbuf_addstr(&longform, pathspec_magic[i].name);
> > +             }
> > +     }
>
> OK, so we are collecting the ones given by the short form so far.
> For e.g. ":!(glob)", we write "exclude" in the longform buffer.  If
> we had more than one, then before adding the second one, we add a
> comma, so we may see "top,exclude" for ":/!(glob)".  Good.
>
> > +     advise_if_enabled(ADVICE_MIXED_PATHSPEC_MAGIC,
> > +                       _("'%.*s(...': cannot mix short- and longform pathspec magic!\n"
>
> Do we need to "shout!"?  I think a normal full-stop would be sufficient.
>
> 'elem' is pointing at ':', 'pos' is where we read '(' from, so
> the above gives us "':/!(...': cannot..." for "':/!(glob)".  OK.
>
> > +                         "Either spell the shortform magic '%.*s' as ':(%s,...'\n"
>
> Here, the shortform %.*s excludes the ':' that introduced the magic,
> so we would see "shortform magic '/!' as ':(top,exclude,...'".  Good.
>
> > +                         "or end magic pathspec matching with '%.*s:'."),
>
> This one I am not sure about.  Something like
>
>     $ git add -- ":!(0) preface.txt" \*.txt
>
> may be plausible, albeit rare, and it may be a good advice to
> explicitly terminate the shortform pathspec magic before the '(' in
> such a case.
>
> But presumably it is much rarer for '(' to be a part of a pathspec
> element than an attempt to introduce a longform magic, it might be
> worth spending an extra line to explain in what narrow cases the
> latter choice may make sense.  Here is my attempt.
>
>     or if '(...' is indeed the beginning of a pathname, end the shortform
>     magic sequence explicitly with another ':' before it, e.g. '%.*s:(...'
>
>
> > +                       (int)(pos - elem), elem,
> > +                       (int)(pos - (elem + 1)), elem + 1,
> > +                       longform.buf,
> > +                       (int)(pos - elem), elem);
> > +}
> >  /*
> >   * Parse the pathspec element looking for short magic
> >   *
> > @@ -356,6 +386,9 @@ static const char *parse_short_magic(unsigned *magic, const char *elem)
> >                       continue;
> >               }
> >
> > +             if (ch == '(')
> > +                     warn_mixed_magic(*magic, elem, pos);
> > +
> >               if (!is_pathspec_magic(ch))
> >                       break;
> >
> > diff --git a/t/t6132-pathspec-exclude.sh b/t/t6132-pathspec-exclude.sh
> > index 30328b87f0..8b9d543e1e 100755
> > --- a/t/t6132-pathspec-exclude.sh
> > +++ b/t/t6132-pathspec-exclude.sh
> > @@ -244,4 +244,42 @@ test_expect_success 'grep --untracked PATTERN :(exclude)*FILE' '
> >       test_cmp expect-grep actual-grep
> >  '
> >
> > +cat > expected_warn <<"EOF"
> > +hint: ':!(...': cannot mix short- and longform pathspec magic!
> > +hint: Either spell the shortform magic '!' as ':(exclude,...'
> > +hint: or end magic pathspec matching with ':!:'.
> > +hint: Disable this message with "git config advice.mixedPathspecMagic false"
> > +EOF
> > +test_expect_success 'warn pathspec not matching longform magic in :!(...)' '
> > +     git log --oneline --format=%s -- '"'"':!(glob)**/file'"'"' >actual 2>warn &&
> > +     cat <<EOF >expect &&
> > +sub2/file
> > +sub/sub/sub/file
> > +sub/file2
> > +sub/sub/file
> > +sub/file
> > +file
> > +EOF
> > +     cat actual &&
> > +     cat warn &&
>
> Are these leftover debugging aid?
>
> > +     test_cmp expect actual &&
> > +     test_cmp expected_warn warn
> > +'
> > +
> > +test_expect_success 'do not warn pathspec not matching longform magic in :!:(...) (i.e. if magic parsing is explicitly stopped)' '
> > +     git log --oneline --format=%s -- '"'"':!:(glob)**/file'"'"' >actual 2>warn &&
> > +     cat <<EOF >expect &&
> > +sub2/file
> > +sub/sub/sub/file
> > +sub/file2
> > +sub/sub/file
> > +sub/file
> > +file
> > +EOF
> > +     cat actual &&
> > +     cat warn &&
>
> Are these leftover debugging aid?
>
> > +     test_cmp expect actual &&
> > +     ! test_cmp expected_warn warn
> > +'
> > +
> >  test_done
>
>
> Thanks.

[-- Attachment #2: v4-0001-pathspec-advice-long-and-short-form-of-pathspec-m.patch --]
[-- Type: text/x-patch, Size: 6661 bytes --]

From edb4af00979124c8cc1674a5ad76f9634f74d42e Mon Sep 17 00:00:00 2001
From: Stavros Ntentos <133706+stdedos@users.noreply.github.com>
Date: Sun, 28 Mar 2021 15:53:50 +0300
Subject: [PATCH v4] pathspec: advice: long and short form of pathspec magic
 are incompatible

It is a hard-to-notice mistake to try mixing short and long
forms of pathspec magic, e.g. instead of ':(exclude,glob)', it
may be tempting to write ':!(glob)'.
The end of the short-form pathspec magic (`:!`), and the `(glob)`
is taken as the beginning part of the pathspec, wanting to match
a file or a directory whose name begins with that literal string.

Teach git to issue an advice when such a pathspec is given.
        i.e.: While in short form parsing:
        * if the parsing is not explicitly terminated with `:`, and
        * if the string contains an open parenthesis [`(`]
        issue an advice hinting to that fact.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stavros Ntentos <133706+stdedos@users.noreply.github.com>
---
 Documentation/config/advice.txt |  3 +++
 advice.c                        |  3 +++
 advice.h                        |  2 ++
 pathspec.c                      | 34 ++++++++++++++++++++++++++++++++
 t/t6132-pathspec-exclude.sh     | 35 +++++++++++++++++++++++++++++++++
 5 files changed, 77 insertions(+)

diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index acbd0c09aa..05a3cbc164 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -119,4 +119,7 @@ advice.*::
 	addEmptyPathspec::
 		Advice shown if a user runs the add command without providing
 		the pathspec parameter.
+	mixedPathspecMagic::
+		Advice shown if a user tries to mix short- and
+		longform pathspec magic.
 --
diff --git a/advice.c b/advice.c
index 164742305f..b1955966d5 100644
--- a/advice.c
+++ b/advice.c
@@ -33,6 +33,7 @@ int advice_checkout_ambiguous_remote_branch_name = 1;
 int advice_submodule_alternate_error_strategy_die = 1;
 int advice_add_ignored_file = 1;
 int advice_add_empty_pathspec = 1;
+int advice_mixed_pathspec_magic = 1;
 
 static int advice_use_color = -1;
 static char advice_colors[][COLOR_MAXLEN] = {
@@ -95,6 +96,7 @@ static struct {
 	{ "submoduleAlternateErrorStrategyDie", &advice_submodule_alternate_error_strategy_die },
 	{ "addIgnoredFile", &advice_add_ignored_file },
 	{ "addEmptyPathspec", &advice_add_empty_pathspec },
+	{ "mixedPathspecMagic", &advice_mixed_pathspec_magic },
 
 	/* make this an alias for backward compatibility */
 	{ "pushNonFastForward", &advice_push_update_rejected }
@@ -137,6 +139,7 @@ static struct {
 	[ADVICE_STATUS_U_OPTION]			= { "statusUoption", 1 },
 	[ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = { "submoduleAlternateErrorStrategyDie", 1 },
 	[ADVICE_WAITING_FOR_EDITOR]			= { "waitingForEditor", 1 },
+	[ADVICE_MIXED_PATHSPEC_MAGIC]			= { "mixedPathspecMagic", 1 },
 };
 
 static const char turn_off_instructions[] =
diff --git a/advice.h b/advice.h
index bc2432980a..aa229fded8 100644
--- a/advice.h
+++ b/advice.h
@@ -33,6 +33,7 @@ extern int advice_checkout_ambiguous_remote_branch_name;
 extern int advice_submodule_alternate_error_strategy_die;
 extern int advice_add_ignored_file;
 extern int advice_add_empty_pathspec;
+extern int advice_mixed_pathspec_magic;
 
 /*
  * To add a new advice, you need to:
@@ -72,6 +73,7 @@ extern int advice_add_empty_pathspec;
 	ADVICE_STATUS_U_OPTION,
 	ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE,
 	ADVICE_WAITING_FOR_EDITOR,
+	ADVICE_MIXED_PATHSPEC_MAGIC,
 };
 
 int git_default_advice_config(const char *var, const char *value);
diff --git a/pathspec.c b/pathspec.c
index 18b3be362a..da560ac7ad 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -336,6 +336,37 @@ static const char *parse_long_magic(unsigned *magic, int *prefix_len,
 	return pos;
 }
 
+
+/*
+ * Give hint for a common mistake of mixing short and long
+ * form of pathspec magic, as well as possible corrections
+ */
+static void warn_mixed_magic(unsigned magic, const char *elem, const char *pos)
+{
+	struct strbuf longform = STRBUF_INIT;
+	int i;
+
+	if (!advice_enabled(ADVICE_MIXED_PATHSPEC_MAGIC))
+		return;
+
+	for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
+		if (pathspec_magic[i].bit & magic) {
+			if (longform.len)
+				strbuf_addch(&longform, ',');
+			strbuf_addstr(&longform, pathspec_magic[i].name);
+		}
+	}
+	advise_if_enabled(ADVICE_MIXED_PATHSPEC_MAGIC,
+	                  _("'%.*s(...': cannot mix short- and longform pathspec magic.\n"
+	                    "Either spell the shortform magic '%.*s' as ':(%s,...',\n"
+	                    "or if '(...' is indeed the beginning of a pathname, end the shortform\n"
+	                    "magic sequence explicitly with another ':' before it, e.g. '%.*s:(...'."),
+	                  (int)(pos - elem), elem,
+	                  (int)(pos - (elem + 1)), elem + 1,
+	                  longform.buf,
+	                  (int)(pos - elem), elem);
+}
+
 /*
  * Parse the pathspec element looking for short magic
  *
@@ -356,6 +387,9 @@ static const char *parse_short_magic(unsigned *magic, const char *elem)
 			continue;
 		}
 
+		if (ch == '(')
+			warn_mixed_magic(*magic, elem, pos);
+
 		if (!is_pathspec_magic(ch))
 			break;
 
diff --git a/t/t6132-pathspec-exclude.sh b/t/t6132-pathspec-exclude.sh
index 30328b87f0..e39b068686 100755
--- a/t/t6132-pathspec-exclude.sh
+++ b/t/t6132-pathspec-exclude.sh
@@ -244,4 +244,39 @@ test_expect_success 'grep --untracked PATTERN :(exclude)*FILE' '
 	test_cmp expect-grep actual-grep
 '
 
+cat > expected_warn <<"EOF"
+hint: ':!(...': cannot mix short- and longform pathspec magic.
+hint: Either spell the shortform magic '!' as ':(exclude,...',
+hint: or if '(...' is indeed the beginning of a pathname, end the shortform
+hint: magic sequence explicitly with another ':' before it, e.g. ':!:(...'.
+hint: Disable this message with "git config advice.mixedPathspecMagic false"
+EOF
+test_expect_success 'warn pathspec not matching longform magic in :!(...)' '
+	git log --oneline --format=%s -- '"'"':!(glob)**/file'"'"' >actual 2>warn &&
+	cat <<EOF >expect &&
+sub2/file
+sub/sub/sub/file
+sub/file2
+sub/sub/file
+sub/file
+file
+EOF
+	test_cmp expect actual &&
+	test_cmp expected_warn warn
+'
+
+test_expect_success 'do not warn pathspec not matching longform magic in :!:(...) (i.e. if magic parsing is explicitly stopped)' '
+	git log --oneline --format=%s -- '"'"':!:(glob)**/file'"'"' >actual 2>warn &&
+	cat <<EOF >expect &&
+sub2/file
+sub/sub/sub/file
+sub/file2
+sub/sub/file
+sub/file
+file
+EOF
+	test_cmp expect actual &&
+	! test_cmp expected_warn warn
+'
+
 test_done
-- 
2.31.1

Re: [PATCH v3] pathspec: advice: long and short forms are incompatible

[Junio C Hamano]

Σταύρος Ντέντος  <stdedos@gmail.com> writes:

> The author *is* Stavros Ntentos <133706+stdedos@users.noreply.github.com>;
> I don't know why it is messed up.

Hmph, that is unfortunate.  As authorship and sign-off is a part of
the mechanism we use to keep track of provenance of our codebase,
we'd prefer to be talking to somebody whom we can reach when needed;
the noreply address apparently is designed to be unreachable, no?

Re: [PATCH v3] pathspec: advice: long and short forms are incompatible

[Σταύρος Ντέντος]

On Sun, 11 Apr 2021 at 22:10, Junio C Hamano <gitster@pobox.com> wrote:
>
> Hmph, that is unfortunate.  As authorship and sign-off is a part of
> the mechanism we use to keep track of provenance of our codebase,
> we'd prefer to be talking to somebody whom we can reach when needed;
> the noreply address apparently is designed to be unreachable, no?
>

I would like to "just" contribute, if possible.
If someone *really* needs to track me down, I assume the mailing list
is anyway already public,
and the patchset title already will directly link to the thread.

With regards,
Ntentos Stavros

[PATCH v2] pathspec: advice: long and short forms are incompatible

[Stavros Ntentos]

It can be a "reasonable" mistake to mix short and long forms,
e.g. `:!(glob)`, instead of the (correct) `:(exclude,glob)`.

Teach git to issue an advice when such a pathspec is given.
	i.e.: While in short form parsing:
	* if the string contains an open parenthesis [`(`], and
	* without having explicitly terminated magic parsing (`:`)
	issue an advice hinting to that fact.

Based on "Junio C Hamano <gitster@pobox.com>"'s code suggestion
(in https://lore.kernel.org/git/xmqqa6qoqw9n.fsf@gitster.g/).

Signed-off-by: Stavros Ntentos <133706+stdedos@users.noreply.github.com>
---
 Documentation/config/advice.txt |  5 +++++
 advice.c                        |  3 +++
 advice.h                        |  2 ++
 pathspec.c                      | 16 ++++++++++++++
 t/t6132-pathspec-exclude.sh     | 37 +++++++++++++++++++++++++++++++++
 5 files changed, 63 insertions(+)

diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index acbd0c09aa..f7ccd05c3a 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -119,4 +119,9 @@ advice.*::
 	addEmptyPathspec::
 		Advice shown if a user runs the add command without providing
 		the pathspec parameter.
+	mixedShortLongMagic::
+		Advice shown if a user provides a pathspec that could be
+		interpreted as a mixed short and long magic modifier(s)
+		(i.e. contains an open parenthesis `(` without explictly
+		terminating pathspec magic parsing with `:`).
 --
diff --git a/advice.c b/advice.c
index 164742305f..6630c57678 100644
--- a/advice.c
+++ b/advice.c
@@ -33,6 +33,7 @@ int advice_checkout_ambiguous_remote_branch_name = 1;
 int advice_submodule_alternate_error_strategy_die = 1;
 int advice_add_ignored_file = 1;
 int advice_add_empty_pathspec = 1;
+int advice_mixed_short_long_magic_pathspec = 1;
 
 static int advice_use_color = -1;
 static char advice_colors[][COLOR_MAXLEN] = {
@@ -95,6 +96,7 @@ static struct {
 	{ "submoduleAlternateErrorStrategyDie", &advice_submodule_alternate_error_strategy_die },
 	{ "addIgnoredFile", &advice_add_ignored_file },
 	{ "addEmptyPathspec", &advice_add_empty_pathspec },
+	{ "mixedShortLongMagicPathspec", &advice_mixed_short_long_magic_pathspec },
 
 	/* make this an alias for backward compatibility */
 	{ "pushNonFastForward", &advice_push_update_rejected }
@@ -137,6 +139,7 @@ static struct {
 	[ADVICE_STATUS_U_OPTION]			= { "statusUoption", 1 },
 	[ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = { "submoduleAlternateErrorStrategyDie", 1 },
 	[ADVICE_WAITING_FOR_EDITOR]			= { "waitingForEditor", 1 },
+	[ADVICE_MIXED_SHORT_LONG_MAGIC_PATHSPEC]	= { "mixedShortLongMagicPathspec", 1 },
 };
 
 static const char turn_off_instructions[] =
diff --git a/advice.h b/advice.h
index bc2432980a..050f058274 100644
--- a/advice.h
+++ b/advice.h
@@ -33,6 +33,7 @@ extern int advice_checkout_ambiguous_remote_branch_name;
 extern int advice_submodule_alternate_error_strategy_die;
 extern int advice_add_ignored_file;
 extern int advice_add_empty_pathspec;
+extern int advice_mixed_short_long_magic_pathspec;
 
 /*
  * To add a new advice, you need to:
@@ -72,6 +73,7 @@ extern int advice_add_empty_pathspec;
 	ADVICE_STATUS_U_OPTION,
 	ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE,
 	ADVICE_WAITING_FOR_EDITOR,
+	ADVICE_MIXED_SHORT_LONG_MAGIC_PATHSPEC,
 };
 
 int git_default_advice_config(const char *var, const char *value);
diff --git a/pathspec.c b/pathspec.c
index 18b3be362a..085a624ad9 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -336,6 +336,11 @@ static const char *parse_long_magic(unsigned *magic, int *prefix_len,
 	return pos;
 }
 
+static const char mixed_pathspec_magic[] = N_(
+	"'%.*s...': cannot mix shortform magic with longform [e.g. like :(glob)].\n"
+	"If '%.*s...' is a valid path, explicitly terminate magic parsing with ':'; or");
+static int extra_lookahead_chars = 7;
+
 /*
  * Parse the pathspec element looking for short magic
  *
@@ -356,6 +361,17 @@ static const char *parse_short_magic(unsigned *magic, const char *elem)
 			continue;
 		}
 
+		if (ch == '(') {
+			/*
+			 * a possible mistake: once you started a shortform
+			 * you cannot add a longform, e.g. ":!(top)"
+			 */
+			advise_if_enabled(ADVICE_MIXED_SHORT_LONG_MAGIC_PATHSPEC,
+			                  _(mixed_pathspec_magic),
+			                  (int)(pos-elem+extra_lookahead_chars), elem,
+			                  extra_lookahead_chars, pos);
+		}
+
 		if (!is_pathspec_magic(ch))
 			break;
 
diff --git a/t/t6132-pathspec-exclude.sh b/t/t6132-pathspec-exclude.sh
index 30328b87f0..b5d51c1429 100755
--- a/t/t6132-pathspec-exclude.sh
+++ b/t/t6132-pathspec-exclude.sh
@@ -244,4 +244,41 @@ test_expect_success 'grep --untracked PATTERN :(exclude)*FILE' '
 	test_cmp expect-grep actual-grep
 '
 
+cat > expected_warn <<"EOF"
+hint: ':!(glob)*...': cannot mix shortform magic with longform [e.g. like :(glob)].
+hint: If '(glob)*...' is a valid path, explicitly terminate magic parsing with ':'; or
+hint: Disable this message with "git config advice.mixedShortLongMagicPathspec false"
+EOF
+test_expect_success 'warn pathspec not matching longform magic in :!(...)' '
+	git log --oneline --format=%s -- '"'"':!(glob)**/file'"'"' >actual 2>warn &&
+	cat <<EOF >expect &&
+sub2/file
+sub/sub/sub/file
+sub/file2
+sub/sub/file
+sub/file
+file
+EOF
+	cat actual &&
+	cat warn &&
+	test_cmp expect actual &&
+	test_cmp expected_warn warn
+'
+
+test_expect_success 'do not warn pathspec not matching longform magic in :!:(...) (i.e. if magic parsing is explicitly stopped)' '
+	git log --oneline --format=%s -- '"'"':!:(glob)**/file'"'"' >actual 2>warn &&
+	cat <<EOF >expect &&
+sub2/file
+sub/sub/sub/file
+sub/file2
+sub/sub/file
+sub/file
+file
+EOF
+	cat actual &&
+	cat warn &&
+	test_cmp expect actual &&
+	! test_cmp expected_warn warn
+'
+
 test_done
-- 
2.31.0

Re: [PATCH v2] pathspec: advice: long and short forms are incompatible

[Junio C Hamano]

Stavros Ntentos <stdedos@gmail.com> writes:

Administrivia.

If "Stavros Ntentos <133706+stdedos@users.noreply.github.com>" is an
address that is not meant to receive any e-mail, please do not
include it on the Cc line and force those who respond to you to
remove it when replying.

> +static const char mixed_pathspec_magic[] = N_(
> +	"'%.*s...': cannot mix shortform magic with longform [e.g. like :(glob)].\n"

OK.  Just a bit of bikeshedding.

cannot mix short and long form magic
cannot mix shortform magic with longform
	
The former is a bit shorter.  Also, if we show (with %.*s) the
actual beginning of their attempt, e.g.  when they gave us [*]

	git show -- ':!(global,icase)foo'

if we show

	':!(glo...': cannot mix short and long form pathspec magic

or even just

	':!(...': cannot mix short and long form pathspec magic

it may be sufficiently clear where the problem is.

> +	"If '%.*s...' is a valid path, explicitly terminate magic parsing with ':'; or");

The seemingly-stray '; or' at the end aside, I am not sure what this
is trying to say.  If the sample input were [*] above, what are we
asking?  "if 'foo' is a valid path"?  No, we are showing 7 chars
starting at pos, so "if 'global,i...' is a valid path"?

If ':(global,icase)foo' were the exact path the user wants to match
with, then "prefix the whole thing with ":(literal)" would be an
understandable hint, but that is not what you are suggesting.

In short, I do not quite agree with the second line of the message.

It may be more helpful if, rather than looking at what comes after
'(', we looked at what came before '(' and helped the user write
them out in the longform, i.e. perhaps we can tell them the moral
equivalent of:

    If you meant by to use the pathspec magic '!' with other
    longform magic after '(' with ":!(...", use ":(exclude,..."
    instead.  Short and long form of pathspec magic do not mix.

> +static int extra_lookahead_chars = 7;

A few problems:

 - This is not something we want to configure.  It does not need to
   be a variable.

 - This is not something anybody other than the code in the new
   block "if (ch  == '(')" in parse_short_magic() needs to know.  It
   does not need to be a file-scope static.

 - 7 is way too long for warning against something like ":!(glob)",
   no?

But with the above "It may be more helpful" suggestion, notice that
I am deliberately refraining from looking at what comes after '(',
so extra-lookahead may not be necessary after all, and nitpicking
about it may be moot.

Thanks.

> @@ -356,6 +361,17 @@ static const char *parse_short_magic(unsigned *magic, const char *elem)
>  			continue;
>  		}
>  
> +		if (ch == '(') {
> +			/*
> +			 * a possible mistake: once you started a shortform
> +			 * you cannot add a longform, e.g. ":!(top)"
> +			 */
> +			advise_if_enabled(ADVICE_MIXED_SHORT_LONG_MAGIC_PATHSPEC,
> +			                  _(mixed_pathspec_magic),
> +			                  (int)(pos-elem+extra_lookahead_chars), elem,
> +			                  extra_lookahead_chars, pos);
> +		}
> +
>  		if (!is_pathspec_magic(ch))
>  			break;

Re: [PATCH v2] pathspec: advice: long and short forms are incompatible

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> +static const char mixed_pathspec_magic[] = N_(
>> +	"'%.*s...': cannot mix shortform magic with longform [e.g. like :(glob)].\n"
>
> OK.  Just a bit of bikeshedding.
>
> cannot mix short and long form magic
> cannot mix shortform magic with longform
> 	
> The former is a bit shorter.  Also, if we show (with %.*s) the
> actual beginning of their attempt, e.g.  when they gave us [*]
>
> 	git show -- ':!(global,icase)foo'
>
> if we show
> ...
> 	':!(...': cannot mix short and long form pathspec magic
>
> it may be sufficiently clear where the problem is.

I meant "clear where the problem is, without adding [e.g. like :(glob)]".

Thanks.

Re: [PATCH v2] pathspec: advice: long and short forms are incompatible

[Stavros Ntentos]

> Administrivia.
>
> If "Stavros Ntentos <133706+stdedos@users.noreply.github.com>" is an
> address that is not meant to receive any e-mail, please do not
> include it on the Cc line and force those who respond to you to
> remove it when replying.

I am trying. However, git-send-email keeps pulling that no-reply address, and
git-send-email does not offer any `--exclude-addresses="*glob*"`-like option.

> or even just
>
> 	':!(...': cannot mix short and long form pathspec magic
>
> it may be sufficiently clear where the problem is.

I slightly disagree, and prefer the `extra_lookahead_chars`. Just 3 characters
[`:!(`] is a bit too little, and the total message sits below the "you can
disable this message" hint.

> The seemingly-stray '; or' at the end aside, I am not sure what this
> is trying to say.

See the testcase:

> hint: If '(glob)*...' is a valid path, explicitly terminate magic parsing with ':'; or
> hint: Disable this message with "git config advice.mixedShortLongMagicPathspec false"

I am segway-ing from the "explicitly stop parsing" to the "disable this message" sentence.

> If ':(global,icase)foo' were the exact path the user wants to match
> with, then "prefix the whole thing with ":(literal)" would be an
> understandable hint, but that is not what you are suggesting.

I am siding with the "user entered this situation by mistake", and not with the
"user is explicitly trying to match a file named `:(global,icase)foo`" side.

Offering a more complete message, will become more complex. I disagree with that.
I can settle by offering examples (mine and yours) in the documentation.

> It may be more helpful if, rather than looking at what comes after
> '(', we looked at what came before '(' and helped the user write
> them out in the longform

I don't see any explicit code in parsing the shortform magics, except:
		/* Special case alias for '!' */
		if (ch == '^') {
			*magic |= PATHSPEC_EXCLUDE;
			continue;
		}
and therefore I would like to avoid such task (although I love well-written
DWIMs-or-close-to-them).

> > +static int extra_lookahead_chars = 7;
>
> A few problems:
>
>  - This is not something we want to configure.  It does not need to
>    be a variable.

I hate macros, only because I cannot expand or modify them during gdb.
(Suggestions welcome! :-D)

>
>  - This is not something anybody other than the code in the new
>    block "if (ch  == '(')" in parse_short_magic() needs to know.  It
>    does not need to be a file-scope static.

True, but the message was explicitly referred to with i18n code
specifically targeted for such initialization.

I like code doing the same job, sitting together.
I'd prefer to either move both inside (since no one else will ever
refer to this message either), or keep them as-is.

>  - 7 is way too long for warning against something like ":!(glob)",
>    no?

GRRRRR C :-p
(I'll push the changes on the next iteration; including the `like glob`
removed, and whatever comes from our discussion.)

> But with the above "It may be more helpful" suggestion, notice that
> I am deliberately refraining from looking at what comes after '(',
> so extra-lookahead may not be necessary after all, and nitpicking
> about it may be moot.

Lookahead is simply to inform user what git will do with the current
state of affairs, i.e.:

	git log --oneline --format=%s -- ':!(glob)**/file'

will filter with

	NOT '(glob)**/file'

path (truncated for brevity)

Re: [PATCH v2] pathspec: advice: long and short forms are incompatible

[Stavros Ntentos]

> >  - 7 is way too long for warning against something like ":!(glob)",
> >    no?
>
> GRRRRR C :-p
> (I'll push the changes on the next iteration; including the `like glob`
> removed, and whatever comes from our discussion.)

Actually ... C does not have a problem with it:

	root@94ae60990b85:/usr/src/git/t# ../git log -- ':!(glob)'
	hint: ':!(glob)...': cannot mix shortform magic with longform [e.g. like :(glob)].
	hint: If '(glob)...' is a valid path, explicitly terminate magic parsing with ':'; or
	hint: Disable this message with "git config advice.mixedShortLongMagicPathspec false"
	commit 14b8580dde326a0bbf9abacac7e09dbe4c38c25e (HEAD -> fix/mixed-short-long-magic)

True, it does not look nice; theoretically, any number (even 1!) could be
too much to contain ellipsis after that text.
Given the size of the change, I would want to say that it is acceptable to
leave this as-is.

Unless you'd like me to write a generic

	truncate_at_chars(char *text, int width, char *ellipsis_char_or_default)

function.

Re: [PATCH v2] pathspec: advice: long and short forms are incompatible

[Junio C Hamano]

Stavros Ntentos <stdedos@gmail.com> writes:

>> It may be more helpful if, rather than looking at what comes after
>> '(', we looked at what came before '(' and helped the user write
>> them out in the longform
>
> I don't see any explicit code in parsing the shortform magics, except:
> 		/* Special case alias for '!' */
> 		if (ch == '^') {
> 			*magic |= PATHSPEC_EXCLUDE;
> 			continue;
> 		}
> and therefore I would like to avoid such task (although I love well-written
> DWIMs-or-close-to-them).

I am not yet interested in doing DWIM, but a good advice is always
welcome.

Along the lines of the attached illustration patch, perhaps.

    $ git show --oneline --stat -- ':/!(glob)**/*.txt'
    hint: ':/!(...': cannot mix short- and longform pathspec magic
    hint: instead, spell the shortform magic '/!' as 'top,exclude' inside :(...


 Documentation/config/advice.txt |  3 +++
 advice.c                        |  3 +++
 advice.h                        |  2 ++
 pathspec.c                      | 29 +++++++++++++++++++++++++++++
 4 files changed, 37 insertions(+)

diff --git c/Documentation/config/advice.txt w/Documentation/config/advice.txt
index acbd0c09aa..05a3cbc164 100644
--- c/Documentation/config/advice.txt
+++ w/Documentation/config/advice.txt
@@ -119,4 +119,7 @@ advice.*::
 	addEmptyPathspec::
 		Advice shown if a user runs the add command without providing
 		the pathspec parameter.
+	mixedPathspecMagic::
+		Advice shown if a user tries to mix short- and
+		longform pathspec magic.
 --
diff --git c/advice.c w/advice.c
index 164742305f..9426bc5295 100644
--- c/advice.c
+++ w/advice.c
@@ -33,6 +33,7 @@ int advice_checkout_ambiguous_remote_branch_name = 1;
 int advice_submodule_alternate_error_strategy_die = 1;
 int advice_add_ignored_file = 1;
 int advice_add_empty_pathspec = 1;
+int advice_mixed_pathspec_magic = 1;
 
 static int advice_use_color = -1;
 static char advice_colors[][COLOR_MAXLEN] = {
@@ -95,6 +96,7 @@ static struct {
 	{ "submoduleAlternateErrorStrategyDie", &advice_submodule_alternate_error_strategy_die },
 	{ "addIgnoredFile", &advice_add_ignored_file },
 	{ "addEmptyPathspec", &advice_add_empty_pathspec },
+	{ "mixedPathspecMagic", &advice_mixed_pathspec_magic },
 
 	/* make this an alias for backward compatibility */
 	{ "pushNonFastForward", &advice_push_update_rejected }
@@ -113,6 +115,7 @@ static struct {
 	[ADVICE_GRAFT_FILE_DEPRECATED]			= { "graftFileDeprecated", 1 },
 	[ADVICE_IGNORED_HOOK]				= { "ignoredHook", 1 },
 	[ADVICE_IMPLICIT_IDENTITY]			= { "implicitIdentity", 1 },
+	[ADVICE_MIXED_PATHSPEC_MAGIC]			= { "mixedPathspecMagic", 1 },
 	[ADVICE_NESTED_TAG]				= { "nestedTag", 1 },
 	[ADVICE_OBJECT_NAME_WARNING]			= { "objectNameWarning", 1 },
 	[ADVICE_PUSH_ALREADY_EXISTS]			= { "pushAlreadyExists", 1 },
diff --git c/advice.h w/advice.h
index bc2432980a..d56c1896a0 100644
--- c/advice.h
+++ w/advice.h
@@ -33,6 +33,7 @@ extern int advice_checkout_ambiguous_remote_branch_name;
 extern int advice_submodule_alternate_error_strategy_die;
 extern int advice_add_ignored_file;
 extern int advice_add_empty_pathspec;
+extern int advice_mixed_pathspec_magic;
 
 /*
  * To add a new advice, you need to:
@@ -51,6 +52,7 @@ extern int advice_add_empty_pathspec;
 	ADVICE_GRAFT_FILE_DEPRECATED,
 	ADVICE_IGNORED_HOOK,
 	ADVICE_IMPLICIT_IDENTITY,
+	ADVICE_MIXED_PATHSPEC_MAGIC,
 	ADVICE_NESTED_TAG,
 	ADVICE_OBJECT_NAME_WARNING,
 	ADVICE_PUSH_ALREADY_EXISTS,
diff --git c/pathspec.c w/pathspec.c
index 18b3be362a..ce9d1738a8 100644
--- c/pathspec.c
+++ w/pathspec.c
@@ -336,6 +336,32 @@ static const char *parse_long_magic(unsigned *magic, int *prefix_len,
 	return pos;
 }
 
+/*
+ * Give hint for a common mistake of mixing short and long
+ * form of pathspec magic
+ */
+static void warn_mixed_magic(unsigned magic, const char *elem, const char *pos)
+{
+	struct strbuf longform = STRBUF_INIT;
+	int i;
+
+	if (!advice_enabled(ADVICE_MIXED_PATHSPEC_MAGIC))
+		return;
+	for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
+		if (pathspec_magic[i].bit & magic) {
+			if (longform.len)
+				strbuf_addch(&longform, ',');
+			strbuf_addstr(&longform, pathspec_magic[i].name);
+		}
+	}
+	advise(_("'%.*s(...': cannot mix short- and longform pathspec magic"),
+	       (int)(pos - elem), elem);
+	advise(_("instead, spell the shortform magic '%.*s' as '%s' inside :(..."),
+	       (int)(pos - (elem + 1)), elem + 1,
+	       longform.buf);
+}
+
+
 /*
  * Parse the pathspec element looking for short magic
  *
@@ -356,6 +382,9 @@ static const char *parse_short_magic(unsigned *magic, const char *elem)
 			continue;
 		}
 
+		if (ch == '(')
+			warn_mixed_magic(*magic, elem, pos);
+
 		if (!is_pathspec_magic(ch))
 			break;
 

[PATCH v3] pathspec: advice: long and short forms are incompatible

[Stavros Ntentos]

It can be a "reasonable" mistake to mix short and long forms,
e.g. `:!(glob)`, instead of the (correct) `:(exclude,glob)`.

Teach git to issue an advice when such a pathspec is given.
        i.e.: While in short form parsing:
        * if the string contains an open parenthesis [`(`], and
        * without having explicitly terminated magic parsing (`:`)
        issue an advice hinting to that fact.

Based on "Junio C Hamano <gitster@pobox.com>"'s code suggestions:
* https://lore.kernel.org/git/xmqqa6qoqw9n.fsf@gitster.g/
* https://lore.kernel.org/git/xmqqo8f0cr9z.fsf@gitster.g/

Signed-off-by: Stavros Ntentos <133706+stdedos@users.noreply.github.com>
---
 Documentation/config/advice.txt |  3 +++
 advice.c                        |  3 +++
 advice.h                        |  2 ++
 pathspec.c                      | 33 ++++++++++++++++++++++++++++
 t/t6132-pathspec-exclude.sh     | 38 +++++++++++++++++++++++++++++++++
 5 files changed, 79 insertions(+)

diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index acbd0c09aa..05a3cbc164 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -119,4 +119,7 @@ advice.*::
 	addEmptyPathspec::
 		Advice shown if a user runs the add command without providing
 		the pathspec parameter.
+	mixedPathspecMagic::
+		Advice shown if a user tries to mix short- and
+		longform pathspec magic.
 --
diff --git a/advice.c b/advice.c
index 164742305f..f94505e0d6 100644
--- a/advice.c
+++ b/advice.c
@@ -33,6 +33,7 @@ int advice_checkout_ambiguous_remote_branch_name = 1;
 int advice_submodule_alternate_error_strategy_die = 1;
 int advice_add_ignored_file = 1;
 int advice_add_empty_pathspec = 1;
+int advice_mixed_pathspec_magic = 1;
 
 static int advice_use_color = -1;
 static char advice_colors[][COLOR_MAXLEN] = {
@@ -95,6 +96,7 @@ static struct {
 	{ "submoduleAlternateErrorStrategyDie", &advice_submodule_alternate_error_strategy_die },
 	{ "addIgnoredFile", &advice_add_ignored_file },
 	{ "addEmptyPathspec", &advice_add_empty_pathspec },
+	{ "mixedPathspecMagic", &advice_mixed_pathspec_magic },
 
 	/* make this an alias for backward compatibility */
 	{ "pushNonFastForward", &advice_push_update_rejected }
@@ -137,6 +139,7 @@ static struct {
 	[ADVICE_STATUS_U_OPTION]			= { "statusUoption", 1 },
 	[ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = { "submoduleAlternateErrorStrategyDie", 1 },
 	[ADVICE_WAITING_FOR_EDITOR]			= { "waitingForEditor", 1 },
+	[ADVICE_MIXED_PATHSPEC_MAGIC]	= { "mixedPathspecMagic", 1 },
 };
 
 static const char turn_off_instructions[] =
diff --git a/advice.h b/advice.h
index bc2432980a..aa229fded8 100644
--- a/advice.h
+++ b/advice.h
@@ -33,6 +33,7 @@ extern int advice_checkout_ambiguous_remote_branch_name;
 extern int advice_submodule_alternate_error_strategy_die;
 extern int advice_add_ignored_file;
 extern int advice_add_empty_pathspec;
+extern int advice_mixed_pathspec_magic;
 
 /*
  * To add a new advice, you need to:
@@ -72,6 +73,7 @@ extern int advice_add_empty_pathspec;
 	ADVICE_STATUS_U_OPTION,
 	ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE,
 	ADVICE_WAITING_FOR_EDITOR,
+	ADVICE_MIXED_PATHSPEC_MAGIC,
 };
 
 int git_default_advice_config(const char *var, const char *value);
diff --git a/pathspec.c b/pathspec.c
index 18b3be362a..5f77e2e8d8 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -336,6 +336,36 @@ static const char *parse_long_magic(unsigned *magic, int *prefix_len,
 	return pos;
 }
 
+
+/*
+ * Give hint for a common mistake of mixing short and long
+ * form of pathspec magic, as well as possible corrections
+ */
+static void warn_mixed_magic(unsigned magic, const char *elem, const char *pos)
+{
+	struct strbuf longform = STRBUF_INIT;
+	int i;
+
+	if (!advice_enabled(ADVICE_MIXED_PATHSPEC_MAGIC))
+		return;
+
+	for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
+		if (pathspec_magic[i].bit & magic) {
+			if (longform.len)
+				strbuf_addch(&longform, ',');
+			strbuf_addstr(&longform, pathspec_magic[i].name);
+		}
+	}
+	advise_if_enabled(ADVICE_MIXED_PATHSPEC_MAGIC,
+	                  _("'%.*s(...': cannot mix short- and longform pathspec magic!\n"
+	                    "Either spell the shortform magic '%.*s' as ':(%s,...'\n"
+	                    "or end magic pathspec matching with '%.*s:'."),
+	                  (int)(pos - elem), elem,
+	                  (int)(pos - (elem + 1)), elem + 1,
+	                  longform.buf,
+	                  (int)(pos - elem), elem);
+}
+
 /*
  * Parse the pathspec element looking for short magic
  *
@@ -356,6 +386,9 @@ static const char *parse_short_magic(unsigned *magic, const char *elem)
 			continue;
 		}
 
+		if (ch == '(')
+			warn_mixed_magic(*magic, elem, pos);
+
 		if (!is_pathspec_magic(ch))
 			break;
 
diff --git a/t/t6132-pathspec-exclude.sh b/t/t6132-pathspec-exclude.sh
index 30328b87f0..8b9d543e1e 100755
--- a/t/t6132-pathspec-exclude.sh
+++ b/t/t6132-pathspec-exclude.sh
@@ -244,4 +244,42 @@ test_expect_success 'grep --untracked PATTERN :(exclude)*FILE' '
 	test_cmp expect-grep actual-grep
 '
 
+cat > expected_warn <<"EOF"
+hint: ':!(...': cannot mix short- and longform pathspec magic!
+hint: Either spell the shortform magic '!' as ':(exclude,...'
+hint: or end magic pathspec matching with ':!:'.
+hint: Disable this message with "git config advice.mixedPathspecMagic false"
+EOF
+test_expect_success 'warn pathspec not matching longform magic in :!(...)' '
+	git log --oneline --format=%s -- '"'"':!(glob)**/file'"'"' >actual 2>warn &&
+	cat <<EOF >expect &&
+sub2/file
+sub/sub/sub/file
+sub/file2
+sub/sub/file
+sub/file
+file
+EOF
+	cat actual &&
+	cat warn &&
+	test_cmp expect actual &&
+	test_cmp expected_warn warn
+'
+
+test_expect_success 'do not warn pathspec not matching longform magic in :!:(...) (i.e. if magic parsing is explicitly stopped)' '
+	git log --oneline --format=%s -- '"'"':!:(glob)**/file'"'"' >actual 2>warn &&
+	cat <<EOF >expect &&
+sub2/file
+sub/sub/sub/file
+sub/file2
+sub/sub/file
+sub/file
+file
+EOF
+	cat actual &&
+	cat warn &&
+	test_cmp expect actual &&
+	! test_cmp expected_warn warn
+'
+
 test_done
-- 
2.31.1

[PATCH v4] pathspec: advice: long and short forms are incompatible

[Stavros Ntentos]

It can be a "reasonable" mistake to mix short and long forms,
e.g. `:!(glob)`, instead of the (correct) `:(exclude,glob)`.

Teach git to issue an advice when such a pathspec is given.
        i.e.: While in short form parsing:
        * if the string contains an open parenthesis [`(`], and
        * without having explicitly terminated magic parsing (`:`)
        issue an advice hinting to that fact.

Based on "Junio C Hamano <gitster@pobox.com>"'s code suggestions:
* https://lore.kernel.org/git/xmqqa6qoqw9n.fsf@gitster.g/
* https://lore.kernel.org/git/xmqqo8f0cr9z.fsf@gitster.g/

Signed-off-by: Stavros Ntentos <133706+stdedos@users.noreply.github.com>
---
 Documentation/config/advice.txt |  3 +++
 advice.c                        |  3 +++
 advice.h                        |  2 ++
 pathspec.c                      | 33 ++++++++++++++++++++++++++++
 t/t6132-pathspec-exclude.sh     | 38 +++++++++++++++++++++++++++++++++
 5 files changed, 79 insertions(+)

diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index acbd0c09aa..05a3cbc164 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -119,4 +119,7 @@ advice.*::
 	addEmptyPathspec::
 		Advice shown if a user runs the add command without providing
 		the pathspec parameter.
+	mixedPathspecMagic::
+		Advice shown if a user tries to mix short- and
+		longform pathspec magic.
 --
diff --git a/advice.c b/advice.c
index 164742305f..b1955966d5 100644
--- a/advice.c
+++ b/advice.c
@@ -33,6 +33,7 @@ int advice_checkout_ambiguous_remote_branch_name = 1;
 int advice_submodule_alternate_error_strategy_die = 1;
 int advice_add_ignored_file = 1;
 int advice_add_empty_pathspec = 1;
+int advice_mixed_pathspec_magic = 1;
 
 static int advice_use_color = -1;
 static char advice_colors[][COLOR_MAXLEN] = {
@@ -95,6 +96,7 @@ static struct {
 	{ "submoduleAlternateErrorStrategyDie", &advice_submodule_alternate_error_strategy_die },
 	{ "addIgnoredFile", &advice_add_ignored_file },
 	{ "addEmptyPathspec", &advice_add_empty_pathspec },
+	{ "mixedPathspecMagic", &advice_mixed_pathspec_magic },
 
 	/* make this an alias for backward compatibility */
 	{ "pushNonFastForward", &advice_push_update_rejected }
@@ -137,6 +139,7 @@ static struct {
 	[ADVICE_STATUS_U_OPTION]			= { "statusUoption", 1 },
 	[ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = { "submoduleAlternateErrorStrategyDie", 1 },
 	[ADVICE_WAITING_FOR_EDITOR]			= { "waitingForEditor", 1 },
+	[ADVICE_MIXED_PATHSPEC_MAGIC]			= { "mixedPathspecMagic", 1 },
 };
 
 static const char turn_off_instructions[] =
diff --git a/advice.h b/advice.h
index bc2432980a..aa229fded8 100644
--- a/advice.h
+++ b/advice.h
@@ -33,6 +33,7 @@ extern int advice_checkout_ambiguous_remote_branch_name;
 extern int advice_submodule_alternate_error_strategy_die;
 extern int advice_add_ignored_file;
 extern int advice_add_empty_pathspec;
+extern int advice_mixed_pathspec_magic;
 
 /*
  * To add a new advice, you need to:
@@ -72,6 +73,7 @@ extern int advice_add_empty_pathspec;
 	ADVICE_STATUS_U_OPTION,
 	ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE,
 	ADVICE_WAITING_FOR_EDITOR,
+	ADVICE_MIXED_PATHSPEC_MAGIC,
 };
 
 int git_default_advice_config(const char *var, const char *value);
diff --git a/pathspec.c b/pathspec.c
index 18b3be362a..5f77e2e8d8 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -336,6 +336,36 @@ static const char *parse_long_magic(unsigned *magic, int *prefix_len,
 	return pos;
 }
 
+
+/*
+ * Give hint for a common mistake of mixing short and long
+ * form of pathspec magic, as well as possible corrections
+ */
+static void warn_mixed_magic(unsigned magic, const char *elem, const char *pos)
+{
+	struct strbuf longform = STRBUF_INIT;
+	int i;
+
+	if (!advice_enabled(ADVICE_MIXED_PATHSPEC_MAGIC))
+		return;
+
+	for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
+		if (pathspec_magic[i].bit & magic) {
+			if (longform.len)
+				strbuf_addch(&longform, ',');
+			strbuf_addstr(&longform, pathspec_magic[i].name);
+		}
+	}
+	advise_if_enabled(ADVICE_MIXED_PATHSPEC_MAGIC,
+	                  _("'%.*s(...': cannot mix short- and longform pathspec magic!\n"
+	                    "Either spell the shortform magic '%.*s' as ':(%s,...'\n"
+	                    "or end magic pathspec matching with '%.*s:'."),
+	                  (int)(pos - elem), elem,
+	                  (int)(pos - (elem + 1)), elem + 1,
+	                  longform.buf,
+	                  (int)(pos - elem), elem);
+}
+
 /*
  * Parse the pathspec element looking for short magic
  *
@@ -356,6 +386,9 @@ static const char *parse_short_magic(unsigned *magic, const char *elem)
 			continue;
 		}
 
+		if (ch == '(')
+			warn_mixed_magic(*magic, elem, pos);
+
 		if (!is_pathspec_magic(ch))
 			break;
 
diff --git a/t/t6132-pathspec-exclude.sh b/t/t6132-pathspec-exclude.sh
index 30328b87f0..8b9d543e1e 100755
--- a/t/t6132-pathspec-exclude.sh
+++ b/t/t6132-pathspec-exclude.sh
@@ -244,4 +244,42 @@ test_expect_success 'grep --untracked PATTERN :(exclude)*FILE' '
 	test_cmp expect-grep actual-grep
 '
 
+cat > expected_warn <<"EOF"
+hint: ':!(...': cannot mix short- and longform pathspec magic!
+hint: Either spell the shortform magic '!' as ':(exclude,...'
+hint: or end magic pathspec matching with ':!:'.
+hint: Disable this message with "git config advice.mixedPathspecMagic false"
+EOF
+test_expect_success 'warn pathspec not matching longform magic in :!(...)' '
+	git log --oneline --format=%s -- '"'"':!(glob)**/file'"'"' >actual 2>warn &&
+	cat <<EOF >expect &&
+sub2/file
+sub/sub/sub/file
+sub/file2
+sub/sub/file
+sub/file
+file
+EOF
+	cat actual &&
+	cat warn &&
+	test_cmp expect actual &&
+	test_cmp expected_warn warn
+'
+
+test_expect_success 'do not warn pathspec not matching longform magic in :!:(...) (i.e. if magic parsing is explicitly stopped)' '
+	git log --oneline --format=%s -- '"'"':!:(glob)**/file'"'"' >actual 2>warn &&
+	cat <<EOF >expect &&
+sub2/file
+sub/sub/sub/file
+sub/file2
+sub/sub/file
+sub/file
+file
+EOF
+	cat actual &&
+	cat warn &&
+	test_cmp expect actual &&
+	! test_cmp expected_warn warn
+'
+
 test_done
-- 
2.31.1