Re: [PATCH] pathspec: die on empty strings as pathspec
I ran the tests and none of them failed. Technically, the state of the index would indeed be different with these new changes, but this shouldn't be an issue. In the current version, there's one only item added to the index that ends up getting used in subsequent tests. That is, foo1, which is tested in: 'git add --chmod=[+-]x stages correctly'. With the current code, foo1 gets added to the index with a mode of 100644. When the 'git add --chmod=[+-]x stages correctly' test executes, it removes foo1 from the working directory, creates a new foo1, and runs 'git add --chmod=+x'. It then checks if foo1 is in the index with a file mode of 100755. Given how things are set up, the tests pass either way. But I think it'd actually be nicer with the new changes in this patch, as foo1 would not already be in the index by the time 'git add --chmod=[+-]x stages correctly' executes. On Sat, Jun 10, 2017 at 1:54 AM, Junio C Hamano wrote: > > Emily Xie writes: > > > diff --git a/t/t3700-add.sh b/t/t3700-add.sh > > index f3a4b4a..40a0d2b 100755 > > --- a/t/t3700-add.sh > > +++ b/t/t3700-add.sh > > @@ -331,9 +331,8 @@ test_expect_success 'git add --dry-run --ignore-missing > > of non-existing file out > > test_i18ncmp expect.err actual.err > > ' > > > > -test_expect_success 'git add empty string should invoke warning' ' > > - git add "" 2>output && > > - test_i18ngrep "warning: empty strings" output > > +test_expect_success 'git add empty string should fail' ' > > + test_must_fail git add "" > > ' > > Doesn't this affect the tests that follow this step? We used to > warn but went ahead and added all of them anyway, but now we fail > without adding anything, which means that the state of the index > before and after this patch would be different. > > > test_expect_success 'git add --chmod=[+-]x stages correctly' '
[PATCH] pathspec: die on empty strings as pathspec
An empty string as a pathspec element matches all paths. A buggy
script, however, could accidentally assign an empty string to a
variable that then gets passed to a Git command invocation, e.g.:
path=... compute a path to be removed in $path ...
git rm -r "$path"
which would unintentionally remove all paths in the current
directory.
The fix for this issue comprises of two steps. Step 1, which warns
that empty strings as pathspecs will become invalid, has already
been implemented in commit d426430 ("pathspec: warn on empty strings
as pathspec", 2016-06-22).
This patch is step 2. It removes the warning and throws an error
instead.
Signed-off-by: Emily Xie
Reported-by: David Turner
---
pathspec.c | 10 --
t/t3600-rm.sh | 5 ++---
t/t3700-add.sh | 5 ++---
3 files changed, 8 insertions(+), 12 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index 50f76ff..65e18b1 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -605,7 +605,7 @@ void parse_pathspec(struct pathspec *pathspec,
{
struct pathspec_item *item;
const char *entry = argv ? *argv : NULL;
- int i, n, prefixlen, warn_empty_string, nr_exclude = 0;
+ int i, n, prefixlen, nr_exclude = 0;
memset(pathspec, 0, sizeof(*pathspec));
@@ -638,12 +638,10 @@ void parse_pathspec(struct pathspec *pathspec,
}
n = 0;
- warn_empty_string = 1;
while (argv[n]) {
- if (*argv[n] == '\0' && warn_empty_string) {
- warning(_("empty strings as pathspecs will be made
invalid in upcoming releases. "
- "please use . instead if you meant to match
all paths"));
- warn_empty_string = 0;
+ if (*argv[n] == '\0') {
+ die("empty string is not a valid pathspec. "
+ "please use . instead if you meant to match
all paths");
}
n++;
}
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 5f9913b..c787eac 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -858,9 +858,8 @@ test_expect_success 'rm files with two different errors' '
test_i18ncmp expect actual
'
-test_expect_success 'rm empty string should invoke warning' '
- git rm -rf "" 2>output &&
- test_i18ngrep "warning: empty strings" output
+test_expect_success 'rm empty string should fail' '
+ test_must_fail git rm -rf ""
'
test_done
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index f3a4b4a..40a0d2b 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -331,9 +331,8 @@ test_expect_success 'git add --dry-run --ignore-missing of
non-existing file out
test_i18ncmp expect.err actual.err
'
-test_expect_success 'git add empty string should invoke warning' '
- git add "" 2>output &&
- test_i18ngrep "warning: empty strings" output
+test_expect_success 'git add empty string should fail' '
+ test_must_fail git add ""
'
test_expect_success 'git add --chmod=[+-]x stages correctly' '
--
2.8.4
Re: [PATCH] pathspec: warn on empty strings as pathspec
I suppose I got ahead of myself there. :-) Thanks for the clarification on the process. On Thu, Jun 23, 2016 at 2:17 AM, Junio C Hamano wrote: > Emily Xie writes: > >> Awesome. Thanks, Junio---this is exciting! > > No, thank _you_. > >> As for step 2: do you have a good sense on the timing? Around how long >> should I wait to submit the patch for it? > > Not so fast. We'll wait for others to comment first. > > I am queuing this change but that does not mean anything more than > that I am not outright rejecting the idea. > > It is possible that others may assess the cost of having to do the > two-step migration differently, and the list concensus may end up > being "if it hurts, don't pass an empty string", i.e. we'd better > without this patch or subsequent second step. If that happens, the > first step dies without hitting 'next'. I'd say we'd wait at least > for a week. > > Otherwise, if the change is received favourably, we'll merge it to > 'next', do the same waiting for comments. Repeat the same and then > merge to 'master'. After it hits the next feature release (probably > at around the end of August), the change will be seen by wider > general public, and at that point we may see strong opposition from > them with a good argument neither of us thought of why we shouldn't > do this change, in which case we might have to revise the plan and > scrap the whole thing. > > So, we'll wait and see. > -- Emily Xie xie-emily.com 207.399.6370 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] pathspec: warn on empty strings as pathspec
Awesome. Thanks, Junio---this is exciting! As for step 2: do you have a good sense on the timing? Around how long should I wait to submit the patch for it? Otherwise, let me know if there is anything else that I need to do! - Emily -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] pathspec: warn on empty strings as pathspec
Currently, passing an empty string as a pathspec results in
a match on all paths. This is because a pathspec match is a
leading substring match that honors directory boundaries.
So, just as pathspec "dir/" (or "dir") matches "dir/file",
"" matches "file".
However, this causes problems. Namely, a user's
carelessly-written script could accidentally assign an
empty string to a variable that then gets passed to a Git
command invocation, e.g.:
git rm -r "$path"
git add "$path"
which would unintentionally affect all paths in the current
directory.
The fix for this issue requires a two-step approach. As
there may be existing scripts that knowingly use empty
strings in this manner, the first step simply invokes a
warning that (1) declares using empty strings to
match all paths will become invalid and (2) asks the user
to use "." if their intent was to match all.
For step two, a follow-up patch several release cycles
later will remove the warnings and throw an error instead.
This patch is the first step.
Signed-off-by: Emily Xie
Reported-by: David Turner
Mentored-by: Michail Denchev
Thanks-to: Sarah Sharp and James Sharp
---
pathspec.c | 11 +--
t/t3600-rm.sh | 5 +
t/t3700-add.sh | 5 +
3 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index c9e9b6c..02aa691 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -364,7 +364,7 @@ void parse_pathspec(struct pathspec *pathspec,
{
struct pathspec_item *item;
const char *entry = argv ? *argv : NULL;
- int i, n, prefixlen, nr_exclude = 0;
+ int i, n, prefixlen, warn_empty_string, nr_exclude = 0;
memset(pathspec, 0, sizeof(*pathspec));
@@ -402,8 +402,15 @@ void parse_pathspec(struct pathspec *pathspec,
}
n = 0;
- while (argv[n])
+ warn_empty_string = 1;
+ while (argv[n]) {
+ if (*argv[n] == '\0' && warn_empty_string) {
+ warning(_("empty strings as pathspecs will be made
invalid in upcoming releases. "
+ "please use . instead if you meant to match
all paths"));
+ warn_empty_string = 0;
+ }
n++;
+ }
pathspec->nr = n;
ALLOC_ARRAY(pathspec->items, n);
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index d046d98..14f0edc 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -881,4 +881,9 @@ test_expect_success 'rm files with two different errors' '
test_i18ncmp expect actual
'
+test_expect_success 'rm empty string should invoke warning' '
+ git rm -rf "" 2>output &&
+ test_i18ngrep "warning: empty strings" output
+'
+
test_done
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index f14a665..05379d0 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -332,4 +332,9 @@ test_expect_success 'git add --dry-run --ignore-missing of
non-existing file out
test_i18ncmp expect.err actual.err
'
+test_expect_success 'git add empty string should invoke warning' '
+ git add "" 2>output &&
+ test_i18ngrep "warning: empty strings" output
+'
+
test_done
--
2.8.4
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] pathspec: warn on empty strings as pathspec
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. A two-step implemetation will
prevent such cases.
This patch, as step one, invokes a warning whenever an empty
string is detected as a pathspec, introducing users to the upcoming
change. For step two, a follow-up patch several release cycles later
will remove the warnings and actually implement the change by
throwing an error instead.
Signed-off-by: Emily Xie
Reported-by: David Turner
Mentored-by: Michail Denchev
Thanks-to: Sarah Sharp and James Sharp
---
pathspec.c | 6 +-
t/t3600-rm.sh | 6 +-
t/t3700-add.sh | 4
3 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index c9e9b6c..79e370e 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')
+ warning(_("empty strings are not valid pathspecs and
will no longer "
+ "be supported in upcoming releases"));
n++;
+ }
pathspec->nr = n;
ALLOC_ARRAY(pathspec->items, n);
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index d046d98..4503a14 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -881,4 +881,8 @@ test_expect_success 'rm files with two different errors' '
test_i18ncmp expect actual
'
-test_done
+test_expect_success 'rm empty string should invoke warning' '
+ git rm -rf "" 2>&1 | grep "warning: empty string"
+'
+
+test_done
\ No newline at end of file
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index f14a665..5dbe8c2 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -207,6 +207,10 @@ test_expect_success POSIXPERM,SANITY 'git add should fail
atomically upon an unr
! ( git ls-files foo1 | grep foo1 )
'
+test_expect_success 'git add empty string should invoke warning' '
+ git add "" 2>&1 | grep "warning: empty string"
+'
+
rm -f foo2
test_expect_success POSIXPERM,SANITY 'git add --ignore-errors' '
--
2.8.4
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] pathspec: prevent empty strings as pathspecs
Thanks for the quick responses on this patch! In response to Junio: > "At least you would need two-step process to introduce a change like > this to warn the people whose tools and workflows you are breaking. > That is, (1) in one release, you add code to only detect the case > you will be changing the behaviour in a later version and give > warning messages, and (2) in another release that is several release > cycles later, stop warning and actually change the behaviour > I do not mind a two-step breaking of compatibility to address this > issue; I would also understand if the author thinks it is not worth > the hassle to do so. The sudden behaviour change with this patch > alone is however not acceptable, I would think." I understand your hesitance with the original method. If the agreed solution is a two-step implementation, I think it would definitely be worth my time and hasslein part because I'm particularly excited about the prospect of contributing to Git, but mostly because I do believe that it would be a good improvement. Given this, I'll edit the patch and re-submit to only emit warning messages for now. - Emily -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] pathspec: prevent empty strings as pathspecs
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
Reported-by: David Turner
Mentored-by: Michail Denchev
Thanks-to: Sarah Sharp and James Sharp
---
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.");
+ }
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 ""
+'
+
test_expect_success 'rm files with different staged content' '
cat >expect <<-\EOF &&
error: the following files have staged content different from both the
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index f14a665..49cb080 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -207,6 +207,10 @@ test_expect_success POSIXPERM,SANITY 'git add should fail
atomically upon an unr
! ( git ls-files foo1 | grep foo1 )
'
+test_expect_success 'git add empty string as pathspec must fail' '
+ test_must_fail git add ""
+'
+
rm -f foo2
test_expect_success POSIXPERM,SANITY 'git add --ignore-errors' '
--
2.8.4
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
