git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2 0/2] perf-lib.sh: fix 0baf78e7bc regression, refactor & fix bugs

From
Jeff King <peff@peff.net>
Date
May 7, 2019, 07:06 UTC
Message-ID
<20190507070622.GA28060@sigill.intra.peff.net>
In-Reply-To
<20190506232309.28538-1-avarab@gmail.com>
On Tue, May 07, 2019 at 01:23:07AM +0200, Ævar Arnfjörð Bjarmason wrote:
Show 19 quoted lines
> > What if instead we:
> >
> >   - taught test-lib.sh to make GIT_TEST_INSTALLED absolute (since after
> >     all it is the one who is planning to chdir and wreck the relative
> >     path)
> >
> >   - let callers pass in $GIT_PERF_RESULTS_PREFIX instead of guessing at
> >     it ourselves from the path name. Then the "run" script could quite
> >     reasonably just pass in the tree oid it already has instead of us
> >     trying to decode it. And nobody would care about whether
> >     $GIT_TEST_INSTALLED has been mangled.
> >
> > I thought about going this route for my original patch, but I wanted to
> > fix the regression (which I agree is quite serious and embarrassing) as
> > quickly and simply as possible.
> 
> It seems simplest after the changes I've made here to just make a
> relative GIT_TEST_INSTALLED be an error in test-lib.sh, why bend over
> backwards to support it?

I'm OK with that, though _somebody_ has to generate the full path. My thinking was that it would be a little nicer if we did at the lowest level, but it's pretty easy to do it directly in the "run" script.

Show 6 quoted lines
> Re GIT_PERF_RESULTS_PREFIX: Depending on what you mean we now have
> that in 2/2 as PERF_RESULTS_PREFIX.
> 
> But if you mean the user can pass it in that doesn't make sense, since
> we need to pick a different prefix revision we test, so it's a
> many-to-many relationship.

I couldn't quite parse this. What I meant was that any caller of the perf scripts would pass in two pieces of data: the path to the git we want to test (GIT_TEST_INSTALLED) and the unique name under which it should be saved (GIT_PERF_PREFIX, which would generally be either empty or the tree oid).

I see that perf-lib.sh also has support for running git out of some arbitrary installed directory. I'm not sure if anybody actually uses that, but it's not too hard to support either (the run script just generates the prefix).

Something like (just for illustration on top of master; this doesn't have the actual bugfix):

diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
index 169f92eae3..e2b342f216 100644
--- a/t/perf/perf-lib.sh
+++ b/t/perf/perf-lib.sh
@@ -29,10 +29,7 @@ TEST_NO_MALLOC_CHECK=t
 
 . ../test-lib.sh
 
-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]" "[_*]")"."
+if test -n "$GIT_TEST_INSTALLED"; then
 	GIT_TEST_INSTALLED=$ABSOLUTE_GIT_TEST_INSTALLED
 fi
 
