threads / discuss / 39277

t0005-signals.sh fails with ksh

Subject: t0005-signals.sh fails with ksh

## tl;dr

11 messages between May 8, 2015 and May 9, 2015.

replies: 10people: 5as markdown or json

evgeny· May 8, 2015, 20:15 UTC · lore
Hi,
Test t0005-signals.sh fails when I run 'make test' under ksh (on Solaris and on Linux)
$ ksh t0005-signals.sh -v
Initialized empty Git repository in /home/test/git/t/trash directory.t0005-signals/.git/
expecting success: 
        { test-sigchain >actual; ret=$?; } &&
        case "$ret" in
        143) true ;; # POSIX w/ SIGTERM=15
        271) true ;; # ksh w/ SIGTERM=15
          3) true ;; # Windows
          *) false ;;
        esac &&
        test_cmp expect actual

t0005-signals.sh[499]: eval: line 4: 23622: Terminated ok 1 - sigchain works

expecting success: 
        # we use exec here to avoid any sub-shell interpretation
        # of the exit code
        git config alias.sigterm "!exec test-sigchain" &&
        test_expect_code 143 git sigterm

three two one error: exec test-sigchain died of signal 15 ok 2 - signals are propagated using shell convention

expecting success: 
        test-genrandom foo 16384 >file &&
        git add file
ok 3 - create blob
expecting success: 
        OUT=$( ((large_git; echo $? 1>&3) | :) 3>&1 ) &&
        test "$OUT" -eq 141

