git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jonathan Nieder <jrnieder@gmail.com>
To: William Chargin <wchargin@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 1/1] t/test-lib: make `test_dir_is_empty` more robust
Date: Sat, 4 Aug 2018 21:19:56 -0700	[thread overview]
Message-ID: <20180805041956.GI258270@aiede.svl.corp.google.com> (raw)
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.

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

  reply	other threads:[~2018-08-05  4:20 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]
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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180805041956.GI258270@aiede.svl.corp.google.com \
    --to=jrnieder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=wchargin@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).