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

From: William Chargin <wchargin@gmail.com>
Date: Aug 12, 2018, 06:32 UTC
Message-ID: <CAFW+GMDu-FVFXZeB=fV5a+HyGXea-UoMpZS90T9b2G_=xd3iLw@mail.gmail.com>
In-Reply-To: <CAPig+cTca5YsXeXYbEEuq8Y_GraUYWwzdKO7r+PR9=JRTR_xFA@mail.gmail.com>

Thanks for the review.

> We usually avoid "touch" unless the timestamp of the file is
> significant.

Makes sense. Will change as you suggest.

> 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 `!`. (Admittedly, the implementations are different, but only slightly: within Git, a Valgrind error 126 is a failure, not success.)

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?

(I'm nevertheless happy to change this as you suggest.)

I'll make these changes locally and hold on to the patch, waiting for others' input.

Best, WC