[GSoC PATCH] t9400: prefer test_path_* helper functions

[GSoC PATCH] t9400: prefer test_path_* helper functions

[Aryan Pathania]

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(-)

diff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh
index e499c7f955..6c7cb1964c 100755
--- a/t/t9400-git-cvsserver-server.sh
+++ b/t/t9400-git-cvsserver-server.sh
@@ -254,7 +254,7 @@ test_expect_success 'gitcvs.enabled = false' \
      true
    fi &&
    grep "GITCVS emulation disabled" cvs.log &&
-   test ! -d cvswork2'
+   test_path_is_missing cvswork2'
 
 rm -fr cvswork2
 test_expect_success 'gitcvs.ext.enabled = true' '
@@ -276,7 +276,7 @@ test_expect_success 'gitcvs.ext.enabled = false' '
 		true
 	fi &&
 	grep "GITCVS emulation disabled" cvs.log &&
-	test ! -d cvswork2
+	test_path_is_missing cvswork2
 '
 
 rm -fr cvswork2
@@ -285,7 +285,7 @@ test_expect_success 'gitcvs.dbname' '
 	GIT_DIR="$SERVERDIR" git config gitcvs.dbname %Ggitcvs.%a.%m.sqlite &&
 	GIT_CONFIG="$git_config" cvs -Q co -d cvswork2 main >cvs.log 2>&1 &&
 	test_cmp cvswork cvswork2 &&
-	test -f "$SERVERDIR/gitcvs.ext.main.sqlite" &&
+	test_path_is_file_not_symlink "$SERVERDIR/gitcvs.ext.main.sqlite" &&
 	cmp "$SERVERDIR/gitcvs.main.sqlite" "$SERVERDIR/gitcvs.ext.main.sqlite"
 '
 
@@ -296,8 +296,8 @@ test_expect_success 'gitcvs.ext.dbname' '
 	GIT_DIR="$SERVERDIR" git config gitcvs.dbname %Ggitcvs2.%a.%m.sqlite &&
 	GIT_CONFIG="$git_config" cvs -Q co -d cvswork2 main >cvs.log 2>&1 &&
 	test_cmp cvswork cvswork2 &&
-	test -f "$SERVERDIR/gitcvs1.ext.main.sqlite" &&
-	test ! -f "$SERVERDIR/gitcvs2.ext.main.sqlite" &&
+	test_path_is_file_not_symlink "$SERVERDIR/gitcvs1.ext.main.sqlite" &&
+	test_path_is_missing "$SERVERDIR/gitcvs2.ext.main.sqlite" &&
 	cmp "$SERVERDIR/gitcvs.main.sqlite" "$SERVERDIR/gitcvs1.ext.main.sqlite"
 '
 
@@ -346,7 +346,7 @@ test_expect_failure "cvs update w/o -d doesn't create subdir (TODO)" '
 	git push gitcvs.git >/dev/null &&
 	cd cvswork &&
 	GIT_CONFIG="$git_config" cvs -Q update &&
-	test ! -d test
+	test_path_is_missing test
 '
 
 cd "$WORKDIR"
@@ -379,7 +379,7 @@ test_expect_success 'cvs update (delete file)' '
 	cd cvswork &&
 	GIT_CONFIG="$git_config" cvs -Q update &&
 	test -z "$(grep testfile1 CVS/Entries)" &&
-	test ! -f testfile1
+	test_path_is_missing testfile1
 '
 
 cd "$WORKDIR"
-- 
2.39.5 (Apple Git-154)

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

[Patrick Steinhardt]

On Sat, Mar 08, 2025 at 06:03:58PM +0900, Aryan Pathania wrote:
> 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.

> output when running the script with `-v` or `-x`

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.

> Signed-off-by: Aryan Pathania <contact@aynp.dev>
> ---
>  t/t9400-git-cvsserver-server.sh | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh
> index e499c7f955..6c7cb1964c 100755
> --- a/t/t9400-git-cvsserver-server.sh
> +++ b/t/t9400-git-cvsserver-server.sh
> @@ -254,7 +254,7 @@ test_expect_success 'gitcvs.enabled = false' \
>       true
>     fi &&
>     grep "GITCVS emulation disabled" cvs.log &&
> -   test ! -d cvswork2'
> +   test_path_is_missing cvswork2'

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.

> @@ -285,7 +285,7 @@ test_expect_success 'gitcvs.dbname' '
>  	GIT_DIR="$SERVERDIR" git config gitcvs.dbname %Ggitcvs.%a.%m.sqlite &&
>  	GIT_CONFIG="$git_config" cvs -Q co -d cvswork2 main >cvs.log 2>&1 &&
>  	test_cmp cvswork cvswork2 &&
> -	test -f "$SERVERDIR/gitcvs.ext.main.sqlite" &&
> +	test_path_is_file_not_symlink "$SERVERDIR/gitcvs.ext.main.sqlite" &&
>  	cmp "$SERVERDIR/gitcvs.main.sqlite" "$SERVERDIR/gitcvs.ext.main.sqlite"
>  '
>  

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

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

