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:44 UTC
Message-ID: <CAPig+cQSg-t6KGj3s0LJi+FU7LSQMNTmSHhMJJH=PgMUU9GWOA@mail.gmail.com>
In-Reply-To: <CAFW+GMDu-FVFXZeB=fV5a+HyGXea-UoMpZS90T9b2G_=xd3iLw@mail.gmail.com>

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

Show 7 quoted lines

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