[PATCH v6 0/7] submodule--helper credential.* pass into submodule

[PATCH v6 0/7] submodule--helper credential.* pass into submodule

[Jacob Keller]

From: Jacob Keller <jacob.keller@gmail.com>

This series implements ability to pass the credential.* configuration
variables from the command line directly into the submodule. The other
patches in this series are small fixups noticed along the way.

--> Interdiff between v5 and v6 <--

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 43943eef81b4..3d37c3f1822d 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -140,18 +140,18 @@ static int submodule_config_ok(const char *var)
 
 static int sanitize_submodule_config(const char *var, const char *value, void *data)
 {
-	struct strbuf quoted = STRBUF_INIT;
 	struct strbuf *out = data;
 
 	if (submodule_config_ok(var)) {
 		if (out->len)
 			strbuf_addch(out, ' ');
 
-		sq_quotef(out, "%s=%s", var, value);
+		if (value)
+			sq_quotef(out, "%s=%s", var, value);
+		else
+			sq_quote_buf(out, var);
 	}
 
-	strbuf_release(&quoted);
-
 	return 0;
 }
 
@@ -235,16 +235,17 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 
 	const char *const git_submodule_helper_usage[] = {
 		N_("git submodule--helper clone [--prefix=<path>] [--quiet] "
-		   "[--reference <repository>] [--name <name>] [--url <url>]"
-		   "[--depth <depth>] [--path <path>]"),
+		   "[--reference <repository>] [--name <name>] [--depth <depth>] "
+		   "--url <url> --path <path>"),
 		NULL
 	};
 
 	argc = parse_options(argc, argv, prefix, module_clone_options,
 			     git_submodule_helper_usage, 0);
 
-	if (argc)
-		usage(*git_submodule_helper_usage);
+	if (argc || !url || !path)
+		usage_with_options(git_submodule_helper_usage,
+				   module_clone_options);
 
 	strbuf_addf(&sb, "%s/modules/%s", get_git_dir(), name);
 	sm_gitdir = strbuf_detach(&sb, NULL);

Jacob Keller (6):
  submodule: don't pass empty string arguments to submodule--helper
    clone
  submodule: check argc count for git submodule--helper clone
  submodule: fix submodule--helper clone usage
  submodule: fix segmentation fault in submodule--helper clone
  quote: implement sq_quotef()
  git: submodule honor -c credential.* from command line

Michael J Gruber (1):
  t/lib-httpd: load mod_unixd

 builtin/submodule--helper.c  | 76 ++++++++++++++++++++++++++++++++++++++++++--
 git-submodule.sh             | 40 ++++++++++++++---------
 quote.c                      | 13 ++++++++
 quote.h                      |  3 ++
 t/lib-httpd/apache.conf      |  3 ++
 t/t5550-http-fetch-dumb.sh   | 17 ++++++++++
 t/t7412-submodule--helper.sh | 26 +++++++++++++++
 7 files changed, 160 insertions(+), 18 deletions(-)
 create mode 100755 t/t7412-submodule--helper.sh

-- 
2.7.1.429.g45cd78e

[PATCH v6 1/7] t/lib-httpd: load mod_unixd

[Jacob Keller]

From: Michael J Gruber <git@drmicha.warpmail.net>

In contrast to apache 2.2, apache 2.4 does not load mod_unixd in its
default configuration (because there are choices). Thus, with the
current config, apache 2.4.10 will not be started and the httpd tests
will not run on distros with default apache config (RedHat type).

Enable mod_unixd to make the httpd tests run. This does not affect
distros negatively which have that config already in their default
(Debian type). httpd tests will run on these before and after this patch.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

I am sending this as an indicator that we need to queue my submodule
work on top of this, because at least locally on my Fedora 23 system I
an unable to get the httpd tests to pass otherwise.

 t/lib-httpd/apache.conf | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 7d15e6d44c83..f667e7ce2f33 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -64,6 +64,9 @@ LockFile accept.lock
 <IfModule !mod_mpm_prefork.c>
 	LoadModule mpm_prefork_module modules/mod_mpm_prefork.so
 </IfModule>
+<IfModule !mod_unixd.c>
+	LoadModule unixd_module modules/mod_unixd.so
+</IfModule>
 </IfVersion>
 
 PassEnv GIT_VALGRIND
-- 
2.7.1.429.g45cd78e

[PATCH v6 2/7] submodule: don't pass empty string arguments to submodule--helper clone

[Jacob Keller]

From: Jacob Keller <jacob.keller@gmail.com>

When --reference or --depth are unused, the current git-submodule.sh
results in empty "" arguments appended to the end of the argv array
inside git submodule--helper clone. This is not caught because the argc
count is not checked today.

Fix git-submodule.sh to only pass an argument when --reference or
--depth are used, preventing the addition of two empty string arguments
on the tail of the argv array.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
This is a split of a previous patch as suggested by Junio, to make it
easier to review the changes.

 git-submodule.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 9bc5c5f94d1d..2dd29b3df0e6 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -347,7 +347,7 @@ Use -f if you really want to add it." >&2
 				echo "$(eval_gettext "Reactivating local git directory for submodule '\$sm_name'.")"
 			fi
 		fi
-		git submodule--helper clone ${GIT_QUIET:+--quiet} --prefix "$wt_prefix" --path "$sm_path" --name "$sm_name" --url "$realrepo" "$reference" "$depth" || exit
+		git submodule--helper clone ${GIT_QUIET:+--quiet} --prefix "$wt_prefix" --path "$sm_path" --name "$sm_name" --url "$realrepo" ${reference:+"$reference"} ${depth:+"$depth"} || exit
 		(
 			clear_local_git_env
 			cd "$sm_path" &&
@@ -709,7 +709,7 @@ Maybe you want to use 'update --init'?")"
 
 		if ! test -d "$sm_path"/.git && ! test -f "$sm_path"/.git
 		then
-			git submodule--helper clone ${GIT_QUIET:+--quiet} --prefix "$prefix" --path "$sm_path" --name "$name" --url "$url" "$reference" "$depth" || exit
+			git submodule--helper clone ${GIT_QUIET:+--quiet} --prefix "$prefix" --path "$sm_path" --name "$name" --url "$url" ${reference:+"$reference"} ${depth:+"$depth"} || exit
 			cloned_modules="$cloned_modules;$name"
 			subsha1=
 		else
-- 
2.7.1.429.g45cd78e

[PATCH v6 3/7] submodule: check argc count for git submodule--helper clone

[Jacob Keller]

From: Jacob Keller <jacob.keller@gmail.com>

Extra unused arguments to git submodule--helper clone subcommand were
being silently ignored. Add a check to the argc count after options
handling to ensure that no extra arguments were left on the argv array.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 builtin/submodule--helper.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f4c3eff179b5..1e18075ed90f 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -194,6 +194,10 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, module_clone_options,
 			     git_submodule_helper_usage, 0);
 
