{"thread":{"id":"50744","subject":"[PATCH] perf-lib.sh: rely on test-lib.sh for --tee handling","startedAt":"2019-03-15T06:15:35Z","lastAt":"2019-03-15T06:15:35Z","messageCount":1,"participants":["Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"371564","messageId":"20190315061416.GA13068@sigill.intra.peff.net","threadId":"50744","inReplyTo":null,"subject":"[PATCH] perf-lib.sh: rely on test-lib.sh for --tee handling","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-15T06:14:17Z","receivedAt":"2019-03-15T06:15:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since its inception, the perf-lib.sh script has manually handled the\n\"--tee\" option (and other options which imply it, like \"--valgrind\")\nwith a cut-and-pasted block from test-lib.sh. That block has grown stale\nover the years, and has at least three problems:\n\n  1. It uses $SHELL to re-exec the script, whereas the version in\n     test-lib.sh learned to use $TEST_SHELL_PATH.\n\n  2. It does an ad-hoc search of the \"$*\" string, whereas test-lib.sh\n     learned to carefully parse the arguments left to right.\n\n  3. It never learned about --verbose-log (which also implies --tee),\n     so it would not trigger for that option.\n\nThis last one was especially annoying, because t/perf/run uses the\nGIT_TEST_OPTS from your config.mak to run the perf scripts. So if you've\nset, say, \"-x --verbose-log\" there, it will be passed as part of most\nperf runs. And while this script doesn't recognize the option, the\ntest-lib.sh that we source _does_, and the behavior ends up being much\nmore annoying:\n\n  - as the comment at the top of the block says, we have to run this\n    tee code early, before we start munging variables (it says\n    GIT_BUILD_DIR, but the problematic variable is actually\n    GIT_TEST_INSTALLED).\n\n  - since we don't recognize --verbose-log, we don't trigger the block.\n    We go on to munge GIT_TEST_INSTALLED, converting it from a relative\n    to an absolute path.\n\n  - then we source test-lib.sh, which _does_ recognize --verbose-log. It\n    re-execs the script, which runs again. But this time with an\n    absolute version of GIT_TEST_INSTALLED.\n\n  - As a result, we copy the absolute version of GIT_TEST_INSTALLED into\n    perf_results_prefix. Instead of writing our results to the expected\n    \"test-results/build_1234abcd.p1234-whatever.times\", we instead write\n    them to \"test-results/_full_path_to_repo_t_perf_build_1234...\".\n\n    The aggregate.perl script doesn't expect this, and so it prints\n    \"<missing>\" for each result (even though it spent considerable time\n    running the tests!).\n\nWe can solve all of these in one blow by just deleting our custom\nhandling, and relying on the inclusion of test-lib.sh to handle --tee,\n--verbose-log, etc.\n\nThere's one catch, though. We want to handle GIT_TEST_INSTALLED after\nwe've included test-lib.sh, since we want it un-munged in the re-exec'd\nversion of the script. But if we want to convert it from a relative\nto an absolute path, we must do so before we load test-lib.sh, since it\nwill change our working directory. So we compute the absolute directory\nfirst, store it away, then include test-lib.sh, and finally assign to\nGIT_TEST_INSTALLED as appropriate.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis has been bugging me for months, so I finally tracked it down\nearlier today.\n\nI recommend viewing the diff with \"--color-moved\".\n\n t/perf/perf-lib.sh | 34 +++++++++++-----------------------\n 1 file changed, 11 insertions(+), 23 deletions(-)\n\ndiff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\nindex 2e33ab3ec3..169f92eae3 100644\n--- a/t/perf/perf-lib.sh\n+++ b/t/perf/perf-lib.sh\n@@ -17,37 +17,25 @@\n # You should have received a copy of the GNU General Public License\n # along with this program.  If not, see http://www.gnu.org/licenses/ .\n \n-# do the --tee work early; it otherwise confuses our careful\n-# GIT_BUILD_DIR mangling\n-case \"$GIT_TEST_TEE_STARTED, $* \" in\n-done,*)\n-\t# do not redirect again\n-\t;;\n-*' --tee '*|*' --va'*)\n-\tmkdir -p test-results\n-\tBASE=test-results/$(basename \"$0\" .sh)\n-\t(GIT_TEST_TEE_STARTED=done ${SHELL-sh} \"$0\" \"$@\" 2>&1;\n-\t echo $? > $BASE.exit) | tee $BASE.out\n-\ttest \"$(cat $BASE.exit)\" = 0\n-\texit\n-\t;;\n-esac\n-\n+# These variables must be set before the inclusion of test-lib.sh below,\n+# because it will change our working directory.\n TEST_DIRECTORY=$(pwd)/..\n TEST_OUTPUT_DIRECTORY=$(pwd)\n-if test -z \"$GIT_TEST_INSTALLED\"; then\n-\tperf_results_prefix=\n-else\n-\tperf_results_prefix=$(printf \"%s\" \"${GIT_TEST_INSTALLED%/bin-wrappers}\" | tr -c \"[a-zA-Z0-9]\" \"[_*]\")\".\"\n-\t# make the tested dir absolute\n-\tGIT_TEST_INSTALLED=$(cd \"$GIT_TEST_INSTALLED\" && pwd)\n-fi\n+ABSOLUTE_GIT_TEST_INSTALLED=$(\n+\ttest -n \"$GIT_TEST_INSTALLED\" && cd \"$GIT_TEST_INSTALLED\" && pwd)\n \n TEST_NO_CREATE_REPO=t\n TEST_NO_MALLOC_CHECK=t\n \n . ../test-lib.sh\n \n+if test -z \"$GIT_TEST_INSTALLED\"; then\n+\tperf_results_prefix=\n+else\n+\tperf_results_prefix=$(printf \"%s\" \"${GIT_TEST_INSTALLED%/bin-wrappers}\" | tr -c \"[a-zA-Z0-9]\" \"[_*]\")\".\"\n+\tGIT_TEST_INSTALLED=$ABSOLUTE_GIT_TEST_INSTALLED\n+fi\n+\n # Variables from test-lib that are normally internal to the tests; we\n # need to export them for test_perf subshells\n export TEST_DIRECTORY TRASH_DIRECTORY GIT_BUILD_DIR GIT_TEST_CMP\n-- \n2.21.0.543.g90eed137f3\n"}]}