v2git: submodule honor -c credential.* from command line

> Due to the way that the git-submodule code works, it clears all local
> git environment variables before entering submodules. This is normally
> a good thing since we want to clear settings such as GIT_WORKTREE and
> other variables which would affect the operation of submodule commands.
> However, GIT_CONFIG_PARAMETERS is special, and we actually do want to
> preserve these settings. However, we do not want to preserve all
> configuration as many things should be left specific to the parent
> project.
>
> Add a git submodule--helper function, sanitize-config, which shall be
> used to sanitize GIT_CONFIG_PARAMETERS, removing all key/value pairs
> except a small subset that are known to be safe and necessary.
>
> Replace all the calls to clear_local_git_env with a wrapped function
> that filters GIT_CONFIG_PARAMETERS using the new helper and then
> restores it to the filtered subset after clearing the rest of the
> environment.

> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> ---
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> @@ -255,6 +255,61 @@ static int module_clone(int argc, const char **argv, const char *prefix)
> +/* 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. Right now only "credential.*" fits both criteria.
> + */

> +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 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 */

> +               sq_quote_buf(out, quoted.buf);
> +       }
> +       return 0;
> +}
> +
> +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;
> +}
> +
> diff --git a/git-submodule.sh b/git-submodule.sh
> @@ -192,6 +192,16 @@ isnumber()
> +# 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
> +}
> diff --git a/t/t7412-submodule--helper.sh b/t/t7412-submodule--helper.sh
> @@ -0,0 +1,25 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2016 Jacob Keller
> +#
> +
> +test_description='Basic plumbing support of submodule--helper
> +
> +This test tries to verify 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
> +'
> +
> +test_expect_success 'sanitize-config keeps credential.helper' '
> +       git -c credential.helper="helper" submodule--helper sanitize-config >actual &&
> +       echo "'\''credential.helper=helper'\''" >expect &&
> +       test_cmp expect actual
> +'
> +
> +test_done

> > diff --git a/git-submodule.sh b/git-submodule.sh
> > @@ -192,6 +192,16 @@ isnumber()
> > +# 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)
> 
> Is 'local' a bashism? (Although, I see that 'local' is already being
> used in relative_path(); perhaps that ought to be cleaned up.)

>> ---
>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
>> @@ -255,6 +255,61 @@ static int module_clone(int argc, const char **argv, const char *prefix)
>> +/* 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. Right now only "credential.*" fits both criteria.
>> + */
>
> Drop the final sentence for a couple reasons:
>
> 1. It's merely repeating what the code itself already says, and...
> 2. It's likely to become outdated when additional variables are added.
>

> Also, style:
>
>     /*
>      * Multi-line comment
>      * style.
>      */
>

>> +               /* combined all the values before we quote them */
>
> Comment repeats what the code already says, thus not terribly useful.
>
> Also: s/combined/combine/
>

>> +               strbuf_addstr(&quoted, var);
>> +               strbuf_addch(&quoted, '=');
>> +               strbuf_addstr(&quoted, value);
>
> strbuf_addf(&quoted, "%s=%s", var, value);
>
>> +               /* safely quote them for shell use */
>
> Comment repeats what the code already says.
>

>
> Perhaps not a big deal since the program exits immediately after, but you could:
>
>     strbuf_release(&sanitized_config);
>

>> +       local sanitized_config = $(git submodule--helper sanitize-config)
>
> Is 'local' a bashism? (Although, I see that 'local' is already being
> used in relative_path(); perhaps that ought to be cleaned up.)
>

>> +
>> +test_description='Basic plumbing support of submodule--helper
>> +
>> +This test tries to verify the submodule--helper plumbing command used
>
> Maybe: s/tries to verify/verifies/
>

> >> +               /* combined all the values before we quote them */
> >
> > Comment repeats what the code already says, thus not terribly useful.
> >
> > Also: s/combined/combine/
> >
> I tend to make comments like this when I change the obvious way it was
> done, I commented this because I based it on a scratch patch from Jeff
> that didn't have them put together before quoting.

> On Wed, Feb 24, 2016 at 10:19:05PM -0800, Jacob Keller wrote:
>
>> >> +               /* combined all the values before we quote them */
>> >
>> > Comment repeats what the code already says, thus not terribly useful.
>> >
>> > Also: s/combined/combine/
>> >
>> I tend to make comments like this when I change the obvious way it was
>> done, I commented this because I based it on a scratch patch from Jeff
>> that didn't have them put together before quoting.
>
> That's a good point. The _what_ is not interesting here, but the _why_
> might be. Namely that we must quote the whole thing as a unit, or the
> parser on the receiving end will not be able to read it.
>

