Re: [PATCH] pathspec: prevent empty strings as pathspecs
- From
David Turner <novalis@novalis.org>
- Date
- Jun 19, 2016, 01:49 UTC
- Message-ID
- <1466300967.28660.9.camel@frank>
- In-Reply-To
- <20160619005704.1771-1-emilyxxie@gmail.com>
On Sat, 2016-06-18 at 20:57 -0400, Emily Xie wrote:
Show 13 quoted lines
> 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.
Show 18 quoted lines
> 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. Show 17 quoted lines
> 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?