list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH] rely on for --tee handling
@ 2019-03-15  6:14 Jeff King
  0 siblings, 0 replies; only message in thread
From: Jeff King @ 2019-03-15  6:14 UTC (permalink / raw)
  To: git

Since its inception, the script has manually handled the
"--tee" option (and other options which imply it, like "--valgrind")
with a cut-and-pasted block from That block has grown stale
over the years, and has at least three problems:

  1. It uses $SHELL to re-exec the script, whereas the version in learned to use $TEST_SHELL_PATH.

  2. It does an ad-hoc search of the "$*" string, whereas
     learned to carefully parse the arguments left to right.

  3. It never learned about --verbose-log (which also implies --tee),
     so it would not trigger for that option.

This last one was especially annoying, because t/perf/run uses the
GIT_TEST_OPTS from your config.mak to run the perf scripts. So if you've
set, say, "-x --verbose-log" there, it will be passed as part of most
perf runs. And while this script doesn't recognize the option, the that we source _does_, and the behavior ends up being much
more annoying:

  - as the comment at the top of the block says, we have to run this
    tee code early, before we start munging variables (it says
    GIT_BUILD_DIR, but the problematic variable is actually

  - since we don't recognize --verbose-log, we don't trigger the block.
    We go on to munge GIT_TEST_INSTALLED, converting it from a relative
    to an absolute path.

  - then we source, which _does_ recognize --verbose-log. It
    re-execs the script, which runs again. But this time with an
    absolute version of GIT_TEST_INSTALLED.

  - As a result, we copy the absolute version of GIT_TEST_INSTALLED into
    perf_results_prefix. Instead of writing our results to the expected
    "test-results/build_1234abcd.p1234-whatever.times", we instead write
    them to "test-results/_full_path_to_repo_t_perf_build_1234...".

    The aggregate.perl script doesn't expect this, and so it prints
    "<missing>" for each result (even though it spent considerable time
    running the tests!).

We can solve all of these in one blow by just deleting our custom
handling, and relying on the inclusion of to handle --tee,
--verbose-log, etc.

There's one catch, though. We want to handle GIT_TEST_INSTALLED after
we've included, since we want it un-munged in the re-exec'd
version of the script. But if we want to convert it from a relative
to an absolute path, we must do so before we load, since it
will change our working directory. So we compute the absolute directory
first, store it away, then include, and finally assign to
GIT_TEST_INSTALLED as appropriate.

Signed-off-by: Jeff King <>
This has been bugging me for months, so I finally tracked it down
earlier today.

I recommend viewing the diff with "--color-moved".

 t/perf/ | 34 +++++++++++-----------------------
 1 file changed, 11 insertions(+), 23 deletions(-)

diff --git a/t/perf/ b/t/perf/
index 2e33ab3ec3..169f92eae3 100644
--- a/t/perf/
+++ b/t/perf/
@@ -17,37 +17,25 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see .
-# do the --tee work early; it otherwise confuses our careful
-# GIT_BUILD_DIR mangling
-case "$GIT_TEST_TEE_STARTED, $* " in
-	# do not redirect again
-	;;
-*' --tee '*|*' --va'*)
-	mkdir -p test-results
-	BASE=test-results/$(basename "$0" .sh)
-	(GIT_TEST_TEE_STARTED=done ${SHELL-sh} "$0" "$@" 2>&1;
-	 echo $? > $BASE.exit) | tee $BASE.out
-	test "$(cat $BASE.exit)" = 0
-	exit
-	;;
+# These variables must be set before the inclusion of below,
+# because it will change our working directory.
-if test -z "$GIT_TEST_INSTALLED"; then
-	perf_results_prefix=
-	perf_results_prefix=$(printf "%s" "${GIT_TEST_INSTALLED%/bin-wrappers}" | tr -c "[a-zA-Z0-9]" "[_*]")"."
-	# make the tested dir absolute
+	test -n "$GIT_TEST_INSTALLED" && cd "$GIT_TEST_INSTALLED" && pwd)
 . ../
+if test -z "$GIT_TEST_INSTALLED"; then
+	perf_results_prefix=
+	perf_results_prefix=$(printf "%s" "${GIT_TEST_INSTALLED%/bin-wrappers}" | tr -c "[a-zA-Z0-9]" "[_*]")"."
 # Variables from test-lib that are normally internal to the tests; we
 # need to export them for test_perf subshells

^ permalink raw reply	[flat|nested] only message in thread

only message in thread, other threads:[~2019-03-15  6:15 UTC | newest]

Thread overview: (only message) (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-15  6:14 [PATCH] rely on for --tee handling Jeff King

Code repositories for project(s) associated with this inbox:

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).