Pass or not to pass config environment down...

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.

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?

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.

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 11:58 AM Junio C Hamano [off-list ref] wrote:

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

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..?

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.