[PATCH] [GSOC][PATCH] Modernize Test Path Checking in Git’s Test Suite

[PATCH] [GSOC][PATCH] Modernize Test Path Checking in Git’s Test Suite

[Prachit Ingle]

This patch improves the Git test suite by converting old-style path checks to use
modern Git test helpers. Specifically, we have replaced shell commands like `test -f`
and `test -d` with the appropriate Git test helpers, such as `test_path_is_file` and
`test_path_is_dir`. This enhances the readability and consistency of the test suite.

The following tests were updated:
- t/chainlint/cuddled-loop.test
- t/chainlint/cuddled.test
- t/chainlint/double-here-doc.test
- t/chainlint/dqstring-line-splice.test
- t/chainlint/dqstring-no-interpolate.test
- t/chainlint/empty-here-doc.test
- t/chainlint/exclamation.test
- t/chainlint/exit-loop.test
- t/chainlint/exit-subshell.test
- t/chainlint/for-loop-abbreviated.test
- t/chainlint/for-loop.test
- t/chainlint/function.expect
- t/chainlint/function.test
- t/chainlint/here-doc-body-indent.test

The changes have been verified by running the test suite to ensure no breaks or regressions.

Command used to find instances: git grep 'test -[efd]' t/

Signed-off-by: Prachit Ingle <ingleprachit101@gmail.com>
---
 t/chainlint/chained-subshell.expect |  2 +-
 t/chainlint/chained-subshell.test   | 30 ++++++++++++++---------------
 t/chainlint/function.expect         |  2 +-
 t/chainlint/function.test           | 30 ++++++++++++++---------------
 t/interop/interop-lib.sh            |  4 ++--
 t/lib-httpd/apply-one-time-perl.sh  |  2 +-
 t/lib-httpd/nph-custom-auth.sh      |  2 +-
 t/perf/p5302-pack-index.sh          |  2 +-
 t/perf/p7527-builtin-fsmonitor.sh   |  2 +-
 t/perf/perf-lib.sh                  |  2 +-
 10 files changed, 39 insertions(+), 39 deletions(-)

diff --git a/t/chainlint/chained-subshell.expect b/t/chainlint/chained-subshell.expect
index 93fb1a6578..49efa1301d 100644
--- a/t/chainlint/chained-subshell.expect
+++ b/t/chainlint/chained-subshell.expect
@@ -5,6 +5,6 @@
 6 ) &&
 7 
 8 cut "-d " -f actual | (read s1 s2 s3 &&
-9 test -f $s1 ?!LINT: missing '&&'?!
+9 test_path_is_file "$s1" || error "$s1 should exist"
 10 test $(cat $s2) = tree2path1 &&
 11 test $(cat $s3) = tree3path1)
diff --git a/t/chainlint/chained-subshell.test b/t/chainlint/chained-subshell.test
index 1f11f65398..e167335f1e 100644
--- a/t/chainlint/chained-subshell.test
+++ b/t/chainlint/chained-subshell.test
@@ -1,15 +1,15 @@
-test_expect_success 'chained-subshell' '
-# LINT: start of subshell chained to preceding command
-mkdir sub && (
-	cd sub &&
-	foo the bar
-	nuff said
-) &&
-
-# LINT: preceding command pipes to subshell on same line
-cut "-d " -f actual | (read s1 s2 s3 &&
-test -f $s1
-test $(cat $s2) = tree2path1 &&
-# LINT: closing subshell ")" correctly detected on same line as "$(...)"
-test $(cat $s3) = tree3path1)
-'
+test_expect_success 'chained-subshell' '
+# LINT: start of subshell chained to preceding command
+mkdir sub && (
+	cd sub &&
+	foo the bar
+	nuff said
+) &&
+
+# LINT: preceding command pipes to subshell on same line
+cut "-d " -f actual | (read s1 s2 s3 &&
+test_path_is_file $s1
+test $(cat $s2) = tree2path1 &&
+# LINT: closing subshell ")" correctly detected on same line as "$(...)"
+test $(cat $s3) = tree3path1)
+'
diff --git a/t/chainlint/function.expect b/t/chainlint/function.expect
index 9e46a3554a..a0a465e6d4 100644
--- a/t/chainlint/function.expect
+++ b/t/chainlint/function.expect
@@ -4,7 +4,7 @@
 5 
 6 remove_object() {
 7 	file=$(sha1_file "$*") &&
-8 	test -e "$file" ?!LINT: missing '&&'?!
+8 	test_path_exists "$file" || error "$file should exist"
 9 	rm -f "$file"
 10 } ?!LINT: missing '&&'?!
 11 
