Re: [PATCH v2 1/1] t3600: use test_path_is_dir and test_path_is_file

["SZEDER Gábor" <szeder.dev@gmail.com>]

Hi and welcome!

On Tue, Feb 26, 2019 at 06:26:09AM -0800, Rohit Ashiwal via GitGitGadget wrote:
> Previously we were using `test -(d|f)` to verify
> the presence of a directory/file, but we already
> have helper functions, viz, `test_path_is_dir`
> and `test_path_is_file` with better functionality.
> This patch will replace `test -(d|f)` calls in t3660.sh

We prefer to use imperative mode when talking about what a patch does,
as if the author were to give orders to the code base.  So e.g.
instead of

  This patch will ...

we would usually write something like this:

  Replace 'test -(d|f)' calls in t3600 with the corresponding helper
  functions.

> 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 | 96 +++++++++++++++++++++++++--------------------------
>  1 file changed, 48 insertions(+), 48 deletions(-)

The patch itself seems to be a straightforward application of

  s/test -f/test_path_is_file/
  s/test -d/test_path_is_dir/

so it looks good for the most part, but it has a few issues:

>  test_expect_success 'Recursive with -r -f' '
>  	git rm -f -r frotz &&
> -	! test -f frotz/nitfol &&
> -	! test -d frotz
> +	! test_path_is_file frotz/nitfol &&
> +	! test_path_is_dir frotz
>  '

These should rather use the test_path_is_missing helper function.

However, if the directory 'frotz' is missing, then surely
'frotz/nitfol' could not possibly exist either, could it?  I'm not
sure why this test (and a couple of others) checks both, and wonder
whether the redundant check for the file inside the supposedly
non-existing directory could be removed.


Furthermore, there are a couple of place where the '!' is not in front
of the whole 'test' command but is given as an argument, e.g.:

  test ! -f file

Please convert those cases as well.