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

From: Elijah Newren <newren@gmail.com>
To: Jonathan Tan <jonathantanmy@google.com>
Cc: Git Mailing List <git@vger.kernel.org>,
	Taylor Blau <me@ttaylorr.com>,
	Emily Shaffer <emilyshaffer@google.com>,
	Junio C Hamano <gitster@pobox.com>, Jeff King <peff@peff.net>
Subject: Re: [PATCH v3 3/5] submodule: refrain from filtering GIT_CONFIG_COUNT
Date: Thu, 10 Jun 2021 14:13:49 -0700	[thread overview]
Message-ID: <CABPp-BFpW7KLQGbe1kz5FNWABrTNaHOCZw3poXBMWQ9MP91GuQ@mail.gmail.com> (raw)
In-Reply-To: <e1a40108f42addf8a589c1d0ac4ed76fb525be9b.1623345496.git.jonathantanmy@google.com>

Adding Peff to cc as per comments about 89044baa8b ("submodule: stop
sanitizing config options", 2016-05-04) below.

On Thu, Jun 10, 2021 at 10:35 AM Jonathan Tan <jonathantanmy@google.com> wrote:
>
> 14111fc492 ("git: submodule honor -c credential.* from command line",
> 2016-03-01) taught Git to pass through the GIT_CONFIG_PARAMETERS
> environment variable when invoking a subprocess on behalf of a
> submodule. But when d8d77153ea ("config: allow specifying config entries
> via envvar pairs", 2021-01-15) introduced support for GIT_CONFIG_COUNT
> (and its associated GIT_CONFIG_KEY_? and GIT_CONFIG_VALUE_?), the
> subprocess mechanism wasn't updated to also pass through these
> variables.
>
> Since they are conceptually the same (d8d77153ea was written to address
> a shortcoming of GIT_CONFIG_PARAMETERS), update the submodule subprocess
> mechanism to also pass through GIT_CONFIG_COUNT.
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  submodule.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> 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.

Digging around for a while led me to commit 89044baa8b ("submodule:
stop sanitizing config options", 2016-05-04), which suggests that the
passing of GIT_CONFIG_PARAMETERS is not done here but in
git-submodule.sh.  It still didn't make it clear to me why it's
stripped out here, but something makes me thing that git-submodule.sh
should be affected by your change as well.

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

I've been looking at this patch longer than I care to admit and I
still feel like I don't have a clue what's going on.