{"thread":{"id":"64735","subject":"[PATCH 0/2] more t/perf meson/GIT-BUILD-OPTIONS fallout","startedAt":"2026-01-06T10:10:51Z","lastAt":"2026-01-16T16:54:54Z","messageCount":5,"participants":["Jeff King","Ramsay Jones"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"533119","messageId":"20260106101043.GA3723319@coredump.intra.peff.net","threadId":"64735","inReplyTo":null,"subject":"[PATCH 0/2] more t/perf meson/GIT-BUILD-OPTIONS fallout","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-01-06T10:10:43Z","receivedAt":"2026-01-06T10:10:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This series fixes two bugs when trying to use the t/perf/run script to\ncompare two versions of Git.\n\n  [1/2]: t/perf/perf-lib: fix assignment of TEST_OUTPUT_DIRECTORY\n  [2/2]: t/perf/run: preserve GIT_PERF_* from environment\n\n t/perf/perf-lib.sh |  3 ++-\n t/perf/run         | 10 ++++++++++\n 2 files changed, 12 insertions(+), 1 deletion(-)\n\n-Peff\n"},{"id":"533120","messageId":"20260106101349.GA3727538@coredump.intra.peff.net","threadId":"64735","inReplyTo":"20260106101043.GA3723319@coredump.intra.peff.net","subject":"[PATCH 1/2] t/perf/perf-lib: fix assignment of TEST_OUTPUT_DIRECTORY","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-01-06T10:13:49Z","receivedAt":"2026-01-06T10:13:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Using the perf suite's \"run\" helper in a vanilla build fails like this:\n\n  $ make && (cd t/perf && ./run p0000-perf-lib-sanity.sh)\n  === Running 1 tests in this tree ===\n  perf 1 - test_perf_default_repo works: 1 2 3 ok\n  perf 2 - test_checkout_worktree works: 1 2 3 ok\n  ok 3 - test_export works\n  perf 4 - export a weird var: 1 2 3 ok\n  perf 5 - éḿíẗ ńöń-ÁŚĆÍÍ ćḧáŕáćẗéŕś: 1 2 3 ok\n  ok 6 - test_export works with weird vars\n  perf 7 - important variables available in subshells: 1 2 3 ok\n  perf 8 - test-lib-functions correctly loaded in subshells: 1 2 3 ok\n  # passed all 8 test(s)\n  1..8\n  cannot open test-results/p0000-perf-lib-sanity.subtests: No such file or directory at ./aggregate.perl line 159.\n\nIt is trying to aggregate results written into t/perf/test-results, but\nthe p0000 script did not write anything there.\n\nThe \"run\" script looks in $TEST_OUTPUT_DIRECTORY/test-results, or if\nthat variable is not set, in test-results in the current working\ndirectory (which should be t/perf itself). It pulls the value of\n$TEST_OUTPUT_DIRECTORY from the GIT-BUILD-OPTIONS file.\n\nBut that doesn't quite match the setup in perf-lib.sh (which is what\nscripts like p0000 use). There we do this at the top of the script:\n\n  TEST_OUTPUT_DIRECTORY=$(pwd)\n\nand then let test-lib.sh append \"/test-results\" to that. Historically,\nthat made the vanilla case work: we'd always use t/perf/test-results.\nBut when $TEST_OUTPUT_DIRECTORY was set, it would break.\n\nCommit 5756ccd181 (t/perf: fix benchmarks with out-of-tree builds,\n2025-04-28) fixed that second case by loading GIT-BUILD-OPTIONS\nourselves. But that broke the vanilla case!\n\nNow our setting of $TEST_OUTPUT_DIRECTORY in perf-lib.sh is ignored,\nbecause it is overwritten by GIT-BUILD-OPTIONS. And when test-lib.sh\nsees that the output directory is empty, it defaults to t/test-results,\nrather than t/perf/test-results.\n\nNobody seems to have noticed, probably for two reasons:\n\n  1. It only matters if you're trying to aggregate results (like the\n     \"run\" script does). Just running \"./p0000-perf-lib-sanity.sh\"\n     manually still produces useful output; the stored result files are\n     just in an unexpected place.\n\n  2. There might be leftover files in t/perf/test-results from previous\n     runs (before 5756ccd181). In particular, the \".subtests\" files\n     don't tend to change, and the lack of that file is what causes it\n     to barf completely. So it's possible that the aggregation could\n     have been showing stale results that did not match the run that\n     just happened.\n\nWe can fix it by setting TEST_OUTPUT_DIRECTORY only after we've loaded\nGIT-BUILD-OPTIONS, so that we override its value and not the other way\naround. And we'll do so only when the variable is not set, which should\nretain the fix for that case from 5756ccd181.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/perf/perf-lib.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\nindex b15c74d6f1..2ac007888e 100644\n--- a/t/perf/perf-lib.sh\n+++ b/t/perf/perf-lib.sh\n@@ -20,7 +20,7 @@\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+perf_dir=$(pwd)\n \n TEST_NO_CREATE_REPO=t\n TEST_NO_MALLOC_CHECK=t\n@@ -58,6 +58,7 @@ then\n fi\n \n . \"$GIT_BUILD_DIR\"/GIT-BUILD-OPTIONS\n+: ${TEST_OUTPUT_DIRECTORY:=$perf_dir}\n . \"$GIT_SOURCE_DIR\"/t/test-lib.sh\n \n # Then restore GIT_PERF_* settings.\n-- \n2.52.0.664.g9f53c65b4c\n\n"},{"id":"533121","messageId":"20260106101604.GB3727538@coredump.intra.peff.net","threadId":"64735","inReplyTo":"20260106101043.GA3723319@coredump.intra.peff.net","subject":"[PATCH 2/2] t/perf/run: preserve GIT_PERF_* from environment","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-01-06T10:16:04Z","receivedAt":"2026-01-06T10:16:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If you run:\n\n  GIT_PERF_LARGE_REPO=/some/path ./p1006-cat-file.sh\n\nit will use the repo in /some/path. But if you use the \"run\" helper\nscript to aggregate and compare results, like this:\n\n  GIT_PERF_LARGE_REPO=/some/path ./run HEAD^ HEAD p1006-cat-file.sh\n\nit will ignore that variable. This is because the presence of the\nLARGE_REPO variable in GIT-BUILD-OPTIONS overrides what's in the\nenvironment. This started with 4638e8806e (Makefile: use common template\nfor GIT-BUILD-OPTIONS, 2024-12-06), which now writes even empty\nvariables (though arguably it was wrong even before with a non-empty\nvalue, as we generally prefer the environment to take precedence over\non-disk config).\n\nWe had the same problem in perf-lib.sh itself, and we hacked around it\nwith 32b74b9809 (perf: do allow `GIT_PERF_*` to be overridden again,\n2025-04-04). That's what lets the direct invocation of \"./p1006\" work\nabove.\n\nAnd in fact that was sufficient for \"./run\", too, until it started\nloading GIT-BUILD-OPTIONS itself in 5756ccd181 (t/perf: fix benchmarks\nwith out-of-tree builds, 2025-04-28). Now it has the same problem: it\nclobbers any incoming GIT_PERF options from the environment.\n\nWe can use the same hack here in the \"run\" script. It's quite ugly, but\nit's just short enough that I don't think it's worth trying to factor it\nout into a common shell library.\n\nIn the long run, we might consider teaching GIT-BUILD-OPTIONS to be more\ngentle in overwriting existing entries. There are probably other\nGIT_TEST_* variables which would need the same treatment. And if and\nwhen we come up with a more complete solution, we can use it in both\nspots.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/perf/run | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/t/perf/run b/t/perf/run\nindex 073bcb2aff..13913db4a3 100755\n--- a/t/perf/run\n+++ b/t/perf/run\n@@ -204,8 +204,18 @@ run_subsection () {\n get_var_from_env_or_config \"GIT_PERF_CODESPEED_OUTPUT\" \"perf\" \"codespeedOutput\" \"--bool\"\n get_var_from_env_or_config \"GIT_PERF_SEND_TO_CODESPEED\" \"perf\" \"sendToCodespeed\"\n \n+# Preserve GIT_PERF settings from the environment when loading\n+# GIT-BUILD-OPTIONS; see the similar hack in perf-lib.sh.\n+git_perf_settings=\"$(env |\n+        sed -n \"/^GIT_PERF_/{\n+                # escape all single-quotes in the value\n+                s/'/'\\\\\\\\''/g\n+                # turn this into an eval-able assignment\n+                s/^\\\\([^=]*=\\\\)\\\\(.*\\\\)/\\\\1'\\\\2'/p\n+        }\")\"\n cd \"$(dirname $0)\"\n . ../../GIT-BUILD-OPTIONS\n+eval \"$git_perf_settings\"\n \n if test -n \"$TEST_OUTPUT_DIRECTORY\"\n then\n-- \n2.52.0.664.g9f53c65b4c\n"},{"id":"533155","messageId":"1a430542-715e-4cf1-86f5-d9424951204a@ramsayjones.plus.com","threadId":"64735","inReplyTo":"20260106101043.GA3723319@coredump.intra.peff.net","subject":"Re: [PATCH 0/2] more t/perf meson/GIT-BUILD-OPTIONS fallout","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2026-01-06T17:07:11Z","receivedAt":"2026-01-06T17:10:23Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Hi Jeff,\n\nOn 06/01/2026 10:10 am, Jeff King wrote:\n> This series fixes two bugs when trying to use the t/perf/run script to\n> compare two versions of Git.\n> \n>   [1/2]: t/perf/perf-lib: fix assignment of TEST_OUTPUT_DIRECTORY\n>   [2/2]: t/perf/run: preserve GIT_PERF_* from environment\n> \n>  t/perf/perf-lib.sh |  3 ++-\n>  t/perf/run         | 10 ++++++++++\n>  2 files changed, 12 insertions(+), 1 deletion(-)\n\nI hesitated to send this email because I have been reduced to simply skimming\nthe git mailing list (very busy with other projects/real life!), and I may\nhave misunderstood what you aim to do here. ;)\n\nIn essence, I was triggered by the 'GIT-BUILD-OPTIONS fallout' phrase in the\nsubject line! That reminded me of a problem/patch I was looking at earlier\nthis (wait, last) year. The patch (below) was a complete 'hack' (as you can\nsee) to allow the environment to override the 'GIT-BUILD-OPTIONS' file. This\nwas in an old branch named 'meson-wip' which I have been meaning to look at\nagain to either delete or fix-up.\n\nOne of the many reasons (apart from being a disgusting hack) that I didn't\nprogress this patch is because I felt that not all 'options' in that file\nshould be able to be 'overridden'. So, that implies that the file needs to\nbe split into two; one file of options which can be overridden from the\nenvironment and one that can't. If so, then someone has to decide which is\nwhich.\n\n[I'm sure you could do a much better job than the patch below!]\n\nBTW, I can't remember why I wanted to do this anyway ... :)\n\nHopefully, this is not a complete waste of the list's time. If so, sorry in\nadvance!\n\nThanks.\n\nATB,\nRamsay Jones\n\n-------- >8 --------\nDate: Tue, 27 May 2025 21:53:49 +0100\nSubject: [PATCH] test-lib.sh: allow environment to override GIT-BUILD-OPTIONS\n\nSigned-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>\n---\n t/test-lib.sh | 13 ++++++++++++-\n 1 file changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 621cd31ae1..5239042b97 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -101,7 +101,18 @@ then\n \techo >&2 'error: GIT-BUILD-OPTIONS missing (has Git been built?).'\n \texit 1\n fi\n-. \"$GIT_BUILD_DIR\"/GIT-BUILD-OPTIONS\n+\n+# allow the environment to override the settings from GIT-BUILD-OPTIONS\n+while IFS== read var val\n+do\n+\te_val=$(eval echo '${'\"$var\"'}' 2>/dev/null)\n+\tif test -n \"$e_val\"\n+\tthen\n+\t\tval=\"'$e_val'\"\n+\tfi\n+\teval \"$var\"=$val\n+done < \"$GIT_BUILD_DIR\"/GIT-BUILD-OPTIONS\n+\n export PERL_PATH SHELL_PATH\n \n if test -z \"$TEST_OUTPUT_DIRECTORY\"\n-- \n2.52.0\n\n"},{"id":"534057","messageId":"20260116165451.GB1636797@coredump.intra.peff.net","threadId":"64735","inReplyTo":"1a430542-715e-4cf1-86f5-d9424951204a@ramsayjones.plus.com","subject":"Re: [PATCH 0/2] more t/perf meson/GIT-BUILD-OPTIONS fallout","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-01-16T16:54:51Z","receivedAt":"2026-01-16T16:54:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 06, 2026 at 05:07:11PM +0000, Ramsay Jones wrote:\n\n> I hesitated to send this email because I have been reduced to simply skimming\n> the git mailing list (very busy with other projects/real life!), and I may\n> have misunderstood what you aim to do here. ;)\n> \n> In essence, I was triggered by the 'GIT-BUILD-OPTIONS fallout' phrase in the\n> subject line! That reminded me of a problem/patch I was looking at earlier\n> this (wait, last) year. The patch (below) was a complete 'hack' (as you can\n> see) to allow the environment to override the 'GIT-BUILD-OPTIONS' file. This\n> was in an old branch named 'meson-wip' which I have been meaning to look at\n> again to either delete or fix-up.\n> \n> One of the many reasons (apart from being a disgusting hack) that I didn't\n> progress this patch is because I felt that not all 'options' in that file\n> should be able to be 'overridden'. So, that implies that the file needs to\n> be split into two; one file of options which can be overridden from the\n> environment and one that can't. If so, then someone has to decide which is\n> which.\n\nI think you understood my goal. :) This is more or less what my patch is\ndoing, but just for a select set of options (to un-break t/perf). I\nthink a larger fix may look something like this, but:\n\n  1. I agree with you that we may need to consider which options should\n     be able to be overridden and which should not.\n\n  2. This hack has to go everywhere that GIT-BUILD-OPTIONS is read. So\n     in test-lib.sh where you have it, but also in perf-lib.sh (matching\n     the fix by Dscho earlier) and also in t/perf/run (matching the fix\n     here).\n\nIt would be nice if we could write GIT-BUILD-OPTIONS in a way that did\nthe right thing. E.g., by writing:\n\n  : ${GIT_FOO:=some-value}\n\nAnd then the writer (which is the ultimate source of authority for which\nvariables are included) could decide which ones can be overridden.\n\nI _thought_ this wouldn't work because we also source the build options\nfiles from the Makefile (and so it has to support both syntaxes). But a\nquick grep doesn't show us including it. So maybe we used to do so, or\nmaybe I'm mis-remembering (and confusing it with GIT-VERSION-FILE\nperhaps?).\n\n-Peff\n"}]}