git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Eric Sunshine <sunshine@sunshineco.com>
To: William Chargin <wchargin@gmail.com>
Cc: Git List <git@vger.kernel.org>,
	Jonathan Nieder <jrnieder@gmail.com>, Jeff King <peff@peff.net>
Subject: Re: [PATCH v3] test_dir_is_empty: properly detect files with newline in name
Date: Sun, 12 Aug 2018 02:17:52 -0400	[thread overview]
Message-ID: <CAPig+cTca5YsXeXYbEEuq8Y_GraUYWwzdKO7r+PR9=JRTR_xFA@mail.gmail.com> (raw)
In-Reply-To: <20180812040620.15298-1-wchargin@gmail.com>

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.

  reply	other threads:[~2018-08-12  6:18 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
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 [this message]
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

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

  git send-email \
    --in-reply-to='CAPig+cTca5YsXeXYbEEuq8Y_GraUYWwzdKO7r+PR9=JRTR_xFA@mail.gmail.com' \
    --to=sunshine@sunshineco.com \
    --cc=git@vger.kernel.org \
    --cc=jrnieder@gmail.com \
    --cc=peff@peff.net \
    --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.
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 NNTP newsgroup(s).