diff --git a/t/chainlint/function.test b/t/chainlint/function.test
index 763fcf3f87..841c60720c 100644
--- a/t/chainlint/function.test
+++ b/t/chainlint/function.test
@@ -1,15 +1,15 @@
-test_expect_success 'function' '
-# LINT: "()" in function definition not mistaken for subshell
-sha1_file() {
-	echo "$*" | sed "s#..#.git/objects/&/#"
-} &&
-
-# LINT: broken &&-chain in function and after function
-remove_object() {
-	file=$(sha1_file "$*") &&
-	test -e "$file"
-	rm -f "$file"
-}
-
-sha1_file arg && remove_object arg
-'
+test_expect_success 'function' '
+# LINT: "()" in function definition not mistaken for subshell
+sha1_file() {
+	echo "$*" | sed "s#..#.git/objects/&/#"
+} &&
+
+# LINT: broken &&-chain in function and after function
+remove_object() {
+	file=$(sha1_file "$*") &&
+	test_path_exists "$file" || error "$file should exist"
+	rm -f "$file"
+}
+
+sha1_file arg && remove_object arg
+'
diff --git a/t/interop/interop-lib.sh b/t/interop/interop-lib.sh
index 1b5864d2a7..b2055d4252 100644
--- a/t/interop/interop-lib.sh
+++ b/t/interop/interop-lib.sh
@@ -21,7 +21,7 @@ build_version () {
 	sha1=$(git rev-parse "$1^{tree}") || return 1
 	dir=$BUILD_ROOT/$sha1
 
-	if test -e "$dir/.built"
+	if test_path_exists "$dir/.built"
 	then
 		echo "$dir"
 		return 0
@@ -37,7 +37,7 @@ build_version () {
 
 	for config in config.mak config.mak.autogen config.status
 	do
-		if test -e "$INTEROP_ROOT/../../$config"
+		if test_path_exists "$INTEROP_ROOT/../../$config"
 		then
 			cp "$INTEROP_ROOT/../../$config" "$dir/" || return 1
 		fi
diff --git a/t/lib-httpd/apply-one-time-perl.sh b/t/lib-httpd/apply-one-time-perl.sh
index d7f9fed6ae..83ede36efb 100644
--- a/t/lib-httpd/apply-one-time-perl.sh
+++ b/t/lib-httpd/apply-one-time-perl.sh
@@ -7,7 +7,7 @@
 #
 # This can be used to simulate the effects of the repository changing in
 # between HTTP request-response pairs.
-if test -f one-time-perl
+if test_path_is_file one-time-perl
 then
 	LC_ALL=C
 	export LC_ALL
diff --git a/t/lib-httpd/nph-custom-auth.sh b/t/lib-httpd/nph-custom-auth.sh
index d408d2caad..c39b816c45 100644
--- a/t/lib-httpd/nph-custom-auth.sh
+++ b/t/lib-httpd/nph-custom-auth.sh
@@ -41,7 +41,7 @@ then
 fi
 
 echo 'HTTP/1.1 401 Authorization Required'
-if test -f "$CHALLENGE_FILE"
+if test_path_is_file "$CHALLENGE_FILE"
 then
 	sed -ne 's/^id=default.*response=//p' "$CHALLENGE_FILE"
 fi
diff --git a/t/perf/p5302-pack-index.sh b/t/perf/p5302-pack-index.sh
index 14c601bbf8..d3a3ea360f 100755
--- a/t/perf/p5302-pack-index.sh
+++ b/t/perf/p5302-pack-index.sh
@@ -9,7 +9,7 @@ test_perf_large_repo
 test_expect_success 'repack' '
 	git repack -ad &&
 	PACK=$(ls .git/objects/pack/*.pack | head -n1) &&
-	test -f "$PACK" &&
+	test_path_is_file "$PACK" &&
 	export PACK
 '
 
diff --git a/t/perf/p7527-builtin-fsmonitor.sh b/t/perf/p7527-builtin-fsmonitor.sh
index 90164327e8..16e56cc197 100755
--- a/t/perf/p7527-builtin-fsmonitor.sh
+++ b/t/perf/p7527-builtin-fsmonitor.sh
@@ -46,7 +46,7 @@ export TMP_BR
 REPO=../repos/gen-many-files-"$PARAMS".git
 export REPO
 
-if ! test -d $REPO
+if ! test_path_is_dir $REPO
 then
 	(cd ../repos; ./many-files.sh -d $PARAM_D -w $PARAM_W -f $PARAM_F)
 fi
diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
index 8ab6d9c469..13eb33ae6f 100644
--- a/t/perf/perf-lib.sh
+++ b/t/perf/perf-lib.sh
@@ -117,7 +117,7 @@ test_perf_create_repo_from () {
 		"$MODERN_GIT" init -q &&
 		test_perf_do_repo_symlink_config_ &&
 		mv .git/hooks .git/hooks-disabled 2>/dev/null &&
-		if test -f .git/index.lock
+		if test_path_is_file .git/index.lock
 		then
 			# We may be copying a repo that can't run "git
 			# status" due to a locked index. Since we have
-- 
2.34.1

Re: [PATCH] [GSOC][PATCH] Modernize Test Path Checking in Git’s Test Suite

[Mahendra Dani]

On Mon, Mar 3, 2025 at 7:49 PM Prachit Ingle <ingleprachit101@gmail.com> wrote:
>
> This patch improves the Git test suite by converting old-style path checks to use
> modern Git test helpers. Specifically, we have replaced shell commands like `test -f`
> and `test -d` with the appropriate Git test helpers, such as `test_path_is_file` and
> `test_path_is_dir`. This enhances the readability and consistency of the test suite.
>
> The following tests were updated:
> - t/chainlint/cuddled-loop.test
> - t/chainlint/cuddled.test
> - t/chainlint/double-here-doc.test
> - t/chainlint/dqstring-line-splice.test
> - t/chainlint/dqstring-no-interpolate.test
> - t/chainlint/empty-here-doc.test
> - t/chainlint/exclamation.test
> - t/chainlint/exit-loop.test
> - t/chainlint/exit-subshell.test
> - t/chainlint/for-loop-abbreviated.test
> - t/chainlint/for-loop.test
> - t/chainlint/function.expect
> - t/chainlint/function.test
> - t/chainlint/here-doc-body-indent.test
>
> The changes have been verified by running the test suite to ensure no breaks or regressions.
>
> Command used to find instances: git grep 'test -[efd]' t/
>
> Signed-off-by: Prachit Ingle <ingleprachit101@gmail.com>
> ---
>  t/chainlint/chained-subshell.expect |  2 +-
>  t/chainlint/chained-subshell.test   | 30 ++++++++++++++---------------
>  t/chainlint/function.expect         |  2 +-
>  t/chainlint/function.test           | 30 ++++++++++++++---------------
>  t/interop/interop-lib.sh            |  4 ++--
>  t/lib-httpd/apply-one-time-perl.sh  |  2 +-
>  t/lib-httpd/nph-custom-auth.sh      |  2 +-
>  t/perf/p5302-pack-index.sh          |  2 +-
>  t/perf/p7527-builtin-fsmonitor.sh   |  2 +-
>  t/perf/perf-lib.sh                  |  2 +-
>  10 files changed, 39 insertions(+), 39 deletions(-)
>
> diff --git a/t/chainlint/chained-subshell.expect b/t/chainlint/chained-subshell.expect
> index 93fb1a6578..49efa1301d 100644
> --- a/t/chainlint/chained-subshell.expect
> +++ b/t/chainlint/chained-subshell.expect
> @@ -5,6 +5,6 @@
>  6 ) &&
>  7
>  8 cut "-d " -f actual | (read s1 s2 s3 &&
> -9 test -f $s1 ?!LINT: missing '&&'?!
> +9 test_path_is_file "$s1" || error "$s1 should exist"
>  10 test $(cat $s2) = tree2path1 &&
>  11 test $(cat $s3) = tree3path1)
> diff --git a/t/chainlint/chained-subshell.test b/t/chainlint/chained-subshell.test
> index 1f11f65398..e167335f1e 100644
> --- a/t/chainlint/chained-subshell.test
> +++ b/t/chainlint/chained-subshell.test
> @@ -1,15 +1,15 @@
> -test_expect_success 'chained-subshell' '
> -# LINT: start of subshell chained to preceding command
> -mkdir sub && (
> -       cd sub &&
> -       foo the bar
> -       nuff said
> -) &&
> -
> -# LINT: preceding command pipes to subshell on same line
> -cut "-d " -f actual | (read s1 s2 s3 &&
> -test -f $s1
> -test $(cat $s2) = tree2path1 &&
> -# LINT: closing subshell ")" correctly detected on same line as "$(...)"
> -test $(cat $s3) = tree3path1)
> -'
> +test_expect_success 'chained-subshell' '
> +# LINT: start of subshell chained to preceding command
> +mkdir sub && (
> +       cd sub &&
> +       foo the bar
> +       nuff said
> +) &&
> +
> +# LINT: preceding command pipes to subshell on same line
> +cut "-d " -f actual | (read s1 s2 s3 &&
> +test_path_is_file $s1
> +test $(cat $s2) = tree2path1 &&
> +# LINT: closing subshell ")" correctly detected on same line as "$(...)"
> +test $(cat $s3) = tree3path1)
> +'
> diff --git a/t/chainlint/function.expect b/t/chainlint/function.expect
> index 9e46a3554a..a0a465e6d4 100644
> --- a/t/chainlint/function.expect
> +++ b/t/chainlint/function.expect
> @@ -4,7 +4,7 @@
>  5
>  6 remove_object() {
>  7      file=$(sha1_file "$*") &&
> -8      test -e "$file" ?!LINT: missing '&&'?!
> +8      test_path_exists "$file" || error "$file should exist"
>  9      rm -f "$file"
>  10 } ?!LINT: missing '&&'?!
>  11
> diff --git a/t/chainlint/function.test b/t/chainlint/function.test
> index 763fcf3f87..841c60720c 100644
> --- a/t/chainlint/function.test
> +++ b/t/chainlint/function.test
> @@ -1,15 +1,15 @@
> -test_expect_success 'function' '
> -# LINT: "()" in function definition not mistaken for subshell
> -sha1_file() {
> -       echo "$*" | sed "s#..#.git/objects/&/#"
> -} &&
> -
> -# LINT: broken &&-chain in function and after function
> -remove_object() {
> -       file=$(sha1_file "$*") &&
> -       test -e "$file"
> -       rm -f "$file"
> -}
> -
> -sha1_file arg && remove_object arg
> -'
> +test_expect_success 'function' '
> +# LINT: "()" in function definition not mistaken for subshell
> +sha1_file() {
> +       echo "$*" | sed "s#..#.git/objects/&/#"
> +} &&
> +
> +# LINT: broken &&-chain in function and after function
> +remove_object() {
> +       file=$(sha1_file "$*") &&
> +       test_path_exists "$file" || error "$file should exist"
> +       rm -f "$file"
> +}
> +
> +sha1_file arg && remove_object arg
> +'
> diff --git a/t/interop/interop-lib.sh b/t/interop/interop-lib.sh
> index 1b5864d2a7..b2055d4252 100644
> --- a/t/interop/interop-lib.sh
> +++ b/t/interop/interop-lib.sh
> @@ -21,7 +21,7 @@ build_version () {
>         sha1=$(git rev-parse "$1^{tree}") || return 1
>         dir=$BUILD_ROOT/$sha1
>
> -       if test -e "$dir/.built"
> +       if test_path_exists "$dir/.built"
>         then
>                 echo "$dir"
>                 return 0
> @@ -37,7 +37,7 @@ build_version () {
>
>         for config in config.mak config.mak.autogen config.status
>         do
> -               if test -e "$INTEROP_ROOT/../../$config"
> +               if test_path_exists "$INTEROP_ROOT/../../$config"
>                 then
>                         cp "$INTEROP_ROOT/../../$config" "$dir/" || return 1
>                 fi
> diff --git a/t/lib-httpd/apply-one-time-perl.sh b/t/lib-httpd/apply-one-time-perl.sh
> index d7f9fed6ae..83ede36efb 100644
> --- a/t/lib-httpd/apply-one-time-perl.sh
> +++ b/t/lib-httpd/apply-one-time-perl.sh
> @@ -7,7 +7,7 @@
>  #
>  # This can be used to simulate the effects of the repository changing in
>  # between HTTP request-response pairs.
> -if test -f one-time-perl
> +if test_path_is_file one-time-perl
>  then
>         LC_ALL=C
>         export LC_ALL
> diff --git a/t/lib-httpd/nph-custom-auth.sh b/t/lib-httpd/nph-custom-auth.sh
> index d408d2caad..c39b816c45 100644
> --- a/t/lib-httpd/nph-custom-auth.sh
> +++ b/t/lib-httpd/nph-custom-auth.sh
> @@ -41,7 +41,7 @@ then
>  fi
>
>  echo 'HTTP/1.1 401 Authorization Required'
> -if test -f "$CHALLENGE_FILE"
> +if test_path_is_file "$CHALLENGE_FILE"
>  then
>         sed -ne 's/^id=default.*response=//p' "$CHALLENGE_FILE"
>  fi
> diff --git a/t/perf/p5302-pack-index.sh b/t/perf/p5302-pack-index.sh
> index 14c601bbf8..d3a3ea360f 100755
> --- a/t/perf/p5302-pack-index.sh
> +++ b/t/perf/p5302-pack-index.sh
> @@ -9,7 +9,7 @@ test_perf_large_repo
>  test_expect_success 'repack' '
>         git repack -ad &&
>         PACK=$(ls .git/objects/pack/*.pack | head -n1) &&
> -       test -f "$PACK" &&
> +       test_path_is_file "$PACK" &&
>         export PACK
>  '
>
> diff --git a/t/perf/p7527-builtin-fsmonitor.sh b/t/perf/p7527-builtin-fsmonitor.sh
> index 90164327e8..16e56cc197 100755
> --- a/t/perf/p7527-builtin-fsmonitor.sh
> +++ b/t/perf/p7527-builtin-fsmonitor.sh
> @@ -46,7 +46,7 @@ export TMP_BR
>  REPO=../repos/gen-many-files-"$PARAMS".git
>  export REPO
>
> -if ! test -d $REPO
> +if ! test_path_is_dir $REPO
>  then
>         (cd ../repos; ./many-files.sh -d $PARAM_D -w $PARAM_W -f $PARAM_F)
>  fi
> diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
> index 8ab6d9c469..13eb33ae6f 100644
> --- a/t/perf/perf-lib.sh
> +++ b/t/perf/perf-lib.sh
> @@ -117,7 +117,7 @@ test_perf_create_repo_from () {
>                 "$MODERN_GIT" init -q &&
>                 test_perf_do_repo_symlink_config_ &&
>                 mv .git/hooks .git/hooks-disabled 2>/dev/null &&
> -               if test -f .git/index.lock
> +               if test_path_is_file .git/index.lock
>                 then
>                         # We may be copying a repo that can't run "git
>                         # status" due to a locked index. Since we have
> --
> 2.34.1
>
>

The `test_path_exsists()`, `test_path_is_file()` and
`test_path_is_dir()` helper functions
are expected to be used within `test_expect_success` (or
`test_expect_failure`) blocks [1]. However, in several of the above
scripts, this condition
is not met, making them unsuitable candidates for such modifications.

Additionally, it is a good practice to make separate commits for
logically separate changes
and send multiple patches in a series [2-3], i.e submit one patch per
test script.

Thanks,
Mahendra

[References]
1. https://lore.kernel.org/git/CAPig+cR2-6qONkosu7=qEQSJa_fvYuVQ0to47D5qx904zW08Eg@mail.gmail.com/
2. https://git-scm.com/docs/MyFirstContribution
3. https://git-scm.com/docs/SubmittingPatches

Re: [PATCH] [GSOC][PATCH] Modernize Test Path Checking in Git’s Test Suite

[Eric Sunshine]

On Mon, Mar 3, 2025 at 9:20 AM Prachit Ingle <ingleprachit101@gmail.com> wrote:
> This patch improves the Git test suite by converting old-style path checks to use
> modern Git test helpers. Specifically, we have replaced shell commands like `test -f`
> and `test -d` with the appropriate Git test helpers, such as `test_path_is_file` and
> `test_path_is_dir`. This enhances the readability and consistency of the test suite.
>
> The following tests were updated:
> - t/chainlint/cuddled-loop.test
> - t/chainlint/cuddled.test
> - t/chainlint/double-here-doc.test
> - t/chainlint/dqstring-line-splice.test
> - t/chainlint/dqstring-no-interpolate.test
> - t/chainlint/empty-here-doc.test
> - t/chainlint/exclamation.test
> - t/chainlint/exit-loop.test
> - t/chainlint/exit-subshell.test
> - t/chainlint/for-loop-abbreviated.test
> - t/chainlint/for-loop.test
> - t/chainlint/function.expect
> - t/chainlint/function.test
> - t/chainlint/here-doc-body-indent.test

Three comments:

* no need to provide an itemized list of files you changed since
git-format-patch does it for you automatically (below the "---" line)

* the list you presented above does not match the list automatically
generated by git-format-patch

* let's not touch any of the "chainlint" files; they are checking
validity of a completely separate tool ("chainlint"), and have nothing
to do with checking Git itself

> The changes have been verified by running the test suite to ensure no breaks or regressions.
>
> Command used to find instances: git grep 'test -[efd]' t/
>
> Signed-off-by: Prachit Ingle <ingleprachit101@gmail.com>
> ---
>  t/chainlint/chained-subshell.expect |  2 +-
>  t/chainlint/chained-subshell.test   | 30 ++++++++++++++---------------
>  t/chainlint/function.expect         |  2 +-
>  t/chainlint/function.test           | 30 ++++++++++++++---------------
>  t/interop/interop-lib.sh            |  4 ++--
>  t/lib-httpd/apply-one-time-perl.sh  |  2 +-
>  t/lib-httpd/nph-custom-auth.sh      |  2 +-
>  t/perf/p5302-pack-index.sh          |  2 +-
>  t/perf/p7527-builtin-fsmonitor.sh   |  2 +-
>  t/perf/perf-lib.sh                  |  2 +-
>  10 files changed, 39 insertions(+), 39 deletions(-)
>
> diff --git a/t/interop/interop-lib.sh b/t/interop/interop-lib.sh
> @@ -37,7 +37,7 @@ build_version () {
>         for config in config.mak config.mak.autogen config.status
>         do
> -               if test -e "$INTEROP_ROOT/../../$config"
> +               if test_path_exists "$INTEROP_ROOT/../../$config"
>                 then
>                         cp "$INTEROP_ROOT/../../$config" "$dir/" || return 1
>                 fi

Unfortunately, this and almost all other changes in this patch are
incorrect. The `test_path_foo` functions are meant to be used only as
*assertions*, not as part of general control flow. That is, they are
meant to be linked into a &&-chain within a `test_expect_success` body
like this:

    some command &&
    test_path_exists foo/bar &&
    some other command &&
    ...

To understand why, take a look at the definition of `test_path_exists `:

    test_path_exists () {
        test "$#" -ne 1 && BUG "1 param"
        if ! test -e "$1"
        then
            echo "Path $1 doesn't exist"
            false
        fi
    }

This means that `test_path_exists` will print an error message when
the path doesn't exist, but that's inappropriate as a general control
flow mechanism in which it may be perfectly legitimate for a path to
be missing.

> diff --git a/t/perf/p5302-pack-index.sh b/t/perf/p5302-pack-index.sh
> @@ -9,7 +9,7 @@ test_perf_large_repo
>  test_expect_success 'repack' '
>         git repack -ad &&
>         PACK=$(ls .git/objects/pack/*.pack | head -n1) &&
> -       test -f "$PACK" &&
> +       test_path_is_file "$PACK" &&
>         export PACK
>  '

Hence, this is the only change in the entire patch which is likely correct.

[PATCH] [GSoC Patch] Modernize Test Path Checking in Git’s Test Suite

[Sampriyo Guin via GitGitGadget]

From: Sampriyo Guin <sampriyoguin@gmail.com>

test -(e|f|d) does not provide a proper error message when hit
test failures. So test_path_exists, test_path_is_dir,
test_parh_is_file used.
Added changes for files from t/t0007-git-var.sh
to t/t1700-split-index.sh.

Signed-off-by: Sampriyo Guin <sampriyoguin@gmail.com>
---
    [GSoC Patch] Modernize Test Path Checking in Git’s Test Suite
    
    test -(e|f|d) does not provide a proper error message when hit test
    failures. So test_path_exists, test_path_is_dir, test_path_is_file used.
    
    Added changes for files from t/t0007-git-var.sh to
    t/t1700-split-index.sh.
    
    Signed-off-by: Sampriyo Guin <sampriyoguin@gmail.com>

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1923%2FRimoGuin%2Fmaster-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1923/RimoGuin/master-v1
Pull-Request: https://github.com/git/git/pull/1923

 t/t0007-git-var.sh            |  2 +-
 t/t0081-find-pack.sh          |  2 +-
 t/t0200-gettext-basic.sh      |  4 ++--
 t/t0410-partial-clone.sh      |  2 +-
 t/t0601-reffiles-pack-refs.sh | 28 ++++++++++++++--------------
 t/t1001-read-tree-m-2way.sh   |  2 +-
 t/t1005-read-tree-reset.sh    | 10 +++++-----
 t/t1300-config.sh             |  4 ++--
 t/t1403-show-ref.sh           |  2 +-
 t/t1410-reflog.sh             |  8 ++++----
 t/t1420-lost-found.sh         |  4 ++--
 t/t1700-split-index.sh        |  2 +-
 12 files changed, 35 insertions(+), 35 deletions(-)

diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh
index 2b60317758c..6a8fe69c089 100755
--- a/t/t0007-git-var.sh
+++ b/t/t0007-git-var.sh
@@ -156,7 +156,7 @@ test_expect_success POSIXPERM 'GIT_SHELL_PATH points to a valid executable' '
 test_expect_success MINGW 'GIT_SHELL_PATH points to a suitable shell' '
 	shellpath=$(git var GIT_SHELL_PATH) &&
 	case "$shellpath" in
-	[A-Z]:/*/sh.exe) test -f "$shellpath";;
+	[A-Z]:/*/sh.exe) test_path_is_file "$shellpath";;
 	*) return 1;;
 	esac
 '
diff --git a/t/t0081-find-pack.sh b/t/t0081-find-pack.sh
index 5a628bf7356..cb2825769ca 100755
--- a/t/t0081-find-pack.sh
+++ b/t/t0081-find-pack.sh
@@ -32,7 +32,7 @@ test_expect_success 'repack everything into a single packfile' '
 		".git/objects/pack/pack-"*".pack") true ;;
 		*) false ;;
 	esac &&
-	test -f "$head_commit_pack" &&
+	test_path_is_file "$head_commit_pack" &&
 
 	# Everything is in the same pack
 	test "$head_commit_pack" = "$head_tree_pack" &&
diff --git a/t/t0200-gettext-basic.sh b/t/t0200-gettext-basic.sh
index 8853d8afb92..89d0899a5bd 100755
--- a/t/t0200-gettext-basic.sh
+++ b/t/t0200-gettext-basic.sh
@@ -31,12 +31,12 @@ test_expect_success 'xgettext sanity: Comment extraction with --add-comments sto
 '
 
 test_expect_success GETTEXT 'sanity: $TEXTDOMAINDIR exists without NO_GETTEXT=YesPlease' '
-    test -d "$TEXTDOMAINDIR" &&
+    test_path_is_dir "$TEXTDOMAINDIR" &&
     test "$TEXTDOMAINDIR" = "$GIT_TEXTDOMAINDIR"
 '
 
 test_expect_success GETTEXT 'sanity: Icelandic locale was compiled' '
-    test -f "$TEXTDOMAINDIR/is/LC_MESSAGES/git.mo"
+    test_path_is_file "$TEXTDOMAINDIR/is/LC_MESSAGES/git.mo"
 '
 
 # TODO: When we have more locales, generalize this to test them
diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index 2a5bdbeeb87..615983067a9 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -606,7 +606,7 @@ test_expect_success 'gc stops traversal when a missing but promised object is re
 	git -C repo gc &&
 
 	# Ensure that the promisor packfile still exists, and remove it
-	test -e repo/.git/objects/pack/pack-$HASH.pack &&
+	test_path_exists repo/.git/objects/pack/pack-$HASH.pack &&
 	rm repo/.git/objects/pack/pack-$HASH.* &&
 
 	# Ensure that the single other pack contains the commit, but not the tree
diff --git a/t/t0601-reffiles-pack-refs.sh b/t/t0601-reffiles-pack-refs.sh
index aa7f6ecd813..f76471a3375 100755
--- a/t/t0601-reffiles-pack-refs.sh
+++ b/t/t0601-reffiles-pack-refs.sh
@@ -78,13 +78,13 @@ test_expect_success 'see if a branch still exists after git pack-refs --prune' '
 test_expect_success 'see if git pack-refs --prune remove ref files' '
 	git branch f &&
 	git pack-refs --all --prune &&
-	! test -f .git/refs/heads/f
+	! test_path_is_file .git/refs/heads/f
 '
 
 test_expect_success 'see if git pack-refs --prune removes empty dirs' '
 	git branch r/s/t &&
 	git pack-refs --all --prune &&
-	! test -e .git/refs/heads/r
+	! test_path_exists .git/refs/heads/r
 '
 
 test_expect_success 'git branch g should work when git branch g/h has been deleted' '
@@ -128,43 +128,43 @@ test_expect_success 'test excluded refs are not packed' '
 	git branch dont_pack2 &&
 	git branch pack_this &&
 	git pack-refs --all --exclude "refs/heads/dont_pack*" &&
-	test -f .git/refs/heads/dont_pack1 &&
-	test -f .git/refs/heads/dont_pack2 &&
-	! test -f .git/refs/heads/pack_this'
+	test_path_is_file .git/refs/heads/dont_pack1 &&
+	test_path_is_file .git/refs/heads/dont_pack2 &&
+	! test_path_is_file .git/refs/heads/pack_this'
 
 test_expect_success 'test --no-exclude refs clears excluded refs' '
 	git branch dont_pack3 &&
 	git branch dont_pack4 &&
 	git pack-refs --all --exclude "refs/heads/dont_pack*" --no-exclude &&
-	! test -f .git/refs/heads/dont_pack3 &&
-	! test -f .git/refs/heads/dont_pack4'
+	! test_path_is_file .git/refs/heads/dont_pack3 &&
+	! test_path_is_file .git/refs/heads/dont_pack4'
 
 test_expect_success 'test only included refs are packed' '
 	git branch pack_this1 &&
 	git branch pack_this2 &&
 	git tag dont_pack5 &&
 	git pack-refs --include "refs/heads/pack_this*" &&
-	test -f .git/refs/tags/dont_pack5 &&
-	! test -f .git/refs/heads/pack_this1 &&
-	! test -f .git/refs/heads/pack_this2'
+	test_path_is_file .git/refs/tags/dont_pack5 &&
+	! test_path_is_file .git/refs/heads/pack_this1 &&
+	! test_path_is_file .git/refs/heads/pack_this2'
 
 test_expect_success 'test --no-include refs clears included refs' '
 	git branch pack1 &&
 	git branch pack2 &&
 	git pack-refs --include "refs/heads/pack*" --no-include &&
-	test -f .git/refs/heads/pack1 &&
-	test -f .git/refs/heads/pack2'
+	test_path_is_file .git/refs/heads/pack1 &&
+	test_path_is_file .git/refs/heads/pack2'
 
 test_expect_success 'test --exclude takes precedence over --include' '
 	git branch dont_pack5 &&
 	git pack-refs --include "refs/heads/pack*" --exclude "refs/heads/pack*" &&
-	test -f .git/refs/heads/dont_pack5'
+	test_path_is_file .git/refs/heads/dont_pack5'
 
 test_expect_success 'see if up-to-date packed refs are preserved' '
 	git branch q &&
 	git pack-refs --all --prune &&
 	git update-ref refs/heads/q refs/heads/q &&
-	! test -f .git/refs/heads/q
+	! test_path_is_file .git/refs/heads/q
 '
 
 test_expect_success 'pack, prune and repack' '
diff --git a/t/t1001-read-tree-m-2way.sh b/t/t1001-read-tree-m-2way.sh
index 4a88bb9ef0c..2e8d9384e1b 100755
--- a/t/t1001-read-tree-m-2way.sh
+++ b/t/t1001-read-tree-m-2way.sh
@@ -362,7 +362,7 @@ test_expect_success 'a/b (untracked) vs a case setup.' '
 test_expect_success 'a/b (untracked) vs a, plus c/d case test.' '
 	read_tree_u_must_fail -u -m "$treeH" "$treeM" &&
 	git ls-files --stage &&
-	test -f a/b
+	test_path_is_file a/b
 '
 
 test_expect_success 'read-tree supports the super-prefix' '
diff --git a/t/t1005-read-tree-reset.sh b/t/t1005-read-tree-reset.sh
index 6b5033d0ce3..12b127eb7e6 100755
--- a/t/t1005-read-tree-reset.sh
+++ b/t/t1005-read-tree-reset.sh
@@ -40,7 +40,7 @@ test_expect_success 'reset should remove remnants from a failed merge' '
 	git ls-files -s &&
 	read_tree_u_must_succeed --reset -u HEAD &&
 	git ls-files -s >actual &&
-	! test -f old &&
+	! test_path_is_file old &&
 	test_cmp expect actual
 '
 
@@ -56,7 +56,7 @@ test_expect_success 'two-way reset should remove remnants too' '
 	git ls-files -s &&
 	read_tree_u_must_succeed --reset -u HEAD HEAD &&
 	git ls-files -s >actual &&
-	! test -f old &&
+	! test_path_is_file old &&
 	test_cmp expect actual
 '
 
@@ -72,7 +72,7 @@ test_expect_success 'Porcelain reset should remove remnants too' '
 	git ls-files -s &&
 	git reset --hard &&
 	git ls-files -s >actual &&
-	! test -f old &&
+	! test_path_is_file old &&
 	test_cmp expect actual
 '
 
@@ -88,7 +88,7 @@ test_expect_success 'Porcelain checkout -f should remove remnants too' '
 	git ls-files -s &&
 	git checkout -f &&
 	git ls-files -s >actual &&
-	! test -f old &&
+	! test_path_is_file old &&
 	test_cmp expect actual
 '
 
@@ -104,7 +104,7 @@ test_expect_success 'Porcelain checkout -f HEAD should remove remnants too' '
 	git ls-files -s &&
 	git checkout -f HEAD &&
 	git ls-files -s >actual &&
-	! test -f old &&
+	! test_path_is_file old &&
 	test_cmp expect actual
 '
 
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 51a85e83c27..9820d2348bc 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1236,11 +1236,11 @@ test_expect_success SYMLINKS 'symlinked configuration' '
 	ln -s notyet myconfig &&
 	git config --file=myconfig test.frotz nitfol &&
 	test -h myconfig &&
-	test -f notyet &&
+	test_path_is_file notyet &&
 	test "z$(git config --file=notyet test.frotz)" = znitfol &&
 	git config --file=myconfig test.xyzzy rezrov &&
 	test -h myconfig &&
-	test -f notyet &&
+	test_path_is_file notyet &&
 	cat >expect <<-\EOF &&
 	nitfol
 	rezrov
diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh
index 9d698b3cc35..12f7b600244 100755
--- a/t/t1403-show-ref.sh
+++ b/t/t1403-show-ref.sh
@@ -196,7 +196,7 @@ test_expect_success 'show-ref --verify with dangling ref' '
 
 	remove_object() {
 		file=$(sha1_file "$*") &&
-		test -e "$file" &&
+		test_path_exists "$file" &&
 		rm -f "$file"
 	} &&
 
diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
index 388fdf9ae57..429ff59d2cb 100755
--- a/t/t1410-reflog.sh
+++ b/t/t1410-reflog.sh
@@ -130,10 +130,10 @@ test_expect_success 'pass through -- to sub-command' '
 
 test_expect_success rewind '
 	test_tick && git reset --hard HEAD~2 &&
-	test -f C &&
-	test -f A/B/E &&
-	! test -f F &&
-	! test -f A/G &&
+	test_path_is_file C &&
+	test_path_is_file A/B/E &&
+	! test_path_is_file F &&
+	! test_path_is_file A/G &&
 
 	check_have A B C D E F G H I J K L &&
 
diff --git a/t/t1420-lost-found.sh b/t/t1420-lost-found.sh
index 2fb2f44f021..5fbb1d10ede 100755
--- a/t/t1420-lost-found.sh
+++ b/t/t1420-lost-found.sh
@@ -29,8 +29,8 @@ test_expect_success 'lost and found something' '
 	git reset --hard HEAD^ &&
 	git fsck --lost-found &&
 	test 2 = $(ls .git/lost-found/*/* | wc -l) &&
-	test -f .git/lost-found/commit/$(cat lost-commit) &&
-	test -f .git/lost-found/other/$(cat lost-other)
+	test_path_is_file .git/lost-found/commit/$(cat lost-commit) &&
+	test_path_is_file .git/lost-found/other/$(cat lost-other)
 '
 
 test_done
diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
index ac4a5b2734c..38d6f19152a 100755
--- a/t/t1700-split-index.sh
+++ b/t/t1700-split-index.sh
@@ -460,7 +460,7 @@ test_expect_success POSIXPERM,SANITY 'graceful handling when splitting index is
 		cd ro &&
 		test_commit initial &&
 		git update-index --split-index &&
-		test -f .git/sharedindex.*
+		test_path_is_file .git/sharedindex.*
 	) &&
 	cp ro/.git/index new-index &&
 	test_when_finished "chmod u+w ro/.git" &&

