[PATCH 0/1] t/test-lib: make `test_dir_is_empty` more robust

[PATCH 0/1] t/test-lib: make `test_dir_is_empty` more robust

[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

[PATCH 1/1] t/test-lib: make `test_dir_is_empty` more robust

[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

Re: [PATCH 1/1] t/test-lib: make `test_dir_is_empty` more robust

[Jonathan Nieder]

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

Re: [PATCH 1/1] t/test-lib: make `test_dir_is_empty` more robust

[Eric Sunshine]

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.

Re: [PATCH 1/1] t/test-lib: make `test_dir_is_empty` more robust

[Jeff King]

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

Re: [PATCH 1/1] t/test-lib: make `test_dir_is_empty` more robust

[Jeff King]

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

Re: [PATCH 1/1] t/test-lib: make `test_dir_is_empty` more robust

[Eric Sunshine]

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.

[PATCH v3] test_dir_is_empty: properly detect files with newline in name

[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 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

Re: [PATCH v3] test_dir_is_empty: properly detect files with newline in name

[Eric Sunshine]

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.

Re: [PATCH v3] test_dir_is_empty: properly detect files with newline in name

[William Chargin]

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

[Eric Sunshine]

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().

[PATCH v4] test_dir_is_empty: fix edge cases with newlines and hyphens

[William Chargin]

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

Re: [PATCH v4] test_dir_is_empty: fix edge cases with newlines and hyphens

[Junio C Hamano]

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...

[PATCH v4] test_dir_is_empty: fix edge cases with newlines and hyphens

[William Chargin]

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

Re: [PATCH 1/1] t/test-lib: make `test_dir_is_empty` more robust

[William Chargin]

> 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

Re: [PATCH 1/1] t/test-lib: make `test_dir_is_empty` more robust

[William Chargin]

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

Re: [PATCH 1/1] t/test-lib: make `test_dir_is_empty` more robust

[Jonathan Nieder]

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

Re: [PATCH 1/1] t/test-lib: make `test_dir_is_empty` more robust

[Junio C Hamano]

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.

Re: [PATCH 1/1] t/test-lib: make `test_dir_is_empty` more robust

[Jonathan Nieder]

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

Re: [PATCH 0/1] t/test-lib: make `test_dir_is_empty` more robust

[Jonathan Nieder]

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

Re: [PATCH 0/1] t/test-lib: make `test_dir_is_empty` more robust

[William Chargin]

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. :-)

[PATCH v2] t/test-lib: make `test_dir_is_empty` more robust

[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"
-- 
2.18.0.548.geb6c14151

Re: [PATCH v2] t/test-lib: make `test_dir_is_empty` more robust

[Johannes Sixt]

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