On Wed, Feb 24, 2016 at 5:41 PM, Jeff King <peff@xxxxxxxx> wrote:
> 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("ed, var);
>> + strbuf_addch("ed, '=');
>> + strbuf_addstr("ed, value);
>> +
>> + /* safely quote them for shell use */
>> + sq_quote_buf(out, quoted.buf);
>> + }
>> + return 0;
>> +}
>
> This leaks "quoted", doesn't it?
>
Yes this was an oversight. Will fix 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)?
>
Hah, that would be confusing. The comment will be dropped in v3.
>> +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).
>
I don't think there will be a later, but I didn't think to check argc,
since a few other submodule--helpers fail to check it. I will clean
this out, and possibly provide a second patch which cleans up the
other case(s?) of missed argc checks as well. I think it was only the
submodule--helper list subcommand, but I don't recall right now.
>> +# 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?
> 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
Wouldn't hurt, I was trying to come up with a good name as well, but I
like sanitize_submodule_env better.
>
>> 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
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.
Regards,
Jake
--
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