base-commit: 683c54c999c301c2cd6f715c411407c413b1d84e
-- 
gitgitgadget

Re: [PATCH] [GSoC Patch] Modernize Test Path Checking in Git’s Test Suite

[Junio C Hamano]

"Sampriyo Guin via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Sampriyo Guin <sampriyoguin@gmail.com>
>
> test -(e|f|d) does not provide a proper error message when hit
> test failures. So test_path_exists, test_path_is_dir,
> test_parh_is_file used.

Correct but ...

> diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh
> index 2b60317758c..6a8fe69c089 100755
> --- a/t/t0007-git-var.sh
> +++ b/t/t0007-git-var.sh
> @@ -156,7 +156,7 @@ test_expect_success POSIXPERM 'GIT_SHELL_PATH points to a valid executable' '
>  test_expect_success MINGW 'GIT_SHELL_PATH points to a suitable shell' '
>  	shellpath=$(git var GIT_SHELL_PATH) &&
>  	case "$shellpath" in
> -	[A-Z]:/*/sh.exe) test -f "$shellpath";;
> +	[A-Z]:/*/sh.exe) test_path_is_file "$shellpath";;
>  	*) return 1;;
>  	esac
>  '

... this one is iffy.  How well does it mesh with the "return 1"
case in the same case/esac?  I do not use Windows, but if
GIT_SHELL_PATH set by that system is not in a form that the platform
expects (i.e. a drive letter and then path to a file "sh.exe"), it
is just as grave a problem worth reporting as the path given is not
a regular file, yet "return 1" case does not give any specific error
message (instead it just lets the test fail), so it feels a bit
uneven to make the "test -f" failure alone louder than it currently
is.

