[PATCH] submodule: stop sanitizing config options

[Jeff King <peff@peff.net>]

[+cc Stefan and Jacob since this is really resuming that earlier thread]

On Wed, May 04, 2016 at 03:45:59AM -0400, Jeff King wrote:

> On Wed, May 04, 2016 at 02:26:18AM -0400, Jeff King wrote:
> 
> > >   submodule: pass on http.extraheader config settings
> > 
> > IMHO this should come on top of jk/submodule-config-sanitize-fix (I was
> > surprised at first that your test worked at all, but that is because it
> > is using "clone", which is the one code path that works).
> > 
> > But I think we are waiting on going one of two paths:
> > 
> >   1. drop sanitizing entirely
> > 
> >   2. fix sanitizing and add more variables to it
> > 
> > If we go the route of (2), then we'd want my fix topic and this patch.
> > And if not, then we don't need any of it (just a patch dropping the
> > filtering, which AFAIK nobody has written yet).
> 
> Actually, I think this last bit is not quite true. If we want to go back
> to "nothing gets passed to submodules", we can drop all of my patches,
> but I don't think anybody wants to do that.
> 
> But if we want "everything gets passed to submodules", then we do need
> something like my patch series, because every use of local_repo_env
> needs to be come "local_repo_env excluding GIT_CONFIG_PARAMETERS". I
> don't think we want to simply drop that variable from local_repo_env
> (which would also mean that it would be propagated to a local
> git-upload-pack, for example, along with any third-party scripts that
> use rev-parse --local-env-vars).
> 
> So I think we'd actually want my series as a preliminary fix, followed
> by dropping the whitelist entirely on top of that, and then probably
> simplifying the shell sanitize_submodule_env() on top of that (it would
> be correct without the whitelist, but you can also trivially implement
> it without having to call submodule--helper at all).

I think we'd actually do it all in one, and that patch looks something
like the one below (on top of jk/submodule-config-sanitize-fix).

I don't feel that strongly about going either direction with this, but I
figure it doesn't hurt to make the patch so we know what the actual
option looks like.

-- >8 --
Subject: [PATCH] submodule: stop sanitizing config options

The point of having a whitelist of command-line config
options to pass to submodules was two-fold:

  1. It prevented obvious nonsense like using core.worktree
     for multiple repos.

  2. It could prevent surprise when the user did not mean
     for the options to leak to the submodules (e.g.,
     http.sslverify=false).

For case 1, the answer is mostly "if it hurts, don't do
that". For case 2, we can note that any such example has a
matching inverted surprise (e.g., a user who meant
http.sslverify=true to apply everywhere, but it didn't).

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.

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).

We can do this easily from within the submodule shell
script, which lets us drop the submodule--helper option
entirely (and it's OK to do so because as a "--" program, it
is entirely a private implementation detail).

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/submodule--helper.c  | 17 -----------------
 git-submodule.sh             |  4 ++--
 submodule.c                  | 40 +---------------------------------------
 t/t7412-submodule--helper.sh | 26 --------------------------
 4 files changed, 3 insertions(+), 84 deletions(-)
 delete mode 100755 t/t7412-submodule--helper.sh

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index de3ad5b..48cfc48 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -246,22 +246,6 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
-static int module_sanitize_config(int argc, const char **argv, const char *prefix)
-{
-	struct strbuf sanitized_config = STRBUF_INIT;
-
-	if (argc > 1)
-		usage(_("git submodule--helper sanitize-config"));
-
-	git_config_from_parameters(sanitize_submodule_config, &sanitized_config);
-	if (sanitized_config.len)
-		printf("%s\n", sanitized_config.buf);
-
-	strbuf_release(&sanitized_config);
-
-	return 0;
-}
-
 struct submodule_update_clone {
 	/* index into 'list', the list of submodules to look into for cloning */
 	int current;
@@ -522,7 +506,6 @@ static struct cmd_struct commands[] = {
 	{"list", module_list},
 	{"name", module_name},
 	{"clone", module_clone},
-	{"sanitize-config", module_sanitize_config},
 	{"update-clone", update_clone}
 };
 
diff --git a/git-submodule.sh b/git-submodule.sh
index 3a40d4b..c9d53e1 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -197,9 +197,9 @@ isnumber()
 # of the settings from GIT_CONFIG_PARAMETERS.
 sanitize_submodule_env()
 {
-	sanitized_config=$(git submodule--helper sanitize-config)
+	save_config=$GIT_CONFIG_PARAMETERS
 	clear_local_git_env
-	GIT_CONFIG_PARAMETERS=$sanitized_config
+	GIT_CONFIG_PARAMETERS=$save_config
 	export GIT_CONFIG_PARAMETERS
 }
 
diff --git a/submodule.c b/submodule.c
index 4e76b98..072ea82 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1131,50 +1131,12 @@ int parallel_submodules(void)
 	return parallel_jobs;
 }
 
-/*
- * Rules to sanitize configuration variables that are Ok to be passed into
- * submodule operations from the parent project using "-c". Should only
- * include keys which are both (a) safe and (b) necessary for proper
- * operation.
- */
-static int submodule_config_ok(const char *var)
-{
-	if (starts_with(var, "credential."))
-		return 1;
-	return 0;
-}
-
-int sanitize_submodule_config(const char *var, const char *value, void *data)
-{
-	struct strbuf *out = data;
-
-	if (submodule_config_ok(var)) {
-		if (out->len)
-			strbuf_addch(out, ' ');
-
-		if (value)
-			sq_quotef(out, "%s=%s", var, value);
-		else
-			sq_quote_buf(out, var);
-	}
-
-	return 0;
-}
-
 void prepare_submodule_repo_env(struct argv_array *out)
 {
 	const char * const *var;
 
 	for (var = local_repo_env; *var; var++) {
-		if (!strcmp(*var, CONFIG_DATA_ENVIRONMENT)) {
-			struct strbuf sanitized_config = STRBUF_INIT;
-			git_config_from_parameters(sanitize_submodule_config,
-						   &sanitized_config);
-			argv_array_pushf(out, "%s=%s", *var, sanitized_config.buf);
-			strbuf_release(&sanitized_config);
-		} else {
+		if (strcmp(*var, CONFIG_DATA_ENVIRONMENT))
 			argv_array_push(out, *var);
-		}
 	}
-
 }
diff --git a/t/t7412-submodule--helper.sh b/t/t7412-submodule--helper.sh
deleted file mode 100755
index 149d428..0000000
--- a/t/t7412-submodule--helper.sh
+++ /dev/null
@@ -1,26 +0,0 @@
-#!/bin/sh
-#
-# Copyright (c) 2016 Jacob Keller
-#
-
-test_description='Basic plumbing support of submodule--helper
-
-This test verifies the submodule--helper plumbing command used to implement
-git-submodule.
-'
-
-. ./test-lib.sh
-
-test_expect_success 'sanitize-config clears configuration' '
-	git -c user.name="Some User" submodule--helper sanitize-config >actual &&
-	test_must_be_empty actual
-'
-
-sq="'"
-test_expect_success 'sanitize-config keeps credential.helper' '
-	git -c credential.helper=helper submodule--helper sanitize-config >actual &&
-	echo "${sq}credential.helper=helper${sq}" >expect &&
-	test_cmp expect actual
-'
-
-test_done
-- 
2.8.2.600.g439cdc9