t7800 test failure

t7800 test failure

[Armin Kunaschik]

t7800 fails on systems where readlink (GNUism?) is not available.
An easy workaround for the very basic readlink usage of this test
would be to use a shell function like this:

---
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index ff7a9e9..be3d19f 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -420,6 +420,7 @@ run_dir_diff_test 'difftool --dir-diff when
worktree file is missing' '
 '

 write_script .git/CHECK_SYMLINKS <<\EOF
+readlink() { ls -ld "$1" | sed 's/.* -> //'; }
 for f in file file2 sub/sub
 do
        echo "$f"

Re: t7800 test failure

[Matthieu Moy]

Armin Kunaschik <megabreit@googlemail.com> writes:

> t7800 fails on systems where readlink (GNUism?) is not available.

I don't think it's POSIX, but it is present on all POSIX-like systems I
know. On which system did you get the issue?

> +readlink() { ls -ld "$1" | sed 's/.* -> //'; }

This is much less robust than the actual readlink. For example, if ->
appears in the link name, it breaks.

It would be acceptable as a fall-back if readlink is not present, but
shouldn't activate the "ls" hack by default.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: t7800 test failure

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Armin Kunaschik <megabreit@googlemail.com> writes:
>
>> t7800 fails on systems where readlink (GNUism?) is not available.
>
> I don't think it's POSIX, but it is present on all POSIX-like systems I
> know. On which system did you get the issue?
>
>> +readlink() { ls -ld "$1" | sed 's/.* -> //'; }
>
> This is much less robust than the actual readlink. For example, if ->
> appears in the link name, it breaks.

I wouldn't allow it in our scripted Porcelain, but the environment
of our test scripts are under our control, so I do not think it is a
problem ("ls piped to sed" has been an established idiom before
readlink(1) was widely accepted, by the way).

> It would be acceptable as a fall-back if readlink is not present, but
> shouldn't activate the "ls" hack by default.

Yup.

Re: t7800 test failure

[Armin Kunaschik]

On Tue, May 24, 2016 at 6:57 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> Armin Kunaschik <megabreit@googlemail.com> writes:
>>
>>> t7800 fails on systems where readlink (GNUism?) is not available.
>>
>> I don't think it's POSIX, but it is present on all POSIX-like systems I
>> know. On which system did you get the issue?

It's not available in AIX or HP-UX.

>>> +readlink() { ls -ld "$1" | sed 's/.* -> //'; }
>>
>> This is much less robust than the actual readlink. For example, if ->
>> appears in the link name, it breaks.
>
> I wouldn't allow it in our scripted Porcelain, but the environment
> of our test scripts are under our control, so I do not think it is a
> problem ("ls piped to sed" has been an established idiom before
> readlink(1) was widely accepted, by the way).

I think so too. Maybe I can improve the sed expression a bit, but
it will never be a universal readlink replacement. But it doesn't have to.
It's defined locally for this one test only and it does the specific job.

>> It would be acceptable as a fall-back if readlink is not present, but
>> shouldn't activate the "ls" hack by default.
>
> Yup.

Ok, how can this be implemented within the test environment?

Re: t7800 test failure

[Junio C Hamano]

Armin Kunaschik <megabreit@googlemail.com> writes:

>> I wouldn't allow it in our scripted Porcelain, but the environment
>> of our test scripts are under our control, so I do not think it is a
>> problem ("ls piped to sed" has been an established idiom before
>> readlink(1) was widely accepted, by the way).
>
> I think so too. Maybe I can improve the sed expression a bit, but
> it will never be a universal readlink replacement. But it doesn't have to.
> It's defined locally for this one test only and it does the specific job.
>
>>> It would be acceptable as a fall-back if readlink is not present, but
>>> shouldn't activate the "ls" hack by default.
>>
>> Yup.
>
> Ok, how can this be implemented within the test environment?

I actually think an unconditional check like this is sufficient.

 t/t7800-difftool.sh | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 7ce4cd7..f304228 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -442,15 +442,16 @@ run_dir_diff_test 'difftool --dir-diff with unmerged files' '
 	test_cmp expect actual
 '
 
-write_script .git/CHECK_SYMLINKS <<\EOF
-for f in file file2 sub/sub
-do
-	echo "$f"
-	readlink "$2/$f"
-done >actual
-EOF
-
 test_expect_success PERL,SYMLINKS 'difftool --dir-diff --symlink without unstaged changes' '
