# [PATCH 0/2] more t/perf meson/GIT-BUILD-OPTIONS fallout 5 messages from 2026-01-06 to 2026-01-16. Participants: Jeff King, Ramsay Jones. Thread: https://gitlist.dev/t/64735 ## Jeff King, 2026-01-06 10:10 Subject: [PATCH 0/2] more t/perf meson/GIT-BUILD-OPTIONS fallout Message-ID: <20260106101043.GA3723319@coredump.intra.peff.net> URL: https://gitlist.dev/e/20260106101043.GA3723319%40coredump.intra.peff.net ``` This series fixes two bugs when trying to use the t/perf/run script to compare two versions of Git. [1/2]: t/perf/perf-lib: fix assignment of TEST_OUTPUT_DIRECTORY [2/2]: t/perf/run: preserve GIT_PERF_* from environment t/perf/perf-lib.sh | 3 ++- t/perf/run | 10 ++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) -Peff ``` ## Jeff King, 2026-01-06 10:13 Subject: [PATCH 1/2] t/perf/perf-lib: fix assignment of TEST_OUTPUT_DIRECTORY Message-ID: <20260106101349.GA3727538@coredump.intra.peff.net> URL: https://gitlist.dev/e/20260106101349.GA3727538%40coredump.intra.peff.net In-Reply-To: <20260106101043.GA3723319@coredump.intra.peff.net> ``` Using the perf suite's "run" helper in a vanilla build fails like this: $ make && (cd t/perf && ./run p0000-perf-lib-sanity.sh) === Running 1 tests in this tree === perf 1 - test_perf_default_repo works: 1 2 3 ok perf 2 - test_checkout_worktree works: 1 2 3 ok ok 3 - test_export works perf 4 - export a weird var: 1 2 3 ok perf 5 - éḿíẗ ńöń-ÁŚĆÍÍ ćḧáŕáćẗéŕś: 1 2 3 ok ok 6 - test_export works with weird vars perf 7 - important variables available in subshells: 1 2 3 ok perf 8 - test-lib-functions correctly loaded in subshells: 1 2 3 ok # passed all 8 test(s) 1..8 cannot open test-results/p0000-perf-lib-sanity.subtests: No such file or directory at ./aggregate.perl line 159. It is trying to aggregate results written into t/perf/test-results, but the p0000 script did not write anything there. The "run" script looks in $TEST_OUTPUT_DIRECTORY/test-results, or if that variable is not set, in test-results in the current working directory (which should be t/perf itself). It pulls the value of $TEST_OUTPUT_DIRECTORY from the GIT-BUILD-OPTIONS file. But that doesn't quite match the setup in perf-lib.sh (which is what scripts like p0000 use). There we do this at the top of the script: TEST_OUTPUT_DIRECTORY=$(pwd) and then let test-lib.sh append "/test-results" to that. Historically, that made the vanilla case work: we'd always use t/perf/test-results. But when $TEST_OUTPUT_DIRECTORY was set, it would break. Commit 5756ccd181 (t/perf: fix benchmarks with out-of-tree builds, 2025-04-28) fixed that second case by loading GIT-BUILD-OPTIONS ourselves. But that broke the vanilla case! Now our setting of $TEST_OUTPUT_DIRECTORY in perf-lib.sh is ignored, because it is overwritten by GIT-BUILD-OPTIONS. And when test-lib.sh sees that the output directory is empty, it defaults to t/test-results, rather than t/perf/test-results. Nobody seems to have noticed, probably for two reasons: 1. It only matters if you're trying to aggregate results (like the "run" script does). Just running "./p0000-perf-lib-sanity.sh" manually still produces useful output; the stored result files are just in an unexpected place. 2. There might be leftover files in t/perf/test-results from previous runs (before 5756ccd181). In particular, the ".subtests" files don't tend to change, and the lack of that file is what causes it to barf completely. So it's possible that the aggregation could have been showing stale results that did not match the run that just happened. We can fix it by setting TEST_OUTPUT_DIRECTORY only after we've loaded GIT-BUILD-OPTIONS, so that we override its value and not the other way around. And we'll do so only when the variable is not set, which should retain the fix for that case from 5756ccd181. Signed-off-by: Jeff King --- t/perf/perf-lib.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh index b15c74d6f1..2ac007888e 100644 --- a/t/perf/perf-lib.sh +++ b/t/perf/perf-lib.sh @@ -20,7 +20,7 @@ # These variables must be set before the inclusion of test-lib.sh below, # because it will change our working directory. TEST_DIRECTORY=$(pwd)/.. -TEST_OUTPUT_DIRECTORY=$(pwd) +perf_dir=$(pwd) TEST_NO_CREATE_REPO=t TEST_NO_MALLOC_CHECK=t @@ -58,6 +58,7 @@ then fi . "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS +: ${TEST_OUTPUT_DIRECTORY:=$perf_dir} . "$GIT_SOURCE_DIR"/t/test-lib.sh # Then restore GIT_PERF_* settings. -- 2.52.0.664.g9f53c65b4c ``` ## Jeff King, 2026-01-06 10:16 Subject: [PATCH 2/2] t/perf/run: preserve GIT_PERF_* from environment Message-ID: <20260106101604.GB3727538@coredump.intra.peff.net> URL: https://gitlist.dev/e/20260106101604.GB3727538%40coredump.intra.peff.net In-Reply-To: <20260106101043.GA3723319@coredump.intra.peff.net> ``` If you run: GIT_PERF_LARGE_REPO=/some/path ./p1006-cat-file.sh it will use the repo in /some/path. But if you use the "run" helper script to aggregate and compare results, like this: GIT_PERF_LARGE_REPO=/some/path ./run HEAD^ HEAD p1006-cat-file.sh it will ignore that variable. This is because the presence of the LARGE_REPO variable in GIT-BUILD-OPTIONS overrides what's in the environment. This started with 4638e8806e (Makefile: use common template for GIT-BUILD-OPTIONS, 2024-12-06), which now writes even empty variables (though arguably it was wrong even before with a non-empty value, as we generally prefer the environment to take precedence over on-disk config). We had the same problem in perf-lib.sh itself, and we hacked around it with 32b74b9809 (perf: do allow `GIT_PERF_*` to be overridden again, 2025-04-04). That's what lets the direct invocation of "./p1006" work above. And in fact that was sufficient for "./run", too, until it started loading GIT-BUILD-OPTIONS itself in 5756ccd181 (t/perf: fix benchmarks with out-of-tree builds, 2025-04-28). Now it has the same problem: it clobbers any incoming GIT_PERF options from the environment. We can use the same hack here in the "run" script. It's quite ugly, but it's just short enough that I don't think it's worth trying to factor it out into a common shell library. In the long run, we might consider teaching GIT-BUILD-OPTIONS to be more gentle in overwriting existing entries. There are probably other GIT_TEST_* variables which would need the same treatment. And if and when we come up with a more complete solution, we can use it in both spots. Signed-off-by: Jeff King --- t/perf/run | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/t/perf/run b/t/perf/run index 073bcb2aff..13913db4a3 100755 --- a/t/perf/run +++ b/t/perf/run @@ -204,8 +204,18 @@ run_subsection () { get_var_from_env_or_config "GIT_PERF_CODESPEED_OUTPUT" "perf" "codespeedOutput" "--bool" get_var_from_env_or_config "GIT_PERF_SEND_TO_CODESPEED" "perf" "sendToCodespeed" +# Preserve GIT_PERF settings from the environment when loading +# GIT-BUILD-OPTIONS; see the similar hack in perf-lib.sh. +git_perf_settings="$(env | + sed -n "/^GIT_PERF_/{ + # escape all single-quotes in the value + s/'/'\\\\''/g + # turn this into an eval-able assignment + s/^\\([^=]*=\\)\\(.*\\)/\\1'\\2'/p + }")" cd "$(dirname $0)" . ../../GIT-BUILD-OPTIONS +eval "$git_perf_settings" if test -n "$TEST_OUTPUT_DIRECTORY" then -- 2.52.0.664.g9f53c65b4c ``` ## Ramsay Jones, 2026-01-06 17:07 Subject: Re: [PATCH 0/2] more t/perf meson/GIT-BUILD-OPTIONS fallout Message-ID: <1a430542-715e-4cf1-86f5-d9424951204a@ramsayjones.plus.com> URL: https://gitlist.dev/e/1a430542-715e-4cf1-86f5-d9424951204a%40ramsayjones.plus.com In-Reply-To: <20260106101043.GA3723319@coredump.intra.peff.net> ``` Hi Jeff, On 06/01/2026 10:10 am, Jeff King wrote: > This series fixes two bugs when trying to use the t/perf/run script to > compare two versions of Git. > > [1/2]: t/perf/perf-lib: fix assignment of TEST_OUTPUT_DIRECTORY > [2/2]: t/perf/run: preserve GIT_PERF_* from environment > > t/perf/perf-lib.sh | 3 ++- > t/perf/run | 10 ++++++++++ > 2 files changed, 12 insertions(+), 1 deletion(-) I hesitated to send this email because I have been reduced to simply skimming the git mailing list (very busy with other projects/real life!), and I may have misunderstood what you aim to do here. ;) In essence, I was triggered by the 'GIT-BUILD-OPTIONS fallout' phrase in the subject line! That reminded me of a problem/patch I was looking at earlier this (wait, last) year. The patch (below) was a complete 'hack' (as you can see) to allow the environment to override the 'GIT-BUILD-OPTIONS' file. This was in an old branch named 'meson-wip' which I have been meaning to look at again to either delete or fix-up. One of the many reasons (apart from being a disgusting hack) that I didn't progress this patch is because I felt that not all 'options' in that file should be able to be 'overridden'. So, that implies that the file needs to be split into two; one file of options which can be overridden from the environment and one that can't. If so, then someone has to decide which is which. [I'm sure you could do a much better job than the patch below!] BTW, I can't remember why I wanted to do this anyway ... :) Hopefully, this is not a complete waste of the list's time. If so, sorry in advance! Thanks. ATB, Ramsay Jones -------- >8 -------- Date: Tue, 27 May 2025 21:53:49 +0100 Subject: [PATCH] test-lib.sh: allow environment to override GIT-BUILD-OPTIONS Signed-off-by: Ramsay Jones --- t/test-lib.sh | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/t/test-lib.sh b/t/test-lib.sh index 621cd31ae1..5239042b97 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -101,7 +101,18 @@ then echo >&2 'error: GIT-BUILD-OPTIONS missing (has Git been built?).' exit 1 fi -. "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS + +# allow the environment to override the settings from GIT-BUILD-OPTIONS +while IFS== read var val +do + e_val=$(eval echo '${'"$var"'}' 2>/dev/null) + if test -n "$e_val" + then + val="'$e_val'" + fi + eval "$var"=$val +done < "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS + export PERL_PATH SHELL_PATH if test -z "$TEST_OUTPUT_DIRECTORY" -- 2.52.0 ``` ## Jeff King, 2026-01-16 16:54 Subject: Re: [PATCH 0/2] more t/perf meson/GIT-BUILD-OPTIONS fallout Message-ID: <20260116165451.GB1636797@coredump.intra.peff.net> URL: https://gitlist.dev/e/20260116165451.GB1636797%40coredump.intra.peff.net In-Reply-To: <1a430542-715e-4cf1-86f5-d9424951204a@ramsayjones.plus.com> ``` On Tue, Jan 06, 2026 at 05:07:11PM +0000, Ramsay Jones wrote: > I hesitated to send this email because I have been reduced to simply skimming > the git mailing list (very busy with other projects/real life!), and I may > have misunderstood what you aim to do here. ;) > > In essence, I was triggered by the 'GIT-BUILD-OPTIONS fallout' phrase in the > subject line! That reminded me of a problem/patch I was looking at earlier > this (wait, last) year. The patch (below) was a complete 'hack' (as you can > see) to allow the environment to override the 'GIT-BUILD-OPTIONS' file. This > was in an old branch named 'meson-wip' which I have been meaning to look at > again to either delete or fix-up. > > One of the many reasons (apart from being a disgusting hack) that I didn't > progress this patch is because I felt that not all 'options' in that file > should be able to be 'overridden'. So, that implies that the file needs to > be split into two; one file of options which can be overridden from the > environment and one that can't. If so, then someone has to decide which is > which. I think you understood my goal. :) This is more or less what my patch is doing, but just for a select set of options (to un-break t/perf). I think a larger fix may look something like this, but: 1. I agree with you that we may need to consider which options should be able to be overridden and which should not. 2. This hack has to go everywhere that GIT-BUILD-OPTIONS is read. So in test-lib.sh where you have it, but also in perf-lib.sh (matching the fix by Dscho earlier) and also in t/perf/run (matching the fix here). It would be nice if we could write GIT-BUILD-OPTIONS in a way that did the right thing. E.g., by writing: : ${GIT_FOO:=some-value} And then the writer (which is the ultimate source of authority for which variables are included) could decide which ones can be overridden. I _thought_ this wouldn't work because we also source the build options files from the Makefile (and so it has to support both syntaxes). But a quick grep doesn't show us including it. So maybe we used to do so, or maybe I'm mis-remembering (and confusing it with GIT-VERSION-FILE perhaps?). -Peff ```