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

From: Aryan Pathania <contact@aynp.dev>
Date: Mar 12, 2025, 17:34 UTC
Message-ID: <r5572ospfh3d7nwniod36jcy5ikv5pkmiwtqj25ll7p5ts3zay@okbxrhy77iyv>
In-Reply-To: <Z86LrOEhH3CJOIey@pks.im>

Hi Patrick, Thank you so much for taking the time out of your schedule to review my patch.

>> use `test_path_*` instead of `test -[efd]` to avoid false complaints and
>
>Nit: We want to have full sentences in commit messages. Those sentences
>should start with an upper-case letter and end with punctuation.

I understand. Sorry for not knowing this.

Show 5 quoted lines

>Hm. I'm not exactly sure what "false complaints" you are talking about
>here. The benefit of `test_path_*`()` over `test -[efd]` is that they
>actually print information _why_ they have failed, which may help devs
>to figure out what's happening. So it's kind of the opposite of what you
>say in the commit message: we're now printing _more_ output, not less.

I'm really sorry, I got confused in using `! test_path_*` and `test !*`. Bad mistake while writing the message.

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

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

I'll make the required changes in a new patch version. Sorry for the trouble and mistakes.