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

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