Re: [PATCH v2 1/1] t3600: use test_path_is_dir and test_path_is_file
- Date: Tue, 26 Feb 2019 17:37:37 +0100
- From: SZEDER Gábor <szeder.dev@xxxxxxxxx>
- Subject: Re: [PATCH v2 1/1] t3600: use test_path_is_dir and test_path_is_file
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.
This patch will ...
we would usually write something like this:
Replace 'test -(d|f)' calls in t3600 with the corresponding helper
> 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@xxxxxxxxx>
> t/t3600-rm.sh | 96 +++++++++++++++++++++++++--------------------------
> 1 file changed, 48 insertions(+), 48 deletions(-)
The patch itself seems to be a straightforward application of
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.