[PATCH 1/2] t/test-lib.sh: support Korn shell by converting GIT_EXIT_OK to GIT_EXIT_CODE

[PATCH 1/2] t/test-lib.sh: support Korn shell by converting GIT_EXIT_OK to GIT_EXIT_CODE

[Brandon Casey]

From: Brandon Casey <drafnel@gmail.com>

Commit 6e7b5aaf introduced the concept of GIT_EXIT_OK as a way to indicate
to die(), the exit handler, whether the exit was initiated by the test
harness, or whether it was unexpected.  die() expects $? to contain the
value passed to exit(), and when GIT_EXIT_OK is set, die() calls exit with
the value in $?.  This works as expected when using the Bash shell.  For
the Korn shell, $? has the value of the last executed statement _before_
the call to exit.  If that statement completed successfully, then die()
would incorrectly exit with a successful status when GIT_EXIT_OK is set.

So, rather than relying on the behavior of Bash in order to get the exit
code from $? inside die(), change GIT_EXIT_OK into GIT_EXIT_CODE, and set
it to the code that we want to exit with.  This allows the test suite to
be run with the Korn shell.

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---
 t/test-lib.sh |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index f2ca536..64e793a 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -150,7 +150,7 @@ fi
 
 error () {
 	say_color error "error: $*"
-	GIT_EXIT_OK=t
+	GIT_EXIT_CODE=1
 	exit 1
 }
 
@@ -183,16 +183,16 @@ test_success=0
 
 die () {
 	code=$?
-	if test -n "$GIT_EXIT_OK"
+	if test -n "$GIT_EXIT_CODE"
 	then
-		exit $code
+		exit $GIT_EXIT_CODE
 	else
 		echo >&5 "FATAL: Unexpected exit with code $code"
 		exit 1
 	fi
 }
 
-GIT_EXIT_OK=
+GIT_EXIT_CODE=
 trap 'die' EXIT
 
 # The semantics of the editor variables are that of invoking