+
+	write_script .git/CHECK_SYMLINKS <<-\EOF &&
+	for f in file file2 sub/sub
+	do
+		echo "$f"
+		ls -ld "$2/$f" | sed -e "s/.* -> //"
+	done >actual
+	EOF
+
 	cat >expect <<-EOF &&
 	file
 	$PWD/file

Re: t7800 test failure

[Armin Kunaschik]

On Tue, May 24, 2016 at 7:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Armin Kunaschik <megabreit@googlemail.com> writes:
>>
>> Ok, how can this be implemented within the test environment?
>
> I actually think an unconditional check like this is sufficient.

Ah, good. My thoughts were a bit more complicated.
Anyway, this works for me.
Thanks!

>  t/t7800-difftool.sh | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
> index 7ce4cd7..f304228 100755
> --- a/t/t7800-difftool.sh
> +++ b/t/t7800-difftool.sh
> @@ -442,15 +442,16 @@ run_dir_diff_test 'difftool --dir-diff with unmerged files' '
>         test_cmp expect actual
>  '
>
> -write_script .git/CHECK_SYMLINKS <<\EOF
> -for f in file file2 sub/sub
> -do
> -       echo "$f"
> -       readlink "$2/$f"
> -done >actual
> -EOF
> -
>  test_expect_success PERL,SYMLINKS 'difftool --dir-diff --symlink without unstaged changes' '
> +
> +       write_script .git/CHECK_SYMLINKS <<-\EOF &&
> +       for f in file file2 sub/sub
> +       do
> +               echo "$f"
> +               ls -ld "$2/$f" | sed -e "s/.* -> //"
> +       done >actual
> +       EOF
> +
>         cat >expect <<-EOF &&
>         file
>         $PWD/file

Re: t7800 test failure

[David Aguilar]

On Wed, May 25, 2016 at 11:33:33AM +0200, Armin Kunaschik wrote:
> On Tue, May 24, 2016 at 7:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
> > Armin Kunaschik <megabreit@googlemail.com> writes:
> >>
> >> Ok, how can this be implemented within the test environment?
> >
> > I actually think an unconditional check like this is sufficient.
> 
> Ah, good. My thoughts were a bit more complicated.
> Anyway, this works for me.
> Thanks!

Would you mind submitting a patch so that we can support these
tests when running on AIX/HP-UX?


> >  t/t7800-difftool.sh | 17 +++++++++--------
> >  1 file changed, 9 insertions(+), 8 deletions(-)
> >
> > diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
> > index 7ce4cd7..f304228 100755
> > --- a/t/t7800-difftool.sh
> > +++ b/t/t7800-difftool.sh
> > @@ -442,15 +442,16 @@ run_dir_diff_test 'difftool --dir-diff with unmerged files' '
> >         test_cmp expect actual
> >  '
> >
> > -write_script .git/CHECK_SYMLINKS <<\EOF
> > -for f in file file2 sub/sub
> > -do
> > -       echo "$f"
> > -       readlink "$2/$f"
> > -done >actual
> > -EOF
> > -
> >  test_expect_success PERL,SYMLINKS 'difftool --dir-diff --symlink without unstaged changes' '
> > +
> > +       write_script .git/CHECK_SYMLINKS <<-\EOF &&
> > +       for f in file file2 sub/sub
> > +       do
> > +               echo "$f"
> > +               ls -ld "$2/$f" | sed -e "s/.* -> //"
> > +       done >actual
> > +       EOF
> > +
> >         cat >expect <<-EOF &&
> >         file
> >         $PWD/file

I was curious so I whipped together a small tweak to
t/check-non-portable-shell.pl below.

The difftool tests are not the only ones that use readlink.
My guess is you haven't run the p4 tests because AIX/HP-UX doesn't have p4?

	$ make test-lint-shell-syntax
	t7800-difftool.sh:449: error: readlink is not portable (please use ls -ld | sed):       readlink "$2/$f"
	t9802-git-p4-filetype.sh:266: error: readlink is not portable (please use ls -ld | sed):                test $(readlink symlink) = symlink-target
	t9802-git-p4-filetype.sh:332: error: readlink is not portable (please use ls -ld | sed):                test $(readlink empty-symlink) = target2
	test-lib.sh:757: error: readlink is not portable (please use ls -ld | sed):             test "$1" = "$(readlink "$2")" || {

If we want to ban use of readlink from the test suite we could
add it to the check script.  test-lib.sh also includes it for
valgrind support so I'm not really sure whether we'd want to
apply this, but I figured I'd bring it up for discussion.

If we end up fixing all of these then I can send this to the
list as a proper patch.

Curious, is there an easy way to get readlink and mktemp installed on AIX?

Another alternative is that we can compile our own
"git-readlink" and "git-mktemp" programs and use those instead,
but that seems like a big maintenance burden compared to the
relative simplicity of the test-suite workarounds.

Thanks for fixing my non-portable tests ;-)

