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

[Eric Sunshine <sunshine@sunshineco.com>]

On Thu, Feb 2, 2023 at 5:37 PM Junio C Hamano <gitster@pobox.com> wrote:
> 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.