| Message ID | e1a40108f42addf8a589c1d0ac4ed76fb525be9b.1623345496.git.jonathantanmy@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | First steps towards partial clone submodules | expand |
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.
On Thu, Jun 10, 2021 at 02:13:49PM -0700, Elijah Newren wrote: > > 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
> > 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 As Peff describes, if an envvar is present in the list, it becomes unset. (Perhaps confusingly, if an string of the form "ENVVAR=VALUE" (note the "=") is present in the list, it becomes set to the given value.) So in order to *not* filter out the envvar from the subprocess, we need to filter out the envvar from env_array. If you can think of a better way to document this, please let me know. One way I thought of that might reduce confusion is for this function to take the struct child_process directly. I don't like taking the whole struct when we're just modifying env_array, but I think that this becomes easier to document (just say that we're unsetting all these envvars from the child process, and in the function body, say that to unset a variable, we need to make it appear without a "=" in env_array).
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); } }
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(-)