{"thread":{"id":"50729","subject":"[PATCH 00/11] tests: introduce 'test_atexit'","startedAt":"2019-03-13T12:24:44Z","lastAt":"2019-03-18T01:50:31Z","messageCount":21,"participants":["SZEDER Gábor","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":11},"messages":[{"id":"371353","messageId":"20190313122419.2210-1-szeder.dev@gmail.com","threadId":"50729","inReplyTo":null,"subject":"[PATCH 00/11] tests: introduce 'test_atexit'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-03-13T12:24:08Z","receivedAt":"2019-03-13T12:24:44Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Introduce the 'test_atexit' helper function to run cleanup commands\nunconditionally at the end of a test script, mainly to reliably stop\ndaemon processes.  'test_atexit' provides a shorter, simpler, more\nconsistent, and more robust way to accomplish that compared to the\ncurrent situation where tests and test libraries manually register\ncleanup commands on trap on EXIT.\n\n\nDscho had the first stab at 'test_atexit' as part of an early version\nof his Azure Pipelines CI patch series:\n\n  https://public-inbox.org/git/12d6137f8d2ccc2041bed8d56d88a09b1db0fd77.1539598316.git.gitgitgadget@gmail.com/\n\nbut he dropped those three patches from later versions, and we agreed\nthat I take them over.\n\n\n01/11 test-lib: fix interrupt handling with 'dash' and '--verbose-log -x'\n\n  This was a fun puzzle to piece together...\n\n02/11 t/lib-git-daemon: make sure to kill the 'git-daemon' process\n\n  I'm not sure whether this is the right thing to do, and have the\n  nagging feeling that the issue this patch fixes/works around might\n  be a symptom of something more fundamental, but I didn't look into\n  how the main git \"wrapper\" and dashed external commands interact.\n\n03/11 test-lib: introduce 'test_atexit'\n04/11 git-daemon: use 'test_atexit` to stop 'git-daemon'\n05/11 tests: use 'test_atexit' to stop httpd\n06/11 t0301-credential-cache: use 'test_atexit' to stop the credentials helper\n07/11 git p4 test: use 'test_atexit' to kill p4d and the watchdog process\n\n  Introduce 'test_atexit' and use it to stop the various daemons\n  involved in our tests.\n\n08/11 git p4 test: clean up the p4d cleanup functions\n09/11 git p4 test: simplify timeout handling\n10/11 git p4 test: disable '-x' tracing in the p4d watchdog loop\n\n  Various cleanups to 't/lib-git-p4.sh'.\n\n11/11 t9811-git-p4-label-import: fix pipeline negation\n\n  A little while-at-it portability fix that I happened to stumble\n  upon.\n\n\nThe range-diff below is primarily for Dscho's benefit (assuming he\nstill remembers his old patches... :)  It was generated by comparing\nthis patch series to his three 'test_atexit' patches cherry-picked on\ntop of v2.21.0.  My modifications to his patches are mainly\ndocumentation updates and a few minor fixes.\n\n\nJohannes Schindelin (3):\n  test-lib: introduce 'test_atexit'\n  git-daemon: use 'test_atexit` to stop 'git-daemon'\n  git p4 test: use 'test_atexit' to kill p4d and the watchdog process\n\nSZEDER Gábor (8):\n  test-lib: fix interrupt handling with 'dash' and '--verbose-log -x'\n  t/lib-git-daemon: make sure to kill the 'git-daemon' process\n  tests: use 'test_atexit' to stop httpd\n  t0301-credential-cache: use 'test_atexit' to stop the credentials\n    helper\n  git p4 test: clean up the p4d cleanup functions\n  git p4 test: simplify timeout handling\n  git p4 test: disable '-x' tracing in the p4d watchdog loop\n  t9811-git-p4-label-import: fix pipeline negation\n\n t/README                                      | 20 ++++++\n t/interop/i5500-git-daemon.sh                 |  1 -\n t/lib-git-daemon.sh                           | 20 ++++--\n t/lib-git-p4.sh                               | 65 +++++++------------\n t/lib-git-svn.sh                              |  5 --\n t/lib-httpd.sh                                |  6 +-\n t/t0000-basic.sh                              | 18 +++++\n t/t0301-credential-cache.sh                   |  7 +-\n t/t0410-partial-clone.sh                      |  2 -\n t/t5500-fetch-pack.sh                         |  3 -\n t/t5510-fetch.sh                              |  2 -\n t/t5537-fetch-shallow.sh                      |  2 -\n t/t5539-fetch-http-shallow.sh                 |  1 -\n t/t5540-http-push-webdav.sh                   |  2 -\n t/t5541-http-push-smart.sh                    |  1 -\n t/t5542-push-http-shallow.sh                  |  1 -\n t/t5545-push-options.sh                       |  2 -\n t/t5550-http-fetch-dumb.sh                    |  1 -\n t/t5551-http-fetch-smart.sh                   |  1 -\n t/t5561-http-backend.sh                       |  1 -\n t/t5570-git-daemon.sh                         |  1 -\n t/t5581-http-curl-verbose.sh                  |  2 -\n t/t5601-clone.sh                              |  2 -\n t/t5616-partial-clone.sh                      |  2 -\n t/t5700-protocol-v1.sh                        |  2 -\n t/t5702-protocol-v2.sh                        |  2 -\n t/t5703-upload-pack-ref-in-want.sh            |  2 -\n t/t5812-proto-disable-http.sh                 |  1 -\n t/t9115-git-svn-dcommit-funky-renames.sh      |  2 -\n t/t9118-git-svn-funky-branch-names.sh         |  2 -\n t/t9120-git-svn-clone-with-percent-escapes.sh |  2 -\n t/t9142-git-svn-shallow-clone.sh              |  2 -\n t/t9800-git-p4-basic.sh                       |  4 --\n t/t9801-git-p4-branch.sh                      |  8 +--\n t/t9802-git-p4-filetype.sh                    |  4 --\n t/t9803-git-p4-shell-metachars.sh             |  4 --\n t/t9804-git-p4-label.sh                       |  4 --\n t/t9805-git-p4-skip-submit-edit.sh            |  4 --\n t/t9806-git-p4-options.sh                     |  5 --\n t/t9807-git-p4-submit.sh                      |  4 --\n t/t9808-git-p4-chdir.sh                       |  4 --\n t/t9809-git-p4-client-view.sh                 |  4 --\n t/t9810-git-p4-rcs.sh                         |  4 --\n t/t9811-git-p4-label-import.sh                |  7 +-\n t/t9812-git-p4-wildcards.sh                   |  4 --\n t/t9813-git-p4-preserve-users.sh              |  4 --\n t/t9814-git-p4-rename.sh                      |  4 --\n t/t9815-git-p4-submit-fail.sh                 |  4 --\n t/t9816-git-p4-locked.sh                      |  4 --\n t/t9817-git-p4-exclude.sh                     |  4 --\n t/t9818-git-p4-block.sh                       |  4 --\n t/t9819-git-p4-case-folding.sh                |  4 --\n t/t9820-git-p4-editor-handling.sh             |  4 --\n t/t9821-git-p4-path-variations.sh             |  4 --\n t/t9822-git-p4-path-encoding.sh               |  4 --\n t/t9823-git-p4-mock-lfs.sh                    |  4 --\n t/t9824-git-p4-git-lfs.sh                     |  4 --\n t/t9825-git-p4-handle-utf16-without-bom.sh    |  4 --\n t/t9826-git-p4-keep-empty-commits.sh          |  4 --\n t/t9827-git-p4-change-filetype.sh             |  4 --\n t/t9828-git-p4-map-user.sh                    |  4 --\n t/t9829-git-p4-jobs.sh                        |  4 --\n t/t9830-git-p4-symlink-dir.sh                 |  4 --\n t/t9831-git-p4-triggers.sh                    |  4 --\n t/t9832-unshelve.sh                           |  3 -\n t/t9833-errors.sh                             |  5 --\n t/test-lib-functions.sh                       | 28 ++++++++\n t/test-lib.sh                                 | 28 +++++++-\n 68 files changed, 136 insertions(+), 247 deletions(-)\n\nRange-diff:\n -:  ---------- >  1:  2a0aeb32f6 test-lib: fix interrupt handling with 'dash' and '--verbose-log -x'\n -:  ---------- >  2:  1dfab6cf94 t/lib-git-daemon: make sure to kill the 'git-daemon' process\n 1:  b4769ffa31 !  3:  c9c3efcc7a tests: introduce `test_atexit`\n    @@ -1,22 +1,83 @@\n     Author: Johannes Schindelin <johannes.schindelin@gmx.de>\n     \n    -    tests: introduce `test_atexit`\n    +    test-lib: introduce 'test_atexit'\n     \n    -    When running the p4 daemon or `git daemon`, we want to kill it at the\n    -    end of the test script.\n    +    When running Apache, 'git daemon', or p4d, we want to kill them at the\n    +    end of the test script, otherwise a leftover daemon process will keep\n    +    its port open indefinitely, and thus will interfere with subsequent\n    +    executions of the same test script.\n     \n    -    So far, we do this \"manually\".\n    +    So far, we stop these daemon processes \"manually\", i.e.:\n     \n    -    However, in the next few commits we want to teach the test suite to\n    -    optionally re-run scripts with different options, therefore we will have\n    -    to have a consistent way to stop daemons.\n    +      - by registering functions or commands in the trap on EXIT to stop\n    +        the daemon while preserving the last seen exit code before the\n    +        trap (to deal with a failure when run with '--immediate' or with\n    +        interrupts by ctrl-C),\n     \n    -    Let's introduce `test_atexit`, which is loosely modeled after\n    -    `test_when_finished` (but has a broader scope: rather than running the\n    +      - and by invoking these functions/commands last thing before\n    +        'test_done' (and sometimes restoring the test framework's default\n    +        trap on EXIT, to prevent the daemons from being killed twice).\n    +\n    +    On one hand, we do this inconsistently, e.g. 'git p4' tests invoke\n    +    different functions in the trap on EXIT and in the last test before\n    +    'test_done', and they neither restore the test framework's default trap\n    +    on EXIT nor preserve the last seen exit code.  On the other hand, this\n    +    is error prone, because, as shown in a previous patch in this series,\n    +    any output from the cleanup commands in the trap on EXIT can prevent a\n    +    proper cleanup when a test script run with '--verbose-log' and certain\n    +    shells, notably 'dash', is interrupted.\n    +\n    +    Let's introduce 'test_atexit', which is loosely modeled after\n    +    'test_when_finished', but has a broader scope: rather than running the\n         commands after the current test case, run them when the test script\n    -    finishes, and also run them when the `--immediate` option is in effect).\n    +    finishes, and also run them when the test is interrupted, or exits\n    +    early in case of a failure while the '--immediate' option is in\n    +    effect.\n    +\n    +    When running the cleanup commands at the end of a successful test,\n    +    then they will be run in 'test_done' before it removes the trash\n    +    directory, i.e. the cleanup commands will still be able to access any\n    +    pidfiles or socket files in there.  When running the cleanup commands\n    +    after an interrupt or failure with '--immediate', then they will be\n    +    run in the trap on EXIT.  In both cases they will be run in\n    +    'test_eval_', i.e. both standard error and output of all cleanup\n    +    commands will go where they should according to the '-v' or\n    +    '--verbose-log' options, and thus won't cause any troubles when\n    +    interrupting a test script run with '--verbose-log'.\n     \n         Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n    +    Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n    +\n    + diff --git a/t/README b/t/README\n    + --- a/t/README\n    + +++ b/t/README\n    +@@\n    + \t\t...\n    + \t'\n    + \n    ++ - test_atexit <script>\n    ++\n    ++   Prepend <script> to a list of commands to run unconditionally to\n    ++   clean up before the test script exits, e.g. to stop a daemon:\n    ++\n    ++\ttest_expect_success 'test git daemon' '\n    ++\t\tgit daemon &\n    ++\t\tdaemon_pid=$! &&\n    ++\t\ttest_atexit 'kill $daemon_pid' &&\n    ++\t\thello world\n    ++\t'\n    ++\n    ++   The commands will be executed before the trash directory is removed,\n    ++   i.e. the atexit commands will still be able to access any pidfiles or\n    ++   socket files.\n    ++\n    ++   Note that these commands will be run even when a test script run\n    ++   with '--immediate' fails.  Be careful with your atexit commands to\n    ++   minimize any changes to the failed state.\n    ++\n    +  - test_write_lines <lines>\n    + \n    +    Write <lines> on standard output, one line per argument.\n     \n      diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\n      --- a/t/t0000-basic.sh\n    @@ -38,7 +99,7 @@\n     +\t'\n     +\ttest_done\n     +\tEOF\n    -+\ttest_path_exists dont-clean-atexit &&\n    ++\ttest_path_is_file dont-clean-atexit &&\n     +\ttest_path_is_missing clean-atexit &&\n     +\ttest_path_is_missing also-clean-atexit\n     +\"\n    @@ -60,9 +121,17 @@\n     +#\ttest_expect_success 'test git daemon' '\n     +#\t\tgit daemon &\n     +#\t\tdaemon_pid=$! &&\n    -+#\t\ttest_atexit \"kill $daemon_pid\" &&\n    ++#\t\ttest_atexit 'kill $daemon_pid' &&\n     +#\t\thello world\n     +#\t'\n    ++#\n    ++# The commands will be executed before the trash directory is removed,\n    ++# i.e. the atexit commands will still be able to access any pidfiles or\n    ++# socket files.\n    ++#\n    ++# Note that these commands will be run even when a test script run\n    ++# with '--immediate' fails.  Be careful with your atexit commands to\n    ++# minimize any changes to the failed state.\n     +\n     +test_atexit () {\n     +\t# We cannot detect when we are in a subshell in general, but by\n    @@ -73,15 +142,6 @@\n     +\ttest_atexit_cleanup=\"{ $*\n     +\t\t} && (exit \\\"\\$eval_ret\\\"); eval_ret=\\$?; $test_atexit_cleanup\"\n     +}\n    -+\n    -+test_atexit_handler () {\n    -+\ttest : != \"$test_atexit_cleanup\" || return 0\n    -+\n    -+\tsetup_malloc_check\n    -+\ttest_eval_ \"$test_atexit_cleanup\"\n    -+\ttest_atexit_cleanup=:\n    -+\tteardown_malloc_check\n    -+}\n     +\n      # Most tests can use the created repository, but some may need to create more.\n      # Usage: test_create_repo <directory>\n    @@ -94,6 +154,9 @@\n      \n      die () {\n      \tcode=$?\n    ++\t# This is responsible for running the atexit commands even when a\n    ++\t# test script run with '--immediate' fails, or when the user hits\n    ++\t# ctrl-C, i.e. when 'test_done' is not invoked at all.\n     +\ttest_atexit_handler || code=$?\n      \tif test -n \"$GIT_EXIT_OK\"\n      \tthen\n    @@ -103,10 +166,26 @@\n      }\n      \n     +test_atexit_cleanup=:\n    ++test_atexit_handler () {\n    ++\t# In a succeeding test script 'test_atexit_handler' is invoked\n    ++\t# twice: first from 'test_done', then from 'die' in the trap on\n    ++\t# EXIT.\n    ++\t# This condition and resetting 'test_atexit_cleanup' below makes\n    ++\t# sure that the registered cleanup commands are run only once.\n    ++\ttest : != \"$test_atexit_cleanup\" || return 0\n    ++\n    ++\tsetup_malloc_check\n    ++\ttest_eval_ \"$test_atexit_cleanup\"\n    ++\ttest_atexit_cleanup=:\n    ++\tteardown_malloc_check\n    ++}\n    ++\n      test_done () {\n      \tGIT_EXIT_OK=t\n      \n    -+\ttest -n \"$immediate\" || test_atexit_handler\n    ++\t# Run the atexit commands _before_ the trash directory is\n    ++\t# removed, so the commands can access pidfiles and socket files.\n    ++\ttest_atexit_handler\n     +\n      \tif test -n \"$write_junit_xml\" && test -n \"$junit_xml_path\"\n      \tthen\n 2:  39ad1480c5 !  4:  52dfb87ae3 git-daemon: use `test_atexit` in the tests\n    @@ -1,12 +1,18 @@\n     Author: Johannes Schindelin <johannes.schindelin@gmx.de>\n     \n    -    git-daemon: use `test_atexit` in the tests\n    +    git-daemon: use 'test_atexit` to stop 'git-daemon'\n     \n    -    This makes use of the just-introduced consistent way to specify that a\n    -    long-running process needs to be terminated at the end of a test script\n    -    run.\n    +    Use 'test_atexit' to run cleanup commands to stop 'git-daemon' at the\n    +    end of the test script or upon interrupt or failure, as it is shorter,\n    +    simpler, and more robust than registering such cleanup commands in the\n    +    trap on EXIT in the test scripts.\n    +\n    +    Note that in 't5570-git-daemon.sh' the daemon is stopped and then\n    +    re-started in the middle of the test script; take care that the\n    +    cleanup functions to stop the daemon are only registered once.\n     \n         Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n    +    Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n     \n      diff --git a/t/interop/i5500-git-daemon.sh b/t/interop/i5500-git-daemon.sh\n      --- a/t/interop/i5500-git-daemon.sh\n    @@ -30,14 +36,46 @@\n      \n      test_tristate GIT_TEST_GIT_DAEMON\n     @@\n    + GIT_DAEMON_HOST_PORT=127.0.0.1:$LIB_GIT_DAEMON_PORT\n    + GIT_DAEMON_URL=git://$GIT_DAEMON_HOST_PORT\n    + \n    ++registered_stop_git_daemon_atexit_handler=\n    + start_git_daemon() {\n    + \tif test -n \"$GIT_DAEMON_PID\"\n    + \tthen\n    +@@\n      \n      \tmkdir -p \"$GIT_DAEMON_DOCUMENT_ROOT_PATH\"\n      \n     -\ttrap 'code=$?; stop_git_daemon; (exit $code); die' EXIT\n    -+\ttest_atexit 'stop_git_daemon'\n    ++\t# One of the test scripts stops and then re-starts 'git daemon'.\n    ++\t# Don't register and then run the same atexit handlers several times.\n    ++\tif test -z \"$registered_stop_git_daemon_atexit_handler\"\n    ++\tthen\n    ++\t\ttest_atexit 'stop_git_daemon'\n    ++\t\tregistered_stop_git_daemon_atexit_handler=AlreadyDone\n    ++\tfi\n      \n      \tsay >&3 \"Starting git daemon ...\"\n      \tmkfifo git_daemon_output\n    +@@\n    + \tthen\n    + \t\tkill \"$GIT_DAEMON_PID\"\n    + \t\twait \"$GIT_DAEMON_PID\"\n    +-\t\ttrap 'die' EXIT\n    ++\t\tunset GIT_DAEMON_PID\n    + \t\ttest_skip_or_die $GIT_TEST_GIT_DAEMON \\\n    + \t\t\t\"git daemon failed to start\"\n    + \tfi\n    +@@\n    + \t\treturn\n    + \tfi\n    + \n    +-\ttrap 'die' EXIT\n    +-\n    + \t# kill git-daemon child of git\n    + \tsay >&3 \"Stopping git daemon ...\"\n    + \tkill \"$GIT_DAEMON_PID\"\n     \n      diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh\n      --- a/t/t5570-git-daemon.sh\n -:  ---------- >  5:  32cde9101e tests: use 'test_atexit' to stop httpd\n -:  ---------- >  6:  fafad7f83d t0301-credential-cache: use 'test_atexit' to stop the credentials helper\n 3:  d938232a92 !  7:  5a3cdbae15 git-p4: use `test_atexit` to kill the daemon\n    @@ -1,60 +1,53 @@\n     Author: Johannes Schindelin <johannes.schindelin@gmx.de>\n     \n    -    git-p4: use `test_atexit` to kill the daemon\n    +    git p4 test: use 'test_atexit' to kill p4d and the watchdog process\n     \n    -    This should be more reliable than the current method, and prepares the\n    -    test suite for a consistent way to clean up before re-running the tests\n    -    with different options.\n    +    Use 'test_atexit' to run cleanup commands to stop 'p4d' at the end of\n    +    the test script or upon interrupt or failure, as it is shorter,\n    +    simpler, and more robust than registering such cleanup commands in the\n    +    trap on EXIT in the test scripts.\n    +\n    +    Note that one of the test scripts, 't9801-git-p4-branch.sh', stops and\n    +    then re-starts 'p4d' twice in the middle of the script; take care that\n    +    the cleanup functions to stop 'p4d' are only registered once.\n    +\n    +    Note also that 'git p4' tests invoke different functions in the trap\n    +    on EXIT ('cleanup') and in the last test before 'test_done'\n    +    ('kill_p4d').  Register both of these functions with 'test_atexit' for\n    +    now, and a a later patch in this series will then clean up the\n    +    redundancy.\n     \n         Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n    +    Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n     \n      diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh\n      --- a/t/lib-git-p4.sh\n      +++ b/t/lib-git-p4.sh\n     @@\n    - git=\"$TRASH_DIRECTORY/git\"\n    - pidfile=\"$TRASH_DIRECTORY/p4d.pid\"\n    - \n    --# Sometimes \"prove\" seems to hang on exit because p4d is still running\n    --cleanup () {\n    --\tif test -f \"$pidfile\"\n    --\tthen\n    --\t\tkill -9 $(cat \"$pidfile\") 2>/dev/null && exit 255\n    --\tfi\n    --}\n    --trap cleanup EXIT\n    --\n    - # git p4 submit generates a temp file, which will\n    - # not get cleaned up if the submission fails.  Don't\n    - # clutter up /tmp on the test machine.\n    -@@\n    - \t\t# p4d failed to start\n    - \t\treturn 1\n    + \t\tkill -9 $(cat \"$pidfile\") 2>/dev/null && exit 255\n      \tfi\n    -+\ttest_atexit kill_p4d\n    - \n    - \t# build a p4 user so author@example.com has an entry\n    - \tp4_add_user author\n    -\n    - diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\n    - --- a/t/t0000-basic.sh\n    - +++ b/t/t0000-basic.sh\n    -@@\n    - \t)\n      }\n    +-trap cleanup EXIT\n      \n    -+cat >/dev/null <<\\DDD\n    - test_expect_success 'pretend we have a fully passing test suite' \"\n    - \trun_sub_test_lib_test full-pass '3 passing tests' <<-\\\\EOF &&\n    - \tfor i in 1 2 3\n    + # git p4 submit generates a temp file, which will\n    + # not get cleaned up if the submission fails.  Don't\n     @@\n    - \t> 1..2\n    - \tEOF\n    - \"\n    -+DDD\n    - \n    - test_expect_success 'test_atexit is run' \"\n    - \ttest_must_fail run_sub_test_lib_test \\\n    + TMPDIR=\"$TRASH_DIRECTORY\"\n    + export TMPDIR\n    + \n    ++registered_stop_p4d_atexit_handler=\n    + start_p4d () {\n    ++\t# One of the test scripts stops and then re-starts p4d.\n    ++\t# Don't register and then run the same atexit handlers several times.\n    ++\tif test -z \"$registered_stop_p4d_atexit_handler\"\n    ++\tthen\n    ++\t\ttest_atexit 'kill_p4d; cleanup'\n    ++\t\tregistered_stop_p4d_atexit_handler=AlreadyDone\n    ++\tfi\n    ++\n    + \tmkdir -p \"$db\" \"$cli\" \"$git\" &&\n    + \trm -f \"$pidfile\" &&\n    + \t(\n     \n      diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh\n      --- a/t/t9800-git-p4-basic.sh\n -:  ---------- >  8:  ad083c6a74 git p4 test: clean up the p4d cleanup functions\n -:  ---------- >  9:  05ecee5c13 git p4 test: simplify timeout handling\n -:  ---------- > 10:  7626c0df88 git p4 test: disable '-x' tracing in the p4d watchdog loop\n -:  ---------- > 11:  53082f8ce1 t9811-git-p4-label-import: fix pipeline negation\n-- \n2.21.0.499.g4d310c7a8e.dirty\n\n"},{"id":"371354","messageId":"20190313122419.2210-2-szeder.dev@gmail.com","threadId":"50729","inReplyTo":"20190313122419.2210-1-szeder.dev@gmail.com","subject":"[PATCH 01/11] test-lib: fix interrupt handling with 'dash' and '--verbose-log -x'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-03-13T12:24:09Z","receivedAt":"2019-03-13T12:24:45Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"When a test script run with 'dash' and '--verbose-log -x' is\ninterrupted by ctrl-C, SIGTERM, or closing the terminal window, then\nmost of the time the registered EXIT trap actions are not executed.\nThis is an annoying issue with tests involving daemons, because they\nshould run cleanup commands to kill those daemon processes in the trap\non EXIT, but since these cleanup commands are not executed, the\ndaemons are left alive and keep their port open, thus interfering with\nsubsequent execution of the same test script.\n\nThe cause of this issue is the subtle combination of several factors\n(bear with me, or skip over the indented part):\n\n  - Even when the test script is interrupted, the cleanup commands are\n    not run in the trap on INT, TERM, or HUP, but in the trap on EXIT\n    after the trap on the signals invokes 'exit' [1].\n\n  - According to POSIX [2]:\n\n      \"The environment in which the shell executes a trap on EXIT\n      shall be identical to the environment immediately after the last\n      command executed before the trap on EXIT was taken.\"\n\n    Pertinent to the issue at hand is that all open file descriptors\n    and the state of '-x' tracing should be preserved.  All shells\n    I've tried [3] preserve '-x'.  Unfortunately, however:\n\n      - 'dash' doesn't conform to this when it comes to open file\n        descriptors: even when standard output and/or error are\n        redirected somewhere when 'exit' is invoked, anything written\n        to them in the trap on EXIT goes to the script's original\n        stdout and stderr [4].\n\n        We can't dismiss this with a simple \"it doesn't conform to\n        POSIX, so we don't care\", because 'dash' is the default\n        /bin/sh in some of the more popular Linux distros.\n\n      - As far as I can tell, POSIX doesn't explicitly say anything\n        about the environment of trap actions for various signals.\n\n        In practice it seems that most shells behave sensibly and\n        preserve both open file descriptors and the state of '-x'\n        tracing for the traps on INT, TERM, and HUP, including even\n        'dash'.  The exceptions are 'mksh' and 'lksh': they do\n        preserve '-x', but not the open file descriptors.\n\n  - When a test script run with '-x' tracing enabled is interrupted,\n    then it's very likely that the signal arrives mid-test, i.e.:\n\n      - while '-x' tracing is enabled, and, consequently, our trap\n        actions on INT, TERM, HUP, and EXIT will produce trace output\n        as well.\n\n      - while standard output and error are redirected to a log file,\n        to the test script's original standard output and error, or to\n        /dev/null, depending on whether the test script was run with\n        '--verbose-log', '-v', or neither.  According to the above, we\n        can't rely on these redirections still be in effect when\n        running the traps on INT, TERM, HUP, and/or EXIT.\n\n  - When a test script is run with '--verbose-log', then the test\n    script is re-executed with its standard output and error piped\n    into 'tee', in order to send the \"regular\" non-verbose test's\n    output both to the terminal and to the log file.  When the test is\n    interrupted, then the signal interrupts the downstream 'tee' as\n    well.\n\nPutting these together, when a test script run with 'dash' and\n'--verbose-log -x' is interrupted, then 'dash' tries to write the\ntrace output from the EXIT trap to the script's original standard\nerror, but it very likely can't, because the 'tee' downstream of the\npipe is interrupted as well.  This causes the shell running the test\nscript to die because of SIGPIPE, without running any of the commands\nin the EXIT trap.\n\nDisable '-x' tracing in the trap on INT, TERM, and HUP to avoid this\nissue, as it disables tracing in the chained trap on EXIT as well.\nWrap it in a '{ ... } 2>/dev/null' block, so the trace of the command\ndisabling the tracing doesn't go to standard error either [5].\n\nNote that it's not only '-x' tracing that can be problematic, but any\nshell builtin, e.g. 'echo', that writes to standard output or error in\nthe trap on EXIT, while a test running with 'dash' and '--verbose-log'\n(even without '-x') is interrupted.  As far as I can tell, this is not\nan issue at the moment:\n\n - The cleanup commands to stop the credential-helper, Apache, or\n   'p4d' don't use any such shell builtins.\n\n - stop_git_daemon() does use 'say' and 'error', both wrappers around\n   'echo', but it redirects 'say' to fd 3, i.e. to the log file, and\n   while 'error' does write to standard output, it comes only after\n   the daemon was killed.\n\n - The non-builtin commands that actually stop the daemons ('kill',\n   'apache2 -k stop', 'git credential-cache exit') are silent, so they\n   won't get SIGPIPE before finishing their job.\n\n[1] The trap on EXIT must run cleanup commands, because we want to\n    stop any daemons when a test script run with '--immediate' fails\n    and exits early with error.  By chaining up the trap on signals to\n    the trap on EXIT we can deal with cleanup commands a bit simpler,\n    because the tests involving daemons only have to set a single\n    trap.\n\n[2] http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#trap\n\n[3] The shells I tried: dash, Bash, ksh, ksh93, mksh, lksh, yash,\n    BusyBox sh, FreeBSD /bin/sh, NetBSD /bin/sh.\n\n[4] $ cat trap-output.sh\n    #!/bin/sh\n    trap \"echo output; echo error >&2\" EXIT\n    { exit; } >OUT 2>ERR\n    $ dash ./trap-output.sh\n    output\n    error\n    $ wc -c OUT ERR\n    0 OUT\n    0 ERR\n\n    On a related note, 'ksh', 'ksh93', and BusyBox sh don't conform to\n    the specs in this respect, either.\n\n[5] This '{ set +x; } 2>/dev/null' trick won't help those shells that\n    show trace output for any redirections and don't preserve open\n    file descriptors for the trap on INT, TERM and HUP.  The only such\n    shells I'm aware of are 'mksh' and 'lksh'.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n t/test-lib.sh | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 8665b0a9b6..db3875d1e4 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -631,7 +631,10 @@ die () {\n \n GIT_EXIT_OK=\n trap 'die' EXIT\n-trap 'exit $?' INT TERM HUP\n+# Disable '-x' tracing, because with some shells, notably dash, it\n+# prevents running the cleanup commands when a test script run with\n+# '--verbose-log -x' is interrupted.\n+trap '{ code=$?; set +x; } 2>/dev/null; exit $code' INT TERM HUP\n \n # The user-facing functions are loaded from a separate file so that\n # test_perf subshells can have them too\n-- \n2.21.0.499.g4d310c7a8e.dirty\n\n"},{"id":"371355","messageId":"20190313122419.2210-3-szeder.dev@gmail.com","threadId":"50729","inReplyTo":"20190313122419.2210-1-szeder.dev@gmail.com","subject":"[PATCH 02/11] t/lib-git-daemon: make sure to kill the 'git-daemon' process","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-03-13T12:24:10Z","receivedAt":"2019-03-13T12:24:46Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"After 'start_git_daemon' starts 'git daemon' (note the space in the\nmiddle) in the background, it saves the background process' PID, so\nthe daemon can be stopped at the end of the test script.  However,\n'git-daemon' is not a builtin but a dashed external command, which\nmeans that the dashless 'git daemon' executes the dashed 'git-daemon'\ncommand, and, consequently, the PID recorded is not the PID of the\n\"real\" daemon process, but that of the main 'git' wrapper.  Now, if a\ntest script involving 'git daemon' is interrupted by ctrl-C, then only\nthe main 'git' process is stopped, but the real daemon process tends\nto survive somehow, and keeps on running in the background\nindefinitely, keeping the daemon's port to itself, and thus preventing\nsubsequent runs of the same test script.\n\nWork this around by running 'git daemon' with the '--pidfile=...'\noption to save the PID of the real daemon process, and kill that\nprocess in 'stop_git_daemon' as well.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n t/lib-git-daemon.sh | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh\nindex 79db3b7ae5..6dab8766e7 100644\n--- a/t/lib-git-daemon.sh\n+++ b/t/lib-git-daemon.sh\n@@ -31,6 +31,7 @@ fi\n test_set_port LIB_GIT_DAEMON_PORT\n \n GIT_DAEMON_PID=\n+GIT_DAEMON_PIDFILE=\"$PWD\"/daemon.pid\n GIT_DAEMON_DOCUMENT_ROOT_PATH=\"$PWD\"/repo\n GIT_DAEMON_HOST_PORT=127.0.0.1:$LIB_GIT_DAEMON_PORT\n GIT_DAEMON_URL=git://$GIT_DAEMON_HOST_PORT\n@@ -49,7 +50,7 @@ start_git_daemon() {\n \tmkfifo git_daemon_output\n \t${LIB_GIT_DAEMON_COMMAND:-git daemon} \\\n \t\t--listen=127.0.0.1 --port=\"$LIB_GIT_DAEMON_PORT\" \\\n-\t\t--reuseaddr --verbose \\\n+\t\t--reuseaddr --verbose --pid-file=\"$GIT_DAEMON_PIDFILE\" \\\n \t\t--base-path=\"$GIT_DAEMON_DOCUMENT_ROOT_PATH\" \\\n \t\t\"$@\" \"$GIT_DAEMON_DOCUMENT_ROOT_PATH\" \\\n \t\t>&3 2>git_daemon_output &\n@@ -88,8 +89,9 @@ stop_git_daemon() {\n \tthen\n \t\terror \"git daemon exited with status: $ret\"\n \tfi\n+\tkill \"$(cat \"$GIT_DAEMON_PIDFILE\")\" 2>/dev/null\n \tGIT_DAEMON_PID=\n-\trm -f git_daemon_output\n+\trm -f git_daemon_output \"$GIT_DAEMON_PIDFILE\"\n }\n \n # A stripped-down version of a netcat client, that connects to a \"host:port\"\n-- \n2.21.0.499.g4d310c7a8e.dirty\n\n"},{"id":"371356","messageId":"20190313122419.2210-5-szeder.dev@gmail.com","threadId":"50729","inReplyTo":"20190313122419.2210-1-szeder.dev@gmail.com","subject":"[PATCH 04/11] git-daemon: use 'test_atexit` to stop 'git-daemon'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-03-13T12:24:12Z","receivedAt":"2019-03-13T12:24:48Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nUse 'test_atexit' to run cleanup commands to stop 'git-daemon' at the\nend of the test script or upon interrupt or failure, as it is shorter,\nsimpler, and more robust than registering such cleanup commands in the\ntrap on EXIT in the test scripts.\n\nNote that in 't5570-git-daemon.sh' the daemon is stopped and then\nre-started in the middle of the test script; take care that the\ncleanup functions to stop the daemon are only registered once.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n t/interop/i5500-git-daemon.sh |  1 -\n t/lib-git-daemon.sh           | 14 +++++++++-----\n t/t5570-git-daemon.sh         |  1 -\n 3 files changed, 9 insertions(+), 7 deletions(-)\n\ndiff --git a/t/interop/i5500-git-daemon.sh b/t/interop/i5500-git-daemon.sh\nindex 1daf69420b..4d22e42f84 100755\n--- a/t/interop/i5500-git-daemon.sh\n+++ b/t/interop/i5500-git-daemon.sh\n@@ -37,5 +37,4 @@ test_expect_success \"fetch with $VERSION_B\" '\n \ttest_cmp expect actual\n '\n \n-stop_git_daemon\n test_done\ndiff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh\nindex 6dab8766e7..7b3407134e 100644\n--- a/t/lib-git-daemon.sh\n+++ b/t/lib-git-daemon.sh\n@@ -13,7 +13,6 @@\n #\n #\ttest_expect_success ...\n #\n-#\tstop_git_daemon\n #\ttest_done\n \n test_tristate GIT_TEST_GIT_DAEMON\n@@ -36,6 +35,7 @@ GIT_DAEMON_DOCUMENT_ROOT_PATH=\"$PWD\"/repo\n GIT_DAEMON_HOST_PORT=127.0.0.1:$LIB_GIT_DAEMON_PORT\n GIT_DAEMON_URL=git://$GIT_DAEMON_HOST_PORT\n \n+registered_stop_git_daemon_atexit_handler=\n start_git_daemon() {\n \tif test -n \"$GIT_DAEMON_PID\"\n \tthen\n@@ -44,7 +44,13 @@ start_git_daemon() {\n \n \tmkdir -p \"$GIT_DAEMON_DOCUMENT_ROOT_PATH\"\n \n-\ttrap 'code=$?; stop_git_daemon; (exit $code); die' EXIT\n+\t# One of the test scripts stops and then re-starts 'git daemon'.\n+\t# Don't register and then run the same atexit handlers several times.\n+\tif test -z \"$registered_stop_git_daemon_atexit_handler\"\n+\tthen\n+\t\ttest_atexit 'stop_git_daemon'\n+\t\tregistered_stop_git_daemon_atexit_handler=AlreadyDone\n+\tfi\n \n \tsay >&3 \"Starting git daemon ...\"\n \tmkfifo git_daemon_output\n@@ -66,7 +72,7 @@ start_git_daemon() {\n \tthen\n \t\tkill \"$GIT_DAEMON_PID\"\n \t\twait \"$GIT_DAEMON_PID\"\n-\t\ttrap 'die' EXIT\n+\t\tunset GIT_DAEMON_PID\n \t\ttest_skip_or_die $GIT_TEST_GIT_DAEMON \\\n \t\t\t\"git daemon failed to start\"\n \tfi\n@@ -78,8 +84,6 @@ stop_git_daemon() {\n \t\treturn\n \tfi\n \n-\ttrap 'die' EXIT\n-\n \t# kill git-daemon child of git\n \tsay >&3 \"Stopping git daemon ...\"\n \tkill \"$GIT_DAEMON_PID\"\ndiff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh\nindex 58ee787685..00fc612cac 100755\n--- a/t/t5570-git-daemon.sh\n+++ b/t/t5570-git-daemon.sh\n@@ -198,5 +198,4 @@ test_expect_success FAKENC 'hostname interpolation works after LF-stripping' '\n \ttest_cmp expect actual\n '\n \n-stop_git_daemon\n test_done\n-- \n2.21.0.499.g4d310c7a8e.dirty\n\n"},{"id":"371357","messageId":"20190313122419.2210-4-szeder.dev@gmail.com","threadId":"50729","inReplyTo":"20190313122419.2210-1-szeder.dev@gmail.com","subject":"[PATCH 03/11] test-lib: introduce 'test_atexit'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-03-13T12:24:11Z","receivedAt":"2019-03-13T12:24:49Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nWhen running Apache, 'git daemon', or p4d, we want to kill them at the\nend of the test script, otherwise a leftover daemon process will keep\nits port open indefinitely, and thus will interfere with subsequent\nexecutions of the same test script.\n\nSo far, we stop these daemon processes \"manually\", i.e.:\n\n  - by registering functions or commands in the trap on EXIT to stop\n    the daemon while preserving the last seen exit code before the\n    trap (to deal with a failure when run with '--immediate' or with\n    interrupts by ctrl-C),\n\n  - and by invoking these functions/commands last thing before\n    'test_done' (and sometimes restoring the test framework's default\n    trap on EXIT, to prevent the daemons from being killed twice).\n\nOn one hand, we do this inconsistently, e.g. 'git p4' tests invoke\ndifferent functions in the trap on EXIT and in the last test before\n'test_done', and they neither restore the test framework's default trap\non EXIT nor preserve the last seen exit code.  On the other hand, this\nis error prone, because, as shown in a previous patch in this series,\nany output from the cleanup commands in the trap on EXIT can prevent a\nproper cleanup when a test script run with '--verbose-log' and certain\nshells, notably 'dash', is interrupted.\n\nLet's introduce 'test_atexit', which is loosely modeled after\n'test_when_finished', but has a broader scope: rather than running the\ncommands after the current test case, run them when the test script\nfinishes, and also run them when the test is interrupted, or exits\nearly in case of a failure while the '--immediate' option is in\neffect.\n\nWhen running the cleanup commands at the end of a successful test,\nthen they will be run in 'test_done' before it removes the trash\ndirectory, i.e. the cleanup commands will still be able to access any\npidfiles or socket files in there.  When running the cleanup commands\nafter an interrupt or failure with '--immediate', then they will be\nrun in the trap on EXIT.  In both cases they will be run in\n'test_eval_', i.e. both standard error and output of all cleanup\ncommands will go where they should according to the '-v' or\n'--verbose-log' options, and thus won't cause any troubles when\ninterrupting a test script run with '--verbose-log'.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n t/README                | 20 ++++++++++++++++++++\n t/t0000-basic.sh        | 18 ++++++++++++++++++\n t/test-lib-functions.sh | 28 ++++++++++++++++++++++++++++\n t/test-lib.sh           | 23 +++++++++++++++++++++++\n 4 files changed, 89 insertions(+)\n\ndiff --git a/t/README b/t/README\nindex 886bbec5bc..af510d236a 100644\n--- a/t/README\n+++ b/t/README\n@@ -862,6 +862,26 @@ library for your script to use.\n \t\t...\n \t'\n \n+ - test_atexit <script>\n+\n+   Prepend <script> to a list of commands to run unconditionally to\n+   clean up before the test script exits, e.g. to stop a daemon:\n+\n+\ttest_expect_success 'test git daemon' '\n+\t\tgit daemon &\n+\t\tdaemon_pid=$! &&\n+\t\ttest_atexit 'kill $daemon_pid' &&\n+\t\thello world\n+\t'\n+\n+   The commands will be executed before the trash directory is removed,\n+   i.e. the atexit commands will still be able to access any pidfiles or\n+   socket files.\n+\n+   Note that these commands will be run even when a test script run\n+   with '--immediate' fails.  Be careful with your atexit commands to\n+   minimize any changes to the failed state.\n+\n  - test_write_lines <lines>\n \n    Write <lines> on standard output, one line per argument.\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex b6566003dd..c03054c538 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -825,6 +825,24 @@ test_expect_success 'tests clean up even on failures' \"\n \tEOF\n \"\n \n+test_expect_success 'test_atexit is run' \"\n+\ttest_must_fail run_sub_test_lib_test \\\n+\t\tatexit-cleanup 'Run atexit commands' -i <<-\\\\EOF &&\n+\ttest_expect_success 'tests clean up even after a failure' '\n+\t\t> ../../clean-atexit &&\n+\t\ttest_atexit rm ../../clean-atexit &&\n+\t\t> ../../also-clean-atexit &&\n+\t\ttest_atexit rm ../../also-clean-atexit &&\n+\t\t> ../../dont-clean-atexit &&\n+\t\t(exit 1)\n+\t'\n+\ttest_done\n+\tEOF\n+\ttest_path_is_file dont-clean-atexit &&\n+\ttest_path_is_missing clean-atexit &&\n+\ttest_path_is_missing also-clean-atexit\n+\"\n+\n test_expect_success 'test_oid setup' '\n \ttest_oid_init\n '\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 80402a428f..6a50dba390 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -934,6 +934,34 @@ test_when_finished () {\n \t\t} && (exit \\\"\\$eval_ret\\\"); eval_ret=\\$?; $test_cleanup\"\n }\n \n+# This function can be used to schedule some commands to be run\n+# unconditionally at the end of the test script, e.g. to stop a daemon:\n+#\n+#\ttest_expect_success 'test git daemon' '\n+#\t\tgit daemon &\n+#\t\tdaemon_pid=$! &&\n+#\t\ttest_atexit 'kill $daemon_pid' &&\n+#\t\thello world\n+#\t'\n+#\n+# The commands will be executed before the trash directory is removed,\n+# i.e. the atexit commands will still be able to access any pidfiles or\n+# socket files.\n+#\n+# Note that these commands will be run even when a test script run\n+# with '--immediate' fails.  Be careful with your atexit commands to\n+# minimize any changes to the failed state.\n+\n+test_atexit () {\n+\t# We cannot detect when we are in a subshell in general, but by\n+\t# doing so on Bash is better than nothing (the test will\n+\t# silently pass on other shells).\n+\ttest \"${BASH_SUBSHELL-0}\" = 0 ||\n+\terror \"bug in test script: test_atexit does nothing in a subshell\"\n+\ttest_atexit_cleanup=\"{ $*\n+\t\t} && (exit \\\"\\$eval_ret\\\"); eval_ret=\\$?; $test_atexit_cleanup\"\n+}\n+\n # Most tests can use the created repository, but some may need to create more.\n # Usage: test_create_repo <directory>\n test_create_repo () {\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex db3875d1e4..b35881696f 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -620,6 +620,10 @@ test_external_has_tap=0\n \n die () {\n \tcode=$?\n+\t# This is responsible for running the atexit commands even when a\n+\t# test script run with '--immediate' fails, or when the user hits\n+\t# ctrl-C, i.e. when 'test_done' is not invoked at all.\n+\ttest_atexit_handler || code=$?\n \tif test -n \"$GIT_EXIT_OK\"\n \tthen\n \t\texit $code\n@@ -1045,9 +1049,28 @@ write_junit_xml_testcase () {\n \tjunit_have_testcase=t\n }\n \n+test_atexit_cleanup=:\n+test_atexit_handler () {\n+\t# In a succeeding test script 'test_atexit_handler' is invoked\n+\t# twice: first from 'test_done', then from 'die' in the trap on\n+\t# EXIT.\n+\t# This condition and resetting 'test_atexit_cleanup' below makes\n+\t# sure that the registered cleanup commands are run only once.\n+\ttest : != \"$test_atexit_cleanup\" || return 0\n+\n+\tsetup_malloc_check\n+\ttest_eval_ \"$test_atexit_cleanup\"\n+\ttest_atexit_cleanup=:\n+\tteardown_malloc_check\n+}\n+\n test_done () {\n \tGIT_EXIT_OK=t\n \n+\t# Run the atexit commands _before_ the trash directory is\n+\t# removed, so the commands can access pidfiles and socket files.\n+\ttest_atexit_handler\n+\n \tif test -n \"$write_junit_xml\" && test -n \"$junit_xml_path\"\n \tthen\n \t\ttest -n \"$junit_have_testcase\" || {\n-- \n2.21.0.499.g4d310c7a8e.dirty\n\n"},{"id":"371358","messageId":"20190313122419.2210-6-szeder.dev@gmail.com","threadId":"50729","inReplyTo":"20190313122419.2210-1-szeder.dev@gmail.com","subject":"[PATCH 05/11] tests: use 'test_atexit' to stop httpd","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-03-13T12:24:13Z","receivedAt":"2019-03-13T12:24:52Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Use 'test_atexit' to run cleanup commands to stop httpd at the end of\nthe test script or upon interrupt or failure, as it is shorter,\nsimpler, and more robust than registering such cleanup commands in the\ntrap on EXIT in the test scripts.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n t/lib-git-svn.sh                              | 5 -----\n t/lib-httpd.sh                                | 6 +-----\n t/t0410-partial-clone.sh                      | 2 --\n t/t5500-fetch-pack.sh                         | 3 ---\n t/t5510-fetch.sh                              | 2 --\n t/t5537-fetch-shallow.sh                      | 2 --\n t/t5539-fetch-http-shallow.sh                 | 1 -\n t/t5540-http-push-webdav.sh                   | 2 --\n t/t5541-http-push-smart.sh                    | 1 -\n t/t5542-push-http-shallow.sh                  | 1 -\n t/t5545-push-options.sh                       | 2 --\n t/t5550-http-fetch-dumb.sh                    | 1 -\n t/t5551-http-fetch-smart.sh                   | 1 -\n t/t5561-http-backend.sh                       | 1 -\n t/t5581-http-curl-verbose.sh                  | 2 --\n t/t5601-clone.sh                              | 2 --\n t/t5616-partial-clone.sh                      | 2 --\n t/t5700-protocol-v1.sh                        | 2 --\n t/t5702-protocol-v2.sh                        | 2 --\n t/t5703-upload-pack-ref-in-want.sh            | 2 --\n t/t5812-proto-disable-http.sh                 | 1 -\n t/t9115-git-svn-dcommit-funky-renames.sh      | 2 --\n t/t9118-git-svn-funky-branch-names.sh         | 2 --\n t/t9120-git-svn-clone-with-percent-escapes.sh | 2 --\n t/t9142-git-svn-shallow-clone.sh              | 2 --\n 25 files changed, 1 insertion(+), 50 deletions(-)\n\ndiff --git a/t/lib-git-svn.sh b/t/lib-git-svn.sh\nindex f3b478c307..c1271d6863 100644\n--- a/t/lib-git-svn.sh\n+++ b/t/lib-git-svn.sh\n@@ -76,11 +76,6 @@ maybe_start_httpd () {\n \t\tLIB_HTTPD_SVN=\"$loc\"\n \t\tstart_httpd\n \t\t;;\n-\t*)\n-\t\tstop_httpd () {\n-\t\t\t: noop\n-\t\t}\n-\t\t;;\n \tesac\n }\n \ndiff --git a/t/lib-httpd.sh b/t/lib-httpd.sh\nindex 0dfb48c2f6..b3cc62bd36 100644\n--- a/t/lib-httpd.sh\n+++ b/t/lib-httpd.sh\n@@ -14,7 +14,6 @@\n #\n #\ttest_expect_success ...\n #\n-#\tstop_httpd\n #\ttest_done\n #\n # Can be configured using the following variables.\n@@ -176,7 +175,7 @@ prepare_httpd() {\n start_httpd() {\n \tprepare_httpd >&3 2>&4\n \n-\ttrap 'code=$?; stop_httpd; (exit $code); die' EXIT\n+\ttest_atexit stop_httpd\n \n \t\"$LIB_HTTPD_PATH\" -d \"$HTTPD_ROOT_PATH\" \\\n \t\t-f \"$TEST_PATH/apache.conf\" $HTTPD_PARA \\\n@@ -184,15 +183,12 @@ start_httpd() {\n \t\t>&3 2>&4\n \tif test $? -ne 0\n \tthen\n-\t\ttrap 'die' EXIT\n \t\tcat \"$HTTPD_ROOT_PATH\"/error.log >&4 2>/dev/null\n \t\ttest_skip_or_die $GIT_TEST_HTTPD \"web server setup failed\"\n \tfi\n }\n \n stop_httpd() {\n-\ttrap 'die' EXIT\n-\n \t\"$LIB_HTTPD_PATH\" -d \"$HTTPD_ROOT_PATH\" \\\n \t\t-f \"$TEST_PATH/apache.conf\" $HTTPD_PARA -k stop\n }\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex bce02788e6..5bd892f2f7 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -518,6 +518,4 @@ test_expect_success 'fetching of missing objects from an HTTP server' '\n \tgit verify-pack --verbose \"$IDX\" | grep \"$HASH\"\n '\n \n-stop_httpd\n-\n test_done\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 49c540b1e1..32426fa5d1 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -918,7 +918,4 @@ test_expect_success 'fetch with --filter=blob:limit=0 and HTTP' '\n \tfetch_filter_blob_limit_zero \"$HTTPD_DOCUMENT_ROOT_PATH/server\" \"$HTTPD_URL/smart/server\"\n '\n \n-stop_httpd\n-\n-\n test_done\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 3b7b30568c..e98d90dd9b 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -978,6 +978,4 @@ test_expect_success '--negotiation-tip limits \"have\" lines sent with HTTP protoc\n \tcheck_negotiation_tip\n '\n \n-stop_httpd\n-\n test_done\ndiff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh\nindex 6caf628efa..66f0b64d39 100755\n--- a/t/t5537-fetch-shallow.sh\n+++ b/t/t5537-fetch-shallow.sh\n@@ -255,6 +255,4 @@ test_expect_success 'shallow fetches check connectivity before writing shallow f\n \tgit -C client fsck\n '\n \n-stop_httpd\n-\n test_done\ndiff --git a/t/t5539-fetch-http-shallow.sh b/t/t5539-fetch-http-shallow.sh\nindex 5fbf67c446..98f028f203 100755\n--- a/t/t5539-fetch-http-shallow.sh\n+++ b/t/t5539-fetch-http-shallow.sh\n@@ -146,5 +146,4 @@ test_expect_success 'fetching deepen' '\n \t)\n '\n \n-stop_httpd\n test_done\ndiff --git a/t/t5540-http-push-webdav.sh b/t/t5540-http-push-webdav.sh\nindex 88ff5a49e4..a094fd5e71 100755\n--- a/t/t5540-http-push-webdav.sh\n+++ b/t/t5540-http-push-webdav.sh\n@@ -176,6 +176,4 @@ test_expect_failure 'push to password-protected repository (no user in URL)' '\n \ttest_cmp expect actual\n '\n \n-stop_httpd\n-\n test_done\ndiff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh\nindex 5475afc052..bdf40f445e 100755\n--- a/t/t5541-http-push-smart.sh\n+++ b/t/t5541-http-push-smart.sh\n@@ -373,5 +373,4 @@ test_expect_success 'colorize errors/hints' '\n \ttest_i18ngrep ! \"^hint: \" decoded\n '\n \n-stop_httpd\n test_done\ndiff --git a/t/t5542-push-http-shallow.sh b/t/t5542-push-http-shallow.sh\nindex 5165833157..ddc1db722d 100755\n--- a/t/t5542-push-http-shallow.sh\n+++ b/t/t5542-push-http-shallow.sh\n@@ -90,5 +90,4 @@ EOF\n \t)\n '\n \n-stop_httpd\n test_done\ndiff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh\nindex b47a95871c..6d1d59c9b1 100755\n--- a/t/t5545-push-options.sh\n+++ b/t/t5545-push-options.sh\n@@ -278,6 +278,4 @@ test_expect_success 'push options keep quoted characters intact (http)' '\n \ttest_cmp expect \"$HTTPD_DOCUMENT_ROOT_PATH\"/upstream.git/hooks/pre-receive.push_options\n '\n \n-stop_httpd\n-\n test_done\ndiff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh\nindex 6d7d88ccc9..57f6f8c628 100755\n--- a/t/t5550-http-fetch-dumb.sh\n+++ b/t/t5550-http-fetch-dumb.sh\n@@ -408,5 +408,4 @@ test_expect_success 'print HTTP error when any intermediate redirect throws erro\n \ttest_i18ngrep \"unable to access.*/redir-to/502\" stderr\n '\n \n-stop_httpd\n test_done\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex ba83e567e5..9faf6349cf 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -434,5 +434,4 @@ test_expect_success 'server-side error detected' '\n \tgrep \"server-side error\" actual\n '\n \n-stop_httpd\n test_done\ndiff --git a/t/t5561-http-backend.sh b/t/t5561-http-backend.sh\nindex 1c49054595..6eb0294978 100755\n--- a/t/t5561-http-backend.sh\n+++ b/t/t5561-http-backend.sh\n@@ -132,5 +132,4 @@ test_expect_success 'server request log matches test results' '\n \tcheck_access_log exp\n '\n \n-stop_httpd\n test_done\ndiff --git a/t/t5581-http-curl-verbose.sh b/t/t5581-http-curl-verbose.sh\nindex cd9283eeec..5129b0724f 100755\n--- a/t/t5581-http-curl-verbose.sh\n+++ b/t/t5581-http-curl-verbose.sh\n@@ -23,6 +23,4 @@ test_expect_success 'failure in git-upload-pack is shown' '\n \tgrep \"< HTTP/1.1 500 Intentional Breakage\" curl_log\n '\n \n-stop_httpd\n-\n test_done\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex d6948cbdab..b04d668684 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -733,6 +733,4 @@ test_expect_success 'partial clone using HTTP' '\n \tpartial_clone \"$HTTPD_DOCUMENT_ROOT_PATH/server\" \"$HTTPD_URL/smart/server\"\n '\n \n-stop_httpd\n-\n test_done\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex 9643acb161..9a8f9886b3 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -331,6 +331,4 @@ test_expect_success 'when partial cloning, tolerate server not sending target of\n \t! test -e \"$HTTPD_ROOT_PATH/one-time-sed\"\n '\n \n-stop_httpd\n-\n test_done\ndiff --git a/t/t5700-protocol-v1.sh b/t/t5700-protocol-v1.sh\nindex ba86a44eb1..b0e4752232 100755\n--- a/t/t5700-protocol-v1.sh\n+++ b/t/t5700-protocol-v1.sh\n@@ -289,6 +289,4 @@ test_expect_success 'push with http:// using protocol v1' '\n \tgrep \"git< version 1\" log\n '\n \n-stop_httpd\n-\n test_done\ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex db4ae09f2f..35424bb8af 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -656,6 +656,4 @@ test_expect_success 'when server does not send \"ready\", expect FLUSH' '\n \ttest_i18ngrep \"expected no other sections to be sent after no .ready.\" err\n '\n \n-stop_httpd\n-\n test_done\ndiff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh\nindex f87b2f6df3..b6a995e857 100755\n--- a/t/t5703-upload-pack-ref-in-want.sh\n+++ b/t/t5703-upload-pack-ref-in-want.sh\n@@ -257,8 +257,6 @@ test_expect_success 'server loses a ref - ref in want' '\n \ttest_i18ngrep \"fatal: remote error: unknown ref refs/heads/raster\" err\n '\n \n-stop_httpd\n-\n REPO=\"$(pwd)/repo\"\n LOCAL_PRISTINE=\"$(pwd)/local_pristine\"\n \ndiff --git a/t/t5812-proto-disable-http.sh b/t/t5812-proto-disable-http.sh\nindex 872788ac8c..af8772fada 100755\n--- a/t/t5812-proto-disable-http.sh\n+++ b/t/t5812-proto-disable-http.sh\n@@ -34,5 +34,4 @@ test_expect_success 'http can be limited to from-user' '\n \t\tclone \"$HTTPD_URL/smart-redir-perm/repo.git\" redir.git\n '\n \n-stop_httpd\n test_done\ndiff --git a/t/t9115-git-svn-dcommit-funky-renames.sh b/t/t9115-git-svn-dcommit-funky-renames.sh\nindex 64bb495834..9b44a44bc1 100755\n--- a/t/t9115-git-svn-dcommit-funky-renames.sh\n+++ b/t/t9115-git-svn-dcommit-funky-renames.sh\n@@ -120,6 +120,4 @@ test_expect_success !MINGW,!UTF8_NFD_TO_NFC 'svn.pathnameencoding=cp932 rename o\n \tgit svn dcommit\n '\n \n-stop_httpd\n-\n test_done\ndiff --git a/t/t9118-git-svn-funky-branch-names.sh b/t/t9118-git-svn-funky-branch-names.sh\nindex 41a026637f..a159ff96b7 100755\n--- a/t/t9118-git-svn-funky-branch-names.sh\n+++ b/t/t9118-git-svn-funky-branch-names.sh\n@@ -87,6 +87,4 @@ test_expect_success 'test dcommit to trailing_dotlock branch' '\n \t)\n \t'\n \n-stop_httpd\n-\n test_done\ndiff --git a/t/t9120-git-svn-clone-with-percent-escapes.sh b/t/t9120-git-svn-clone-with-percent-escapes.sh\nindex b28a1741e3..40b714df31 100755\n--- a/t/t9120-git-svn-clone-with-percent-escapes.sh\n+++ b/t/t9120-git-svn-clone-with-percent-escapes.sh\n@@ -74,6 +74,4 @@ test_expect_success 'test clone -s with unescaped space' '\n \t)\n '\n \n-stop_httpd\n-\n test_done\ndiff --git a/t/t9142-git-svn-shallow-clone.sh b/t/t9142-git-svn-shallow-clone.sh\nindex 9ee23be640..a30730502d 100755\n--- a/t/t9142-git-svn-shallow-clone.sh\n+++ b/t/t9142-git-svn-shallow-clone.sh\n@@ -26,6 +26,4 @@ test_expect_success 'clone trunk with \"-r HEAD\"' '\n \t( cd g && git rev-parse --symbolic --verify HEAD )\n '\n \n-stop_httpd\n-\n test_done\n-- \n2.21.0.499.g4d310c7a8e.dirty\n\n"},{"id":"371359","messageId":"20190313122419.2210-7-szeder.dev@gmail.com","threadId":"50729","inReplyTo":"20190313122419.2210-1-szeder.dev@gmail.com","subject":"[PATCH 06/11] t0301-credential-cache: use 'test_atexit' to stop the credentials helper","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-03-13T12:24:14Z","receivedAt":"2019-03-13T12:24:53Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Use 'test_atexit' to run cleanup commands to stop the credentials\nhelper at the end of the test script or upon interrupt or failure, as\nit is shorter, simpler, and more robust than registering such cleanup\ncommands in the trap on EXIT in the test scripts.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n t/t0301-credential-cache.sh | 7 +------\n 1 file changed, 1 insertion(+), 6 deletions(-)\n\ndiff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh\nindex fd92533acf..ebd5fa5249 100755\n--- a/t/t0301-credential-cache.sh\n+++ b/t/t0301-credential-cache.sh\n@@ -10,7 +10,7 @@ test -z \"$NO_UNIX_SOCKETS\" || {\n }\n \n # don't leave a stale daemon running\n-trap 'code=$?; git credential-cache exit; (exit $code); die' EXIT\n+test_atexit 'git credential-cache exit'\n \n # test that the daemon works with no special setup\n helper_test cache\n@@ -108,9 +108,4 @@ test_expect_success SYMLINKS 'use user socket if user directory is a symlink to\n \n helper_test_timeout cache --timeout=1\n \n-# we can't rely on our \"trap\" above working after test_done,\n-# as test_done will delete the trash directory containing\n-# our socket, leaving us with no way to access the daemon.\n-git credential-cache exit\n-\n test_done\n-- \n2.21.0.499.g4d310c7a8e.dirty\n\n"},{"id":"371360","messageId":"20190313122419.2210-8-szeder.dev@gmail.com","threadId":"50729","inReplyTo":"20190313122419.2210-1-szeder.dev@gmail.com","subject":"[PATCH 07/11] git p4 test: use 'test_atexit' to kill p4d and the watchdog process","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-03-13T12:24:15Z","receivedAt":"2019-03-13T12:24:56Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nUse 'test_atexit' to run cleanup commands to stop 'p4d' at the end of\nthe test script or upon interrupt or failure, as it is shorter,\nsimpler, and more robust than registering such cleanup commands in the\ntrap on EXIT in the test scripts.\n\nNote that one of the test scripts, 't9801-git-p4-branch.sh', stops and\nthen re-starts 'p4d' twice in the middle of the script; take care that\nthe cleanup functions to stop 'p4d' are only registered once.\n\nNote also that 'git p4' tests invoke different functions in the trap\non EXIT ('cleanup') and in the last test before 'test_done'\n('kill_p4d').  Register both of these functions with 'test_atexit' for\nnow, and a a later patch in this series will then clean up the\nredundancy.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n t/lib-git-p4.sh                            | 10 +++++++++-\n t/t9800-git-p4-basic.sh                    |  4 ----\n t/t9801-git-p4-branch.sh                   |  4 ----\n t/t9802-git-p4-filetype.sh                 |  4 ----\n t/t9803-git-p4-shell-metachars.sh          |  4 ----\n t/t9804-git-p4-label.sh                    |  4 ----\n t/t9805-git-p4-skip-submit-edit.sh         |  4 ----\n t/t9806-git-p4-options.sh                  |  5 -----\n t/t9807-git-p4-submit.sh                   |  4 ----\n t/t9808-git-p4-chdir.sh                    |  4 ----\n t/t9809-git-p4-client-view.sh              |  4 ----\n t/t9810-git-p4-rcs.sh                      |  4 ----\n t/t9811-git-p4-label-import.sh             |  5 -----\n t/t9812-git-p4-wildcards.sh                |  4 ----\n t/t9813-git-p4-preserve-users.sh           |  4 ----\n t/t9814-git-p4-rename.sh                   |  4 ----\n t/t9815-git-p4-submit-fail.sh              |  4 ----\n t/t9816-git-p4-locked.sh                   |  4 ----\n t/t9817-git-p4-exclude.sh                  |  4 ----\n t/t9818-git-p4-block.sh                    |  4 ----\n t/t9819-git-p4-case-folding.sh             |  4 ----\n t/t9820-git-p4-editor-handling.sh          |  4 ----\n t/t9821-git-p4-path-variations.sh          |  4 ----\n t/t9822-git-p4-path-encoding.sh            |  4 ----\n t/t9823-git-p4-mock-lfs.sh                 |  4 ----\n t/t9824-git-p4-git-lfs.sh                  |  4 ----\n t/t9825-git-p4-handle-utf16-without-bom.sh |  4 ----\n t/t9826-git-p4-keep-empty-commits.sh       |  4 ----\n t/t9827-git-p4-change-filetype.sh          |  4 ----\n t/t9828-git-p4-map-user.sh                 |  4 ----\n t/t9829-git-p4-jobs.sh                     |  4 ----\n t/t9830-git-p4-symlink-dir.sh              |  4 ----\n t/t9831-git-p4-triggers.sh                 |  4 ----\n t/t9832-unshelve.sh                        |  3 ---\n t/t9833-errors.sh                          |  5 -----\n 35 files changed, 9 insertions(+), 139 deletions(-)\n\ndiff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh\nindex b3be3ba011..958e33b77e 100644\n--- a/t/lib-git-p4.sh\n+++ b/t/lib-git-p4.sh\n@@ -74,7 +74,6 @@ cleanup () {\n \t\tkill -9 $(cat \"$pidfile\") 2>/dev/null && exit 255\n \tfi\n }\n-trap cleanup EXIT\n \n # git p4 submit generates a temp file, which will\n # not get cleaned up if the submission fails.  Don't\n@@ -82,7 +81,16 @@ trap cleanup EXIT\n TMPDIR=\"$TRASH_DIRECTORY\"\n export TMPDIR\n \n+registered_stop_p4d_atexit_handler=\n start_p4d () {\n+\t# One of the test scripts stops and then re-starts p4d.\n+\t# Don't register and then run the same atexit handlers several times.\n+\tif test -z \"$registered_stop_p4d_atexit_handler\"\n+\tthen\n+\t\ttest_atexit 'kill_p4d; cleanup'\n+\t\tregistered_stop_p4d_atexit_handler=AlreadyDone\n+\tfi\n+\n \tmkdir -p \"$db\" \"$cli\" \"$git\" &&\n \trm -f \"$pidfile\" &&\n \t(\ndiff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh\nindex 729cd25770..5856563068 100755\n--- a/t/t9800-git-p4-basic.sh\n+++ b/t/t9800-git-p4-basic.sh\n@@ -326,8 +326,4 @@ test_expect_success 'submit from worktree' '\n \t)\n '\n \n-test_expect_success 'kill p4d' '\n-\tkill_p4d\n-'\n-\n test_done\ndiff --git a/t/t9801-git-p4-branch.sh b/t/t9801-git-p4-branch.sh\nindex 6a86d6996b..50013132c8 100755\n--- a/t/t9801-git-p4-branch.sh\n+++ b/t/t9801-git-p4-branch.sh\n@@ -610,8 +610,4 @@ test_expect_success 'Update a file in git side and submit to P4 using client vie\n \t)\n '\n \n-test_expect_success 'kill p4d' '\n-\tkill_p4d\n-'\n-\n test_done\ndiff --git a/t/t9802-git-p4-filetype.sh b/t/t9802-git-p4-filetype.sh\nindex 9978352d78..94edebe272 100755\n--- a/t/t9802-git-p4-filetype.sh\n+++ b/t/t9802-git-p4-filetype.sh\n@@ -333,8 +333,4 @@ test_expect_success SYMLINKS 'empty symlink target' '\n \t)\n '\n \n-test_expect_success 'kill p4d' '\n-\tkill_p4d\n-'\n-\n test_done\ndiff --git a/t/t9803-git-p4-shell-metachars.sh b/t/t9803-git-p4-shell-metachars.sh\nindex d5c3675100..2913277013 100755\n--- a/t/t9803-git-p4-shell-metachars.sh\n+++ b/t/t9803-git-p4-shell-metachars.sh\n@@ -105,8 +105,4 @@ test_expect_success 'branch with shell char' '\n \t)\n '\n \n-test_expect_success 'kill p4d' '\n-\tkill_p4d\n-'\n-\n test_done\ndiff --git a/t/t9804-git-p4-label.sh b/t/t9804-git-p4-label.sh\nindex e30f80e617..3236457106 100755\n--- a/t/t9804-git-p4-label.sh\n+++ b/t/t9804-git-p4-label.sh\n@@ -108,8 +108,4 @@ test_expect_failure 'two labels on the same changelist' '\n \t)\n '\n \n-test_expect_success 'kill p4d' '\n-\tkill_p4d\n-'\n-\n test_done\ndiff --git a/t/t9805-git-p4-skip-submit-edit.sh b/t/t9805-git-p4-skip-submit-edit.sh\nindex 5fbf904dc8..90ef647db7 100755\n--- a/t/t9805-git-p4-skip-submit-edit.sh\n+++ b/t/t9805-git-p4-skip-submit-edit.sh\n@@ -98,8 +98,4 @@ test_expect_success 'no config, edited' '\n \t)\n '\n \n-test_expect_success 'kill p4d' '\n-\tkill_p4d\n-'\n-\n test_done\ndiff --git a/t/t9806-git-p4-options.sh b/t/t9806-git-p4-options.sh\nindex 3f5291b857..4e794a01bf 100755\n--- a/t/t9806-git-p4-options.sh\n+++ b/t/t9806-git-p4-options.sh\n@@ -300,9 +300,4 @@ test_expect_success 'use --git-dir option and GIT_DIR' '\n \ttest_path_is_file \"$git\"/cli_file2.t\n '\n \n-\n-test_expect_success 'kill p4d' '\n-\tkill_p4d\n-'\n-\n test_done\ndiff --git a/t/t9807-git-p4-submit.sh b/t/t9807-git-p4-submit.sh\nindex 850d979119..eaaae414a1 100755\n--- a/t/t9807-git-p4-submit.sh\n+++ b/t/t9807-git-p4-submit.sh\n@@ -593,8 +593,4 @@ test_expect_success 'update a shelve involving moved and copied files' '\n \t)\n '\n \n-test_expect_success 'kill p4d' '\n-\tkill_p4d\n-'\n-\n test_done\ndiff --git a/t/t9808-git-p4-chdir.sh b/t/t9808-git-p4-chdir.sh\nindex 11d2b5102c..58a9b3b71e 100755\n--- a/t/t9808-git-p4-chdir.sh\n+++ b/t/t9808-git-p4-chdir.sh\n@@ -83,8 +83,4 @@ test_expect_success SYMLINKS 'p4 client root symlink should stay symbolic' '\n \t)\n '\n \n-test_expect_success 'kill p4d' '\n-\tkill_p4d\n-'\n-\n test_done\ndiff --git a/t/t9809-git-p4-client-view.sh b/t/t9809-git-p4-client-view.sh\nindex 897b3c3034..3cff1fce1b 100755\n--- a/t/t9809-git-p4-client-view.sh\n+++ b/t/t9809-git-p4-client-view.sh\n@@ -836,8 +836,4 @@ test_expect_success 'quotes on both sides' '\n \tgit_verify \"cdir 1/file11\" \"cdir 1/file12\"\n '\n \n-test_expect_success 'kill p4d' '\n-\tkill_p4d\n-'\n-\n test_done\ndiff --git a/t/t9810-git-p4-rcs.sh b/t/t9810-git-p4-rcs.sh\nindex cc53debe19..57b533dc6f 100755\n--- a/t/t9810-git-p4-rcs.sh\n+++ b/t/t9810-git-p4-rcs.sh\n@@ -360,8 +360,4 @@ test_expect_failure 'Add keywords in git which do not match the default p4 value\n \t)\n '\n \n-test_expect_success 'kill p4d' '\n-\tkill_p4d\n-'\n-\n test_done\ndiff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh\nindex 602b0a5d5c..b70e81c3cd 100755\n--- a/t/t9811-git-p4-label-import.sh\n+++ b/t/t9811-git-p4-label-import.sh\n@@ -259,9 +259,4 @@ test_expect_success 'importing labels with missing revisions' '\n \t)\n '\n \n-\n-test_expect_success 'kill p4d' '\n-\tkill_p4d\n-'\n-\n test_done\ndiff --git a/t/t9812-git-p4-wildcards.sh b/t/t9812-git-p4-wildcards.sh\nindex 0206771fbb..254a7c2446 100755\n--- a/t/t9812-git-p4-wildcards.sh\n+++ b/t/t9812-git-p4-wildcards.sh\n@@ -211,8 +211,4 @@ test_expect_success 'wildcard files requiring keyword scrub' '\n \t)\n '\n \n-test_expect_success 'kill p4d' '\n-\tkill_p4d\n-'\n-\n test_done\ndiff --git a/t/t9813-git-p4-preserve-users.sh b/t/t9813-git-p4-preserve-users.sh\nindex 783c6ad165..fd018c87a8 100755\n--- a/t/t9813-git-p4-preserve-users.sh\n+++ b/t/t9813-git-p4-preserve-users.sh\n@@ -138,8 +138,4 @@ test_expect_success 'not preserving user with mixed authorship' '\n \t)\n '\n \n-test_expect_success 'kill p4d' '\n-\tkill_p4d\n-'\n-\n test_done\ndiff --git a/t/t9814-git-p4-rename.sh b/t/t9814-git-p4-rename.sh\nindex 60baa06e27..468767cbf4 100755\n--- a/t/t9814-git-p4-rename.sh\n+++ b/t/t9814-git-p4-rename.sh\n@@ -242,8 +242,4 @@ test_expect_success P4D_HAVE_CONFIGURABLE_RUN_MOVE_ALLOW \\\n \t)\n '\n \n-test_expect_success 'kill p4d' '\n-\tkill_p4d\n-'\n-\n test_done\ndiff --git a/t/t9815-git-p4-submit-fail.sh b/t/t9815-git-p4-submit-fail.sh\nindex eaf03a6563..9779dc0d11 100755\n--- a/t/t9815-git-p4-submit-fail.sh\n+++ b/t/t9815-git-p4-submit-fail.sh\n@@ -422,8 +422,4 @@ test_expect_success 'cleanup chmod after submit cancel' '\n \t)\n '\n \n-test_expect_success 'kill p4d' '\n-\tkill_p4d\n-'\n-\n test_done\ndiff --git a/t/t9816-git-p4-locked.sh b/t/t9816-git-p4-locked.sh\nindex d048bd33fa..932841003c 100755\n--- a/t/t9816-git-p4-locked.sh\n+++ b/t/t9816-git-p4-locked.sh\n@@ -138,8 +138,4 @@ test_expect_failure 'move with lock taken' '\n \t)\n '\n \n-test_expect_success 'kill p4d' '\n-\tkill_p4d\n-'\n-\n test_done\ndiff --git a/t/t9817-git-p4-exclude.sh b/t/t9817-git-p4-exclude.sh\nindex aac568eadf..96d25f0c02 100755\n--- a/t/t9817-git-p4-exclude.sh\n+++ b/t/t9817-git-p4-exclude.sh\n@@ -64,8 +64,4 @@ test_expect_success 'clone, then sync with exclude' '\n \t)\n '\n \n-test_expect_success 'kill p4d' '\n-\tkill_p4d\n-'\n-\n test_done\ndiff --git a/t/t9818-git-p4-block.sh b/t/t9818-git-p4-block.sh\nindex ce7cb22ad3..0db7ab9918 100755\n--- a/t/t9818-git-p4-block.sh\n+++ b/t/t9818-git-p4-block.sh\n@@ -146,8 +146,4 @@ test_expect_success 'Clone repo with self-sizing block size' '\n \ttest_line_count \\> 10 log\n '\n \n-test_expect_success 'kill p4d' '\n-\tkill_p4d\n-'\n-\n test_done\ndiff --git a/t/t9819-git-p4-case-folding.sh b/t/t9819-git-p4-case-folding.sh\nindex d808c008c1..600ce1e0b0 100755\n--- a/t/t9819-git-p4-case-folding.sh\n+++ b/t/t9819-git-p4-case-folding.sh\n@@ -53,8 +53,4 @@ test_expect_failure 'Clone UC repo with lc name' '\n \ttest_must_fail git p4 clone //depot/uc/...\n '\n \n-test_expect_success 'kill p4d' '\n-\tkill_p4d\n-'\n-\n test_done\ndiff --git a/t/t9820-git-p4-editor-handling.sh b/t/t9820-git-p4-editor-handling.sh\nindex 3c22f74bd4..fa1bba1dd9 100755\n--- a/t/t9820-git-p4-editor-handling.sh\n+++ b/t/t9820-git-p4-editor-handling.sh\n@@ -31,8 +31,4 @@ test_expect_success 'EDITOR with options' '\n \t)\n '\n \n-test_expect_success 'kill p4d' '\n-\tkill_p4d\n-'\n-\n test_done\ndiff --git a/t/t9821-git-p4-path-variations.sh b/t/t9821-git-p4-path-variations.sh\nindex 81e46acfa8..ef80f1690b 100755\n--- a/t/t9821-git-p4-path-variations.sh\n+++ b/t/t9821-git-p4-path-variations.sh\n@@ -193,8 +193,4 @@ test_expect_success 'Add a new file and clone path with new file (ignorecase)' '\n \t)\n '\n \n-test_expect_success 'kill p4d' '\n-\tkill_p4d\n-'\n-\n test_done\ndiff --git a/t/t9822-git-p4-path-encoding.sh b/t/t9822-git-p4-path-encoding.sh\nindex c78477c19b..1bf7635016 100755\n--- a/t/t9822-git-p4-path-encoding.sh\n+++ b/t/t9822-git-p4-path-encoding.sh\n@@ -67,8 +67,4 @@ test_expect_success 'Delete iso8859-1 encoded paths and clone' '\n \t)\n '\n \n-test_expect_success 'kill p4d' '\n-\tkill_p4d\n-'\n-\n test_done\ndiff --git a/t/t9823-git-p4-mock-lfs.sh b/t/t9823-git-p4-mock-lfs.sh\nindex 1f2dc369bf..88b76dc4d6 100755\n--- a/t/t9823-git-p4-mock-lfs.sh\n+++ b/t/t9823-git-p4-mock-lfs.sh\n@@ -185,8 +185,4 @@ test_expect_success 'Run git p4 submit in repo configured with large file system\n \t)\n '\n \n-test_expect_success 'kill p4d' '\n-\tkill_p4d\n-'\n-\n test_done\ndiff --git a/t/t9824-git-p4-git-lfs.sh b/t/t9824-git-p4-git-lfs.sh\nindex ed80ca858c..a28dbbdd56 100755\n--- a/t/t9824-git-p4-git-lfs.sh\n+++ b/t/t9824-git-p4-git-lfs.sh\n@@ -287,8 +287,4 @@ test_expect_success 'Add big files to repo and store files in LFS based on compr\n \t)\n '\n \n-test_expect_success 'kill p4d' '\n-\tkill_p4d\n-'\n-\n test_done\ndiff --git a/t/t9825-git-p4-handle-utf16-without-bom.sh b/t/t9825-git-p4-handle-utf16-without-bom.sh\nindex 1551845dc1..f049ff8229 100755\n--- a/t/t9825-git-p4-handle-utf16-without-bom.sh\n+++ b/t/t9825-git-p4-handle-utf16-without-bom.sh\n@@ -43,8 +43,4 @@ test_expect_failure 'clone depot with invalid UTF-16 file in non-verbose mode' '\n \tgit p4 clone --dest=\"$git\" //depot\n '\n \n-test_expect_success 'kill p4d' '\n-\tkill_p4d\n-'\n-\n test_done\ndiff --git a/t/t9826-git-p4-keep-empty-commits.sh b/t/t9826-git-p4-keep-empty-commits.sh\nindex fa8b9daf1f..fd64afe064 100755\n--- a/t/t9826-git-p4-keep-empty-commits.sh\n+++ b/t/t9826-git-p4-keep-empty-commits.sh\n@@ -127,8 +127,4 @@ test_expect_success 'Clone repo subdir with all history' '\n \t)\n '\n \n-test_expect_success 'kill p4d' '\n-\tkill_p4d\n-'\n-\n test_done\ndiff --git a/t/t9827-git-p4-change-filetype.sh b/t/t9827-git-p4-change-filetype.sh\nindex 7433998f47..d3670bd7a2 100755\n--- a/t/t9827-git-p4-change-filetype.sh\n+++ b/t/t9827-git-p4-change-filetype.sh\n@@ -59,8 +59,4 @@ test_expect_success SYMLINKS 'change symbolic link to file' '\n \t)\n '\n \n-test_expect_success 'kill p4d' '\n-\tkill_p4d\n-'\n-\n test_done\ndiff --git a/t/t9828-git-p4-map-user.sh b/t/t9828-git-p4-map-user.sh\nindex e20395c89f..ca6c2942bd 100755\n--- a/t/t9828-git-p4-map-user.sh\n+++ b/t/t9828-git-p4-map-user.sh\n@@ -54,8 +54,4 @@ test_expect_success 'Clone repo root path with all history' '\n \t)\n '\n \n-test_expect_success 'kill p4d' '\n-\tkill_p4d\n-'\n-\n test_done\ndiff --git a/t/t9829-git-p4-jobs.sh b/t/t9829-git-p4-jobs.sh\nindex 971aeeea1f..88cfb1fcd3 100755\n--- a/t/t9829-git-p4-jobs.sh\n+++ b/t/t9829-git-p4-jobs.sh\n@@ -92,8 +92,4 @@ test_expect_success 'check log message of changelist with more jobs' '\n \t)\n '\n \n-test_expect_success 'kill p4d' '\n-\tkill_p4d\n-'\n-\n test_done\ndiff --git a/t/t9830-git-p4-symlink-dir.sh b/t/t9830-git-p4-symlink-dir.sh\nindex 2ad1b0810d..3fb6960c18 100755\n--- a/t/t9830-git-p4-symlink-dir.sh\n+++ b/t/t9830-git-p4-symlink-dir.sh\n@@ -36,8 +36,4 @@ test_expect_success 'symlinked directory' '\n \n '\n \n-test_expect_success 'kill p4d' '\n-\tkill_p4d\n-'\n-\n test_done\ndiff --git a/t/t9831-git-p4-triggers.sh b/t/t9831-git-p4-triggers.sh\nindex be44c9751a..d743ca33ee 100755\n--- a/t/t9831-git-p4-triggers.sh\n+++ b/t/t9831-git-p4-triggers.sh\n@@ -96,8 +96,4 @@ test_expect_success 'submit description with extra info lines from verbose p4 ch\n \t)\n '\n \n-test_expect_success 'kill p4d' '\n-\tkill_p4d\n-'\n-\n test_done\ndiff --git a/t/t9832-unshelve.sh b/t/t9832-unshelve.sh\nindex 41c09f11f4..1286a5b824 100755\n--- a/t/t9832-unshelve.sh\n+++ b/t/t9832-unshelve.sh\n@@ -174,8 +174,5 @@ test_expect_success 'unshelve specifying the origin' '\n \t\ttest_path_is_file file_to_shelve\n \t)\n '\n-test_expect_success 'kill p4d' '\n-\tkill_p4d\n-'\n \n test_done\ndiff --git a/t/t9833-errors.sh b/t/t9833-errors.sh\nindex 47b312e1c9..e22369ccdf 100755\n--- a/t/t9833-errors.sh\n+++ b/t/t9833-errors.sh\n@@ -45,9 +45,4 @@ test_expect_success 'ticket logged out' '\n \t)\n '\n \n-test_expect_success 'kill p4d' '\n-\tkill_p4d\n-'\n-\n-\n test_done\n-- \n2.21.0.499.g4d310c7a8e.dirty\n\n"},{"id":"371361","messageId":"20190313122419.2210-11-szeder.dev@gmail.com","threadId":"50729","inReplyTo":"20190313122419.2210-1-szeder.dev@gmail.com","subject":"[PATCH 10/11] git p4 test: disable '-x' tracing in the p4d watchdog loop","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-03-13T12:24:18Z","receivedAt":"2019-03-13T12:24:57Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"According to the comments in 't/lib-git-p4.sh', sometimes 'p4d' seems\nto hang, and to deal with that 'start_p4d' starts a watchdog process\nto kill it after a long-enough timeout ($P4D_TIMEOUT, defaults to\n300s).  This watchdog process is implemented as a background subshell\nloop iterating once every second until the timeout expires, producing\na few lines of trace output on each iteration when the test script is\nrun with '-x' tracing enabled.  The watchdog loop's trace gets\nintermixed with the real test output and trace, and makes that harder\nto read.\n\nSend the trace output of this loop to /dev/null to avoid polluting the\nreal test output.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n t/lib-git-p4.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh\nindex c18f85082f..547b9f88e1 100644\n--- a/t/lib-git-p4.sh\n+++ b/t/lib-git-p4.sh\n@@ -106,7 +106,7 @@ start_p4d () {\n \t\tfi\n \t\tsleep 1\n \t\tnr_tries_left=$(($nr_tries_left - 1))\n-\tdone &\n+\tdone 2>/dev/null 4>&2 &\n \twatchdog_pid=$!\n \n \tready=\n-- \n2.21.0.499.g4d310c7a8e.dirty\n\n"},{"id":"371362","messageId":"20190313122419.2210-10-szeder.dev@gmail.com","threadId":"50729","inReplyTo":"20190313122419.2210-1-szeder.dev@gmail.com","subject":"[PATCH 09/11] git p4 test: simplify timeout handling","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-03-13T12:24:17Z","receivedAt":"2019-03-13T12:24:58Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"'lib-git-p4.sh' uses timeouts in a watchdog process to kill a\npotentially stuck 'p4d' process and for certain cleanup operation\nbetween tests.  It does so by first computing when the timeout should\nexpire, and then repeatedly asking for the current time in seconds\nuntil it exceeds the expiration time, and for portability reasons it\nuses a one-liner Python script to ask for the current time.\n\nReplace these timeouts with downcounters, which, though not\nnecessarily shorter, are much simpler, at least in the sense that they\ndon't execute the Python interpreter every second.\n\nAfter this change the helper function with that Python one-liner has\nno callers left, remove it.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n t/lib-git-p4.sh | 19 ++++++-------------\n 1 file changed, 6 insertions(+), 13 deletions(-)\n\ndiff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh\nindex b5cb5075c2..c18f85082f 100644\n--- a/t/lib-git-p4.sh\n+++ b/t/lib-git-p4.sh\n@@ -44,15 +44,6 @@ native_path () {\n \techo \"$path\"\n }\n \n-# On Solaris the 'date +%s' function is not supported and therefore we\n-# need this replacement.\n-# Attention: This function is not safe again against time offset updates\n-# at runtime (e.g. via NTP). The 'clock_gettime(CLOCK_MONOTONIC)'\n-# function could fix that but it is not in Python until 3.3.\n-time_in_seconds () {\n-\t(cd / && \"$PYTHON_PATH\" -c 'import time; print(int(time.time()))')\n-}\n-\n test_set_port P4DPORT\n \n P4PORT=localhost:$P4DPORT\n@@ -105,15 +96,16 @@ start_p4d () {\n \t# will be caught with the \"kill -0\" check below.\n \ti=${P4D_START_PATIENCE:-300}\n \n-\ttimeout=$(($(time_in_seconds) + $P4D_TIMEOUT))\n+\tnr_tries_left=$P4D_TIMEOUT\n \twhile true\n \tdo\n-\t\tif test $(time_in_seconds) -gt $timeout\n+\t\tif test $nr_tries_left -eq 0\n \t\tthen\n \t\t\tkill -9 $p4d_pid\n \t\t\texit 1\n \t\tfi\n \t\tsleep 1\n+\t\tnr_tries_left=$(($nr_tries_left - 1))\n \tdone &\n \twatchdog_pid=$!\n \n@@ -167,10 +159,11 @@ p4_add_job () {\n }\n \n retry_until_success () {\n-\ttimeout=$(($(time_in_seconds) + $RETRY_TIMEOUT))\n-\tuntil \"$@\" 2>/dev/null || test $(time_in_seconds) -gt $timeout\n+\tnr_tries_left=$RETRY_TIMEOUT\n+\tuntil \"$@\" 2>/dev/null || test $nr_tries_left -eq 0\n \tdo\n \t\tsleep 1\n+\t\tnr_tries_left=$(($nr_tries_left - 1))\n \tdone\n }\n \n-- \n2.21.0.499.g4d310c7a8e.dirty\n\n"},{"id":"371363","messageId":"20190313122419.2210-12-szeder.dev@gmail.com","threadId":"50729","inReplyTo":"20190313122419.2210-1-szeder.dev@gmail.com","subject":"[PATCH 11/11] t9811-git-p4-label-import: fix pipeline negation","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-03-13T12:24:19Z","receivedAt":"2019-03-13T12:24:59Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"In 't9811-git-p4-label-import.sh' the test 'tag that cannot be\nexported' runs\n\n  !(p4 labels | grep GIT_TAG_ON_A_BRANCH)\n\nto check that the given string is not printed by 'p4 labels'.  This is\nproblematic, because according to POSIX [1]:\n\n  \"If the pipeline begins with the reserved word ! and command1 is a\n  subshell command, the application shall ensure that the ( operator\n  at the beginning of command1 is separated from the ! by one or more\n  <blank> characters. The behavior of the reserved word ! immediately\n  followed by the ( operator is unspecified.\"\n\nWhile most common shells still interpret this '!' as \"negate the exit\ncode of the last command in the pipeline\", 'mksh/lksh' don't and\ninterpret it as a negative file name pattern instead.  As a result\nthey attempt to run a command made up of the pathnames in the current\ndirectory (it contains a single directory called 'main'), which, of\ncourse, fails the test.\n\nWe could fix it simply by adding a space between the '!' and '(', but\ninstead let's fix it by removing the unnecessary subshell.\n\n[1] http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09_02\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n t/t9811-git-p4-label-import.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh\nindex b70e81c3cd..c1446f26ab 100755\n--- a/t/t9811-git-p4-label-import.sh\n+++ b/t/t9811-git-p4-label-import.sh\n@@ -191,7 +191,7 @@ test_expect_success 'tag that cannot be exported' '\n \t(\n \t\tcd \"$cli\" &&\n \t\tp4 sync ... &&\n-\t\t!(p4 labels | grep GIT_TAG_ON_A_BRANCH)\n+\t\t! p4 labels | grep GIT_TAG_ON_A_BRANCH\n \t)\n '\n \n-- \n2.21.0.499.g4d310c7a8e.dirty\n\n"},{"id":"371364","messageId":"20190313122419.2210-9-szeder.dev@gmail.com","threadId":"50729","inReplyTo":"20190313122419.2210-1-szeder.dev@gmail.com","subject":"[PATCH 08/11] git p4 test: clean up the p4d cleanup functions","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-03-13T12:24:16Z","receivedAt":"2019-03-13T12:25:00Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Confusingly, the 'git p4' tests used two cleanup functions:\n\n  - 'kill_p4d' was run in the last test before 'test_done', and it not\n    only killed 'p4d', but it killed the watchdog process, and cleaned\n    up after 'p4d' as well by removing all directories used by the P4\n    daemon and client.\n\n    This cleanup is not necessary right before 'test_done', because\n    the whole trash directory is about to get removed anyway, but it\n    is necessary in 't9801-git-p4-branch.sh', which uses 'kill_p4d' to\n    stop 'p4d' before re-starting it in the middle of the test script.\n\n  - 'cleanup' was run in the trap on EXIT, and it killed 'p4d', but,\n    it didn't kill the watchdog process, and, contrarily to its name,\n    didn't perform any cleanup whatsoever.\n\nMake it clearer what's going on by renaming and simplifying the\ncleanup functions, so in the end we'll have:\n\n  - 'stop_p4d_and_watchdog' replaces 'cleanup' as it will try to live\n    up to its name and stop both the 'p4d' and the watchdog processes,\n    and as the sole function registered with 'test_atexit' it will be\n    responsible for no leaving any stray processes behind after 'git p4'\n    tests were finished or interrupted.\n\n  - 'stop_and_cleanup_p4d' replaces 'kill_p4d' as it will stop 'p4d'\n    (and the watchdog) and remove all directories used by the P4\n    daemon and cliean, so it can be used mid-script to stop and then\n    re-start 'p4d'.\n\nNote that while 'cleanup' sent a single SIGKILL to 'p4d', 'kill_p4d'\nwas quite brutal, as it first sent SIGTERM to the daemon repeatedly,\neither until its pid disappeared or until a given timeout was up, and\nthen it sent SIGKILL repeatedly, for good measure.  This is overkill\n(pardon the pun): a single SIGKILL should be able to take down any\nprocess in a sensible state, and if a process were to somehow end up\nstuck in the dreaded uninterruptible sleep state then even repeated\nSIGKILLs won't bring immediate help.  So ditch all the repeated\nSIGTERM/SIGKILL parts, and use a single SIGKILL to stop 'p4d', and\nmake sure that there are no races between asynchron signal delivery\nand subsequent restart of 'p4d' by waiting for it to die.\n\nWith this change the 'retry_until_fail' helper has no callers left,\nremove it.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n t/lib-git-p4.sh          | 36 ++++++++++--------------------------\n t/t9801-git-p4-branch.sh |  4 ++--\n 2 files changed, 12 insertions(+), 28 deletions(-)\n\ndiff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh\nindex 958e33b77e..b5cb5075c2 100644\n--- a/t/lib-git-p4.sh\n+++ b/t/lib-git-p4.sh\n@@ -67,12 +67,8 @@ cli=\"$TRASH_DIRECTORY/cli\"\n git=\"$TRASH_DIRECTORY/git\"\n pidfile=\"$TRASH_DIRECTORY/p4d.pid\"\n \n-# Sometimes \"prove\" seems to hang on exit because p4d is still running\n-cleanup () {\n-\tif test -f \"$pidfile\"\n-\tthen\n-\t\tkill -9 $(cat \"$pidfile\") 2>/dev/null && exit 255\n-\tfi\n+stop_p4d_and_watchdog () {\n+\tkill -9 $p4d_pid $watchdog_pid\n }\n \n # git p4 submit generates a temp file, which will\n@@ -87,7 +83,7 @@ start_p4d () {\n \t# Don't register and then run the same atexit handlers several times.\n \tif test -z \"$registered_stop_p4d_atexit_handler\"\n \tthen\n-\t\ttest_atexit 'kill_p4d; cleanup'\n+\t\ttest_atexit 'stop_p4d_and_watchdog'\n \t\tregistered_stop_p4d_atexit_handler=AlreadyDone\n \tfi\n \n@@ -100,6 +96,7 @@ start_p4d () {\n \t\t\techo $! >\"$pidfile\"\n \t\t}\n \t) &&\n+\tp4d_pid=$(cat \"$pidfile\")\n \n \t# This gives p4d a long time to start up, as it can be\n \t# quite slow depending on the machine.  Set this environment\n@@ -107,14 +104,13 @@ start_p4d () {\n \t# an automated test setup.  If the p4d process dies, that\n \t# will be caught with the \"kill -0\" check below.\n \ti=${P4D_START_PATIENCE:-300}\n-\tpid=$(cat \"$pidfile\")\n \n \ttimeout=$(($(time_in_seconds) + $P4D_TIMEOUT))\n \twhile true\n \tdo\n \t\tif test $(time_in_seconds) -gt $timeout\n \t\tthen\n-\t\t\tkill -9 $pid\n+\t\t\tkill -9 $p4d_pid\n \t\t\texit 1\n \t\tfi\n \t\tsleep 1\n@@ -131,7 +127,7 @@ start_p4d () {\n \t\t\tbreak\n \t\tfi\n \t\t# fail if p4d died\n-\t\tkill -0 $pid 2>/dev/null || break\n+\t\tkill -0 $p4d_pid 2>/dev/null || break\n \t\techo waiting for p4d to start\n \t\tsleep 1\n \t\ti=$(( $i - 1 ))\n@@ -178,22 +174,10 @@ retry_until_success () {\n \tdone\n }\n \n-retry_until_fail () {\n-\ttimeout=$(($(time_in_seconds) + $RETRY_TIMEOUT))\n-\tuntil ! \"$@\" 2>/dev/null || test $(time_in_seconds) -gt $timeout\n-\tdo\n-\t\tsleep 1\n-\tdone\n-}\n-\n-kill_p4d () {\n-\tpid=$(cat \"$pidfile\")\n-\tretry_until_fail kill $pid\n-\tretry_until_fail kill -9 $pid\n-\t# complain if it would not die\n-\ttest_must_fail kill $pid >/dev/null 2>&1 &&\n-\trm -rf \"$db\" \"$cli\" \"$pidfile\" &&\n-\tretry_until_fail kill -9 $watchdog_pid\n+stop_and_cleanup_p4d () {\n+\tkill -9 $p4d_pid $watchdog_pid\n+\twait $p4d_pid\n+\trm -rf \"$db\" \"$cli\" \"$pidfile\"\n }\n \n cleanup_git () {\ndiff --git a/t/t9801-git-p4-branch.sh b/t/t9801-git-p4-branch.sh\nindex 50013132c8..38d6b9043b 100755\n--- a/t/t9801-git-p4-branch.sh\n+++ b/t/t9801-git-p4-branch.sh\n@@ -151,7 +151,7 @@ test_expect_success 'import depot, branch detection, branchList branch definitio\n '\n \n test_expect_success 'restart p4d' '\n-\tkill_p4d &&\n+\tstop_and_cleanup_p4d &&\n \tstart_p4d\n '\n \n@@ -505,7 +505,7 @@ test_expect_success 'use-client-spec detect-branches skips files in branches' '\n '\n \n test_expect_success 'restart p4d' '\n-\tkill_p4d &&\n+\tstop_and_cleanup_p4d &&\n \tstart_p4d\n '\n \n-- \n2.21.0.499.g4d310c7a8e.dirty\n\n"},{"id":"371443","messageId":"xmqq1s3aci66.fsf@gitster-ct.c.googlers.com","threadId":"50729","inReplyTo":"20190313122419.2210-2-szeder.dev@gmail.com","subject":"Re: [PATCH 01/11] test-lib: fix interrupt handling with 'dash' and '--verbose-log -x'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-14T02:18:41Z","receivedAt":"2019-03-14T02:18:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor <szeder.dev@gmail.com> writes:\n\n> Putting these together, when a test script run with 'dash' and\n> '--verbose-log -x' is interrupted, then 'dash' tries to write the\n> trace output from the EXIT trap to the script's original standard\n> error, but it very likely can't, because the 'tee' downstream of the\n> pipe is interrupted as well.  This causes the shell running the test\n> script to die because of SIGPIPE, without running any of the commands\n> in the EXIT trap.\n\nSo... the clean-up actions do not get a chance to run because the\nshell that is going to execute would be killed by SIGPIPE because\ntee that is reading from it goes away?\n\nWhile I like the patch very much, I also wonder if it is possible to\ntell tee to not to die upon seeing INT, TERM, etc.  When the shell\nupstream of tee exits (after performing clean-up actions), tee that\nis reading from it will exit, too, and will not be left behind (I do\nnot mean to say it that is a better alternative solution---I am just\ntrying to see if I read the problem correctly from the description\ngiven above).\n\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 8665b0a9b6..db3875d1e4 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -631,7 +631,10 @@ die () {\n>  \n>  GIT_EXIT_OK=\n>  trap 'die' EXIT\n> -trap 'exit $?' INT TERM HUP\n> +# Disable '-x' tracing, because with some shells, notably dash, it\n> +# prevents running the cleanup commands when a test script run with\n> +# '--verbose-log -x' is interrupted.\n> +trap '{ code=$?; set +x; } 2>/dev/null; exit $code' INT TERM HUP\n>  \n>  # The user-facing functions are loaded from a separate file so that\n>  # test_perf subshells can have them too\n\nThanks.\n"},{"id":"371445","messageId":"xmqqpnqub2tx.fsf@gitster-ct.c.googlers.com","threadId":"50729","inReplyTo":"20190313122419.2210-3-szeder.dev@gmail.com","subject":"Re: [PATCH 02/11] t/lib-git-daemon: make sure to kill the 'git-daemon' process","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-14T02:35:22Z","receivedAt":"2019-03-14T02:35:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor <szeder.dev@gmail.com> writes:\n\n> After 'start_git_daemon' starts 'git daemon' (note the space in the\n> middle) in the background, it saves the background process' PID, so\n> the daemon can be stopped at the end of the test script.  However,\n> 'git-daemon' is not a builtin but a dashed external command, which\n> means that the dashless 'git daemon' executes the dashed 'git-daemon'\n> command, and, consequently, the PID recorded is not the PID of the\n> \"real\" daemon process, but that of the main 'git' wrapper.\n\nHmph.  execv_dashed_external() does not quite execv() but uses\nrun_command() and waits, which is the source of this issue.  It\nfeels almost possible to work it around by actually exec'ing,\nthough.\n\n\t/*\n\t * If we fail because the command is not found, it is\n\t * OK to return. Otherwise, we just pass along the status code,\n\t * or our usual generic code if we were not even able to exec\n\t * the program.\n\t */\n\tstatus = run_command(&cmd);\n\tif (status >= 0)\n\t\texit(status);\n\telse if (errno != ENOENT)\n\t\texit(128);\n\nwould become\n\n\t... prepare args from cmd ...\n\texecv(...);\n\tif (errno != ENOENT)\n\t\texit(128);\n\nand if exec succeeds, the whole process will exit with the status of\ndashed command, failing to exec for reasons other than ENOENT will\nexit with 128 and ENOENT will continue returning.\n\n> Now, if a\n> test script involving 'git daemon' is interrupted by ctrl-C, then only\n> the main 'git' process is stopped, but the real daemon process tends\n> to survive somehow, and keeps on running in the background\n> indefinitely, keeping the daemon's port to itself, and thus preventing\n> subsequent runs of the same test script.\n>\n> Work this around by running 'git daemon' with the '--pidfile=...'\n> option to save the PID of the real daemon process, and kill that\n> process in 'stop_git_daemon' as well.\n\nOK, so for the purpose of waiting and monitoring, we still use the\npid of the git potty that is running and waiting for the daemon, but\nwe'll send another signal to kill the daemon off.  Makes sense.\n"},{"id":"371447","messageId":"xmqqh8c6b0pv.fsf@gitster-ct.c.googlers.com","threadId":"50729","inReplyTo":"20190313122419.2210-4-szeder.dev@gmail.com","subject":"Re: [PATCH 03/11] test-lib: introduce 'test_atexit'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-14T03:21:00Z","receivedAt":"2019-03-14T03:21:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor <szeder.dev@gmail.com> writes:\n\n> +test_atexit () {\n> +\t# We cannot detect when we are in a subshell in general, but by\n> +\t# doing so on Bash is better than nothing (the test will\n> +\t# silently pass on other shells).\n> +\ttest \"${BASH_SUBSHELL-0}\" = 0 ||\n> +\terror \"bug in test script: test_atexit does nothing in a subshell\"\n> +\ttest_atexit_cleanup=\"{ $*\n> +\t\t} && (exit \\\"\\$eval_ret\\\"); eval_ret=\\$?; $test_atexit_cleanup\"\n> +}\n\nThis chaining is modelled after how $test_cleaup is built and\nmaintained by test_when_finished.  Use of eval_ret makes sense in\nthat original context as eval_ret _is_ used to keep track of the\nresult of 'test_eval_ \"$1\"' in test_run_ that executed the body\nof a single test_expect_$something, and $test_cleanup would want to\nkeep the resulting status from that body when clean-up succeeds (or\notherwise, make that failure to clean-up visible as $eval_ret).\n\nBut does it make sense in the context of the whole test script to\ntry maintaining $eval_ret?\n\n>  # Most tests can use the created repository, but some may need to create more.\n>  # Usage: test_create_repo <directory>\n>  test_create_repo () {\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index db3875d1e4..b35881696f 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -620,6 +620,10 @@ test_external_has_tap=0\n>  \n>  die () {\n>  \tcode=$?\n> +\t# This is responsible for running the atexit commands even when a\n> +\t# test script run with '--immediate' fails, or when the user hits\n> +\t# ctrl-C, i.e. when 'test_done' is not invoked at all.\n> +\ttest_atexit_handler || code=$?\n>  \tif test -n \"$GIT_EXIT_OK\"\n>  \tthen\n>  \t\texit $code\n> @@ -1045,9 +1049,28 @@ write_junit_xml_testcase () {\n>  \tjunit_have_testcase=t\n>  }\n>  \n> +test_atexit_cleanup=:\n> +test_atexit_handler () {\n> +\t# In a succeeding test script 'test_atexit_handler' is invoked\n> +\t# twice: first from 'test_done', then from 'die' in the trap on\n> +\t# EXIT.\n\nWe are guaranteed to still have the trash directory when we are run\nin the exit handler after getting interrupted or test_failure_()\nexits under the \"-i\" option, and when test_done() calls us.  What\nwill cause us trouble is the exit handler at the end of a successful\nrun after test_done() finishes.  At that point, test_done would have\nalready cleared the trash directory, so we may not have enough state\nto allow us to clean-up at exit.\n\nClearing the exit trap in test_done after it calls us might be an\nalternative, but I think it is equivalent to clearing the\ntest_atexit_cleanup variable, and it is cleaner, so I think I agree\nwith the approach this patch uses.\n\n> +\t# This condition and resetting 'test_atexit_cleanup' below makes\n> +\t# sure that the registered cleanup commands are run only once.\n> +\ttest : != \"$test_atexit_cleanup\" || return 0\n\nI think test_when_finished uses a special value in $test_cleanup in\na similar way but it even skips when there is no point doing the\ntest_eval_ of the \"accumulated\" scriptlet when it is empty.\n\nShouldn't we be doing the same thing, i.e.\n\n\tif test -z \"$test_atexit_cleanup\"\n\tthen\n\t\treturn 0\n\tfi\n\t... do the heavy lifting ...\n\ttest_atexit_cleanup=\n\nThat will make the handler truly a no-op when there is no atexit\ndefined.\n\n> +\tsetup_malloc_check\n> +\ttest_eval_ \"$test_atexit_cleanup\"\n> +\ttest_atexit_cleanup=:\n> +\tteardown_malloc_check\n> +}\n> +\n>  test_done () {\n>  \tGIT_EXIT_OK=t\n>  \n> +\t# Run the atexit commands _before_ the trash directory is\n> +\t# removed, so the commands can access pidfiles and socket files.\n> +\ttest_atexit_handler\n> +\n>  \tif test -n \"$write_junit_xml\" && test -n \"$junit_xml_path\"\n>  \tthen\n>  \t\ttest -n \"$junit_have_testcase\" || {\n"},{"id":"371448","messageId":"xmqqd0mub0d8.fsf@gitster-ct.c.googlers.com","threadId":"50729","inReplyTo":"20190313122419.2210-6-szeder.dev@gmail.com","subject":"Re: [PATCH 05/11] tests: use 'test_atexit' to stop httpd","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-14T03:28:35Z","receivedAt":"2019-03-14T03:28:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor <szeder.dev@gmail.com> writes:\n\n>  t/lib-git-svn.sh                              | 5 -----\n>  t/lib-httpd.sh                                | 6 +-----\n>  t/t0410-partial-clone.sh                      | 2 --\n>  t/t5500-fetch-pack.sh                         | 3 ---\n>  t/t5510-fetch.sh                              | 2 --\n>  t/t5537-fetch-shallow.sh                      | 2 --\n>  t/t5539-fetch-http-shallow.sh                 | 1 -\n>  t/t5540-http-push-webdav.sh                   | 2 --\n>  t/t5541-http-push-smart.sh                    | 1 -\n>  t/t5542-push-http-shallow.sh                  | 1 -\n>  t/t5545-push-options.sh                       | 2 --\n>  t/t5550-http-fetch-dumb.sh                    | 1 -\n>  t/t5551-http-fetch-smart.sh                   | 1 -\n>  t/t5561-http-backend.sh                       | 1 -\n>  t/t5581-http-curl-verbose.sh                  | 2 --\n>  t/t5601-clone.sh                              | 2 --\n>  t/t5616-partial-clone.sh                      | 2 --\n>  t/t5700-protocol-v1.sh                        | 2 --\n>  t/t5702-protocol-v2.sh                        | 2 --\n>  t/t5703-upload-pack-ref-in-want.sh            | 2 --\n>  t/t5812-proto-disable-http.sh                 | 1 -\n>  t/t9115-git-svn-dcommit-funky-renames.sh      | 2 --\n>  t/t9118-git-svn-funky-branch-names.sh         | 2 --\n>  t/t9120-git-svn-clone-with-percent-escapes.sh | 2 --\n>  t/t9142-git-svn-shallow-clone.sh              | 2 --\n\nI see most of these changes are removal of stop_httpd because it is\ndone as part of start_httpd() to arrange it to be called at exit.\n\nBut ...\n\n> @@ -176,7 +175,7 @@ prepare_httpd() {\n>  start_httpd() {\n>  \tprepare_httpd >&3 2>&4\n>  \n> -\ttrap 'code=$?; stop_httpd; (exit $code); die' EXIT\n> +\ttest_atexit stop_httpd\n>  \n>  \t\"$LIB_HTTPD_PATH\" -d \"$HTTPD_ROOT_PATH\" \\\n>  \t\t-f \"$TEST_PATH/apache.conf\" $HTTPD_PARA \\\n> @@ -184,15 +183,12 @@ start_httpd() {\n>  \t\t>&3 2>&4\n>  \tif test $? -ne 0\n>  \tthen\n> -\t\ttrap 'die' EXIT\n>  \t\tcat \"$HTTPD_ROOT_PATH\"/error.log >&4 2>/dev/null\n>  \t\ttest_skip_or_die $GIT_TEST_HTTPD \"web server setup failed\"\n>  \tfi\n>  }\n>  \n>  stop_httpd() {\n> -\ttrap 'die' EXIT\n> -\n>  \t\"$LIB_HTTPD_PATH\" -d \"$HTTPD_ROOT_PATH\" \\\n>  \t\t-f \"$TEST_PATH/apache.conf\" $HTTPD_PARA -k stop\n>  }\n\n... I see we lost many \"trap 'die' EXIT\" in the orignal.  Is that\nsomething we want to lose as part of this commit?  It does not make\nsense, at least to me, to add a \"test_atexit die\" and I am mostly\nwondering what these traps were trying to do in the original.\n\n"},{"id":"371453","messageId":"xmqqva0m9ire.fsf@gitster-ct.c.googlers.com","threadId":"50729","inReplyTo":"xmqqd0mub0d8.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 05/11] tests: use 'test_atexit' to stop httpd","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-14T04:34:13Z","receivedAt":"2019-03-14T04:34:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> I see most of these changes are removal of stop_httpd because it is\n> done as part of start_httpd() to arrange it to be called at exit.\n>\n> But ...\n> ...\n> ... I see we lost many \"trap 'die' EXIT\" in the orignal.  Is that\n> something we want to lose as part of this commit?\n\nAnd the answer of course is that we cannot afford to, and it does\nnot make sense to, keep it, as one of the early thing die() does is\nto execute the atexit cleanup scriptlets.\n"},{"id":"371505","messageId":"20190314145026.GF28939@szeder.dev","threadId":"50729","inReplyTo":"xmqq1s3aci66.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 01/11] test-lib: fix interrupt handling with 'dash' and '--verbose-log -x'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-03-14T14:50:26Z","receivedAt":"2019-03-14T14:50:32Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Mar 14, 2019 at 11:18:41AM +0900, Junio C Hamano wrote:\n> SZEDER Gábor <szeder.dev@gmail.com> writes:\n> \n> > Putting these together, when a test script run with 'dash' and\n> > '--verbose-log -x' is interrupted, then 'dash' tries to write the\n> > trace output from the EXIT trap to the script's original standard\n> > error, but it very likely can't, because the 'tee' downstream of the\n> > pipe is interrupted as well.  This causes the shell running the test\n> > script to die because of SIGPIPE, without running any of the commands\n> > in the EXIT trap.\n> \n> So... the clean-up actions do not get a chance to run because the\n> shell that is going to execute would be killed by SIGPIPE because\n> tee that is reading from it goes away?\n\nYes.\n\n> While I like the patch very much, I also wonder if it is possible to\n> tell tee to not to die upon seeing INT, TERM, etc.\n\n'tee -i' ignores only INT, but not TERM and HUP.\n\nWhat could work is something like:\n\n  <re-executing the test script> | (\n        # Ignore common interrupt signals to prevent 'tee' from dying\n        # while the upstream test process still produces output.\n        trap '' INT TERM HUP\n        tee logfile\n  )\n\nbut I'm not sure what should happen with a process ignoring HUP when\nit still produces output to the terminal after that terminal has been\nclosed, i.e. this 'tee' process.  FWIW, In my quick test such a\nprocess continued happily without receiving SIGPIPE or other\nunpleasantness.\n\n>  When the shell\n> upstream of tee exits (after performing clean-up actions), tee that\n> is reading from it will exit, too, and will not be left behind (I do\n> not mean to say it that is a better alternative solution---I am just\n> trying to see if I read the problem correctly from the description\n> given above).\n> \n> > diff --git a/t/test-lib.sh b/t/test-lib.sh\n> > index 8665b0a9b6..db3875d1e4 100644\n> > --- a/t/test-lib.sh\n> > +++ b/t/test-lib.sh\n> > @@ -631,7 +631,10 @@ die () {\n> >  \n> >  GIT_EXIT_OK=\n> >  trap 'die' EXIT\n> > -trap 'exit $?' INT TERM HUP\n> > +# Disable '-x' tracing, because with some shells, notably dash, it\n> > +# prevents running the cleanup commands when a test script run with\n> > +# '--verbose-log -x' is interrupted.\n> > +trap '{ code=$?; set +x; } 2>/dev/null; exit $code' INT TERM HUP\n> >  \n> >  # The user-facing functions are loaded from a separate file so that\n> >  # test_perf subshells can have them too\n> \n> Thanks.\n"},{"id":"371512","messageId":"20190314151937.GG28939@szeder.dev","threadId":"50729","inReplyTo":"xmqqd0mub0d8.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 05/11] tests: use 'test_atexit' to stop httpd","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-03-14T15:19:37Z","receivedAt":"2019-03-14T15:19:43Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Mar 14, 2019 at 12:28:35PM +0900, Junio C Hamano wrote:\n> SZEDER Gábor <szeder.dev@gmail.com> writes:\n\n> I see most of these changes are removal of stop_httpd because it is\n> done as part of start_httpd() to arrange it to be called at exit.\n> \n> But ...\n> \n> > @@ -176,7 +175,7 @@ prepare_httpd() {\n> >  start_httpd() {\n> >  \tprepare_httpd >&3 2>&4\n> >  \n> > -\ttrap 'code=$?; stop_httpd; (exit $code); die' EXIT\n> > +\ttest_atexit stop_httpd\n> >  \n> >  \t\"$LIB_HTTPD_PATH\" -d \"$HTTPD_ROOT_PATH\" \\\n> >  \t\t-f \"$TEST_PATH/apache.conf\" $HTTPD_PARA \\\n> > @@ -184,15 +183,12 @@ start_httpd() {\n> >  \t\t>&3 2>&4\n> >  \tif test $? -ne 0\n> >  \tthen\n> > -\t\ttrap 'die' EXIT\n> >  \t\tcat \"$HTTPD_ROOT_PATH\"/error.log >&4 2>/dev/null\n> >  \t\ttest_skip_or_die $GIT_TEST_HTTPD \"web server setup failed\"\n> >  \tfi\n> >  }\n> >  \n> >  stop_httpd() {\n> > -\ttrap 'die' EXIT\n> > -\n> >  \t\"$LIB_HTTPD_PATH\" -d \"$HTTPD_ROOT_PATH\" \\\n> >  \t\t-f \"$TEST_PATH/apache.conf\" $HTTPD_PARA -k stop\n> >  }\n> \n> ... I see we lost many \"trap 'die' EXIT\" in the orignal.  Is that\n> something we want to lose as part of this commit?  It does not make\n> sense, at least to me, to add a \"test_atexit die\" and I am mostly\n> wondering what these traps were trying to do in the original.\n\nIt restored our test framework's default EXIT trap, because without it\n'stop_httpd' would have been invoked after 'test_done' as well.  While\ninvoking it twice is probably not that big of a deal (though arguably\nnot the cleanest solution), invoking it after 'test_done' is\ndefinitely bad, because at that point the trash directory and thus\n$HTTPD_ROOT_PATH has already been removed, resulting in an ugly error:\n\n  $ ./t5561-http-backend.sh\n  <....>\n  ok 14 - server request log matches test results\n  # passed all 14 test(s)\n  1..14\n  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\n\n'test_atexit' doesn't overwrite the test frameowork's default EXIT\ntrap, so we don't have to restore anything.\n\n"},{"id":"371528","messageId":"20190314174024.GH28939@szeder.dev","threadId":"50729","inReplyTo":"xmqqh8c6b0pv.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 03/11] test-lib: introduce 'test_atexit'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-03-14T17:40:24Z","receivedAt":"2019-03-14T17:40:31Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Mar 14, 2019 at 12:21:00PM +0900, Junio C Hamano wrote:\n> SZEDER Gábor <szeder.dev@gmail.com> writes:\n> \n> > +test_atexit () {\n> > +\t# We cannot detect when we are in a subshell in general, but by\n> > +\t# doing so on Bash is better than nothing (the test will\n> > +\t# silently pass on other shells).\n> > +\ttest \"${BASH_SUBSHELL-0}\" = 0 ||\n> > +\terror \"bug in test script: test_atexit does nothing in a subshell\"\n> > +\ttest_atexit_cleanup=\"{ $*\n> > +\t\t} && (exit \\\"\\$eval_ret\\\"); eval_ret=\\$?; $test_atexit_cleanup\"\n> > +}\n> \n> This chaining is modelled after how $test_cleaup is built and\n> maintained by test_when_finished.  Use of eval_ret makes sense in\n> that original context as eval_ret _is_ used to keep track of the\n> result of 'test_eval_ \"$1\"' in test_run_ that executed the body\n> of a single test_expect_$something, and $test_cleanup would want to\n> keep the resulting status from that body when clean-up succeeds (or\n> otherwise, make that failure to clean-up visible as $eval_ret).\n> \n> But does it make sense in the context of the whole test script to\n> try maintaining $eval_ret?\n\nRight, it doesn't, as 'die' preserves the last seen exit code, and any\nexit codes from the atexit commands are ignored anyway.\n\n> >  # Most tests can use the created repository, but some may need to create more.\n> >  # Usage: test_create_repo <directory>\n> >  test_create_repo () {\n> > diff --git a/t/test-lib.sh b/t/test-lib.sh\n> > index db3875d1e4..b35881696f 100644\n> > --- a/t/test-lib.sh\n> > +++ b/t/test-lib.sh\n> > @@ -620,6 +620,10 @@ test_external_has_tap=0\n> >  \n> >  die () {\n> >  \tcode=$?\n> > +\t# This is responsible for running the atexit commands even when a\n> > +\t# test script run with '--immediate' fails, or when the user hits\n> > +\t# ctrl-C, i.e. when 'test_done' is not invoked at all.\n> > +\ttest_atexit_handler || code=$?\n> >  \tif test -n \"$GIT_EXIT_OK\"\n> >  \tthen\n> >  \t\texit $code\n> > @@ -1045,9 +1049,28 @@ write_junit_xml_testcase () {\n> >  \tjunit_have_testcase=t\n> >  }\n> >  \n> > +test_atexit_cleanup=:\n> > +test_atexit_handler () {\n> > +\t# In a succeeding test script 'test_atexit_handler' is invoked\n> > +\t# twice: first from 'test_done', then from 'die' in the trap on\n> > +\t# EXIT.\n> \n> We are guaranteed to still have the trash directory when we are run\n> in the exit handler after getting interrupted or test_failure_()\n> exits under the \"-i\" option, and when test_done() calls us.  What\n> will cause us trouble is the exit handler at the end of a successful\n> run after test_done() finishes.  At that point, test_done would have\n> already cleared the trash directory, so we may not have enough state\n> to allow us to clean-up at exit.\n> \n> Clearing the exit trap in test_done after it calls us might be an\n> alternative, but I think it is equivalent to clearing the\n> test_atexit_cleanup variable, and it is cleaner, so I think I agree\n> with the approach this patch uses.\n> \n> > +\t# This condition and resetting 'test_atexit_cleanup' below makes\n> > +\t# sure that the registered cleanup commands are run only once.\n> > +\ttest : != \"$test_atexit_cleanup\" || return 0\n> \n> I think test_when_finished uses a special value in $test_cleanup in\n> a similar way\n\nThat's right.\n\n> but it even skips when there is no point doing the\n> test_eval_ of the \"accumulated\" scriptlet when it is empty.\n\nBut this is not, because $test_cleanup is initialized to this special\nvalue and it can never be empty, and indeed 'test_eval_' uses this\ncondition:\n\n  if test -z \"$immediate\" || test $eval_ret = 0 ||\n     test -n \"$expecting_failure\" && test \"$test_cleanup\" != \":\"\n\nand it never checks $test_cleanup's emptiness.\n\n> Shouldn't we be doing the same thing, i.e.\n> \n> \tif test -z \"$test_atexit_cleanup\"\n> \tthen\n> \t\treturn 0\n> \tfi\n> \t... do the heavy lifting ...\n> \ttest_atexit_cleanup=\n> \n> That will make the handler truly a no-op when there is no atexit\n> defined.\n\n$test_atexit_cleanup is used the same way as $test_cleanup; it's\ninitialized to the same special value and can never be empty, so there\nis no need to check for its emptiness either.\n\n> > +\tsetup_malloc_check\n> > +\ttest_eval_ \"$test_atexit_cleanup\"\n> > +\ttest_atexit_cleanup=:\n> > +\tteardown_malloc_check\n> > +}\n> > +\n> >  test_done () {\n> >  \tGIT_EXIT_OK=t\n> >  \n> > +\t# Run the atexit commands _before_ the trash directory is\n> > +\t# removed, so the commands can access pidfiles and socket files.\n> > +\ttest_atexit_handler\n> > +\n> >  \tif test -n \"$write_junit_xml\" && test -n \"$junit_xml_path\"\n> >  \tthen\n> >  \t\ttest -n \"$junit_have_testcase\" || {\n"},{"id":"371758","messageId":"xmqqwokx6jdo.fsf@gitster-ct.c.googlers.com","threadId":"50729","inReplyTo":"20190314174024.GH28939@szeder.dev","subject":"Re: [PATCH 03/11] test-lib: introduce 'test_atexit'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-18T01:50:27Z","receivedAt":"2019-03-18T01:50:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor <szeder.dev@gmail.com> writes:\n\n>> but it even skips when there is no point doing the\n>> test_eval_ of the \"accumulated\" scriptlet when it is empty.\n>\n> But this is not, because $test_cleanup is initialized to this special\n> value and it can never be empty, and indeed 'test_eval_' uses this\n> condition:\n>\n>   if test -z \"$immediate\" || test $eval_ret = 0 ||\n>      test -n \"$expecting_failure\" && test \"$test_cleanup\" != \":\"\n>\n> and it never checks $test_cleanup's emptiness.\n\nYeah, I used \"empty\" not in the literal sense; I know why the \"empty\nin spirit\" setting is a single colon for $test_cleanup.\n\nI just did not realize that this new variable was using that exact\npattern and using \":\" as the empty in spirit, and that was where my\nfuzzy wordibng came from.\n\nSo in short, I misread the code, and part of that is because I was\nmisled by the comment:\n\n> +\t# This condition and resetting 'test_atexit_cleanup' below makes\n> +\t# sure that the registered cleanup commands are run only once.\n> +\ttest : != \"$test_atexit_cleanup\" || return 0\n\nIt over-stresses the \"run only once\", but the true value of this is\nthat it avoids running an \"empty in spirit\" clean-up sequence.  \n\nThe avoidance of double execution merely takes advantage of this\nimplementation detail by \"resetting\" the variable is better\nexplained where the \"resetting\" happens (i.e. \"we reset the variable\nto the 'no-command' state, as we've run all of them here, but just\nbefore the process finally exits, the helper will be called and we\ndo not want to run these commands again when it happens\").\n\nThanks.\n"}]}