On Mon, Mar 3, 2025 at 7:49 PM Prachit Ingle <ingleprachit101@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxx/
2. https://git-scm.com/docs/MyFirstContribution
3. https://git-scm.com/docs/SubmittingPatches
