From e1e9403d2ce9e4a8d165eb20581f17a153c400d2 Mon Sep 17 00:00:00 2001 From: Jacob Keller Date: Mon, 29 Feb 2016 14:58:30 -0800 Subject: [PATCH 01/17] 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 d56207ea05..0689852613 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" && @@ -736,7 +736,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 From f8742866b0b42425afd27540b3cb485689411bd5 Mon Sep 17 00:00:00 2001 From: Jacob Keller Date: Mon, 29 Feb 2016 14:58:31 -0800 Subject: [PATCH 02/17] 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 Signed-off-by: Johannes Schindelin --- builtin/submodule--helper.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 3bea3aaa50..9c04f21c9d 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -191,6 +191,10 @@ static int module_clone(int argc, const char **argv, const char *prefix) if (!path || !*path) die(_("submodule--helper: unspecified or empty --path")); + if (argc) + usage_with_options(git_submodule_helper_usage, + module_clone_options); + strbuf_addf(&sb, "%s/modules/%s", get_git_dir(), name); sm_gitdir = xstrdup(absolute_path(sb.buf)); strbuf_reset(&sb); From 91ee1d3a65e3769a4e80c0087fcae202bdc707c1 Mon Sep 17 00:00:00 2001 From: Jacob Keller Date: Mon, 29 Feb 2016 14:58:32 -0800 Subject: [PATCH 03/17] 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 Signed-off-by: Johannes Schindelin --- 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 9c04f21c9d..f7a37b9c67 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -181,7 +181,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 }; From 5ce58ff0eb94c3b1adbc1787fed65dbd6e25f32a Mon Sep 17 00:00:00 2001 From: Jacob Keller Date: Mon, 29 Feb 2016 14:58:33 -0800 Subject: [PATCH 04/17] 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 Signed-off-by: Johannes Schindelin --- 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 f7a37b9c67..f2b571565d 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -180,8 +180,8 @@ 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 }; @@ -191,7 +191,7 @@ static int module_clone(int argc, const char **argv, const char *prefix) if (!path || !*path) die(_("submodule--helper: unspecified or empty --path")); - if (argc) + if (argc || !url) usage_with_options(git_submodule_helper_usage, module_clone_options); From dc7f50f41b525af280fd6c7222c38a291c9a2464 Mon Sep 17 00:00:00 2001 From: Jacob Keller Date: Mon, 29 Feb 2016 14:58:34 -0800 Subject: [PATCH 05/17] quote: implement sq_quotef() Signed-off-by: Jacob Keller Signed-off-by: Junio C Hamano Signed-off-by: Johannes Schindelin --- 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 From 5ad1c571793c3c45096da4f1ab1389a4bdbf5481 Mon Sep 17 00:00:00 2001 From: Jacob Keller Date: Mon, 29 Feb 2016 14:58:35 -0800 Subject: [PATCH 06/17] 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 Signed-off-by: Johannes Schindelin --- builtin/submodule--helper.c | 68 +++++++++++++++++++++++++++++++++++- git-submodule.sh | 38 ++++++++++++-------- t/t5550-http-fetch-dumb.sh | 17 +++++++++ t/t7412-submodule--helper.sh | 26 ++++++++++++++ 4 files changed, 134 insertions(+), 15 deletions(-) create mode 100755 t/t7412-submodule--helper.sh diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index f2b571565d..f57daa4d65 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -118,6 +118,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) { @@ -139,7 +188,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); @@ -248,6 +297,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 *); @@ -257,6 +322,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 0689852613..553f7fd446 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 ... @@ -601,14 +611,14 @@ cmd_deinit() } is_tip_reachable () ( - clear_local_git_env + sanitize_submodule_env && cd "$1" && rev=$(git rev-list -n 1 "$2" --not --all 2>/dev/null) && test -z "$rev" ) fetch_in_submodule () ( - clear_local_git_env + sanitize_submodule_env && cd "$1" && case "$2" in '') @@ -740,7 +750,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 @@ -750,11 +760,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 @@ -819,7 +829,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" @@ -835,7 +845,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 ) @@ -873,7 +883,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 || @@ -1157,7 +1167,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" @@ -1167,7 +1177,7 @@ cmd_status() then ( prefix="$displaypath/" - clear_local_git_env + sanitize_submodule_env wt_prefix= cd "$sm_path" && eval cmd_status @@ -1242,7 +1252,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 From 42465dd723d051848dd84594fda6ef639d04e4e9 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 22 Mar 2016 15:50:51 -0400 Subject: [PATCH 07/17] 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 Signed-off-by: Johannes Schindelin --- 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 6dbc8a409e..8d2e1c8ad7 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 3d6f1db9da..d934a24417 100755 --- a/t/t1300-repo-config.sh +++ b/t/t1300-repo-config.sh @@ -1087,6 +1087,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 && From 40d6e59fbd36589d248c25a903f1b0f288dd752c Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 28 Apr 2016 09:36:37 -0400 Subject: [PATCH 08/17] 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 Signed-off-by: Johannes Schindelin --- 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 ' From 29bba8da17de884627ea78a344ff6a529d63fcf8 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 28 Apr 2016 09:37:04 -0400 Subject: [PATCH 09/17] 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 Signed-off-by: Johannes Schindelin --- 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 && From d9cf30d8fba9427b358ec3de782fe7009c043eef Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 28 Apr 2016 09:37:44 -0400 Subject: [PATCH 10/17] 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 Signed-off-by: Johannes Schindelin --- 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 553f7fd446..5ccbfb6069 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 && From b70904b6930985434fb36a09421292b10694972c Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 28 Apr 2016 09:38:20 -0400 Subject: [PATCH 11/17] 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 Signed-off-by: Johannes Schindelin --- 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 f57daa4d65..686e39ea29 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -119,54 +119,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 62c4356c50..f22e22d333 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; @@ -1094,3 +1095,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 From 04b5b27eacbaf9e38ebbf4f46027dfd056a9670e Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 28 Apr 2016 09:39:15 -0400 Subject: [PATCH 12/17] 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 Signed-off-by: Johannes Schindelin --- 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 f22e22d333..42f3cd8ddf 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", @@ -795,7 +795,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; @@ -856,7 +856,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; @@ -889,7 +889,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 && From c7aea25fc602a825d4c62b37db2bf00b6adc36a1 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 4 May 2016 21:22:19 -0400 Subject: [PATCH 13/17] 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 Signed-off-by: Johannes Schindelin --- 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 686e39ea29..c23da94957 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -249,22 +249,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 *); @@ -274,7 +258,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 5ccbfb6069..b597cf36c7 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 42f3cd8ddf..4b02da6987 100644 --- a/submodule.c +++ b/submodule.c @@ -1095,50 +1095,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 From 933d3490c119233cd5cab999d4fb6ca716fb3551 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 27 Apr 2016 14:20:37 +0200 Subject: [PATCH 14/17] http: support sending custom HTTP headers We introduce a way to send custom HTTP headers with all requests. This allows us, for example, to send an extra token from build agents for temporary access to private repositories. (This is the use case that triggered this patch.) This feature can be used like this: git -c http.extraheader='Secret: sssh!' fetch $URL $REF Note that `curl_easy_setopt(..., CURLOPT_HTTPHEADER, ...)` takes only a single list, overriding any previous call. This means we have to collect _all_ of the headers we want to use into a single list, and feed it to cURL in one shot. Since we already unconditionally set a "pragma" header when initializing the curl handles, we can add our new headers to that list. For callers which override the default header list (like probe_rpc), we provide `http_copy_default_headers()` so they can do the same trick. Big thanks to Jeff King and Junio Hamano for their outstanding help and patient reviews. Signed-off-by: Johannes Schindelin Reviewed-by: Jeff King Signed-off-by: Junio C Hamano Signed-off-by: Johannes Schindelin --- Documentation/config.txt | 6 ++++++ http-push.c | 10 +++++----- http.c | 35 ++++++++++++++++++++++++++++++++--- http.h | 1 + remote-curl.c | 4 ++-- t/lib-httpd/apache.conf | 8 ++++++++ t/t5551-http-fetch-smart.sh | 7 +++++++ 7 files changed, 61 insertions(+), 10 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index 02696208c9..6e436227e5 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -1668,6 +1668,12 @@ http.emptyAuth:: a username in the URL, as libcurl normally requires a username for authentication. +http.extraHeader:: + Pass an additional HTTP header when communicating with a server. If + more than one such entry exists, all of them are added as extra + headers. To allow overriding the settings inherited from the system + config, an empty value will reset the extra headers to the empty list. + http.cookieFile:: The pathname of a file containing previously stored cookie lines, which should be used diff --git a/http-push.c b/http-push.c index bd60668707..ae2b7f19d2 100644 --- a/http-push.c +++ b/http-push.c @@ -211,7 +211,7 @@ static void curl_setup_http(CURL *curl, const char *url, static struct curl_slist *get_dav_token_headers(struct remote_lock *lock, enum dav_header_flag options) { struct strbuf buf = STRBUF_INIT; - struct curl_slist *dav_headers = NULL; + struct curl_slist *dav_headers = http_copy_default_headers(); if (options & DAV_HEADER_IF) { strbuf_addf(&buf, "If: (<%s>)", lock->token); @@ -417,7 +417,7 @@ static void start_put(struct transfer_request *request) static void start_move(struct transfer_request *request) { struct active_request_slot *slot; - struct curl_slist *dav_headers = NULL; + struct curl_slist *dav_headers = http_copy_default_headers(); slot = get_active_slot(); slot->callback_func = process_response; @@ -845,7 +845,7 @@ static struct remote_lock *lock_remote(const char *path, long timeout) char *ep; char timeout_header[25]; struct remote_lock *lock = NULL; - struct curl_slist *dav_headers = NULL; + struct curl_slist *dav_headers = http_copy_default_headers(); struct xml_ctx ctx; char *escaped; @@ -1126,7 +1126,7 @@ static void remote_ls(const char *path, int flags, struct slot_results results; struct strbuf in_buffer = STRBUF_INIT; struct buffer out_buffer = { STRBUF_INIT, 0 }; - struct curl_slist *dav_headers = NULL; + struct curl_slist *dav_headers = http_copy_default_headers(); struct xml_ctx ctx; struct remote_ls_ctx ls; @@ -1204,7 +1204,7 @@ static int locking_available(void) struct slot_results results; struct strbuf in_buffer = STRBUF_INIT; struct buffer out_buffer = { STRBUF_INIT, 0 }; - struct curl_slist *dav_headers = NULL; + struct curl_slist *dav_headers = http_copy_default_headers(); struct xml_ctx ctx; int lock_flags = 0; char *escaped; diff --git a/http.c b/http.c index 1044f9ba0e..6fe74d5eea 100644 --- a/http.c +++ b/http.c @@ -114,6 +114,7 @@ static unsigned long http_auth_methods = CURLAUTH_ANY; static struct curl_slist *pragma_header; static struct curl_slist *no_pragma_header; +static struct curl_slist *extra_http_headers; static struct active_request_slot *active_queue_head; @@ -323,6 +324,19 @@ static int http_options(const char *var, const char *value, void *cb) #endif } + if (!strcmp("http.extraheader", var)) { + if (!value) { + return config_error_nonbool(var); + } else if (!*value) { + curl_slist_free_all(extra_http_headers); + extra_http_headers = NULL; + } else { + extra_http_headers = + curl_slist_append(extra_http_headers, value); + } + return 0; + } + /* Fall back on the default ones */ return git_default_config(var, value, cb); } @@ -678,8 +692,10 @@ void http_init(struct remote *remote, const char *url, int proactive_auth) if (remote) var_override(&http_proxy_authmethod, remote->http_proxy_authmethod); - pragma_header = curl_slist_append(pragma_header, "Pragma: no-cache"); - no_pragma_header = curl_slist_append(no_pragma_header, "Pragma:"); + pragma_header = curl_slist_append(http_copy_default_headers(), + "Pragma: no-cache"); + no_pragma_header = curl_slist_append(http_copy_default_headers(), + "Pragma:"); #ifdef USE_CURL_MULTI { @@ -765,6 +781,9 @@ void http_cleanup(void) #endif curl_global_cleanup(); + curl_slist_free_all(extra_http_headers); + extra_http_headers = NULL; + curl_slist_free_all(pragma_header); pragma_header = NULL; @@ -1163,6 +1182,16 @@ int run_one_slot(struct active_request_slot *slot, return handle_curl_result(results); } +struct curl_slist *http_copy_default_headers(void) +{ + struct curl_slist *headers = NULL, *h; + + for (h = extra_http_headers; h; h = h->next) + headers = curl_slist_append(headers, h->data); + + return headers; +} + static CURLcode curlinfo_strbuf(CURL *curl, CURLINFO info, struct strbuf *buf) { char *ptr; @@ -1380,7 +1409,7 @@ static int http_request(const char *url, { struct active_request_slot *slot; struct slot_results results; - struct curl_slist *headers = NULL; + struct curl_slist *headers = http_copy_default_headers(); struct strbuf buf = STRBUF_INIT; const char *accept_language; int ret; diff --git a/http.h b/http.h index 4ef4bbda7d..36f558bfb3 100644 --- a/http.h +++ b/http.h @@ -106,6 +106,7 @@ extern void step_active_slots(void); extern void http_init(struct remote *remote, const char *url, int proactive_auth); extern void http_cleanup(void); +extern struct curl_slist *http_copy_default_headers(void); extern long int git_curl_ipresolve; extern int active_requests; diff --git a/remote-curl.c b/remote-curl.c index 15e48e25fb..672b382e5a 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -474,7 +474,7 @@ static int run_slot(struct active_request_slot *slot, static int probe_rpc(struct rpc_state *rpc, struct slot_results *results) { struct active_request_slot *slot; - struct curl_slist *headers = NULL; + struct curl_slist *headers = http_copy_default_headers(); struct strbuf buf = STRBUF_INIT; int err; @@ -503,7 +503,7 @@ static int probe_rpc(struct rpc_state *rpc, struct slot_results *results) static int post_rpc(struct rpc_state *rpc) { struct active_request_slot *slot; - struct curl_slist *headers = NULL; + struct curl_slist *headers = http_copy_default_headers(); int use_gzip = rpc->gzip_request; char *gzip_body = NULL; size_t gzip_size = 0; diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf index 9317ba0858..b8ed96fac6 100644 --- a/t/lib-httpd/apache.conf +++ b/t/lib-httpd/apache.conf @@ -102,6 +102,14 @@ Alias /auth/dumb/ www/auth/dumb/ SetEnv GIT_HTTP_EXPORT_ALL Header set Set-Cookie name=value + + + Require expr %{HTTP:x-magic-one} == 'abra' + Require expr %{HTTP:x-magic-two} == 'cadabra' + + SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH} + SetEnv GIT_HTTP_EXPORT_ALL + ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1 ScriptAlias /broken_smart/ broken-smart-http.sh/ ScriptAlias /error/ error.sh/ diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh index 58207d8825..e44fe72c7a 100755 --- a/t/t5551-http-fetch-smart.sh +++ b/t/t5551-http-fetch-smart.sh @@ -282,5 +282,12 @@ test_expect_success EXPENSIVE 'http can handle enormous ref negotiation' ' test_line_count = 100000 tags ' +test_expect_success 'custom http headers' ' + test_must_fail git fetch "$HTTPD_URL/smart_headers/repo.git" && + git -c http.extraheader="x-magic-one: abra" \ + -c http.extraheader="x-magic-two: cadabra" \ + fetch "$HTTPD_URL/smart_headers/repo.git" +' + stop_httpd test_done From 3f74de568025ff04c0addd0c642c85942991d0d5 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Mon, 9 May 2016 07:59:16 +0200 Subject: [PATCH 15/17] tests: adjust the configuration for Apache 2.2 Lars Schneider noticed that the configuration introduced to test the extra HTTP headers cannot be used with Apache 2.2 (which is still actively maintained, as pointed out by Junio Hamano). To let the tests pass with Apache 2.2 again, let's substitute the offending and `expr` by using old school RewriteCond statements. As RewriteCond does not allow testing for *non*-matches, we simply match the desired case first and let it pass by marking the RewriteRule as '[L]' ("last rule, do not process any other matching RewriteRules after this"), and then have another RewriteRule that matches all other cases and lets them fail via '[F]' ("fail"). Signed-off-by: Johannes Schindelin Tested-by: Lars Schneider Signed-off-by: Junio C Hamano Signed-off-by: Johannes Schindelin --- t/lib-httpd/apache.conf | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf index b8ed96fac6..018a83a5a1 100644 --- a/t/lib-httpd/apache.conf +++ b/t/lib-httpd/apache.conf @@ -103,10 +103,6 @@ Alias /auth/dumb/ www/auth/dumb/ Header set Set-Cookie name=value - - Require expr %{HTTP:x-magic-one} == 'abra' - Require expr %{HTTP:x-magic-two} == 'cadabra' - SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH} SetEnv GIT_HTTP_EXPORT_ALL @@ -136,6 +132,18 @@ RewriteRule ^/ftp-redir/(.*)$ ftp://localhost:1000/$1 [R=302] RewriteRule ^/loop-redir/x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-(.*) /$1 [R=302] RewriteRule ^/loop-redir/(.*)$ /loop-redir/x-$1 [R=302] +# Apache 2.2 does not understand , so we use RewriteCond. +# And as RewriteCond does not allow testing for non-matches, we match +# the desired case first (one has abra, two has cadabra), and let it +# pass by marking the RewriteRule as [L], "last rule, do not process +# any other matching RewriteRules after this"), and then have another +# RewriteRule that matches all other cases and lets them fail via '[F]', +# "fail the request". +RewriteCond %{HTTP:x-magic-one} =abra +RewriteCond %{HTTP:x-magic-two} =cadabra +RewriteRule ^/smart_headers/.* - [L] +RewriteRule ^/smart_headers/.* - [F] + LoadModule ssl_module modules/mod_ssl.so From 64524be76a0f18a704e796ccba816428a461c585 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Mon, 9 May 2016 08:19:00 +0200 Subject: [PATCH 16/17] t5551: make the test for extra HTTP headers more robust To test that extra HTTP headers are passed correctly, t5551 verifies that a fetch succeeds when two required headers are passed, and that the fetch does not succeed when those headers are not passed. However, this test would also succeed if the configuration required only one header. As Apache's configuration is notoriously tricky (this developer frequently requires StackOverflow's help to understand Apache's documentation), especially when still supporting the 2.2 line, let's just really make sure that the test verifies what we want it to verify. Signed-off-by: Johannes Schindelin Reviewed-by: Jeff King Signed-off-by: Junio C Hamano --- t/t5551-http-fetch-smart.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh index e44fe72c7a..43b257e7fd 100755 --- a/t/t5551-http-fetch-smart.sh +++ b/t/t5551-http-fetch-smart.sh @@ -283,7 +283,8 @@ test_expect_success EXPENSIVE 'http can handle enormous ref negotiation' ' ' test_expect_success 'custom http headers' ' - test_must_fail git fetch "$HTTPD_URL/smart_headers/repo.git" && + test_must_fail git -c http.extraheader="x-magic-two: cadabra" \ + fetch "$HTTPD_URL/smart_headers/repo.git" && git -c http.extraheader="x-magic-one: abra" \ -c http.extraheader="x-magic-two: cadabra" \ fetch "$HTTPD_URL/smart_headers/repo.git" From 2a2b52bb4edabfbd6fa93ae8e9812f6fa5fc4368 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 10 May 2016 09:08:56 +0200 Subject: [PATCH 17/17] submodule: ensure that -c http.extraheader is heeded To support this developer's use case of allowing build agents token-based access to private repositories, we introduced the http.extraheader feature, allowing extra HTTP headers to be sent along with every HTTP request. This patch verifies that we can configure these extra HTTP headers via the command-line for use with `git submodule update`, too. Example: git -c http.extraheader="Secret: Sauce" submodule update --init Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano Signed-off-by: Johannes Schindelin --- t/t5551-http-fetch-smart.sh | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh index 43b257e7fd..2f375eb94d 100755 --- a/t/t5551-http-fetch-smart.sh +++ b/t/t5551-http-fetch-smart.sh @@ -287,7 +287,16 @@ test_expect_success 'custom http headers' ' fetch "$HTTPD_URL/smart_headers/repo.git" && git -c http.extraheader="x-magic-one: abra" \ -c http.extraheader="x-magic-two: cadabra" \ - fetch "$HTTPD_URL/smart_headers/repo.git" + fetch "$HTTPD_URL/smart_headers/repo.git" && + git update-index --add --cacheinfo 160000,$(git rev-parse HEAD),sub && + git config -f .gitmodules submodule.sub.path sub && + git config -f .gitmodules submodule.sub.url \ + "$HTTPD_URL/smart_headers/repo.git" && + git submodule init sub && + test_must_fail git submodule update sub && + git -c http.extraheader="x-magic-one: abra" \ + -c http.extraheader="x-magic-two: cadabra" \ + submodule update sub ' stop_httpd