> diff --git a/t/t0601-reffiles-pack-refs.sh b/t/t0601-reffiles-pack-refs.sh
> index aa7f6ecd813..f76471a3375 100755
> --- a/t/t0601-reffiles-pack-refs.sh
> +++ b/t/t0601-reffiles-pack-refs.sh
> @@ -78,13 +78,13 @@ test_expect_success 'see if a branch still exists after git pack-refs --prune' '
>  test_expect_success 'see if git pack-refs --prune remove ref files' '
>  	git branch f &&
>  	git pack-refs --all --prune &&
> -	! test -f .git/refs/heads/f
> +	! test_path_is_file .git/refs/heads/f
>  '

This conversion is wrong.  We are expecting that the file does *not*
exist, and your complaint and the reason why you are rewriting this
line is that "! test -f" does not give loud message when that file
does exist.  What does "! test_path_is_file foo" do when 'foo'
exists?

You can find the implementation of test_path_is_file with "git grep"
and see for yourself.

    # debugging-friendly alternatives to "test [-f|-d|-e]"
    # The commands test the existence or non-existence of $1
    test_path_is_file () {
            test "$#" -ne 1 && BUG "1 param"
            if ! test -f "$1"
            then
                    echo "File $1 doesn't exist"
                    false
            fi
    }

