Re: [PATCH v2] git: submodule honor -c credential.* from command line

[Jeff King <peff@peff.net>]

On Wed, Feb 24, 2016 at 03:59:12PM -0800, Jacob Keller wrote:

> +int sanitize_submodule_config(const char *var, const char *value, void *data)
> +{
> +	struct strbuf quoted = STRBUF_INIT;
> +	struct strbuf *out = data;
> +
> +	if (submodule_config_ok(var)) {
> +		if (out->len)
> +			strbuf_addch(out, ' ');
> +
> +		/* combined all the values before we quote them */
> +		strbuf_addstr(&quoted, var);
> +		strbuf_addch(&quoted, '=');
> +		strbuf_addstr(&quoted, value);
> +
> +		/* safely quote them for shell use */
> +		sq_quote_buf(out, quoted.buf);
> +	}
> +	return 0;
> +}

This leaks "quoted", doesn't it?

I was confused by the "combine all the values" comment. We just have
_one_ config key/value here, right (I had thought originally that you
were putting multiple keys into a single sq-quoted string, which would be
wrong)?

I agree with Eric, though, that you could just drop the comment
entirely.

> +static int module_sanitize_config(int argc, const char **argv, const char *prefix)
> +{
> +	struct strbuf sanitized_config = STRBUF_INIT;
> +
> +	struct option module_sanitize_config_options[] = {
> +		OPT_END()
> +	};
> +
> +	const char *const git_submodule_helper_usage[] = {
> +		N_("git submodule--helper sanitize-config"),
> +		NULL
> +	};
> +
> +	argc = parse_options(argc, argv, prefix, module_sanitize_config_options,
> +			     git_submodule_helper_usage, 0);
> +
> +	git_config_from_parameters(sanitize_submodule_config, &sanitized_config);
> +	if (sanitized_config.len)
> +		printf("%s\n", sanitized_config.buf);
> +
> +	return 0;
> +}

The empty option list looked funny to me for a minute, but I guess you
use it to complain about:

  git submodule--helper sanitize-config --foo

Should we also warn about:

  git submodule--helper sanitize-config foo

I think you could catch both with just:

  if (argc > 1)
	usage(...);

(though I do not mind the empty option list staying in that case, as it
provides the necessary boilerplate for later).

> +# Sanitize the local git environment for use within a submodule. We
> +# can't simply use clear_local_git_env since we want to preserve some
> +# of the settings from GIT_CONFIG_PARAMETERS.
> +sanitize_local_git_env()
> +{
> +	local sanitized_config = $(git submodule--helper sanitize-config)
> +	clear_local_git_env
> +	GIT_CONFIG_PARAMETERS=$sanitized_config
> +}

Do we need to export GIT_CONFIG_PARAMETERS? I guess not; if it is
already exported, we don't need, and if it isn't, then by definition
$sanitized_config will be empty.

The name of this function isn't very descriptive (it's easy to see what
it does from the implementation, but in the callers, it's unclear what
the difference between "clear" and "sanitize" is). Should it maybe be
"sanitize_submodule_env" or something to make it clear that this is
about passing through things for child submodules?

Probably not that big a deal as its local to this script

> diff --git a/t/t7412-submodule--helper.sh b/t/t7412-submodule--helper.sh
> new file mode 100755
> index 000000000000..376f58afe967
> --- /dev/null
> +++ b/t/t7412-submodule--helper.sh

In the long run I think we want to kill off submodule--helper, as it's
just an implementation detail until git-submodule is all in C. I do not
mind these tests in the meantime, as they can act as unit tests. But it
would be nice to also (or instead, if you like) test the actual
user-visible effects. Otherwise, once git-submodule turns into C, these
behaviors are likely to end up completely untested (and it's during that
conversion that you you're most likely to run into regressions!).

-Peff