patch, 2 partsMakefile: use the native (Korn) shell on SunOS and IRIX/IRIX64

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(-)

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(-)

Brandon Casey wrote:

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.

[...]

$code can be removed now, right?

Jonathan

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

Jonathan Nieder wrote:

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.

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

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

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?

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(-)

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 <drafnel@gmail.com> writes:

Ah, of course you are right.

Hi,

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

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.

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

Clemens Buchacher <drizzd@aon.at> writes:

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

Andreas.

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

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

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

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