On Tue, Mar 23, 2021 at 2:48 PM Junio C Hamano <gitster@xxxxxxxxx> 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
