Re: js/partial-clone-connectivity-check (was: What's cooking in git.git (Apr 2019, #05; Thu, 25))

[Jeff King <peff@peff.net>]

On Thu, May 02, 2019 at 05:45:09PM -0400, Jeff King wrote:

> Here's what I came up with. Note that there's a bug in 'master' right
> now which causes perf to produce nonsense results. It's due to my
> 0baf78e7bc (perf-lib.sh: rely on test-lib.sh for --tee handling,
> 2019-03-15). I'll fix that separately (the timing below is done with
> that commit reverted).

And here's the fix for that. It's rather subtle, so I hope I explained
it sufficiently. I didn't notice it while working on the original
because everything _appears_ to run fine, but you just get timings from
the wrong version of Git. Which is only noticeable if you're literally
testing two versions that you expect to differ.

-- >8 --
Subject: [PATCH] t/perf: set GIT_TEST_INSTALLED before including test-lib.sh

Commit 0baf78e7bc (perf-lib.sh: rely on test-lib.sh for --tee handling,
2019-03-15) introduced a bug which causes perf test runs to always find
git in the $PATH, rather than actually testing $GIT_TEST_INSTALLED
(which is used by the "run" script).

That commit bumped the conversion of $GIT_TEST_INSTALLED to an absolute
path until after test-lib.sh was been included. That solved the original
problem of generating a bogus $perf_results_prefix value from the
absolute path. But it introduced a new one: test-lib.sh needs to see
that absolute value because it will put it at the front of the $PATH.

The root of the issue is that we need both the relative and the absolute
paths available. That earlier commit chose to stuff the absolute path
into $ABSOLUTE_GIT_TEST_INSTALLED, keep the relative one in
$GIT_TEST_INSTALLED, and then resolve the two after test-lib.sh has been
loaded. We can fix it by reversing our strategy: we'll keep the absolute
path in $GIT_TEST_INSTALLED, drop our now-useless ABSOLUTE variable, and
introduce a new $TEST_INSTALLED_ORIG variable to hold the original value
(from which we will later derive the prefix).

Note that we need to export our ORIG variable because the whole point of
0baf78e7bc is that test-lib.sh may actually re-exec a new copy of the
script to handle tee-ing the output.

I'd hoped to add a test to p0000-perf-lib-sanity to cover this, but the
bug is actually outside that scope. It triggers if you do something
like:

  cat >t/perf/p1234-foo.sh <<-\EOF
  #!/bin/sh
  test_description=debugging
  . ./perf-lib.sh
  test_expect_success 'version' 'which git && git version'
  test_perf 'fake timing' 'true'
  test_done
  EOF

  chmod +x t/perf/p1234-foo.sh
  make GIT_TEST_OPTS=--verbose-log
  cd t/perf
  ./run origin p1234-foo.sh
  cat test-results/p1234-foo.out

Without this patch, you'll see that we're running whatever git is in
your $PATH, not the one from "origin". With this patch, we should see:

  - the correct git is run

  - we get timing results out of ./run, showing that the prefix bug from
    0baf78e7bc was not regressed

  - our values make it across the script re-exec due to --verbose-log

Signed-off-by: Jeff King <peff@peff.net>
---
 t/perf/perf-lib.sh | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
index 169f92eae3..8317dbef44 100644
--- a/t/perf/perf-lib.sh
+++ b/t/perf/perf-lib.sh
@@ -21,7 +21,11 @@
 # because it will change our working directory.
 TEST_DIRECTORY=$(pwd)/..
 TEST_OUTPUT_DIRECTORY=$(pwd)
-ABSOLUTE_GIT_TEST_INSTALLED=$(
+
+# Likewise we must turn GIT_TEST_INSTALLED into an absolute path; but remember
+# and export the original value, since we'll later generate our prefix from it.
+: ${TEST_INSTALLED_ORIG=$GIT_TEST_INSTALLED}; export TEST_INSTALLED_ORIG
+GIT_TEST_INSTALLED=$(
 	test -n "$GIT_TEST_INSTALLED" && cd "$GIT_TEST_INSTALLED" && pwd)
 
 TEST_NO_CREATE_REPO=t
@@ -32,8 +36,7 @@ TEST_NO_MALLOC_CHECK=t
 if test -z "$GIT_TEST_INSTALLED"; then
 	perf_results_prefix=
 else
-	perf_results_prefix=$(printf "%s" "${GIT_TEST_INSTALLED%/bin-wrappers}" | tr -c "[a-zA-Z0-9]" "[_*]")"."
-	GIT_TEST_INSTALLED=$ABSOLUTE_GIT_TEST_INSTALLED
+	perf_results_prefix=$(printf "%s" "${TEST_INSTALLED_ORIG%/bin-wrappers}" | tr -c "[a-zA-Z0-9]" "[_*]")"."
 fi
 
 # Variables from test-lib that are normally internal to the tests; we
-- 
2.21.0.1314.g224b191707