Re: [GSoC][PATCH] t/t5000-tar-tree: add helper function

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Feb 2, 2023, 23:09 UTC
Message-ID: <CAPig+cTC4TUK2NXFmP6ymEYYjYAmCSd53dzFhKn0ido8x6uh1Q@mail.gmail.com>
In-Reply-To: <xmqqa61vsniy.fsf@gitster.g>

On Thu, Feb 2, 2023 at 5:37 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 15 quoted lines

> Kostya Farber <kostya.farber@gmail.com> writes:
> > Add the helper function test_file_path_exists to the
> > interpret pax header test. This change makes it clearer
> > as to what the test is trying to check, in this case whether
> > a file path exists.
> >
> > -                             if test -h $data || test -e $data
> > +                             if test -h $data || test_file_path_exists $data
>
> Nothing seems to be adding a new helper whose name is
> test_file_path_exists; the patch expects such a helper already
> exists and uses it in place for existing "test -e".
>
> Perhaps you meant to say "use test_path_exists" not "add helper" on
> the title, and use that function in the patch instead?

A couple comments...

The test framework does not define a function named "test_file_path_exists". Probably "test_path_exists" was intended.

Delving more deeply, though, this change seems undesirable from a clarity viewpoint. The function "test_path_exists" is an assertion; its purpose is to make the test fail if the path is expected to exist but doesn't. However, in the original code from t5000:

    if test -h $data || test -e $data
    then
        path=$(get_pax_header $header path) &&
        if test -n "$path"
        then
            mv "$data" "$path" || exit 1
        fi
    fi

it is perfectly fine if the path is neither a symbolic link nor an actual file; that's not considered an error. Therefore, using an assertion function -- which suggests test failure -- muddles the intent of the code rather than clarifying it.

Additionally, t/test-lib-functions.sh also defines the function "test_path_is_symlink" which would seem to be the obvious complement to "test_path_exists", thus one might have expected the patch to change the code to:

    if test_path_is_symlink $data || test_path_exists $data
    then
        ...

however, "test_path_is_symlink" is also an assertion, thus not really suitable for this case in which it is acceptable (not an error) if neither condition holds true.

So, t5000 seems to be one of those relatively rare cases in which the raw "test" command is more correct than the higher-level helper functions.