The test wants to make sure that 'f' is not a file.  So you want to
see a loud complaint when 'f' exists as a file.  Does it do so when
you say

	! test_path_is_file .git/refs/heads/f

in this test?  No, it will not enter the "then" block and silently
succeed, and that return status is negated by that "!", so the test
will notice that the expectation is not met, but you didn't achieve
your goal of making it louder when it fail.  Worse, if the file is
not there, as the test expects when Git is working correctly, your

	! test_path_is_file .git/refs/heads/f

will enter the "then" block, complain that the file does not exist,
returns a failure, and your "!" will turn it into a success.  The
test passes, but the user is given an error message when the test is
run with "-v" option.

>  test_expect_success 'see if git pack-refs --prune removes empty dirs' '
>  	git branch r/s/t &&
>  	git pack-refs --all --prune &&
> -	! test -e .git/refs/heads/r
> +	! test_path_exists .git/refs/heads/r
>  '

Ditto.

I'll stop here.  I think all "! test_path_foo" changes in this patch
are wrong.

Unlike "test_grep" that lets you write "test_grep ! foo bar" to make
sure that file 'bar' has no 'foo' in it (and complains loudly if
'foo' appears in 'bar'), test_path_foo family of helper functions do
not let you write "test_path_exists ! no-such-file" unfortunately.
So my recommendation for a microproject sample is to just drop these
negations from the changes and stop there.

