Re: Pass or not to pass config environment down...

On Tue, Mar 23, 2021 at 2:48 PM Junio C Hamano [off-list ref] wrote:

Many callers do child.env = local_repo_env when spawning a
subprocess.  The elements of child.env is treated as if each of them
were passed to setenv() (if there is '=') or unsetenv (otherwise),
and because local_repo_env[] spells only the variable names, the
effect is to unexport them.  The helper function shown at the
beginning of the message you are responding to, which you wrote more
than 5 years ago, is to exclude GIT_CONFIG_PARAMETERS from that
treatment.  I.e. the code wants run_command() not to drop that
particular environment variable when running a subprocess.

Ok, right. So the part that was confusing me is that by adding the
value into the local_repo_env, we were telling it to clear the
variables. Yep.

Removing GIT_CONFIG_PARAMETERS from local_repo_env[] should have the
same effect, without the helper to special case it in its loop.
We have been passing GIT_CONFIG_PARAMETERS, and we will keep passing
it even if we make such a change to remove it from local_repo_env[].

Yea, makes sense. I think that's a much better approach than special
casing in a separate function.

The configuration parameters passed via the newer GIT_CONFIG_COUNT
mechanism, because local_repo_env[] has it but the above helper does
not special case it, are dropped and not seen by the subprocess.
Assuming that it is a bug and we would want to pass them to the
subprocess the same way as GIT_CONFIG_PARAMETERS environment
variable, we could tweak the helper function to make it special case
GIT_CONFIG_COUNT the same way as we've done GIT_CONFIG_PARAMETERS
for the past 5 years.  But if we suspect that other codepaths (not
the ones that use the above helper) may be doing it wrong and they
too should pass the configuration parameters to the subprocess, a
simpler way would be to remove them from local_repo_env[].

That is the summary of the current status and what would happen if
we did the attached patch.

Thank you for restating and clarifying. I think this is the right
approach, and I agree that GIT_CONFIG_COUNT should be treated in the
same way as GIT_CONFIG_PARAMETERS.

So, I think this direction is good. I imagine a full patch would
include also dropping the specialized helper function that is no
longer needed, and possibly adding new tests for the behavior of
GIT_CONFIG_COUNT?

Thanks,
Jake