Re: [PATCH v3 3/5] submodule: refrain from filtering GIT_CONFIG_COUNT
flat view
From: Jonathan Tan <hidden>
Date: 2021-06-11 17:03:06
quoted
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 listquoted
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).