From d10e3b4260628fbe07117e2b57003291500e4f47 Mon Sep 17 00:00:00 2001 From: Jacob Keller Date: Mon, 29 Feb 2016 14:58:30 -0800 Subject: submodule: don't pass empty string arguments to submodule--helper clone 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 Reviewed-by: Stefan Beller Signed-off-by: Junio C Hamano --- git-submodule.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/git-submodule.sh b/git-submodule.sh index 9bc5c5f94d..2dd29b3df0 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 -- cgit v1.2.1 From 08e0970a862aaa3b42c532ad0086bcaabe5108cf Mon Sep 17 00:00:00 2001 From: Jacob Keller Date: Mon, 29 Feb 2016 14:58:31 -0800 Subject: submodule: check argc count for git submodule--helper clone 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 Reviewed-by: Stefan Beller Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index f4c3eff179..1e18075ed9 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); -- cgit v1.2.1 From 717416ca87edcd3889e40d8191fd4b6721e8a02e Mon Sep 17 00:00:00 2001 From: Jacob Keller Date: Mon, 29 Feb 2016 14:58:32 -0800 Subject: submodule: fix submodule--helper clone usage 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 Reviewed-by: Stefan Beller Signed-off-by: Junio C Hamano --- 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 1e18075ed9..3c4d3ff7f4 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=] [--quiet] " "[--reference ] [--name ] [--url ]" - "[--depth ] [--] [...]"), + "[--depth ] [--path ]"), NULL }; -- cgit v1.2.1 From 7dad2633348423191844eef49022a9013242d6ef Mon Sep 17 00:00:00 2001 From: Jacob Keller Date: Mon, 29 Feb 2016 14:58:33 -0800 Subject: submodule: fix segmentation fault in submodule--helper clone 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 Reviewed-by: Stefan Beller Signed-off-by: Junio C Hamano --- 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 3c4d3ff7f4..35ae85a7e1 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=] [--quiet] " - "[--reference ] [--name ] [--url ]" - "[--depth ] [--path ]"), + "[--reference ] [--name ] [--depth ] " + "--url --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); -- cgit v1.2.1 From e70986d72515b7601aea7d8d86a9d3bed53866f3 Mon Sep 17 00:00:00 2001 From: Jacob Keller Date: Mon, 29 Feb 2016 14:58:34 -0800 Subject: quote: implement sq_quotef() Signed-off-by: Jacob Keller Signed-off-by: Junio C Hamano --- quote.c | 13 +++++++++++++ quote.h | 3 +++ 2 files changed, 16 insertions(+) diff --git a/quote.c b/quote.c index fe884d2452..b281a8fe45 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 99e04d34bf..6c53a2cc66 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 -- cgit v1.2.1 From 14111fc49272a70ceaeb5039796fbceb8a6e1cb7 Mon Sep 17 00:00:00 2001 From: Jacob Keller Date: Mon, 29 Feb 2016 14:58:35 -0800 Subject: git: submodule honor -c credential.* from command line 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 Signed-off-by: Junio C Hamano --- 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 35ae85a7e1..3d37c3f182 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 2dd29b3df0..1f132b489b 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 64146352ae..48e2ab62da 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 0000000000..149d42864f --- /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 -- cgit v1.2.1 From d1f884986d4bdea6b2d5f424ef7d2ca441070a61 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 22 Mar 2016 15:50:51 -0400 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: Shin Fan Signed-off-by: Jeff King Reviewed-by: Jonathan Nieder Tested-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- 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 b95ac3a9cd..8570eec51d 100644 --- a/config.c +++ b/config.c @@ -161,7 +161,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 52678e7d0a..c967740304 100755 --- a/t/t1300-repo-config.sh +++ b/t/t1300-repo-config.sh @@ -1083,6 +1083,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 && -- cgit v1.2.1 From 1149ee214e6b9e2450f817a808f74e4e761e7295 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 28 Apr 2016 09:36:37 -0400 Subject: t5550: fix typo in $HTTPD_URL Commit 14111fc (git: submodule honor -c credential.* from command line, 2016-02-29) accidentally wrote $HTTP_URL. It happened to work because we ended up with "credential..helper", which we treat the same as "credential.helper", applying it to all URLs. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t5550-http-fetch-dumb.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh index 48e2ab62da..81cc57f8be 100755 --- a/t/t5550-http-fetch-dumb.sh +++ b/t/t5550-http-fetch-dumb.sh @@ -103,7 +103,7 @@ test_expect_success 'cmdline credential config passes into submodules' ' 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" \ + git -c "credential.$HTTPD_URL.username=user@host" \ clone --recursive super super-clone && expect_askpass pass user@host ' -- cgit v1.2.1 From 455d22c1c6abc19693beb8a25bc3d976290d6b84 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 28 Apr 2016 09:37:04 -0400 Subject: t5550: break submodule config test into multiple sub-tests Right now we test only the cloning case, but there are other interesting cases (e.g., fetching). Let's pull the setup bits into their own test, which will make things flow more logically once we start adding more tests which use the setup. Let's also introduce some whitespace to the clone-test to split the two parts: making sure it fails without our cmdline config, and that it succeeds with it. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t5550-http-fetch-dumb.sh | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh index 81cc57f8be..e8e91bbb6d 100755 --- a/t/t5550-http-fetch-dumb.sh +++ b/t/t5550-http-fetch-dumb.sh @@ -91,17 +91,21 @@ test_expect_success 'configured username does not override URL' ' expect_askpass pass user@host ' -test_expect_success 'cmdline credential config passes into submodules' ' +test_expect_success 'set up repo with http 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" - ) && + ) +' + +test_expect_success 'cmdline credential config passes to submodule via clone' ' 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.$HTTPD_URL.username=user@host" \ clone --recursive super super-clone && -- cgit v1.2.1 From 860cba61a3eac38151fd203547df7515023303e9 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 28 Apr 2016 09:37:44 -0400 Subject: submodule: export sanitized GIT_CONFIG_PARAMETERS Commit 14111fc (git: submodule honor -c credential.* from command line, 2016-02-29) taught git-submodule.sh to save the sanitized value of $GIT_CONFIG_PARAMETERS when clearing the environment for a submodule. However, it failed to export the result, meaning that it had no effect for any sub-programs. We didn't catch this in our initial tests because we checked only the "clone" case, which does not go through the shell script at all. Provoking "git submodule update" to do a fetch demonstrates the bug. Noticed-by: Lars Schneider Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- git-submodule.sh | 1 + t/t5550-http-fetch-dumb.sh | 17 +++++++++++++++++ 2 files changed, 18 insertions(+) diff --git a/git-submodule.sh b/git-submodule.sh index 1f132b489b..91f5856df8 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -200,6 +200,7 @@ sanitize_submodule_env() sanitized_config=$(git submodule--helper sanitize-config) clear_local_git_env GIT_CONFIG_PARAMETERS=$sanitized_config + export GIT_CONFIG_PARAMETERS } # diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh index e8e91bbb6d..13ac788fde 100755 --- a/t/t5550-http-fetch-dumb.sh +++ b/t/t5550-http-fetch-dumb.sh @@ -112,6 +112,23 @@ test_expect_success 'cmdline credential config passes to submodule via clone' ' expect_askpass pass user@host ' +test_expect_success 'cmdline credential config passes submodule update' ' + # advance the submodule HEAD so that a fetch is required + git commit --allow-empty -m foo && + git push "$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/repo.git" HEAD && + sha1=$(git rev-parse HEAD) && + git -C super-clone update-index --cacheinfo 160000,$sha1,sub && + + set_askpass wrong pass@host && + test_must_fail git -C super-clone submodule update && + + set_askpass wrong pass@host && + git -C super-clone \ + -c "credential.$HTTPD_URL.username=user@host" \ + submodule update && + expect_askpass pass user@host +' + test_expect_success 'fetch changes via http' ' echo content >>file && git commit -a -m two && -- cgit v1.2.1 From 4638728c632e59715b7346ddeca83528d37a4894 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 28 Apr 2016 09:38:20 -0400 Subject: submodule--helper: move config-sanitizing to submodule.c These functions should be used by any code which spawns a submodule process, which may happen in submodule.c (e.g., for spawning fetch). Let's move them there and make them public so that submodule--helper can continue to use them. Since they're now public, let's also provide a basic overview of their intended use. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 48 --------------------------------------------- submodule.c | 48 +++++++++++++++++++++++++++++++++++++++++++++ submodule.h | 16 +++++++++++++++ 3 files changed, 64 insertions(+), 48 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 3d37c3f182..16d6432f91 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -125,54 +125,6 @@ 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) { diff --git a/submodule.c b/submodule.c index b83939c294..24e8182f59 100644 --- a/submodule.c +++ b/submodule.c @@ -13,6 +13,7 @@ #include "argv-array.h" #include "blob.h" #include "thread-utils.h" +#include "quote.h" static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND; static struct string_list changed_submodule_paths; @@ -1097,3 +1098,50 @@ void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir) strbuf_release(&rel_path); free((void *)real_work_tree); } +/* + * 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; +} + +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; +} + +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); + } + } + +} diff --git a/submodule.h b/submodule.h index e06eaa5ebb..48690b156a 100644 --- a/submodule.h +++ b/submodule.h @@ -43,4 +43,20 @@ int find_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_nam int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name); void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir); +/* + * This function is intended as a callback for use with + * git_config_from_parameters(). It ignores any config options which + * are not suitable for passing along to a submodule, and accumulates the rest + * in "data", which must be a pointer to a strbuf. The end result can + * be put into $GIT_CONFIG_PARAMETERS for passing to a sub-process. + */ +int sanitize_submodule_config(const char *var, const char *value, void *data); + +/* + * Prepare the "env_array" parameter of a "struct child_process" for executing + * a submodule by clearing any repo-specific envirionment variables, but + * retaining any config approved by sanitize_submodule_config(). + */ +void prepare_submodule_repo_env(struct argv_array *out); + #endif -- cgit v1.2.1 From c12e8656700be6084aec49df66447e701fda1ecf Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 28 Apr 2016 09:39:15 -0400 Subject: submodule: use prepare_submodule_repo_env consistently Before 14111fc (git: submodule honor -c credential.* from command line, 2016-02-29), it was sufficient for code which spawned a process in a submodule to just set the child process's "env" field to "local_repo_env" to clear the environment of any repo-specific variables. That commit introduced a more complicated procedure, in which we clear most variables but allow through sanitized config. For C code, we used that procedure only for cloning, but not for any of the programs spawned by submodule.c. As a result, things like "git fetch --recurse-submodules" behave differently than "git clone --recursive"; the former will not pass through the sanitized config. We can fix this by using prepare_submodule_repo_env() everywhere in submodule.c. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- submodule.c | 14 +++++++------- t/t5550-http-fetch-dumb.sh | 11 +++++++++++ 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/submodule.c b/submodule.c index 24e8182f59..c18ab9b397 100644 --- a/submodule.c +++ b/submodule.c @@ -367,7 +367,7 @@ static int submodule_needs_pushing(const char *path, const unsigned char sha1[20 argv[1] = sha1_to_hex(sha1); cp.argv = argv; - cp.env = local_repo_env; + prepare_submodule_repo_env(&cp.env_array); cp.git_cmd = 1; cp.no_stdin = 1; cp.out = -1; @@ -454,7 +454,7 @@ static int push_submodule(const char *path) const char *argv[] = {"push", NULL}; cp.argv = argv; - cp.env = local_repo_env; + prepare_submodule_repo_env(&cp.env_array); cp.git_cmd = 1; cp.no_stdin = 1; cp.dir = path; @@ -500,7 +500,7 @@ static int is_submodule_commit_present(const char *path, unsigned char sha1[20]) argv[3] = sha1_to_hex(sha1); cp.argv = argv; - cp.env = local_repo_env; + prepare_submodule_repo_env(&cp.env_array); cp.git_cmd = 1; cp.no_stdin = 1; cp.dir = path; @@ -683,7 +683,7 @@ static int get_next_submodule(struct child_process *cp, if (is_directory(git_dir)) { child_process_init(cp); cp->dir = strbuf_detach(&submodule_path, NULL); - cp->env = local_repo_env; + prepare_submodule_repo_env(&cp->env_array); cp->git_cmd = 1; if (!spf->quiet) strbuf_addf(err, "Fetching submodule %s%s\n", @@ -796,7 +796,7 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked) argv[2] = "-uno"; cp.argv = argv; - cp.env = local_repo_env; + prepare_submodule_repo_env(&cp.env_array); cp.git_cmd = 1; cp.no_stdin = 1; cp.out = -1; @@ -857,7 +857,7 @@ int submodule_uses_gitfile(const char *path) /* Now test that all nested submodules use a gitfile too */ cp.argv = argv; - cp.env = local_repo_env; + prepare_submodule_repo_env(&cp.env_array); cp.git_cmd = 1; cp.no_stdin = 1; cp.no_stderr = 1; @@ -890,7 +890,7 @@ int ok_to_remove_submodule(const char *path) return 0; cp.argv = argv; - cp.env = local_repo_env; + prepare_submodule_repo_env(&cp.env_array); cp.git_cmd = 1; cp.no_stdin = 1; cp.out = -1; diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh index 13ac788fde..3484b6f0f3 100755 --- a/t/t5550-http-fetch-dumb.sh +++ b/t/t5550-http-fetch-dumb.sh @@ -112,6 +112,17 @@ test_expect_success 'cmdline credential config passes to submodule via clone' ' expect_askpass pass user@host ' +test_expect_success 'cmdline credential config passes submodule via fetch' ' + set_askpass wrong pass@host && + test_must_fail git -C super-clone fetch --recurse-submodules && + + set_askpass wrong pass@host && + git -C super-clone \ + -c "credential.$HTTPD_URL.username=user@host" \ + fetch --recurse-submodules && + expect_askpass pass user@host +' + test_expect_success 'cmdline credential config passes submodule update' ' # advance the submodule HEAD so that a fetch is required git commit --allow-empty -m foo && -- cgit v1.2.1 From 89044baa8b8a14b48e78a42ebdc43cfcd144ce28 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 4 May 2016 21:22:19 -0400 Subject: submodule: stop sanitizing config options The point of having a whitelist of command-line config options to pass to submodules was two-fold: 1. It prevented obvious nonsense like using core.worktree for multiple repos. 2. It could prevent surprise when the user did not mean for the options to leak to the submodules (e.g., http.sslverify=false). For case 1, the answer is mostly "if it hurts, don't do that". For case 2, we can note that any such example has a matching inverted surprise (e.g., a user who meant http.sslverify=true to apply everywhere, but it didn't). So this whitelist is probably not giving us any benefit, and is already creating a hassle as people propose things to put on it. Let's just drop it entirely. Note that we still need to keep a special code path for "prepare the submodule environment", because we still have to take care to pass through $GIT_CONFIG_PARAMETERS (and block the rest of the repo-specific environment variables). We can do this easily from within the submodule shell script, which lets us drop the submodule--helper option entirely (and it's OK to do so because as a "--" program, it is entirely a private implementation detail). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 17 ----------------- git-submodule.sh | 4 ++-- submodule.c | 39 +-------------------------------------- submodule.h | 11 +---------- t/t7412-submodule--helper.sh | 26 -------------------------- 5 files changed, 4 insertions(+), 93 deletions(-) delete mode 100755 t/t7412-submodule--helper.sh diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 16d6432f91..89250f0b78 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -260,22 +260,6 @@ 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 *); @@ -285,7 +269,6 @@ 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 91f5856df8..b1c056c715 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -197,9 +197,9 @@ isnumber() # of the settings from GIT_CONFIG_PARAMETERS. sanitize_submodule_env() { - sanitized_config=$(git submodule--helper sanitize-config) + save_config=$GIT_CONFIG_PARAMETERS clear_local_git_env - GIT_CONFIG_PARAMETERS=$sanitized_config + GIT_CONFIG_PARAMETERS=$save_config export GIT_CONFIG_PARAMETERS } diff --git a/submodule.c b/submodule.c index c18ab9b397..d598881114 100644 --- a/submodule.c +++ b/submodule.c @@ -1098,50 +1098,13 @@ void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir) strbuf_release(&rel_path); free((void *)real_work_tree); } -/* - * 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; -} - -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; -} 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 { + if (strcmp(*var, CONFIG_DATA_ENVIRONMENT)) argv_array_push(out, *var); - } } - } diff --git a/submodule.h b/submodule.h index 48690b156a..869d259fac 100644 --- a/submodule.h +++ b/submodule.h @@ -43,19 +43,10 @@ int find_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_nam int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name); void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir); -/* - * This function is intended as a callback for use with - * git_config_from_parameters(). It ignores any config options which - * are not suitable for passing along to a submodule, and accumulates the rest - * in "data", which must be a pointer to a strbuf. The end result can - * be put into $GIT_CONFIG_PARAMETERS for passing to a sub-process. - */ -int sanitize_submodule_config(const char *var, const char *value, void *data); - /* * Prepare the "env_array" parameter of a "struct child_process" for executing * a submodule by clearing any repo-specific envirionment variables, but - * retaining any config approved by sanitize_submodule_config(). + * retaining any config in the environment. */ void prepare_submodule_repo_env(struct argv_array *out); diff --git a/t/t7412-submodule--helper.sh b/t/t7412-submodule--helper.sh deleted file mode 100755 index 149d42864f..0000000000 --- a/t/t7412-submodule--helper.sh +++ /dev/null @@ -1,26 +0,0 @@ -#!/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 -- cgit v1.2.1