If this were a patch for real, it would make sense to give a
preliminary update to test_path_foo helpers to allow them to take
negated "test_path_exists ! no-such-file" form, and then convert
the negative form as well, but I think that would be a bit more than
what a typical microproject would take.

Thanks.

Re: [PATCH] [GSoC Patch] Modernize Test Path Checking in Git’s Test Suite

[Eric Sunshine]

On Tue, Mar 18, 2025 at 5:14 PM Junio C Hamano <gitster@pobox.com> wrote:
> "Sampriyo Guin via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >  test_expect_success MINGW 'GIT_SHELL_PATH points to a suitable shell' '
> >       shellpath=$(git var GIT_SHELL_PATH) &&
> >       case "$shellpath" in
> > -     [A-Z]:/*/sh.exe) test -f "$shellpath";;
> > +     [A-Z]:/*/sh.exe) test_path_is_file "$shellpath";;
> >       *) return 1;;
> >       esac
> >  '
>
> ... this one is iffy.  How well does it mesh with the "return 1"
> case in the same case/esac?  I do not use Windows, but if
> GIT_SHELL_PATH set by that system is not in a form that the platform
> expects (i.e. a drive letter and then path to a file "sh.exe"), it
> is just as grave a problem worth reporting as the path given is not
> a regular file, yet "return 1" case does not give any specific error
> message (instead it just lets the test fail), so it feels a bit
> uneven to make the "test -f" failure alone louder than it currently
> is.

