On Mon, Mar 3, 2025 at 9:20 AM 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
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@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/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.
