Re: [PATCH 1/1] [GSoC][PATCH] t3070: refactor test -e command

On Thu, Feb 29, 2024 at 10:05 AM shejialuo <shejialuo@gmail.com> wrote:

Thanks for providing this GSoC submission. The aim of this patch makes sense, but it turns out that t3070 is not a good choice for this exercise. Before getting into that, though, a few minor comments about the commit message.

This patch isn't actually refactoring the code, so using "refactor" in the title is misleading.

Rather than mentioning only the object-ID, we normally reference other commits like this (using `git log --pretty=reference -1 <object-id>`):

    7e9055bb00 (t7406: prefer test_* helper functions to test -[feds],
2018-08-08)

In this case, it's not clear why you chose to reference that particular commit over any of the others which make similar changes. It probably would be simpler to drop mention of that commit and just copy its reasoning into your commit message.

Taking all the above into account, a possible rewrite of the commit message might be:

    t3070: prefer test_path_exists helper function

    test -e does not provide a nice error message when we hit test
    failures, so use test_path_exists instead.

The point of functions such as test_path_exists() 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. Here is how test_path_exists() is defined:

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

It is meant to replace noisy code such as:

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

with much simpler:

    test_path_exists bloop &&
    other-code

It is also meant to be used within `test_expect_success` (or `test_expect_failure`) blocks. So, the changes made by this patch are undesirable for a couple reasons...

First, this code is outside a `test_expect_success` (or `test_expect_failure`) block.

Second, as noted above, test_path_exists() is an _assertion_ which requires the file to exist, and aborts the test if the file does not exist. But the `test -e` being changed here is part of the proper control-flow of this logic; it is not asserting anything, but merely branching to one or another part of the code depending upon the result of the `test -e` test. Thus, replacing this control-flow check with the assertion function test_path_exists() changes the logic in an undesirable way.

The above comments are applicable to most of the changes made by this patch. The only exceptions are the last two changes...

... which _do_ use test_path_exists() within a `test_expect_success` block. However, the changes are still undesirable because, as above, this `test -e` is merely part of the normal control-flow; it's not acting as an assertion, thus test_path_exists() -- which is an assertion -- is not correct.

Unfortunately, none of the uses of`test -e` in t3070 are being used as assertions worthy of replacement with test_path_exists(), thus this isn't a good script in which to make such changes. If you reroll, you may be able to find a good candidate script by searching for code which looks something like this:

    foo &&
    test -e path &&
    bar &&

and replacing it with:

    foo &&
    test_path_exists path &&
    bar &&