[PATCH 00/11] tests: introduce 'test_atexit'

[PATCH 00/11] tests: introduce 'test_atexit'

[SZEDER Gábor]

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
-- 
2.21.0.499.g4d310c7a8e.dirty

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

[SZEDER Gábor]

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

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 8665b0a9b6..db3875d1e4 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -631,7 +631,10 @@ die () {
 
 GIT_EXIT_OK=
 trap 'die' EXIT
-trap 'exit $?' INT TERM HUP
+# Disable '-x' tracing, because with some shells, notably dash, it
+# prevents running the cleanup commands when a test script run with
+# '--verbose-log -x' is interrupted.
+trap '{ code=$?; set +x; } 2>/dev/null; exit $code' INT TERM HUP
 
 # The user-facing functions are loaded from a separate file so that
 # test_perf subshells can have them too
-- 
2.21.0.499.g4d310c7a8e.dirty

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

[SZEDER Gábor]

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

diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
index 79db3b7ae5..6dab8766e7 100644
--- a/t/lib-git-daemon.sh
+++ b/t/lib-git-daemon.sh
@@ -31,6 +31,7 @@ fi
 test_set_port LIB_GIT_DAEMON_PORT
 
 GIT_DAEMON_PID=
+GIT_DAEMON_PIDFILE="$PWD"/daemon.pid
 GIT_DAEMON_DOCUMENT_ROOT_PATH="$PWD"/repo
 GIT_DAEMON_HOST_PORT=127.0.0.1:$LIB_GIT_DAEMON_PORT
 GIT_DAEMON_URL=git://$GIT_DAEMON_HOST_PORT
@@ -49,7 +50,7 @@ start_git_daemon() {
 	mkfifo git_daemon_output
 	${LIB_GIT_DAEMON_COMMAND:-git daemon} \
 		--listen=127.0.0.1 --port="$LIB_GIT_DAEMON_PORT" \
-		--reuseaddr --verbose \
+		--reuseaddr --verbose --pid-file="$GIT_DAEMON_PIDFILE" \
 		--base-path="$GIT_DAEMON_DOCUMENT_ROOT_PATH" \
 		"$@" "$GIT_DAEMON_DOCUMENT_ROOT_PATH" \
 		>&3 2>git_daemon_output &
@@ -88,8 +89,9 @@ stop_git_daemon() {
 	then
 		error "git daemon exited with status: $ret"
 	fi
+	kill "$(cat "$GIT_DAEMON_PIDFILE")" 2>/dev/null
 	GIT_DAEMON_PID=
-	rm -f git_daemon_output
+	rm -f git_daemon_output "$GIT_DAEMON_PIDFILE"
 }
 
 # A stripped-down version of a netcat client, that connects to a "host:port"
-- 
2.21.0.499.g4d310c7a8e.dirty

[PATCH 03/11] test-lib: introduce 'test_atexit'

[SZEDER Gábor]

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

diff --git a/t/README b/t/README
index 886bbec5bc..af510d236a 100644
--- a/t/README
+++ b/t/README
@@ -862,6 +862,26 @@ library for your script to use.
 		...
 	'
 
+ - 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
index b6566003dd..c03054c538 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -825,6 +825,24 @@ test_expect_success 'tests clean up even on failures' "
 	EOF
 "
 
+test_expect_success 'test_atexit is run' "
+	test_must_fail run_sub_test_lib_test \
+		atexit-cleanup 'Run atexit commands' -i <<-\\EOF &&
+	test_expect_success 'tests clean up even after a failure' '
+		> ../../clean-atexit &&
+		test_atexit rm ../../clean-atexit &&
+		> ../../also-clean-atexit &&
+		test_atexit rm ../../also-clean-atexit &&
+		> ../../dont-clean-atexit &&
+		(exit 1)
+	'
+	test_done
+	EOF
+	test_path_is_file dont-clean-atexit &&
+	test_path_is_missing clean-atexit &&
+	test_path_is_missing also-clean-atexit
+"
+
 test_expect_success 'test_oid setup' '
 	test_oid_init
 '
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 80402a428f..6a50dba390 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -934,6 +934,34 @@ test_when_finished () {
 		} && (exit \"\$eval_ret\"); eval_ret=\$?; $test_cleanup"
 }
 
+# This function can be used to schedule some commands to be run
+# unconditionally at the end of the test script, 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_atexit () {
+	# We cannot detect when we are in a subshell in general, but by
+	# doing so on Bash is better than nothing (the test will
+	# silently pass on other shells).
+	test "${BASH_SUBSHELL-0}" = 0 ||
+	error "bug in test script: test_atexit does nothing in a subshell"
+	test_atexit_cleanup="{ $*
+		} && (exit \"\$eval_ret\"); eval_ret=\$?; $test_atexit_cleanup"
+}
+
 # Most tests can use the created repository, but some may need to create more.
 # Usage: test_create_repo <directory>
 test_create_repo () {
diff --git a/t/test-lib.sh b/t/test-lib.sh
index db3875d1e4..b35881696f 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -620,6 +620,10 @@ test_external_has_tap=0
 
 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
 		exit $code
@@ -1045,9 +1049,28 @@ write_junit_xml_testcase () {
 	junit_have_testcase=t
 }
 
+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
 
+	# 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
 		test -n "$junit_have_testcase" || {
-- 
2.21.0.499.g4d310c7a8e.dirty

[PATCH 04/11] git-daemon: use 'test_atexit` to stop 'git-daemon'

[SZEDER Gábor]

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

diff --git a/t/interop/i5500-git-daemon.sh b/t/interop/i5500-git-daemon.sh
index 1daf69420b..4d22e42f84 100755
--- a/t/interop/i5500-git-daemon.sh
+++ b/t/interop/i5500-git-daemon.sh
@@ -37,5 +37,4 @@ test_expect_success "fetch with $VERSION_B" '
 	test_cmp expect actual
 '
 
-stop_git_daemon
 test_done
diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
index 6dab8766e7..7b3407134e 100644
--- a/t/lib-git-daemon.sh
+++ b/t/lib-git-daemon.sh
@@ -13,7 +13,6 @@
 #
 #	test_expect_success ...
 #
-#	stop_git_daemon
 #	test_done
 
 test_tristate GIT_TEST_GIT_DAEMON
@@ -36,6 +35,7 @@ GIT_DAEMON_DOCUMENT_ROOT_PATH="$PWD"/repo
 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
@@ -44,7 +44,13 @@ start_git_daemon() {
 
 	mkdir -p "$GIT_DAEMON_DOCUMENT_ROOT_PATH"
 
-	trap 'code=$?; stop_git_daemon; (exit $code); die' EXIT
+	# 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
@@ -66,7 +72,7 @@ start_git_daemon() {
 	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
@@ -78,8 +84,6 @@ stop_git_daemon() {
 		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
index 58ee787685..00fc612cac 100755
--- a/t/t5570-git-daemon.sh
+++ b/t/t5570-git-daemon.sh
@@ -198,5 +198,4 @@ test_expect_success FAKENC 'hostname interpolation works after LF-stripping' '
 	test_cmp expect actual
 '
 
-stop_git_daemon
 test_done
-- 
2.21.0.499.g4d310c7a8e.dirty

[PATCH 05/11] tests: use 'test_atexit' to stop httpd

[SZEDER Gábor]

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

diff --git a/t/lib-git-svn.sh b/t/lib-git-svn.sh
index f3b478c307..c1271d6863 100644
--- a/t/lib-git-svn.sh
+++ b/t/lib-git-svn.sh
@@ -76,11 +76,6 @@ maybe_start_httpd () {
 		LIB_HTTPD_SVN="$loc"
 		start_httpd
 		;;
-	*)
-		stop_httpd () {
-			: noop
-		}
-		;;
 	esac
 }
 
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 0dfb48c2f6..b3cc62bd36 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -14,7 +14,6 @@
 #
 #	test_expect_success ...
 #
-#	stop_httpd
 #	test_done
 #
 # Can be configured using the following variables.
@@ -176,7 +175,7 @@ prepare_httpd() {
 start_httpd() {
 	prepare_httpd >&3 2>&4
 
-	trap 'code=$?; stop_httpd; (exit $code); die' EXIT
+	test_atexit stop_httpd
 
 	"$LIB_HTTPD_PATH" -d "$HTTPD_ROOT_PATH" \
 		-f "$TEST_PATH/apache.conf" $HTTPD_PARA \
@@ -184,15 +183,12 @@ start_httpd() {
 		>&3 2>&4
 	if test $? -ne 0
 	then
-		trap 'die' EXIT
 		cat "$HTTPD_ROOT_PATH"/error.log >&4 2>/dev/null
 		test_skip_or_die $GIT_TEST_HTTPD "web server setup failed"
 	fi
 }
 
 stop_httpd() {
-	trap 'die' EXIT
-
 	"$LIB_HTTPD_PATH" -d "$HTTPD_ROOT_PATH" \
 		-f "$TEST_PATH/apache.conf" $HTTPD_PARA -k stop
 }
diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index bce02788e6..5bd892f2f7 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -518,6 +518,4 @@ test_expect_success 'fetching of missing objects from an HTTP server' '
 	git verify-pack --verbose "$IDX" | grep "$HASH"
 '
 
-stop_httpd
-
 test_done
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 49c540b1e1..32426fa5d1 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -918,7 +918,4 @@ test_expect_success 'fetch with --filter=blob:limit=0 and HTTP' '
 	fetch_filter_blob_limit_zero "$HTTPD_DOCUMENT_ROOT_PATH/server" "$HTTPD_URL/smart/server"
 '
 
-stop_httpd
-
-
 test_done
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 3b7b30568c..e98d90dd9b 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -978,6 +978,4 @@ test_expect_success '--negotiation-tip limits "have" lines sent with HTTP protoc
 	check_negotiation_tip
 '
 
-stop_httpd
-
 test_done
diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index 6caf628efa..66f0b64d39 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -255,6 +255,4 @@ test_expect_success 'shallow fetches check connectivity before writing shallow f
 	git -C client fsck
 '
 
-stop_httpd
-
 test_done
diff --git a/t/t5539-fetch-http-shallow.sh b/t/t5539-fetch-http-shallow.sh
index 5fbf67c446..98f028f203 100755
--- a/t/t5539-fetch-http-shallow.sh
+++ b/t/t5539-fetch-http-shallow.sh
@@ -146,5 +146,4 @@ test_expect_success 'fetching deepen' '
 	)
 '
 
-stop_httpd
 test_done
diff --git a/t/t5540-http-push-webdav.sh b/t/t5540-http-push-webdav.sh
index 88ff5a49e4..a094fd5e71 100755
--- a/t/t5540-http-push-webdav.sh
+++ b/t/t5540-http-push-webdav.sh
@@ -176,6 +176,4 @@ test_expect_failure 'push to password-protected repository (no user in URL)' '
 	test_cmp expect actual
 '
 
-stop_httpd
-
 test_done
diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
index 5475afc052..bdf40f445e 100755
--- a/t/t5541-http-push-smart.sh
+++ b/t/t5541-http-push-smart.sh
@@ -373,5 +373,4 @@ test_expect_success 'colorize errors/hints' '
 	test_i18ngrep ! "^hint: " decoded
 '
 
-stop_httpd
 test_done
diff --git a/t/t5542-push-http-shallow.sh b/t/t5542-push-http-shallow.sh
index 5165833157..ddc1db722d 100755
--- a/t/t5542-push-http-shallow.sh
+++ b/t/t5542-push-http-shallow.sh
@@ -90,5 +90,4 @@ EOF
 	)
 '
 
-stop_httpd
 test_done
diff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh
index b47a95871c..6d1d59c9b1 100755
--- a/t/t5545-push-options.sh
+++ b/t/t5545-push-options.sh
@@ -278,6 +278,4 @@ test_expect_success 'push options keep quoted characters intact (http)' '
 	test_cmp expect "$HTTPD_DOCUMENT_ROOT_PATH"/upstream.git/hooks/pre-receive.push_options
 '
 
-stop_httpd
-
 test_done
diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
index 6d7d88ccc9..57f6f8c628 100755
--- a/t/t5550-http-fetch-dumb.sh
+++ b/t/t5550-http-fetch-dumb.sh
@@ -408,5 +408,4 @@ test_expect_success 'print HTTP error when any intermediate redirect throws erro
 	test_i18ngrep "unable to access.*/redir-to/502" stderr
 '
 
-stop_httpd
 test_done
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index ba83e567e5..9faf6349cf 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -434,5 +434,4 @@ test_expect_success 'server-side error detected' '
 	grep "server-side error" actual
 '
 
-stop_httpd
 test_done
diff --git a/t/t5561-http-backend.sh b/t/t5561-http-backend.sh
index 1c49054595..6eb0294978 100755
--- a/t/t5561-http-backend.sh
+++ b/t/t5561-http-backend.sh
@@ -132,5 +132,4 @@ test_expect_success 'server request log matches test results' '
 	check_access_log exp
 '
 
-stop_httpd
 test_done
diff --git a/t/t5581-http-curl-verbose.sh b/t/t5581-http-curl-verbose.sh
index cd9283eeec..5129b0724f 100755
--- a/t/t5581-http-curl-verbose.sh
+++ b/t/t5581-http-curl-verbose.sh
@@ -23,6 +23,4 @@ test_expect_success 'failure in git-upload-pack is shown' '
 	grep "< HTTP/1.1 500 Intentional Breakage" curl_log
 '
 
-stop_httpd
-
 test_done
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index d6948cbdab..b04d668684 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -733,6 +733,4 @@ test_expect_success 'partial clone using HTTP' '
 	partial_clone "$HTTPD_DOCUMENT_ROOT_PATH/server" "$HTTPD_URL/smart/server"
 '
 
-stop_httpd
-
 test_done
diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
index 9643acb161..9a8f9886b3 100755
--- a/t/t5616-partial-clone.sh
+++ b/t/t5616-partial-clone.sh
@@ -331,6 +331,4 @@ test_expect_success 'when partial cloning, tolerate server not sending target of
 	! test -e "$HTTPD_ROOT_PATH/one-time-sed"
 '
 
-stop_httpd
-
 test_done
diff --git a/t/t5700-protocol-v1.sh b/t/t5700-protocol-v1.sh
index ba86a44eb1..b0e4752232 100755
--- a/t/t5700-protocol-v1.sh
+++ b/t/t5700-protocol-v1.sh
@@ -289,6 +289,4 @@ test_expect_success 'push with http:// using protocol v1' '
 	grep "git< version 1" log
 '
 
-stop_httpd
-
 test_done
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index db4ae09f2f..35424bb8af 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -656,6 +656,4 @@ test_expect_success 'when server does not send "ready", expect FLUSH' '
 	test_i18ngrep "expected no other sections to be sent after no .ready." err
 '
 
-stop_httpd
-
 test_done
diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh
index f87b2f6df3..b6a995e857 100755
--- a/t/t5703-upload-pack-ref-in-want.sh
+++ b/t/t5703-upload-pack-ref-in-want.sh
@@ -257,8 +257,6 @@ test_expect_success 'server loses a ref - ref in want' '
 	test_i18ngrep "fatal: remote error: unknown ref refs/heads/raster" err
 '
 
-stop_httpd
-
 REPO="$(pwd)/repo"
 LOCAL_PRISTINE="$(pwd)/local_pristine"
 
diff --git a/t/t5812-proto-disable-http.sh b/t/t5812-proto-disable-http.sh
index 872788ac8c..af8772fada 100755
--- a/t/t5812-proto-disable-http.sh
+++ b/t/t5812-proto-disable-http.sh
@@ -34,5 +34,4 @@ test_expect_success 'http can be limited to from-user' '
 		clone "$HTTPD_URL/smart-redir-perm/repo.git" redir.git
 '
 
-stop_httpd
 test_done
diff --git a/t/t9115-git-svn-dcommit-funky-renames.sh b/t/t9115-git-svn-dcommit-funky-renames.sh
index 64bb495834..9b44a44bc1 100755
--- a/t/t9115-git-svn-dcommit-funky-renames.sh
+++ b/t/t9115-git-svn-dcommit-funky-renames.sh
@@ -120,6 +120,4 @@ test_expect_success !MINGW,!UTF8_NFD_TO_NFC 'svn.pathnameencoding=cp932 rename o
 	git svn dcommit
 '
 
-stop_httpd
-
 test_done
diff --git a/t/t9118-git-svn-funky-branch-names.sh b/t/t9118-git-svn-funky-branch-names.sh
index 41a026637f..a159ff96b7 100755
--- a/t/t9118-git-svn-funky-branch-names.sh
+++ b/t/t9118-git-svn-funky-branch-names.sh
@@ -87,6 +87,4 @@ test_expect_success 'test dcommit to trailing_dotlock branch' '
 	)
 	'
 
-stop_httpd
-
 test_done
diff --git a/t/t9120-git-svn-clone-with-percent-escapes.sh b/t/t9120-git-svn-clone-with-percent-escapes.sh
index b28a1741e3..40b714df31 100755
--- a/t/t9120-git-svn-clone-with-percent-escapes.sh
+++ b/t/t9120-git-svn-clone-with-percent-escapes.sh
@@ -74,6 +74,4 @@ test_expect_success 'test clone -s with unescaped space' '
 	)
 '
 
-stop_httpd
-
 test_done
diff --git a/t/t9142-git-svn-shallow-clone.sh b/t/t9142-git-svn-shallow-clone.sh
index 9ee23be640..a30730502d 100755
--- a/t/t9142-git-svn-shallow-clone.sh
+++ b/t/t9142-git-svn-shallow-clone.sh
@@ -26,6 +26,4 @@ test_expect_success 'clone trunk with "-r HEAD"' '
 	( cd g && git rev-parse --symbolic --verify HEAD )
 '
 
-stop_httpd
-
 test_done
-- 
2.21.0.499.g4d310c7a8e.dirty

[PATCH 06/11] t0301-credential-cache: use 'test_atexit' to stop the credentials helper

[SZEDER Gábor]

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

diff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh
index fd92533acf..ebd5fa5249 100755
--- a/t/t0301-credential-cache.sh
+++ b/t/t0301-credential-cache.sh
@@ -10,7 +10,7 @@ test -z "$NO_UNIX_SOCKETS" || {
 }
 
 # don't leave a stale daemon running
-trap 'code=$?; git credential-cache exit; (exit $code); die' EXIT
+test_atexit 'git credential-cache exit'
 
 # test that the daemon works with no special setup
 helper_test cache
@@ -108,9 +108,4 @@ test_expect_success SYMLINKS 'use user socket if user directory is a symlink to
 
 helper_test_timeout cache --timeout=1
 
-# we can't rely on our "trap" above working after test_done,
-# as test_done will delete the trash directory containing
-# our socket, leaving us with no way to access the daemon.
-git credential-cache exit
-
 test_done
-- 
2.21.0.499.g4d310c7a8e.dirty

[PATCH 07/11] git p4 test: use 'test_atexit' to kill p4d and the watchdog process

[SZEDER Gábor]

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

diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
index b3be3ba011..958e33b77e 100644
--- a/t/lib-git-p4.sh
+++ b/t/lib-git-p4.sh
@@ -74,7 +74,6 @@ cleanup () {
 		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
@@ -82,7 +81,16 @@ trap cleanup EXIT
 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
index 729cd25770..5856563068 100755
--- a/t/t9800-git-p4-basic.sh
+++ b/t/t9800-git-p4-basic.sh
@@ -326,8 +326,4 @@ test_expect_success 'submit from worktree' '
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9801-git-p4-branch.sh b/t/t9801-git-p4-branch.sh
index 6a86d6996b..50013132c8 100755
--- a/t/t9801-git-p4-branch.sh
+++ b/t/t9801-git-p4-branch.sh
@@ -610,8 +610,4 @@ test_expect_success 'Update a file in git side and submit to P4 using client vie
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9802-git-p4-filetype.sh b/t/t9802-git-p4-filetype.sh
index 9978352d78..94edebe272 100755
--- a/t/t9802-git-p4-filetype.sh
+++ b/t/t9802-git-p4-filetype.sh
@@ -333,8 +333,4 @@ test_expect_success SYMLINKS 'empty symlink target' '
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9803-git-p4-shell-metachars.sh b/t/t9803-git-p4-shell-metachars.sh
index d5c3675100..2913277013 100755
--- a/t/t9803-git-p4-shell-metachars.sh
+++ b/t/t9803-git-p4-shell-metachars.sh
@@ -105,8 +105,4 @@ test_expect_success 'branch with shell char' '
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9804-git-p4-label.sh b/t/t9804-git-p4-label.sh
index e30f80e617..3236457106 100755
--- a/t/t9804-git-p4-label.sh
+++ b/t/t9804-git-p4-label.sh
@@ -108,8 +108,4 @@ test_expect_failure 'two labels on the same changelist' '
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9805-git-p4-skip-submit-edit.sh b/t/t9805-git-p4-skip-submit-edit.sh
index 5fbf904dc8..90ef647db7 100755
--- a/t/t9805-git-p4-skip-submit-edit.sh
+++ b/t/t9805-git-p4-skip-submit-edit.sh
@@ -98,8 +98,4 @@ test_expect_success 'no config, edited' '
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9806-git-p4-options.sh b/t/t9806-git-p4-options.sh
index 3f5291b857..4e794a01bf 100755
--- a/t/t9806-git-p4-options.sh
+++ b/t/t9806-git-p4-options.sh
@@ -300,9 +300,4 @@ test_expect_success 'use --git-dir option and GIT_DIR' '
 	test_path_is_file "$git"/cli_file2.t
 '
 
-
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9807-git-p4-submit.sh b/t/t9807-git-p4-submit.sh
index 850d979119..eaaae414a1 100755
--- a/t/t9807-git-p4-submit.sh
+++ b/t/t9807-git-p4-submit.sh
@@ -593,8 +593,4 @@ test_expect_success 'update a shelve involving moved and copied files' '
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9808-git-p4-chdir.sh b/t/t9808-git-p4-chdir.sh
index 11d2b5102c..58a9b3b71e 100755
--- a/t/t9808-git-p4-chdir.sh
+++ b/t/t9808-git-p4-chdir.sh
@@ -83,8 +83,4 @@ test_expect_success SYMLINKS 'p4 client root symlink should stay symbolic' '
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9809-git-p4-client-view.sh b/t/t9809-git-p4-client-view.sh
index 897b3c3034..3cff1fce1b 100755
--- a/t/t9809-git-p4-client-view.sh
+++ b/t/t9809-git-p4-client-view.sh
@@ -836,8 +836,4 @@ test_expect_success 'quotes on both sides' '
 	git_verify "cdir 1/file11" "cdir 1/file12"
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9810-git-p4-rcs.sh b/t/t9810-git-p4-rcs.sh
index cc53debe19..57b533dc6f 100755
--- a/t/t9810-git-p4-rcs.sh
+++ b/t/t9810-git-p4-rcs.sh
@@ -360,8 +360,4 @@ test_expect_failure 'Add keywords in git which do not match the default p4 value
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh
index 602b0a5d5c..b70e81c3cd 100755
--- a/t/t9811-git-p4-label-import.sh
+++ b/t/t9811-git-p4-label-import.sh
@@ -259,9 +259,4 @@ test_expect_success 'importing labels with missing revisions' '
 	)
 '
 
-
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9812-git-p4-wildcards.sh b/t/t9812-git-p4-wildcards.sh
index 0206771fbb..254a7c2446 100755
--- a/t/t9812-git-p4-wildcards.sh
+++ b/t/t9812-git-p4-wildcards.sh
@@ -211,8 +211,4 @@ test_expect_success 'wildcard files requiring keyword scrub' '
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9813-git-p4-preserve-users.sh b/t/t9813-git-p4-preserve-users.sh
index 783c6ad165..fd018c87a8 100755
--- a/t/t9813-git-p4-preserve-users.sh
+++ b/t/t9813-git-p4-preserve-users.sh
@@ -138,8 +138,4 @@ test_expect_success 'not preserving user with mixed authorship' '
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9814-git-p4-rename.sh b/t/t9814-git-p4-rename.sh
index 60baa06e27..468767cbf4 100755
--- a/t/t9814-git-p4-rename.sh
+++ b/t/t9814-git-p4-rename.sh
@@ -242,8 +242,4 @@ test_expect_success P4D_HAVE_CONFIGURABLE_RUN_MOVE_ALLOW \
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9815-git-p4-submit-fail.sh b/t/t9815-git-p4-submit-fail.sh
index eaf03a6563..9779dc0d11 100755
--- a/t/t9815-git-p4-submit-fail.sh
+++ b/t/t9815-git-p4-submit-fail.sh
@@ -422,8 +422,4 @@ test_expect_success 'cleanup chmod after submit cancel' '
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9816-git-p4-locked.sh b/t/t9816-git-p4-locked.sh
index d048bd33fa..932841003c 100755
--- a/t/t9816-git-p4-locked.sh
+++ b/t/t9816-git-p4-locked.sh
@@ -138,8 +138,4 @@ test_expect_failure 'move with lock taken' '
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9817-git-p4-exclude.sh b/t/t9817-git-p4-exclude.sh
index aac568eadf..96d25f0c02 100755
--- a/t/t9817-git-p4-exclude.sh
+++ b/t/t9817-git-p4-exclude.sh
@@ -64,8 +64,4 @@ test_expect_success 'clone, then sync with exclude' '
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9818-git-p4-block.sh b/t/t9818-git-p4-block.sh
index ce7cb22ad3..0db7ab9918 100755
--- a/t/t9818-git-p4-block.sh
+++ b/t/t9818-git-p4-block.sh
@@ -146,8 +146,4 @@ test_expect_success 'Clone repo with self-sizing block size' '
 	test_line_count \> 10 log
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9819-git-p4-case-folding.sh b/t/t9819-git-p4-case-folding.sh
index d808c008c1..600ce1e0b0 100755
--- a/t/t9819-git-p4-case-folding.sh
+++ b/t/t9819-git-p4-case-folding.sh
@@ -53,8 +53,4 @@ test_expect_failure 'Clone UC repo with lc name' '
 	test_must_fail git p4 clone //depot/uc/...
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9820-git-p4-editor-handling.sh b/t/t9820-git-p4-editor-handling.sh
index 3c22f74bd4..fa1bba1dd9 100755
--- a/t/t9820-git-p4-editor-handling.sh
+++ b/t/t9820-git-p4-editor-handling.sh
@@ -31,8 +31,4 @@ test_expect_success 'EDITOR with options' '
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9821-git-p4-path-variations.sh b/t/t9821-git-p4-path-variations.sh
index 81e46acfa8..ef80f1690b 100755
--- a/t/t9821-git-p4-path-variations.sh
+++ b/t/t9821-git-p4-path-variations.sh
@@ -193,8 +193,4 @@ test_expect_success 'Add a new file and clone path with new file (ignorecase)' '
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9822-git-p4-path-encoding.sh b/t/t9822-git-p4-path-encoding.sh
index c78477c19b..1bf7635016 100755
--- a/t/t9822-git-p4-path-encoding.sh
+++ b/t/t9822-git-p4-path-encoding.sh
@@ -67,8 +67,4 @@ test_expect_success 'Delete iso8859-1 encoded paths and clone' '
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9823-git-p4-mock-lfs.sh b/t/t9823-git-p4-mock-lfs.sh
index 1f2dc369bf..88b76dc4d6 100755
--- a/t/t9823-git-p4-mock-lfs.sh
+++ b/t/t9823-git-p4-mock-lfs.sh
@@ -185,8 +185,4 @@ test_expect_success 'Run git p4 submit in repo configured with large file system
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9824-git-p4-git-lfs.sh b/t/t9824-git-p4-git-lfs.sh
index ed80ca858c..a28dbbdd56 100755
--- a/t/t9824-git-p4-git-lfs.sh
+++ b/t/t9824-git-p4-git-lfs.sh
@@ -287,8 +287,4 @@ test_expect_success 'Add big files to repo and store files in LFS based on compr
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9825-git-p4-handle-utf16-without-bom.sh b/t/t9825-git-p4-handle-utf16-without-bom.sh
index 1551845dc1..f049ff8229 100755
--- a/t/t9825-git-p4-handle-utf16-without-bom.sh
+++ b/t/t9825-git-p4-handle-utf16-without-bom.sh
@@ -43,8 +43,4 @@ test_expect_failure 'clone depot with invalid UTF-16 file in non-verbose mode' '
 	git p4 clone --dest="$git" //depot
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9826-git-p4-keep-empty-commits.sh b/t/t9826-git-p4-keep-empty-commits.sh
index fa8b9daf1f..fd64afe064 100755
--- a/t/t9826-git-p4-keep-empty-commits.sh
+++ b/t/t9826-git-p4-keep-empty-commits.sh
@@ -127,8 +127,4 @@ test_expect_success 'Clone repo subdir with all history' '
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9827-git-p4-change-filetype.sh b/t/t9827-git-p4-change-filetype.sh
index 7433998f47..d3670bd7a2 100755
--- a/t/t9827-git-p4-change-filetype.sh
+++ b/t/t9827-git-p4-change-filetype.sh
@@ -59,8 +59,4 @@ test_expect_success SYMLINKS 'change symbolic link to file' '
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9828-git-p4-map-user.sh b/t/t9828-git-p4-map-user.sh
index e20395c89f..ca6c2942bd 100755
--- a/t/t9828-git-p4-map-user.sh
+++ b/t/t9828-git-p4-map-user.sh
@@ -54,8 +54,4 @@ test_expect_success 'Clone repo root path with all history' '
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9829-git-p4-jobs.sh b/t/t9829-git-p4-jobs.sh
index 971aeeea1f..88cfb1fcd3 100755
--- a/t/t9829-git-p4-jobs.sh
+++ b/t/t9829-git-p4-jobs.sh
@@ -92,8 +92,4 @@ test_expect_success 'check log message of changelist with more jobs' '
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9830-git-p4-symlink-dir.sh b/t/t9830-git-p4-symlink-dir.sh
index 2ad1b0810d..3fb6960c18 100755
--- a/t/t9830-git-p4-symlink-dir.sh
+++ b/t/t9830-git-p4-symlink-dir.sh
@@ -36,8 +36,4 @@ test_expect_success 'symlinked directory' '
 
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9831-git-p4-triggers.sh b/t/t9831-git-p4-triggers.sh
index be44c9751a..d743ca33ee 100755
--- a/t/t9831-git-p4-triggers.sh
+++ b/t/t9831-git-p4-triggers.sh
@@ -96,8 +96,4 @@ test_expect_success 'submit description with extra info lines from verbose p4 ch
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
 test_done
diff --git a/t/t9832-unshelve.sh b/t/t9832-unshelve.sh
index 41c09f11f4..1286a5b824 100755
--- a/t/t9832-unshelve.sh
+++ b/t/t9832-unshelve.sh
@@ -174,8 +174,5 @@ test_expect_success 'unshelve specifying the origin' '
 		test_path_is_file file_to_shelve
 	)
 '
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
 
 test_done
diff --git a/t/t9833-errors.sh b/t/t9833-errors.sh
index 47b312e1c9..e22369ccdf 100755
--- a/t/t9833-errors.sh
+++ b/t/t9833-errors.sh
@@ -45,9 +45,4 @@ test_expect_success 'ticket logged out' '
 	)
 '
 
-test_expect_success 'kill p4d' '
-	kill_p4d
-'
-
-
 test_done
-- 
2.21.0.499.g4d310c7a8e.dirty

[PATCH 08/11] git p4 test: clean up the p4d cleanup functions

[SZEDER Gábor]

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

diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
index 958e33b77e..b5cb5075c2 100644
--- a/t/lib-git-p4.sh
+++ b/t/lib-git-p4.sh
@@ -67,12 +67,8 @@ cli="$TRASH_DIRECTORY/cli"
 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
+stop_p4d_and_watchdog () {
+	kill -9 $p4d_pid $watchdog_pid
 }
 
 # git p4 submit generates a temp file, which will
@@ -87,7 +83,7 @@ start_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'
+		test_atexit 'stop_p4d_and_watchdog'
 		registered_stop_p4d_atexit_handler=AlreadyDone
 	fi
 
@@ -100,6 +96,7 @@ start_p4d () {
 			echo $! >"$pidfile"
 		}
 	) &&