+	if (argc)
+		usage_with_options(git_submodule_helper_usage,
+				   module_clone_options);
+
 	strbuf_addf(&sb, "%s/modules/%s", get_git_dir(), name);
 	sm_gitdir = strbuf_detach(&sb, NULL);
 
-- 
2.7.1.429.g45cd78e

[PATCH v6 4/7] submodule: fix submodule--helper clone usage

[Jacob Keller]

From: Jacob Keller <jacob.keller@gmail.com>

git submodule--helper clone usage stated that paths were added after the
[--] argument. The actual implementation required use of --path argument
and only supports one path at a time. Update the usage string to match
the current implementation.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 builtin/submodule--helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 1e18075ed90f..3c4d3ff7f4af 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -187,7 +187,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 	const char *const git_submodule_helper_usage[] = {
 		N_("git submodule--helper clone [--prefix=<path>] [--quiet] "
 		   "[--reference <repository>] [--name <name>] [--url <url>]"
-		   "[--depth <depth>] [--] [<path>...]"),
+		   "[--depth <depth>] [--path <path>]"),
 		NULL
 	};
 
-- 
2.7.1.429.g45cd78e

[PATCH v6 5/7] submodule: fix segmentation fault in submodule--helper clone

[Jacob Keller]

From: Jacob Keller <jacob.keller@gmail.com>

The git submodule--helper clone command will fail with a segmentation
fault when given a null url or null path variable. Since these are
required for proper functioning of the submodule--helper clone
subcommand, add checks to prevent running and fail gracefully when
missing.

Update the usage string to reflect the requirement that the --url and
--path "options" are required.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
This bug was discovered when trying to test the usage string of the
previous change. I bisected and this has been in the submodule--helper
code since the first iteration of module_clone. I am not really sure how
much we care, since only internal callers will be using
submodule--helper, but I suspect that it was not intended to segfault.

I fixed the usage string to remove the [] around the url and path
options, hopefully indicating they aren't "optional". Alternatively we
could start requiring them as regular arguments if we wanted.

Note the error message resulting from --url="" or --path="" are not very
obvious but they do not result in a segfault so I didn't check for those
in this patch.

 builtin/submodule--helper.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 3c4d3ff7f4af..35ae85a7e1bc 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -186,15 +186,15 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 
 	const char *const git_submodule_helper_usage[] = {
 		N_("git submodule--helper clone [--prefix=<path>] [--quiet] "
-		   "[--reference <repository>] [--name <name>] [--url <url>]"
-		   "[--depth <depth>] [--path <path>]"),
+		   "[--reference <repository>] [--name <name>] [--depth <depth>] "
+		   "--url <url> --path <path>"),
 		NULL
 	};
 
 	argc = parse_options(argc, argv, prefix, module_clone_options,
 			     git_submodule_helper_usage, 0);
 
-	if (argc)
+	if (argc || !url || !path)
 		usage_with_options(git_submodule_helper_usage,
 				   module_clone_options);
 
-- 
2.7.1.429.g45cd78e

[PATCH v6 6/7] quote: implement sq_quotef()

[Jacob Keller]

From: Jacob Keller <jacob.keller@gmail.com>

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 quote.c | 13 +++++++++++++
 quote.h |  3 +++
 2 files changed, 16 insertions(+)

diff --git a/quote.c b/quote.c
index fe884d24521f..b281a8fe454e 100644
--- a/quote.c
+++ b/quote.c
@@ -43,6 +43,19 @@ void sq_quote_buf(struct strbuf *dst, const char *src)
 	free(to_free);
 }
 
