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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Feb 25, 2016, 00:27 UTC
Message-ID: <CAPig+cROoc+Y-V_6hw6Hx4X9pFGmRjfhpzYGKhswwa=bA5b=+g@mail.gmail.com>
In-Reply-To: <1456358352-28939-1-git-send-email-jacob.e.keller@intel.com>

On Wed, Feb 24, 2016 at 6:59 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:

Show 17 quoted lines

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

A few superficial comments below...

Show 9 quoted lines

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

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

Show 17 quoted lines

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

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.

Show 24 quoted lines

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

Perhaps not a big deal since the program exits immediately after, but you could:

    strbuf_release(&sanitized_config);

Show 11 quoted lines

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

Is 'local' a bashism? (Although, I see that 'local' is already being used in relative_path(); perhaps that ought to be cleaned up.)

Show 13 quoted lines

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

Maybe: s/tries to verify/verifies/

Show 17 quoted lines

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