diff mbox series

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

Message ID 20210325102228.14901-2-stdedos@gmail.com (mailing list archive)
State Superseded
Headers show
Series pathspec: warn for a no-glob entry that contains `**` | expand

Commit Message

Σταύρος Ντέντος March 25, 2021, 10:22 a.m. UTC 
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(+)

Comments

Bagas Sanjaya March 25, 2021, 11 a.m. UTC | #1 
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
>
Bagas Sanjaya March 25, 2021, 11:04 a.m. UTC | #2 
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
>
Σταύρος Ντέντος March 25, 2021, 4:09 p.m. UTC | #3 
> > +	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.
Junio C Hamano March 25, 2021, 8:09 p.m. UTC | #4 
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.
diff mbox series

Patch

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