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

From: Eric Sunshine <hidden>
Date: 2016-06-15 23:08:26

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

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

quoted hunk ↗ jump to hunk

Signed-off-by: Jacob Keller <redacted>
---

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

quoted hunk ↗ jump to hunk

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

quoted hunk ↗ jump to hunk

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

quoted hunk ↗ jump to hunk

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

quoted hunk ↗ jump to hunk

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

quoted hunk ↗ jump to hunk

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

quoted hunk ↗ jump to hunk

+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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help