Re: [PATCH v3 1/1] t3600: use test_path_is_* functions

[Duy Nguyen <pclouds@gmail.com>]

On Wed, Feb 27, 2019 at 5:49 AM Rohit Ashiwal via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
>
> Replace `test -(d|f|e)` calls in t3600-rm.sh
>
> Previously we were using `test -(d|f|e)` to verify
> the presence of a directory/file, but we already
> have helper functions, viz, `test_path_is_dir`,
> `test_path_is_file` and `test_path_is_missing`
> with better functionality.
>
> These helper functions make code more readable
> and informative to someone new to code, also
> these functions have better error messages
>
> Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
> ---
>  t/t3600-rm.sh | 138 +++++++++++++++++++++++++-------------------------
>  1 file changed, 69 insertions(+), 69 deletions(-)
>
> diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
> index 04e5d42bd3..ad638490ac 100755
> --- a/t/t3600-rm.sh
> +++ b/t/t3600-rm.sh
> @@ -83,7 +83,7 @@ test_expect_success \
>
>  test_expect_success \
>      'Post-check that bar does not exist and is not in index after "git rm -f bar"' \
> -    '! [ -f bar ] && test_must_fail git ls-files --error-unmatch bar'
> +    'test_path_is_missing bar && test_must_fail git ls-files --error-unmatch bar'

This line should be broken down in two. It was reasonably short
before, but now getting long and two checks in one line seem easy to
miss.

I was a bit worried that the "test ! something" could be incorrectly
converted because for example, "test ! -d foo" is not always the same
as "test_path_is_missing". If "foo" is intended to be a file, then the
conversion is wrong.

But I don't think you made any wrong conversion here. All these
negative "test" are preceded by "git rm" so the expectation is always
"test ! -e".
-- 
Duy