Re: [PATCH] submodule: stop sanitizing config options

On Wed, May 04, 2016 at 10:58:26AM -0700, Stefan Beller wrote:

quoted

So this whitelist is probably not giving us any benefit, and
is already creating a hassle as people propose things to put
on it. Let's just drop it entirely.

Just to recap:
Before jk/submodule-config-sanitize-fix (jk/submodule-c-credential actually)
we passed nothing down to the commands operating on submodules.

Then we decided to pass on some of it based on a curated list.

Curating the list is too hard, so we pass on everything now, because
it is easy to maintain and easy to explain. And when the user is hurt,
they're holding it wrong?

Yes, I think that sums it up (the last paragraph is what's under
discussion, so it's up for debate).

quoted

Note that we still need to keep a special code path for
"prepare the submodule environment", because we still have
to take care to pass through $GIT_CONFIG_PARAMETERS (and
block the rest of the repo-specific environment variables).

So when running `git -c foo=bar command --recurse-submodules`
the `-c` parsing calls git_config_push_parameter, which
exports that string `foo=baz` into the environment variable
GIT_CONFIG_PARAMETERS.

When the submodule command is called, sanitize_submodule_env
just wipes all the Git related configurations except those in
GIT_CONFIG_PARAMETERS as they are set again after
clear_local_git_env wiped it.

Right.

I wonder about the implementation detail, if we rather want to introduce
a `git rev-parse --repo-only-local-env-vars` which is
`git rev-parse --local-env-vars` without the GIT_CONFIG_PARAMETERS.
such that clear_local_git_env does the right thing and we don't
have to have 2 functions for it (i.e. clear_local_git_env and
sanitize_submodule_env, which is the newer not as strict version of it)

I don't think that really buys you much. The policy is technically
duplicated in prepare_submodule_repo_env() in submodule.c and
sanitize_submodule_env in submodule.h. But without the whitelist, it's
such a simple policy that I'm not sure it's worth the boilerplate of
having the shell function call into the C code.

If we did want to go that route, the more appropriate interface would
probably be:

  git submodule--helper sanitize-env

or something. That avoids making it a public interface, and makes it
clear that we are invoking the submodule rules.

Do we want to add documentation for the new behavior though?
    Before: pass not -c arguments to submodules
    Now: Pass all the -c arguments to submodules

I don't think there was any documentation for the _old_ behavior, and
certainly jk/submodule-c-credential didn't add any. But it probably is
worth document, maybe as part of "-c"? Care to roll a patch on top?

-Peff