Re: t7800 test failure

[Matthieu Moy]

David Aguilar  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/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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  wrote:
> > Armin Kunaschik  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 
Date: Thu, 26 May 2016 02:42:52 -0700
Subject: [PATCH] tests: add shell portability check for "readlink"

Signed-off-by: David Aguilar 
---
 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: t7800 test failure

[Armin Kunaschik]

On Tue, May 24, 2016 at 7:36 PM, Junio C Hamano  wrote:
> Armin Kunaschik  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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: t7800 test failure

[Junio C Hamano]

Armin Kunaschik  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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: t7800 test failure

[Armin Kunaschik]

On Tue, May 24, 2016 at 6:57 PM, Junio C Hamano  wrote:
> Matthieu Moy  writes:
>
>> Armin Kunaschik  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?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: t7800 test failure

[Junio C Hamano]

Matthieu Moy  writes:

> Armin Kunaschik  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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: t7800 test failure

[Matthieu Moy]

Armin Kunaschik  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/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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"
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html