Re: [PATCH v3 3/5] submodule: refrain from filtering GIT_CONFIG_COUNT

On Thu, Jun 10, 2021 at 02:13:49PM -0700, Elijah Newren wrote:

quoted

diff --git a/submodule.c b/submodule.c
index 0b1d9c1dde..f09031e397 100644
--- a/submodule.c
+++ b/submodule.c

@@ -489,7 +489,8 @@ 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))
+               if (strcmp(*var, CONFIG_DATA_ENVIRONMENT) &&
+                   strcmp(*var, CONFIG_COUNT_ENVIRONMENT))
                        strvec_push(out, *var);
        }
 }
--

2.32.0.rc1.229.g3e70b5a671-goog

I'm super confused.  It appears that
prepare_submodule_repo_env_no_git_dir() is filtering out
"GIT_CONFIG_PARAMETERS" (CONFIG_DATA_ENVIRONMENT) and
"GIT_CONFIG_COUNT" (CONFIG_COUNT_ENVIRONMENT), using all environment
variables other than these ones.  But the commit message talks about
adding an extra environment variable, rather than filtering another
out.  I must be mis-reading something somewhere, but I'm struggling to
figure it out.

I think there might be a double (triple?) negative here:

  - we want to pass through the config parameters variable, but not
    other local repo env variables;

  - so we _don't_ want the config variable to appear in the "out"
    strvec, because its presence would cause it to be cleared
    from the child process environment;

  - so we go through the list adding everything _except_ that variable;

  - and we match using strcmp(), so a true value means "did not match",
    so we should add it to the list

Also, from looking at the other commit messages you reference, it
appears GIT_CONFIG_PARAMETERS was just one big environment variable,
whereas GIT_CONFIG_COUNT is closely associated with 2*N other
environment variables...so shouldn't your loop (and perhaps also
git-submodule.sh) also be checking GIT_CONFIG_KEY_\d+ and
GIT_CONFIG_VALUE_\d+ ?

We definitely could clean out those GIT_CONFIG_KEY_* values. But the
COUNT serves as a master parameter. Anybody who sets COUNT would then
also set the individual key/value parameters, too (and even it only sets
it to "5", and there is a crufty GIT_CONFIG_KEY_6 in the environment,
that is not wrong).

-Peff