--- 8< --- 8< ---
From 40c41402dfa24ca16b41062172c34f238d77b42c Mon Sep 17 00:00:00 2001
From: David Aguilar <davvid@gmail.com>
Date: Thu, 26 May 2016 02:42:52 -0700
Subject: [PATCH] tests: add shell portability check for "readlink"

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 t/check-non-portable-shell.pl | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
index b170cbc..2707e42 100755
--- a/t/check-non-portable-shell.pl
+++ b/t/check-non-portable-shell.pl
@@ -18,6 +18,7 @@ while (<>) {
 	chomp;
 	/\bsed\s+-i/ and err 'sed -i is not portable';
 	/\becho\s+-n/ and err 'echo -n is not portable (please use printf)';
+	/\breadlink\s+/ and err 'readlink is not portable (please use ls -ld | sed)';
 	/^\s*declare\s+/ and err 'arrays/declare not portable';
 	/^\s*[^#]\s*which\s/ and err 'which is not portable (please use type)';
 	/\btest\s+[^=]*==/ and err '"test a == b" is not portable (please use =)';
-- 
2.7.0.rc3

-- 
David

Re: t7800 test failure

[Matthieu Moy]

David Aguilar <davvid@gmail.com> writes:

> Another alternative is that we can compile our own
> "git-readlink" and "git-mktemp" programs and use those instead,
> but that seems like a big maintenance burden compared to the
> relative simplicity of the test-suite workarounds.

Not _that_ big burden as we already have the necessary infrastructure:
git-mktemp would be t/helper/test-mktemp.c already available to tests as
test-mktemp, and it would be easy to do it for readlink too.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

[PATCH] t7800 readlink not found

[Armin Kunaschik]

On 05/27/2016 06:19 AM, David Aguilar wrote:
> On Wed, May 25, 2016 at 11:33:33AM +0200, Armin Kunaschik wrote:
> 
> Would you mind submitting a patch so that we can support these
> tests when running on AIX/HP-UX?

I don't feel comfortable to submit patches for tests I can't verify. I
don't have valgrind and python/p4 here. Looking to the code I'd say,
patching the p4 tests with "ls -ld | sed" looks quite save.
But I'm not sure about the test-lib.sh. When you are really super
paranoid, as written in the comment, you should probably use perl like

perl -e 'print readlink $ARGV[0]' $name

as a replacement.

So, as suggested by Junio, here the readlink workaround for t7800 only.
(hopefully whitespace clean this time)

--- 8< --- 8< ---
From: Armin Kunaschik <megabreit@googlemail.com>
Subject: t7800: readlink is not portable

The readlink(1) command is not available on all platforms (notably not
on AIX and HP-UX) and can be replaced in this test with the "workaround"

ls -ld <name> | sed -e 's/.* -> //'

This is no universal readlink replacement but works in the controlled
test environment good enough.

Signed-off-by: Armin Kunaschik <megabreit@googlemail.com>
---

diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 7ce4cd7..905035c 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -446,7 +446,7 @@ write_script .git/CHECK_SYMLINKS <<\EOF
 for f in file file2 sub/sub
 do
 	echo "$f"
-	readlink "$2/$f"
+	ls -ld "$2/$f" | sed -e 's/.* -> //'
 done >actual
 EOF

Re: [PATCH] t7800 readlink not found

[Torsten Bögershausen]

On 05/31/2016 02:26 AM, Armin Kunaschik wrote:
> On 05/27/2016 06:19 AM, David Aguilar wrote:
>> On Wed, May 25, 2016 at 11:33:33AM +0200, Armin Kunaschik wrote:
>>
>> Would you mind submitting a patch so that we can support these
>> tests when running on AIX/HP-UX?
> I don't feel comfortable to submit patches for tests I can't verify. I
> don't have valgrind and python/p4 here. Looking to the code I'd say,
> patching the p4 tests with "ls -ld | sed" looks quite save.
> But I'm not sure about the test-lib.sh. When you are really super
> paranoid, as written in the comment, you should probably use perl like
>
> perl -e 'print readlink $ARGV[0]' $name
>
> as a replacement.
>
> So, as suggested by Junio, here the readlink workaround for t7800 only.
> (hopefully whitespace clean this time)
>
> --- 8< --- 8< ---
> From: Armin Kunaschik <megabreit@googlemail.com>
> Subject: t7800: readlink is not portable
>
> The readlink(1) command is not available on all platforms (notably not
> on AIX and HP-UX) and can be replaced in this test with the "workaround"
>
> ls -ld <name> | sed -e 's/.* -> //'
>
> This is no universal readlink replacement but works in the controlled
> test environment good enough.
>
> Signed-off-by: Armin Kunaschik <megabreit@googlemail.com>
> ---
>
> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
> index 7ce4cd7..905035c 100755
> --- a/t/t7800-difftool.sh
> +++ b/t/t7800-difftool.sh
> @@ -446,7 +446,7 @@ write_script .git/CHECK_SYMLINKS <<\EOF
>   for f in file file2 sub/sub
>   do
>   	echo "$f"
> -	readlink "$2/$f"
> +	ls -ld "$2/$f" | sed -e 's/.* -> //'
>   done >actual
>   EOF
>
I don't know how portable #ls -ld" really is.
If there is one platform, that doesn't support readlink, would it
make sense to implement readlink() in test-lib.sh,
similar to what we have for MINGW, e.g. sort() or find() ?
And keep t7800 as it is ?

Re: [PATCH] t7800 readlink not found

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

>> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
>> index 7ce4cd7..905035c 100755
>> --- a/t/t7800-difftool.sh
>> +++ b/t/t7800-difftool.sh
>> @@ -446,7 +446,7 @@ write_script .git/CHECK_SYMLINKS <<\EOF
>>   for f in file file2 sub/sub
>>   do
>>   	echo "$f"
>> -	readlink "$2/$f"
>> +	ls -ld "$2/$f" | sed -e 's/.* -> //'
>>   done >actual
>>   EOF
>>
> I don't know how portable #ls -ld" really is.

The parts with mode bits, nlinks, uid, gid, size, and date part do
have some variations.  For example, we have been burned on ACL
enabled systems having some funny suffix after the usual mode bits
stuff.

However, as far as this test is concerned, I do not think "how
portable is the output from ls -ld" is an especially relevant
question.  None of the things we expect early in the output (the
fields I enumerated in the previous paragraph) would contain " -> ".
And we know that we do not use a filename that has " -> " (or "->")
as a substring in our tests.

We don't have to use readlink, even on platforms where we do have
readlink.  Building the conditional to be checked at runtime and
providing a shell function read_link that uses "ls -ld | sed" or
"readlink" depending on the runtime check is wasteful.

Re: [PATCH] t7800 readlink not found

[Armin Kunaschik]

On Tue, May 31, 2016 at 7:51 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Torsten Bögershausen <tboegi@web.de> writes:
>
>>> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
>>> index 7ce4cd7..905035c 100755
>>> --- a/t/t7800-difftool.sh
>>> +++ b/t/t7800-difftool.sh
>>> @@ -446,7 +446,7 @@ write_script .git/CHECK_SYMLINKS <<\EOF
>>>   for f in file file2 sub/sub
>>>   do
>>>      echo "$f"
>>> -    readlink "$2/$f"
>>> +    ls -ld "$2/$f" | sed -e 's/.* -> //'
>>>   done >actual
>>>   EOF
>>>
>> I don't know how portable #ls -ld" really is.
>
> The parts with mode bits, nlinks, uid, gid, size, and date part do
> have some variations.  For example, we have been burned on ACL
> enabled systems having some funny suffix after the usual mode bits
> stuff.
>
> However, as far as this test is concerned, I do not think "how
> portable is the output from ls -ld" is an especially relevant
> question.  None of the things we expect early in the output (the
> fields I enumerated in the previous paragraph) would contain " -> ".
> And we know that we do not use a filename that has " -> " (or "->")
> as a substring in our tests.
>
> We don't have to use readlink, even on platforms where we do have
> readlink.  Building the conditional to be checked at runtime and
> providing a shell function read_link that uses "ls -ld | sed" or
> "readlink" depending on the runtime check is wasteful.
>

Just a short, curious question: Is this patch to be accepted/included some time?
I didn't see it in 2.8.4 nor 2.9.0. Maybe it just fell off the table...

Armin

Re: [PATCH] t7800 readlink not found

[Junio C Hamano]

Armin Kunaschik <megabreit@googlemail.com> writes:

> On Tue, May 31, 2016 at 7:51 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Torsten Bögershausen <tboegi@web.de> writes:
>>
>>>> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
>>>> index 7ce4cd7..905035c 100755
>>>> --- a/t/t7800-difftool.sh
>>>> +++ b/t/t7800-difftool.sh
>>>> @@ -446,7 +446,7 @@ write_script .git/CHECK_SYMLINKS <<\EOF
>>>>   for f in file file2 sub/sub
>>>>   do
>>>>      echo "$f"
>>>> -    readlink "$2/$f"
>>>> +    ls -ld "$2/$f" | sed -e 's/.* -> //'
>>>>   done >actual
>>>>   EOF
>>>>
>>> I don't know how portable #ls -ld" really is.
>>
>> The parts with mode bits, nlinks, uid, gid, size, and date part do
>> have some variations.  For example, we have been burned on ACL
>> enabled systems having some funny suffix after the usual mode bits
>> stuff.
>>
>> However, as far as this test is concerned, I do not think "how
>> portable is the output from ls -ld" is an especially relevant
>> question.  None of the things we expect early in the output (the
>> fields I enumerated in the previous paragraph) would contain " -> ".
>> And we know that we do not use a filename that has " -> " (or "->")
>> as a substring in our tests.
>>
>> We don't have to use readlink, even on platforms where we do have
>> readlink.  Building the conditional to be checked at runtime and
>> providing a shell function read_link that uses "ls -ld | sed" or
>> "readlink" depending on the runtime check is wasteful.
>
> Just a short, curious question: Is this patch to be accepted/included some time?
> I didn't see it in 2.8.4 nor 2.9.0. Maybe it just fell off the table...

Yes, I think this fell off the table as I was waiting for some kind
of agreement or counter-proposal, neither of which came and the
thread was forgotten.

Unless Torsten still has strong objections (or better yet, a better
implementation), I am inclined to queue it as-is.

Thanks for pinging the thread.

Re: [PATCH] t7800 readlink not found

[Torsten Bögershausen]

On 06/21/2016 08:39 PM, Junio C Hamano wrote:
> Armin Kunaschik <megabreit@googlemail.com> writes:
>
>> On Tue, May 31, 2016 at 7:51 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Torsten Bögershausen <tboegi@web.de> writes:
>>>
>>>>> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
>>>>> index 7ce4cd7..905035c 100755
>>>>> --- a/t/t7800-difftool.sh
>>>>> +++ b/t/t7800-difftool.sh
>>>>> @@ -446,7 +446,7 @@ write_script .git/CHECK_SYMLINKS <<\EOF
>>>>>    for f in file file2 sub/sub
>>>>>    do
>>>>>       echo "$f"
>>>>> -    readlink "$2/$f"
>>>>> +    ls -ld "$2/$f" | sed -e 's/.* -> //'
>>>>>    done >actual
>>>>>    EOF
>>>>>
>>>> I don't know how portable #ls -ld" really is.
>>> The parts with mode bits, nlinks, uid, gid, size, and date part do
>>> have some variations.  For example, we have been burned on ACL
>>> enabled systems having some funny suffix after the usual mode bits
>>> stuff.
>>>
>>> However, as far as this test is concerned, I do not think "how
>>> portable is the output from ls -ld" is an especially relevant
>>> question.  None of the things we expect early in the output (the
>>> fields I enumerated in the previous paragraph) would contain " -> ".
>>> And we know that we do not use a filename that has " -> " (or "->")
>>> as a substring in our tests.
>>>
>>> We don't have to use readlink, even on platforms where we do have
>>> readlink.  Building the conditional to be checked at runtime and
>>> providing a shell function read_link that uses "ls -ld | sed" or
>>> "readlink" depending on the runtime check is wasteful.
>> Just a short, curious question: Is this patch to be accepted/included some time?
>> I didn't see it in 2.8.4 nor 2.9.0. Maybe it just fell off the table...
> Yes, I think this fell off the table as I was waiting for some kind
> of agreement or counter-proposal, neither of which came and the
> thread was forgotten.
>
> Unless Torsten still has strong objections (or better yet, a better
> implementation), I am inclined to queue it as-is.
>
I just double-checked the man pages for Mac OS and opengroup:
No better implementation from my side -> No objections