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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Aug 12, 2018, 06:17 UTC
Message-ID: <CAPig+cTca5YsXeXYbEEuq8Y_GraUYWwzdKO7r+PR9=JRTR_xFA@mail.gmail.com>
In-Reply-To: <20180812040620.15298-1-wchargin@gmail.com>

On Sun, Aug 12, 2018 at 12:07 AM William Chargin <wchargin@gmail.com> wrote:

Show 16 quoted lines

> 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

Show 5 quoted lines

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