@@ -295,7 +295,7 @@ test_failure_ () {
 	say_color error "FAIL $test_count: $1"
 	shift
 	echo "$@" | sed -e 's/^/	/'
-	test "$immediate" = "" || { GIT_EXIT_OK=t; exit 1; }
+	test "$immediate" = "" || { GIT_EXIT_CODE=1; exit 1; }
 }
 
 test_known_broken_ok_ () {
@@ -508,7 +508,6 @@ test_create_repo () {
 }
 
 test_done () {
-	GIT_EXIT_OK=t
 	test_results_dir="$TEST_DIRECTORY/test-results"
 	mkdir -p "$test_results_dir"
 	test_results_path="$test_results_dir/${0%.sh}-$$"
@@ -539,9 +538,11 @@ test_done () {
 		cd "$(dirname "$remove_trash")" &&
 		rm -rf "$(basename "$remove_trash")"
 
+		GIT_EXIT_CODE=0
 		exit 0 ;;
 
 	*)
+		GIT_EXIT_CODE=1
 		say_color error "failed $test_failure among $msg"
 		exit 1 ;;
 
@@ -655,7 +656,7 @@ case "$test" in
 esac
 test ! -z "$debug" || remove_trash=$TRASH_DIRECTORY
 rm -fr "$test" || {
-	GIT_EXIT_OK=t
+	GIT_EXIT_CODE=1
 	echo >&5 "FATAL: Cannot prepare test area"
 	exit 1
 }
-- 
1.6.5.rc3

[PATCH 2/2] Makefile: use the native (Korn) shell on SunOS and IRIX/IRIX64

[Brandon Casey]

From: Brandon Casey <drafnel@gmail.com>

Now that the test suite supports the Korn shell, we can use it as the
default on platforms that do not ship with Bash.

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---
 Makefile |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index 629ab35..9ada196 100644
--- a/Makefile
+++ b/Makefile
@@ -726,7 +726,7 @@ endif
 ifeq ($(uname_S),SunOS)
 	NEEDS_SOCKET = YesPlease
 	NEEDS_NSL = YesPlease
-	SHELL_PATH = /bin/bash
+	SHELL_PATH = /usr/xpg4/bin/sh
 	SANE_TOOL_PATH = /usr/xpg6/bin:/usr/xpg4/bin
 	NO_STRCASESTR = YesPlease
 	NO_MEMMEM = YesPlease
@@ -850,7 +850,7 @@ ifeq ($(uname_S),IRIX)
 	NO_MMAP = YesPlease
 	NO_EXTERNAL_GREP = UnfortunatelyYes
 	SNPRINTF_RETURNS_BOGUS = YesPlease
-	SHELL_PATH = /usr/gnu/bin/bash
+	SHELL_PATH = /bin/ksh
 	NEEDS_LIBGEN = YesPlease
 	THREADED_DELTA_SEARCH = YesPlease
 endif
@@ -870,7 +870,7 @@ ifeq ($(uname_S),IRIX64)
 	NO_MMAP = YesPlease
 	NO_EXTERNAL_GREP = UnfortunatelyYes
 	SNPRINTF_RETURNS_BOGUS = YesPlease
-	SHELL_PATH=/usr/gnu/bin/bash
+	SHELL_PATH=/bin/ksh
 	NEEDS_LIBGEN = YesPlease
 	THREADED_DELTA_SEARCH = YesPlease
 endif
-- 
1.6.5.rc3

Re: [PATCH 1/2] t/test-lib.sh: support Korn shell by converting GIT_EXIT_OK to GIT_EXIT_CODE

[Jonathan Nieder]

Brandon Casey wrote:

> So, rather than relying on the behavior of Bash in order to get the exit
> code from $? inside die(), change GIT_EXIT_OK into GIT_EXIT_CODE, and set
> it to the code that we want to exit with.  This allows the test suite to
> be run with the Korn shell.
> 
> Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>

Sounds like a good idea.  A few thoughts:

start_httpd() from lib-httpd.sh uses a similar pattern:

lib-httpd.sh:96:        trap 'code=$?; stop_httpd; (exit $code); die' EXIT

It is probably worth changing that, too, unless GIT_TEST_HTTPD would not
work on these platforms for some other reason.

"GIT_EXIT_CODE=1; exit 1" sounds repetitive to my ear.  It’s probably just
me, but if not, it might be worth adding a function like

	expected_exit() {
		GIT_EXIT_CODE=$1
		exit "$1"
	}

I’m not sure.

> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index f2ca536..64e793a 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
[...]
> @@ -183,16 +183,16 @@ test_success=0
>  
>  die () {
>  	code=$?
> -	if test -n "$GIT_EXIT_OK"
> +	if test -n "$GIT_EXIT_CODE"
>  	then
> -		exit $code
> +		exit $GIT_EXIT_CODE
>  	else
>  		echo >&5 "FATAL: Unexpected exit with code $code"
>  		exit 1
>  	fi
>  }

$code can be removed now, right?

Jonathan

Re: [PATCH 1/2] t/test-lib.sh: support Korn shell by converting GIT_EXIT_OK to GIT_EXIT_CODE

[Jonathan Nieder]

Sorry to reply to self, just want to correct something I missed.

Jonathan Nieder wrote:
> Brandon Casey wrote:

>> diff --git a/t/test-lib.sh b/t/test-lib.sh
>> index f2ca536..64e793a 100644
>> --- a/t/test-lib.sh
>> +++ b/t/test-lib.sh
> [...]
>> @@ -183,16 +183,16 @@ test_success=0
>>  
>>  die () {
>>  	code=$?
>> -	if test -n "$GIT_EXIT_OK"
>> +	if test -n "$GIT_EXIT_CODE"
>>  	then
>> -		exit $code
>> +		exit $GIT_EXIT_CODE
>>  	else
>>  		echo >&5 "FATAL: Unexpected exit with code $code"
>>  		exit 1
>>  	fi
>>  }
>
> $code can be removed now, right?

Sloppy reading on my part here: $code is still used in the error
message on unexpected exits.  The $code will be inaccurate on Suns
in some cases, but this is only a cosmetic problem and the wrong
value should be better than nothing for debugging.

> lib-httpd.sh:96:        trap 'code=$?; stop_httpd; (exit $code); die' EXIT
>
> It is probably worth changing that, too, unless GIT_TEST_HTTPD would not
> work on these platforms for some other reason.

This is used to support that error message, so it should not be
changed, either.

Your patch looks good.  Maybe the commit message could explain this,
though?

Apologies for the noise,
Jonathan

Re: [PATCH 1/2] t/test-lib.sh: support Korn shell by converting GIT_EXIT_OK to GIT_EXIT_CODE

[Junio C Hamano]

Brandon Casey <casey@nrlssc.navy.mil> writes:

> From: Brandon Casey <drafnel@gmail.com>
>
> Commit 6e7b5aaf introduced the concept of GIT_EXIT_OK as a way to indicate
> to die(), the exit handler, whether the exit was initiated by the test
> harness, or whether it was unexpected.  die() expects $? to contain the
> value passed to exit(), and when GIT_EXIT_OK is set, die() calls exit with
> the value in $?.  This works as expected when using the Bash shell.  For
> the Korn shell, $? has the value of the last executed statement _before_
> the call to exit.  If that statement completed successfully, then die()
> would incorrectly exit with a successful status when GIT_EXIT_OK is set.

That's somewhat unexpected, as I did not think this was an anomaly in
bash.  FWIW, dash seems to behave the same way.

    The environment in which the shell executes a trap on EXIT shall be
    identical to the environment immediately after the last command executed
    before the trap on EXIT was taken.

If the trap action was triggered because somebody called "exit 13", $? is
expected to contain that value, because that is the last command executed
before the trap on EXIT was taken, no?

> So, rather than relying on the behavior of Bash in order to get the exit
> code from $? inside die(), change GIT_EXIT_OK into GIT_EXIT_CODE, and set
> it to the code that we want to exit with.  This allows the test suite to
> be run with the Korn shell.

I wonder if the attached is a more clear and contained workaround for this
issue.

---
 t/test-lib.sh |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index f2ca536..c47a295 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -185,7 +185,14 @@ die () {
 	code=$?
 	if test -n "$GIT_EXIT_OK"
 	then
-		exit $code
+		# Korn does not update $? when "exit 47" triggers
+		# the EXIT trap.
+		if test $code = 0
+		then
+			exit 1
+		else
+			exit $code
+		fi
 	else
 		echo >&5 "FATAL: Unexpected exit with code $code"
 		exit 1

Re: [PATCH 1/2] t/test-lib.sh: support Korn shell by converting GIT_EXIT_OK to GIT_EXIT_CODE

[Brandon Casey]

I'm away from a computer right now. Junio, if gmail is showing me the
entirety of your workaround, then no, I don't think that will work.
Your code will always exit non-zero, but there are cases where 'exit
0' is called and a '0' exit status is desired. e.g. when test_done is
called.

I'll look more closely at the suggestions when I get back to a computer.

-brandon

On 10/9/09, Junio C Hamano <gitster@pobox.com> wrote:
> Brandon Casey <casey@nrlssc.navy.mil> writes:
>
>> From: Brandon Casey <drafnel@gmail.com>
>>
>> Commit 6e7b5aaf introduced the concept of GIT_EXIT_OK as a way to indicate
>> to die(), the exit handler, whether the exit was initiated by the test
>> harness, or whether it was unexpected.  die() expects $? to contain the
>> value passed to exit(), and when GIT_EXIT_OK is set, die() calls exit with
>> the value in $?.  This works as expected when using the Bash shell.  For
>> the Korn shell, $? has the value of the last executed statement _before_
>> the call to exit.  If that statement completed successfully, then die()
>> would incorrectly exit with a successful status when GIT_EXIT_OK is set.
>
> That's somewhat unexpected, as I did not think this was an anomaly in
> bash.  FWIW, dash seems to behave the same way.
>
>     The environment in which the shell executes a trap on EXIT shall be
>     identical to the environment immediately after the last command executed
>     before the trap on EXIT was taken.
>
> If the trap action was triggered because somebody called "exit 13", $? is
> expected to contain that value, because that is the last command executed
> before the trap on EXIT was taken, no?
>
>> So, rather than relying on the behavior of Bash in order to get the exit
>> code from $? inside die(), change GIT_EXIT_OK into GIT_EXIT_CODE, and set
>> it to the code that we want to exit with.  This allows the test suite to
>> be run with the Korn shell.
>
> I wonder if the attached is a more clear and contained workaround for this
> issue.
>
> ---
>  t/test-lib.sh |    9 ++++++++-
>  1 files changed, 8 insertions(+), 1 deletions(-)
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index f2ca536..c47a295 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -185,7 +185,14 @@ die () {
>  	code=$?
>  	if test -n "$GIT_EXIT_OK"
>  	then
> -		exit $code
> +		# Korn does not update $? when "exit 47" triggers
> +		# the EXIT trap.
> +		if test $code = 0
> +		then
> +			exit 1
> +		else
> +			exit $code
> +		fi
>  	else
>  		echo >&5 "FATAL: Unexpected exit with code $code"
>  		exit 1
>

-- 
Sent from my mobile device

Re: [PATCH 1/2] t/test-lib.sh: support Korn shell by converting GIT_EXIT_OK to GIT_EXIT_CODE

[Junio C Hamano]

Brandon Casey <drafnel@gmail.com> writes:

> I'm away from a computer right now. Junio, if gmail is showing me the
> entirety of your workaround, then no, I don't think that will work.
> Your code will always exit non-zero, but there are cases where 'exit
> 0' is called and a '0' exit status is desired. e.g. when test_done is
> called.

Ah, of course you are right.

Re: [PATCH 1/2] t/test-lib.sh: support Korn shell by converting GIT_EXIT_OK to GIT_EXIT_CODE

[Clemens Buchacher]

Hi,

On Fri, Oct 09, 2009 at 01:39:56PM -0500, Brandon Casey wrote:

> For the Korn shell, $? has the value of the last executed statement
> _before_ the call to exit.

I just installed ksh/stable (version 93s+ 2008-01-31) on Debian and it
behaves correctly. Maybe you need to upgrade? This really looks like a bug
in your shell to me.

>  die () {
>  	code=$?
> -	if test -n "$GIT_EXIT_OK"
> +	if test -n "$GIT_EXIT_CODE"
>  	then
> -		exit $code
> +		exit $GIT_EXIT_CODE
>  	else
>  		echo >&5 "FATAL: Unexpected exit with code $code"
>  		exit 1
>  	fi
>  }

So in your shell an unexpected exit will always output this?

 FATAL: Unexpected exit with code <some command>

If we can't rely on the value of $?, we should not use it.

Clemens

Re: [PATCH 1/2] t/test-lib.sh: support Korn shell by converting GIT_EXIT_OK to GIT_EXIT_CODE

[Andreas Schwab]

Clemens Buchacher <drizzd@aon.at> writes:

> Hi,
>
> On Fri, Oct 09, 2009 at 01:39:56PM -0500, Brandon Casey wrote:
>
>> For the Korn shell, $? has the value of the last executed statement
>> _before_ the call to exit.
>
> I just installed ksh/stable (version 93s+ 2008-01-31) on Debian and it
> behaves correctly. Maybe you need to upgrade? This really looks like a bug
> in your shell to me.

According to the autoconf docs this bug is also present in the Solaris
/bin/sh (and autoconf generated scripts use some elaborated workaround).

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

Re: [PATCH 1/2] t/test-lib.sh: support Korn shell by converting GIT_EXIT_OK to GIT_EXIT_CODE

[Ben Walton]

[-- Attachment #1: Type: text/plain, Size: 1159 bytes --]

Excerpts from Andreas Schwab's message of Sat Oct 10 08:24:40 -0400 2009:

> According to the autoconf docs this bug is also present in the Solaris
> /bin/sh (and autoconf generated scripts use some elaborated workaround).

Solaris' /bin/sh shouldn't be used for anything but forking a decent
shell, imo.  It was brought to my attention recently that the test
suite for mercurial on solaris was failing the git tests.  The
mercurial folks initially thought this was a git on solaris bug.

After looking into it, I discovered that ^ is still treated[1] as a
pipe symbol[2] in some cases.  So, test suite programs run under
/bin/sh on solaris with commands like `git reset --hard HEAD^` were
misbehaving due to the shell setting up a pipeline under the right
circumstances.

-Ben

[1] http://cvs.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/cmd/sh/cmd.c
    line 184

[2] This dates back to the Thomson shell:
    http://en.wikipedia.org/wiki/Thompson_shell
-- 
Ben Walton
Systems Programmer - CHASS
University of Toronto
C:416.407.5610 | W:416.978.4302

GPG Key Id: 8E89F6D2; Key Server: pgp.mit.edu
Contact me to arrange for a CAcert assurance meeting.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 1/2] t/test-lib.sh: support Korn shell by converting GIT_EXIT_OK to GIT_EXIT_CODE

[Jeff King]

On Sat, Oct 10, 2009 at 08:53:00AM -0400, Ben Walton wrote:

> > According to the autoconf docs this bug is also present in the Solaris
> > /bin/sh (and autoconf generated scripts use some elaborated workaround).
> 
> Solaris' /bin/sh shouldn't be used for anything but forking a decent
> shell, imo.  It was brought to my attention recently that the test
> suite for mercurial on solaris was failing the git tests.  The
> mercurial folks initially thought this was a git on solaris bug.

Yes, that is the attitude we take towards /bin/sh on Solaris. Most
people running git on Solaris (or other broken-shell platforms) point
SHELL_PATH to bash. I think the point of Brandon's patch is to let them
use a native shell.

So while /bin/sh is broken, the question is whether the alternate native
shell is broken enough to use or not.

-Peff