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

Jacob Keller [off-list ref] writes:

On Tue, Mar 23, 2021 at 11:58 AM Junio C Hamano [off-list ref] wrote:

quoted

I was grepping around and found this piece of code today:

        static void prepare_submodule_repo_env_no_git_dir(struct strvec *out)
        {
                const char * const *var;

                for (var = local_repo_env; *var; var++) {
                        if (strcmp(*var, CONFIG_DATA_ENVIRONMENT))
                                strvec_push(out, *var);
                }
        }

which tries to "unsetenv" the environment variables that pertain to
the current repository listed in loocal_repo_env[], but makes
exception for GIT_CONFIG_PARAMETERS.

Right. We need to unset some parameters, but not everything...

quoted

It originally came from 14111fc4 (git: submodule honor -c
credential.* from command line, 2016-02-29) and later simplified by
89044baa (submodule: stop sanitizing config options, 2016-05-04).

Now after d8d77153 (config: allow specifying config entries via
envvar pairs, 2021-01-12), we have yet another way to pass a set of
custom one-shot configuration via the environment variable, using
GIT_CONFIG_COUNT (which is in local_repo_env[] and will be removed
from the environment by this helper function), GIT_CONFIG_KEY_$n and
GIT_CONFIG_VALUE_$n (which are unbound set and naturally not in
local_repo_env[]).  Leaving the latter two exported will not hurt if
we do intend to hide the custom configuration from the subprocess by
unsetting GIT_CONFIG_COUNT, but should we be doing so?

I think it's a difficult question because if I recall, there was some
question/concern about what values need to stay for the submodule vs
which ones do not..?

quoted

There are many run_command() users that just pass local_repo_env[]
to the child.env when running a subprocess.  Given that the code
that works in a submodule, which presumably is THE primary target
of the "we do not want to pass environment variables that pertain to
the current repository but not to the repository the child process
works in" consideration that the local_repo_env[] is about, does *not*
want the GIT_CONFIG_PARAMETERS cleansed, I have to wonder if the
environment variables (the original GIT_CONFIG_PARAMETERS as well as
Patrick's GIT_CONFIG_{COUNT,KEY_$n,VALUE_$n}) should be in that
local_repo_env[] array in the first place.  If we remove them, the
above helper function can just go away and be replaced with the
usual child.env = local_repo_env assignment like everybody else.

I think that makes a reasonable amount of sense. So if I understand
right, this change makes it so that CONFIG_DATA_ENVIRONMENT and
CONFIG_COUNT_ENVIRONMENT will no longer be forwarded? I guess I am a
bit confused about the current status vs what we want here.

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.

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[].

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.

quoted

Comments?

 environment.c | 2 --
 1 file changed, 2 deletions(-)

diff --git c/environment.c w/environment.c
index 2f27008424..6dcee6a9c5 100644
--- c/environment.c
+++ w/environment.c

@@ -116,8 +116,6 @@ static char *super_prefix;
 const char * const local_repo_env[] = {
        ALTERNATE_DB_ENVIRONMENT,
        CONFIG_ENVIRONMENT,
-       CONFIG_DATA_ENVIRONMENT,
-       CONFIG_COUNT_ENVIRONMENT,
        DB_ENVIRONMENT,
        GIT_DIR_ENVIRONMENT,
        GIT_WORK_TREE_ENVIRONMENT,

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