Re: [PATCH] submodule: stop sanitizing config options

> -- >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                  | 39 +--------------------------------------
>  t/t7412-submodule--helper.sh | 26 --------------------------
>  4 files changed, 3 insertions(+), 83 deletions(-)
>  delete mode 100755 t/t7412-submodule--helper.sh
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 16d6432..89250f0 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -260,22 +260,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 cmd_struct {
>  	const char *cmd;
>  	int (*fn)(int, const char **, const char *);
> @@ -285,7 +269,6 @@ static struct cmd_struct commands[] = {
>  	{"list", module_list},
>  	{"name", module_name},
>  	{"clone", module_clone},
> -	{"sanitize-config", module_sanitize_config},
>  };
>  
>  int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 91f5856..b1c056c 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 c18ab9b..d598881 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1098,50 +1098,13 @@ void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir)
>  	strbuf_release(&rel_path);
>  	free((void *)real_work_tree);
>  }
> -/*
> - * 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