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

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

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