> I'd also be amenable to relaxing the parser (which is as strict as it is
> only out of laziness, and the fact that it was reading the output only
> of its nearby generator function). But I can understand if you don't
> feel like digging into that.
>
> -Peff

> +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;
> +}

> +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;
> +}

> +# 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
> +}

> 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

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

>> +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

> >> +# 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.
> >
> How does modifying an exported variable work?

> I 100% agree. I think the test file is useful for now, and there are
> (currently) no other tests for submodule--helper, so I'd like to get
> them all confined to this test. I think we need a real way to test the
> change here, but I think figuring out how to test the
> credential.helper is a bit outside the scope of what i had time for
> today. I can try to find some cycles to check out tomorrow. You
> mentioned we'd need a test in the same idea as one of the http clone
> tests? I don't know where to start with something like this though.

> I think something like this would work:
> [...]
> but it does not seem to pass with your patch (even after I fixed up the
> weird "local" thing). I think the problem is that we ask
> submodule--helper to do the clone, and it uses local_repo_env. So in
> addition to your patch, you probably need a C version of the same thing
> which outputs to an argv_array.

> On Thu, Feb 25, 2016 at 02:00:36AM -0500, Jeff King wrote:
>
>> I think something like this would work:
>> [...]
>> but it does not seem to pass with your patch (even after I fixed up the
>> weird "local" thing). I think the problem is that we ask
>> submodule--helper to do the clone, and it uses local_repo_env. So in
>> addition to your patch, you probably need a C version of the same thing
>> which outputs to an argv_array.
>
> Something like this (which passes my test, but I didn't think hard about
> it beyond that):

>  static int clone_submodule(const char *path, const char *gitdir, const char *url,
>                            const char *depth, const char *reference, int quiet)
>  {
> @@ -145,7 +166,7 @@ static int clone_submodule(const char *path, const char *gitdir, const char *url
>         argv_array_push(&cp.args, path);
>
>         cp.git_cmd = 1;
> -       cp.env = local_repo_env;
> +       add_submodule_repo_env(&cp.env_array);
>         cp.no_stdin = 1;
>
>         return run_command(&cp);

> On Wed, Feb 24, 2016 at 11:11 PM, Jeff King <peff@peff.net> wrote:
>>  static int clone_submodule(const char *path, const char *gitdir, const char *url,
>>                            const char *depth, const char *reference, int quiet)
>>  {
>> @@ -145,7 +166,7 @@ static int clone_submodule(const char *path, const char *gitdir, const char *url
>>         argv_array_push(&cp.args, path);
>>
>>         cp.git_cmd = 1;
>> -       cp.env = local_repo_env;
>> +       add_submodule_repo_env(&cp.env_array);
>>         cp.no_stdin = 1;
>>
>>         return run_command(&cp);
>
>
> Ignore my previous comment. The cp.env API is *very* subtle. If the
> line is just a name, it removes the environment variable, while
> "name=value" adds it. That is definitely not what I was expecting
> here, so I misread how it works.

> Jacob Keller <jacob.keller@gmail.com> writes:
>
>> On Wed, Feb 24, 2016 at 11:11 PM, Jeff King <peff@peff.net> wrote:
>>>  static int clone_submodule(const char *path, const char *gitdir, const char *url,
>>>                            const char *depth, const char *reference, int quiet)
>>>  {
>>> @@ -145,7 +166,7 @@ static int clone_submodule(const char *path, const char *gitdir, const char *url
>>>         argv_array_push(&cp.args, path);
>>>
>>>         cp.git_cmd = 1;
>>> -       cp.env = local_repo_env;
>>> +       add_submodule_repo_env(&cp.env_array);
>>>         cp.no_stdin = 1;
>>>
>>>         return run_command(&cp);
>>
>>
>> Ignore my previous comment. The cp.env API is *very* subtle. If the
>> line is just a name, it removes the environment variable, while
>> "name=value" adds it. That is definitely not what I was expecting
>> here, so I misread how it works.
>
> I think that is modelled after how putenv(3) is made capable of
> removing an existing environment variable.