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

[Rohit Ashiwal <rohit.ashiwal265@gmail.com>]

Hi SZEDER

On Tue, Feb 26, 2019 at 10:07 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:
>
> 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.
>

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

Okay! I'll scan through the file to check for redundancy like this and fix them.

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

I think since I'm modifying `test_path_is_{dir|file}` functions to
handle calls like `! test_path_is_dir` well as mentioned in this
thread[1]. I think we should replace `! test` calls with `test !`, so
that the changes are in agreement with each other. What do you say?

Thanks for advice
Rohit

[1]: https://public-inbox.org/git/CAN0heSqSp-a0zUKT5EaGLBYnRtESTnu9GKWtGARz2kaOAhc1HQ@mail.gmail.com/