Re: t5570 trap use in start/stop_git_daemon

From: Jeff King <peff@peff.net>
Date: Feb 13, 2015, 07:44 UTC
Message-ID: <20150213074403.GB26775@peff.net>
In-Reply-To: <013601d04702$d7e721e0$87b565a0$@nexbridge.com>

On Thu, Feb 12, 2015 at 03:31:12PM -0500, Randall S. Becker wrote:

Show 5 quoted lines

> On the NonStop port, we found that trap was causing an issue with test
> success for t5570. When start_git_daemon completes, the shell (ksh,bash) on
> this platform is sending a signal 0 that is being caught and acted on by the
> trap command within the start_git_daemon and stop_git_daemon functions. I am
> taking this up with the operating system group,

Yeah, that seems wrong. If it were a subshell, even, I could see some argument for it, but it seems odd to trap 0 when a function returns (bash does have a RETURN trap, which AFAIK is bash-specific, but it should not trigger a 0-trap).

Show 14 quoted lines

> but in any case, it may be
> appropriate to include a trap reset at the end of both functions, as below.
> I verified this change on SUSE Linux.
> 
> diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
> index bc4b341..543e98a 100644
> --- a/t/lib-git-daemon.sh
> +++ b/t/lib-git-daemon.sh
> @@ -62,6 +62,7 @@ start_git_daemon() {
>                 test_skip_or_die $GIT_TEST_GIT_DAEMON \
>                         "git daemon failed to start"
>        fi
> +       trap '' EXIT
> }

I don't think this is the right thing to do. That trap is meant to live beyond the function's return. Without it, there is nothing to clean up the running git-daemon if we exit the test script prematurely (e.g., by a test failing in immediate-mode). We pollute the environment with a running process which would cause subsequent test runs to fail.

Show 8 quoted lines

> stop_git_daemon() {
> @@ -84,4 +85,6 @@ stop_git_daemon() {
>         fi
>         GIT_DAEMON_PID=
>         rm -f git_daemon_output
> +
> +       trap '' EXIT
> }

This one is slightly less bad, in that we are dropping our daemon-specific cleanup here anyway. But the appropriate trap is still:

  trap 'die' EXIT

which we set earlier in the function. Without it, the test harness's ability to detect a premature failure is lost.

So I do not know quite what is going on with your shell, but turning off the traps in these functions is definitely not an acceptable (general) workaround; it makes things much worse on working platforms.

-Peff