Re: [PATCH] t9146: replace test -d/-f with appropriate test_path_is_* function

On Sun, Feb 11, 2024 at 9:53 AM Chandra Pratap via GitGitGadget <gitgitgadget@gmail.com> wrote:

The aim of this patch makes sense, but the implementation needs some refinement...

The point of functions such as test_path_is_missing() is to _assert_ that some condition is true, thus allowing the test to succeed; if the condition is not true, then the function prints an error message and the test aborts and fails.

    test_path_is_missing () {
        if test -e "$1"
        then
            echo "Path exists:"
            ls -ld "$1"
            false
        fi
    }

It is meant to replace noisy code such as:

    if ! test -f bloop
    then
        echo >&2 "error message" &&
        exit 1
    fi &&
    other-code

with much simpler:

    test_path_exists bloop &&
    other-code

So, the changes made by this patch are incorrect in two ways...

First, the patch retains code which prints an error message even though this code becomes redundant since the test_path_foo() functions already take care of printing the error message.

Second, and more problematic, the patch incorrectly inverts the sense of what is being tested. For instance, the title of this test is "empty directories exist", and the body of the test asserts that the empty directories _do_ exist, but the patch changes the condition to assert that the directories do _not_ exist, which is wrong.

Taking these observations into account, this test should become:

    test_expect_success 'empty directories exist' '
        (
            cd cloned &&
            for i in a b c d d/e d/e/f "weird file name"
            do
                test_path_exists "$i" || exit 1
            done
        )
    '

Many of the other changes made by this patch suffer similar problems

This change is wrong/undesirable for a different reason. Taking into account what was said above about test_path_is_file() being an _assertion_ that some condition is true, coupled with the comment above this `if` statement which says "Compress::Zlib may not be available", then this `test -f` is legitimately part of the control-flow of the function. It is not a mere assertion. Thus, replacing it with the assertion function test_path_is_file() breaks the test for the case when Compress::Zlib is not available.

These replacements are correct in that they replace the _assertion_ `test -f` with the equivalent assertion `test_path_is_file`.