Thanks for stating what I was thinking upon seeing this change.

> >  test_expect_success 'see if git pack-refs --prune remove ref files' '
> >       git branch f &&
> >       git pack-refs --all --prune &&
> > -     ! test -f .git/refs/heads/f
> > +     ! test_path_is_file .git/refs/heads/f
> >  '
>
> This conversion is wrong.  [...]
>
>     test_path_is_file () {
>             test "$#" -ne 1 && BUG "1 param"
>             if ! test -f "$1"
>             then
>                     echo "File $1 doesn't exist"
>                     false
>             fi
>     }
>
> The test wants to make sure that 'f' is not a file.  So you want to
> see a loud complaint when 'f' exists as a file.  Does it do so when
> you say
>
>         ! test_path_is_file .git/refs/heads/f
>
> in this test?  No, it will not enter the "then" block and silently
> succeed, and that return status is negated by that "!", so the test
> will notice that the expectation is not met, but you didn't achieve
> your goal of making it louder when it fail.  Worse, if the file is
> not there, as the test expects when Git is working correctly, your
>
>         ! test_path_is_file .git/refs/heads/f
>
> will enter the "then" block, complain that the file does not exist,
> returns a failure, and your "!" will turn it into a success.  The
> test passes, but the user is given an error message when the test is
> run with "-v" option.

