[PATCH v3] test_dir_is_empty: properly detect files with newline in name
While the `test_dir_is_empty` function appears correct in most normal
use cases, it can fail when filenames contain newlines. This patch
changes the implementation to check that the output of `ls -a` has at
most two lines (for `.` and `..`), which should be better behaved.
The newly added unit test fails before this change and passes after it.
Signed-off-by: William Chargin
---
This patch depends on "t: factor out FUNNYNAMES as shared lazy prereq"
(2018-08-06), available from `wc/make-funnynames-shared-lazy-prereq`.
The code will work on master, but the test will not run due to a missing
prereq.
I originally wrote this patch for the standalone Sharness library, but
that library advises that such patches be sent to the Git mailing list
first.
t/t-basic.sh| 31 +++
t/test-lib-functions.sh | 2 +-
2 files changed, 32 insertions(+), 1 deletion(-)
diff --git a/t/t-basic.sh b/t/t-basic.sh
index 34859fe4a..60134d7ab 100755
--- a/t/t-basic.sh
+++ b/t/t-basic.sh
@@ -821,6 +821,37 @@ test_expect_success 'tests clean up even on failures' "
EOF
"
+test_expect_success FUNNYNAMES \
+ 'test_dir_is_empty behaves even in pathological cases' "
+ run_sub_test_lib_test \
+ dir-empty 'behavior of test_dir_is_empty' <<-\\EOF &&
+ test_expect_success 'should pass with actually empty directory' '
+ mkdir empty_dir &&
+ test_dir_is_empty empty_dir
+ '
+ test_expect_success 'should fail with a normal filename' '
+ mkdir nonempty_dir &&
+ touch nonempty_dir/some_file &&
+ test_must_fail test_dir_is_empty nonempty_dir
+ '
+ test_expect_success 'should fail with dot-newline-dot filename' '
+ mkdir pathological_dir &&
+ touch \"pathological_dir/.
+ .\" &&
+ test_must_fail test_dir_is_empty pathological_dir
+ '
+ test_done
+ EOF
+ check_sub_test_lib_test dir-empty <<-\\EOF
+ > ok 1 - should pass with actually empty directory
+ > ok 2 - should fail with a normal filename
+ > ok 3 - should fail with dot-newline-dot filename
+ > # passed all 3 test(s)
+ > 1..3
+ EOF
+"
+
+
# Basics of the basics
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 2b2181dca..f0241992b 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -568,7 +568,7 @@ test_path_is_dir () {
# Check if the directory exists and is empty as expected, barf otherwise.
test_dir_is_empty () {
test_path_is_dir "$1" &&
- if test -n "$(ls -a1 "$1" | egrep -v '^\.\.?$')"
+ if test "$(ls -a1 "$1" | wc -l)" -gt 2
then
echo "Directory '$1' is not empty, it contains:"
ls -la "$1"
--
2.18.0.548.g101af7bd4
Re: [PATCH v3] test_dir_is_empty: properly detect files with newline in name
On Sun, Aug 12, 2018 at 12:07 AM William Chargin wrote: > While the `test_dir_is_empty` function appears correct in most normal > use cases, it can fail when filenames contain newlines. This patch > changes the implementation to check that the output of `ls -a` has at > most two lines (for `.` and `..`), which should be better behaved. > > The newly added unit test fails before this change and passes after it. > > Signed-off-by: William Chargin > --- > diff --git a/t/t-basic.sh b/t/t-basic.sh > @@ -821,6 +821,37 @@ test_expect_success 'tests clean up even on failures' " > +test_expect_success FUNNYNAMES \ > + 'test_dir_is_empty behaves even in pathological cases' " > + test_expect_success 'should fail with a normal filename' ' > + mkdir nonempty_dir && > + touch nonempty_dir/some_file && We usually avoid "touch" unless the timestamp of the file is significant. In this case, it isn't, so it would be more idiomatic to say simply: >nonempty_dir/some_file && > + test_must_fail test_dir_is_empty nonempty_dir This is an abuse of test_must_fail() which is intended strictly for testing 'git' invocations which might fail for reasons other than the expected one (for instance, git might crash). Instead, you should just use '!', like this: ! test_dir_is_empty nonempty_dir > + test_expect_success 'should fail with dot-newline-dot filename' ' > + mkdir pathological_dir && > + touch \"pathological_dir/. > + .\" && > + test_must_fail test_dir_is_empty pathological_dir Same comments as above.
Re: [PATCH v3] test_dir_is_empty: properly detect files with newline in name
Thanks for the review. > We usually avoid "touch" unless the timestamp of the file is > significant. Makes sense. Will change as you suggest. > This is an abuse of test_must_fail() which is intended strictly for > testing 'git' invocations which might fail for reasons other than the > expected one (for instance, git might crash). Interesting. I didn't infer this from the docs on `test_must_fail` in `t/test-lib-functions.sh`. Sharness, which is supposed to be independent of Git, explicitly says to use `test_must_fail` instead of `!`. (Admittedly, the implementations are different, but only slightly: within Git, a Valgrind error 126 is a failure, not success.) I also see other uses of `test_must_fail` throughout the codebase: e.g., with `kill`, `test`, `test_cmp`, `run_sub_test_lib_test`, etc. as the target command. Are these invocations in error? (I'm nevertheless happy to change this as you suggest.) I'll make these changes locally and hold on to the patch, waiting for others' input. Best, WC
Re: [PATCH v3] test_dir_is_empty: properly detect files with newline in name
On Sun, Aug 12, 2018 at 2:33 AM William Chargin wrote: > > This is an abuse of test_must_fail() which is intended strictly for > > testing 'git' invocations which might fail for reasons other than the > > expected one (for instance, git might crash). > > Interesting. I didn't infer this from the docs on `test_must_fail` in > `t/test-lib-functions.sh`. Sharness, which is supposed to be independent > of Git, explicitly says to use `test_must_fail` instead of `!`. This sort of knowledge is, perhaps unfortunately, spread around in too many places. In this case, it's mentioned in t/README. The relevant excerpt: Don't use '! git cmd' when you want to make sure the git command exits with failure in a controlled way by calling "die()". Instead, use 'test_must_fail git cmd'. This will signal a failure if git dies in an unexpected way (e.g. segfault). On the other hand, don't use test_must_fail for running regular platform commands; just use '! cmd'. We are not in the business of verifying that the world given to us sanely works. It probably wouldn't hurt to update the comment block above test_must_fail() in t/test-functions-lib.sh. > I also see other uses of `test_must_fail` throughout the codebase: e.g., > with `kill`, `test`, `test_cmp`, `run_sub_test_lib_test`, etc. as the > target command. Are these invocations in error? Looks that way, even run_sub_test_lib_test().
