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

Hi,

On Wed, Feb 24, 2016 at 3:27 PM, Stefan Beller [off-list ref] wrote:

On Wed, Feb 24, 2016 at 12:09 PM, Jacob Keller [off-list ref] wrote:

quoted

From: Jacob Keller <redacted>
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?

Yes, I figured that we would expand it as there became a reason to use
it.. If we instead blacklist things we may introduce many more issues
in the future.

quoted

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.

Ya, I didn't see any other tests for submodule--helper in master
branch yet, so I just added one.

quoted

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

Yea I figured there would be some conflicts, but I based it on master
so it should be relatively easy to fix things up.

quoted

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

quoted

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.

I think that it makes more sense, since none of the other files really
fit the submodule--helper. In addition, I figure that "eventually" it
would go away after all of git-submodule.sh was re-written in C, and
git-submodule--helper is obsolete. That meant that having only a
single location for these tests would be easier to isolate and remove
them when we need to.

quoted

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

I just picked a simple key. This could really be anything, but I
figured that user.name is unlikely to be ever allowed to pass through
to submodules though it is probably harmless to do so. I don't believe
this is necessarily a valid case but rather just a way to make sure
that all configuration was not passed through.

Thanks,
Jake