patch, 11 partstests: introduce 'test_atexit'

Introduce the 'test_atexit' helper function to run cleanup commands unconditionally at the end of a test script, mainly to reliably stop daemon processes. 'test_atexit' provides a shorter, simpler, more consistent, and more robust way to accomplish that compared to the current situation where tests and test libraries manually register cleanup commands on trap on EXIT.

Dscho had the first stab at 'test_atexit' as part of an early version of his Azure Pipelines CI patch series:

  https://public-inbox.org/git/12d6137f8d2ccc2041bed8d56d88a09b1db0fd77.1539598316.git.gitgitgadget@gmail.com/

but he dropped those three patches from later versions, and we agreed that I take them over.

01/11 test-lib: fix interrupt handling with 'dash' and '--verbose-log -x'

  This was a fun puzzle to piece together...

02/11 t/lib-git-daemon: make sure to kill the 'git-daemon' process

  I'm not sure whether this is the right thing to do, and have the
  nagging feeling that the issue this patch fixes/works around might
  be a symptom of something more fundamental, but I didn't look into
  how the main git "wrapper" and dashed external commands interact.

03/11 test-lib: introduce 'test_atexit' 04/11 git-daemon: use 'test_atexit` to stop 'git-daemon' 05/11 tests: use 'test_atexit' to stop httpd 06/11 t0301-credential-cache: use 'test_atexit' to stop the credentials helper 07/11 git p4 test: use 'test_atexit' to kill p4d and the watchdog process

  Introduce 'test_atexit' and use it to stop the various daemons
  involved in our tests.

08/11 git p4 test: clean up the p4d cleanup functions 09/11 git p4 test: simplify timeout handling 10/11 git p4 test: disable '-x' tracing in the p4d watchdog loop

  Various cleanups to 't/lib-git-p4.sh'.

11/11 t9811-git-p4-label-import: fix pipeline negation

  A little while-at-it portability fix that I happened to stumble
  upon.

The range-diff below is primarily for Dscho's benefit (assuming he still remembers his old patches... :) It was generated by comparing this patch series to his three 'test_atexit' patches cherry-picked on top of v2.21.0. My modifications to his patches are mainly documentation updates and a few minor fixes.

Johannes Schindelin (3):
  test-lib: introduce 'test_atexit'
  git-daemon: use 'test_atexit` to stop 'git-daemon'
  git p4 test: use 'test_atexit' to kill p4d and the watchdog process

SZEDER Gábor (8):
  test-lib: fix interrupt handling with 'dash' and '--verbose-log -x'
  t/lib-git-daemon: make sure to kill the 'git-daemon' process
  tests: use 'test_atexit' to stop httpd
  t0301-credential-cache: use 'test_atexit' to stop the credentials
    helper
  git p4 test: clean up the p4d cleanup functions
  git p4 test: simplify timeout handling
  git p4 test: disable '-x' tracing in the p4d watchdog loop
  t9811-git-p4-label-import: fix pipeline negation

 t/README                                      | 20 ++++++
 t/interop/i5500-git-daemon.sh                 |  1 -
 t/lib-git-daemon.sh                           | 20 ++++--
 t/lib-git-p4.sh                               | 65 +++++++------------
 t/lib-git-svn.sh                              |  5 --
 t/lib-httpd.sh                                |  6 +-
 t/t0000-basic.sh                              | 18 +++++
 t/t0301-credential-cache.sh                   |  7 +-
 t/t0410-partial-clone.sh                      |  2 -
 t/t5500-fetch-pack.sh                         |  3 -
 t/t5510-fetch.sh                              |  2 -
 t/t5537-fetch-shallow.sh                      |  2 -
 t/t5539-fetch-http-shallow.sh                 |  1 -
 t/t5540-http-push-webdav.sh                   |  2 -
 t/t5541-http-push-smart.sh                    |  1 -
 t/t5542-push-http-shallow.sh                  |  1 -
 t/t5545-push-options.sh                       |  2 -
 t/t5550-http-fetch-dumb.sh                    |  1 -
 t/t5551-http-fetch-smart.sh                   |  1 -
 t/t5561-http-backend.sh                       |  1 -
 t/t5570-git-daemon.sh                         |  1 -
 t/t5581-http-curl-verbose.sh                  |  2 -
 t/t5601-clone.sh                              |  2 -
 t/t5616-partial-clone.sh                      |  2 -
 t/t5700-protocol-v1.sh                        |  2 -
 t/t5702-protocol-v2.sh                        |  2 -
 t/t5703-upload-pack-ref-in-want.sh            |  2 -
 t/t5812-proto-disable-http.sh                 |  1 -
 t/t9115-git-svn-dcommit-funky-renames.sh      |  2 -
 t/t9118-git-svn-funky-branch-names.sh         |  2 -
 t/t9120-git-svn-clone-with-percent-escapes.sh |  2 -
 t/t9142-git-svn-shallow-clone.sh              |  2 -
 t/t9800-git-p4-basic.sh                       |  4 --
 t/t9801-git-p4-branch.sh                      |  8 +--
 t/t9802-git-p4-filetype.sh                    |  4 --
 t/t9803-git-p4-shell-metachars.sh             |  4 --
 t/t9804-git-p4-label.sh                       |  4 --
 t/t9805-git-p4-skip-submit-edit.sh            |  4 --
 t/t9806-git-p4-options.sh                     |  5 --
 t/t9807-git-p4-submit.sh                      |  4 --
 t/t9808-git-p4-chdir.sh                       |  4 --
 t/t9809-git-p4-client-view.sh                 |  4 --
 t/t9810-git-p4-rcs.sh                         |  4 --
 t/t9811-git-p4-label-import.sh                |  7 +-
 t/t9812-git-p4-wildcards.sh                   |  4 --
 t/t9813-git-p4-preserve-users.sh              |  4 --
 t/t9814-git-p4-rename.sh                      |  4 --
 t/t9815-git-p4-submit-fail.sh                 |  4 --
 t/t9816-git-p4-locked.sh                      |  4 --
 t/t9817-git-p4-exclude.sh                     |  4 --
 t/t9818-git-p4-block.sh                       |  4 --
 t/t9819-git-p4-case-folding.sh                |  4 --
 t/t9820-git-p4-editor-handling.sh             |  4 --
 t/t9821-git-p4-path-variations.sh             |  4 --
 t/t9822-git-p4-path-encoding.sh               |  4 --
 t/t9823-git-p4-mock-lfs.sh                    |  4 --
 t/t9824-git-p4-git-lfs.sh                     |  4 --
 t/t9825-git-p4-handle-utf16-without-bom.sh    |  4 --
 t/t9826-git-p4-keep-empty-commits.sh          |  4 --
 t/t9827-git-p4-change-filetype.sh             |  4 --
 t/t9828-git-p4-map-user.sh                    |  4 --
 t/t9829-git-p4-jobs.sh                        |  4 --
 t/t9830-git-p4-symlink-dir.sh                 |  4 --
 t/t9831-git-p4-triggers.sh                    |  4 --
 t/t9832-unshelve.sh                           |  3 -
 t/t9833-errors.sh                             |  5 --
 t/test-lib-functions.sh                       | 28 ++++++++
 t/test-lib.sh                                 | 28 +++++++-
 68 files changed, 136 insertions(+), 247 deletions(-)

