From: William Chargin <wchargin@gmail.com>
Date: Sun, 12 Aug 2018 06:32:37 GMT
Subject: Re: [PATCH v3] test_dir_is_empty: properly detect files with newline in name
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