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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Aug 5, 2018, 04:19 UTC
Message-ID: <20180805041956.GI258270@aiede.svl.corp.google.com>
In-Reply-To: <20180805022002.28907-2-wchargin@gmail.com>

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.

[...]

Show 8 quoted lines

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