Range-diff:
 -:  ---------- >  1:  2a0aeb32f6 test-lib: fix interrupt handling with 'dash' and '--verbose-log -x'
 -:  ---------- >  2:  1dfab6cf94 t/lib-git-daemon: make sure to kill the 'git-daemon' process
 1:  b4769ffa31 !  3:  c9c3efcc7a tests: introduce `test_atexit`
    @@ -1,22 +1,83 @@
     Author: Johannes Schindelin <johannes.schindelin@gmx.de>
     
    -    tests: introduce `test_atexit`
    +    test-lib: introduce 'test_atexit'
     
    -    When running the p4 daemon or `git daemon`, we want to kill it at the
    -    end of the test script.
    +    When running Apache, 'git daemon', or p4d, we want to kill them at the
    +    end of the test script, otherwise a leftover daemon process will keep
    +    its port open indefinitely, and thus will interfere with subsequent
    +    executions of the same test script.
     
    -    So far, we do this "manually".
    +    So far, we stop these daemon processes "manually", i.e.:
     
    -    However, in the next few commits we want to teach the test suite to
    -    optionally re-run scripts with different options, therefore we will have
    -    to have a consistent way to stop daemons.
    +      - by registering functions or commands in the trap on EXIT to stop
    +        the daemon while preserving the last seen exit code before the
    +        trap (to deal with a failure when run with '--immediate' or with
    +        interrupts by ctrl-C),
     
    -    Let's introduce `test_atexit`, which is loosely modeled after
    -    `test_when_finished` (but has a broader scope: rather than running the
    +      - and by invoking these functions/commands last thing before
    +        'test_done' (and sometimes restoring the test framework's default
    +        trap on EXIT, to prevent the daemons from being killed twice).
    +
    +    On one hand, we do this inconsistently, e.g. 'git p4' tests invoke
    +    different functions in the trap on EXIT and in the last test before
    +    'test_done', and they neither restore the test framework's default trap
    +    on EXIT nor preserve the last seen exit code.  On the other hand, this
    +    is error prone, because, as shown in a previous patch in this series,
    +    any output from the cleanup commands in the trap on EXIT can prevent a
    +    proper cleanup when a test script run with '--verbose-log' and certain
    +    shells, notably 'dash', is interrupted.
    +
    +    Let's introduce 'test_atexit', which is loosely modeled after
    +    'test_when_finished', but has a broader scope: rather than running the
         commands after the current test case, run them when the test script
    -    finishes, and also run them when the `--immediate` option is in effect).
    +    finishes, and also run them when the test is interrupted, or exits
    +    early in case of a failure while the '--immediate' option is in
    +    effect.
    +
    +    When running the cleanup commands at the end of a successful test,
    +    then they will be run in 'test_done' before it removes the trash
    +    directory, i.e. the cleanup commands will still be able to access any
    +    pidfiles or socket files in there.  When running the cleanup commands
    +    after an interrupt or failure with '--immediate', then they will be
    +    run in the trap on EXIT.  In both cases they will be run in
    +    'test_eval_', i.e. both standard error and output of all cleanup
    +    commands will go where they should according to the '-v' or
    +    '--verbose-log' options, and thus won't cause any troubles when
    +    interrupting a test script run with '--verbose-log'.
     
         Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
    +    Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
    +
    + diff --git a/t/README b/t/README
    + --- a/t/README
    + +++ b/t/README
    +@@
    + 		...
    + 	'
    + 
    ++ - test_atexit <script>
    ++
    ++   Prepend <script> to a list of commands to run unconditionally to
    ++   clean up before the test script exits, e.g. to stop a daemon:
    ++
    ++	test_expect_success 'test git daemon' '
    ++		git daemon &
    ++		daemon_pid=$! &&
    ++		test_atexit 'kill $daemon_pid' &&
    ++		hello world
    ++	'
    ++
    ++   The commands will be executed before the trash directory is removed,
    ++   i.e. the atexit commands will still be able to access any pidfiles or
    ++   socket files.
    ++
    ++   Note that these commands will be run even when a test script run
    ++   with '--immediate' fails.  Be careful with your atexit commands to
    ++   minimize any changes to the failed state.
    ++
    +  - test_write_lines <lines>
    + 
    +    Write <lines> on standard output, one line per argument.
     
      diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
      --- a/t/t0000-basic.sh
    @@ -38,7 +99,7 @@
     +	'
     +	test_done
     +	EOF
    -+	test_path_exists dont-clean-atexit &&
    ++	test_path_is_file dont-clean-atexit &&
     +	test_path_is_missing clean-atexit &&
     +	test_path_is_missing also-clean-atexit
     +"
    @@ -60,9 +121,17 @@
     +#	test_expect_success 'test git daemon' '
     +#		git daemon &
     +#		daemon_pid=$! &&
    -+#		test_atexit "kill $daemon_pid" &&
    ++#		test_atexit 'kill $daemon_pid' &&
     +#		hello world
     +#	'
    ++#
    ++# The commands will be executed before the trash directory is removed,
    ++# i.e. the atexit commands will still be able to access any pidfiles or
    ++# socket files.
    ++#
    ++# Note that these commands will be run even when a test script run
    ++# with '--immediate' fails.  Be careful with your atexit commands to
    ++# minimize any changes to the failed state.
     +
     +test_atexit () {
     +	# We cannot detect when we are in a subshell in general, but by
    @@ -73,15 +142,6 @@
     +	test_atexit_cleanup="{ $*
     +		} && (exit \"\$eval_ret\"); eval_ret=\$?; $test_atexit_cleanup"
     +}
    -+
    -+test_atexit_handler () {
    -+	test : != "$test_atexit_cleanup" || return 0
    -+
    -+	setup_malloc_check
    -+	test_eval_ "$test_atexit_cleanup"
    -+	test_atexit_cleanup=:
    -+	teardown_malloc_check
    -+}
     +
      # Most tests can use the created repository, but some may need to create more.
      # Usage: test_create_repo <directory>
    @@ -94,6 +154,9 @@
      
      die () {
      	code=$?
    ++	# This is responsible for running the atexit commands even when a
    ++	# test script run with '--immediate' fails, or when the user hits
    ++	# ctrl-C, i.e. when 'test_done' is not invoked at all.
     +	test_atexit_handler || code=$?
      	if test -n "$GIT_EXIT_OK"
      	then
    @@ -103,10 +166,26 @@
      }
      
     +test_atexit_cleanup=:
    ++test_atexit_handler () {
    ++	# In a succeeding test script 'test_atexit_handler' is invoked
    ++	# twice: first from 'test_done', then from 'die' in the trap on
    ++	# EXIT.
    ++	# This condition and resetting 'test_atexit_cleanup' below makes
    ++	# sure that the registered cleanup commands are run only once.
    ++	test : != "$test_atexit_cleanup" || return 0
    ++
    ++	setup_malloc_check
    ++	test_eval_ "$test_atexit_cleanup"
    ++	test_atexit_cleanup=:
    ++	teardown_malloc_check
    ++}
    ++
      test_done () {
      	GIT_EXIT_OK=t
      
    -+	test -n "$immediate" || test_atexit_handler
    ++	# Run the atexit commands _before_ the trash directory is
    ++	# removed, so the commands can access pidfiles and socket files.
    ++	test_atexit_handler
     +
      	if test -n "$write_junit_xml" && test -n "$junit_xml_path"
      	then
 2:  39ad1480c5 !  4:  52dfb87ae3 git-daemon: use `test_atexit` in the tests
    @@ -1,12 +1,18 @@
     Author: Johannes Schindelin <johannes.schindelin@gmx.de>
     
    -    git-daemon: use `test_atexit` in the tests
    +    git-daemon: use 'test_atexit` to stop 'git-daemon'
     
    -    This makes use of the just-introduced consistent way to specify that a
    -    long-running process needs to be terminated at the end of a test script
    -    run.
    +    Use 'test_atexit' to run cleanup commands to stop 'git-daemon' at the
    +    end of the test script or upon interrupt or failure, as it is shorter,
    +    simpler, and more robust than registering such cleanup commands in the
    +    trap on EXIT in the test scripts.
    +
    +    Note that in 't5570-git-daemon.sh' the daemon is stopped and then
    +    re-started in the middle of the test script; take care that the
    +    cleanup functions to stop the daemon are only registered once.
     
         Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
    +    Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
     
      diff --git a/t/interop/i5500-git-daemon.sh b/t/interop/i5500-git-daemon.sh
      --- a/t/interop/i5500-git-daemon.sh
    @@ -30,14 +36,46 @@
      
      test_tristate GIT_TEST_GIT_DAEMON
     @@
    + GIT_DAEMON_HOST_PORT=127.0.0.1:$LIB_GIT_DAEMON_PORT
    + GIT_DAEMON_URL=git://$GIT_DAEMON_HOST_PORT
    + 
    ++registered_stop_git_daemon_atexit_handler=
    + start_git_daemon() {
    + 	if test -n "$GIT_DAEMON_PID"
    + 	then
    +@@
      
      	mkdir -p "$GIT_DAEMON_DOCUMENT_ROOT_PATH"
      
     -	trap 'code=$?; stop_git_daemon; (exit $code); die' EXIT
    -+	test_atexit 'stop_git_daemon'
    ++	# One of the test scripts stops and then re-starts 'git daemon'.
    ++	# Don't register and then run the same atexit handlers several times.
    ++	if test -z "$registered_stop_git_daemon_atexit_handler"
    ++	then
    ++		test_atexit 'stop_git_daemon'
    ++		registered_stop_git_daemon_atexit_handler=AlreadyDone
    ++	fi
      
      	say >&3 "Starting git daemon ..."
      	mkfifo git_daemon_output
    +@@
    + 	then
    + 		kill "$GIT_DAEMON_PID"
    + 		wait "$GIT_DAEMON_PID"
    +-		trap 'die' EXIT
    ++		unset GIT_DAEMON_PID
    + 		test_skip_or_die $GIT_TEST_GIT_DAEMON \
    + 			"git daemon failed to start"
    + 	fi
    +@@
    + 		return
    + 	fi
    + 
    +-	trap 'die' EXIT
    +-
    + 	# kill git-daemon child of git
    + 	say >&3 "Stopping git daemon ..."
    + 	kill "$GIT_DAEMON_PID"
     
      diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
      --- a/t/t5570-git-daemon.sh
 -:  ---------- >  5:  32cde9101e tests: use 'test_atexit' to stop httpd
 -:  ---------- >  6:  fafad7f83d t0301-credential-cache: use 'test_atexit' to stop the credentials helper
 3:  d938232a92 !  7:  5a3cdbae15 git-p4: use `test_atexit` to kill the daemon
    @@ -1,60 +1,53 @@
     Author: Johannes Schindelin <johannes.schindelin@gmx.de>
     
    -    git-p4: use `test_atexit` to kill the daemon
    +    git p4 test: use 'test_atexit' to kill p4d and the watchdog process
     
    -    This should be more reliable than the current method, and prepares the
    -    test suite for a consistent way to clean up before re-running the tests
    -    with different options.
    +    Use 'test_atexit' to run cleanup commands to stop 'p4d' at the end of
    +    the test script or upon interrupt or failure, as it is shorter,
    +    simpler, and more robust than registering such cleanup commands in the
    +    trap on EXIT in the test scripts.
    +
    +    Note that one of the test scripts, 't9801-git-p4-branch.sh', stops and
    +    then re-starts 'p4d' twice in the middle of the script; take care that
    +    the cleanup functions to stop 'p4d' are only registered once.
    +
    +    Note also that 'git p4' tests invoke different functions in the trap
    +    on EXIT ('cleanup') and in the last test before 'test_done'
    +    ('kill_p4d').  Register both of these functions with 'test_atexit' for
    +    now, and a a later patch in this series will then clean up the
    +    redundancy.
     
         Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
    +    Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
     
      diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
      --- a/t/lib-git-p4.sh
      +++ b/t/lib-git-p4.sh
     @@
    - git="$TRASH_DIRECTORY/git"
    - pidfile="$TRASH_DIRECTORY/p4d.pid"
    - 
    --# Sometimes "prove" seems to hang on exit because p4d is still running
    --cleanup () {
    --	if test -f "$pidfile"
    --	then
    --		kill -9 $(cat "$pidfile") 2>/dev/null && exit 255
    --	fi
    --}
    --trap cleanup EXIT
    --
    - # git p4 submit generates a temp file, which will
    - # not get cleaned up if the submission fails.  Don't
    - # clutter up /tmp on the test machine.
    -@@
    - 		# p4d failed to start
    - 		return 1
    + 		kill -9 $(cat "$pidfile") 2>/dev/null && exit 255
      	fi
    -+	test_atexit kill_p4d
    - 
    - 	# build a p4 user so author@example.com has an entry
    - 	p4_add_user author
    -
    - diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
    - --- a/t/t0000-basic.sh
    - +++ b/t/t0000-basic.sh
    -@@
    - 	)
      }
    +-trap cleanup EXIT
      
    -+cat >/dev/null <<\DDD
    - test_expect_success 'pretend we have a fully passing test suite' "
    - 	run_sub_test_lib_test full-pass '3 passing tests' <<-\\EOF &&
    - 	for i in 1 2 3
    + # git p4 submit generates a temp file, which will
    + # not get cleaned up if the submission fails.  Don't
     @@
    - 	> 1..2
    - 	EOF
    - "
    -+DDD
    - 
    - test_expect_success 'test_atexit is run' "
    - 	test_must_fail run_sub_test_lib_test \
    + TMPDIR="$TRASH_DIRECTORY"
    + export TMPDIR
    + 
    ++registered_stop_p4d_atexit_handler=
    + start_p4d () {
    ++	# One of the test scripts stops and then re-starts p4d.
    ++	# Don't register and then run the same atexit handlers several times.
    ++	if test -z "$registered_stop_p4d_atexit_handler"
    ++	then
    ++		test_atexit 'kill_p4d; cleanup'
    ++		registered_stop_p4d_atexit_handler=AlreadyDone
    ++	fi
    ++
    + 	mkdir -p "$db" "$cli" "$git" &&
    + 	rm -f "$pidfile" &&
    + 	(
     
      diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh
      --- a/t/t9800-git-p4-basic.sh
 -:  ---------- >  8:  ad083c6a74 git p4 test: clean up the p4d cleanup functions
 -:  ---------- >  9:  05ecee5c13 git p4 test: simplify timeout handling
 -:  ---------- > 10:  7626c0df88 git p4 test: disable '-x' tracing in the p4d watchdog loop
 -:  ---------- > 11:  53082f8ce1 t9811-git-p4-label-import: fix pipeline negation

When a test script run with 'dash' and '--verbose-log -x' is interrupted by ctrl-C, SIGTERM, or closing the terminal window, then most of the time the registered EXIT trap actions are not executed. This is an annoying issue with tests involving daemons, because they should run cleanup commands to kill those daemon processes in the trap on EXIT, but since these cleanup commands are not executed, the daemons are left alive and keep their port open, thus interfering with subsequent execution of the same test script.

The cause of this issue is the subtle combination of several factors (bear with me, or skip over the indented part):

  - Even when the test script is interrupted, the cleanup commands are
    not run in the trap on INT, TERM, or HUP, but in the trap on EXIT
    after the trap on the signals invokes 'exit' [1].

  - According to POSIX [2]:

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

    Pertinent to the issue at hand is that all open file descriptors
    and the state of '-x' tracing should be preserved.  All shells
    I've tried [3] preserve '-x'.  Unfortunately, however:

      - 'dash' doesn't conform to this when it comes to open file
        descriptors: even when standard output and/or error are
        redirected somewhere when 'exit' is invoked, anything written
        to them in the trap on EXIT goes to the script's original
        stdout and stderr [4].

        We can't dismiss this with a simple "it doesn't conform to
        POSIX, so we don't care", because 'dash' is the default
        /bin/sh in some of the more popular Linux distros.

      - As far as I can tell, POSIX doesn't explicitly say anything
        about the environment of trap actions for various signals.

        In practice it seems that most shells behave sensibly and
        preserve both open file descriptors and the state of '-x'
        tracing for the traps on INT, TERM, and HUP, including even
        'dash'.  The exceptions are 'mksh' and 'lksh': they do
        preserve '-x', but not the open file descriptors.

  - When a test script run with '-x' tracing enabled is interrupted,
    then it's very likely that the signal arrives mid-test, i.e.:

      - while '-x' tracing is enabled, and, consequently, our trap
        actions on INT, TERM, HUP, and EXIT will produce trace output
        as well.

      - while standard output and error are redirected to a log file,
        to the test script's original standard output and error, or to
        /dev/null, depending on whether the test script was run with
        '--verbose-log', '-v', or neither.  According to the above, we
        can't rely on these redirections still be in effect when
        running the traps on INT, TERM, HUP, and/or EXIT.

  - When a test script is run with '--verbose-log', then the test
    script is re-executed with its standard output and error piped
    into 'tee', in order to send the "regular" non-verbose test's
    output both to the terminal and to the log file.  When the test is
    interrupted, then the signal interrupts the downstream 'tee' as
    well.

Putting these together, when a test script run with 'dash' and '--verbose-log -x' is interrupted, then 'dash' tries to write the trace output from the EXIT trap to the script's original standard error, but it very likely can't, because the 'tee' downstream of the pipe is interrupted as well. This causes the shell running the test script to die because of SIGPIPE, without running any of the commands in the EXIT trap.

Disable '-x' tracing in the trap on INT, TERM, and HUP to avoid this issue, as it disables tracing in the chained trap on EXIT as well. Wrap it in a '{ ... } 2>/dev/null' block, so the trace of the command disabling the tracing doesn't go to standard error either [5].

Note that it's not only '-x' tracing that can be problematic, but any shell builtin, e.g. 'echo', that writes to standard output or error in the trap on EXIT, while a test running with 'dash' and '--verbose-log' (even without '-x') is interrupted. As far as I can tell, this is not an issue at the moment:

 - The cleanup commands to stop the credential-helper, Apache, or
   'p4d' don't use any such shell builtins.

 - stop_git_daemon() does use 'say' and 'error', both wrappers around
   'echo', but it redirects 'say' to fd 3, i.e. to the log file, and
   while 'error' does write to standard output, it comes only after
   the daemon was killed.

 - The non-builtin commands that actually stop the daemons ('kill',
   'apache2 -k stop', 'git credential-cache exit') are silent, so they
   won't get SIGPIPE before finishing their job.

[1] The trap on EXIT must run cleanup commands, because we want to
    stop any daemons when a test script run with '--immediate' fails
    and exits early with error.  By chaining up the trap on signals to
    the trap on EXIT we can deal with cleanup commands a bit simpler,
    because the tests involving daemons only have to set a single
    trap.

[2] http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#trap

[3] The shells I tried: dash, Bash, ksh, ksh93, mksh, lksh, yash,
    BusyBox sh, FreeBSD /bin/sh, NetBSD /bin/sh.

[4] $ cat trap-output.sh
    #!/bin/sh
    trap "echo output; echo error >&2" EXIT
    { exit; } >OUT 2>ERR
    $ dash ./trap-output.sh
    output
    error
    $ wc -c OUT ERR
    0 OUT
    0 ERR

    On a related note, 'ksh', 'ksh93', and BusyBox sh don't conform to
    the specs in this respect, either.

[5] This '{ set +x; } 2>/dev/null' trick won't help those shells that
    show trace output for any redirections and don't preserve open
    file descriptors for the trap on INT, TERM and HUP.  The only such
    shells I'm aware of are 'mksh' and 'lksh'.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/test-lib.sh | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

SZEDER Gábor <szeder.dev@gmail.com> writes:

So... the clean-up actions do not get a chance to run because the shell that is going to execute would be killed by SIGPIPE because tee that is reading from it goes away?

While I like the patch very much, I also wonder if it is possible to tell tee to not to die upon seeing INT, TERM, etc. When the shell upstream of tee exits (after performing clean-up actions), tee that is reading from it will exit, too, and will not be left behind (I do not mean to say it that is a better alternative solution---I am just trying to see if I read the problem correctly from the description given above).

Thanks.

On Thu, Mar 14, 2019 at 11:18:41AM +0900, Junio C Hamano wrote:

Yes.

'tee -i' ignores only INT, but not TERM and HUP.

What could work is something like:

  <re-executing the test script> | (
        # Ignore common interrupt signals to prevent 'tee' from dying
        # while the upstream test process still produces output.
        trap '' INT TERM HUP
        tee logfile
  )

but I'm not sure what should happen with a process ignoring HUP when it still produces output to the terminal after that terminal has been closed, i.e. this 'tee' process. FWIW, In my quick test such a process continued happily without receiving SIGPIPE or other unpleasantness.

After 'start_git_daemon' starts 'git daemon' (note the space in the middle) in the background, it saves the background process' PID, so the daemon can be stopped at the end of the test script. However, 'git-daemon' is not a builtin but a dashed external command, which means that the dashless 'git daemon' executes the dashed 'git-daemon' command, and, consequently, the PID recorded is not the PID of the "real" daemon process, but that of the main 'git' wrapper. Now, if a test script involving 'git daemon' is interrupted by ctrl-C, then only the main 'git' process is stopped, but the real daemon process tends to survive somehow, and keeps on running in the background indefinitely, keeping the daemon's port to itself, and thus preventing subsequent runs of the same test script.

Work this around by running 'git daemon' with the '--pidfile=...' option to save the PID of the real daemon process, and kill that process in 'stop_git_daemon' as well.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/lib-git-daemon.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

SZEDER Gábor <szeder.dev@gmail.com> writes:

Hmph. execv_dashed_external() does not quite execv() but uses run_command() and waits, which is the source of this issue. It feels almost possible to work it around by actually exec'ing, though.

	/*
	 * If we fail because the command is not found, it is
	 * OK to return. Otherwise, we just pass along the status code,
	 * or our usual generic code if we were not even able to exec
	 * the program.
	 */
	status = run_command(&cmd);
	if (status >= 0)
		exit(status);
	else if (errno != ENOENT)
		exit(128);

would become

	... prepare args from cmd ...
	execv(...);
	if (errno != ENOENT)
		exit(128);

and if exec succeeds, the whole process will exit with the status of dashed command, failing to exec for reasons other than ENOENT will exit with 128 and ENOENT will continue returning.

OK, so for the purpose of waiting and monitoring, we still use the pid of the git potty that is running and waiting for the daemon, but we'll send another signal to kill the daemon off. Makes sense.

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Use 'test_atexit' to run cleanup commands to stop 'git-daemon' at the end of the test script or upon interrupt or failure, as it is shorter, simpler, and more robust than registering such cleanup commands in the trap on EXIT in the test scripts.

Note that in 't5570-git-daemon.sh' the daemon is stopped and then re-started in the middle of the test script; take care that the cleanup functions to stop the daemon are only registered once.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/interop/i5500-git-daemon.sh |  1 -
 t/lib-git-daemon.sh           | 14 +++++++++-----
 t/t5570-git-daemon.sh         |  1 -
 3 files changed, 9 insertions(+), 7 deletions(-)

From: Johannes Schindelin <johannes.schindelin@gmx.de>

When running Apache, 'git daemon', or p4d, we want to kill them at the end of the test script, otherwise a leftover daemon process will keep its port open indefinitely, and thus will interfere with subsequent executions of the same test script.

So far, we stop these daemon processes "manually", i.e.:

  - by registering functions or commands in the trap on EXIT to stop
    the daemon while preserving the last seen exit code before the
    trap (to deal with a failure when run with '--immediate' or with
    interrupts by ctrl-C),

  - and by invoking these functions/commands last thing before
    'test_done' (and sometimes restoring the test framework's default
    trap on EXIT, to prevent the daemons from being killed twice).

On one hand, we do this inconsistently, e.g. 'git p4' tests invoke different functions in the trap on EXIT and in the last test before 'test_done', and they neither restore the test framework's default trap on EXIT nor preserve the last seen exit code. On the other hand, this is error prone, because, as shown in a previous patch in this series, any output from the cleanup commands in the trap on EXIT can prevent a proper cleanup when a test script run with '--verbose-log' and certain shells, notably 'dash', is interrupted.

Let's introduce 'test_atexit', which is loosely modeled after 'test_when_finished', but has a broader scope: rather than running the commands after the current test case, run them when the test script finishes, and also run them when the test is interrupted, or exits early in case of a failure while the '--immediate' option is in effect.

When running the cleanup commands at the end of a successful test, then they will be run in 'test_done' before it removes the trash directory, i.e. the cleanup commands will still be able to access any pidfiles or socket files in there. When running the cleanup commands after an interrupt or failure with '--immediate', then they will be run in the trap on EXIT. In both cases they will be run in 'test_eval_', i.e. both standard error and output of all cleanup commands will go where they should according to the '-v' or '--verbose-log' options, and thus won't cause any troubles when interrupting a test script run with '--verbose-log'.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/README                | 20 ++++++++++++++++++++
 t/t0000-basic.sh        | 18 ++++++++++++++++++
 t/test-lib-functions.sh | 28 ++++++++++++++++++++++++++++
 t/test-lib.sh           | 23 +++++++++++++++++++++++
 4 files changed, 89 insertions(+)

SZEDER Gábor <szeder.dev@gmail.com> writes:

This chaining is modelled after how $test_cleaup is built and maintained by test_when_finished. Use of eval_ret makes sense in that original context as eval_ret _is_ used to keep track of the result of 'test_eval_ "$1"' in test_run_ that executed the body of a single test_expect_$something, and $test_cleanup would want to keep the resulting status from that body when clean-up succeeds (or otherwise, make that failure to clean-up visible as $eval_ret).

But does it make sense in the context of the whole test script to try maintaining $eval_ret?

We are guaranteed to still have the trash directory when we are run in the exit handler after getting interrupted or test_failure_() exits under the "-i" option, and when test_done() calls us. What will cause us trouble is the exit handler at the end of a successful run after test_done() finishes. At that point, test_done would have already cleared the trash directory, so we may not have enough state to allow us to clean-up at exit.

Clearing the exit trap in test_done after it calls us might be an alternative, but I think it is equivalent to clearing the test_atexit_cleanup variable, and it is cleaner, so I think I agree with the approach this patch uses.

I think test_when_finished uses a special value in $test_cleanup in a similar way but it even skips when there is no point doing the test_eval_ of the "accumulated" scriptlet when it is empty.

Shouldn't we be doing the same thing, i.e.

	if test -z "$test_atexit_cleanup"
	then
		return 0
	fi
	... do the heavy lifting ...
	test_atexit_cleanup=

That will make the handler truly a no-op when there is no atexit defined.

On Thu, Mar 14, 2019 at 12:21:00PM +0900, Junio C Hamano wrote:

Right, it doesn't, as 'die' preserves the last seen exit code, and any exit codes from the atexit commands are ignored anyway.

That's right.

But this is not, because $test_cleanup is initialized to this special value and it can never be empty, and indeed 'test_eval_' uses this condition:

  if test -z "$immediate" || test $eval_ret = 0 ||
     test -n "$expecting_failure" && test "$test_cleanup" != ":"

and it never checks $test_cleanup's emptiness.

$test_atexit_cleanup is used the same way as $test_cleanup; it's initialized to the same special value and can never be empty, so there is no need to check for its emptiness either.

SZEDER Gábor <szeder.dev@gmail.com> writes:

Yeah, I used "empty" not in the literal sense; I know why the "empty in spirit" setting is a single colon for $test_cleanup.

I just did not realize that this new variable was using that exact pattern and using ":" as the empty in spirit, and that was where my fuzzy wordibng came from.

So in short, I misread the code, and part of that is because I was misled by the comment:

It over-stresses the "run only once", but the true value of this is that it avoids running an "empty in spirit" clean-up sequence.

The avoidance of double execution merely takes advantage of this implementation detail by "resetting" the variable is better explained where the "resetting" happens (i.e. "we reset the variable to the 'no-command' state, as we've run all of them here, but just before the process finally exits, the helper will be called and we do not want to run these commands again when it happens").

Thanks.

Use 'test_atexit' to run cleanup commands to stop httpd at the end of the test script or upon interrupt or failure, as it is shorter, simpler, and more robust than registering such cleanup commands in the trap on EXIT in the test scripts.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/lib-git-svn.sh                              | 5 -----
 t/lib-httpd.sh                                | 6 +-----
 t/t0410-partial-clone.sh                      | 2 --
 t/t5500-fetch-pack.sh                         | 3 ---
 t/t5510-fetch.sh                              | 2 --
 t/t5537-fetch-shallow.sh                      | 2 --
 t/t5539-fetch-http-shallow.sh                 | 1 -
 t/t5540-http-push-webdav.sh                   | 2 --
 t/t5541-http-push-smart.sh                    | 1 -
 t/t5542-push-http-shallow.sh                  | 1 -
 t/t5545-push-options.sh                       | 2 --
 t/t5550-http-fetch-dumb.sh                    | 1 -
 t/t5551-http-fetch-smart.sh                   | 1 -
 t/t5561-http-backend.sh                       | 1 -
 t/t5581-http-curl-verbose.sh                  | 2 --
 t/t5601-clone.sh                              | 2 --
 t/t5616-partial-clone.sh                      | 2 --
 t/t5700-protocol-v1.sh                        | 2 --
 t/t5702-protocol-v2.sh                        | 2 --
 t/t5703-upload-pack-ref-in-want.sh            | 2 --
 t/t5812-proto-disable-http.sh                 | 1 -
 t/t9115-git-svn-dcommit-funky-renames.sh      | 2 --
 t/t9118-git-svn-funky-branch-names.sh         | 2 --
 t/t9120-git-svn-clone-with-percent-escapes.sh | 2 --
 t/t9142-git-svn-shallow-clone.sh              | 2 --
 25 files changed, 1 insertion(+), 50 deletions(-)

SZEDER Gábor <szeder.dev@gmail.com> writes:

I see most of these changes are removal of stop_httpd because it is done as part of start_httpd() to arrange it to be called at exit.

But ...

... I see we lost many "trap 'die' EXIT" in the orignal. Is that something we want to lose as part of this commit? It does not make sense, at least to me, to add a "test_atexit die" and I am mostly wondering what these traps were trying to do in the original.

Junio C Hamano <gitster@pobox.com> writes:

And the answer of course is that we cannot afford to, and it does not make sense to, keep it, as one of the early thing die() does is to execute the atexit cleanup scriptlets.

On Thu, Mar 14, 2019 at 12:28:35PM +0900, Junio C Hamano wrote:

It restored our test framework's default EXIT trap, because without it 'stop_httpd' would have been invoked after 'test_done' as well. While invoking it twice is probably not that big of a deal (though arguably not the cleanest solution), invoking it after 'test_done' is definitely bad, because at that point the trash directory and thus $HTTPD_ROOT_PATH has already been removed, resulting in an ugly error:

  $ ./t5561-http-backend.sh
  <....>
  ok 14 - server request log matches test results
  # passed all 14 test(s)
  1..14
  apache2: Syntax error on line 11 of /home/szeder/src/git/t/lib-httpd/apache.conf: Cannot load modules/mod_alias.so into server: /home/szeder/src/git/t/trash directory.t5561-http-backend/httpd/modules/mod_alias.so: cannot open shared object file: No such file or directory

'test_atexit' doesn't overwrite the test frameowork's default EXIT trap, so we don't have to restore anything.

Use 'test_atexit' to run cleanup commands to stop the credentials helper at the end of the test script or upon interrupt or failure, as it is shorter, simpler, and more robust than registering such cleanup commands in the trap on EXIT in the test scripts.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t0301-credential-cache.sh | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Use 'test_atexit' to run cleanup commands to stop 'p4d' at the end of the test script or upon interrupt or failure, as it is shorter, simpler, and more robust than registering such cleanup commands in the trap on EXIT in the test scripts.

Note that one of the test scripts, 't9801-git-p4-branch.sh', stops and then re-starts 'p4d' twice in the middle of the script; take care that the cleanup functions to stop 'p4d' are only registered once.

Note also that 'git p4' tests invoke different functions in the trap on EXIT ('cleanup') and in the last test before 'test_done' ('kill_p4d'). Register both of these functions with 'test_atexit' for now, and a a later patch in this series will then clean up the redundancy.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/lib-git-p4.sh                            | 10 +++++++++-
 t/t9800-git-p4-basic.sh                    |  4 ----
 t/t9801-git-p4-branch.sh                   |  4 ----
 t/t9802-git-p4-filetype.sh                 |  4 ----
 t/t9803-git-p4-shell-metachars.sh          |  4 ----
 t/t9804-git-p4-label.sh                    |  4 ----
 t/t9805-git-p4-skip-submit-edit.sh         |  4 ----
 t/t9806-git-p4-options.sh                  |  5 -----
 t/t9807-git-p4-submit.sh                   |  4 ----
 t/t9808-git-p4-chdir.sh                    |  4 ----
 t/t9809-git-p4-client-view.sh              |  4 ----
 t/t9810-git-p4-rcs.sh                      |  4 ----
 t/t9811-git-p4-label-import.sh             |  5 -----
 t/t9812-git-p4-wildcards.sh                |  4 ----
 t/t9813-git-p4-preserve-users.sh           |  4 ----
 t/t9814-git-p4-rename.sh                   |  4 ----
 t/t9815-git-p4-submit-fail.sh              |  4 ----
 t/t9816-git-p4-locked.sh                   |  4 ----
 t/t9817-git-p4-exclude.sh                  |  4 ----
 t/t9818-git-p4-block.sh                    |  4 ----
 t/t9819-git-p4-case-folding.sh             |  4 ----
 t/t9820-git-p4-editor-handling.sh          |  4 ----
 t/t9821-git-p4-path-variations.sh          |  4 ----
 t/t9822-git-p4-path-encoding.sh            |  4 ----
 t/t9823-git-p4-mock-lfs.sh                 |  4 ----
 t/t9824-git-p4-git-lfs.sh                  |  4 ----
 t/t9825-git-p4-handle-utf16-without-bom.sh |  4 ----
 t/t9826-git-p4-keep-empty-commits.sh       |  4 ----
 t/t9827-git-p4-change-filetype.sh          |  4 ----
 t/t9828-git-p4-map-user.sh                 |  4 ----
 t/t9829-git-p4-jobs.sh                     |  4 ----
 t/t9830-git-p4-symlink-dir.sh              |  4 ----
 t/t9831-git-p4-triggers.sh                 |  4 ----
 t/t9832-unshelve.sh                        |  3 ---
 t/t9833-errors.sh                          |  5 -----
 35 files changed, 9 insertions(+), 139 deletions(-)

According to the comments in 't/lib-git-p4.sh', sometimes 'p4d' seems to hang, and to deal with that 'start_p4d' starts a watchdog process to kill it after a long-enough timeout ($P4D_TIMEOUT, defaults to 300s). This watchdog process is implemented as a background subshell loop iterating once every second until the timeout expires, producing a few lines of trace output on each iteration when the test script is run with '-x' tracing enabled. The watchdog loop's trace gets intermixed with the real test output and trace, and makes that harder to read.

Send the trace output of this loop to /dev/null to avoid polluting the real test output.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/lib-git-p4.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

'lib-git-p4.sh' uses timeouts in a watchdog process to kill a potentially stuck 'p4d' process and for certain cleanup operation between tests. It does so by first computing when the timeout should expire, and then repeatedly asking for the current time in seconds until it exceeds the expiration time, and for portability reasons it uses a one-liner Python script to ask for the current time.

Replace these timeouts with downcounters, which, though not necessarily shorter, are much simpler, at least in the sense that they don't execute the Python interpreter every second.

After this change the helper function with that Python one-liner has no callers left, remove it.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/lib-git-p4.sh | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

In 't9811-git-p4-label-import.sh' the test 'tag that cannot be exported' runs

  !(p4 labels | grep GIT_TAG_ON_A_BRANCH)

to check that the given string is not printed by 'p4 labels'. This is problematic, because according to POSIX [1]:

  "If the pipeline begins with the reserved word ! and command1 is a
  subshell command, the application shall ensure that the ( operator
  at the beginning of command1 is separated from the ! by one or more
  <blank> characters. The behavior of the reserved word ! immediately
  followed by the ( operator is unspecified."

While most common shells still interpret this '!' as "negate the exit code of the last command in the pipeline", 'mksh/lksh' don't and interpret it as a negative file name pattern instead. As a result they attempt to run a command made up of the pathnames in the current directory (it contains a single directory called 'main'), which, of course, fails the test.

We could fix it simply by adding a space between the '!' and '(', but instead let's fix it by removing the unnecessary subshell.

[1] http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09_02

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t9811-git-p4-label-import.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Confusingly, the 'git p4' tests used two cleanup functions:

  - 'kill_p4d' was run in the last test before 'test_done', and it not
    only killed 'p4d', but it killed the watchdog process, and cleaned
    up after 'p4d' as well by removing all directories used by the P4
    daemon and client.

    This cleanup is not necessary right before 'test_done', because
    the whole trash directory is about to get removed anyway, but it
    is necessary in 't9801-git-p4-branch.sh', which uses 'kill_p4d' to
    stop 'p4d' before re-starting it in the middle of the test script.

  - 'cleanup' was run in the trap on EXIT, and it killed 'p4d', but,
    it didn't kill the watchdog process, and, contrarily to its name,
    didn't perform any cleanup whatsoever.

Make it clearer what's going on by renaming and simplifying the cleanup functions, so in the end we'll have:

  - 'stop_p4d_and_watchdog' replaces 'cleanup' as it will try to live
    up to its name and stop both the 'p4d' and the watchdog processes,
    and as the sole function registered with 'test_atexit' it will be
    responsible for no leaving any stray processes behind after 'git p4'
    tests were finished or interrupted.

  - 'stop_and_cleanup_p4d' replaces 'kill_p4d' as it will stop 'p4d'
    (and the watchdog) and remove all directories used by the P4
    daemon and cliean, so it can be used mid-script to stop and then
    re-start 'p4d'.

Note that while 'cleanup' sent a single SIGKILL to 'p4d', 'kill_p4d' was quite brutal, as it first sent SIGTERM to the daemon repeatedly, either until its pid disappeared or until a given timeout was up, and then it sent SIGKILL repeatedly, for good measure. This is overkill (pardon the pun): a single SIGKILL should be able to take down any process in a sensible state, and if a process were to somehow end up stuck in the dreaded uninterruptible sleep state then even repeated SIGKILLs won't bring immediate help. So ditch all the repeated SIGTERM/SIGKILL parts, and use a single SIGKILL to stop 'p4d', and make sure that there are no races between asynchron signal delivery and subsequent restart of 'p4d' by waiting for it to die.

With this change the 'retry_until_fail' helper has no callers left, remove it.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/lib-git-p4.sh          | 36 ++++++++++--------------------------
 t/t9801-git-p4-branch.sh |  4 ++--
 2 files changed, 12 insertions(+), 28 deletions(-)