@@ -179,7 +176,8 @@ test_wrapper_ () {
 		base=$(basename "$0" .sh)
 		echo "$test_count" >>"$perf_results_dir"/$base.subtests
 		echo "$1" >"$perf_results_dir"/$base.$test_count.descr
-		base="$perf_results_dir"/"$perf_results_prefix$(basename "$0" .sh)"."$test_count"
+		prefix=$PERF_RESULTS_PREFIX${PERF_RESULTS_PREFIX:+.}
+		base="$perf_results_dir"/"$prefix$(basename "$0" .sh)"."$test_count"
 		"$test_wrapper_func_" "$@"
 	fi
 
diff --git a/t/perf/run b/t/perf/run
index 9aaa733c77..e5695ec8de 100755
--- a/t/perf/run
+++ b/t/perf/run
@@ -87,15 +87,20 @@ run_dirs_helper () {
 		fi
 		build_git_rev $rev "$mydir"
 		mydir=build/$rev
+		PERF_RESULTS_PREFIX=$rev
+	else
+		PERF_RESULTS_PREFIX=$(echo mydir | tr -c "[a-zA-Z0-9]" "[_*]")"."
 	fi
 	if test "$mydir" = .; then
 		unset GIT_TEST_INSTALLED
+		unset PERF_RESULTS_PREFIX
 	else
 		GIT_TEST_INSTALLED="$mydir/bin-wrappers"
 		# Older versions of git lacked bin-wrappers; fallback to the
 		# files in the root.
 		test -d "$GIT_TEST_INSTALLED" || GIT_TEST_INSTALLED=$mydir
 		export GIT_TEST_INSTALLED
+		export PERF_RESULTS_PREFIX
 	fi
 	run_one_dir "$@"
 }

It does mean that anybody calling "GIT_TEST_INSTALLED=whatever
./p1234-foo.sh" has to pass in PERF_RESULTS_PREFIX, but I think that's
OK (they should be using the "run" script anyway).

> As seen in 2/2 modifying some of the shell & associated Perl code it's
> a bit nasty that we need to duplicate the logic for picking these
> PERF_RESULTS_PREFIXes in the shell code and Perl code. Ideally the
> shell code would pick it, and pass the mapping to the Perl code
> somehow. But that's another "has sucked since forever, future TODO"
> item.

Yeah, I didn't realize that aggregate.perl has the duplicate logic.
Gross. But I agree that's not a topic for now. It would continue to
compute the same PERF_RESULTS_PREFIX that "run" does, except it doesn't
need to bother to pass it to anybody else.

(Note that in my patch above, I dropped the pointless "build_" from the
rev-based names, so aggregate.perl would need to do the same).

-Peff
Previous: Ævar Arnfjörð BjarmasonNext: Ævar Arnfjörð Bjarmason
Message 11 of 38 in “What's cooking in git.git (Apr 2019, #05; Thu, 25)”
  1. Junio C HamanoApr 25, 2019
  2. Elijah NewrenApr 25, 2019
  3. js/partial-clone-connectivity-check (was: What's cooking in git.git (Apr 2019, #05; Thu, 25))Josh Steadmon, Apr 25, 2019
  4. Jeff KingMay 2, 2019
  5. Josh SteadmonMay 2, 2019
  6. Jeff KingMay 2, 2019
  7. Jeff KingMay 2, 2019
  8. perf-lib.sh: make "./run <revisions>" use the correct gitsÆvar Arnfjörð Bjarmason, May 6, 2019
  9. Jeff KingMay 6, 2019
  10. 0/2 perf-lib.sh: fix 0baf78e7bc regression, refactor & fix bugsÆvar Arnfjörð Bjarmason, May 6, 2019
  11. Jeff KingMay 7, 2019
  12. 1/6 perf README: correct docs for 3c8f12c96c regressionÆvar Arnfjörð Bjarmason, May 7, 2019
  13. 3/6 perf-lib.sh: make "./run <revisions>" use the correct gitsÆvar Arnfjörð Bjarmason, May 7, 2019
  14. 2/6 perf aggregate: remove GIT_TEST_INSTALLED from --codespeedÆvar Arnfjörð Bjarmason, May 7, 2019
  15. 4/6 perf-lib.sh: remove GIT_TEST_INSTALLED from perf-lib.shÆvar Arnfjörð Bjarmason, May 7, 2019
  16. 5/6 perf tests: add "bindir" prefix to git tree test resultsÆvar Arnfjörð Bjarmason, May 7, 2019
  17. 0/6 perf-lib.sh: fix 0baf78e7bc regression, refactor & fix bugsÆvar Arnfjörð Bjarmason, May 7, 2019
  18. Jeff KingMay 7, 2019
  19. Junio C HamanoMay 8, 2019
  20. 6/6 perf-lib.sh: forbid the use of GIT_TEST_INSTALLEDÆvar Arnfjörð Bjarmason, May 7, 2019
  21. 1/2 perf-lib.sh: make "./run <revisions>" use the correct gitsÆvar Arnfjörð Bjarmason, May 6, 2019
  22. Jeff KingMay 7, 2019
  23. 2/2 perf-lib.sh: remove GIT_TEST_INSTALLED from perf-lib.shÆvar Arnfjörð Bjarmason, May 6, 2019
  24. Jeff KingMay 7, 2019
  25. Ævar Arnfjörð BjarmasonMay 7, 2019
  26. Jeff KingMay 7, 2019
  27. Taylor BlauApr 26, 2019
  28. Junio C HamanoApr 26, 2019
  29. Taylor BlauApr 26, 2019
  30. Junio C HamanoApr 26, 2019
  31. Kaartic SivaraamApr 26, 2019
  32. Junio C HamanoApr 27, 2019
  33. Kaartic SivaraamApr 27, 2019
  34. Jeff KingMay 2, 2019
  35. js/macos-gettext-build, was Re: What's cooking in git.git (Apr 2019, #05; Thu, 25)Johannes Schindelin, Apr 29, 2019
  36. Junio C HamanoMay 5, 2019
  37. Duy NguyenMay 6, 2019
  38. Junio C HamanoMay 7, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.