On Tue, Mar 04, 2025 at 05:08:59PM -0500, Taylor Blau wrote:

quoted

Hmm. I am not sure what this is supposed to do, as I would not expect
that "make -C t/perf" to build anything at all. It will use the working
tree version built in the first step. So I'd expect your initial "make"
to do all the work (and either fail or not depending on whether NO_EXPAT
is set in your config.mak).

Oops, I should have used the t/interop example instead of t/perf from
above. I agree that t/perf doesn't build anything. The buggy invocation
that Elijah and I noticed was:

    make &&
    make -C t/interop GIT_INTEROP_MAKE_OPTS='...'

Ah, that makes much more sense.

And while it's the same issue as my:

quoted

  make &&
  (cd t/perf && GIT_PERF_MAKE_OPTS=NO_EXPAT=1 ./run HEAD^ HEAD)

I can see how it's even more confusing, since you are feeding it as a
make variable, and not through the environment (though of course it is
through the environment that you'd eventually expect it to make it).

quoted

I do think it would be nice if environment variables took precedence
over the sourced GIT-BUILD-OPTIONS for "./run", but I suspect doing so
is a little tricky.

Yeah, I agree, and I think that would be tantamount to also fixing the
pre-4638e8806e behavior, which would be nice. I think a good middle
ground would be to continue to allow environment variables to override
options that are unset in GIT-BUILD-OPTIONS, which definitely is a
regression in 4638e8806e.

IMHO the root of the problem is writing GIT_PERF_MAKE_OPTS and
GIT_INTEROP_MAKE_OPTS into GIT-BUILD-OPTIONS at all. It is not used in
the actual build of Git run by the initial "make" invocation, and if it
were to change we would not actually need to rebuild anything. And yet:

  make GIT_PERF_MAKE_OPTS=foo

will not fail (since we don't look at the nonsense value), and:

  make GIT_PERF_MAKE_OPTS=bar

will rebuild at least scripts because it doesn't know that the changed
value is uninteresting.

If we instead just read the value when running "make -C t/interop", we'd
always get the fresh value (whether from the command-line, the
environment, or reading config.mak).

But there are two catches:

  - right now if you do:

      make GIT_INTEROP_MAKE_OPTS=whatever
      make -C t/interop

    it will use the value from the original "make". I think that is the
    root of the confusion, but possibly people depend on that? I don't
    know. I would (and in fact do) put it into my config.mak, where it
    would be seen by both invocations.

  - we'd want to see the values from both make and the shell, and their
    syntaxes are slightly different. In particular, right now I'd do:

      echo GIT_PERF_MAKE_OPTS=whatever >>config.mak
      make  ;# this writes it into GIT-BUILD-OPTIONS
      cd t/perf
      ./run HEAD^ HEAD ;# this sources GIT-BUILD-OPTIONS

    How does that work without GIT-BUILD-OPTIONS? I'd need ./run to
    source the config.mak file, but it's in make format, not shell. So
    I'd have to do something like this in the run script:

      opts=$(make -epn | perl -lne 'print $1 if /^GIT_PERF_MAKE_OPTS = (.*)/')

    (or there are variants that can be implemented with a special
    target). It's an extra step, but maybe not too bad.

So I dunno. It may not be worth changing the status quo.

quoted

quoted

So I think a more robust fix might look like only filling out those
lines in the GIT-BUILD-OPTIONS template when they are non-empty, similar
to the pre-4638e8806e behavior. Something like:

Yeah, that would fix the regression. But I kind of feel like your
initial command is already skirting the edges of what the original code
was meant to handle.

Hmm. I'm not sure I am following what you're saying here. How so?

I think you were lucky that giving a different GIT_PERF_MAKE_OPTS
variable to your second make invocation ever worked at all. It was not
not the intended mechanism (at least that is my understanding; like I
said, most of this was not documented and the implications of various
approaches were probably not through through all that much).

So yes, 4638e8806e is a regression for your particular invocation, but
given the adjacent issues (if you had those variables set in your
config.mak), is it worth trying to support your case? Or put another
way, is there any reason you can't just do:

  make GIT_INTEROP_MAKE_OPTS=whatever && make -C t/interop

? If this change impacted a lot of users, I'd be more worried about
retaining corner cases. But the perf and interop suites are developer
tools, and I'd guess that only a tiny fraction of Git devs even use
them.

Not that I am particularly opposed to your "don't write empty values to
the GIT-BUILD-OPTIONS" solution. My main complaint is just that it's new
extra code, though if we can at least write it _once_ instead of having
to remember to do it for each variable, that's not too bad.

-Peff