+	p4d_pid=$(cat "$pidfile")
 
 	# This gives p4d a long time to start up, as it can be
 	# quite slow depending on the machine.  Set this environment
@@ -107,14 +104,13 @@ start_p4d () {
 	# an automated test setup.  If the p4d process dies, that
 	# will be caught with the "kill -0" check below.
 	i=${P4D_START_PATIENCE:-300}
-	pid=$(cat "$pidfile")
 
 	timeout=$(($(time_in_seconds) + $P4D_TIMEOUT))
 	while true
 	do
 		if test $(time_in_seconds) -gt $timeout
 		then
-			kill -9 $pid
+			kill -9 $p4d_pid
 			exit 1
 		fi
 		sleep 1
@@ -131,7 +127,7 @@ start_p4d () {
 			break
 		fi
 		# fail if p4d died
-		kill -0 $pid 2>/dev/null || break
+		kill -0 $p4d_pid 2>/dev/null || break
 		echo waiting for p4d to start
 		sleep 1
 		i=$(( $i - 1 ))
@@ -178,22 +174,10 @@ retry_until_success () {
 	done
 }
 
-retry_until_fail () {
-	timeout=$(($(time_in_seconds) + $RETRY_TIMEOUT))
-	until ! "$@" 2>/dev/null || test $(time_in_seconds) -gt $timeout
-	do
-		sleep 1
-	done
-}
-
-kill_p4d () {
-	pid=$(cat "$pidfile")
-	retry_until_fail kill $pid
-	retry_until_fail kill -9 $pid
-	# complain if it would not die
-	test_must_fail kill $pid >/dev/null 2>&1 &&
-	rm -rf "$db" "$cli" "$pidfile" &&
-	retry_until_fail kill -9 $watchdog_pid
+stop_and_cleanup_p4d () {
+	kill -9 $p4d_pid $watchdog_pid
+	wait $p4d_pid
+	rm -rf "$db" "$cli" "$pidfile"
 }
 
 cleanup_git () {
diff --git a/t/t9801-git-p4-branch.sh b/t/t9801-git-p4-branch.sh
index 50013132c8..38d6b9043b 100755
--- a/t/t9801-git-p4-branch.sh
+++ b/t/t9801-git-p4-branch.sh
@@ -151,7 +151,7 @@ test_expect_success 'import depot, branch detection, branchList branch definitio
 '
 
 test_expect_success 'restart p4d' '
-	kill_p4d &&
+	stop_and_cleanup_p4d &&
 	start_p4d
 '
 
@@ -505,7 +505,7 @@ test_expect_success 'use-client-spec detect-branches skips files in branches' '
 '
 
 test_expect_success 'restart p4d' '
-	kill_p4d &&
+	stop_and_cleanup_p4d &&
 	start_p4d
 '
 
-- 
2.21.0.499.g4d310c7a8e.dirty

[PATCH 09/11] git p4 test: simplify timeout handling

[SZEDER Gábor]

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

diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
index b5cb5075c2..c18f85082f 100644
--- a/t/lib-git-p4.sh
+++ b/t/lib-git-p4.sh
@@ -44,15 +44,6 @@ native_path () {
 	echo "$path"
 }
 
-# On Solaris the 'date +%s' function is not supported and therefore we
-# need this replacement.
-# Attention: This function is not safe again against time offset updates
-# at runtime (e.g. via NTP). The 'clock_gettime(CLOCK_MONOTONIC)'
-# function could fix that but it is not in Python until 3.3.
-time_in_seconds () {
-	(cd / && "$PYTHON_PATH" -c 'import time; print(int(time.time()))')
-}
-
 test_set_port P4DPORT
 
 P4PORT=localhost:$P4DPORT
@@ -105,15 +96,16 @@ start_p4d () {
 	# will be caught with the "kill -0" check below.
 	i=${P4D_START_PATIENCE:-300}
 
-	timeout=$(($(time_in_seconds) + $P4D_TIMEOUT))
+	nr_tries_left=$P4D_TIMEOUT
 	while true
 	do
-		if test $(time_in_seconds) -gt $timeout
+		if test $nr_tries_left -eq 0
 		then
 			kill -9 $p4d_pid
 			exit 1
 		fi
 		sleep 1
+		nr_tries_left=$(($nr_tries_left - 1))
 	done &
 	watchdog_pid=$!
 
@@ -167,10 +159,11 @@ p4_add_job () {
 }
 
 retry_until_success () {
-	timeout=$(($(time_in_seconds) + $RETRY_TIMEOUT))
-	until "$@" 2>/dev/null || test $(time_in_seconds) -gt $timeout
+	nr_tries_left=$RETRY_TIMEOUT
+	until "$@" 2>/dev/null || test $nr_tries_left -eq 0
 	do
 		sleep 1
+		nr_tries_left=$(($nr_tries_left - 1))
 	done
 }
 
-- 
2.21.0.499.g4d310c7a8e.dirty

[PATCH 10/11] git p4 test: disable '-x' tracing in the p4d watchdog loop

[SZEDER Gábor]

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

diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
index c18f85082f..547b9f88e1 100644
--- a/t/lib-git-p4.sh
+++ b/t/lib-git-p4.sh
@@ -106,7 +106,7 @@ start_p4d () {
 		fi
 		sleep 1
 		nr_tries_left=$(($nr_tries_left - 1))
-	done &
+	done 2>/dev/null 4>&2 &
 	watchdog_pid=$!
 
 	ready=
-- 
2.21.0.499.g4d310c7a8e.dirty

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

[SZEDER Gábor]

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

diff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh
index b70e81c3cd..c1446f26ab 100755
--- a/t/t9811-git-p4-label-import.sh
+++ b/t/t9811-git-p4-label-import.sh
@@ -191,7 +191,7 @@ test_expect_success 'tag that cannot be exported' '
 	(
 		cd "$cli" &&
 		p4 sync ... &&
-		!(p4 labels | grep GIT_TAG_ON_A_BRANCH)
+		! p4 labels | grep GIT_TAG_ON_A_BRANCH
 	)
 '
 
-- 
2.21.0.499.g4d310c7a8e.dirty

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

[Junio C Hamano]

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

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

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

> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 8665b0a9b6..db3875d1e4 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -631,7 +631,10 @@ die () {
>  
>  GIT_EXIT_OK=
>  trap 'die' EXIT
> -trap 'exit $?' INT TERM HUP
> +# Disable '-x' tracing, because with some shells, notably dash, it
> +# prevents running the cleanup commands when a test script run with
> +# '--verbose-log -x' is interrupted.
> +trap '{ code=$?; set +x; } 2>/dev/null; exit $code' INT TERM HUP
>  
>  # The user-facing functions are loaded from a separate file so that
>  # test_perf subshells can have them too

Thanks.

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

[Junio C Hamano]

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

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

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.

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

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.

Re: [PATCH 03/11] test-lib: introduce 'test_atexit'

[Junio C Hamano]

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

> +test_atexit () {
> +	# We cannot detect when we are in a subshell in general, but by
> +	# doing so on Bash is better than nothing (the test will
> +	# silently pass on other shells).
> +	test "${BASH_SUBSHELL-0}" = 0 ||
> +	error "bug in test script: test_atexit does nothing in a subshell"
> +	test_atexit_cleanup="{ $*
> +		} && (exit \"\$eval_ret\"); eval_ret=\$?; $test_atexit_cleanup"
> +}

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?

>  # Most tests can use the created repository, but some may need to create more.
>  # Usage: test_create_repo <directory>
>  test_create_repo () {
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index db3875d1e4..b35881696f 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -620,6 +620,10 @@ test_external_has_tap=0
>  
>  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
>  		exit $code
> @@ -1045,9 +1049,28 @@ write_junit_xml_testcase () {
>  	junit_have_testcase=t
>  }
>  
> +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.

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.

> +	# 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

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.

> +	setup_malloc_check
> +	test_eval_ "$test_atexit_cleanup"
> +	test_atexit_cleanup=:
> +	teardown_malloc_check
> +}
> +
>  test_done () {
>  	GIT_EXIT_OK=t
>  
> +	# 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
>  		test -n "$junit_have_testcase" || {

Re: [PATCH 05/11] tests: use 'test_atexit' to stop httpd

[Junio C Hamano]

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

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

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

> @@ -176,7 +175,7 @@ prepare_httpd() {
>  start_httpd() {
>  	prepare_httpd >&3 2>&4
>  
> -	trap 'code=$?; stop_httpd; (exit $code); die' EXIT
> +	test_atexit stop_httpd
>  
>  	"$LIB_HTTPD_PATH" -d "$HTTPD_ROOT_PATH" \
>  		-f "$TEST_PATH/apache.conf" $HTTPD_PARA \
> @@ -184,15 +183,12 @@ start_httpd() {
>  		>&3 2>&4
>  	if test $? -ne 0
>  	then
> -		trap 'die' EXIT
>  		cat "$HTTPD_ROOT_PATH"/error.log >&4 2>/dev/null
>  		test_skip_or_die $GIT_TEST_HTTPD "web server setup failed"
>  	fi
>  }
>  
>  stop_httpd() {
> -	trap 'die' EXIT
> -
>  	"$LIB_HTTPD_PATH" -d "$HTTPD_ROOT_PATH" \
>  		-f "$TEST_PATH/apache.conf" $HTTPD_PARA -k stop
>  }

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

Re: [PATCH 05/11] tests: use 'test_atexit' to stop httpd

[Junio C Hamano]

Junio C Hamano <gitster@pobox.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?

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.

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

[SZEDER Gábor]

On Thu, Mar 14, 2019 at 11:18:41AM +0900, Junio C Hamano wrote:
> SZEDER Gábor <szeder.dev@gmail.com> writes:
> 
> > 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.
> 
> 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?

Yes.

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

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

>  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).
> 
> > diff --git a/t/test-lib.sh b/t/test-lib.sh
> > index 8665b0a9b6..db3875d1e4 100644
> > --- a/t/test-lib.sh
> > +++ b/t/test-lib.sh
> > @@ -631,7 +631,10 @@ die () {
> >  
> >  GIT_EXIT_OK=
> >  trap 'die' EXIT
> > -trap 'exit $?' INT TERM HUP
> > +# Disable '-x' tracing, because with some shells, notably dash, it
> > +# prevents running the cleanup commands when a test script run with
> > +# '--verbose-log -x' is interrupted.
> > +trap '{ code=$?; set +x; } 2>/dev/null; exit $code' INT TERM HUP
> >  
> >  # The user-facing functions are loaded from a separate file so that
> >  # test_perf subshells can have them too
> 
> Thanks.

Re: [PATCH 05/11] tests: use 'test_atexit' to stop httpd

[SZEDER Gábor]

On Thu, Mar 14, 2019 at 12:28:35PM +0900, Junio C Hamano wrote:
> 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 ...
> 
> > @@ -176,7 +175,7 @@ prepare_httpd() {
> >  start_httpd() {
> >  	prepare_httpd >&3 2>&4
> >  
> > -	trap 'code=$?; stop_httpd; (exit $code); die' EXIT
> > +	test_atexit stop_httpd
> >  
> >  	"$LIB_HTTPD_PATH" -d "$HTTPD_ROOT_PATH" \
> >  		-f "$TEST_PATH/apache.conf" $HTTPD_PARA \
> > @@ -184,15 +183,12 @@ start_httpd() {
> >  		>&3 2>&4
> >  	if test $? -ne 0
> >  	then
> > -		trap 'die' EXIT
> >  		cat "$HTTPD_ROOT_PATH"/error.log >&4 2>/dev/null
> >  		test_skip_or_die $GIT_TEST_HTTPD "web server setup failed"
> >  	fi
> >  }
> >  
> >  stop_httpd() {
> > -	trap 'die' EXIT
> > -
> >  	"$LIB_HTTPD_PATH" -d "$HTTPD_ROOT_PATH" \
> >  		-f "$TEST_PATH/apache.conf" $HTTPD_PARA -k stop
> >  }
> 
> ... 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.

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.

Re: [PATCH 03/11] test-lib: introduce 'test_atexit'

[SZEDER Gábor]

On Thu, Mar 14, 2019 at 12:21:00PM +0900, Junio C Hamano wrote:
> SZEDER Gábor <szeder.dev@gmail.com> writes:
> 
> > +test_atexit () {
> > +	# We cannot detect when we are in a subshell in general, but by
> > +	# doing so on Bash is better than nothing (the test will
> > +	# silently pass on other shells).
> > +	test "${BASH_SUBSHELL-0}" = 0 ||
> > +	error "bug in test script: test_atexit does nothing in a subshell"
> > +	test_atexit_cleanup="{ $*
> > +		} && (exit \"\$eval_ret\"); eval_ret=\$?; $test_atexit_cleanup"
> > +}
> 
> 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?

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

> >  # Most tests can use the created repository, but some may need to create more.
> >  # Usage: test_create_repo <directory>
> >  test_create_repo () {
> > diff --git a/t/test-lib.sh b/t/test-lib.sh
> > index db3875d1e4..b35881696f 100644
> > --- a/t/test-lib.sh
> > +++ b/t/test-lib.sh
> > @@ -620,6 +620,10 @@ test_external_has_tap=0
> >  
> >  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
> >  		exit $code
> > @@ -1045,9 +1049,28 @@ write_junit_xml_testcase () {
> >  	junit_have_testcase=t
> >  }
> >  
> > +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.
> 
> 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.
> 
> > +	# 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
> 
> I think test_when_finished uses a special value in $test_cleanup in
> a similar way

That's right.

> but it even skips when there is no point doing the
> test_eval_ of the "accumulated" scriptlet when it is empty.

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.

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

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

> > +	setup_malloc_check
> > +	test_eval_ "$test_atexit_cleanup"
> > +	test_atexit_cleanup=:
> > +	teardown_malloc_check
> > +}
> > +
> >  test_done () {
> >  	GIT_EXIT_OK=t
> >  
> > +	# 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
> >  		test -n "$junit_have_testcase" || {

Re: [PATCH 03/11] test-lib: introduce 'test_atexit'

[Junio C Hamano]

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

>> but it even skips when there is no point doing the
>> test_eval_ of the "accumulated" scriptlet when it is empty.
>
> 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.

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:

> +	# 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

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.