Re: [GSoC PATCH] t9400: prefer test_path_* helper functions

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 12, 2025, 18:13 UTC
Message-ID: <xmqqplimxgkb.fsf@gitster.g>
In-Reply-To: <r5572ospfh3d7nwniod36jcy5ikv5pkmiwtqj25ll7p5ts3zay@okbxrhy77iyv>

Aryan Pathania <contact@aynp.dev> writes:

Show 5 quoted lines

>>This isn't quite equivalent: we've been checking that the path is not a
>>directory before, but now we verify that the path doesn't exist.
> I understand. I could not find `test_path_is_not_dir` or any equivalent
> function in `test-lib-functions.sh`. Maybe we can keep this stronger
> check. I'll mention in the commit message of next version of patch.

That is exactly Patrick suggested (go back and read it). I agree with him that the updated stronger check is an improvement and it deserves to be explained in the commit message.

>>We tend to use `test_path_is_file` rather than
>>`test_path_is_file_not_symlink`, but I don't mind it too much.
> I believe `test -f` is equivalent to `test_path_is_file_not_symlink` and
> is a stronger check so maybe it's fine.

Don't believe what you think you know; if you are unsure, check to verify before you base your actions on them.

    $ >this-is-file
    $ ln -s this-is-file this-is-symlink
    $ test -f this-is-symlink; echo $?
    0
    $ test -f this-is-file; echo $?
    0

And if you are not unsure, then learn to be unsure more often ;-)

I do not see any reason in the part of the code Patrick commented on to insist that gitcvs.ext.main.sqlite file must be a regular file and not a symbolic link to another file. Both test_path_is_file and its original before the patch, "test -f", would be more appropriate than test_path_is_file_not_symlink, which was specifically invented for use in t3903 where the tests used both files and symbolic links to make sure the operation being tested would not confuse one with the other.

> Sorry for the trouble and mistakes.

No need for that. This is for both sides to experience to learn to work well together.