On Thu, Feb 2, 2023 at 10:36 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Kostya Farber <kostya.farber@xxxxxxxxx> 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.
>
> Really?
>
> The code with "test -e" is already clear that it is checking if the
> path $data exists. This change does not make it any clearer. What
> it helps is that it gives a message upon failure, when the test is
> run with the "-v" option.
Okay, noted. I will look into the helper functions more closely to
understand why and how they useful compared to other methods (i.e test
-e in this instance)
> > diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
> > index d473048138..19d5bd0c04 100755
> > --- a/t/t5000-tar-tree.sh
> > +++ b/t/t5000-tar-tree.sh
> > @@ -73,7 +73,7 @@ check_tar() {
> > for header in *.paxheader
> > do
> > data=${header%.paxheader}.data &&
> > - if test -h $data || test -e $data
> > + if test -h $data || test_file_path_exists $data
> > then
> > path=$(get_pax_header $header path) &&
> > if test -n "$path"
>
> 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?
Yes you are right. I made a mistake by using the wrong function name
and I think "use test_path_exists" was my intention as a title name.
> Thanks.
