{"thread":{"id":"41573","subject":"[PATCH v6 6/7] quote: implement sq_quotef()","startedAt":"2016-02-29T22:58:28Z","lastAt":"2016-03-23T20:46:03Z","messageCount":27,"participants":["Jacob Keller","Stefan Beller","Jeff King","Junio C Hamano","Jonathan Nieder"],"isPatch":true,"patchVersion":6,"patchTotal":7},"messages":[{"id":"279868","messageId":"1456786715-24256-1-git-send-email-jacob.e.keller@intel.com","threadId":"41573","inReplyTo":null,"subject":"[PATCH v6 0/7] submodule--helper credential.* pass into submodule","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2016-02-29T22:58:28Z","receivedAt":"2016-02-29T22:58:28Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller \n\nThis series implements ability to pass the credential.* configuration\nvariables from the command line directly into the submodule. The other\npatches in this series are small fixups noticed along the way.\n\n--> Interdiff between v5 and v6 <--\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 43943eef81b4..3d37c3f1822d 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -140,18 +140,18 @@ static int submodule_config_ok(const char *var)\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\tif (value)\n+\t\t\tsq_quotef(out, \"%s=%s\", var, value);\n+\t\telse\n+\t\t\tsq_quote_buf(out, var);\n \t}\n \n-\tstrbuf_release("ed);\n-\n \treturn 0;\n }\n \n@@ -235,16 +235,17 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \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 ] [--path ]\"),\n+\t\t \"[--reference ] [--name ] [--depth ] \"\n+\t\t \"--url --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+\tif (argc || !url || !path)\n+\t\tusage_with_options(git_submodule_helper_usage,\n+\t\t\t\t module_clone_options);\n \n \tstrbuf_addf(&sb, \"%s/modules/%s\", get_git_dir(), name);\n \tsm_gitdir = strbuf_detach(&sb, NULL);\n\nJacob Keller (6):\n submodule: don't pass empty string arguments to submodule--helper\n clone\n submodule: check argc count for git submodule--helper clone\n submodule: fix submodule--helper clone usage\n submodule: fix segmentation fault in submodule--helper clone\n quote: implement sq_quotef()\n git: submodule honor -c credential.* from command line\n\nMichael J Gruber (1):\n t/lib-httpd: load mod_unixd\n\n builtin/submodule--helper.c | 76 ++++++++++++++++++++++++++++++++++++++++++--\n git-submodule.sh | 40 ++++++++++++++---------\n quote.c | 13 ++++++++\n quote.h | 3 ++\n t/lib-httpd/apache.conf | 3 ++\n t/t5550-http-fetch-dumb.sh | 17 ++++++++++\n t/t7412-submodule--helper.sh | 26 +++++++++++++++\n 7 files changed, 160 insertions(+), 18 deletions(-)\n create mode 100755 t/t7412-submodule--helper.sh\n\n-- \n2.7.1.429.g45cd78e\n"},{"id":"279870","messageId":"1456786715-24256-2-git-send-email-jacob.e.keller@intel.com","threadId":"41573","inReplyTo":"1456786715-24256-1-git-send-email-jacob.e.keller@intel.com","subject":"[PATCH v6 1/7] t/lib-httpd: load mod_unixd","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2016-02-29T22:58:29Z","receivedAt":"2016-02-29T22:58:29Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Michael J Gruber \n\nIn contrast to apache 2.2, apache 2.4 does not load mod_unixd in its\ndefault configuration (because there are choices). Thus, with the\ncurrent config, apache 2.4.10 will not be started and the httpd tests\nwill not run on distros with default apache config (RedHat type).\n\nEnable mod_unixd to make the httpd tests run. This does not affect\ndistros negatively which have that config already in their default\n(Debian type). httpd tests will run on these before and after this patch.\n\nSigned-off-by: Michael J Gruber \nSigned-off-by: Junio C Hamano \n---\n\nI am sending this as an indicator that we need to queue my submodule\nwork on top of this, because at least locally on my Fedora 23 system I\nan unable to get the httpd tests to pass otherwise.\n\n t/lib-httpd/apache.conf | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf\nindex 7d15e6d44c83..f667e7ce2f33 100644\n--- a/t/lib-httpd/apache.conf\n+++ b/t/lib-httpd/apache.conf\n@@ -64,6 +64,9 @@ LockFile accept.lock\n \n \tLoadModule mpm_prefork_module modules/mod_mpm_prefork.so\n \n+\n+\tLoadModule unixd_module modules/mod_unixd.so\n+\n \n \n PassEnv GIT_VALGRIND\n-- \n2.7.1.429.g45cd78e\n"},{"id":"279872","messageId":"1456786715-24256-3-git-send-email-jacob.e.keller@intel.com","threadId":"41573","inReplyTo":"1456786715-24256-1-git-send-email-jacob.e.keller@intel.com","subject":"[PATCH v6 2/7] submodule: don't pass empty string arguments to submodule--helper clone","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2016-02-29T22:58:30Z","receivedAt":"2016-02-29T22:58:30Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller \n\nWhen --reference or --depth are unused, the current git-submodule.sh\nresults in empty \"\" arguments appended to the end of the argv array\ninside git submodule--helper clone. This is not caught because the argc\ncount is not checked today.\n\nFix git-submodule.sh to only pass an argument when --reference or\n--depth are used, preventing the addition of two empty string arguments\non the tail of the argv array.\n\nSigned-off-by: Jacob Keller \n---\nThis is a split of a previous patch as suggested by Junio, to make it\neasier to review the changes.\n\n git-submodule.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\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":"279869","messageId":"1456786715-24256-4-git-send-email-jacob.e.keller@intel.com","threadId":"41573","inReplyTo":"1456786715-24256-1-git-send-email-jacob.e.keller@intel.com","subject":"[PATCH v6 3/7] submodule: check argc count for git submodule--helper clone","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2016-02-29T22:58:31Z","receivedAt":"2016-02-29T22:58:31Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller \n\nExtra unused arguments to git submodule--helper clone subcommand were\nbeing silently ignored. Add a check to the argc count after options\nhandling to ensure that no extra arguments were left on the argv array.\n\nSigned-off-by: Jacob Keller \n---\n builtin/submodule--helper.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex f4c3eff179b5..1e18075ed90f 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -194,6 +194,10 @@ static int module_clone(int argc, const char **argv, const char *prefix)\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_with_options(git_submodule_helper_usage,\n+\t\t\t\t module_clone_options);\n+\n \tstrbuf_addf(&sb, \"%s/modules/%s\", get_git_dir(), name);\n \tsm_gitdir = strbuf_detach(&sb, NULL);\n \n-- \n2.7.1.429.g45cd78e\n"},{"id":"279867","messageId":"1456786715-24256-5-git-send-email-jacob.e.keller@intel.com","threadId":"41573","inReplyTo":"1456786715-24256-1-git-send-email-jacob.e.keller@intel.com","subject":"[PATCH v6 4/7] submodule: fix submodule--helper clone usage","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2016-02-29T22:58:32Z","receivedAt":"2016-02-29T22:58:32Z","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 stated that paths were added after the\n[--] argument. The actual implementation required use of --path argument\nand only supports one path at a time. Update the usage string to match\nthe current implementation.\n\nSigned-off-by: Jacob Keller \n---\n builtin/submodule--helper.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 1e18075ed90f..3c4d3ff7f4af 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -187,7 +187,7 @@ 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-- \n2.7.1.429.g45cd78e\n"},{"id":"279866","messageId":"1456786715-24256-6-git-send-email-jacob.e.keller@intel.com","threadId":"41573","inReplyTo":"1456786715-24256-1-git-send-email-jacob.e.keller@intel.com","subject":"[PATCH v6 5/7] submodule: fix segmentation fault in submodule--helper clone","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2016-02-29T22:58:33Z","receivedAt":"2016-02-29T22:58:33Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller \n\nThe git submodule--helper clone command will fail with a segmentation\nfault when given a null url or null path variable. Since these are\nrequired for proper functioning of the submodule--helper clone\nsubcommand, add checks to prevent running and fail gracefully when\nmissing.\n\nUpdate the usage string to reflect the requirement that the --url and\n--path \"options\" are required.\n\nSigned-off-by: Jacob Keller \n---\nThis bug was discovered when trying to test the usage string of the\nprevious change. I bisected and this has been in the submodule--helper\ncode since the first iteration of module_clone. I am not really sure how\nmuch we care, since only internal callers will be using\nsubmodule--helper, but I suspect that it was not intended to segfault.\n\nI fixed the usage string to remove the [] around the url and path\noptions, hopefully indicating they aren't \"optional\". Alternatively we\ncould start requiring them as regular arguments if we wanted.\n\nNote the error message resulting from --url=\"\" or --path=\"\" are not very\nobvious but they do not result in a segfault so I didn't check for those\nin this patch.\n\n builtin/submodule--helper.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 3c4d3ff7f4af..35ae85a7e1bc 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -186,15 +186,15 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \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 ] [--path ]\"),\n+\t\t \"[--reference ] [--name ] [--depth ] \"\n+\t\t \"--url --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+\tif (argc || !url || !path)\n \t\tusage_with_options(git_submodule_helper_usage,\n \t\t\t\t module_clone_options);\n \n-- \n2.7.1.429.g45cd78e\n"},{"id":"279865","messageId":"1456786715-24256-7-git-send-email-jacob.e.keller@intel.com","threadId":"41573","inReplyTo":"1456786715-24256-1-git-send-email-jacob.e.keller@intel.com","subject":"[PATCH v6 6/7] quote: implement sq_quotef()","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2016-02-29T22:58:34Z","receivedAt":"2016-02-29T22:58:34Z","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":"279871","messageId":"1456786715-24256-8-git-send-email-jacob.e.keller@intel.com","threadId":"41573","inReplyTo":"1456786715-24256-1-git-send-email-jacob.e.keller@intel.com","subject":"[PATCH v6 7/7] git: submodule honor -c credential.* from command line","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2016-02-29T22:58:35Z","receivedAt":"2016-02-29T22:58:35Z","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 - v6\n * remove unused strbuf 'quoted'\n * handle empty configuration variables\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 35ae85a7e1bc..3d37c3f1822d 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 *out = data;\n+\n+\tif (submodule_config_ok(var)) {\n+\t\tif (out->len)\n+\t\t\tstrbuf_addch(out, ' ');\n+\n+\t\tif (value)\n+\t\t\tsq_quotef(out, \"%s=%s\", var, value);\n+\t\telse\n+\t\t\tsq_quote_buf(out, var);\n+\t}\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@@ -259,6 +308,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@@ -268,6 +333,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":"279874","messageId":"CAGZ79kY+AbOcwPnW2LSno=z8VueXgOgeB0O+MS_aOML7NNS0Cg@mail.gmail.com","threadId":"41573","inReplyTo":"1456786715-24256-4-git-send-email-jacob.e.keller@intel.com","subject":"Re: [PATCH v6 3/7] submodule: check argc count for git submodule--helper clone","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-29T23:14:03Z","receivedAt":"2016-02-29T23:14:03Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Feb 29, 2016 at 2:58 PM, Jacob Keller wrote:\n>\n> Extra unused arguments to git submodule--helper clone subcommand were\n> being silently ignored. Add a check to the argc count after options\n> handling to ensure that no extra arguments were left on the argv array.\n>\n> Signed-off-by: Jacob Keller \n\nReviewed-by: Stefan Beller \n"},{"id":"279875","messageId":"CAGZ79kZGzXHJ=oWfx8tsTEs=OU7c+KuV-G9sA7ABHQGQtXLOzQ@mail.gmail.com","threadId":"41573","inReplyTo":"1456786715-24256-3-git-send-email-jacob.e.keller@intel.com","subject":"Re: [PATCH v6 2/7] submodule: don't pass empty string arguments to submodule--helper clone","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-29T23:14:45Z","receivedAt":"2016-02-29T23:14:45Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Feb 29, 2016 at 2:58 PM, Jacob Keller wrote:\n> From: Jacob Keller \n>\n> When --reference or --depth are unused, the current git-submodule.sh\n> results in empty \"\" arguments appended to the end of the argv array\n> inside git submodule--helper clone. This is not caught because the argc\n> count is not checked today.\n>\n> Fix git-submodule.sh to only pass an argument when --reference or\n> --depth are used, preventing the addition of two empty string arguments\n> on the tail of the argv array.\n>\n> Signed-off-by: Jacob Keller \n\nReviewed-by: Stefan Beller \n"},{"id":"279876","messageId":"CAGZ79kaTSyqYWumBPn6fSQdyiEsyXTBxcH4DjTRo6vyhZ+d-mg@mail.gmail.com","threadId":"41573","inReplyTo":"1456786715-24256-5-git-send-email-jacob.e.keller@intel.com","subject":"Re: [PATCH v6 4/7] submodule: fix submodule--helper clone usage","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-29T23:15:06Z","receivedAt":"2016-02-29T23:15:06Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Feb 29, 2016 at 2:58 PM, Jacob Keller wrote:\n> From: Jacob Keller \n>\n> git submodule--helper clone usage stated that paths were added after the\n> [--] argument. The actual implementation required use of --path argument\n> and only supports one path at a time. Update the usage string to match\n> the current implementation.\n>\n> Signed-off-by: Jacob Keller \n\nReviewed-by: Stefan Beller \n"},{"id":"279877","messageId":"CAGZ79kZdct21==TeRoVr4NpgeKuHeUfpJ1Qsh+_tPJt0apr5DA@mail.gmail.com","threadId":"41573","inReplyTo":"1456786715-24256-6-git-send-email-jacob.e.keller@intel.com","subject":"Re: [PATCH v6 5/7] submodule: fix segmentation fault in submodule--helper clone","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-29T23:20:53Z","receivedAt":"2016-02-29T23:20:53Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Feb 29, 2016 at 2:58 PM, Jacob Keller wrote:\n> From: Jacob Keller \n>\n> The git submodule--helper clone command will fail with a segmentation\n> fault when given a null url or null path variable. Since these are\n> required for proper functioning of the submodule--helper clone\n> subcommand, add checks to prevent running and fail gracefully when\n> missing.\n>\n> Update the usage string to reflect the requirement that the --url and\n> --path \"options\" are required.\n>\n> Signed-off-by: Jacob Keller \n> ---\n> This bug was discovered when trying to test the usage string of the\n> previous change. I bisected and this has been in the submodule--helper\n> code since the first iteration of module_clone. I am not really sure how\n> much we care, since only internal callers will be using\n> submodule--helper, but I suspect that it was not intended to segfault.\n>\n> I fixed the usage string to remove the [] around the url and path\n> options, hopefully indicating they aren't \"optional\". Alternatively we\n> could start requiring them as regular arguments if we wanted.\n>\n> Note the error message resulting from --url=\"\" or --path=\"\" are not very\n> obvious but they do not result in a segfault so I didn't check for those\n> in this patch.\n\nI think this bug was put in, by \"literally\" translating from shell,\nsee ee8838d15776, where the shell code was rewritten to C,\nspecially:\n\n git clone $quiet ${depth:+\"$depth\"} -n ${reference:+\"$reference\"} \\\n --separate-git-dir \"$gitdir\" \"$url\" \"$sm_path\"\n\nAnything except url and path are done optionally, but url, and path not so.\nThe C code was inspired by this and aligned.\n\nThis patch makes sense to me,\nThanks,\nStefan\n\n\n>\n> builtin/submodule--helper.c | 6 +++---\n> 1 file changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 3c4d3ff7f4af..35ae85a7e1bc 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -186,15 +186,15 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n>\n> const char *const git_submodule_helper_usage[] = {\n> N_(\"git submodule--helper clone [--prefix=] [--quiet] \"\n> - \"[--reference ] [--name ] [--url ]\"\n> - \"[--depth ] [--path ]\"),\n> + \"[--reference ] [--name ] [--depth ] \"\n> + \"--url --path \"),\n> NULL\n> };\n>\n> argc = parse_options(argc, argv, prefix, module_clone_options,\n> git_submodule_helper_usage, 0);\n>\n> - if (argc)\n> + if (argc || !url || !path)\n> usage_with_options(git_submodule_helper_usage,\n> module_clone_options);\n>\n> --\n> 2.7.1.429.g45cd78e\n>\n"},{"id":"279878","messageId":"CA+P7+xpOBa5sKQyZRVpGU9kxn9281vpnF4rUYyNOeBtWiqjjSg@mail.gmail.com","threadId":"41573","inReplyTo":"CAGZ79kZdct21==TeRoVr4NpgeKuHeUfpJ1Qsh+_tPJt0apr5DA@mail.gmail.com","subject":"Re: [PATCH v6 5/7] submodule: fix segmentation fault in submodule--helper clone","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-02-29T23:36:40Z","receivedAt":"2016-02-29T23:36: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 3:20 PM, Stefan Beller wrote:\n>\n> I think this bug was put in, by \"literally\" translating from shell,\n> see ee8838d15776, where the shell code was rewritten to C,\n> specially:\n>\n> git clone $quiet ${depth:+\"$depth\"} -n ${reference:+\"$reference\"} \\\n> --separate-git-dir \"$gitdir\" \"$url\" \"$sm_path\"\n>\n> Anything except url and path are done optionally, but url, and path not so.\n> The C code was inspired by this and aligned.\n>\n> This patch makes sense to me,\n> Thanks,\n> Stefan\n>\n\nThe only thing that bothers me is that git submodule--helper clone\nuses --path and --url instead of just taking two arguments after all\nthe optional parameters. Traditionally options are supposed to be just\nthat, optional. However given that long term we do not intend to keep\nsubmodule--helper, I am not too worried about how it works here.\n\nThanks,\nJake\n"},{"id":"279880","messageId":"CAGZ79kbDaV=i0augzh5RgGYpTWXOuTLx=7Occhc-6iE+0pBVNg@mail.gmail.com","threadId":"41573","inReplyTo":"1456786715-24256-8-git-send-email-jacob.e.keller@intel.com","subject":"Re: [PATCH v6 7/7] git: submodule honor -c credential.* from command line","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-29T23:39:28Z","receivedAt":"2016-02-29T23:39:28Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Feb 29, 2016 at 2:58 PM, Jacob Keller wrote:\n>\n> +test_expect_success 'cmdline credential config passes into submodules' '\n> + git init super &&\n> + set_askpass user@host pass@host &&\n\nI wonder why we use pass@host as a password, it seems confusing to me.\np@ssword would have worked if we wanted to test a password containing an @,\npass@host doesn't quite fit my mental model of how passwords work.\nNo need to change anything, better be consistent with the rest of the tests.\n\n\n> + (\n> + cd super &&\n> + git submodule add \"$HTTPD_URL/auth/dumb/repo.git\" sub &&\n> + git commit -m \"add submodule\"\n> + ) &&\n> + set_askpass wrong pass@host &&\n> + test_must_fail git clone --recursive super super-clone &&\n> + rm -rf super-clone &&\n> + set_askpass wrong pass@host &&\n\nWhy set set_askpass a second time here?\n\n> + git -c \"credential.$HTTP_URL.username=user@host\" \\\n> + clone --recursive super super-clone &&\n> + expect_askpass pass user@host\n> +'\n\nThanks for this test!\nStefan\n"},{"id":"279881","messageId":"CA+P7+xrr61wO0XrhXCEbSLPbSo7HxxzDWnq=6K14fvyo7RfscA@mail.gmail.com","threadId":"41573","inReplyTo":"CAGZ79kbDaV=i0augzh5RgGYpTWXOuTLx=7Occhc-6iE+0pBVNg@mail.gmail.com","subject":"Re: [PATCH v6 7/7] git: submodule honor -c credential.* from command line","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-02-29T23:44:51Z","receivedAt":"2016-02-29T23:44:51Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Feb 29, 2016 at 3:39 PM, Stefan Beller wrote:\n> On Mon, Feb 29, 2016 at 2:58 PM, Jacob Keller wrote:\n>>\n>> +test_expect_success 'cmdline credential config passes into submodules' '\n>> + git init super &&\n>> + set_askpass user@host pass@host &&\n>\n> I wonder why we use pass@host as a password, it seems confusing to me.\n> p@ssword would have worked if we wanted to test a password containing an @,\n> pass@host doesn't quite fit my mental model of how passwords work.\n> No need to change anything, better be consistent with the rest of the tests.\n>\n\nI am not sure, but I don't think it particularly matters what we use.\nMost of this is pretty much copied as suggested by Peff.\n\n>\n>> + (\n>> + cd super &&\n>> + git submodule add \"$HTTPD_URL/auth/dumb/repo.git\" sub &&\n>> + git commit -m \"add submodule\"\n>> + ) &&\n>> + set_askpass wrong pass@host &&\n>> + test_must_fail git clone --recursive super super-clone &&\n>> + rm -rf super-clone &&\n>> + set_askpass wrong pass@host &&\n>\n> Why set set_askpass a second time here?\n\nInteresingly, it fails unless I add this line with:\n\nSubmodule path 'sub': checked out '678c534310f3cd5727f8e066cba0cd420bd7948e'\n--- \"/home/jekeller/git/git/t/trash\ndirectory.t5550-http-fetch-dumb/askpass-expect\" 2016-02-29\n23:43:35.724179569 +0000\n+++ \"/home/jekeller/git/git/t/trash\ndirectory.t5550-http-fetch-dumb/askpass-query\" 2016-02-29\n23:43:35.681179568 +0000\n@@ -1 +1,3 @@\n+askpass: Username for 'http://127.0.0.1:5550':\n+askpass: Password for 'http://wrong@127.0.0.1:5550':\n askpass: Password for 'http://user@host@127.0.0.1:5550':\nnot ok 13 - cmdline credential config passes into submodules\n#\n# git init super &&\n# set_askpass user@host pass@host &&\n# (\n# cd super &&\n# git submodule add \"$HTTPD_URL/auth/dumb/repo.git\" sub &&\n# git commit -m \"add submodule\"\n# ) &&\n# set_askpass wrong pass@host &&\n# test_must_fail git clone --recursive super super-clone &&\n# rm -rf super-clone &&\n# git -c \"credential.$HTTP_URL.username=user@host\" \\\n# clone --recursive super super-clone &&\n# expect_askpass pass user@host\n#\n\nI really don't understand why adding the extra askpass setting fixes\nthis? Possibly because the query and expect files are appended to? Or\nsomething else subtle going on?\n\nBasically: without this, the expect vs the query don't match. With it,\nthey do. I don't understand the reasoning why.\n\n>\n> Thanks for this test!\n> Stefan\n\nJeff King suggested it, originally.\n\nThanks,\nJake\n"},{"id":"279921","messageId":"20160301013616.GA18301@sigill.intra.peff.net","threadId":"41573","inReplyTo":"CA+P7+xrr61wO0XrhXCEbSLPbSo7HxxzDWnq=6K14fvyo7RfscA@mail.gmail.com","subject":"Re: [PATCH v6 7/7] git: submodule honor -c credential.* from command line","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-01T01:36:16Z","receivedAt":"2016-03-01T01:36:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 29, 2016 at 03:44:51PM -0800, Jacob Keller wrote:\n\n> On Mon, Feb 29, 2016 at 3:39 PM, Stefan Beller wrote:\n> > On Mon, Feb 29, 2016 at 2:58 PM, Jacob Keller wrote:\n> >>\n> >> +test_expect_success 'cmdline credential config passes into submodules' '\n> >> + git init super &&\n> >> + set_askpass user@host pass@host &&\n> >\n> > I wonder why we use pass@host as a password, it seems confusing to me.\n> > p@ssword would have worked if we wanted to test a password containing an @,\n> > pass@host doesn't quite fit my mental model of how passwords work.\n> > No need to change anything, better be consistent with the rest of the tests.\n> >\n> \n> I am not sure, but I don't think it particularly matters what we use.\n> Most of this is pretty much copied as suggested by Peff.\n\nYes, this dates back to 3cf8fe1 (t5550: test HTTP authentication and\nuserinfo decoding, 2010-11-14), and the point is just to have an \"@\" in\neach field. And then because that's the only username defined in the\napache setup, it's the one all the tests use. :)\n\nI also have found it off-putting, because it looks like \"host\" is\nsupposed to be meaningful. Perhaps the original author had a case where\ntheir web authentication involved a hostname (which seems plausible for\na hosting site which covers multiple domains).\n\nIt might be nice to fix that, either by using something like \"us@r\" and\n\"p@ssword\", or possibly by just introducing a new, easier to read\nalternative to lib-httpd/passwd and using it, as most sites are not\ntesting URL parsing (though I suppose that using the @-filled ones\nconsistently means we _do_ test the other code paths more thoroughly).\n\nBut either way, I don't think it should be part of this series, which\nhas already expanded well beyond its original 1-patch goal.\n\n> > Why set set_askpass a second time here?\n> \n> Interesingly, it fails unless I add this line with:\n> [...]\n> I really don't understand why adding the extra askpass setting fixes\n> this? Possibly because the query and expect files are appended to? Or\n> something else subtle going on?\n\nRight. The askpass test-helper logs the queries it gets, and\nexpect_askpass makes sure that we got the right set of queries. It has\nto append to the query-log because it may be invoked multiple times\n(e.g., once for the username, once for the password). So we have to\nclear the query log, and that is one of the things that set_askpass does\n(the other, obviously, is writing the user/pass values for askpass to\nfeed back to git). Perhaps it should have been called init_askpass or\nsomething, but I don't think it's worth changing now.\n\nYou could get away with just:\n\n >\"$TRASH_DIRECTORY/askpass-query\"\n\nbut IMHO it is less obscure to just call set_askpass a second time.\n\n-Peff\n"},{"id":"279983","messageId":"xmqqh9gq85yc.fsf@gitster.mtv.corp.google.com","threadId":"41573","inReplyTo":"CAGZ79kbDaV=i0augzh5RgGYpTWXOuTLx=7Occhc-6iE+0pBVNg@mail.gmail.com","subject":"Re: [PATCH v6 7/7] git: submodule honor -c credential.* from command line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-01T17:26:51Z","receivedAt":"2016-03-01T17:26:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> On Mon, Feb 29, 2016 at 2:58 PM, Jacob Keller wrote:\n>>\n>> +test_expect_success 'cmdline credential config passes into submodules' '\n>> + git init super &&\n>> + set_askpass user@host pass@host &&\n>\n> I wonder why we use pass@host as a password, it seems confusing to me.\n> p@ssword would have worked if we wanted to test a password containing an @,\n> pass@host doesn't quite fit my mental model of how passwords work.\n> No need to change anything, better be consistent with the rest of the tests.\n>\n>\n>> + (\n>> + cd super &&\n>> + git submodule add \"$HTTPD_URL/auth/dumb/repo.git\" sub &&\n>> + git commit -m \"add submodule\"\n>> + ) &&\n>> + set_askpass wrong pass@host &&\n>> + test_must_fail git clone --recursive super super-clone &&\n>> + rm -rf super-clone &&\n>> + set_askpass wrong pass@host &&\n>\n> Why set set_askpass a second time here?\n\nI find this in t/lib-httpd.sh:\n\n set_askpass() {\n >\"$TRASH_DIRECTORY/askpass-query\" &&\n echo \"$1\" >\"$TRASH_DIRECTORY/askpass-user\" &&\n echo \"$2\" >\"$TRASH_DIRECTORY/askpass-pass\"\n }\n\nand expect_askpass peeks at askpass-query to see if Git asked the\nright questions.\n\nI think askpass-query is cleared here because the author of the test\nsuite expected that the helpers are used in such a way that you\nalways (1) set-askpass once, (2) run a Git command that asks\ncredentials, (3) use expect_askpass to validate and do these three\nsteps as a logical unit?\n\nThat \"clone\" the test expects to fail does ask the credential, so\neven though the test does not check if the \"clone\" asked the right\nquestion, it finishes the three-step logical unit, and then you need\nto clear askpass-query.\n\nIt may have been cleaner if you had clear_askpass_query helper that\nis called (1) at the beginning of set_askpass instead of this manual\nclearing, (2) at the end of expect_askpass, as the exchange has been\ntested already at that point, and (3) in place of expect_askpass if\nyou choose not to call it (e.g. this place, instead of the second\nset_askpass, you would say clear_askpass_query), perhaps, but I do\nknow if that is worth it.\n"},{"id":"279989","messageId":"CA+P7+xp41mkHjA0CF=69extO4R2Oam2V3sJA7PoqNbHD=9kw+g@mail.gmail.com","threadId":"41573","inReplyTo":"xmqqh9gq85yc.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v6 7/7] git: submodule honor -c credential.* from command line","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-03-01T18:05:34Z","receivedAt":"2016-03-01T18:05:34Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Tue, Mar 1, 2016 at 9:26 AM, Junio C Hamano wrote:\n> I find this in t/lib-httpd.sh:\n>\n> set_askpass() {\n> >\"$TRASH_DIRECTORY/askpass-query\" &&\n> echo \"$1\" >\"$TRASH_DIRECTORY/askpass-user\" &&\n> echo \"$2\" >\"$TRASH_DIRECTORY/askpass-pass\"\n> }\n>\n> and expect_askpass peeks at askpass-query to see if Git asked the\n> right questions.\n>\n> I think askpass-query is cleared here because the author of the test\n> suite expected that the helpers are used in such a way that you\n> always (1) set-askpass once, (2) run a Git command that asks\n> credentials, (3) use expect_askpass to validate and do these three\n> steps as a logical unit?\n>\n> That \"clone\" the test expects to fail does ask the credential, so\n> even though the test does not check if the \"clone\" asked the right\n> question, it finishes the three-step logical unit, and then you need\n> to clear askpass-query.\n>\n> It may have been cleaner if you had clear_askpass_query helper that\n> is called (1) at the beginning of set_askpass instead of this manual\n> clearing, (2) at the end of expect_askpass, as the exchange has been\n> tested already at that point, and (3) in place of expect_askpass if\n> you choose not to call it (e.g. this place, instead of the second\n> set_askpass, you would say clear_askpass_query), perhaps, but I do\n> know if that is worth it.\n>\n\nProbably something worth looking at doing in the future.\n\nI could call expect_askpass here at each time but I don't think it\nwould be meaningful after a test_must_fail.\n\nThanks,\nJake\n"},{"id":"279993","messageId":"xmqqmvqi6mqb.fsf@gitster.mtv.corp.google.com","threadId":"41573","inReplyTo":"CA+P7+xp41mkHjA0CF=69extO4R2Oam2V3sJA7PoqNbHD=9kw+g@mail.gmail.com","subject":"Re: [PATCH v6 7/7] git: submodule honor -c credential.* from command line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-01T19:07:24Z","receivedAt":"2016-03-01T19:07:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller writes:\n\n> On Tue, Mar 1, 2016 at 9:26 AM, Junio C Hamano wrote:\n>> I find this in t/lib-httpd.sh:\n>>\n>> set_askpass() {\n>> >\"$TRASH_DIRECTORY/askpass-query\" &&\n>> echo \"$1\" >\"$TRASH_DIRECTORY/askpass-user\" &&\n>> echo \"$2\" >\"$TRASH_DIRECTORY/askpass-pass\"\n>> }\n>>\n>> and expect_askpass peeks at askpass-query to see if Git asked the\n>> right questions.\n>>\n>> I think askpass-query is cleared here because the author of the test\n>> suite expected that the helpers are used in such a way that you\n>> always (1) set-askpass once, (2) run a Git command that asks\n>> credentials, (3) use expect_askpass to validate and do these three\n>> steps as a logical unit?\n>>\n>> That \"clone\" the test expects to fail does ask the credential, so\n>> even though the test does not check if the \"clone\" asked the right\n>> question, it finishes the three-step logical unit, and then you need\n>> to clear askpass-query.\n>>\n>> It may have been cleaner if you had clear_askpass_query helper that\n>> is called (1) at the beginning of set_askpass instead of this manual\n>> clearing, (2) at the end of expect_askpass, as the exchange has been\n>> tested already at that point, and (3) in place of expect_askpass if\n>> you choose not to call it (e.g. this place, instead of the second\n>> set_askpass, you would say clear_askpass_query), perhaps, but I do\n>> know if that is worth it.\n>\n> Probably something worth looking at doing in the future.\n>\n> I could call expect_askpass here at each time but I don't think it\n> would be meaningful after a test_must_fail.\n\nEven if you call expect_askpass to check, another set_askpass is\nexpected to start the next cycle anyway (unless we restructure the\nhelpers around clear_askpass_query I alluded to, which I am not\nconvinced is a good idea yet), so you'll still be asked \"why another\nset_askpass to set the same 'wrong pass@host'?\".\n\nSo, I dunno.\n"},{"id":"280010","messageId":"CA+P7+xrXiGT+Pv07C6oPx0+opdnHmd3SQWPo3tVYO2LvkqOtnw@mail.gmail.com","threadId":"41573","inReplyTo":"xmqqmvqi6mqb.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v6 7/7] git: submodule honor -c credential.* from command line","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-03-01T22:03:45Z","receivedAt":"2016-03-01T22:03:45Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Tue, Mar 1, 2016 at 11:07 AM, Junio C Hamano wrote:\n>> I could call expect_askpass here at each time but I don't think it\n>> would be meaningful after a test_must_fail.\n>\n> Even if you call expect_askpass to check, another set_askpass is\n> expected to start the next cycle anyway (unless we restructure the\n> helpers around clear_askpass_query I alluded to, which I am not\n> convinced is a good idea yet), so you'll still be asked \"why another\n> set_askpass to set the same 'wrong pass@host'?\".\n>\n> So, I dunno.\n\nCorrect. I think it's not worth changing anything here now. The bigger\nsolution to change to clear_askpass_query might be the right answer\nbut it's a lot more involved.\n\nThanks,\nJake\n"},{"id":"281467","messageId":"20160322185628.GA19993@google.com","threadId":"41573","inReplyTo":"1456786715-24256-8-git-send-email-jacob.e.keller@intel.com","subject":"Re: [PATCH v6 7/7] git: submodule honor -c credential.* from command line","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2016-03-22T18:56:28Z","receivedAt":"2016-03-22T18:56:28Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJacob Keller wrote:\n\n> Due to the way that the git-submodule code works, it clears all local\n> git environment variables before entering submodules. This is normally\n> a good thing since we want to clear settings such as GIT_WORKTREE and\n> other variables which would affect the operation of submodule commands.\n> However, GIT_CONFIG_PARAMETERS is special, and we actually do want to\n> preserve these settings. However, we do not want to preserve all\n> configuration as many things should be left specific to the parent\n> project.\n>\n> Add a git submodule--helper function, sanitize-config, which shall be\n> used to sanitize GIT_CONFIG_PARAMETERS, removing all key/value pairs\n> except a small subset that are known to be safe and necessary.\n>\n> Replace all the calls to clear_local_git_env with a wrapped function\n> that filters GIT_CONFIG_PARAMETERS using the new helper and then\n> restores it to the filtered subset after clearing the rest of the\n> environment.\n\nThis is failing for me when I use \"git submodule add\" with a remote\nhelper I whitelisted with GIT_ALLOW_PROTOCOL, with current \"next\":\n\n $ bin-wrappers/git submodule add persistent-https://kernel.googlesource.com/pub/scm/git/git sm\n Cloning into 'sm'...\n error: bogus format in GIT_CONFIG_PARAMETERS\n fatal: unable to parse command-line config\n fatal: clone of 'persistent-https://kernel.googlesource.com/pub/scm/git/git' into submodule path 'sm' failed\n\nsq_dequote_to_argv doesn't like the space at the beginning of\n$GIT_CONFIG_PARAMETERS. Reverting 14111fc4 fixes it. Known\nproblem?\n\nThanks,\nJonathan\n"},{"id":"281469","messageId":"20160322192309.GA9782@sigill.intra.peff.net","threadId":"41573","inReplyTo":"20160322185628.GA19993@google.com","subject":"Re: [PATCH v6 7/7] git: submodule honor -c credential.* from command line","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-22T19:23:10Z","receivedAt":"2016-03-22T19:23:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 22, 2016 at 11:56:28AM -0700, Jonathan Nieder wrote:\n\n> This is failing for me when I use \"git submodule add\" with a remote\n> helper I whitelisted with GIT_ALLOW_PROTOCOL, with current \"next\":\n> \n> $ bin-wrappers/git submodule add persistent-https://kernel.googlesource.com/pub/scm/git/git sm\n> Cloning into 'sm'...\n> error: bogus format in GIT_CONFIG_PARAMETERS\n> fatal: unable to parse command-line config\n> fatal: clone of 'persistent-https://kernel.googlesource.com/pub/scm/git/git' into submodule path 'sm' failed\n> \n> sq_dequote_to_argv doesn't like the space at the beginning of\n> $GIT_CONFIG_PARAMETERS. Reverting 14111fc4 fixes it. Known\n> problem?\n\nIt's known that the parsing end is excessively picky, but not this\nparticular bug. I found the problem; I'll have a patch out in a few\nminute after I write a test.\n\n-Peff\n"},{"id":"281475","messageId":"20160322195051.GA20563@sigill.intra.peff.net","threadId":"41573","inReplyTo":"20160322192309.GA9782@sigill.intra.peff.net","subject":"[PATCH] git_config_push_parameter: handle empty GIT_CONFIG_PARAMETERS","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-22T19:50:51Z","receivedAt":"2016-03-22T19:50:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 22, 2016 at 03:23:09PM -0400, Jeff King wrote:\n\n> On Tue, Mar 22, 2016 at 11:56:28AM -0700, Jonathan Nieder wrote:\n> \n> > This is failing for me when I use \"git submodule add\" with a remote\n> > helper I whitelisted with GIT_ALLOW_PROTOCOL, with current \"next\":\n> > \n> > $ bin-wrappers/git submodule add persistent-https://kernel.googlesource.com/pub/scm/git/git sm\n> > Cloning into 'sm'...\n> > error: bogus format in GIT_CONFIG_PARAMETERS\n> > fatal: unable to parse command-line config\n> > fatal: clone of 'persistent-https://kernel.googlesource.com/pub/scm/git/git' into submodule path 'sm' failed\n> > \n> > sq_dequote_to_argv doesn't like the space at the beginning of\n> > $GIT_CONFIG_PARAMETERS. Reverting 14111fc4 fixes it. Known\n> > problem?\n> \n> It's known that the parsing end is excessively picky, but not this\n> particular bug. I found the problem; I'll have a patch out in a few\n> minute after I write a test.\n\nHere it is.\n\nObviously another option would be to make the parsing side more liberal\n(which has the added effect that if anybody _else_ ever wants to\ngenerate $GIT_CONFIG_PARAMETERS, they will not get annoyed). But I took\nthis route for now because it's the simplest way to fix the regression.\nAnd even if we do later make the parser more liberal, it's still a good\nidea to keep the output from the generating side as clean as possible.\n\n-- >8 --\nSubject: git_config_push_parameter: handle empty GIT_CONFIG_PARAMETERS\n\nThe \"git -c var=value\" option stuffs the config value into\n$GIT_CONFIG_PARAMETERS, so that sub-processes can see it.\nWhen the config is later read via git_config() or similar,\nwe parse it back out of that variable. The parsing end is a\nlittle bit picky; it assumes that each entry was generated\nwith sq_quote_buf(), and that there is no extraneous\nwhitespace.\n\nOn the generating end, we are careful to append to an\nexisting $GIT_CONFIG_PARAMETERS variable if it exists.\nHowever, our test for \"should we add a space separator\" is\ntoo liberal: it will add one even if the environment\nvariable exists but is empty. As a result, you might end up\nwith:\n\n GIT_CONFIG_PARAMETERS=\" 'core.foo=bar'\"\n\nwhich the parser will choke on.\n\nThis was hard to trigger in older versions of git, since we\nonly set the variable when we had something to put into it\n(though you could certainly trigger it manually). But since\n14111fc (git: submodule honor -c credential.* from command\nline, 2016-02-29), the submodule code will unconditionally\nput the $GIT_CONFIG_PARAMETERS variable into the environment\nof any operation in the submodule, whether it is empty or\nnot. So any of those operations which themselves use \"git\n-c\" will generate the unparseable value and fail.\n\nWe can easily fix it by catching this case on the generating\nside. While we're adding a test, let's also check that\nmultiple layers of \"git -c\" work, which was previously not\ntested at all.\n\nReported-by: Jonathan Nieder \nSigned-off-by: Jeff King \n---\nI just did this on master, and it is standalone. But for the reasons\nabove I think it would also be fine to stick on the tip of the\njk/submodule-c-credential topic.\n\n config.c | 2 +-\n t/t1300-repo-config.sh | 14 ++++++++++++++\n 2 files changed, 15 insertions(+), 1 deletion(-)\n\ndiff --git a/config.c b/config.c\nindex 9ba40bc..8f66519 100644\n--- a/config.c\n+++ b/config.c\n@@ -162,7 +162,7 @@ void git_config_push_parameter(const char *text)\n {\n \tstruct strbuf env = STRBUF_INIT;\n \tconst char *old = getenv(CONFIG_DATA_ENVIRONMENT);\n-\tif (old) {\n+\tif (old && *old) {\n \t\tstrbuf_addstr(&env, old);\n \t\tstrbuf_addch(&env, ' ');\n \t}\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex 8867ce1..2b58312 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -1091,6 +1091,20 @@ test_expect_success 'git -c complains about empty key and value' '\n \ttest_must_fail git -c \"\" rev-parse\n '\n \n+test_expect_success 'multiple git -c appends config' '\n+\ttest_config alias.x \"!git -c x.two=2 config --get-regexp ^x\\.*\" &&\n+\tcat >expect <<-\\EOF &&\n+\tx.one 1\n+\tx.two 2\n+\tEOF\n+\tgit -c x.one=1 x >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'git -c is not confused by empty environment' '\n+\tGIT_CONFIG_PARAMETERS=\"\" git -c x.one=1 config --list\n+'\n+\n test_expect_success 'git config --edit works' '\n \tgit config -f tmp test.value no &&\n \techo test.value=yes >expect &&\n-- \n2.8.0.rc4.388.gdee5eca\n"},{"id":"281479","messageId":"xmqqk2kugst3.fsf@gitster.mtv.corp.google.com","threadId":"41573","inReplyTo":"20160322195051.GA20563@sigill.intra.peff.net","subject":"Re: [PATCH] git_config_push_parameter: handle empty GIT_CONFIG_PARAMETERS","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-22T20:29:28Z","receivedAt":"2016-03-22T20:29:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Obviously another option would be to make the parsing side more liberal\n> (which has the added effect that if anybody _else_ ever wants to\n> generate $GIT_CONFIG_PARAMETERS, they will not get annoyed). But I took\n> this route for now because it's the simplest way to fix the regression.\n> And even if we do later make the parser more liberal, it's still a good\n> idea to keep the output from the generating side as clean as possible.\n\nSounds sensible. Will queue. Thanks. \n\n> I just did this on master, and it is standalone. But for the reasons\n> above I think it would also be fine to stick on the tip of the\n> jk/submodule-c-credential topic.\n\nYup, I think that is fine.\n"},{"id":"281495","messageId":"20160322214355.GI28749@google.com","threadId":"41573","inReplyTo":"20160322195051.GA20563@sigill.intra.peff.net","subject":"Re: [PATCH] git_config_push_parameter: handle empty GIT_CONFIG_PARAMETERS","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2016-03-22T21:43:55Z","receivedAt":"2016-03-22T21:43:55Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote[1]:\n\n> Subject: git_config_push_parameter: handle empty GIT_CONFIG_PARAMETERS\n>\n> The \"git -c var=value\" option stuffs the config value into\n> $GIT_CONFIG_PARAMETERS, so that sub-processes can see it.\n> When the config is later read via git_config() or similar,\n> we parse it back out of that variable. The parsing end is a\n> little bit picky; it assumes that each entry was generated\n> with sq_quote_buf(), and that there is no extraneous\n> whitespace.\n>\n> On the generating end, we are careful to append to an\n> existing $GIT_CONFIG_PARAMETERS variable if it exists.\n> However, our test for \"should we add a space separator\" is\n> too liberal: it will add one even if the environment\n> variable exists but is empty. As a result, you might end up\n> with:\n>\n> GIT_CONFIG_PARAMETERS=\" 'core.foo=bar'\"\n>\n> which the parser will choke on.\n>\n> This was hard to trigger in older versions of git, since we\n> only set the variable when we had something to put into it\n> (though you could certainly trigger it manually). But since\n> 14111fc (git: submodule honor -c credential.* from command\n> line, 2016-02-29), the submodule code will unconditionally\n> put the $GIT_CONFIG_PARAMETERS variable into the environment\n> of any operation in the submodule, whether it is empty or\n> not. So any of those operations which themselves use \"git\n> -c\" will generate the unparseable value and fail.\n>\n> We can easily fix it by catching this case on the generating\n> side. While we're adding a test, let's also check that\n> multiple layers of \"git -c\" work, which was previously not\n> tested at all.\n>\n> Reported-by: Jonathan Nieder \n\nI should have mentioned this is\n\nReported-by: Shin Fan \n\n> Signed-off-by: Jeff King \n> ---\n> I just did this on master, and it is standalone. But for the reasons\n> above I think it would also be fine to stick on the tip of the\n> jk/submodule-c-credential topic.\n>\n> config.c | 2 +-\n> t/t1300-repo-config.sh | 14 ++++++++++++++\n> 2 files changed, 15 insertions(+), 1 deletion(-)\n\nReviewed-by: Jonathan Nieder \nTested-by: Jonathan Nieder \n\nThanks for the quick fix.\n\nSincerely,\nJonathan\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/287928/focus=289551\n"},{"id":"281513","messageId":"CA+P7+xoPe7=1ugLJ11iV-76ChiVzMA+WjGYT9KHnnSy9Z8_48g@mail.gmail.com","threadId":"41573","inReplyTo":"20160322214355.GI28749@google.com","subject":"Re: [PATCH] git_config_push_parameter: handle empty GIT_CONFIG_PARAMETERS","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-03-23T00:14:24Z","receivedAt":"2016-03-23T00:14:24Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Tue, Mar 22, 2016 at 2:43 PM, Jonathan Nieder wrote:\n> Jeff King wrote[1]:\n>\n>> Subject: git_config_push_parameter: handle empty GIT_CONFIG_PARAMETERS\n>>\n>> The \"git -c var=value\" option stuffs the config value into\n>> $GIT_CONFIG_PARAMETERS, so that sub-processes can see it.\n>> When the config is later read via git_config() or similar,\n>> we parse it back out of that variable. The parsing end is a\n>> little bit picky; it assumes that each entry was generated\n>> with sq_quote_buf(), and that there is no extraneous\n>> whitespace.\n>>\n>> On the generating end, we are careful to append to an\n>> existing $GIT_CONFIG_PARAMETERS variable if it exists.\n>> However, our test for \"should we add a space separator\" is\n>> too liberal: it will add one even if the environment\n>> variable exists but is empty. As a result, you might end up\n>> with:\n>>\n>> GIT_CONFIG_PARAMETERS=\" 'core.foo=bar'\"\n>>\n>> which the parser will choke on.\n>>\n>> This was hard to trigger in older versions of git, since we\n>> only set the variable when we had something to put into it\n>> (though you could certainly trigger it manually). But since\n>> 14111fc (git: submodule honor -c credential.* from command\n>> line, 2016-02-29), the submodule code will unconditionally\n>> put the $GIT_CONFIG_PARAMETERS variable into the environment\n>> of any operation in the submodule, whether it is empty or\n>> not. So any of those operations which themselves use \"git\n>> -c\" will generate the unparseable value and fail.\n>>\n>> We can easily fix it by catching this case on the generating\n>> side. While we're adding a test, let's also check that\n>> multiple layers of \"git -c\" work, which was previously not\n>> tested at all.\n>>\n>> Reported-by: Jonathan Nieder \n>\n> I should have mentioned this is\n>\n> Reported-by: Shin Fan \n>\n>> Signed-off-by: Jeff King \n>> ---\n>> I just did this on master, and it is standalone. But for the reasons\n>> above I think it would also be fine to stick on the tip of the\n>> jk/submodule-c-credential topic.\n>>\n>> config.c | 2 +-\n>> t/t1300-repo-config.sh | 14 ++++++++++++++\n>> 2 files changed, 15 insertions(+), 1 deletion(-)\n>\n> Reviewed-by: Jonathan Nieder \n> Tested-by: Jonathan Nieder \n>\n> Thanks for the quick fix.\n>\n> Sincerely,\n> Jonathan\n>\n> [1] http://thread.gmane.org/gmane.comp.version-control.git/287928/focus=289551\n\n\nYep, thanks Jeff. This looked fine to me.\n\nRegards,\nJake\n"},{"id":"281605","messageId":"xmqqy498c48k.fsf@gitster.mtv.corp.google.com","threadId":"41573","inReplyTo":"20160322214355.GI28749@google.com","subject":"Re: [PATCH] git_config_push_parameter: handle empty GIT_CONFIG_PARAMETERS","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-23T20:46:03Z","receivedAt":"2016-03-23T20:46:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> I should have mentioned this is\n>\n> Reported-by: Shin Fan \n>\n>> Signed-off-by: Jeff King \n>> ---\n>> I just did this on master, and it is standalone. But for the reasons\n>> above I think it would also be fine to stick on the tip of the\n>> jk/submodule-c-credential topic.\n>>\n>> config.c | 2 +-\n>> t/t1300-repo-config.sh | 14 ++++++++++++++\n>> 2 files changed, 15 insertions(+), 1 deletion(-)\n>\n> Reviewed-by: Jonathan Nieder \n> Tested-by: Jonathan Nieder \n>\n> Thanks for the quick fix.\n>\n> Sincerely,\n> Jonathan\n>\n> [1] http://thread.gmane.org/gmane.comp.version-control.git/287928/focus=289551\n\nThanks for keeping an eye on the 'next' front.\n\nIt is much more pleasant to find and fix problems before a new topic\nhits 'master', and I wish there were more people like you who use\nGit that is newer than 'master' regularly and report issues.\n\nThanks.\n"}]}