Hi SZEDER
On Tue, Feb 26, 2019 at 10:07 PM SZEDER Gábor <szeder.dev@xxxxxxxxx> 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@xxxxxxxxxxxxxx/
