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

From: Jeff King <peff@peff.net>
Date: Jun 10, 2021, 21:51 UTC
Message-ID: <YMKJSuin9fS/4acr@coredump.intra.peff.net>
In-Reply-To: <CABPp-BFpW7KLQGbe1kz5FNWABrTNaHOCZw3poXBMWQ9MP91GuQ@mail.gmail.com>

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

Show 25 quoted lines

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

Show 6 quoted lines

> 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