patcht9400: prefer test_path_* helper functions

use `test_path_*` instead of `test -[efd]` to avoid false complaints and output when running the script with `-v` or `-x`

Signed-off-by: Aryan Pathania <contact@aynp.dev>
---
 t/t9400-git-cvsserver-server.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

On Sat, Mar 08, 2025 at 06:03:58PM +0900, Aryan Pathania wrote:

Nit: We want to have full sentences in commit messages. Those sentences
should start with an upper-case letter and end with punctuation.

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.

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. It's a sensible change in this context though as our new assertion is stronger than the old one, but it might make sense to point out 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.

Patrick

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

I understand. Sorry for not knowing this.

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

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.

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.

Aryan Pathania <contact@aynp.dev> writes:

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.

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.

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

Hi Junio, thanks a lot for taking time to review my patch.

I apologize for miscomunication. I was agreeing with the suggestion. It was redundant on my part.

Ah, thank you. Sorry I din't know symlink were regular files. I'll be more careful with other things next time.

Aryan Pathania <aryan.pathania2003@gmail.com> writes:

Sorry for misreading what you wrote. Mistakes happen, so please don't over-apologize around here ;-)

Change testcase `gitcvs.enabled = false` to check for missing path instead of a missing file. The change is justified as new assertion is stronger.

All other testcases remain equivalent.
---
 t/t9400-git-cvsserver-server.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Thanks for your GSoC microproject submission. See comments below...

On Tue, Mar 18, 2025 at 9:11 AM Aryan Pathania <contact@aynp.dev> wrote:

According to Documentation/SubmittingPatches, you'd probably instead want to compose the commit message summary line something like this:

    t9400: use test_path_*` functions to improve diagnostic output

The description seems somehow outdated since this particular change is being made to more than just that one test, isn't it?

Rather than describing the new assertion as "stronger", it might make more sense to state that it is more semantically in line with what is actually being tested (i.e. that the directory/path should not exist when, as expected, the command fails).

Okay, but...

... although `test_path_is_file` is a faithful replacement for `test -f`, it could be argued that `test_path_exists` would be a semantically better choice, especially given the use of `test_path_is_missing` for the sibling case.

The same comment applies to one or two other changes in this patch.