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

[PATCH v2] 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

 builtin/submodule--helper.c  | 56 ++++++++++++++++++++++++++++++++++++++++++++
 git-submodule.sh             | 36 ++++++++++++++++++----------
 t/t7412-submodule--helper.sh | 25 ++++++++++++++++++++
 3 files changed, 104 insertions(+), 13 deletions(-)
 create mode 100755 t/t7412-submodule--helper.sh

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f4c3eff179b5..8b3320ecd3e0 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -255,6 +255,61 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 	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. Right now only "credential.*" fits both criteria.
+ */
+int submodule_config_ok(const char *var)
+{
+	if (starts_with(var, "credential."))
+		return 1;
+	return 0;
+}
+
+int sanitize_submodule_config(const char *var, const char *value, void *data)
+{
+	struct strbuf quoted = STRBUF_INIT;
+	struct strbuf *out = data;
+
+	if (submodule_config_ok(var)) {
+		if (out->len)
+			strbuf_addch(out, ' ');
+
+		/* combined all the values before we quote them */
+		strbuf_addstr(&quoted, var);
+		strbuf_addch(&quoted, '=');
+		strbuf_addstr(&quoted, value);
+
+		/* safely quote them for shell use */
+		sq_quote_buf(out, quoted.buf);
+	}
+	return 0;
+}
+
+static int module_sanitize_config(int argc, const char **argv, const char *prefix)
+{
+	struct strbuf sanitized_config = STRBUF_INIT;
+
+	struct option module_sanitize_config_options[] = {
+		OPT_END()
+	};
+
+	const char *const git_submodule_helper_usage[] = {
+		N_("git submodule--helper sanitize-config"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, prefix, module_sanitize_config_options,
+			     git_submodule_helper_usage, 0);
+
+	git_config_from_parameters(sanitize_submodule_config, &sanitized_config);
+	if (sanitized_config.len)
+		printf("%s\n", sanitized_config.buf);
+
+	return 0;
+}
+
 struct cmd_struct {
 	const char *cmd;
 	int (*fn)(int, const char **, const char *);
@@ -264,6 +319,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 9bc5c5f94d1d..dd469ecb2065 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_local_git_env()
+{
+	local 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" "$depth" || exit
 		(
-			clear_local_git_env
+			sanitize_local_git_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_local_git_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_local_git_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_local_git_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_local_git_env; cd "$sm_path" && get_default_remote)
+			sha1=$(sanitize_local_git_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_local_git_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_local_git_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_local_git_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_local_git_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_local_git_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_local_git_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_local_git_env
 				cd "$sm_path"
 				remote=$(get_default_remote)
 				git config remote."$remote".url "$sub_origin_url"
diff --git a/t/t7412-submodule--helper.sh b/t/t7412-submodule--helper.sh
new file mode 100755
index 000000000000..376f58afe967
--- /dev/null
+++ b/t/t7412-submodule--helper.sh
@@ -0,0 +1,25 @@
+#!/bin/sh
+#
+# Copyright (c) 2016 Jacob Keller
+#
+
+test_description='Basic plumbing support of submodule--helper
+
+This test tries to verify the submodule--helper plumbing command used
+to implement git-submodule.
+'
+
+. ./test-lib.sh
+
+test_expect_success 'sanitize-config clears configuration' '
+	git -c user.name="Some User" submodule--helper sanitize-config >actual &&
+	test_must_be_empty actual
+'
+
+test_expect_success 'sanitize-config keeps credential.helper' '
+	git -c credential.helper="helper" submodule--helper sanitize-config >actual &&
+	echo "'\''credential.helper=helper'\''" >expect &&
+	test_cmp expect actual
+'
+
+test_done
-- 
2.7.1.429.g45cd78e

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

[Eric Sunshine]

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

A few superficial comments below...

> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> ---
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> @@ -255,6 +255,61 @@ static int module_clone(int argc, const char **argv, const char *prefix)
> +/* Rules to sanitize configuration variables that are Ok to be passed into
> + * submodule operations from the parent project using "-c". Should only
> + * include keys which are both (a) safe and (b) necessary for proper
> + * operation. Right now only "credential.*" fits both criteria.
> + */

Drop the final sentence for a couple reasons:

1. It's merely repeating what the code itself already says, and...
2. It's likely to become outdated when additional variables are added.

Also, style:

    /*
     * Multi-line comment
     * style.
     */

> +int submodule_config_ok(const char *var)
> +{
> +       if (starts_with(var, "credential."))
> +               return 1;
> +       return 0;
> +}
> +
> +int sanitize_submodule_config(const char *var, const char *value, void *data)
> +{
> +       struct strbuf quoted = STRBUF_INIT;
> +       struct strbuf *out = data;
> +
> +       if (submodule_config_ok(var)) {
> +               if (out->len)
> +                       strbuf_addch(out, ' ');
> +
> +               /* combined all the values before we quote them */

Comment repeats what the code already says, thus not terribly useful.

Also: s/combined/combine/

> +               strbuf_addstr(&quoted, var);
> +               strbuf_addch(&quoted, '=');
> +               strbuf_addstr(&quoted, value);

strbuf_addf(&quoted, "%s=%s", var, value);

> +               /* safely quote them for shell use */

Comment repeats what the code already says.

> +               sq_quote_buf(out, quoted.buf);
> +       }
> +       return 0;
> +}
> +
> +static int module_sanitize_config(int argc, const char **argv, const char *prefix)
> +{
> +       struct strbuf sanitized_config = STRBUF_INIT;
> +
> +       struct option module_sanitize_config_options[] = {
> +               OPT_END()
> +       };
> +
> +       const char *const git_submodule_helper_usage[] = {
> +               N_("git submodule--helper sanitize-config"),
> +               NULL
> +       };
> +
> +       argc = parse_options(argc, argv, prefix, module_sanitize_config_options,
> +                            git_submodule_helper_usage, 0);
> +
> +       git_config_from_parameters(sanitize_submodule_config, &sanitized_config);
> +       if (sanitized_config.len)
> +               printf("%s\n", sanitized_config.buf);

Perhaps not a big deal since the program exits immediately after, but you could:

    strbuf_release(&sanitized_config);

> +       return 0;
> +}
> +
> diff --git a/git-submodule.sh b/git-submodule.sh
> @@ -192,6 +192,16 @@ isnumber()
> +# Sanitize the local git environment for use within a submodule. We
> +# can't simply use clear_local_git_env since we want to preserve some
> +# of the settings from GIT_CONFIG_PARAMETERS.
> +sanitize_local_git_env()
> +{
> +       local sanitized_config = $(git submodule--helper sanitize-config)

Is 'local' a bashism? (Although, I see that 'local' is already being
used in relative_path(); perhaps that ought to be cleaned up.)

> +       clear_local_git_env
> +       GIT_CONFIG_PARAMETERS=$sanitized_config
> +}
> diff --git a/t/t7412-submodule--helper.sh b/t/t7412-submodule--helper.sh
> @@ -0,0 +1,25 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2016 Jacob Keller
> +#
> +
> +test_description='Basic plumbing support of submodule--helper
> +
> +This test tries to verify the submodule--helper plumbing command used

Maybe: s/tries to verify/verifies/

> +to implement git-submodule.
> +'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'sanitize-config clears configuration' '
> +       git -c user.name="Some User" submodule--helper sanitize-config >actual &&
> +       test_must_be_empty actual
> +'
> +
> +test_expect_success 'sanitize-config keeps credential.helper' '
> +       git -c credential.helper="helper" submodule--helper sanitize-config >actual &&
> +       echo "'\''credential.helper=helper'\''" >expect &&
> +       test_cmp expect actual
> +'
> +
> +test_done

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

[Jeff King]

On Wed, Feb 24, 2016 at 07:27:23PM -0500, Eric Sunshine wrote:

> > diff --git a/git-submodule.sh b/git-submodule.sh
> > @@ -192,6 +192,16 @@ isnumber()
> > +# Sanitize the local git environment for use within a submodule. We
> > +# can't simply use clear_local_git_env since we want to preserve some
> > +# of the settings from GIT_CONFIG_PARAMETERS.
> > +sanitize_local_git_env()
> > +{
> > +       local sanitized_config = $(git submodule--helper sanitize-config)
> 
> Is 'local' a bashism? (Although, I see that 'local' is already being
> used in relative_path(); perhaps that ought to be cleaned up.)

It seems to have spread to Almquist shells like dash, but it's
definitely not in POSIX. That covers _most_ platforms these days, but
I'd guess would break on older ksh. We should probably avoid it.

For the curious, there's a very thorough discussion in the first answer
here:

  http://stackoverflow.com/questions/18597697/posix-compliant-way-to-scope-variables-to-a-function-in-a-shell-script

In this case, we know that we're doing the sanitizing in a subshell, so
I think just dropping "local" and clobbering the existing
$sanitized_config is fine.

-Peff

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

[Jacob Keller]

On Wed, Feb 24, 2016 at 4:27 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> ---
>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
>> @@ -255,6 +255,61 @@ static int module_clone(int argc, const char **argv, const char *prefix)
>> +/* Rules to sanitize configuration variables that are Ok to be passed into
>> + * submodule operations from the parent project using "-c". Should only
>> + * include keys which are both (a) safe and (b) necessary for proper
>> + * operation. Right now only "credential.*" fits both criteria.
>> + */
>
> Drop the final sentence for a couple reasons:
>
> 1. It's merely repeating what the code itself already says, and...
> 2. It's likely to become outdated when additional variables are added.
>

Yep. I'll drop that, makes sense.

> Also, style:
>
>     /*
>      * Multi-line comment
>      * style.
>      */
>

Hah. I contribute to the netdev kernel a lot, and for some reason they
prefer the style above (but only in net/ code). I get into a habbit of
doing that too much I'll fix this.

>> +               /* combined all the values before we quote them */
>
> Comment repeats what the code already says, thus not terribly useful.
>
> Also: s/combined/combine/
>

I tend to make comments like this when I change the obvious way it was
done, I commented this because I based it on a scratch patch from Jeff
that didn't have them put together before quoting.

>> +               strbuf_addstr(&quoted, var);
>> +               strbuf_addch(&quoted, '=');
>> +               strbuf_addstr(&quoted, value);
>
> strbuf_addf(&quoted, "%s=%s", var, value);
>
>> +               /* safely quote them for shell use */
>
> Comment repeats what the code already says.
>

Will drop this as well.


>
> Perhaps not a big deal since the program exits immediately after, but you could:
>
>     strbuf_release(&sanitized_config);
>

Yep, just an oversight, same for the quoted string as well.

>> +       local sanitized_config = $(git submodule--helper sanitize-config)
>
> Is 'local' a bashism? (Although, I see that 'local' is already being
> used in relative_path(); perhaps that ought to be cleaned up.)
>

Apparently it is, I wasn't aware of that. It's supported on more than
bash, but it's not POSIX.

>> +
>> +test_description='Basic plumbing support of submodule--helper
>> +
>> +This test tries to verify the submodule--helper plumbing command used
>
> Maybe: s/tries to verify/verifies/
>

Yes. I used "tries" with the intention of needing tests for the actual
functionality as submodule--helper is expected to go away.

Thanks for the review, I'll have these cleaned up in v3

Regards,
Jake

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

[Jeff King]

On Wed, Feb 24, 2016 at 10:19:05PM -0800, Jacob Keller wrote:

> >> +               /* combined all the values before we quote them */
> >
> > Comment repeats what the code already says, thus not terribly useful.
> >
> > Also: s/combined/combine/
> >
> I tend to make comments like this when I change the obvious way it was
> done, I commented this because I based it on a scratch patch from Jeff
> that didn't have them put together before quoting.

That's a good point. The _what_ is not interesting here, but the _why_
might be. Namely that we must quote the whole thing as a unit, or the
parser on the receiving end will not be able to read it.

I'd also be amenable to relaxing the parser (which is as strict as it is
only out of laziness, and the fact that it was reading the output only
of its nearby generator function). But I can understand if you don't
feel like digging into that.

-Peff

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

[Jacob Keller]

On Wed, Feb 24, 2016 at 10:23 PM, Jeff King <peff@peff.net> wrote:
> On Wed, Feb 24, 2016 at 10:19:05PM -0800, Jacob Keller wrote:
>
>> >> +               /* combined all the values before we quote them */
>> >
>> > Comment repeats what the code already says, thus not terribly useful.
>> >
>> > Also: s/combined/combine/
>> >
>> I tend to make comments like this when I change the obvious way it was
>> done, I commented this because I based it on a scratch patch from Jeff
>> that didn't have them put together before quoting.
>
> That's a good point. The _what_ is not interesting here, but the _why_
> might be. Namely that we must quote the whole thing as a unit, or the
> parser on the receiving end will not be able to read it.
>

I can reword the comment to explain the _why_

> I'd also be amenable to relaxing the parser (which is as strict as it is
> only out of laziness, and the fact that it was reading the output only
> of its nearby generator function). But I can understand if you don't
> feel like digging into that.
>
> -Peff

I really don't want to dig that far into it. I think that working
within the current parser is just fine.

Regards,
Jake

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

[Jeff King]

On Wed, Feb 24, 2016 at 03:59:12PM -0800, Jacob Keller wrote:

> +int sanitize_submodule_config(const char *var, const char *value, void *data)
> +{
> +	struct strbuf quoted = STRBUF_INIT;
> +	struct strbuf *out = data;
> +
> +	if (submodule_config_ok(var)) {
> +		if (out->len)
> +			strbuf_addch(out, ' ');
> +
> +		/* combined all the values before we quote them */
> +		strbuf_addstr(&quoted, var);
> +		strbuf_addch(&quoted, '=');
> +		strbuf_addstr(&quoted, value);
> +
> +		/* safely quote them for shell use */
> +		sq_quote_buf(out, quoted.buf);
> +	}
> +	return 0;
> +}

This leaks "quoted", doesn't it?

I was confused by the "combine all the values" comment. We just have
_one_ config key/value here, right (I had thought originally that you
were putting multiple keys into a single sq-quoted string, which would be
wrong)?

I agree with Eric, though, that you could just drop the comment
entirely.

> +static int module_sanitize_config(int argc, const char **argv, const char *prefix)
> +{
> +	struct strbuf sanitized_config = STRBUF_INIT;
> +
> +	struct option module_sanitize_config_options[] = {
> +		OPT_END()
> +	};
> +
> +	const char *const git_submodule_helper_usage[] = {
> +		N_("git submodule--helper sanitize-config"),
> +		NULL
> +	};
> +
> +	argc = parse_options(argc, argv, prefix, module_sanitize_config_options,
> +			     git_submodule_helper_usage, 0);
> +
> +	git_config_from_parameters(sanitize_submodule_config, &sanitized_config);
> +	if (sanitized_config.len)
> +		printf("%s\n", sanitized_config.buf);
> +
> +	return 0;
> +}

The empty option list looked funny to me for a minute, but I guess you
use it to complain about:

  git submodule--helper sanitize-config --foo

Should we also warn about:

  git submodule--helper sanitize-config foo

I think you could catch both with just:

  if (argc > 1)
	usage(...);

(though I do not mind the empty option list staying in that case, as it
provides the necessary boilerplate for later).

> +# Sanitize the local git environment for use within a submodule. We
> +# can't simply use clear_local_git_env since we want to preserve some
> +# of the settings from GIT_CONFIG_PARAMETERS.
> +sanitize_local_git_env()
> +{
> +	local sanitized_config = $(git submodule--helper sanitize-config)
> +	clear_local_git_env
> +	GIT_CONFIG_PARAMETERS=$sanitized_config
> +}

Do we need to export GIT_CONFIG_PARAMETERS? I guess not; if it is
already exported, we don't need, and if it isn't, then by definition
$sanitized_config will be empty.

The name of this function isn't very descriptive (it's easy to see what
it does from the implementation, but in the callers, it's unclear what
the difference between "clear" and "sanitize" is). Should it maybe be
"sanitize_submodule_env" or something to make it clear that this is
about passing through things for child submodules?

Probably not that big a deal as its local to this script

> diff --git a/t/t7412-submodule--helper.sh b/t/t7412-submodule--helper.sh
> new file mode 100755
> index 000000000000..376f58afe967
> --- /dev/null
> +++ b/t/t7412-submodule--helper.sh

In the long run I think we want to kill off submodule--helper, as it's
just an implementation detail until git-submodule is all in C. I do not
mind these tests in the meantime, as they can act as unit tests. But it
would be nice to also (or instead, if you like) test the actual
user-visible effects. Otherwise, once git-submodule turns into C, these
behaviors are likely to end up completely untested (and it's during that
conversion that you you're most likely to run into regressions!).

-Peff

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

[Jacob Keller]

On Wed, Feb 24, 2016 at 5:41 PM, Jeff King <peff@peff.net> wrote:
> On Wed, Feb 24, 2016 at 03:59:12PM -0800, Jacob Keller wrote:
>
>> +int sanitize_submodule_config(const char *var, const char *value, void *data)
>> +{
>> +     struct strbuf quoted = STRBUF_INIT;
>> +     struct strbuf *out = data;
>> +
>> +     if (submodule_config_ok(var)) {
>> +             if (out->len)
>> +                     strbuf_addch(out, ' ');
>> +
>> +             /* combined all the values before we quote them */
>> +             strbuf_addstr(&quoted, var);
>> +             strbuf_addch(&quoted, '=');
>> +             strbuf_addstr(&quoted, value);
>> +
>> +             /* safely quote them for shell use */
>> +             sq_quote_buf(out, quoted.buf);
>> +     }
>> +     return 0;
>> +}
>
> This leaks "quoted", doesn't it?
>

Yes this was an oversight. Will fix it.

> I was confused by the "combine all the values" comment. We just have
> _one_ config key/value here, right (I had thought originally that you
> were putting multiple keys into a single sq-quoted string, which would be
> wrong)?
>

Hah, that would be confusing. The comment will be dropped in v3.

>> +static int module_sanitize_config(int argc, const char **argv, const char *prefix)
>> +{
>> +     struct strbuf sanitized_config = STRBUF_INIT;
>> +
>> +     struct option module_sanitize_config_options[] = {
>> +             OPT_END()
>> +     };
>> +
>> +     const char *const git_submodule_helper_usage[] = {
>> +             N_("git submodule--helper sanitize-config"),
>> +             NULL
>> +     };
>> +
>> +     argc = parse_options(argc, argv, prefix, module_sanitize_config_options,
>> +                          git_submodule_helper_usage, 0);
>> +
>> +     git_config_from_parameters(sanitize_submodule_config, &sanitized_config);
>> +     if (sanitized_config.len)
>> +             printf("%s\n", sanitized_config.buf);
>> +
>> +     return 0;
>> +}
>
> The empty option list looked funny to me for a minute, but I guess you
> use it to complain about:
>
>   git submodule--helper sanitize-config --foo
>
> Should we also warn about:
>
>   git submodule--helper sanitize-config foo
>
> I think you could catch both with just:
>
>   if (argc > 1)
>         usage(...);
>
> (though I do not mind the empty option list staying in that case, as it
> provides the necessary boilerplate for later).
>

I don't think there will be a later, but I didn't think to check argc,
since a few other submodule--helpers fail to check it. I will clean
this out, and possibly provide a second patch which cleans up the
other case(s?) of missed argc checks as well. I think it was only the
submodule--helper list subcommand, but I don't recall right now.

>> +# Sanitize the local git environment for use within a submodule. We
>> +# can't simply use clear_local_git_env since we want to preserve some
>> +# of the settings from GIT_CONFIG_PARAMETERS.
>> +sanitize_local_git_env()
>> +{
>> +     local sanitized_config = $(git submodule--helper sanitize-config)
>> +     clear_local_git_env
>> +     GIT_CONFIG_PARAMETERS=$sanitized_config
>> +}
>
> Do we need to export GIT_CONFIG_PARAMETERS? I guess not; if it is
> already exported, we don't need, and if it isn't, then by definition
> $sanitized_config will be empty.
>

How does modifying an exported variable work?

> The name of this function isn't very descriptive (it's easy to see what
> it does from the implementation, but in the callers, it's unclear what
> the difference between "clear" and "sanitize" is). Should it maybe be
> "sanitize_submodule_env" or something to make it clear that this is
> about passing through things for child submodules?
>
> Probably not that big a deal as its local to this script

Wouldn't hurt, I was trying to come up with a good name as well, but I
like sanitize_submodule_env better.

>
>> diff --git a/t/t7412-submodule--helper.sh b/t/t7412-submodule--helper.sh
>> new file mode 100755
>> index 000000000000..376f58afe967
>> --- /dev/null
>> +++ b/t/t7412-submodule--helper.sh
>
> In the long run I think we want to kill off submodule--helper, as it's
> just an implementation detail until git-submodule is all in C. I do not
> mind these tests in the meantime, as they can act as unit tests. But it
> would be nice to also (or instead, if you like) test the actual
> user-visible effects. Otherwise, once git-submodule turns into C, these
> behaviors are likely to end up completely untested (and it's during that
> conversion that you you're most likely to run into regressions!).
>
> -Peff

I 100% agree. I think the test file is useful for now, and there are
(currently) no other tests for submodule--helper, so I'd like to get
them all confined to this test. I think we need a real way to test the
change here, but I think figuring out how to test the
credential.helper is a bit outside the scope of what i had time for
today. I can try to find some cycles to check out tomorrow. You
mentioned we'd need a test in the same idea as one of the http clone
tests? I don't know where to start with something like this though.

Regards,
Jake

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

[Jeff King]

On Wed, Feb 24, 2016 at 10:23:28PM -0800, Jacob Keller wrote:

> >> +# Sanitize the local git environment for use within a submodule. We
> >> +# can't simply use clear_local_git_env since we want to preserve some
> >> +# of the settings from GIT_CONFIG_PARAMETERS.
> >> +sanitize_local_git_env()
> >> +{
> >> +     local sanitized_config = $(git submodule--helper sanitize-config)
> >> +     clear_local_git_env
> >> +     GIT_CONFIG_PARAMETERS=$sanitized_config
> >> +}
> >
> > Do we need to export GIT_CONFIG_PARAMETERS? I guess not; if it is
> > already exported, we don't need, and if it isn't, then by definition
> > $sanitized_config will be empty.
> >
> How does modifying an exported variable work?

Generally, variables which came to the shell from the environment are
marked for export, and modifying a marked-for-export variable will not
change its export flag.

I have a nagging feeling that there was some shell deep in the past
where that was not the case, but I can't find any mention of it. So
either I dreamed it, or it is so old and broken that even the autoconf
portability page does not bother with it. ;)

> I 100% agree. I think the test file is useful for now, and there are
> (currently) no other tests for submodule--helper, so I'd like to get
> them all confined to this test. I think we need a real way to test the
> change here, but I think figuring out how to test the
> credential.helper is a bit outside the scope of what i had time for
> today. I can try to find some cycles to check out tomorrow. You
> mentioned we'd need a test in the same idea as one of the http clone
> tests? I don't know where to start with something like this though.

I think something like this would work:

diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
index 6414635..c5ce8ff 100755
--- a/t/t5550-http-fetch-dumb.sh
+++ b/t/t5550-http-fetch-dumb.sh
@@ -91,6 +91,20 @@ test_expect_success 'configured username does not override URL' '
 	expect_askpass pass user@host
 '
 
+test_expect_success 'cmdline credential config passes to 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 &&
+	git -c "credential.$HTTPD_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 &&

but it does not seem to pass with your patch (even after I fixed up the
weird "local" thing). I think the problem is that we ask
submodule--helper to do the clone, and it uses local_repo_env. So in
addition to your patch, you probably need a C version of the same thing
which outputs to an argv_array.

-Peff

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

[Jeff King]

On Thu, Feb 25, 2016 at 02:00:36AM -0500, Jeff King wrote:

> I think something like this would work:
> [...]
> but it does not seem to pass with your patch (even after I fixed up the
> weird "local" thing). I think the problem is that we ask
> submodule--helper to do the clone, and it uses local_repo_env. So in
> addition to your patch, you probably need a C version of the same thing
> which outputs to an argv_array.

Something like this (which passes my test, but I didn't think hard about
it beyond that):

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 8b3320e..fa941fd 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -124,6 +124,27 @@ static int module_name(int argc, const char **argv, const char *prefix)
 
 	return 0;
 }
+
+/* this (and submodule_config_ok) should be static in the original */
+int sanitize_submodule_config(const char *, const char *, void *);
+
+static void add_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 +166,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;
+	add_submodule_repo_env(&cp.env_array);
 	cp.no_stdin = 1;
 
 	return run_command(&cp);

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

[Jacob Keller]

On Wed, Feb 24, 2016 at 11:11 PM, Jeff King <peff@peff.net> wrote:
> On Thu, Feb 25, 2016 at 02:00:36AM -0500, Jeff King wrote:
>
>> I think something like this would work:
>> [...]
>> but it does not seem to pass with your patch (even after I fixed up the
>> weird "local" thing). I think the problem is that we ask
>> submodule--helper to do the clone, and it uses local_repo_env. So in
>> addition to your patch, you probably need a C version of the same thing
>> which outputs to an argv_array.
>
> Something like this (which passes my test, but I didn't think hard about
> it beyond that):

I am having trouble getting the httpd tests to work.. The error.log
generated contains the following:

[Thu Feb 25 18:01:58.583832 2016] [core:crit] [pid 16376] AH00136:
Server MUST relinquish startup privileges before accepting
connections.  Please ensure mod_unixd or other system security module
is loaded.
AH00016: Configuration Failed

I have httpd 2.4, so I'm not sure exactly what the deal is here for
these tests..

Any suggestions on what causes this?

Thanks,
Jake

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

[Jacob Keller]

On Wed, Feb 24, 2016 at 11:11 PM, Jeff King <peff@peff.net> wrote:
>  static int clone_submodule(const char *path, const char *gitdir, const char *url,
>                            const char *depth, const char *reference, int quiet)
>  {
> @@ -145,7 +166,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;
> +       add_submodule_repo_env(&cp.env_array);
>         cp.no_stdin = 1;
>
>         return run_command(&cp);


Ignore my previous comment. The cp.env API is *very* subtle. If the
line is just a name, it removes the environment variable, while
"name=value" adds it. That is definitely not what I was expecting
here, so I misread how it works.

I am sending a v3 with an extended test similar to the one that Jeff suggested.

Thanks,
Jake

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

[Junio C Hamano]

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

> On Wed, Feb 24, 2016 at 11:11 PM, Jeff King <peff@peff.net> wrote:
>>  static int clone_submodule(const char *path, const char *gitdir, const char *url,
>>                            const char *depth, const char *reference, int quiet)
>>  {
>> @@ -145,7 +166,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;
>> +       add_submodule_repo_env(&cp.env_array);
>>         cp.no_stdin = 1;
>>
>>         return run_command(&cp);
>
>
> Ignore my previous comment. The cp.env API is *very* subtle. If the
> line is just a name, it removes the environment variable, while
> "name=value" adds it. That is definitely not what I was expecting
> here, so I misread how it works.

I think that is modelled after how putenv(3) is made capable of
removing an existing environment variable.

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

[Jacob Keller]

On Thu, Feb 25, 2016 at 12:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jacob Keller <jacob.keller@gmail.com> writes:
>
>> On Wed, Feb 24, 2016 at 11:11 PM, Jeff King <peff@peff.net> wrote:
>>>  static int clone_submodule(const char *path, const char *gitdir, const char *url,
>>>                            const char *depth, const char *reference, int quiet)
>>>  {
>>> @@ -145,7 +166,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;
>>> +       add_submodule_repo_env(&cp.env_array);
>>>         cp.no_stdin = 1;
>>>
>>>         return run_command(&cp);
>>
>>
>> Ignore my previous comment. The cp.env API is *very* subtle. If the
>> line is just a name, it removes the environment variable, while
>> "name=value" adds it. That is definitely not what I was expecting
>> here, so I misread how it works.
>
> I think that is modelled after how putenv(3) is made capable of
> removing an existing environment variable.

Yes. It makes perfect sense once you read it, i was just very confused
at the initial glance because it felt like it was copying the entire
local_repo_env into the submodule child process. I just read the
technical API documentation and understand it now.

Thanks,
Jake