On Wed, May 04, 2016 at 10:58:26AM -0700, Stefan Beller wrote: > > 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). > > 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html