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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 3, 2025, 20:57 UTC
Message-ID: <CAPig+cTJYsXTSXHyUEDcG+qxUNdh5aVUV7FwC=gHDbUX0mpZsA@mail.gmail.com>
In-Reply-To: <20250303141800.12848-1-ingleprachit101@gmail.com>

On Mon, Mar 3, 2025 at 9:20 AM Prachit Ingle <ingleprachit101@gmail.com> wrote:

Show 20 quoted lines

> 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

Show 27 quoted lines

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

Show 9 quoted lines

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