Re: [PATCH] t/t3903-stash.sh: replace test [-d|-f] with test_path_is_*

From: Cogoni Guillaume <cogoni.guillaume@gmail.com>
Date: Feb 14, 2022, 20:22 UTC
Message-ID: <6fbd4188-3bb5-6d48-fd25-1bdbe9a3cbfb@gmail.com>
In-Reply-To: <xmqq5yplcme1.fsf@gitster.g>

First of all, sorry for the delay of this answer.

Show 6 quoted lines

> On 02/11/2022 at 7:02 PM, Junio C Hamano wrote:
>
> This is not wrong per-se, and I know I shouldn't demand too much
> from a practice patch like this, but for a real patch, I hope
> contributors carefully check if the original is doing the right
> thing.

It's good that you are demanding even for a practice patch because we are here to learn as much as we can. And, we will take a good attention to your ideas.

Show 9 quoted lines

>   * verify that all existing users of test_path_is_file want to
>     reject a symlink to file, and add 'test ! -h "$1" &&' to the
>     implementation of the test helper in t/test-lib-functions.sh
>     (we may want to do the same for test_path_is_dir).
>
>   * introduce test_path_is_symlink and use it appropriately.  This
>     will be a more verbose version of "test -h".
>
>   * introduce test_path_is_file_not_symlink and use it here.

We wouldn't modify test_path_is_file because this function is already use and we won't verify if every uses of this are rejecting symlink.

However, we would like to try to implement test_path_is_symlink and test_path_is_file_not_symlink and the symmetric for directory.

Thanks for your review and the ideas. COGONI Guillaume and BRESSAT Jonathan