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

From: Stefan Beller <sbeller@google.com>
Date: Feb 24, 2016, 23:27 UTC
Message-ID: <CAGZ79kbSd4TRdgdcj2NUrc+pL7ATrGQNcfTYd57TuJtyZ5QBwg@mail.gmail.com>
In-Reply-To: <1456344559-2822-1-git-send-email-jacob.e.keller@intel.com>

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

Show 14 quoted lines

> From: Jacob Keller <jacob.keller@gmail.com>
>
> 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 which can be used to sanitize the
> GIT_CONFIG_PARAMETERS value to only allow certain settings. For now,
> restrict this to only credential.* settings.

I guess for now that subset is fine and will be expanded over time?

Show 5 quoted lines

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

The patch looks good to me, we should have introduced the submodule--helper test file earlier. I may migrate the test for the fix I send earlier today to that file eventually.

Show 8 quoted lines

> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index f4c3eff179b5..8194d3b3d1d5 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -255,6 +255,56 @@ static int module_clone(int argc, const char **argv, const char *prefix)
>         return 0;
>  }
>

Probably this will cause easy to resolve merge conflicts with origin/sb/submodule-parallel-update

Show 5 quoted lines

> @@ -264,6 +314,7 @@ static struct cmd_struct commands[] = {
>         {"list", module_list},
>         {"name", module_name},
>         {"clone", module_clone},
> +       {"sanitize-config", module_sanitize_config},

same here.

Show 5 quoted lines

> 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

Thanks for introducing such a file. I did not do it as I though it was "too small" and would not be enough to test to justify its own file.

> +
> +test_expect_success 'sanitize-config clears configuration' '
> +       git -c user.name="Some User" submodule--helper sanitize-config >actual &&
> +       test_must_be_empty actual

I usually keep my user.name in the global config, so no need to pass it around like that, but for testing purposes this looks good.