From: Bagas Sanjaya <bagasdotme@gmail.com>
Date: Thu, 25 Mar 2021 11:00:29 GMT
Subject: Re: [PATCH v1 1/1] pathspec: warn for a no-glob entry that contains `**`
Message-ID: <0eeedc6c-cf47-41ac-a1f2-242d33963aa8@gmail.com>
In-Reply-To: <20210325102228.14901-2-stdedos@gmail.com>

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