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

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Feb 26, 2019, 16:37 UTC
Message-ID: <20190226163737.GB19739@szeder.dev>
In-Reply-To: <fcafc87b382dfef00d8e33e875bcb8b03d5667e4.1551191168.git.gitgitgadget@gmail.com>

Hi and welcome!

On Tue, Feb 26, 2019 at 06:26:09AM -0800, Rohit Ashiwal via GitGitGadget wrote:

Show 5 quoted lines

> 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:

Show 7 quoted lines

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