Re: [PATCH] perf: do allow `GIT_PERF_*` to be overridden again

From: Jeff King <peff@peff.net>
Date: Apr 19, 2025, 03:54 UTC
Message-ID: <20250419035407.GA93039@coredump.intra.peff.net>
In-Reply-To: <pull.1900.git.1743764167548.gitgitgadget@gmail.com>

On Fri, Apr 04, 2025 at 10:56:07AM +0000, Johannes Schindelin via GitGitGadget wrote:

Show 6 quoted lines

> However, in 4638e8806e3a (Makefile: use common template for
> GIT-BUILD-OPTIONS, 2024-12-06), a subtle change of behavior was
> introduced: Whereas before, a couple of build-time options (the
> `GIT_PERF_*` ones included) were written to `GIT-BUILD-OPTIONS` only
> when their values were non-empty. With this commit, they are also
> written when they are empty.

It doesn't look like Junio picked this up, so I wanted to chime in that this regression bit me today, too (specifically for GIT_PERF_LARGE_REPO, but also another variable which I'll detail in a moment).

This is also the same issue that hit the interop suite discussed in:

  https://lore.kernel.org/git/Z8IX2bMJe+V80idE@nand.local/

(there I was a bit dismissive of it, because I think GIT_*_MAKE_OPTS would usually be set at build time, but for these other variables, they're almost always going to come from the environment).

> Let's work around the original issue, i.e. let `GIT_PERF_*` environment
> variables override what is recorded in `GIT-BUILD-OPTIONS`.

I like this direction. It not only fixes the regression, but makes things generally behave more as I'd expect them to.

I think it doesn't quite fix everything, though. I noticed that GIT_PERF_REPEAT_COUNT no longer correctly defaults to "3" when using the "run" script. And there are two problems here:

  1. The "run" script itself sources GIT-BUILD-OPTIONS, so it would need
     similar treatment.

  2. But even if we did, that, in my case I am not setting
     PERF_REPEAT_COUNT at all. So there is no local env variable that
     we'd use to take precedence. The problem is in the way the "run"
     script assigns defaults. If it sees an environment variable set
     (whether actually from the environment or from GIT-BUILD-OPTIONS)
     it accepts it as-is, rather than installing its fallback. So it
     will never use its default of "3", and instead retain the blank
     string (which, by a stroke of luck, does still cause it to run each
     trial at least once).

So I think we either need to rewrite the "run" script's fallback code, or teach the GIT-BUILD-OPTIONS writer to avoid mentioning unset variables (which is the real source of the problem in 4638e8806e3a).

So...

> Note that this is just the tip of the iceberg, there are a couple of
> `GIT_TEST_*` options that may want a similar fix in `test-lib.sh`. Due
> to time constraints on my side, this here patch focuses exclusively on
> the `GIT_PERF_*` settings.

...yes, this is definitely the tip of the iceberg. I don't mind doing this patch as an incremental step forward (and because it is an improvement in behavior even if 4638e8806e3a were reverted). But the issue is far from solved overall.

Show 10 quoted lines

> +# GIT-BUILD-OPTIONS, sourced by test-lib.sh, overwrites the `GIT_PERF_*`
> +# values that are set by the user (if any). Let's stash them away as
> +# `eval`-able assignments.
> +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
> +	}")"

The implementation here looks correct to me, including the quoting. The number of backslashes is a bit vomit-inducing, but I think unavoidable (we could put the sed command into single-quotes, but then you'd have to escape out of it to show the single-quotes you do need to mention).

-Peff