{"thread":{"id":"41545","subject":"[PATCH v5 1/3] sumodule--helper: fix submodule--helper clone usage and check argc count","startedAt":"2016-02-27T00:13:17Z","lastAt":"2016-02-29T22:09:12Z","messageCount":13,"participants":["Jacob Keller","Junio C Hamano"],"isPatch":true,"patchVersion":5,"patchTotal":3},"messages":[{"id":"279592","messageId":"1456532000-22971-1-git-send-email-jacob.e.keller@intel.com","threadId":"41545","inReplyTo":null,"subject":"[PATCH v5 0/3] pass credential.* to submodules","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2016-02-27T00:13:17Z","receivedAt":"2016-02-27T00:13:17Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller \n\nThis version of the series fixes up a few issues and implements\nsq_quotef function to help make intent more clear in the code. I also\nchose to use Jeff's implementation for the test fix since I think it\nworks better and I prefer the consistency of implementation.\n\nInterdiff between v4 and v5:\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex c6c0a3b06e27..43943eef81b4 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -147,16 +147,7 @@ static int sanitize_submodule_config(const char *var, const char *value, void *d\n \t\tif (out->len)\n \t\t\tstrbuf_addch(out, ' ');\n \n-\t\t/*\n-\t\t * The GIT_CONFIG_PARAMETERS parser is a bit picky and\n-\t\t * requires that each var=value pair be quoted as a single\n-\t\t * unit.\n-\t\t */\n-\t\tstrbuf_addstr("ed, var);\n-\t\tstrbuf_addch("ed, '=');\n-\t\tstrbuf_addstr("ed, value);\n-\n-\t\tsq_quote_buf(out, quoted.buf);\n+\t\tsq_quotef(out, \"%s=%s\", var, value);\n \t}\n \n \tstrbuf_release("ed);\ndiff --git a/quote.c b/quote.c\nindex fe884d24521f..b281a8fe454e 100644\n--- a/quote.c\n+++ b/quote.c\n@@ -43,6 +43,19 @@ void sq_quote_buf(struct strbuf *dst, const char *src)\n \tfree(to_free);\n }\n \n+void sq_quotef(struct strbuf *dst, const char *fmt, ...)\n+{\n+\tstruct strbuf src = STRBUF_INIT;\n+\n+\tva_list ap;\n+\tva_start(ap, fmt);\n+\tstrbuf_vaddf(&src, fmt, ap);\n+\tva_end(ap);\n+\n+\tsq_quote_buf(dst, src.buf);\n+\tstrbuf_release(&src);\n+}\n+\n void sq_quote_argv(struct strbuf *dst, const char** argv, size_t maxlen)\n {\n \tint i;\ndiff --git a/quote.h b/quote.h\nindex 99e04d34bf22..6c53a2cc66c4 100644\n--- a/quote.h\n+++ b/quote.h\n@@ -25,10 +25,13 @@ struct strbuf;\n * sq_quote_buf() writes to an existing buffer of specified size; it\n * will return the number of characters that would have been written\n * excluding the final null regardless of the buffer size.\n+ *\n+ * sq_quotef() quotes the entire formatted string as a single result.\n */\n \n extern void sq_quote_buf(struct strbuf *, const char *src);\n extern void sq_quote_argv(struct strbuf *, const char **argv, size_t maxlen);\n+extern void sq_quotef(struct strbuf *, const char *fmt, ...);\n \n /* This unwraps what sq_quote() produces in place, but returns\n * NULL if the input does not look like what sq_quote would have\ndiff --git a/t/t7412-submodule--helper.sh b/t/t7412-submodule--helper.sh\nindex 9d937de8b224..149d42864f29 100755\n--- a/t/t7412-submodule--helper.sh\n+++ b/t/t7412-submodule--helper.sh\n@@ -16,12 +16,11 @@ test_expect_success 'sanitize-config clears configuration' '\n \ttest_must_be_empty actual\n '\n \n-test_expect_success 'sanitize-config keeps credential.helper' \"\n+sq=\"'\"\n+test_expect_success 'sanitize-config keeps credential.helper' '\n \tgit -c credential.helper=helper submodule--helper sanitize-config >actual &&\n-\tcat >expect <<-EOF &&\n-\t'credential.helper=helper'\n-\tEOF\n+\techo \"${sq}credential.helper=helper${sq}\" >expect &&\n \ttest_cmp expect actual\n-\"\n+'\n \n test_done\n\nJacob Keller (3):\n sumodule--helper: fix submodule--helper clone usage and check argc\n count\n quote: implement sq_quotef()\n git: submodule honor -c credential.* from command line\n\n builtin/submodule--helper.c | 73 ++++++++++++++++++++++++++++++++++++++++++--\n git-submodule.sh | 40 +++++++++++++++---------\n quote.c | 13 ++++++++\n quote.h | 3 ++\n t/t5550-http-fetch-dumb.sh | 17 +++++++++++\n t/t7412-submodule--helper.sh | 26 ++++++++++++++++\n 6 files changed, 155 insertions(+), 17 deletions(-)\n create mode 100755 t/t7412-submodule--helper.sh\n\n-- \n2.7.1.429.g45cd78e\n"},{"id":"279590","messageId":"1456532000-22971-2-git-send-email-jacob.e.keller@intel.com","threadId":"41545","inReplyTo":"1456532000-22971-1-git-send-email-jacob.e.keller@intel.com","subject":"[PATCH v5 1/3] sumodule--helper: fix submodule--helper clone usage and check argc count","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2016-02-27T00:13:18Z","receivedAt":"2016-02-27T00:13:18Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller \n\ngit submodule--helper clone usage specified that paths come after the --\nas a sequence. However, the actual implementation does not, and requires\nonly a single path passed in via --path. In addition, argc was\nunchecked. (allowing arbitrary extra arguments that were silently\nignored).\n\nFix the usage description to match implementation. Add an argc check to\nenforce no extra arguments. Fix a bug in the argument passing in\ngit-submodule.sh which would pass --reference and --depth as empty\nstrings when they were unused, resulting in extra argc after parsing\noptions.\n\nSigned-off-by: Jacob Keller \n---\n builtin/submodule--helper.c | 5 ++++-\n git-submodule.sh | 4 ++--\n 2 files changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex f4c3eff179b5..072d9bbd12a8 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -187,13 +187,16 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \tconst char *const git_submodule_helper_usage[] = {\n \t\tN_(\"git submodule--helper clone [--prefix=] [--quiet] \"\n \t\t \"[--reference ] [--name ] [--url ]\"\n-\t\t \"[--depth ] [--] [...]\"),\n+\t\t \"[--depth ] [--path ]\"),\n \t\tNULL\n \t};\n \n \targc = parse_options(argc, argv, prefix, module_clone_options,\n \t\t\t git_submodule_helper_usage, 0);\n \n+\tif (argc)\n+\t\tusage(*git_submodule_helper_usage);\n+\n \tstrbuf_addf(&sb, \"%s/modules/%s\", get_git_dir(), name);\n \tsm_gitdir = strbuf_detach(&sb, NULL);\n \ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 9bc5c5f94d1d..2dd29b3df0e6 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -347,7 +347,7 @@ Use -f if you really want to add it.\" >&2\n \t\t\t\techo \"$(eval_gettext \"Reactivating local git directory for submodule '\\$sm_name'.\")\"\n \t\t\tfi\n \t\tfi\n-\t\tgit submodule--helper clone ${GIT_QUIET:+--quiet} --prefix \"$wt_prefix\" --path \"$sm_path\" --name \"$sm_name\" --url \"$realrepo\" \"$reference\" \"$depth\" || exit\n+\t\tgit submodule--helper clone ${GIT_QUIET:+--quiet} --prefix \"$wt_prefix\" --path \"$sm_path\" --name \"$sm_name\" --url \"$realrepo\" ${reference:+\"$reference\"} ${depth:+\"$depth\"} || exit\n \t\t(\n \t\t\tclear_local_git_env\n \t\t\tcd \"$sm_path\" &&\n@@ -709,7 +709,7 @@ Maybe you want to use 'update --init'?\")\"\n \n \t\tif ! test -d \"$sm_path\"/.git && ! test -f \"$sm_path\"/.git\n \t\tthen\n-\t\t\tgit submodule--helper clone ${GIT_QUIET:+--quiet} --prefix \"$prefix\" --path \"$sm_path\" --name \"$name\" --url \"$url\" \"$reference\" \"$depth\" || exit\n+\t\t\tgit submodule--helper clone ${GIT_QUIET:+--quiet} --prefix \"$prefix\" --path \"$sm_path\" --name \"$name\" --url \"$url\" ${reference:+\"$reference\"} ${depth:+\"$depth\"} || exit\n \t\t\tcloned_modules=\"$cloned_modules;$name\"\n \t\t\tsubsha1=\n \t\telse\n-- \n2.7.1.429.g45cd78e\n"},{"id":"279591","messageId":"1456532000-22971-3-git-send-email-jacob.e.keller@intel.com","threadId":"41545","inReplyTo":"1456532000-22971-1-git-send-email-jacob.e.keller@intel.com","subject":"[PATCH v5 2/3] quote: implement sq_quotef()","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2016-02-27T00:13:19Z","receivedAt":"2016-02-27T00:13:19Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller \n\nSigned-off-by: Jacob Keller \n---\n quote.c | 13 +++++++++++++\n quote.h | 3 +++\n 2 files changed, 16 insertions(+)\n\ndiff --git a/quote.c b/quote.c\nindex fe884d24521f..b281a8fe454e 100644\n--- a/quote.c\n+++ b/quote.c\n@@ -43,6 +43,19 @@ void sq_quote_buf(struct strbuf *dst, const char *src)\n \tfree(to_free);\n }\n \n+void sq_quotef(struct strbuf *dst, const char *fmt, ...)\n+{\n+\tstruct strbuf src = STRBUF_INIT;\n+\n+\tva_list ap;\n+\tva_start(ap, fmt);\n+\tstrbuf_vaddf(&src, fmt, ap);\n+\tva_end(ap);\n+\n+\tsq_quote_buf(dst, src.buf);\n+\tstrbuf_release(&src);\n+}\n+\n void sq_quote_argv(struct strbuf *dst, const char** argv, size_t maxlen)\n {\n \tint i;\ndiff --git a/quote.h b/quote.h\nindex 99e04d34bf22..6c53a2cc66c4 100644\n--- a/quote.h\n+++ b/quote.h\n@@ -25,10 +25,13 @@ struct strbuf;\n * sq_quote_buf() writes to an existing buffer of specified size; it\n * will return the number of characters that would have been written\n * excluding the final null regardless of the buffer size.\n+ *\n+ * sq_quotef() quotes the entire formatted string as a single result.\n */\n \n extern void sq_quote_buf(struct strbuf *, const char *src);\n extern void sq_quote_argv(struct strbuf *, const char **argv, size_t maxlen);\n+extern void sq_quotef(struct strbuf *, const char *fmt, ...);\n \n /* This unwraps what sq_quote() produces in place, but returns\n * NULL if the input does not look like what sq_quote would have\n-- \n2.7.1.429.g45cd78e\n"},{"id":"279593","messageId":"1456532000-22971-4-git-send-email-jacob.e.keller@intel.com","threadId":"41545","inReplyTo":"1456532000-22971-1-git-send-email-jacob.e.keller@intel.com","subject":"[PATCH v5 3/3] git: submodule honor -c credential.* from command line","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2016-02-27T00:13:20Z","receivedAt":"2016-02-27T00:13:20Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller \n\nDue to the way that the git-submodule code works, it clears all local\ngit environment variables before entering submodules. This is normally\na good thing since we want to clear settings such as GIT_WORKTREE and\nother variables which would affect the operation of submodule commands.\nHowever, GIT_CONFIG_PARAMETERS is special, and we actually do want to\npreserve these settings. However, we do not want to preserve all\nconfiguration as many things should be left specific to the parent\nproject.\n\nAdd a git submodule--helper function, sanitize-config, which shall be\nused to sanitize GIT_CONFIG_PARAMETERS, removing all key/value pairs\nexcept a small subset that are known to be safe and necessary.\n\nReplace all the calls to clear_local_git_env with a wrapped function\nthat filters GIT_CONFIG_PARAMETERS using the new helper and then\nrestores it to the filtered subset after clearing the rest of the\nenvironment.\n\nSigned-off-by: Jacob Keller \n---\n\nNotes:\n - v2\n * Clarify which paramaters are left after the sanitization, and don't seem to\n indicate it is our goal to extend the list.\n * add a comment in the submodule_config_ok function indicating the same\n \n - v3\n * Remove extraneous comments\n * add strbuf_release calls\n * rename sanitize_local_git_env\n * remove use of local\n * add C equivalent of sanitize_config\n * add a test for the credential passing\n \n - v4\n * use argc check instead of empty options check\n * fix brain-melting quotes in t7412-submodule--helper.sh\n \n - v5\n * use new sq_quote_f\n * fix test to use ${sq} style to prevent need of complex quotes or the use of\n double quotes on the test body\n\n builtin/submodule--helper.c | 68 +++++++++++++++++++++++++++++++++++++++++++-\n git-submodule.sh | 36 ++++++++++++++---------\n t/t5550-http-fetch-dumb.sh | 17 +++++++++++\n t/t7412-submodule--helper.sh | 26 +++++++++++++++++\n 4 files changed, 133 insertions(+), 14 deletions(-)\n create mode 100755 t/t7412-submodule--helper.sh\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 072d9bbd12a8..43943eef81b4 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -124,6 +124,55 @@ static int module_name(int argc, const char **argv, const char *prefix)\n \n \treturn 0;\n }\n+\n+/*\n+ * Rules to sanitize configuration variables that are Ok to be passed into\n+ * submodule operations from the parent project using \"-c\". Should only\n+ * include keys which are both (a) safe and (b) necessary for proper\n+ * operation.\n+ */\n+static int submodule_config_ok(const char *var)\n+{\n+\tif (starts_with(var, \"credential.\"))\n+\t\treturn 1;\n+\treturn 0;\n+}\n+\n+static int sanitize_submodule_config(const char *var, const char *value, void *data)\n+{\n+\tstruct strbuf quoted = STRBUF_INIT;\n+\tstruct strbuf *out = data;\n+\n+\tif (submodule_config_ok(var)) {\n+\t\tif (out->len)\n+\t\t\tstrbuf_addch(out, ' ');\n+\n+\t\tsq_quotef(out, \"%s=%s\", var, value);\n+\t}\n+\n+\tstrbuf_release("ed);\n+\n+\treturn 0;\n+}\n+\n+static void prepare_submodule_repo_env(struct argv_array *out)\n+{\n+\tconst char * const *var;\n+\n+\tfor (var = local_repo_env; *var; var++) {\n+\t\tif (!strcmp(*var, CONFIG_DATA_ENVIRONMENT)) {\n+\t\t\tstruct strbuf sanitized_config = STRBUF_INIT;\n+\t\t\tgit_config_from_parameters(sanitize_submodule_config,\n+\t\t\t\t\t\t &sanitized_config);\n+\t\t\targv_array_pushf(out, \"%s=%s\", *var, sanitized_config.buf);\n+\t\t\tstrbuf_release(&sanitized_config);\n+\t\t} else {\n+\t\t\targv_array_push(out, *var);\n+\t\t}\n+\t}\n+\n+}\n+\n static int clone_submodule(const char *path, const char *gitdir, const char *url,\n \t\t\t const char *depth, const char *reference, int quiet)\n {\n@@ -145,7 +194,7 @@ static int clone_submodule(const char *path, const char *gitdir, const char *url\n \targv_array_push(&cp.args, path);\n \n \tcp.git_cmd = 1;\n-\tcp.env = local_repo_env;\n+\tprepare_submodule_repo_env(&cp.env_array);\n \tcp.no_stdin = 1;\n \n \treturn run_command(&cp);\n@@ -258,6 +307,22 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n+static int module_sanitize_config(int argc, const char **argv, const char *prefix)\n+{\n+\tstruct strbuf sanitized_config = STRBUF_INIT;\n+\n+\tif (argc > 1)\n+\t\tusage(_(\"git submodule--helper sanitize-config\"));\n+\n+\tgit_config_from_parameters(sanitize_submodule_config, &sanitized_config);\n+\tif (sanitized_config.len)\n+\t\tprintf(\"%s\\n\", sanitized_config.buf);\n+\n+\tstrbuf_release(&sanitized_config);\n+\n+\treturn 0;\n+}\n+\n struct cmd_struct {\n \tconst char *cmd;\n \tint (*fn)(int, const char **, const char *);\n@@ -267,6 +332,7 @@ static struct cmd_struct commands[] = {\n \t{\"list\", module_list},\n \t{\"name\", module_name},\n \t{\"clone\", module_clone},\n+\t{\"sanitize-config\", module_sanitize_config},\n };\n \n int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 2dd29b3df0e6..1f132b489b81 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -192,6 +192,16 @@ isnumber()\n \tn=$(($1 + 0)) 2>/dev/null && test \"$n\" = \"$1\"\n }\n \n+# Sanitize the local git environment for use within a submodule. We\n+# can't simply use clear_local_git_env since we want to preserve some\n+# of the settings from GIT_CONFIG_PARAMETERS.\n+sanitize_submodule_env()\n+{\n+\tsanitized_config=$(git submodule--helper sanitize-config)\n+\tclear_local_git_env\n+\tGIT_CONFIG_PARAMETERS=$sanitized_config\n+}\n+\n #\n # Add a new submodule to the working tree, .gitmodules and the index\n #\n@@ -349,7 +359,7 @@ Use -f if you really want to add it.\" >&2\n \t\tfi\n \t\tgit submodule--helper clone ${GIT_QUIET:+--quiet} --prefix \"$wt_prefix\" --path \"$sm_path\" --name \"$sm_name\" --url \"$realrepo\" ${reference:+\"$reference\"} ${depth:+\"$depth\"} || exit\n \t\t(\n-\t\t\tclear_local_git_env\n+\t\t\tsanitize_submodule_env\n \t\t\tcd \"$sm_path\" &&\n \t\t\t# ash fails to wordsplit ${branch:+-b \"$branch\"...}\n \t\t\tcase \"$branch\" in\n@@ -418,7 +428,7 @@ cmd_foreach()\n \t\t\tname=$(git submodule--helper name \"$sm_path\")\n \t\t\t(\n \t\t\t\tprefix=\"$prefix$sm_path/\"\n-\t\t\t\tclear_local_git_env\n+\t\t\t\tsanitize_submodule_env\n \t\t\t\tcd \"$sm_path\" &&\n \t\t\t\tsm_path=$(relative_path \"$sm_path\") &&\n \t\t\t\t# we make $path available to scripts ...\n@@ -713,7 +723,7 @@ Maybe you want to use 'update --init'?\")\"\n \t\t\tcloned_modules=\"$cloned_modules;$name\"\n \t\t\tsubsha1=\n \t\telse\n-\t\t\tsubsha1=$(clear_local_git_env; cd \"$sm_path\" &&\n+\t\t\tsubsha1=$(sanitize_submodule_env; cd \"$sm_path\" &&\n \t\t\t\tgit rev-parse --verify HEAD) ||\n \t\t\tdie \"$(eval_gettext \"Unable to find current revision in submodule path '\\$displaypath'\")\"\n \t\tfi\n@@ -723,11 +733,11 @@ Maybe you want to use 'update --init'?\")\"\n \t\t\tif test -z \"$nofetch\"\n \t\t\tthen\n \t\t\t\t# Fetch remote before determining tracking $sha1\n-\t\t\t\t(clear_local_git_env; cd \"$sm_path\" && git-fetch) ||\n+\t\t\t\t(sanitize_submodule_env; cd \"$sm_path\" && git-fetch) ||\n \t\t\t\tdie \"$(eval_gettext \"Unable to fetch in submodule path '\\$sm_path'\")\"\n \t\t\tfi\n-\t\t\tremote_name=$(clear_local_git_env; cd \"$sm_path\" && get_default_remote)\n-\t\t\tsha1=$(clear_local_git_env; cd \"$sm_path\" &&\n+\t\t\tremote_name=$(sanitize_submodule_env; cd \"$sm_path\" && get_default_remote)\n+\t\t\tsha1=$(sanitize_submodule_env; cd \"$sm_path\" &&\n \t\t\t\tgit rev-parse --verify \"${remote_name}/${branch}\") ||\n \t\t\tdie \"$(eval_gettext \"Unable to find current ${remote_name}/${branch} revision in submodule path '\\$sm_path'\")\"\n \t\tfi\n@@ -745,7 +755,7 @@ Maybe you want to use 'update --init'?\")\"\n \t\t\tthen\n \t\t\t\t# Run fetch only if $sha1 isn't present or it\n \t\t\t\t# is not reachable from a ref.\n-\t\t\t\t(clear_local_git_env; cd \"$sm_path\" &&\n+\t\t\t\t(sanitize_submodule_env; cd \"$sm_path\" &&\n \t\t\t\t\t( (rev=$(git rev-list -n 1 $sha1 --not --all 2>/dev/null) &&\n \t\t\t\t\t test -z \"$rev\") || git-fetch)) ||\n \t\t\t\tdie \"$(eval_gettext \"Unable to fetch in submodule path '\\$displaypath'\")\"\n@@ -787,7 +797,7 @@ Maybe you want to use 'update --init'?\")\"\n \t\t\t\tdie \"$(eval_gettext \"Invalid update mode '$update_module' for submodule '$name'\")\"\n \t\t\tesac\n \n-\t\t\tif (clear_local_git_env; cd \"$sm_path\" && $command \"$sha1\")\n+\t\t\tif (sanitize_submodule_env; cd \"$sm_path\" && $command \"$sha1\")\n \t\t\tthen\n \t\t\t\tsay \"$say_msg\"\n \t\t\telif test -n \"$must_die_on_failure\"\n@@ -803,7 +813,7 @@ Maybe you want to use 'update --init'?\")\"\n \t\tthen\n \t\t\t(\n \t\t\t\tprefix=\"$prefix$sm_path/\"\n-\t\t\t\tclear_local_git_env\n+\t\t\t\tsanitize_submodule_env\n \t\t\t\tcd \"$sm_path\" &&\n \t\t\t\teval cmd_update\n \t\t\t)\n@@ -841,7 +851,7 @@ Maybe you want to use 'update --init'?\")\"\n \n set_name_rev () {\n \trevname=$( (\n-\t\tclear_local_git_env\n+\t\tsanitize_submodule_env\n \t\tcd \"$1\" && {\n \t\t\tgit describe \"$2\" 2>/dev/null ||\n \t\t\tgit describe --tags \"$2\" 2>/dev/null ||\n@@ -1125,7 +1135,7 @@ cmd_status()\n \t\telse\n \t\t\tif test -z \"$cached\"\n \t\t\tthen\n-\t\t\t\tsha1=$(clear_local_git_env; cd \"$sm_path\" && git rev-parse --verify HEAD)\n+\t\t\t\tsha1=$(sanitize_submodule_env; cd \"$sm_path\" && git rev-parse --verify HEAD)\n \t\t\tfi\n \t\t\tset_name_rev \"$sm_path\" \"$sha1\"\n \t\t\tsay \"+$sha1 $displaypath$revname\"\n@@ -1135,7 +1145,7 @@ cmd_status()\n \t\tthen\n \t\t\t(\n \t\t\t\tprefix=\"$displaypath/\"\n-\t\t\t\tclear_local_git_env\n+\t\t\t\tsanitize_submodule_env\n \t\t\t\tcd \"$sm_path\" &&\n \t\t\t\teval cmd_status\n \t\t\t) ||\n@@ -1209,7 +1219,7 @@ cmd_sync()\n \t\t\tif test -e \"$sm_path\"/.git\n \t\t\tthen\n \t\t\t(\n-\t\t\t\tclear_local_git_env\n+\t\t\t\tsanitize_submodule_env\n \t\t\t\tcd \"$sm_path\"\n \t\t\t\tremote=$(get_default_remote)\n \t\t\t\tgit config remote.\"$remote\".url \"$sub_origin_url\"\ndiff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh\nindex 64146352ae20..48e2ab62da7a 100755\n--- a/t/t5550-http-fetch-dumb.sh\n+++ b/t/t5550-http-fetch-dumb.sh\n@@ -91,6 +91,23 @@ test_expect_success 'configured username does not override URL' '\n \texpect_askpass pass user@host\n '\n \n+test_expect_success 'cmdline credential config passes into submodules' '\n+\tgit init super &&\n+\tset_askpass user@host pass@host &&\n+\t(\n+\t\tcd super &&\n+\t\tgit submodule add \"$HTTPD_URL/auth/dumb/repo.git\" sub &&\n+\t\tgit commit -m \"add submodule\"\n+\t) &&\n+\tset_askpass wrong pass@host &&\n+\ttest_must_fail git clone --recursive super super-clone &&\n+\trm -rf super-clone &&\n+\tset_askpass wrong pass@host &&\n+\tgit -c \"credential.$HTTP_URL.username=user@host\" \\\n+\t\tclone --recursive super super-clone &&\n+\texpect_askpass pass user@host\n+'\n+\n test_expect_success 'fetch changes via http' '\n \techo content >>file &&\n \tgit commit -a -m two &&\ndiff --git a/t/t7412-submodule--helper.sh b/t/t7412-submodule--helper.sh\nnew file mode 100755\nindex 000000000000..149d42864f29\n--- /dev/null\n+++ b/t/t7412-submodule--helper.sh\n@@ -0,0 +1,26 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2016 Jacob Keller\n+#\n+\n+test_description='Basic plumbing support of submodule--helper\n+\n+This test verifies the submodule--helper plumbing command used to implement\n+git-submodule.\n+'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'sanitize-config clears configuration' '\n+\tgit -c user.name=\"Some User\" submodule--helper sanitize-config >actual &&\n+\ttest_must_be_empty actual\n+'\n+\n+sq=\"'\"\n+test_expect_success 'sanitize-config keeps credential.helper' '\n+\tgit -c credential.helper=helper submodule--helper sanitize-config >actual &&\n+\techo \"${sq}credential.helper=helper${sq}\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n2.7.1.429.g45cd78e\n"},{"id":"279595","messageId":"CA+P7+xo_LU0-B9MbHefCuba2yDauKj87XGjpeuku31ZAptF=2A@mail.gmail.com","threadId":"41545","inReplyTo":"1456532000-22971-1-git-send-email-jacob.e.keller@intel.com","subject":"Re: [PATCH v5 0/3] pass credential.* to submodules","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-02-27T00:37:23Z","receivedAt":"2016-02-27T00:37:23Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Fri, Feb 26, 2016 at 4:13 PM, Jacob Keller wrote:\n> From: Jacob Keller \n>\n> This version of the series fixes up a few issues and implements\n> sq_quotef function to help make intent more clear in the code. I also\n> chose to use Jeff's implementation for the test fix since I think it\n> works better and I prefer the consistency of implementation.\n>\n\nJunio, I didn't use -4 when sending email so the patch which fixes\napache.conf is not here. I still need it myself for running the test,\nso it should be queued somehow.\n\nRegards,\nJake\n"},{"id":"279793","messageId":"xmqqr3fvcsoy.fsf@gitster.mtv.corp.google.com","threadId":"41545","inReplyTo":"1456532000-22971-2-git-send-email-jacob.e.keller@intel.com","subject":"Re: [PATCH v5 1/3] sumodule--helper: fix submodule--helper clone usage and check argc count","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-29T17:49:49Z","receivedAt":"2016-02-29T17:49:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller writes:\n\n> From: Jacob Keller \n>\n> git submodule--helper clone usage specified that paths come after the --\n> as a sequence. However, the actual implementation does not, and requires\n> only a single path passed in via --path. In addition, argc was\n> unchecked. (allowing arbitrary extra arguments that were silently\n> ignored).\n>\n> Fix the usage description to match implementation. Add an argc check to\n> enforce no extra arguments. \n\nThe above sounds very sensible.\n\n> Fix a bug in the argument passing in\n> git-submodule.sh which would pass --reference and --depth as empty\n> strings when they were unused, resulting in extra argc after parsing\n> options.\n\nThis does make sense but it is an unrelated fix. Perhaps split this\npatch into two?\n\n> Signed-off-by: Jacob Keller \n> ---\n> builtin/submodule--helper.c | 5 ++++-\n> git-submodule.sh | 4 ++--\n> 2 files changed, 6 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index f4c3eff179b5..072d9bbd12a8 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -187,13 +187,16 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n> \tconst char *const git_submodule_helper_usage[] = {\n> \t\tN_(\"git submodule--helper clone [--prefix=] [--quiet] \"\n> \t\t \"[--reference ] [--name ] [--url ]\"\n> -\t\t \"[--depth ] [--] [...]\"),\n> +\t\t \"[--depth ] [--path ]\"),\n> \t\tNULL\n> \t};\n> \n> \targc = parse_options(argc, argv, prefix, module_clone_options,\n> \t\t\t git_submodule_helper_usage, 0);\n> \n> +\tif (argc)\n> +\t\tusage(*git_submodule_helper_usage);\n> +\n\nThat asterisk looks very unusual and wanting to be future-proofed\n(i.e. who says that only the first entry matters?). Should't this\nbe calling usage_with_options()?\n\n> \tstrbuf_addf(&sb, \"%s/modules/%s\", get_git_dir(), name);\n> \tsm_gitdir = strbuf_detach(&sb, NULL);\n> \n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index 9bc5c5f94d1d..2dd29b3df0e6 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -347,7 +347,7 @@ Use -f if you really want to add it.\" >&2\n> \t\t\t\techo \"$(eval_gettext \"Reactivating local git directory for submodule '\\$sm_name'.\")\"\n> \t\t\tfi\n> \t\tfi\n> -\t\tgit submodule--helper clone ${GIT_QUIET:+--quiet} --prefix \"$wt_prefix\" --path \"$sm_path\" --name \"$sm_name\" --url \"$realrepo\" \"$reference\" \"$depth\" || exit\n> +\t\tgit submodule--helper clone ${GIT_QUIET:+--quiet} --prefix \"$wt_prefix\" --path \"$sm_path\" --name \"$sm_name\" --url \"$realrepo\" ${reference:+\"$reference\"} ${depth:+\"$depth\"} || exit\n> \t\t(\n> \t\t\tclear_local_git_env\n> \t\t\tcd \"$sm_path\" &&\n> @@ -709,7 +709,7 @@ Maybe you want to use 'update --init'?\")\"\n> \n> \t\tif ! test -d \"$sm_path\"/.git && ! test -f \"$sm_path\"/.git\n> \t\tthen\n> -\t\t\tgit submodule--helper clone ${GIT_QUIET:+--quiet} --prefix \"$prefix\" --path \"$sm_path\" --name \"$name\" --url \"$url\" \"$reference\" \"$depth\" || exit\n> +\t\t\tgit submodule--helper clone ${GIT_QUIET:+--quiet} --prefix \"$prefix\" --path \"$sm_path\" --name \"$name\" --url \"$url\" ${reference:+\"$reference\"} ${depth:+\"$depth\"} || exit\n> \t\t\tcloned_modules=\"$cloned_modules;$name\"\n> \t\t\tsubsha1=\n> \t\telse\n"},{"id":"279796","messageId":"xmqqmvqjcr95.fsf@gitster.mtv.corp.google.com","threadId":"41545","inReplyTo":"1456532000-22971-4-git-send-email-jacob.e.keller@intel.com","subject":"Re: [PATCH v5 3/3] git: submodule honor -c credential.* from command line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-29T18:20:54Z","receivedAt":"2016-02-29T18:20:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller writes:\n\n> +static int sanitize_submodule_config(const char *var, const char *value, void *data)\n> +{\n> +\tstruct strbuf quoted = STRBUF_INIT;\n> +\tstruct strbuf *out = data;\n> +\n> +\tif (submodule_config_ok(var)) {\n> +\t\tif (out->len)\n> +\t\t\tstrbuf_addch(out, ' ');\n> +\n> +\t\tsq_quotef(out, \"%s=%s\", var, value);\n\nCan a configuration variable that comes from the original command\nline be a boolean true that is spelled without \"=true\", i.e. can\nvalue be NULL here?\n\n> +\t}\n> +\n> +\tstrbuf_release("ed);\n> +\n> +\treturn 0;\n> +}\n> +\n> +static void prepare_submodule_repo_env(struct argv_array *out)\n> +{\n> +\tconst char * const *var;\n> +\n> +\tfor (var = local_repo_env; *var; var++) {\n> +\t\tif (!strcmp(*var, CONFIG_DATA_ENVIRONMENT)) {\n> +\t\t\tstruct strbuf sanitized_config = STRBUF_INIT;\n> +\t\t\tgit_config_from_parameters(sanitize_submodule_config,\n> +\t\t\t\t\t\t &sanitized_config);\n> +\t\t\targv_array_pushf(out, \"%s=%s\", *var, sanitized_config.buf);\n> +\t\t\tstrbuf_release(&sanitized_config);\n"},{"id":"279831","messageId":"CA+P7+xryNo=n0fFn9HNrj78z4XtWwoF=j67+Qy8TbO7reBTO-Q@mail.gmail.com","threadId":"41545","inReplyTo":"xmqqr3fvcsoy.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v5 1/3] sumodule--helper: fix submodule--helper clone usage and check argc count","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-02-29T19:34:32Z","receivedAt":"2016-02-29T19:34:32Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Feb 29, 2016 at 9:49 AM, Junio C Hamano wrote:\n> Jacob Keller writes:\n>> Fix the usage description to match implementation. Add an argc check to\n>> enforce no extra arguments.\n>\n> The above sounds very sensible.\n>\n\nRight.\n\n>> Fix a bug in the argument passing in\n>> git-submodule.sh which would pass --reference and --depth as empty\n>> strings when they were unused, resulting in extra argc after parsing\n>> options.\n>\n> This does make sense but it is an unrelated fix. Perhaps split this\n> patch into two?\n>\n\nThat actually is required because otherwise adding a check for argc\nwould break the things. I could split them and do this first and then\ncheck for argc if you really prefer?\n\n>> + if (argc)\n>> + usage(*git_submodule_helper_usage);\n>> +\n>\n> That asterisk looks very unusual and wanting to be future-proofed\n> (i.e. who says that only the first entry matters?). Should't this\n> be calling usage_with_options()?\n>\n\nI... didn't know usage_with_options was a thing! Hah. I can fix these up.\n\nThanks,\nJake\\\n"},{"id":"279832","messageId":"CA+P7+xoNdsDH0jBz6bcikpeMa-qMR6795U+hKqGDSfu52dJFSw@mail.gmail.com","threadId":"41545","inReplyTo":"xmqqmvqjcr95.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v5 3/3] git: submodule honor -c credential.* from command line","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-02-29T19:37:33Z","receivedAt":"2016-02-29T19:37:33Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Feb 29, 2016 at 10:20 AM, Junio C Hamano wrote:\n> Jacob Keller writes:\n>\n>> +static int sanitize_submodule_config(const char *var, const char *value, void *data)\n>> +{\n>> + struct strbuf quoted = STRBUF_INIT;\n>> + struct strbuf *out = data;\n>> +\n>> + if (submodule_config_ok(var)) {\n>> + if (out->len)\n>> + strbuf_addch(out, ' ');\n>> +\n>> + sq_quotef(out, \"%s=%s\", var, value);\n>\n> Can a configuration variable that comes from the original command\n> line be a boolean true that is spelled without \"=true\", i.e. can\n> value be NULL here?\n>\n\nWouldn't it just be the empty string? I'm not sure. I suppose I can do:\n\nsq_quotef(out, \"%s=%s, var, value ? value : \"\")\n\nOr something?\n\nThanks\nJake\n"},{"id":"279833","messageId":"xmqqegbv9u9a.fsf@gitster.mtv.corp.google.com","threadId":"41545","inReplyTo":"CA+P7+xryNo=n0fFn9HNrj78z4XtWwoF=j67+Qy8TbO7reBTO-Q@mail.gmail.com","subject":"Re: [PATCH v5 1/3] sumodule--helper: fix submodule--helper clone usage and check argc count","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-29T19:44:17Z","receivedAt":"2016-02-29T19:44:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller writes:\n\n>> This does make sense but it is an unrelated fix. Perhaps split this\n>> patch into two?\n>\n> That actually is required because otherwise adding a check for argc\n> would break the things. I could split them and do this first and then\n> check for argc if you really prefer?\n\nIt is not \"check for argc breaks\", it is already broken and by\nchecking for argc you are exposing the breakage, no?\n\nSo I'd say fix that first and then fix the clone subcommand?\n\n\n>\n>>> + if (argc)\n>>> + usage(*git_submodule_helper_usage);\n>>> +\n>>\n>> That asterisk looks very unusual and wanting to be future-proofed\n>> (i.e. who says that only the first entry matters?). Should't this\n>> be calling usage_with_options()?\n>>\n>\n> I... didn't know usage_with_options was a thing! Hah. I can fix these up.\n>\n> Thanks,\n> Jake\\\n"},{"id":"279834","messageId":"xmqqa8mj9u3c.fsf@gitster.mtv.corp.google.com","threadId":"41545","inReplyTo":"CA+P7+xoNdsDH0jBz6bcikpeMa-qMR6795U+hKqGDSfu52dJFSw@mail.gmail.com","subject":"Re: [PATCH v5 3/3] git: submodule honor -c credential.* from command line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-29T19:47:51Z","receivedAt":"2016-02-29T19:47:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller writes:\n\n> On Mon, Feb 29, 2016 at 10:20 AM, Junio C Hamano wrote:\n>> Jacob Keller writes:\n>>\n>>> +static int sanitize_submodule_config(const char *var, const char *value, void *data)\n>>> +{\n>>> + struct strbuf quoted = STRBUF_INIT;\n>>> + struct strbuf *out = data;\n>>> +\n>>> + if (submodule_config_ok(var)) {\n>>> + if (out->len)\n>>> + strbuf_addch(out, ' ');\n>>> +\n>>> + sq_quotef(out, \"%s=%s\", var, value);\n>>\n>> Can a configuration variable that comes from the original command\n>> line be a boolean true that is spelled without \"=true\", i.e. can\n>> value be NULL here?\n>>\n>\n> Wouldn't it just be the empty string?\n\nI was talking about the \"not even an equal sign\" true, i.e.\n\n $ git -c random.what -c random.false=no -c random.true=yes \\\n config --bool --get-regexp 'random.*'\n random.what true\n random.false false\n random.true true\n\nWhat would you see for random.what in the above function (if the\ncallchain allowed you to pass it through)?\n"},{"id":"279859","messageId":"CA+P7+xpTHQ3i68cqbuhWqOR_y0gS1icBcBgj4jjaZ=yw-WrPEw@mail.gmail.com","threadId":"41545","inReplyTo":"xmqqegbv9u9a.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v5 1/3] sumodule--helper: fix submodule--helper clone usage and check argc count","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-02-29T22:05:40Z","receivedAt":"2016-02-29T22:05:40Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Feb 29, 2016 at 11:44 AM, Junio C Hamano wrote:\n> Jacob Keller writes:\n>> That actually is required because otherwise adding a check for argc\n>> would break the things. I could split them and do this first and then\n>> check for argc if you really prefer?\n>\n> It is not \"check for argc breaks\", it is already broken and by\n> checking for argc you are exposing the breakage, no?\n>\n> So I'd say fix that first and then fix the clone subcommand?\n>\n\nOk, so one patch which fixes the empty string arguments first, then\nthe second patch which adds the (now valid) check?\n\nThanks,\nJake\n"},{"id":"279860","messageId":"CA+P7+xrZrVbvh=2dhKqMTGnudy0pCSzGMWHBXyirvsp7hCzaZw@mail.gmail.com","threadId":"41545","inReplyTo":"xmqqa8mj9u3c.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v5 3/3] git: submodule honor -c credential.* from command line","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-02-29T22:09:12Z","receivedAt":"2016-02-29T22:09:12Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Feb 29, 2016 at 11:47 AM, Junio C Hamano wrote:\n> I was talking about the \"not even an equal sign\" true, i.e.\n>\n> $ git -c random.what -c random.false=no -c random.true=yes \\\n> config --bool --get-regexp 'random.*'\n> random.what true\n> random.false false\n> random.true true\n>\n> What would you see for random.what in the above function (if the\n> callchain allowed you to pass it through)?\n\nI see 'credential.what=(null)' if I pass it to sanitize-config. That's\nprobably a bug, and we should instead just output 'credential.what'\ninstead. I'll rework this.\n\nThanks,\nJake\n"}]}