And, again, you've said everything I was going to say, thus saving me
the effort of doing so.

Referring to the other thread at [*], perhaps this (avoiding `!` in
front of test_path_*) is yet another clarification which ought to be
added to the microproject description in order to lead candidates in a
more profitable direction.

[*]: https://lore.kernel.org/git/CAPig+cRm+sc+Rk-4SuQ5CrPeZLG2Nzz9B7+6OZxCq7tV5mzmBA@mail.gmail.com/

> I'll stop here.  I think all "! test_path_foo" changes in this patch
> are wrong.
>
> Unlike "test_grep" that lets you write "test_grep ! foo bar" to make
> sure that file 'bar' has no 'foo' in it (and complains loudly if
> 'foo' appears in 'bar'), test_path_foo family of helper functions do
> not let you write "test_path_exists ! no-such-file" unfortunately.
> So my recommendation for a microproject sample is to just drop these
> negations from the changes and stop there.

One other recommendation I would make is to restrict the microproject
submission to just a single test script (rather than updating twelve
of them) in order to avoid exhausting the pool for other potential
candidates.

Re: [PATCH] [GSoC Patch] Modernize Test Path Checking in Git’s Test Suite

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> Referring to the other thread at [*], perhaps this (avoiding `!` in
> front of test_path_*) is yet another clarification which ought to be
> added to the microproject description in order to lead candidates in a
> more profitable direction.
>
> [*]: https://lore.kernel.org/git/CAPig+cRm+sc+Rk-4SuQ5CrPeZLG2Nzz9B7+6OZxCq7tV5mzmBA@mail.gmail.com/

While mentors do that, I'd prefer to see others extend support for 

	test_path_is_file ! this-should-not-be-file

so that the students have a better tool to work with.

> One other recommendation I would make is to restrict the microproject
> submission to just a single test script (rather than updating twelve
> of them) in order to avoid exhausting the pool for other potential
> candidates.

Yes, you made that point in the other thread, and I agree with it
100%.

Thanks.