t7800 test failure

PATCH In review Armin Kunaschik 5 participants 14 messages Jun 16, 2016 – Jun 21, 2016

Share & tools

Armin Kunaschik

author

2016-06-16 02:19 UTC #0 lore raw

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:

---

t/t7800-difftool.sh +0 -0

index ff7a9e9..be3d19f 100755
@@ -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"

Matthieu Moy

2016-06-16 02:19 UTC #1 lore raw

Quoted 1 line · Armin Kunaschik

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?

Quoted 1 line

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

Signature · 2 lines

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

Junio C Hamano

2016-06-16 02:19 UTC #2 lore raw

Quoted 11 lines · Matthieu Moy

Quoted 1 line · Armin Kunaschik
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?
Quoted 1 line
+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).

Quoted 2 lines

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

Yup.

Armin Kunaschik

author

2016-06-16 02:19 UTC #3 lore raw

Quoted 8 lines · Junio C Hamano

Quoted 6 lines · Matthieu Moy
Quoted 1 line · Armin Kunaschik
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.

Quoted 9 lines

Quoted 4 lines
Quoted 1 line
+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.

Quoted 4 lines

Quoted 2 lines
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?

Junio C Hamano

2016-06-16 02:19 UTC #4 lore raw

Quoted 9 lines · Armin Kunaschik

Quoted 4 lines
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.

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
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(-)

t/t7800-difftool.sh +9 -8

index 7ce4cd7..f304228 100755
@@ -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

Armin Kunaschik

author

2016-06-16 02:19 UTC #5 lore raw

Quoted 5 lines · Junio C Hamano

Quoted 2 lines · Armin Kunaschik
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!

Quoted 32 lines

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

t/t7800-difftool.sh +9 -8

index 7ce4cd7..f304228 100755
@@ -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

David Aguilar

2016-06-16 02:19 UTC #6 lore raw

Quoted 10 lines · Armin Kunaschik

Quoted 5 lines · Junio C Hamano
Quoted 2 lines · Armin Kunaschik
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?

Quoted 24 lines

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

t/t7800-difftool.sh +4 -8

index 7ce4cd7..f304228 100755
@@ -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

Quoted 8 lines

t/t7800-difftool.sh run_dir_diff_test 'difftool --dir-diff with unmerged files' ' · line 457
Quoted 8 lines
t/t7800-difftool.sh +5 -0
+               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 <[email protected]>
Date: Thu, 26 May 2016 02:42:52 -0700
Subject: [PATCH] tests: add shell portability check for "readlink"

Signed-off-by David Aguilar <[email protected]>

---
 t/check-non-portable-shell.pl | 1 +
 1 file changed, 1 insertion(+)

t/check-non-portable-shell.pl +1 -0

index b170cbc..2707e42 100755
@@ -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 =)';

Signature · 4 lines

2.7.0.rc3

-- 
David

Matthieu Moy

2016-06-16 02:19 UTC #7 lore raw

Quoted 4 lines · David Aguilar

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.

Signature · 2 lines

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

Armin Kunaschik

author

2016-06-16 02:19 UTC #8 lore raw

PATCH 1/1 t7800 readlink not found

In reply to David Aguilar

Quoted 4 lines · David Aguilar

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 <[email protected]>
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 <[email protected]>

---

t/t7800-difftool.sh +1 -1

index 7ce4cd7..905035c 100755
@@ -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

Torsten Bögershausen

2016-06-16 02:19 UTC #9 lore raw

Re: [PATCH] t7800 readlink not found

Quoted 38 lines · Armin Kunaschik

Quoted 4 lines · David Aguilar
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 <[email protected]>
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 <[email protected]>
---
t/t7800-difftool.sh +0 -0
index 7ce4cd7..905035c 100755

t/t7800-difftool.sh write_script .git/CHECK_SYMLINKS <<\EOF · line 452

Quoted 8 lines

t/t7800-difftool.sh +1 -1

@@ -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 ?

Junio C Hamano

2016-06-16 02:19 UTC #10 lore raw

Re: [PATCH] t7800 readlink not found

Quoted 6 lines · Torsten Bögershausen

Quoted 6 lines

t/t7800-difftool.sh +0 -0

index 7ce4cd7..905035c 100755
@@ -446,7 +446,7 @@ write_script .git/CHECK_SYMLINKS <<\EOF
  for f in file file2 sub/sub

Quoted 8 lines

t/t7800-difftool.sh write_script .git/CHECK_SYMLINKS <<\EOF
Quoted 7 lines
t/t7800-difftool.sh +1 -1
  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.

Armin Kunaschik

author

2016-06-21 14:45 UTC #11 lore raw

Re: [PATCH] t7800 readlink not found

Quoted 29 lines · Junio C Hamano

Quoted 6 lines · Torsten Bögershausen
Quoted 6 lines
t/t7800-difftool.sh +0 -0
index 7ce4cd7..905035c 100755
@@ -446,7 +446,7 @@ write_script .git/CHECK_SYMLINKS <<\EOF
  for f in file file2 sub/sub
Quoted 8 lines
t/t7800-difftool.sh write_script .git/CHECK_SYMLINKS <<\EOF
Quoted 7 lines
t/t7800-difftool.sh +1 -1
  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.

Quoted 5 lines

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

Junio C Hamano

2016-06-21 18:39 UTC #12 lore raw

Re: [PATCH] t7800 readlink not found

Quoted 30 lines · Armin Kunaschik

Quoted 29 lines · Junio C Hamano
Quoted 6 lines · Torsten Bögershausen
Quoted 6 lines
t/t7800-difftool.sh +0 -0
index 7ce4cd7..905035c 100755
@@ -446,7 +446,7 @@ write_script .git/CHECK_SYMLINKS <<\EOF
  for f in file file2 sub/sub
Quoted 8 lines
t/t7800-difftool.sh write_script .git/CHECK_SYMLINKS <<\EOF
Quoted 7 lines
t/t7800-difftool.sh +1 -1
  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.

Quoted 7 lines

Quoted 4 lines
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.

Torsten Bögershausen

2016-06-21 20:30 UTC #13 lore raw

Re: [PATCH] t7800 readlink not found

Quoted 35 lines · Junio C Hamano

Quoted 33 lines · Armin Kunaschik
Quoted 32 lines · Junio C Hamano
Quoted 6 lines · Torsten Bögershausen
Quoted 6 lines
t/t7800-difftool.sh +0 -0
index 7ce4cd7..905035c 100755
@@ -446,7 +446,7 @@ write_script .git/CHECK_SYMLINKS <<\EOF
   for f in file file2 sub/sub
Quoted 8 lines
t/t7800-difftool.sh write_script .git/CHECK_SYMLINKS <<\EOF
Quoted 7 lines
t/t7800-difftool.sh +1 -1
   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.

Quoted 9 lines

Quoted 2 lines
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