* [PATCH 0/1] t/test-lib: make `test_dir_is_empty` more robust @ 2018-08-05 2:20 William Chargin 2018-08-05 2:20 ` [PATCH 1/1] " William Chargin 2018-08-05 3:36 ` [PATCH 0/1] " Jonathan Nieder 0 siblings, 2 replies; 23+ messages in thread From: William Chargin @ 2018-08-05 2:20 UTC (permalink / raw) To: git; +Cc: William Chargin While the `test_dir_is_empty` function appears correct in most normal use cases, it can fail when filenames contain newlines. 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. William Chargin (1): t/test-lib: make `test_dir_is_empty` more robust t/t0000-basic.sh | 29 +++++++++++++++++++++++++++++ t/test-lib-functions.sh | 2 +- 2 files changed, 30 insertions(+), 1 deletion(-) -- 2.18.0.548.g79b975644 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 1/1] t/test-lib: make `test_dir_is_empty` more robust 2018-08-05 2:20 [PATCH 0/1] t/test-lib: make `test_dir_is_empty` more robust William Chargin @ 2018-08-05 2:20 ` William Chargin 2018-08-05 4:19 ` Jonathan Nieder 2018-08-05 3:36 ` [PATCH 0/1] " Jonathan Nieder 1 sibling, 1 reply; 23+ messages in thread From: William Chargin @ 2018-08-05 2:20 UTC (permalink / raw) To: git; +Cc: William Chargin The previous behavior could incorrectly pass given a directory with a filename containing a newline. This patch changes the implementation to use `ls -A`, which is specified by POSIX. The output should be empty exactly if the directory is empty. The newly added unit test fails before this change and passes after it. Signed-off-by: William Chargin <wchargin@gmail.com> --- t/t0000-basic.sh | 29 +++++++++++++++++++++++++++++ t/test-lib-functions.sh | 2 +- 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh index 34859fe4a..3885b26f9 100755 --- a/t/t0000-basic.sh +++ b/t/t0000-basic.sh @@ -821,6 +821,35 @@ test_expect_success 'tests clean up even on failures' " EOF " +test_expect_success '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 && + printf \"pathological_dir/.\\\\n.\\\\0\" | xargs -0 touch && + 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..f7ff28ef6 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 -c)" != 0 then echo "Directory '$1' is not empty, it contains:" ls -la "$1" -- 2.18.0.548.g79b975644 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 1/1] t/test-lib: make `test_dir_is_empty` more robust 2018-08-05 2:20 ` [PATCH 1/1] " William Chargin @ 2018-08-05 4:19 ` Jonathan Nieder 2018-08-05 5:23 ` Eric Sunshine ` (2 more replies) 0 siblings, 3 replies; 23+ messages in thread From: Jonathan Nieder @ 2018-08-05 4:19 UTC (permalink / raw) To: William Chargin; +Cc: git Hi, William Chargin wrote: > Subject: t/test-lib: make `test_dir_is_empty` more robust This subject line will appear out of context in "git shortlog" output, so it's useful to pack in enough information to briefly summarize what the change does. How about something like test_dir_is_empty: avoid being confused by $'.\n.' file or test_dir_is_empty: simplify by using "ls --almost-all" ? [...] > + test_expect_success 'should fail with dot-newline-dot filename' ' > + mkdir pathological_dir && > + printf \"pathological_dir/.\\\\n.\\\\0\" | xargs -0 touch && I don't think "xargs -0" is present on all supported platforms. I'd be tempted to use >pathological_dir/$'.\n.' but $'' is too recent of a shell feature to count on (e.g., dash doesn't support it). See t/t3600-rm.sh for an example of a portable way to do this. Not all filesystems support newlines in filenames. I think we'd want to factor out the FUNNYNAMES prereq from there into a test_lazy_prereq so this test can be skipped on such filenames. [...] > --- 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 -c)" != 0 Another portability gotcha: wc output includes a space on Mac so this test would always return true there. How about if test -n "$(ls -A1 "$1")" ? "ls -A" was added in POSIX.1-2017. Its rationale section explains Earlier versions of this standard did not describe the BSD -A option (like -a, but dot and dot-dot are not written out). It has been added due to widespread implementation. That's very recent, but the widespread implementation it mentions is less so. This would be our first use of "ls -A", so I'd be interested to hear from people on more obscure platforms. It does seem to be widespread. Can you say a little more about where you ran into this? Did you discover it by code inspection? I do think the resulting implementation using -A is simpler. Thanks for writing it. Thanks and hope that helps, Jonathan ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/1] t/test-lib: make `test_dir_is_empty` more robust 2018-08-05 4:19 ` Jonathan Nieder @ 2018-08-05 5:23 ` Eric Sunshine 2018-08-05 20:52 ` Jeff King 2018-08-05 5:24 ` William Chargin 2018-08-05 6:03 ` Junio C Hamano 2 siblings, 1 reply; 23+ messages in thread From: Eric Sunshine @ 2018-08-05 5:23 UTC (permalink / raw) To: Jonathan Nieder; +Cc: wchargin, Git List On Sun, Aug 5, 2018 at 12:20 AM Jonathan Nieder <jrnieder@gmail.com> wrote: > William Chargin wrote: > > test_dir_is_empty () { > > test_path_is_dir "$1" && > > - if test -n "$(ls -a1 "$1" | egrep -v '^\.\.?$')" > > + if test "$(ls -A1 "$1" | wc -c)" != 0 > > Another portability gotcha: wc output includes a space on Mac so this > test would always return true there. How about > > if test -n "$(ls -A1 "$1")" > > "ls -A" was added in POSIX.1-2017. [...] > That's very recent, but the widespread implementation it mentions is > less so. This would be our first use of "ls -A", so I'd be interested > to hear from people on more obscure platforms. It does seem to be > widespread. A simpler approach, without the portability concerns of -A, would be to remove the "." and ".." lines from the top of the listing: ls -f1 "$1" | sed '1,2d' If we're worried about -f not being sufficiently portable, then an even simpler approach would be to check whether the output of 'ls -a1' has more lines than the two expected ("." and ".."): test $(ls -a1 "$1" | wc -l) -gt 2 I think I favor this final implementation over the others. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/1] t/test-lib: make `test_dir_is_empty` more robust 2018-08-05 5:23 ` Eric Sunshine @ 2018-08-05 20:52 ` Jeff King 2018-08-06 13:02 ` Jeff King 0 siblings, 1 reply; 23+ messages in thread From: Jeff King @ 2018-08-05 20:52 UTC (permalink / raw) To: Eric Sunshine; +Cc: Jonathan Nieder, wchargin, Git List On Sun, Aug 05, 2018 at 01:23:05AM -0400, Eric Sunshine wrote: > A simpler approach, without the portability concerns of -A, would be > to remove the "." and ".." lines from the top of the listing: > > ls -f1 "$1" | sed '1,2d' > > If we're worried about -f not being sufficiently portable, then an > even simpler approach would be to check whether the output of 'ls -a1' > has more lines than the two expected ("." and ".."): > > test $(ls -a1 "$1" | wc -l) -gt 2 > > I think I favor this final implementation over the others. Perhaps even simpler: test "$1" = "$(find "$1")" That will recurse any subdirectories, possibly wasting time, but since the point is that we expect it to be empty, that's probably OK. Like Junio said elsewhere, I am not sure if it is even worth caring too much about pathological cases in the test suite, where we control all of the paths. But using `find` does shave off one process invocation, too. :) -Peff ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/1] t/test-lib: make `test_dir_is_empty` more robust 2018-08-05 20:52 ` Jeff King @ 2018-08-06 13:02 ` Jeff King 2018-08-06 17:52 ` Eric Sunshine 0 siblings, 1 reply; 23+ messages in thread From: Jeff King @ 2018-08-06 13:02 UTC (permalink / raw) To: Eric Sunshine; +Cc: Jonathan Nieder, wchargin, Git List On Sun, Aug 05, 2018 at 04:52:31PM -0400, Jeff King wrote: > On Sun, Aug 05, 2018 at 01:23:05AM -0400, Eric Sunshine wrote: > > > A simpler approach, without the portability concerns of -A, would be > > to remove the "." and ".." lines from the top of the listing: > > > > ls -f1 "$1" | sed '1,2d' > > > > If we're worried about -f not being sufficiently portable, then an > > even simpler approach would be to check whether the output of 'ls -a1' > > has more lines than the two expected ("." and ".."): > > > > test $(ls -a1 "$1" | wc -l) -gt 2 > > > > I think I favor this final implementation over the others. > > Perhaps even simpler: > > test "$1" = "$(find "$1")" Actually, I guess it needs to add "-print", since IIRC that is not the default on some old versions of "find". -Peff ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/1] t/test-lib: make `test_dir_is_empty` more robust 2018-08-06 13:02 ` Jeff King @ 2018-08-06 17:52 ` Eric Sunshine 2018-08-12 4:06 ` [PATCH v3] test_dir_is_empty: properly detect files with newline in name William Chargin 2018-08-12 4:06 ` [PATCH 1/1] t/test-lib: make `test_dir_is_empty` more robust William Chargin 0 siblings, 2 replies; 23+ messages in thread From: Eric Sunshine @ 2018-08-06 17:52 UTC (permalink / raw) To: Jeff King; +Cc: Jonathan Nieder, William Chargin, Git List On Mon, Aug 6, 2018 at 9:02 AM Jeff King <peff@peff.net> wrote: > On Sun, Aug 05, 2018 at 04:52:31PM -0400, Jeff King wrote: > > Perhaps even simpler: > > > > test "$1" = "$(find "$1")" > > Actually, I guess it needs to add "-print", since IIRC that is not the > default on some old versions of "find". You recall correctly. I tend to avoid 'find' in scripts when I don't need its recursive behavior due to *extremely* poor performance on Windows (at least that was the case years ago), especially when there are a lot of files in the directory being listed. However, for this usage, we expect the directory to be empty, so perhaps performance isn't an issue. ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v3] test_dir_is_empty: properly detect files with newline in name 2018-08-06 17:52 ` Eric Sunshine @ 2018-08-12 4:06 ` William Chargin 2018-08-12 6:17 ` Eric Sunshine 2018-08-12 4:06 ` [PATCH 1/1] t/test-lib: make `test_dir_is_empty` more robust William Chargin 1 sibling, 1 reply; 23+ messages in thread From: William Chargin @ 2018-08-12 4:06 UTC (permalink / raw) To: git, jrnieder; +Cc: William Chargin, sunshine, peff 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 <wchargin@gmail.com> --- 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/t0000-basic.sh | 31 +++++++++++++++++++++++++++++++ t/test-lib-functions.sh | 2 +- 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh index 34859fe4a..60134d7ab 100755 --- a/t/t0000-basic.sh +++ b/t/t0000-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 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v3] test_dir_is_empty: properly detect files with newline in name 2018-08-12 4:06 ` [PATCH v3] test_dir_is_empty: properly detect files with newline in name William Chargin @ 2018-08-12 6:17 ` Eric Sunshine 2018-08-12 6:32 ` William Chargin 0 siblings, 1 reply; 23+ messages in thread From: Eric Sunshine @ 2018-08-12 6:17 UTC (permalink / raw) To: William Chargin; +Cc: Git List, Jonathan Nieder, Jeff King On Sun, Aug 12, 2018 at 12:07 AM William Chargin <wchargin@gmail.com> 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 <wchargin@gmail.com> > --- > diff --git a/t/t0000-basic.sh b/t/t0000-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. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3] test_dir_is_empty: properly detect files with newline in name 2018-08-12 6:17 ` Eric Sunshine @ 2018-08-12 6:32 ` William Chargin 2018-08-12 6:44 ` Eric Sunshine 0 siblings, 1 reply; 23+ messages in thread From: William Chargin @ 2018-08-12 6:32 UTC (permalink / raw) To: Eric Sunshine; +Cc: Git List, Jonathan Nieder, Jeff King 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3] test_dir_is_empty: properly detect files with newline in name 2018-08-12 6:32 ` William Chargin @ 2018-08-12 6:44 ` Eric Sunshine 2018-09-12 18:35 ` [PATCH v4] test_dir_is_empty: fix edge cases with newlines and hyphens William Chargin 2018-09-12 18:37 ` William Chargin 0 siblings, 2 replies; 23+ messages in thread From: Eric Sunshine @ 2018-08-12 6:44 UTC (permalink / raw) To: William Chargin; +Cc: Git List, Jonathan Nieder, Jeff King On Sun, Aug 12, 2018 at 2:33 AM William Chargin <wchargin@gmail.com> 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(). ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v4] test_dir_is_empty: fix edge cases with newlines and hyphens 2018-08-12 6:44 ` Eric Sunshine @ 2018-09-12 18:35 ` William Chargin 2018-09-12 19:50 ` Junio C Hamano 2018-09-12 18:37 ` William Chargin 1 sibling, 1 reply; 23+ messages in thread From: William Chargin @ 2018-09-12 18:35 UTC (permalink / raw) To: sunshine; +Cc: git, jrnieder, peff, wchargin While the `test_dir_is_empty` function appears correct in most normal use cases, it can improperly pass if a directory contains a filename with a newline, and can improperly fail if an empty directory looks like an argument to `ls`. 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, and adds the `--` delimiter before the directory name when invoking `ls`. The newly added unit test fails before this change and passes after it. Signed-off-by: William Chargin <wchargin@gmail.com> --- This patch depends on "t: factor out FUNNYNAMES as shared lazy prereq" (2018-08-06), which is now in master. 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. Tested on GNU/Linux (Mint 18.2) and macOS (10.13). t/t0000-basic.sh | 43 +++++++++++++++++++++++++++++++++++++++++ t/test-lib-functions.sh | 2 +- 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh index 850f651e4e..a5c57c6aa5 100755 --- a/t/t0000-basic.sh +++ b/t/t0000-basic.sh @@ -821,6 +821,49 @@ 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 && + >nonempty_dir/some_file && + ! test_dir_is_empty nonempty_dir + ' + test_expect_success 'should fail with dot-newline-dot filename' ' + mkdir pathological_dir && + >\"pathological_dir/. + .\" && + ! test_dir_is_empty pathological_dir + ' + test_expect_success 'should pass with an empty directory \"-l\"' ' + mkdir -- -l && + test_dir_is_empty -l && + rmdir -- -l + ' + test_expect_success 'should pass with an empty directory \"--wat\"' ' + mkdir -- --wat && + test_dir_is_empty --wat && + rmdir -- --wat + ' + 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 + > ok 4 - should pass with an empty directory \"-l\" + > ok 5 - should pass with an empty directory \"--wat\" + > # passed all 5 test(s) + > 1..5 + EOF +" + + ################################################################ # Basics of the basics diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index 4207af4077..3df6b8027f 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -576,7 +576,7 @@ test_path_exists () { # 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.549.gd66323a05 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v4] test_dir_is_empty: fix edge cases with newlines and hyphens 2018-09-12 18:35 ` [PATCH v4] test_dir_is_empty: fix edge cases with newlines and hyphens William Chargin @ 2018-09-12 19:50 ` Junio C Hamano 0 siblings, 0 replies; 23+ messages in thread From: Junio C Hamano @ 2018-09-12 19:50 UTC (permalink / raw) To: William Chargin; +Cc: sunshine, git, jrnieder, peff William Chargin <wchargin@gmail.com> writes: > While the `test_dir_is_empty` function appears correct in most normal > use cases, it can improperly pass if a directory contains a filename > with a newline, and can improperly fail if an empty directory looks like > an argument to `ls`. 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, and adds the `--` delimiter before the > directory name when invoking `ls`. AFIAK dot and dot-dot are allowed not to exist; "at most two" is not a good test. Quite honestly, our tests are still run inside a sort-of controlled environment, so if it _requires_ use of things we have avoided depending on, like "ls -A" and "xargs -0", or the fact that most filesystems always have "." and ".." even in an empty directory, in order to be resistant to funnily-named files like dot-LF-dot, I would say it is not worth worrying about these funny names--instead we can simply refrain from using such a pathological name, can't we? In other words, is there a real-world need in the context of our test suite for this change? Also, I find that its support for directories whose names begin with a dash red-herring. All the test scripts in our test suite knows that they can prefix "./" to avoid problems, i.e. test_dir_is_empty ./--wat So it appears that the only problematic case is when we create a directory, create a file or a directory whose name is dot-LF-dot and nothing else, and then do something that ought to cause that file to disappear, and make sure that the directory is empty, e.g. mkdir empty && echo foo >"empty/$dotLFdot" && git add "empty/$dotLFdot" && git reset --hard && test_dir_is_empty empty We do want to make sure funny names can be added with "git add" and "git reset --hard" to HEAD that lacked those paths with funny names to remove them correctly. But the funny names used in such a test do not have to be $dotLFdot; you can use "${dotLFdot}X" instead in the above and can ensure whatever the original test wanted to ensure. So... ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v4] test_dir_is_empty: fix edge cases with newlines and hyphens 2018-08-12 6:44 ` Eric Sunshine 2018-09-12 18:35 ` [PATCH v4] test_dir_is_empty: fix edge cases with newlines and hyphens William Chargin @ 2018-09-12 18:37 ` William Chargin 1 sibling, 0 replies; 23+ messages in thread From: William Chargin @ 2018-09-12 18:37 UTC (permalink / raw) To: git, sunshine; +Cc: William Chargin, jrnieder, peff While the `test_dir_is_empty` function appears correct in most normal use cases, it can improperly pass if a directory contains a filename with a newline, and can improperly fail if an empty directory looks like an argument to `ls`. 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, and adds the `--` delimiter before the directory name when invoking `ls`. The newly added unit test fails before this change and passes after it. Signed-off-by: William Chargin <wchargin@gmail.com> --- This patch depends on "t: factor out FUNNYNAMES as shared lazy prereq" (2018-08-06), which is now in master. 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. Tested on GNU/Linux (Mint 18.2) and macOS (10.13). t/t0000-basic.sh | 43 +++++++++++++++++++++++++++++++++++++++++ t/test-lib-functions.sh | 2 +- 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh index 850f651e4e..a5c57c6aa5 100755 --- a/t/t0000-basic.sh +++ b/t/t0000-basic.sh @@ -821,6 +821,49 @@ 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 && + >nonempty_dir/some_file && + ! test_dir_is_empty nonempty_dir + ' + test_expect_success 'should fail with dot-newline-dot filename' ' + mkdir pathological_dir && + >\"pathological_dir/. + .\" && + ! test_dir_is_empty pathological_dir + ' + test_expect_success 'should pass with an empty directory \"-l\"' ' + mkdir -- -l && + test_dir_is_empty -l && + rmdir -- -l + ' + test_expect_success 'should pass with an empty directory \"--wat\"' ' + mkdir -- --wat && + test_dir_is_empty --wat && + rmdir -- --wat + ' + 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 + > ok 4 - should pass with an empty directory \"-l\" + > ok 5 - should pass with an empty directory \"--wat\" + > # passed all 5 test(s) + > 1..5 + EOF +" + + ################################################################ # Basics of the basics diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index 4207af4077..3df6b8027f 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -576,7 +576,7 @@ test_path_exists () { # 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.549.gd66323a05 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 1/1] t/test-lib: make `test_dir_is_empty` more robust 2018-08-06 17:52 ` Eric Sunshine 2018-08-12 4:06 ` [PATCH v3] test_dir_is_empty: properly detect files with newline in name William Chargin @ 2018-08-12 4:06 ` William Chargin 1 sibling, 0 replies; 23+ messages in thread From: William Chargin @ 2018-08-12 4:06 UTC (permalink / raw) To: Eric Sunshine; +Cc: Jeff King, Jonathan Nieder, Git List > That will recurse any subdirectories, possibly wasting time, but since > the point is that we expect it to be empty, that's probably OK. One caveat involves invocations of `test_must_fail test_dir_is_empty`, wherein we _don't_ actually expect the directory to be empty. It looks like there might not be any such invocations in Git, though, for what it's worth. The solution that counts the lines of `ls -a` seems like it should be universally portable, both in POSIX and in practice, and it doesn't complicate the code at all. (I'd say that it's slightly simpler than the `egrep` approach in the status quo.) I've adjusted the patch to use Eric's version, which I like a bit more than mine; I'll send out the revised commit presently. It's true that within Git we can simply try to avoid creating pathological file names. But Sharness is also exposed as a standalone library, and it seems clear that the public function therein should be correct in all cases. Assuming that the new implementation is sufficiently inoffensive, is there any harm in being perhaps a bit more robust than is practically needed, and also maintaining consistency with Sharness? Thanks for all the suggestions and feedback so far! Best, WC ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/1] t/test-lib: make `test_dir_is_empty` more robust 2018-08-05 4:19 ` Jonathan Nieder 2018-08-05 5:23 ` Eric Sunshine @ 2018-08-05 5:24 ` William Chargin 2018-08-05 6:34 ` Jonathan Nieder 2018-08-05 6:03 ` Junio C Hamano 2 siblings, 1 reply; 23+ messages in thread From: William Chargin @ 2018-08-05 5:24 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Git Mailing List Hi, > This subject line will appear out of context in "git shortlog" output, > so it's useful to pack in enough information to briefly summarize what > the change does. I'm happy to do so. I think that "simplify" is misleading, because this is a bug fix, not a refactoring. I like your first suggestion, though it exceeds the 50-character soft limit. What do you think of: test_dir_is_empty: find files with newline in name ? > I don't think "xargs -0" is present on all supported platforms Wow---I'm shocked that it's not POSIX, but you're right. (That makes `xargs` so much less useful!) t/t3600-rm.sh seems to just literally embed the newline into the argument to `touch`. I can do that. (I intentionally avoided $'' for the reason that you mention.) > Not all filesystems support newlines in filenames. I think we'd want > to factor out the FUNNYNAMES prereq from there into a test_lazy_prereq > so this test can be skipped on such filenames. This makes sense. Would you like me to send in a separate patch to add this `test_lazy_prereq` to `t/test-lib.sh`, fixing up existing uses (of which there are several), and then rebase this patch on top of it? > Another portability gotcha: wc output includes a space on Mac so this > test would always return true there. That's good to know. I can use `test -n "$(ls -A1 "$1")"` as you suggest, although... > "ls -A" was added in POSIX.1-2017. [...] > That's very recent, but the widespread implementation it mentions is > less so. This would be our first use of "ls -A", so I'd be interested > to hear from people on more obscure platforms. It does seem to be > widespread. ...if you prefer, a variant of `test "$(ls -a1 | wc -l)" -eq 2` should satisfy all the crtieria that you mention above (POSIX since forever, should work on Mac). The assumption is that a file with a newline character may take up more than one line, but every file must take up _at least_ one line, and we get two lines from `.` and `..`. If this assumption is false, then I will have learned yet another new thing! > Can you say a little more about where you ran into this? Did you > discover it by code inspection? Sure. Yes. I have a build script that accepts a target output directory, and rejects if the directory is nonempty. I used `ls -A | wc -l` to implement this check. When testing the script with Sharness, I ran across `test_dir_is_empty`. I was curious about the implementation, having recently implemented the same test myself. The `egrep` in the implementation stood out to me as suspicious, and so it was easy to come up with an explicit counterexample. > I do think the resulting implementation using -A is simpler. Thanks > for writing it. You're welcome. Thank you for the detailed and helpful review. Best, WC ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/1] t/test-lib: make `test_dir_is_empty` more robust 2018-08-05 5:24 ` William Chargin @ 2018-08-05 6:34 ` Jonathan Nieder 0 siblings, 0 replies; 23+ messages in thread From: Jonathan Nieder @ 2018-08-05 6:34 UTC (permalink / raw) To: William Chargin; +Cc: Git Mailing List William Chargin wrote: > Jonathan Nieder wrote: >> This subject line will appear out of context in "git shortlog" output, >> so it's useful to pack in enough information to briefly summarize what >> the change does. > > I'm happy to do so. I think that "simplify" is misleading, because this > is a bug fix, not a refactoring. I like your first suggestion, though it > exceeds the 50-character soft limit. What do you think of: > > test_dir_is_empty: find files with newline in name Thanks. I think we can ignore the 50-character soft limit. It's often too low and expressivity is more important. So if you like my first suggestion, then I'd say start with that and tweak to your taste from there. [...] >> I don't think "xargs -0" is present on all supported platforms > > Wow---I'm shocked that it's not POSIX, but you're right. (That makes > `xargs` so much less useful!) To be clear, as Junio mentioned, POSIX is useful as a guide to what *might* be portable, but what we care about is what is portable to the platforms used in practice. [...] >> Not all filesystems support newlines in filenames. I think we'd want >> to factor out the FUNNYNAMES prereq from there into a test_lazy_prereq >> so this test can be skipped on such filenames. > > This makes sense. Would you like me to send in a separate patch to add > this `test_lazy_prereq` to `t/test-lib.sh`, fixing up existing uses (of > which there are several), and then rebase this patch on top of it? Thanks for the offer! Yes, that would be nice: such a patch would be nice as a cleanup in its own right, too. Such a patch would be helpful at any time. For rebasing this patch, my advice would be to hold off until the discussion has settled down a bit. If we're lucky, people in other time zones might have an idea beyond the ones we've come up with. [...] >> "ls -A" was added in POSIX.1-2017. [...] >> That's very recent, but the widespread implementation it mentions is >> less so. This would be our first use of "ls -A", so I'd be interested >> to hear from people on more obscure platforms. It does seem to be >> widespread. > > ...if you prefer, a variant of `test "$(ls -a1 | wc -l)" -eq 2` should > satisfy all the crtieria that you mention above (POSIX since forever, > should work on Mac). The assumption is that a file with a newline > character may take up more than one line, but every file must take up > _at least_ one line, and we get two lines from `.` and `..`. If this > assumption is false, then I will have learned yet another new thing! As a piece of trivia, '.' and '..' are allowed not to exist. So this test can have false negatives and false positives. Filesystems omitting them are quite rare so this might work reasonably well in practice, though. Thanks, Jonathan ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/1] t/test-lib: make `test_dir_is_empty` more robust 2018-08-05 4:19 ` Jonathan Nieder 2018-08-05 5:23 ` Eric Sunshine 2018-08-05 5:24 ` William Chargin @ 2018-08-05 6:03 ` Junio C Hamano 2018-08-05 6:23 ` Jonathan Nieder 2 siblings, 1 reply; 23+ messages in thread From: Junio C Hamano @ 2018-08-05 6:03 UTC (permalink / raw) To: Jonathan Nieder; +Cc: William Chargin, git Jonathan Nieder <jrnieder@gmail.com> writes: > but $'' is too recent of a shell feature to count on (e.g., dash doesn't > support it). See t/t3600-rm.sh for an example of a portable way to do Is that "too recent"? I thought it was bashism, not even in POSIX, but I may be mistaken. Quite honestly, our tests are still run inside a sort-of controlled environment, so if it _requires_ use of things we have avoided so far, like "ls -A" and "xargs -0", in order to be resistant to funnyly-named files like dot-LF-dot, I would say it is not worth worrying about them--instead we can simply refrain from using such a pathological name, can't we? "ls -A" may be in POSIX, but our attitude generally is to avoid saying things like "it is in POSIX so it's your platform's fault that it is not yet supported". We instead say "even it may be in POSIX, in real life many people don't have it, so let's avoid it". And "xargs -0" I do not think is. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/1] t/test-lib: make `test_dir_is_empty` more robust 2018-08-05 6:03 ` Junio C Hamano @ 2018-08-05 6:23 ` Jonathan Nieder 0 siblings, 0 replies; 23+ messages in thread From: Jonathan Nieder @ 2018-08-05 6:23 UTC (permalink / raw) To: Junio C Hamano; +Cc: William Chargin, git Hi, Junio C Hamano wrote: > Jonathan Nieder <jrnieder@gmail.com> writes: >> but $'' is too recent of a shell feature to count on (e.g., dash doesn't >> support it). See t/t3600-rm.sh for an example of a portable way to do > > Is that "too recent"? I thought it was bashism, not even in POSIX, > but I may be mistaken. You're right. I got a little ahead of myself: it's not part of POSIX yet but is likely to be so once the details get ironed out: http://austingroupbugs.net/view.php?id=249 > Quite honestly, our tests are still run inside a sort-of controlled > environment, so if it _requires_ use of things we have avoided so > far, like "ls -A" and "xargs -0", in order to be resistant to > funnyly-named files like dot-LF-dot, I would say it is not worth > worrying about them--instead we can simply refrain from using such a > pathological name, can't we? The "xargs -0" is a bit of a red herring. That construct is definitely not needed for the test it was used in. For "ls -A", I agree with you that the benefit is not very high, so the cost would have to be pretty low for this to be worth it. But given the lineage of "ls -A", I feel there's a chance that it's widespread enough that it would meet that bar. > "ls -A" may be in POSIX, but our attitude generally is to avoid > saying things like "it is in POSIX so it's your platform's fault > that it is not yet supported". We instead say "even it may be in > POSIX, in real life many people don't have it, so let's avoid it". > And "xargs -0" I do not think is. Indeed. Thanks, Jonathan ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 0/1] t/test-lib: make `test_dir_is_empty` more robust 2018-08-05 2:20 [PATCH 0/1] t/test-lib: make `test_dir_is_empty` more robust William Chargin 2018-08-05 2:20 ` [PATCH 1/1] " William Chargin @ 2018-08-05 3:36 ` Jonathan Nieder 2018-08-05 4:19 ` William Chargin 2018-08-05 4:20 ` [PATCH v2] " William Chargin 1 sibling, 2 replies; 23+ messages in thread From: Jonathan Nieder @ 2018-08-05 3:36 UTC (permalink / raw) To: William Chargin; +Cc: git Hi, 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 information belongs in the commit message, since it's useful context for understanding the motivation behind the patch when encountering it with e.g. "git log". That's part of why I recommend never sending a separate cover-letter email for a single-patch series. See [1] from Documentation/SubmittingPatches for more on this subject. > 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. Thanks for writing it! Continuing the note about administrivia, this kind of cover letter material that you want to not be part of the commit message can go below the three-dashes delimiter when you send a patch. There's more about this at [2]. Thanks again, Jonathan [1] https://www.kernel.org/pub/software/scm/git/docs/SubmittingPatches.html#describe-changes [2] https://www.kernel.org/pub/software/scm/git/docs/SubmittingPatches.html#send-patches ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 0/1] t/test-lib: make `test_dir_is_empty` more robust 2018-08-05 3:36 ` [PATCH 0/1] " Jonathan Nieder @ 2018-08-05 4:19 ` William Chargin 2018-08-05 4:20 ` [PATCH v2] " William Chargin 1 sibling, 0 replies; 23+ messages in thread From: William Chargin @ 2018-08-05 4:19 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Git Mailing List Hi Jonathan, > This information belongs in the commit message Agreed: I included it at the start of the commit message, though I suppose that the wording in the cover letter is clearer, so I've amended the commit to use that wording instead. > Continuing the note about administrivia, this > kind of cover letter material that you want to not be part of the > commit message can go below the three-dashes delimiter when you send a > patch. Perfect; this is what I was missing. Thanks. Let me try again. :-) ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2] t/test-lib: make `test_dir_is_empty` more robust 2018-08-05 3:36 ` [PATCH 0/1] " Jonathan Nieder 2018-08-05 4:19 ` William Chargin @ 2018-08-05 4:20 ` William Chargin 2018-08-05 8:34 ` Johannes Sixt 1 sibling, 1 reply; 23+ messages in thread From: William Chargin @ 2018-08-05 4:20 UTC (permalink / raw) To: jrnieder; +Cc: git, wchargin 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 use `ls -A`, which is specified by POSIX. The output should be empty exactly if the directory is empty. The newly added unit test fails before this change and passes after it. Signed-off-by: William Chargin <wchargin@gmail.com> --- 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/t0000-basic.sh | 29 +++++++++++++++++++++++++++++ t/test-lib-functions.sh | 2 +- 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh index 34859fe4a..3885b26f9 100755 --- a/t/t0000-basic.sh +++ b/t/t0000-basic.sh @@ -821,6 +821,35 @@ test_expect_success 'tests clean up even on failures' " EOF " +test_expect_success '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 && + printf \"pathological_dir/.\\\\n.\\\\0\" | xargs -0 touch && + 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..f7ff28ef6 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 -c)" != 0 then echo "Directory '$1' is not empty, it contains:" ls -la "$1" -- 2.18.0.548.geb6c14151 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v2] t/test-lib: make `test_dir_is_empty` more robust 2018-08-05 4:20 ` [PATCH v2] " William Chargin @ 2018-08-05 8:34 ` Johannes Sixt 0 siblings, 0 replies; 23+ messages in thread From: Johannes Sixt @ 2018-08-05 8:34 UTC (permalink / raw) To: William Chargin; +Cc: jrnieder, git Am 05.08.2018 um 06:20 schrieb William Chargin: > 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 use `ls -A`, which is specified by POSIX. > The output should be empty exactly if the directory is empty. > > The newly added unit test fails before this change and passes after it. > > Signed-off-by: William Chargin <wchargin@gmail.com> > --- > > 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/t0000-basic.sh | 29 +++++++++++++++++++++++++++++ > t/test-lib-functions.sh | 2 +- > 2 files changed, 30 insertions(+), 1 deletion(-) > > diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh > index 34859fe4a..3885b26f9 100755 > --- a/t/t0000-basic.sh > +++ b/t/t0000-basic.sh > @@ -821,6 +821,35 @@ test_expect_success 'tests clean up even on failures' " > EOF > " > > +test_expect_success '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 && > + printf \"pathological_dir/.\\\\n.\\\\0\" | xargs -0 touch && > + 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..f7ff28ef6 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 -c)" != 0 > then > echo "Directory '$1' is not empty, it contains:" > ls -la "$1" > Did you accidentally resend the same patch in this v2? I can't spot the difference to v1. -- Hannes ^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2018-09-12 19:50 UTC | newest] Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-08-05 2:20 [PATCH 0/1] t/test-lib: make `test_dir_is_empty` more robust William Chargin 2018-08-05 2:20 ` [PATCH 1/1] " William Chargin 2018-08-05 4:19 ` Jonathan Nieder 2018-08-05 5:23 ` Eric Sunshine 2018-08-05 20:52 ` Jeff King 2018-08-06 13:02 ` Jeff King 2018-08-06 17:52 ` Eric Sunshine 2018-08-12 4:06 ` [PATCH v3] test_dir_is_empty: properly detect files with newline in name William Chargin 2018-08-12 6:17 ` Eric Sunshine 2018-08-12 6:32 ` William Chargin 2018-08-12 6:44 ` Eric Sunshine 2018-09-12 18:35 ` [PATCH v4] test_dir_is_empty: fix edge cases with newlines and hyphens William Chargin 2018-09-12 19:50 ` Junio C Hamano 2018-09-12 18:37 ` William Chargin 2018-08-12 4:06 ` [PATCH 1/1] t/test-lib: make `test_dir_is_empty` more robust William Chargin 2018-08-05 5:24 ` William Chargin 2018-08-05 6:34 ` Jonathan Nieder 2018-08-05 6:03 ` Junio C Hamano 2018-08-05 6:23 ` Jonathan Nieder 2018-08-05 3:36 ` [PATCH 0/1] " Jonathan Nieder 2018-08-05 4:19 ` William Chargin 2018-08-05 4:20 ` [PATCH v2] " William Chargin 2018-08-05 8:34 ` Johannes Sixt
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).