Thread (59 messages) 59 messages, 5 authors, 2021-06-19

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 list
quoted
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).
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help