Re: [PATCH] pathspec: prevent empty strings as pathspecs

[David Turner <novalis@novalis.org>]

On Sat, 2016-06-18 at 20:57 -0400, Emily Xie wrote:
> For any command that takes a pathspec, passing an empty string will
> execute the command on all files in the current directory. This
> results in unexpected behavior. For example, git add "" adds all
> files to staging, while git rm -rf "" recursively removes all files
> from the working tree and index. This patch prevents such cases by
> throwing an error message whenever an empty string is detected as a
> pathspec.
> 
> Signed-off-by: Emily Xie <emilyxxie@gmail.com>
> Reported-by: David Turner <novalis@novalis.org>
> Mentored-by: Michail Denchev <mdenchev@gmail.com>
> Thanks-to: Sarah Sharp <sarah@thesharps.us> and James Sharp <jamey@minilop.net>
> ---

Overall, lgtm.

The reason for this behavior is that, generally, traversals start at
some tree, and if there are no elements in the pathspec, the recursion
ends with that tree. Because this is a UX issue, fixing it at the
pathspec level seems correct to me.


>  pathspec.c     | 6 +++++-
>  t/t3600-rm.sh  | 4 ++++
>  t/t3700-add.sh | 4 ++++
>  3 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/pathspec.c b/pathspec.c
> index c9e9b6c..11901a2 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -402,8 +402,12 @@ void parse_pathspec(struct pathspec *pathspec,
>  	}
>  
>  	n = 0;
> -	while (argv[n])
> +	while (argv[n]) {
> +		if (*argv[n] == '\0') {
> +			die("Empty string is not a valid pathspec.");
> +		}

nit: git style doesn't use {} on one-statement ifs.  

>  		n++;
> +	}
>  
>  	pathspec->nr = n;
>  	ALLOC_ARRAY(pathspec->items, n);
> diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
> index d046d98..1d7dc79 100755
> --- a/t/t3600-rm.sh
> +++ b/t/t3600-rm.sh
> @@ -791,6 +791,10 @@ test_expect_success 'setup for testing rm messages' '
>  	git add bar.txt foo.txt
>  '
>  
> +test_expect_success 'rm files with empty string pathspec' '
> +  test_must_fail git rm ""
> +'
> +

Maybe check the error message here?