+void sq_quotef(struct strbuf *dst, const char *fmt, ...)
+{
+	struct strbuf src = STRBUF_INIT;
+
+	va_list ap;
+	va_start(ap, fmt);
+	strbuf_vaddf(&src, fmt, ap);
+	va_end(ap);
+
+	sq_quote_buf(dst, src.buf);
+	strbuf_release(&src);
+}
+
 void sq_quote_argv(struct strbuf *dst, const char** argv, size_t maxlen)
 {
 	int i;
diff --git a/quote.h b/quote.h
index 99e04d34bf22..6c53a2cc66c4 100644
--- a/quote.h
+++ b/quote.h
@@ -25,10 +25,13 @@ struct strbuf;
  * sq_quote_buf() writes to an existing buffer of specified size; it
  * will return the number of characters that would have been written
  * excluding the final null regardless of the buffer size.
+ *
+ * sq_quotef() quotes the entire formatted string as a single result.
  */
 
 extern void sq_quote_buf(struct strbuf *, const char *src);
 extern void sq_quote_argv(struct strbuf *, const char **argv, size_t maxlen);
+extern void sq_quotef(struct strbuf *, const char *fmt, ...);
 
 /* This unwraps what sq_quote() produces in place, but returns
  * NULL if the input does not look like what sq_quote would have
-- 
2.7.1.429.g45cd78e

[PATCH v6 7/7] git: submodule honor -c credential.* from command line

[Jacob Keller]

From: Jacob Keller <jacob.keller@gmail.com>

Due to the way that the git-submodule code works, it clears all local
git environment variables before entering submodules. This is normally
a good thing since we want to clear settings such as GIT_WORKTREE and
other variables which would affect the operation of submodule commands.
However, GIT_CONFIG_PARAMETERS is special, and we actually do want to
preserve these settings. However, we do not want to preserve all
configuration as many things should be left specific to the parent
project.

Add a git submodule--helper function, sanitize-config, which shall be
used to sanitize GIT_CONFIG_PARAMETERS, removing all key/value pairs
except a small subset that are known to be safe and necessary.

Replace all the calls to clear_local_git_env with a wrapped function
that filters GIT_CONFIG_PARAMETERS using the new helper and then
restores it to the filtered subset after clearing the rest of the
environment.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---

Notes:
    - v2
    * Clarify which paramaters are left after the sanitization, and don't seem to
      indicate it is our goal to extend the list.
    * add a comment in the submodule_config_ok function indicating the same
    
    - v3
    * Remove extraneous comments
    * add strbuf_release calls
    * rename sanitize_local_git_env
    * remove use of local
    * add C equivalent of sanitize_config
    * add a test for the credential passing
    
    - v4
    * use argc check instead of empty options check
    * fix brain-melting quotes in t7412-submodule--helper.sh
    
    - v5
    * use new sq_quote_f
    * fix test to use ${sq} style to prevent need of complex quotes or the use of
      double quotes on the test body
    
    - v6
    * remove unused strbuf 'quoted'
    * handle empty configuration variables

 builtin/submodule--helper.c  | 68 +++++++++++++++++++++++++++++++++++++++++++-
 git-submodule.sh             | 36 ++++++++++++++---------
 t/t5550-http-fetch-dumb.sh   | 17 +++++++++++
 t/t7412-submodule--helper.sh | 26 +++++++++++++++++
 4 files changed, 133 insertions(+), 14 deletions(-)
 create mode 100755 t/t7412-submodule--helper.sh

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 35ae85a7e1bc..3d37c3f1822d 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -124,6 +124,55 @@ static int module_name(int argc, const char **argv, const char *prefix)
 
 	return 0;
 }
+
+/*
+ * Rules to sanitize configuration variables that are Ok to be passed into
+ * submodule operations from the parent project using "-c". Should only
+ * include keys which are both (a) safe and (b) necessary for proper
+ * operation.
+ */
+static int submodule_config_ok(const char *var)
+{
+	if (starts_with(var, "credential."))
+		return 1;
+	return 0;
+}
+
+static int sanitize_submodule_config(const char *var, const char *value, void *data)
+{
+	struct strbuf *out = data;
+
+	if (submodule_config_ok(var)) {
+		if (out->len)
+			strbuf_addch(out, ' ');
+
+		if (value)
+			sq_quotef(out, "%s=%s", var, value);
+		else
+			sq_quote_buf(out, var);
+	}
+
+	return 0;
+}
+
+static void prepare_submodule_repo_env(struct argv_array *out)
+{
+	const char * const *var;
+
+	for (var = local_repo_env; *var; var++) {
+		if (!strcmp(*var, CONFIG_DATA_ENVIRONMENT)) {
+			struct strbuf sanitized_config = STRBUF_INIT;
+			git_config_from_parameters(sanitize_submodule_config,
+						   &sanitized_config);
+			argv_array_pushf(out, "%s=%s", *var, sanitized_config.buf);
+			strbuf_release(&sanitized_config);
+		} else {
+			argv_array_push(out, *var);
+		}
+	}
+
+}
+
 static int clone_submodule(const char *path, const char *gitdir, const char *url,
 			   const char *depth, const char *reference, int quiet)
 {
@@ -145,7 +194,7 @@ static int clone_submodule(const char *path, const char *gitdir, const char *url
 	argv_array_push(&cp.args, path);
 
 	cp.git_cmd = 1;
-	cp.env = local_repo_env;
+	prepare_submodule_repo_env(&cp.env_array);
 	cp.no_stdin = 1;
 
 	return run_command(&cp);
@@ -259,6 +308,22 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+static int module_sanitize_config(int argc, const char **argv, const char *prefix)
+{
+	struct strbuf sanitized_config = STRBUF_INIT;
+
+	if (argc > 1)
+		usage(_("git submodule--helper sanitize-config"));
+
+	git_config_from_parameters(sanitize_submodule_config, &sanitized_config);
+	if (sanitized_config.len)
+		printf("%s\n", sanitized_config.buf);
+
+	strbuf_release(&sanitized_config);
+
+	return 0;
+}
+
 struct cmd_struct {
 	const char *cmd;
 	int (*fn)(int, const char **, const char *);
@@ -268,6 +333,7 @@ static struct cmd_struct commands[] = {
 	{"list", module_list},
 	{"name", module_name},
 	{"clone", module_clone},
+	{"sanitize-config", module_sanitize_config},
 };
 
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
diff --git a/git-submodule.sh b/git-submodule.sh
index 2dd29b3df0e6..1f132b489b81 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -192,6 +192,16 @@ isnumber()
 	n=$(($1 + 0)) 2>/dev/null && test "$n" = "$1"
 }
 
+# Sanitize the local git environment for use within a submodule. We
+# can't simply use clear_local_git_env since we want to preserve some
+# of the settings from GIT_CONFIG_PARAMETERS.
+sanitize_submodule_env()
+{
+	sanitized_config=$(git submodule--helper sanitize-config)
+	clear_local_git_env
+	GIT_CONFIG_PARAMETERS=$sanitized_config
+}
+
 #
 # Add a new submodule to the working tree, .gitmodules and the index
 #
@@ -349,7 +359,7 @@ Use -f if you really want to add it." >&2
 		fi
 		git submodule--helper clone ${GIT_QUIET:+--quiet} --prefix "$wt_prefix" --path "$sm_path" --name "$sm_name" --url "$realrepo" ${reference:+"$reference"} ${depth:+"$depth"} || exit
 		(
-			clear_local_git_env
+			sanitize_submodule_env
 			cd "$sm_path" &&
 			# ash fails to wordsplit ${branch:+-b "$branch"...}
 			case "$branch" in
@@ -418,7 +428,7 @@ cmd_foreach()
 			name=$(git submodule--helper name "$sm_path")
 			(
 				prefix="$prefix$sm_path/"
-				clear_local_git_env
+				sanitize_submodule_env
 				cd "$sm_path" &&
 				sm_path=$(relative_path "$sm_path") &&
 				# we make $path available to scripts ...
@@ -713,7 +723,7 @@ Maybe you want to use 'update --init'?")"
 			cloned_modules="$cloned_modules;$name"
 			subsha1=
 		else
-			subsha1=$(clear_local_git_env; cd "$sm_path" &&
+			subsha1=$(sanitize_submodule_env; cd "$sm_path" &&
 				git rev-parse --verify HEAD) ||
 			die "$(eval_gettext "Unable to find current revision in submodule path '\$displaypath'")"
 		fi
@@ -723,11 +733,11 @@ Maybe you want to use 'update --init'?")"
 			if test -z "$nofetch"
 			then
 				# Fetch remote before determining tracking $sha1
-				(clear_local_git_env; cd "$sm_path" && git-fetch) ||
+				(sanitize_submodule_env; cd "$sm_path" && git-fetch) ||
 				die "$(eval_gettext "Unable to fetch in submodule path '\$sm_path'")"
 			fi
-			remote_name=$(clear_local_git_env; cd "$sm_path" && get_default_remote)
-			sha1=$(clear_local_git_env; cd "$sm_path" &&
+			remote_name=$(sanitize_submodule_env; cd "$sm_path" && get_default_remote)
+			sha1=$(sanitize_submodule_env; cd "$sm_path" &&
 				git rev-parse --verify "${remote_name}/${branch}") ||
 			die "$(eval_gettext "Unable to find current ${remote_name}/${branch} revision in submodule path '\$sm_path'")"
 		fi
@@ -745,7 +755,7 @@ Maybe you want to use 'update --init'?")"
 			then
 				# Run fetch only if $sha1 isn't present or it
 				# is not reachable from a ref.
-				(clear_local_git_env; cd "$sm_path" &&
+				(sanitize_submodule_env; cd "$sm_path" &&
 					( (rev=$(git rev-list -n 1 $sha1 --not --all 2>/dev/null) &&
 					 test -z "$rev") || git-fetch)) ||
 				die "$(eval_gettext "Unable to fetch in submodule path '\$displaypath'")"
@@ -787,7 +797,7 @@ Maybe you want to use 'update --init'?")"
 				die "$(eval_gettext "Invalid update mode '$update_module' for submodule '$name'")"
 			esac
 
-			if (clear_local_git_env; cd "$sm_path" && $command "$sha1")
+			if (sanitize_submodule_env; cd "$sm_path" && $command "$sha1")
 			then
 				say "$say_msg"
 			elif test -n "$must_die_on_failure"
@@ -803,7 +813,7 @@ Maybe you want to use 'update --init'?")"
 		then
 			(
 				prefix="$prefix$sm_path/"
-				clear_local_git_env
+				sanitize_submodule_env
 				cd "$sm_path" &&
 				eval cmd_update
 			)
@@ -841,7 +851,7 @@ Maybe you want to use 'update --init'?")"
 
 set_name_rev () {
 	revname=$( (
-		clear_local_git_env
+		sanitize_submodule_env
 		cd "$1" && {
 			git describe "$2" 2>/dev/null ||
 			git describe --tags "$2" 2>/dev/null ||
@@ -1125,7 +1135,7 @@ cmd_status()
 		else
 			if test -z "$cached"
 			then
-				sha1=$(clear_local_git_env; cd "$sm_path" && git rev-parse --verify HEAD)
+				sha1=$(sanitize_submodule_env; cd "$sm_path" && git rev-parse --verify HEAD)
 			fi
 			set_name_rev "$sm_path" "$sha1"
 			say "+$sha1 $displaypath$revname"
@@ -1135,7 +1145,7 @@ cmd_status()
 		then
 			(
 				prefix="$displaypath/"
-				clear_local_git_env
+				sanitize_submodule_env
 				cd "$sm_path" &&
 				eval cmd_status
 			) ||
@@ -1209,7 +1219,7 @@ cmd_sync()
 			if test -e "$sm_path"/.git
 			then
 			(
-				clear_local_git_env
+				sanitize_submodule_env
 				cd "$sm_path"
 				remote=$(get_default_remote)
 				git config remote."$remote".url "$sub_origin_url"
diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
index 64146352ae20..48e2ab62da7a 100755
--- a/t/t5550-http-fetch-dumb.sh
+++ b/t/t5550-http-fetch-dumb.sh
@@ -91,6 +91,23 @@ test_expect_success 'configured username does not override URL' '
 	expect_askpass pass user@host
 '
 
+test_expect_success 'cmdline credential config passes into submodules' '
+	git init super &&
+	set_askpass user@host pass@host &&
+	(
+		cd super &&
+		git submodule add "$HTTPD_URL/auth/dumb/repo.git" sub &&
+		git commit -m "add submodule"
+	) &&
+	set_askpass wrong pass@host &&
+	test_must_fail git clone --recursive super super-clone &&
+	rm -rf super-clone &&
+	set_askpass wrong pass@host &&
+	git -c "credential.$HTTP_URL.username=user@host" \
+		clone --recursive super super-clone &&
+	expect_askpass pass user@host
+'
+
 test_expect_success 'fetch changes via http' '
 	echo content >>file &&
 	git commit -a -m two &&
diff --git a/t/t7412-submodule--helper.sh b/t/t7412-submodule--helper.sh
new file mode 100755
index 000000000000..149d42864f29
--- /dev/null
+++ b/t/t7412-submodule--helper.sh
@@ -0,0 +1,26 @@
+#!/bin/sh
+#
+# Copyright (c) 2016 Jacob Keller
+#
+
+test_description='Basic plumbing support of submodule--helper
+
+This test verifies the submodule--helper plumbing command used to implement
+git-submodule.
+'
+
+. ./test-lib.sh
+
+test_expect_success 'sanitize-config clears configuration' '
+	git -c user.name="Some User" submodule--helper sanitize-config >actual &&
+	test_must_be_empty actual
+'
+
+sq="'"
+test_expect_success 'sanitize-config keeps credential.helper' '
+	git -c credential.helper=helper submodule--helper sanitize-config >actual &&
+	echo "${sq}credential.helper=helper${sq}" >expect &&
+	test_cmp expect actual
+'
+
+test_done
-- 
2.7.1.429.g45cd78e

Re: [PATCH v6 3/7] submodule: check argc count for git submodule--helper clone

[Stefan Beller]

On Mon, Feb 29, 2016 at 2:58 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:
>
> Extra unused arguments to git submodule--helper clone subcommand were
> being silently ignored. Add a check to the argc count after options
> handling to ensure that no extra arguments were left on the argv array.
>
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>

Reviewed-by: Stefan Beller <sbeller@google.com>

Re: [PATCH v6 2/7] submodule: don't pass empty string arguments to submodule--helper clone

[Stefan Beller]

On Mon, Feb 29, 2016 at 2:58 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:
> From: Jacob Keller <jacob.keller@gmail.com>
>
> When --reference or --depth are unused, the current git-submodule.sh
> results in empty "" arguments appended to the end of the argv array
> inside git submodule--helper clone. This is not caught because the argc
> count is not checked today.
>
> Fix git-submodule.sh to only pass an argument when --reference or
> --depth are used, preventing the addition of two empty string arguments
> on the tail of the argv array.
>
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>

Reviewed-by: Stefan Beller <sbeller@google.com>

Re: [PATCH v6 4/7] submodule: fix submodule--helper clone usage

[Stefan Beller]

On Mon, Feb 29, 2016 at 2:58 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:
> From: Jacob Keller <jacob.keller@gmail.com>
>
> git submodule--helper clone usage stated that paths were added after the
> [--] argument. The actual implementation required use of --path argument
> and only supports one path at a time. Update the usage string to match
> the current implementation.
>
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>

Reviewed-by: Stefan Beller <sbeller@google.com>

Re: [PATCH v6 5/7] submodule: fix segmentation fault in submodule--helper clone

[Stefan Beller]

On Mon, Feb 29, 2016 at 2:58 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:
> From: Jacob Keller <jacob.keller@gmail.com>
>
> The git submodule--helper clone command will fail with a segmentation
> fault when given a null url or null path variable. Since these are
> required for proper functioning of the submodule--helper clone
> subcommand, add checks to prevent running and fail gracefully when
> missing.
>
> Update the usage string to reflect the requirement that the --url and
> --path "options" are required.
>
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> ---
> This bug was discovered when trying to test the usage string of the
> previous change. I bisected and this has been in the submodule--helper
> code since the first iteration of module_clone. I am not really sure how
> much we care, since only internal callers will be using
> submodule--helper, but I suspect that it was not intended to segfault.
>
> I fixed the usage string to remove the [] around the url and path
> options, hopefully indicating they aren't "optional". Alternatively we
> could start requiring them as regular arguments if we wanted.
>
> Note the error message resulting from --url="" or --path="" are not very
> obvious but they do not result in a segfault so I didn't check for those
> in this patch.

I think this bug was put in, by "literally" translating from shell,
see ee8838d15776, where the shell code was rewritten to C,
specially:

 git clone $quiet ${depth:+"$depth"} -n ${reference:+"$reference"} \
     --separate-git-dir "$gitdir" "$url" "$sm_path"

Anything except url and path are done optionally, but url, and path not so.
The C code was inspired by this and aligned.

This patch makes sense to me,
Thanks,
Stefan


>
>  builtin/submodule--helper.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 3c4d3ff7f4af..35ae85a7e1bc 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -186,15 +186,15 @@ static int module_clone(int argc, const char **argv, const char *prefix)
>
>         const char *const git_submodule_helper_usage[] = {
>                 N_("git submodule--helper clone [--prefix=<path>] [--quiet] "
> -                  "[--reference <repository>] [--name <name>] [--url <url>]"
> -                  "[--depth <depth>] [--path <path>]"),
> +                  "[--reference <repository>] [--name <name>] [--depth <depth>] "
> +                  "--url <url> --path <path>"),
>                 NULL
>         };
>
>         argc = parse_options(argc, argv, prefix, module_clone_options,
>                              git_submodule_helper_usage, 0);
>
> -       if (argc)
> +       if (argc || !url || !path)
>                 usage_with_options(git_submodule_helper_usage,
>                                    module_clone_options);
>
> --
> 2.7.1.429.g45cd78e
>

Re: [PATCH v6 5/7] submodule: fix segmentation fault in submodule--helper clone

[Jacob Keller]

On Mon, Feb 29, 2016 at 3:20 PM, Stefan Beller <sbeller@google.com> wrote:
>
> I think this bug was put in, by "literally" translating from shell,
> see ee8838d15776, where the shell code was rewritten to C,
> specially:
>
>  git clone $quiet ${depth:+"$depth"} -n ${reference:+"$reference"} \
>      --separate-git-dir "$gitdir" "$url" "$sm_path"
>
> Anything except url and path are done optionally, but url, and path not so.
> The C code was inspired by this and aligned.
>
> This patch makes sense to me,
> Thanks,
> Stefan
>

The only thing that bothers me is that git submodule--helper clone
uses --path and --url instead of just taking two arguments after all
the optional parameters. Traditionally options are supposed to be just
that, optional. However given that long term we do not intend to keep
submodule--helper, I am not too worried about how it works here.

Thanks,
Jake

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

[Stefan Beller]

On Mon, Feb 29, 2016 at 2:58 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:
>
> +test_expect_success 'cmdline credential config passes into submodules' '
> +       git init super &&
> +       set_askpass user@host pass@host &&

I wonder why we use pass@host as a password, it seems confusing to me.
p@ssword would have worked if we wanted to test a password containing an @,
pass@host doesn't quite fit my mental model of how passwords work.
No need to change anything, better be consistent with the rest of the tests.


> +       (
> +               cd super &&
> +               git submodule add "$HTTPD_URL/auth/dumb/repo.git" sub &&
> +               git commit -m "add submodule"
> +       ) &&
> +       set_askpass wrong pass@host &&
> +       test_must_fail git clone --recursive super super-clone &&
> +       rm -rf super-clone &&
> +       set_askpass wrong pass@host &&

Why set set_askpass a second time here?

> +       git -c "credential.$HTTP_URL.username=user@host" \
> +               clone --recursive super super-clone &&
> +       expect_askpass pass user@host
> +'

Thanks for this test!
Stefan

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

[Jacob Keller]

On Mon, Feb 29, 2016 at 3:39 PM, Stefan Beller <sbeller@google.com> wrote:
> On Mon, Feb 29, 2016 at 2:58 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:
>>
>> +test_expect_success 'cmdline credential config passes into submodules' '
>> +       git init super &&
>> +       set_askpass user@host pass@host &&
>
> I wonder why we use pass@host as a password, it seems confusing to me.
> p@ssword would have worked if we wanted to test a password containing an @,
> pass@host doesn't quite fit my mental model of how passwords work.
> No need to change anything, better be consistent with the rest of the tests.
>

I am not sure, but I don't think it particularly matters what we use.
Most of this is pretty much copied as suggested by Peff.

>
>> +       (
>> +               cd super &&
>> +               git submodule add "$HTTPD_URL/auth/dumb/repo.git" sub &&
>> +               git commit -m "add submodule"
>> +       ) &&
>> +       set_askpass wrong pass@host &&
>> +       test_must_fail git clone --recursive super super-clone &&
>> +       rm -rf super-clone &&
>> +       set_askpass wrong pass@host &&
>
> Why set set_askpass a second time here?

Interesingly, it fails unless I add this line with:

Submodule path 'sub': checked out '678c534310f3cd5727f8e066cba0cd420bd7948e'
--- "/home/jekeller/git/git/t/trash
directory.t5550-http-fetch-dumb/askpass-expect"     2016-02-29
23:43:35.724179569 +0000
+++ "/home/jekeller/git/git/t/trash
directory.t5550-http-fetch-dumb/askpass-query"      2016-02-29
23:43:35.681179568 +0000
@@ -1 +1,3 @@
+askpass: Username for 'http://127.0.0.1:5550':
+askpass: Password for 'http://wrong@127.0.0.1:5550':
 askpass: Password for 'http://user@host@127.0.0.1:5550':
not ok 13 - cmdline credential config passes into submodules
#
#               git init super &&
#               set_askpass user@host pass@host &&
#               (
#                       cd super &&
#                       git submodule add "$HTTPD_URL/auth/dumb/repo.git" sub &&
#                       git commit -m "add submodule"
#               ) &&
#               set_askpass wrong pass@host &&
#               test_must_fail git clone --recursive super super-clone &&
#               rm -rf super-clone &&
#               git -c "credential.$HTTP_URL.username=user@host" \
#                       clone --recursive super super-clone &&
#               expect_askpass pass user@host
#

I really don't understand why adding the extra askpass setting fixes
this? Possibly because the query and expect files are appended to? Or
something else subtle going on?

Basically: without this, the expect vs the query don't match. With it,
they do. I don't understand the reasoning why.

>
> Thanks for this test!
> Stefan

Jeff King suggested it, originally.

Thanks,
Jake

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

[Jeff King]

On Mon, Feb 29, 2016 at 03:44:51PM -0800, Jacob Keller wrote:

> On Mon, Feb 29, 2016 at 3:39 PM, Stefan Beller <sbeller@google.com> wrote:
> > On Mon, Feb 29, 2016 at 2:58 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:
> >>
> >> +test_expect_success 'cmdline credential config passes into submodules' '
> >> +       git init super &&
> >> +       set_askpass user@host pass@host &&
> >
> > I wonder why we use pass@host as a password, it seems confusing to me.
> > p@ssword would have worked if we wanted to test a password containing an @,
> > pass@host doesn't quite fit my mental model of how passwords work.
> > No need to change anything, better be consistent with the rest of the tests.
> >
> 
> I am not sure, but I don't think it particularly matters what we use.
> Most of this is pretty much copied as suggested by Peff.

Yes, this dates back to 3cf8fe1 (t5550: test HTTP authentication and
userinfo decoding, 2010-11-14), and the point is just to have an "@" in
each field. And then because that's the only username defined in the
apache setup, it's the one all the tests use. :)

I also have found it off-putting, because it looks like "host" is
supposed to be meaningful. Perhaps the original author had a case where
their web authentication involved a hostname (which seems plausible for
a hosting site which covers multiple domains).

It might be nice to fix that, either by using something like "us@r" and
"p@ssword", or possibly by just introducing a new, easier to read
alternative to lib-httpd/passwd and using it, as most sites are not
testing URL parsing (though I suppose that using the @-filled ones
consistently means we _do_ test the other code paths more thoroughly).

But either way, I don't think it should be part of this series, which
has already expanded well beyond its original 1-patch goal.

> > Why set set_askpass a second time here?
> 
> Interesingly, it fails unless I add this line with:
> [...]
> I really don't understand why adding the extra askpass setting fixes
> this? Possibly because the query and expect files are appended to? Or
> something else subtle going on?

Right. The askpass test-helper logs the queries it gets, and
expect_askpass makes sure that we got the right set of queries. It has
to append to the query-log because it may be invoked multiple times
(e.g., once for the username, once for the password). So we have to
clear the query log, and that is one of the things that set_askpass does
(the other, obviously, is writing the user/pass values for askpass to
feed back to git). Perhaps it should have been called init_askpass or
something, but I don't think it's worth changing now.

You could get away with just:

  >"$TRASH_DIRECTORY/askpass-query"

but IMHO it is less obscure to just call set_askpass a second time.

-Peff

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Mon, Feb 29, 2016 at 2:58 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:
>>
>> +test_expect_success 'cmdline credential config passes into submodules' '
>> +       git init super &&
>> +       set_askpass user@host pass@host &&
>
> I wonder why we use pass@host as a password, it seems confusing to me.
> p@ssword would have worked if we wanted to test a password containing an @,
> pass@host doesn't quite fit my mental model of how passwords work.
> No need to change anything, better be consistent with the rest of the tests.
>
>
>> +       (
>> +               cd super &&
>> +               git submodule add "$HTTPD_URL/auth/dumb/repo.git" sub &&
>> +               git commit -m "add submodule"
>> +       ) &&
>> +       set_askpass wrong pass@host &&
>> +       test_must_fail git clone --recursive super super-clone &&
>> +       rm -rf super-clone &&
>> +       set_askpass wrong pass@host &&
>
> Why set set_askpass a second time here?

I find this in t/lib-httpd.sh:

        set_askpass() {
                >"$TRASH_DIRECTORY/askpass-query" &&
                echo "$1" >"$TRASH_DIRECTORY/askpass-user" &&
                echo "$2" >"$TRASH_DIRECTORY/askpass-pass"
        }

and expect_askpass peeks at askpass-query to see if Git asked the
right questions.

I think askpass-query is cleared here because the author of the test
suite expected that the helpers are used in such a way that you
always (1) set-askpass once, (2) run a Git command that asks
credentials, (3) use expect_askpass to validate and do these three
steps as a logical unit?

That "clone" the test expects to fail does ask the credential, so
even though the test does not check if the "clone" asked the right
question, it finishes the three-step logical unit, and then you need
to clear askpass-query.

It may have been cleaner if you had clear_askpass_query helper that
is called (1) at the beginning of set_askpass instead of this manual
clearing, (2) at the end of expect_askpass, as the exchange has been
tested already at that point, and (3) in place of expect_askpass if
you choose not to call it (e.g. this place, instead of the second
set_askpass, you would say clear_askpass_query), perhaps, but I do
know if that is worth it.

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

[Jacob Keller]

On Tue, Mar 1, 2016 at 9:26 AM, Junio C Hamano <gitster@pobox.com> wrote:
> I find this in t/lib-httpd.sh:
>
>         set_askpass() {
>                 >"$TRASH_DIRECTORY/askpass-query" &&
>                 echo "$1" >"$TRASH_DIRECTORY/askpass-user" &&
>                 echo "$2" >"$TRASH_DIRECTORY/askpass-pass"
>         }
>
> and expect_askpass peeks at askpass-query to see if Git asked the
> right questions.
>
> I think askpass-query is cleared here because the author of the test
> suite expected that the helpers are used in such a way that you
> always (1) set-askpass once, (2) run a Git command that asks
> credentials, (3) use expect_askpass to validate and do these three
> steps as a logical unit?
>
> That "clone" the test expects to fail does ask the credential, so
> even though the test does not check if the "clone" asked the right
> question, it finishes the three-step logical unit, and then you need
> to clear askpass-query.
>
> It may have been cleaner if you had clear_askpass_query helper that
> is called (1) at the beginning of set_askpass instead of this manual
> clearing, (2) at the end of expect_askpass, as the exchange has been
> tested already at that point, and (3) in place of expect_askpass if
> you choose not to call it (e.g. this place, instead of the second
> set_askpass, you would say clear_askpass_query), perhaps, but I do
> know if that is worth it.
>

Probably something worth looking at doing in the future.

I could call expect_askpass here at each time but I don't think it
would be meaningful after a test_must_fail.

Thanks,
Jake

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

[Junio C Hamano]

Jacob Keller <jacob.keller@gmail.com> writes:

> On Tue, Mar 1, 2016 at 9:26 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> I find this in t/lib-httpd.sh:
>>
>>         set_askpass() {
>>                 >"$TRASH_DIRECTORY/askpass-query" &&
>>                 echo "$1" >"$TRASH_DIRECTORY/askpass-user" &&
>>                 echo "$2" >"$TRASH_DIRECTORY/askpass-pass"
>>         }
>>
>> and expect_askpass peeks at askpass-query to see if Git asked the
>> right questions.
>>
>> I think askpass-query is cleared here because the author of the test
>> suite expected that the helpers are used in such a way that you
>> always (1) set-askpass once, (2) run a Git command that asks
>> credentials, (3) use expect_askpass to validate and do these three
>> steps as a logical unit?
>>
>> That "clone" the test expects to fail does ask the credential, so
>> even though the test does not check if the "clone" asked the right
>> question, it finishes the three-step logical unit, and then you need
>> to clear askpass-query.
>>
>> It may have been cleaner if you had clear_askpass_query helper that
>> is called (1) at the beginning of set_askpass instead of this manual
>> clearing, (2) at the end of expect_askpass, as the exchange has been
>> tested already at that point, and (3) in place of expect_askpass if
>> you choose not to call it (e.g. this place, instead of the second
>> set_askpass, you would say clear_askpass_query), perhaps, but I do
>> know if that is worth it.
>
> Probably something worth looking at doing in the future.
>
> I could call expect_askpass here at each time but I don't think it
> would be meaningful after a test_must_fail.

Even if you call expect_askpass to check, another set_askpass is
expected to start the next cycle anyway (unless we restructure the
helpers around clear_askpass_query I alluded to, which I am not
convinced is a good idea yet), so you'll still be asked "why another
set_askpass to set the same 'wrong pass@host'?".

So, I dunno.

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

[Jacob Keller]

On Tue, Mar 1, 2016 at 11:07 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> I could call expect_askpass here at each time but I don't think it
>> would be meaningful after a test_must_fail.
>
> Even if you call expect_askpass to check, another set_askpass is
> expected to start the next cycle anyway (unless we restructure the
> helpers around clear_askpass_query I alluded to, which I am not
> convinced is a good idea yet), so you'll still be asked "why another
> set_askpass to set the same 'wrong pass@host'?".
>
> So, I dunno.

Correct. I think it's not worth changing anything here now. The bigger
solution to change to clear_askpass_query might be the right answer
but it's a lot more involved.

Thanks,
Jake

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

[Jonathan Nieder]

Hi,

Jacob Keller wrote:

> Due to the way that the git-submodule code works, it clears all local
> git environment variables before entering submodules. This is normally
> a good thing since we want to clear settings such as GIT_WORKTREE and
> other variables which would affect the operation of submodule commands.
> However, GIT_CONFIG_PARAMETERS is special, and we actually do want to
> preserve these settings. However, we do not want to preserve all
> configuration as many things should be left specific to the parent
> project.
>
> Add a git submodule--helper function, sanitize-config, which shall be
> used to sanitize GIT_CONFIG_PARAMETERS, removing all key/value pairs
> except a small subset that are known to be safe and necessary.
>
> Replace all the calls to clear_local_git_env with a wrapped function
> that filters GIT_CONFIG_PARAMETERS using the new helper and then
> restores it to the filtered subset after clearing the rest of the
> environment.

This is failing for me when I use "git submodule add" with a remote
helper I whitelisted with GIT_ALLOW_PROTOCOL, with current "next":

 $ bin-wrappers/git submodule add persistent-https://kernel.googlesource.com/pub/scm/git/git sm
 Cloning into 'sm'...
 error: bogus format in GIT_CONFIG_PARAMETERS
 fatal: unable to parse command-line config
 fatal: clone of 'persistent-https://kernel.googlesource.com/pub/scm/git/git' into submodule path 'sm' failed

sq_dequote_to_argv doesn't like the space at the beginning of
$GIT_CONFIG_PARAMETERS.  Reverting 14111fc4 fixes it.  Known
problem?

Thanks,
Jonathan

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

[Jeff King]

On Tue, Mar 22, 2016 at 11:56:28AM -0700, Jonathan Nieder wrote:

> This is failing for me when I use "git submodule add" with a remote
> helper I whitelisted with GIT_ALLOW_PROTOCOL, with current "next":
> 
>  $ bin-wrappers/git submodule add persistent-https://kernel.googlesource.com/pub/scm/git/git sm
>  Cloning into 'sm'...
>  error: bogus format in GIT_CONFIG_PARAMETERS
>  fatal: unable to parse command-line config
>  fatal: clone of 'persistent-https://kernel.googlesource.com/pub/scm/git/git' into submodule path 'sm' failed
> 
> sq_dequote_to_argv doesn't like the space at the beginning of
> $GIT_CONFIG_PARAMETERS.  Reverting 14111fc4 fixes it.  Known
> problem?

It's known that the parsing end is excessively picky, but not this
particular bug. I found the problem; I'll have a patch out in a few
minute after I write a test.

-Peff

[PATCH] git_config_push_parameter: handle empty GIT_CONFIG_PARAMETERS

[Jeff King]

On Tue, Mar 22, 2016 at 03:23:09PM -0400, Jeff King wrote:

> On Tue, Mar 22, 2016 at 11:56:28AM -0700, Jonathan Nieder wrote:
> 
> > This is failing for me when I use "git submodule add" with a remote
> > helper I whitelisted with GIT_ALLOW_PROTOCOL, with current "next":
> > 
> >  $ bin-wrappers/git submodule add persistent-https://kernel.googlesource.com/pub/scm/git/git sm
> >  Cloning into 'sm'...
> >  error: bogus format in GIT_CONFIG_PARAMETERS
> >  fatal: unable to parse command-line config
> >  fatal: clone of 'persistent-https://kernel.googlesource.com/pub/scm/git/git' into submodule path 'sm' failed
> > 
> > sq_dequote_to_argv doesn't like the space at the beginning of
> > $GIT_CONFIG_PARAMETERS.  Reverting 14111fc4 fixes it.  Known
> > problem?
> 
> It's known that the parsing end is excessively picky, but not this
> particular bug. I found the problem; I'll have a patch out in a few
> minute after I write a test.

Here it is.

Obviously another option would be to make the parsing side more liberal
(which has the added effect that if anybody _else_ ever wants to
generate $GIT_CONFIG_PARAMETERS, they will not get annoyed). But I took
this route for now because it's the simplest way to fix the regression.
And even if we do later make the parser more liberal, it's still a good
idea to keep the output from the generating side as clean as possible.

-- >8 --
Subject: git_config_push_parameter: handle empty GIT_CONFIG_PARAMETERS

The "git -c var=value" option stuffs the config value into
$GIT_CONFIG_PARAMETERS, so that sub-processes can see it.
When the config is later read via git_config() or similar,
we parse it back out of that variable.  The parsing end is a
little bit picky; it assumes that each entry was generated
with sq_quote_buf(), and that there is no extraneous
whitespace.

On the generating end, we are careful to append to an
existing $GIT_CONFIG_PARAMETERS variable if it exists.
However, our test for "should we add a space separator" is
too liberal: it will add one even if the environment
variable exists but is empty. As a result, you might end up
with:

   GIT_CONFIG_PARAMETERS=" 'core.foo=bar'"

which the parser will choke on.

This was hard to trigger in older versions of git, since we
only set the variable when we had something to put into it
(though you could certainly trigger it manually). But since
14111fc (git: submodule honor -c credential.* from command
line, 2016-02-29), the submodule code will unconditionally
put the $GIT_CONFIG_PARAMETERS variable into the environment
of any operation in the submodule, whether it is empty or
not. So any of those operations which themselves use "git
-c" will generate the unparseable value and fail.

We can easily fix it by catching this case on the generating
side. While we're adding a test, let's also check that
multiple layers of "git -c" work, which was previously not
tested at all.

Reported-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
I just did this on master, and it is standalone. But for the reasons
above I think it would also be fine to stick on the tip of the
jk/submodule-c-credential topic.

 config.c               |  2 +-
 t/t1300-repo-config.sh | 14 ++++++++++++++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/config.c b/config.c
index 9ba40bc..8f66519 100644
--- a/config.c
+++ b/config.c
@@ -162,7 +162,7 @@ void git_config_push_parameter(const char *text)
 {
 	struct strbuf env = STRBUF_INIT;
 	const char *old = getenv(CONFIG_DATA_ENVIRONMENT);
-	if (old) {
+	if (old && *old) {
 		strbuf_addstr(&env, old);
 		strbuf_addch(&env, ' ');
 	}
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 8867ce1..2b58312 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1091,6 +1091,20 @@ test_expect_success 'git -c complains about empty key and value' '
 	test_must_fail git -c "" rev-parse
 '
 
+test_expect_success 'multiple git -c appends config' '
+	test_config alias.x "!git -c x.two=2 config --get-regexp ^x\.*" &&
+	cat >expect <<-\EOF &&
+	x.one 1
+	x.two 2
+	EOF
+	git -c x.one=1 x >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'git -c is not confused by empty environment' '
+	GIT_CONFIG_PARAMETERS="" git -c x.one=1 config --list
+'
+
 test_expect_success 'git config --edit works' '
 	git config -f tmp test.value no &&
 	echo test.value=yes >expect &&
-- 
2.8.0.rc4.388.gdee5eca

Re: [PATCH] git_config_push_parameter: handle empty GIT_CONFIG_PARAMETERS

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Obviously another option would be to make the parsing side more liberal
> (which has the added effect that if anybody _else_ ever wants to
> generate $GIT_CONFIG_PARAMETERS, they will not get annoyed). But I took
> this route for now because it's the simplest way to fix the regression.
> And even if we do later make the parser more liberal, it's still a good
> idea to keep the output from the generating side as clean as possible.

Sounds sensible.  Will queue.  Thanks.  

> I just did this on master, and it is standalone. But for the reasons
> above I think it would also be fine to stick on the tip of the
> jk/submodule-c-credential topic.

Yup, I think that is fine.

Re: [PATCH] git_config_push_parameter: handle empty GIT_CONFIG_PARAMETERS

[Jonathan Nieder]

Jeff King wrote[1]:

> Subject: git_config_push_parameter: handle empty GIT_CONFIG_PARAMETERS
>
> The "git -c var=value" option stuffs the config value into
> $GIT_CONFIG_PARAMETERS, so that sub-processes can see it.
> When the config is later read via git_config() or similar,
> we parse it back out of that variable.  The parsing end is a
> little bit picky; it assumes that each entry was generated
> with sq_quote_buf(), and that there is no extraneous
> whitespace.
>
> On the generating end, we are careful to append to an
> existing $GIT_CONFIG_PARAMETERS variable if it exists.
> However, our test for "should we add a space separator" is
> too liberal: it will add one even if the environment
> variable exists but is empty. As a result, you might end up
> with:
>
>    GIT_CONFIG_PARAMETERS=" 'core.foo=bar'"
>
> which the parser will choke on.
>
> This was hard to trigger in older versions of git, since we
> only set the variable when we had something to put into it
> (though you could certainly trigger it manually). But since
> 14111fc (git: submodule honor -c credential.* from command
> line, 2016-02-29), the submodule code will unconditionally
> put the $GIT_CONFIG_PARAMETERS variable into the environment
> of any operation in the submodule, whether it is empty or
> not. So any of those operations which themselves use "git
> -c" will generate the unparseable value and fail.
>
> We can easily fix it by catching this case on the generating
> side. While we're adding a test, let's also check that
> multiple layers of "git -c" work, which was previously not
> tested at all.
>
> Reported-by: Jonathan Nieder <jrnieder@gmail.com>

I should have mentioned this is

Reported-by: Shin Fan <shinfan@google.com>

> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I just did this on master, and it is standalone. But for the reasons
> above I think it would also be fine to stick on the tip of the
> jk/submodule-c-credential topic.
>
>  config.c               |  2 +-
>  t/t1300-repo-config.sh | 14 ++++++++++++++
>  2 files changed, 15 insertions(+), 1 deletion(-)

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Tested-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks for the quick fix.

Sincerely,
Jonathan

[1] http://thread.gmane.org/gmane.comp.version-control.git/287928/focus=289551

Re: [PATCH] git_config_push_parameter: handle empty GIT_CONFIG_PARAMETERS

[Jacob Keller]

On Tue, Mar 22, 2016 at 2:43 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Jeff King wrote[1]:
>
>> Subject: git_config_push_parameter: handle empty GIT_CONFIG_PARAMETERS
>>
>> The "git -c var=value" option stuffs the config value into
>> $GIT_CONFIG_PARAMETERS, so that sub-processes can see it.
>> When the config is later read via git_config() or similar,
>> we parse it back out of that variable.  The parsing end is a
>> little bit picky; it assumes that each entry was generated
>> with sq_quote_buf(), and that there is no extraneous
>> whitespace.
>>
>> On the generating end, we are careful to append to an
>> existing $GIT_CONFIG_PARAMETERS variable if it exists.
>> However, our test for "should we add a space separator" is
>> too liberal: it will add one even if the environment
>> variable exists but is empty. As a result, you might end up
>> with:
>>
>>    GIT_CONFIG_PARAMETERS=" 'core.foo=bar'"
>>
>> which the parser will choke on.
>>
>> This was hard to trigger in older versions of git, since we
>> only set the variable when we had something to put into it
>> (though you could certainly trigger it manually). But since
>> 14111fc (git: submodule honor -c credential.* from command
>> line, 2016-02-29), the submodule code will unconditionally
>> put the $GIT_CONFIG_PARAMETERS variable into the environment
>> of any operation in the submodule, whether it is empty or
>> not. So any of those operations which themselves use "git
>> -c" will generate the unparseable value and fail.
>>
>> We can easily fix it by catching this case on the generating
>> side. While we're adding a test, let's also check that
>> multiple layers of "git -c" work, which was previously not
>> tested at all.
>>
>> Reported-by: Jonathan Nieder <jrnieder@gmail.com>
>
> I should have mentioned this is
>
> Reported-by: Shin Fan <shinfan@google.com>
>
>> Signed-off-by: Jeff King <peff@peff.net>
>> ---
>> I just did this on master, and it is standalone. But for the reasons
>> above I think it would also be fine to stick on the tip of the
>> jk/submodule-c-credential topic.
>>
>>  config.c               |  2 +-
>>  t/t1300-repo-config.sh | 14 ++++++++++++++
>>  2 files changed, 15 insertions(+), 1 deletion(-)
>
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
> Tested-by: Jonathan Nieder <jrnieder@gmail.com>
>
> Thanks for the quick fix.
>
> Sincerely,
> Jonathan
>
> [1] http://thread.gmane.org/gmane.comp.version-control.git/287928/focus=289551


Yep, thanks Jeff. This looked fine to me.

Regards,
Jake

Re: [PATCH] git_config_push_parameter: handle empty GIT_CONFIG_PARAMETERS

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> I should have mentioned this is
>
> Reported-by: Shin Fan <shinfan@google.com>
>
>> Signed-off-by: Jeff King <peff@peff.net>
>> ---
>> I just did this on master, and it is standalone. But for the reasons
>> above I think it would also be fine to stick on the tip of the
>> jk/submodule-c-credential topic.
>>
>>  config.c               |  2 +-
>>  t/t1300-repo-config.sh | 14 ++++++++++++++
>>  2 files changed, 15 insertions(+), 1 deletion(-)
>
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
> Tested-by: Jonathan Nieder <jrnieder@gmail.com>
>
> Thanks for the quick fix.
>
> Sincerely,
> Jonathan
>
> [1] http://thread.gmane.org/gmane.comp.version-control.git/287928/focus=289551

Thanks for keeping an eye on the 'next' front.

It is much more pleasant to find and fix problems before a new topic
hits 'master', and I wish there were more people like you who use
Git that is newer than 'master' regularly and report issues.

Thanks.