t0005-signals.sh[499]: eval: syntax error at line 4: `(' unmatched Memory fault

I rather think there are two issues:
1. two adjacent parentheses
2. numeric constant (141) should be 269 for ksh, like there are two different 
constants in the test 'test-sigchain' in this file (in t0005-signals.sh)
'man ksh' reads:
  (list)
    Execute  list  in  a  separate environment.
    Note, that if two adjacent open parentheses are needed for nesting,
    a space must be inserted to avoid evaluation as an arithmetic command as
    described above.
-- 
Best regards,
 evgeny
Junio C Hamano· May 8, 2015, 20:34 UTC · re: evgeny · lore

Re: t0005-signals.sh fails with ksh

evgeny <illumsoft.org@gmail.com> writes:
Show 6 quoted lines
> expecting success: 
>         OUT=$( ((large_git; echo $? 1>&3) | :) 3>&1 ) &&
>         test "$OUT" -eq 141
>
> t0005-signals.sh[499]: eval: syntax error at line 4: `(' unmatched
> Memory fault
Does this work if you did
	OUT=$( ( (large_git ; echo $? 1>&3) | : ) 3>&2 ) &&
instead?
Show 6 quoted lines
> 'man ksh' reads:
>   (list)
>     Execute  list  in  a  separate environment.
>     Note, that if two adjacent open parentheses are needed for nesting,
>     a space must be inserted to avoid evaluation as an arithmetic command as
>     described above.

Hmm, I cannot see "as described above" in your message, but isn't that talking about a common mistake of turning cmd1 in this pipeline

	x=$(cmd1 | cmd2)
into a series of two commands, e.g. (cmd1a && cmd1b) and saying
	x=$((cmd1a && cmd1b) | cmd2)

which does make "$((" interpreted as the beginning of arithmetic expansion?

And the "OUT=$( ((large..." construct seems to be written in order to avoid that exact issue (notice the SP after "$(").

Puzzled....
Jeff King· May 8, 2015, 20:55 UTC · re: Junio C Hamano · lore

Re: t0005-signals.sh fails with ksh

On Fri, May 08, 2015 at 01:34:49PM -0700, Junio C Hamano wrote:
Show 14 quoted lines
> evgeny <illumsoft.org@gmail.com> writes:
> 
> > expecting success: 
> >         OUT=$( ((large_git; echo $? 1>&3) | :) 3>&1 ) &&
> >         test "$OUT" -eq 141
> >
> > t0005-signals.sh[499]: eval: syntax error at line 4: `(' unmatched
> > Memory fault
> 
> Does this work if you did
> 
> 	OUT=$( ( (large_git ; echo $? 1>&3) | : ) 3>&2 ) &&
> 
> instead?

It does for me. I've tested our suite with mksh before, and it passed (that's why the earlier check already covers ksh). But using the ksh I get from "apt-get install ksh" on Debian (ksh93, it looks like?) fails as described. The change above prevents the shell from crashing, and then if we also massage the code, the test passes (i.e., the patch below).

I'm on the fence, though, on declaring ksh93 to be unsupported. I don't know how many other instances of this are in our test suite, and it's one more maintenance headache to deal with. Are there really platforms with no actual POSIX shell (on Solaris, for example, the xpg6 shell is a much better choice)?

diff --git a/t/t0005-signals.sh b/t/t0005-signals.sh
index e7f27eb..cbf778e 100755
--- a/t/t0005-signals.sh
+++ b/t/t0005-signals.sh
@@ -9,14 +9,23 @@ two
 one
 EOF
 
+# $1 - expected signal number
+# $2 - actual exit code
+check_signal_exit () {
+	if test "$2" = "$(($1 + 128))"; then
+		return 0; # POSIX
+	elif test "$2" = "$(($1 + 256))"; then
+		return 0; # ksh
+	elif test "$2" = 3; then
+		return 0; # Windows
+	fi
+	echo >&2 "expected death by signal $1, got exit code $2"
+	return 1
+}
+
 test_expect_success 'sigchain works' '
 	{ test-sigchain >actual; ret=$?; } &&
-	case "$ret" in
-	143) true ;; # POSIX w/ SIGTERM=15
-	271) true ;; # ksh w/ SIGTERM=15
-	  3) true ;; # Windows
-	  *) false ;;
-	esac &&
+	check_signal_exit 15 $ret &&
 	test_cmp expect actual
 '
 
@@ -40,13 +49,13 @@ test_expect_success 'create blob' '
 '
 
 test_expect_success !MINGW 'a constipated git dies with SIGPIPE' '
-	OUT=$( ((large_git; echo $? 1>&3) | :) 3>&1 ) &&
-	test "$OUT" -eq 141
+	OUT=$( ( (large_git; echo $? 1>&3) | :) 3>&1 ) &&
+	check_signal_exit 13 $OUT
 '
 
 test_expect_success !MINGW 'a constipated git dies with SIGPIPE even if parent ignores it' '
-	OUT=$( ((trap "" PIPE; large_git; echo $? 1>&3) | :) 3>&1 ) &&
-	test "$OUT" -eq 141
+	OUT=$( ( (trap "" PIPE; large_git; echo $? 1>&3) | :) 3>&1 ) &&
+	check_signal_exit 13 $OUT
 '
 
 test_done
Jeff King· May 8, 2015, 21:14 UTC · re: Jeff King · lore

Re: t0005-signals.sh fails with ksh

On Fri, May 08, 2015 at 04:55:48PM -0400, Jeff King wrote:
Show 18 quoted lines
> > Does this work if you did
> > 
> > 	OUT=$( ( (large_git ; echo $? 1>&3) | : ) 3>&2 ) &&
> > 
> > instead?
> 
> It does for me. I've tested our suite with mksh before, and it passed
> (that's why the earlier check already covers ksh). But using the ksh I
> get from "apt-get install ksh" on Debian (ksh93, it looks like?) fails
> as described. The change above prevents the shell from crashing, and
> then if we also massage the code, the test passes (i.e., the patch
> below).
> 
> I'm on the fence, though, on declaring ksh93 to be unsupported. I don't
> know how many other instances of this are in our test suite, and it's
> one more maintenance headache to deal with. Are there really platforms
> with no actual POSIX shell (on Solaris, for example, the xpg6 shell is a
> much better choice)?

Hmph. I just ran the whole suite with SHELL_PATH=ksh. There are about 35 failures overall, and they are not all related. For instance, in t4014, it looks like a variable assignment just goes missing (whether it is because it is a one-shot against a function, or because it contains a bunch of utf8 characters, I'm not sure). We might be able to work around that. But some of the other failures leave me quite confused.

Here's an oddity I isolated based on a failure in t5502:
  $ git init foo && cd foo
  Initialized empty Git repository in /home/peff/foo/.git/
  $ (cd .git/objects && ls)
  info pack
OK, makes sense. Now...
  $ mkdir subdir && cd subdir
  $ ls ../.git/objects
  info  pack
  $ cd ../.git/objects
  ksh: cd: /home/peff/foo/git/objects: [No such file or directory]
Um, what? Apparently using "../.foo" will eat the "." off of "foo"?
-Peff
Junio C Hamano· May 8, 2015, 21:39 UTC · re: Jeff King · lore

Re: t0005-signals.sh fails with ksh

Jeff King <peff@peff.net> writes:
Show 17 quoted lines
> Here's an oddity I isolated based on a failure in t5502:
>
>   $ git init foo && cd foo
>   Initialized empty Git repository in /home/peff/foo/.git/
>
>   $ (cd .git/objects && ls)
>   info pack
>
> OK, makes sense. Now...
>
>   $ mkdir subdir && cd subdir
>   $ ls ../.git/objects
>   info  pack
>   $ cd ../.git/objects
>   ksh: cd: /home/peff/foo/git/objects: [No such file or directory]
>
> Um, what? Apparently using "../.foo" will eat the "." off of "foo"?
That is fun.  And doing
    $ mkdir -p git/objects

just before you create and chdir into subdir does tell me that the last "cd ../.git/objects" is turned into "cd ../git/objects".

Actually, with that extra thing, changing the last one to
    $ cd ../..git/objects
still takes me to ../git/objects, it seems.  But lookie here:
    $ cd ../.....git/objects
    ksh: cd: /var/tmp/x/ksh/...git/objects: [No such file or directory]
WAT.
I am tempted to say that we should write it off as utterly broken.
evgeny· May 8, 2015, 23:43 UTC · re: Junio C Hamano · lore

Re: t0005-signals.sh fails with ksh

Hi, just for information about ksh on Linux and OpenIndiana

           ksh93 on Debian GNU/Linux 7 (wheezy)
$ ksh --version
  version         sh (AT&T Research) 93u+ 2012-02-29

$ git init foo && cd foo Initialized empty Git repository in /var/tmp/foo/.git/ $ (cd .git/objects && ls) info pack $ mkdir subdir && cd subdir $ ls ../.git/objects info pack $ cd ../.git/objects /bin/ksh93: cd: /var/tmp/foo/git/objects: [No such file or directory]

           ksh93 on OpenIndiana oi_151.1.9 X86
$ ksh --version
  version         sh (AT&T Research) 93t+ 2010-03-05

$ pwd /var/tmp $ git init foo && cd foo Initialized empty Git repository in /var/tmp/foo/.git/ $ (cd .git/objects && ls) info pack $ mkdir subdir && cd subdir $ ls ../.git/objects info pack $ cd ../.git/objects $ pwd /var/tmp/foo/.git/objects

Looks like older version (or Solaris version?) of ksh did not have this bug.
Saturday, May 9, 2015, 12:39:32 AM, you wrote:
> Jeff King <peff@peff.net> writes:
Show 17 quoted lines
>> Here's an oddity I isolated based on a failure in t5502:
>>
>>   $ git init foo && cd foo
>>   Initialized empty Git repository in /home/peff/foo/.git/
>>
>>   $ (cd .git/objects && ls)
>>   info pack
>>
>> OK, makes sense. Now...
>>
>>   $ mkdir subdir && cd subdir
>>   $ ls ../.git/objects
>>   info  pack
>>   $ cd ../.git/objects
>>   ksh: cd: /home/peff/foo/git/objects: [No such file or directory]
>>
>> Um, what? Apparently using "../.foo" will eat the "." off of "foo"?
> That is fun.  And doing
>     $ mkdir -p git/objects
> just before you create and chdir into subdir does tell me that the
> last "cd ../.git/objects" is turned into "cd ../git/objects".
> Actually, with that extra thing, changing the last one to
>     $ cd ../..git/objects
> still takes me to ../git/objects, it seems.  But lookie here:
>     $ cd ../.....git/objects
>     ksh: cd: /var/tmp/x/ksh/...git/objects: [No such file or directory]
> WAT.
> I am tempted to say that we should write it off as utterly broken.
-- 
Best regards,
 evgeny                            mailto:illumsoft.org@gmail.com
Andreas Schwab· May 9, 2015, 08:20 UTC · re: Junio C Hamano · lore

Re: t0005-signals.sh fails with ksh

Junio C Hamano <gitster@pobox.com> writes:
Show 6 quoted lines
> That is fun.  And doing
>
>     $ mkdir -p git/objects
>
> just before you create and chdir into subdir does tell me that the
> last "cd ../.git/objects" is turned into "cd ../git/objects".

This has been fixed in ksh93v. Looking at the diff between openSUSE 13.1 and 13.2 version of ksh, this hunk from src/cmd/ksh93/bltins/cd_pwd.c is responsible:

@@ -109,34 +155,22 @@
 		if(!oldpwd)
 			oldpwd = path_pwd(shp,1);
 	}
-	if(*dir=='.')
+	if(dirfd==shp->pwdfd && *dir!='/')
 	{
-		/* test for pathname . ./ .. or ../ */
-		int	n=0;
-		char	*sp;
-		for(dp=dir; *dp=='.'; dp++)
-		{
-			if(*++dp=='.' && (*++dp=='/' || *dp==0))
-				n++;
-			else if(*dp && *dp!='/')
-				break;
-			if(*dp==0)
-				break;
-		}
-		if(n)	
-		{
-			cdpath = 0;
-			sp = oldpwd + strlen(oldpwd);
-			while(n--)
-			{
-				while(--sp > oldpwd && *sp!='/');
-				if(sp==oldpwd)
-					break;
-				
-			}
-			sfwrite(shp->strbuf,oldpwd,sp+1-oldpwd);
-			sfputr(shp->strbuf,dp,0);
-			dir = sfstruse(shp->strbuf);
+		/* check for leading .. */
+
+		char *cp;
+
+		j = sfprintf(shp->strbuf,"%s",dir);
+		cp = sfstruse(shp->strbuf);
+		pathcanon(cp, j + 1, 0);
+		if(cp[0]=='.' && cp[1]=='.' && (cp[2]=='/' || cp[2]==0))
+		{
+			if(!shp->strbuf2)
+				shp->strbuf2 = sfstropen();
+			j = sfprintf(shp->strbuf2,"%s/%s",oldpwd,cp);
+			dir = sfstruse(shp->strbuf2);
+			pathcanon(dir, j + 1, 0);
 		}
 	}
 	rval = -1;


Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
Junio C Hamano· May 8, 2015, 21:16 UTC · re: Jeff King · lore

Re: t0005-signals.sh fails with ksh

Jeff King <peff@peff.net> writes:
Show 22 quoted lines
> On Fri, May 08, 2015 at 01:34:49PM -0700, Junio C Hamano wrote:
>
>> evgeny <illumsoft.org@gmail.com> writes:
>> 
>> > expecting success: 
>> >         OUT=$( ((large_git; echo $? 1>&3) | :) 3>&1 ) &&
>> >         test "$OUT" -eq 141
>> >
>> > t0005-signals.sh[499]: eval: syntax error at line 4: `(' unmatched
>> > Memory fault
>> 
>> Does this work if you did
>> 
>> 	OUT=$( ( (large_git ; echo $? 1>&3) | : ) 3>&2 ) &&
>> 
>> instead?
>
> It does for me. I've tested our suite with mksh before, and it passed
> (that's why the earlier check already covers ksh). But using the ksh I
> get from "apt-get install ksh" on Debian (ksh93, it looks like?) fails
> as described. 
> ...
Yuck.
Show 5 quoted lines
> I'm on the fence, though, on declaring ksh93 to be unsupported. I don't
> know how many other instances of this are in our test suite, and it's
> one more maintenance headache to deal with. Are there really platforms
> with no actual POSIX shell (on Solaris, for example, the xpg6 shell is a
> much better choice)?

Yeah, ksh has gone too far and now is on the other side, I would have to say. Introducing new keywords and semantics to let its users use new features (e.g. "let") is one thing, but breaking a valid POSIX shell construct and interpreting it in an incompatible way is going just too far for it to be treated as a Bourne variant.

I wonder if zsh is in the same league. Do we support people who do SHELL_PATH=/bin/zsh and bend over backwards when it breaks?

I'm on the fence, too.
Jeff King· May 8, 2015, 21:21 UTC · re: Junio C Hamano · lore

Re: t0005-signals.sh fails with ksh

On Fri, May 08, 2015 at 02:16:19PM -0700, Junio C Hamano wrote:
Show 5 quoted lines
> Yeah, ksh has gone too far and now is on the other side, I would
> have to say.  Introducing new keywords and semantics to let its
> users use new features (e.g. "let") is one thing, but breaking a
> valid POSIX shell construct and interpreting it in an incompatible
> way is going just too far for it to be treated as a Bourne variant.

Yeah, especially after my followup email, I think I'm not on the fence anymore.

> I wonder if zsh is in the same league.  Do we support people who do
> SHELL_PATH=/bin/zsh and bend over backwards when it breaks?

I tried "make SHELL_PATH=zsh test", but had trouble seeing the test output for all of the errors being spewed to stderr. ;)

Certainly this:
  $ zsh ./t0000-basic.sh -v -i
  > [...]
  test_cmp:1: command not found: diff -u
  not ok 4 - pretend we have a fully passing test suite

is not especially encouraging (it looks like running "$FOO bar" does not word-split $FOO). I am not a zsh user, though, so there may be ways to convince it to be more POSIX-y (e.g., just calling it as "sh").

-Peff
brian m. carlson· May 9, 2015, 20:01 UTC · re: Jeff King · lore

Re: t0005-signals.sh fails with ksh

On Fri, May 08, 2015 at 05:21:47PM -0400, Jeff King wrote:
Show 17 quoted lines
> On Fri, May 08, 2015 at 02:16:19PM -0700, Junio C Hamano wrote:
> > I wonder if zsh is in the same league.  Do we support people who do
> > SHELL_PATH=/bin/zsh and bend over backwards when it breaks?
> 
> I tried "make SHELL_PATH=zsh test", but had trouble seeing the test
> output for all of the errors being spewed to stderr. ;)
> 
> Certainly this:
> 
>   $ zsh ./t0000-basic.sh -v -i
>   > [...]
>   test_cmp:1: command not found: diff -u
>   not ok 4 - pretend we have a fully passing test suite
> 
> is not especially encouraging (it looks like running "$FOO bar" does not
> word-split $FOO). I am not a zsh user, though, so there may be ways to
> convince it to be more POSIX-y (e.g., just calling it as "sh").

I use zsh. It's possible to convince it to be more POSIXy by saying "emulate sh" or invoking it via a symlink called sh. However, having said that, I don't recommend it. I set /bin/sh to zsh on my Debian system and a lot of things broke. Early versions of Mac OS X did that, too, and they stopped because it was very broken.

As for ksh, I don't know whether you want to kill support for just ksh93 or mksh as well. I've generally had good experience with mksh as /bin/sh when I've tried it, and it's at least semi-supported in Debian because it meets Debian's POSIX-plus requirements.

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187
evgeny· May 8, 2015, 23:05 UTC · re: Junio C Hamano · lore

Re: t0005-signals.sh fails with ksh

Hello Junio,
Friday, May 8, 2015, 11:34:49 PM, you wrote:
> evgeny <illumsoft.org@gmail.com> writes:
Show 6 quoted lines
>> expecting success: 
>>         OUT=$( ((large_git; echo $? 1>&3) | :) 3>&1 ) &&
>>         test "$OUT" -eq 141
>>
>> t0005-signals.sh[499]: eval: syntax error at line 4: `(' unmatched
>> Memory fault
> Does this work if you did
>         OUT=$( ( (large_git ; echo $? 1>&3) | : ) 3>&2 ) &&
> instead?

Yes. when I add a space between parentheses and change 141 to 269 all tests in t0005-signals.sh are passed.

... stripped first lines ... ok 3 - create blob

expecting success: 
        OUT=$( ( (large_git; echo $? 1>&3) | :) 3>&1 ) &&
        test "$OUT" -eq 269
ok 4 - a constipated git dies with SIGPIPE
expecting success: 
        OUT=$( ( (trap "" PIPE; large_git; echo $? 1>&3) | :) 3>&1 ) &&
        test "$OUT" -eq 269
ok 5 - a constipated git dies with SIGPIPE even if parent ignores it

# passed all 5 test(s) 1..5

Show 6 quoted lines
>> 'man ksh' reads:
>>   (list)
>>     Execute  list  in  a  separate environment.
>>     Note, that if two adjacent open parentheses are needed for nesting,
>>     a space must be inserted to avoid evaluation as an arithmetic command as
>>     described above.
> Hmm, I cannot see "as described above" in your message, but isn't
> that talking about a common mistake of turning cmd1 in this pipeline
>         x=$(cmd1 | cmd2)
> into a series of two commands, e.g. (cmd1a && cmd1b) and saying
>         x=$((cmd1a && cmd1b) | cmd2)
> which does make "$((" interpreted as the beginning of arithmetic
> expansion?
> And the "OUT=$( ((large..." construct seems to be written in order
> to avoid that exact issue (notice the SP after "$(").
> Puzzled....

It was an excerpt from ksh's man page, and "as described above" refers to previous paragraph where "((expression))" expained.

I think the space after "$(" refers to "Arithmetic Substitution" paragraph and the following two parentheses are about "Note, that if two adjacent open parentheses are needed for nesting, a space must be inserted"

              
$ man ksh
... skipped ...
      Arithmetic Substitution.
       An  arithmetic  expression  enclosed  in  double parentheses preceded by a dollar sign ( $(()) ) is replaced by the
       value of the arithmetic expression within the double parentheses.
... skipped ...
       ((expression))
              The expression is evaluated using the rules for arithmetic evaluation described below.  If the value of  the
              arithmetic expression is non-zero, the exit status is 0, otherwise the exit status is 1.
       (list)
              Execute list in a separate environment.  Note, that if two adjacent open parentheses are needed for nesting,
              a space must be inserted to avoid evaluation as an arithmetic command as described above.
... skipped ...
Some info about my system:

$ head -1 /etc/os-release PRETTY_NAME="Debian GNU/Linux 7 (wheezy)"

$ ksh --version
  version         sh (AT&T Research) 93u+ 2012-02-29
  It is surprise for me that ksh is so old.
  
$ ls -l $(which ksh)
lrwxrwxrwx 1 root root 29 May  6 22:44 /usr/bin/ksh -> /etc/alternatives/usr.bin.ksh

$ ls -l /etc/alternatives/usr.bin.ksh lrwxrwxrwx 1 root root 10 May 6 22:44 /etc/alternatives/usr.bin.ksh -> /bin/ksh93

$ ls -l /bin/ksh93
-rwxr-xr-x 1 root root 1446836 Jan  3  2013 /bin/ksh93
  
-- 
Best regards,
 evgeny

← back to recent threads