[Aryan Pathania]

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.

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

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

[Junio C Hamano]

Aryan Pathania <contact@aynp.dev> writes:

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

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

[Aryan Pathania]

Hi Junio, thanks a lot for taking time to review my 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.
I apologize for miscomunication. I was agreeing with the suggestion. It
was redundant on my part.

>    $ >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 ;-)

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

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

[Junio C Hamano]

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

> Hi Junio, thanks a lot for taking time to review my 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.
> I apologize for miscomunication. I was agreeing with the suggestion.

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

[GSoC PATCH v2] Use `test_path_*` helper functions instead of `test -[efd]`.

[Aryan Pathania]

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(-)

diff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh
index e499c7f955..4ddde382e8 100755
--- a/t/t9400-git-cvsserver-server.sh
+++ b/t/t9400-git-cvsserver-server.sh
@@ -254,7 +254,7 @@ test_expect_success 'gitcvs.enabled = false' \
      true
    fi &&
    grep "GITCVS emulation disabled" cvs.log &&
-   test ! -d cvswork2'
+   test_path_is_missing cvswork2'
 
 rm -fr cvswork2
 test_expect_success 'gitcvs.ext.enabled = true' '
@@ -276,7 +276,7 @@ test_expect_success 'gitcvs.ext.enabled = false' '
 		true
 	fi &&
 	grep "GITCVS emulation disabled" cvs.log &&
-	test ! -d cvswork2
+	test_path_is_missing cvswork2
 '
 
 rm -fr cvswork2
@@ -285,7 +285,7 @@ test_expect_success 'gitcvs.dbname' '
 	GIT_DIR="$SERVERDIR" git config gitcvs.dbname %Ggitcvs.%a.%m.sqlite &&
 	GIT_CONFIG="$git_config" cvs -Q co -d cvswork2 main >cvs.log 2>&1 &&
 	test_cmp cvswork cvswork2 &&
-	test -f "$SERVERDIR/gitcvs.ext.main.sqlite" &&
+	test_path_is_file "$SERVERDIR/gitcvs.ext.main.sqlite" &&
 	cmp "$SERVERDIR/gitcvs.main.sqlite" "$SERVERDIR/gitcvs.ext.main.sqlite"
 '
 
@@ -296,8 +296,8 @@ test_expect_success 'gitcvs.ext.dbname' '
 	GIT_DIR="$SERVERDIR" git config gitcvs.dbname %Ggitcvs2.%a.%m.sqlite &&
 	GIT_CONFIG="$git_config" cvs -Q co -d cvswork2 main >cvs.log 2>&1 &&
 	test_cmp cvswork cvswork2 &&
-	test -f "$SERVERDIR/gitcvs1.ext.main.sqlite" &&
-	test ! -f "$SERVERDIR/gitcvs2.ext.main.sqlite" &&
+	test_path_is_file "$SERVERDIR/gitcvs1.ext.main.sqlite" &&
+	test_path_is_missing "$SERVERDIR/gitcvs2.ext.main.sqlite" &&
 	cmp "$SERVERDIR/gitcvs.main.sqlite" "$SERVERDIR/gitcvs1.ext.main.sqlite"
 '
 
@@ -346,7 +346,7 @@ test_expect_failure "cvs update w/o -d doesn't create subdir (TODO)" '
 	git push gitcvs.git >/dev/null &&
 	cd cvswork &&
 	GIT_CONFIG="$git_config" cvs -Q update &&
-	test ! -d test
+	test_path_is_missing test
 '
 
 cd "$WORKDIR"
@@ -379,7 +379,7 @@ test_expect_success 'cvs update (delete file)' '
 	cd cvswork &&
 	GIT_CONFIG="$git_config" cvs -Q update &&
 	test -z "$(grep testfile1 CVS/Entries)" &&
-	test ! -f testfile1
+	test_path_is_missing testfile1
 '
 
 cd "$WORKDIR"
-- 
2.39.5 (Apple Git-154)

Re: [GSoC PATCH v2] Use `test_path_*` helper functions instead of `test -[efd]`.

[Eric Sunshine]

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

On Tue, Mar 18, 2025 at 9:11 AM Aryan Pathania <contact@aynp.dev> wrote:
> Use `test_path_*` helper functions instead of `test -[efd]`

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

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

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

> All other testcases remain equivalent.

Okay, but...

> @@ -296,8 +296,8 @@ test_expect_success 'gitcvs.ext.dbname' '
>         test_cmp cvswork cvswork2 &&
> -       test -f "$SERVERDIR/gitcvs1.ext.main.sqlite" &&
> -       test ! -f "$SERVERDIR/gitcvs2.ext.main.sqlite" &&
> +       test_path_is_file "$SERVERDIR/gitcvs1.ext.main.sqlite" &&
> +       test_path_is_missing "$SERVERDIR/gitcvs2.ext.main.sqlite" &&

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