From c72ee44bf4bc7549ee8710037ae8adfae6d8efc2 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 18 May 2016 18:39:02 -0400 Subject: [PATCH 1/7] git_config_with_options: drop "found" counting Prior to 1f2baa7 (config: treat non-existent config files as empty, 2010-10-21), we returned an error if any config files were missing. That commit made this a non-error, but returned the number of sources found, in case any caller wanted to distinguish this case. In the past 5+ years, no caller has; the only two places which bother to check the return value care only about the error case. Let's drop this code, which complicates the function. Similarly, let's drop the "found anything" return from git_config_from_parameters, which was present only to support this (and similarly has never had other callers care for the past 5+ years). Note that we do need to update a comment in one of the callers, even though the code immediately below it doesn't care about this case. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- config.c | 34 +++++++++------------------------- 1 file changed, 9 insertions(+), 25 deletions(-) diff --git a/config.c b/config.c index f51c56bf92..771d0ddbec 100644 --- a/config.c +++ b/config.c @@ -230,7 +230,7 @@ int git_config_from_parameters(config_fn_t fn, void *data) free(argv); free(envw); - return nr > 0; + return 0; } static int get_next_char(void) @@ -1197,47 +1197,31 @@ int git_config_system(void) static int do_git_config_sequence(config_fn_t fn, void *data) { - int ret = 0, found = 0; + int ret = 0; char *xdg_config = xdg_config_home("config"); char *user_config = expand_user_path("~/.gitconfig"); char *repo_config = git_pathdup("config"); - if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0)) { + if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0)) ret += git_config_from_file(fn, git_etc_gitconfig(), data); - found += 1; - } - if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK)) { + if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK)) ret += git_config_from_file(fn, xdg_config, data); - found += 1; - } - if (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK)) { + if (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK)) ret += git_config_from_file(fn, user_config, data); - found += 1; - } - if (repo_config && !access_or_die(repo_config, R_OK, 0)) { + if (repo_config && !access_or_die(repo_config, R_OK, 0)) ret += git_config_from_file(fn, repo_config, data); - found += 1; - } - switch (git_config_from_parameters(fn, data)) { - case -1: /* error */ + if (git_config_from_parameters(fn, data) < 0) die(_("unable to parse command-line config")); - break; - case 0: /* found nothing */ - break; - default: /* found at least one item */ - found++; - break; - } free(xdg_config); free(user_config); free(repo_config); - return ret == 0 ? found : ret; + return ret; } int git_config_with_options(config_fn_t fn, void *data, @@ -1272,7 +1256,7 @@ static void git_config_raw(config_fn_t fn, void *data) if (git_config_with_options(fn, data, NULL, 1) < 0) /* * git_config_with_options() normally returns only - * positive values, as most errors are fatal, and + * zero, as most errors are fatal, and * non-fatal potential errors are guarded by "if" * statements that are entered only when no error is * possible. From a77d6db69b2cbd16e98763f33ceaa76ff5ca2c54 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 18 May 2016 18:39:49 -0400 Subject: [PATCH 2/7] git_config_parse_parameter: refactor cleanup code We have several exits from the function, each of which has to do some cleanup. Let's consolidate these in an "out" label we can jump to. This doesn't save us much now, but it will help as we add more things that need cleanup. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- config.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/config.c b/config.c index 771d0ddbec..c5d0b0c7aa 100644 --- a/config.c +++ b/config.c @@ -205,6 +205,7 @@ int git_config_parse_parameter(const char *text, int git_config_from_parameters(config_fn_t fn, void *data) { const char *env = getenv(CONFIG_DATA_ENVIRONMENT); + int ret = 0; char *envw; const char **argv = NULL; int nr = 0, alloc = 0; @@ -216,21 +217,21 @@ int git_config_from_parameters(config_fn_t fn, void *data) envw = xstrdup(env); if (sq_dequote_to_argv(envw, &argv, &nr, &alloc) < 0) { - free(envw); - return error("bogus format in " CONFIG_DATA_ENVIRONMENT); + ret = error("bogus format in " CONFIG_DATA_ENVIRONMENT); + goto out; } for (i = 0; i < nr; i++) { if (git_config_parse_parameter(argv[i], fn, data) < 0) { - free(argv); - free(envw); - return -1; + ret = -1; + goto out; } } +out: free(argv); free(envw); - return 0; + return ret; } static int get_next_char(void) From 3258258f51f45efeff5ce0e9f825f347a1404efa Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 18 May 2016 18:41:08 -0400 Subject: [PATCH 3/7] config: set up config_source for command-line config When we parse a config file, we set up the global "cf" variable as a pointer to a "struct config_source" describing the file we are parsing. This is used for error messages, as well as for lookup functions like current_config_name(). The "cf" variable is NULL in two cases: 1. When we are parsing command-line config, in which case there is no source file. 2. When we are not parsing any config at all. Callers like current_config_name() must assume we are in case 1 if they see a NULL "cf". However, this means that if they are accidentally used outside of a config parsing callback, they will quietly return a bogus answer. This might seem like an unlikely accident (why would you ask for the current config file if you are not parsing config?), but it's actually an easy mistake to make due to the configset caching. git_config() serves the answers from a configset cache, and any calls to current_config_name() will claim that we are parsing command-line config, no matter what the original source. So let's distinguish these cases by having the command-line config parser set up a config_source with a NULL name (which callers already handle properly). We can use this to catch programming errors in some cases, and to give better messages to the user in others. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- config.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/config.c b/config.c index c5d0b0c7aa..571151f3e4 100644 --- a/config.c +++ b/config.c @@ -131,7 +131,9 @@ static int handle_path_include(const char *path, struct config_include_data *inc if (!access_or_die(path, R_OK, 0)) { if (++inc->depth > MAX_INCLUDE_DEPTH) die(include_depth_advice, MAX_INCLUDE_DEPTH, path, - cf && cf->name ? cf->name : "the command line"); + !cf ? "" : + cf->name ? cf->name : + "the command line"); ret = git_config_from_file(git_config_include, path, inc); inc->depth--; } @@ -210,9 +212,15 @@ int git_config_from_parameters(config_fn_t fn, void *data) const char **argv = NULL; int nr = 0, alloc = 0; int i; + struct config_source source; if (!env) return 0; + + memset(&source, 0, sizeof(source)); + source.prev = cf; + cf = &source; + /* sq_dequote will write over it */ envw = xstrdup(env); @@ -231,6 +239,7 @@ int git_config_from_parameters(config_fn_t fn, void *data) out: free(argv); free(envw); + cf = source.prev; return ret; } @@ -1341,7 +1350,9 @@ static int configset_add_value(struct config_set *cs, const char *key, const cha l_item->e = e; l_item->value_index = e->value_list.nr - 1; - if (cf) { + if (!cf) + die("BUG: configset_add_value has no source"); + if (cf->name) { kv_info->filename = strintern(cf->name); kv_info->linenr = cf->linenr; } else { @@ -2427,10 +2438,14 @@ int parse_config_key(const char *var, const char *current_config_origin_type(void) { - return cf && cf->origin_type ? cf->origin_type : "command line"; + if (!cf) + die("BUG: current_config_origin_type called outside config callback"); + return cf->origin_type ? cf->origin_type : "command line"; } const char *current_config_name(void) { - return cf && cf->name ? cf->name : ""; + if (!cf) + die("BUG: current_config_name called outside config callback"); + return cf->name ? cf->name : ""; } From 0d44a2dacc84fb7dcb5d684800e976f3b3c76d00 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 26 May 2016 20:32:23 -0400 Subject: [PATCH 4/7] config: return configset value for current_config_ functions When 473166b (config: add 'origin_type' to config_source struct, 2016-02-19) added accessor functions for the origin type and name, it taught them only to look at the "cf" struct that is filled in while we are parsing the config. This is sufficient to make it work with git-config, which uses git_config_with_options() under the hood. That function freshly parses the config files and triggers the callback when it parses each key. Most git programs, however, use git_config(). This interface will populate a cache during the actual parse, and then serve values from the cache. Calling current_config_filename() in a callback here will find a NULL cf and produce an error. There are no such callers right now, but let's prepare for adding some by making this work. We already record source information in a struct attached to each value. We just need to make it globally available and then consult it from the accessor functions. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- cache.h | 1 + config.c | 51 ++++++++++++++++++++++++++++++++++-------- t/helper/test-config.c | 20 +++++++++++++++++ t/t1308-config-set.sh | 24 ++++++++++++++++++++ 4 files changed, 87 insertions(+), 9 deletions(-) diff --git a/cache.h b/cache.h index 6049f86711..1bce212e88 100644 --- a/cache.h +++ b/cache.h @@ -1696,6 +1696,7 @@ extern int ignore_untracked_cache_config; struct key_value_info { const char *filename; int linenr; + const char *origin_type; }; extern NORETURN void git_die_config(const char *key, const char *err, ...) __attribute__((format(printf, 2, 3))); diff --git a/config.c b/config.c index 571151f3e4..d555deeb9a 100644 --- a/config.c +++ b/config.c @@ -38,7 +38,24 @@ struct config_source { long (*do_ftell)(struct config_source *c); }; +/* + * These variables record the "current" config source, which + * can be accessed by parsing callbacks. + * + * The "cf" variable will be non-NULL only when we are actually parsing a real + * config source (file, blob, cmdline, etc). + * + * The "current_config_kvi" variable will be non-NULL only when we are feeding + * cached config from a configset into a callback. + * + * They should generally never be non-NULL at the same time. If they are both + * NULL, then we aren't parsing anything (and depending on the function looking + * at the variables, it's either a bug for it to be called in the first place, + * or it's a function which can be reused for non-config purposes, and should + * fall back to some sane behavior). + */ static struct config_source *cf; +static struct key_value_info *current_config_kvi; static int zlib_compression_seen; @@ -1284,16 +1301,20 @@ static void configset_iter(struct config_set *cs, config_fn_t fn, void *data) struct string_list *values; struct config_set_element *entry; struct configset_list *list = &cs->list; - struct key_value_info *kv_info; for (i = 0; i < list->nr; i++) { entry = list->items[i].e; value_index = list->items[i].value_index; values = &entry->value_list; - if (fn(entry->key, values->items[value_index].string, data) < 0) { - kv_info = values->items[value_index].util; - git_die_config_linenr(entry->key, kv_info->filename, kv_info->linenr); - } + + current_config_kvi = values->items[value_index].util; + + if (fn(entry->key, values->items[value_index].string, data) < 0) + git_die_config_linenr(entry->key, + current_config_kvi->filename, + current_config_kvi->linenr); + + current_config_kvi = NULL; } } @@ -1355,10 +1376,12 @@ static int configset_add_value(struct config_set *cs, const char *key, const cha if (cf->name) { kv_info->filename = strintern(cf->name); kv_info->linenr = cf->linenr; + kv_info->origin_type = strintern(cf->origin_type); } else { /* for values read from `git_config_from_parameters()` */ kv_info->filename = NULL; kv_info->linenr = -1; + kv_info->origin_type = NULL; } si->util = kv_info; @@ -2438,14 +2461,24 @@ int parse_config_key(const char *var, const char *current_config_origin_type(void) { - if (!cf) + const char *type; + if (current_config_kvi) + type = current_config_kvi->origin_type; + else if(cf) + type = cf->origin_type; + else die("BUG: current_config_origin_type called outside config callback"); - return cf->origin_type ? cf->origin_type : "command line"; + return type ? type : "command line"; } const char *current_config_name(void) { - if (!cf) + const char *name; + if (current_config_kvi) + name = current_config_kvi->filename; + else if (cf) + name = cf->name; + else die("BUG: current_config_name called outside config callback"); - return cf->name ? cf->name : ""; + return name ? name : ""; } diff --git a/t/helper/test-config.c b/t/helper/test-config.c index 6a77552210..3605ef8ee6 100644 --- a/t/helper/test-config.c +++ b/t/helper/test-config.c @@ -25,6 +25,9 @@ * ascending order of priority from a config_set * constructed from files entered as arguments. * + * iterate -> iterate over all values using git_config(), and print some + * data for each + * * Examples: * * To print the value with highest priority for key "foo.bAr Baz.rock": @@ -32,6 +35,20 @@ * */ +static int iterate_cb(const char *var, const char *value, void *data) +{ + static int nr; + + if (nr++) + putchar('\n'); + + printf("key=%s\n", var); + printf("value=%s\n", value ? value : "(null)"); + printf("origin=%s\n", current_config_origin_type()); + printf("name=%s\n", current_config_name()); + + return 0; +} int main(int argc, char **argv) { @@ -134,6 +151,9 @@ int main(int argc, char **argv) printf("Value not found for \"%s\"\n", argv[2]); goto exit1; } + } else if (!strcmp(argv[1], "iterate")) { + git_config(iterate_cb, NULL); + goto exit0; } die("%s: Please check the syntax and the function name", argv[0]); diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh index 005d66dbef..d345a885a3 100755 --- a/t/t1308-config-set.sh +++ b/t/t1308-config-set.sh @@ -229,4 +229,28 @@ test_expect_success 'error on modifying repo config without repo' ' ) ' +cmdline_config="'foo.bar=from-cmdline'" +test_expect_success 'iteration shows correct origins' ' + echo "[foo]bar = from-repo" >.git/config && + echo "[foo]bar = from-home" >.gitconfig && + cat >expect <<-EOF && + key=foo.bar + value=from-home + origin=file + name=$(pwd)/.gitconfig + + key=foo.bar + value=from-repo + origin=file + name=.git/config + + key=foo.bar + value=from-cmdline + origin=command line + name= + EOF + GIT_CONFIG_PARAMETERS=$cmdline_config test-config iterate >actual && + test_cmp expect actual +' + test_done From 9acc5911119ec0209877fbaa0a1e68aa714c191e Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 18 May 2016 18:44:23 -0400 Subject: [PATCH 5/7] config: add a notion of "scope" A config callback passed to git_config() doesn't know very much about the context in which it sees a variable. It can ask whether the variable comes from a file, and get the file name. But without analyzing the filename (which is hard to do accurately), it cannot tell whether it is in system-level config, user-level config, or repo-specific config. Generally this doesn't matter; the point of not passing this to the callback is that it should treat the config the same no matter where it comes from. But some programs, like upload-pack, are a special case: we should be able to run them in an untrusted repository, which means we cannot use any "dangerous" config from the repository config file (but it is OK to use it from system or user config). This patch teaches the config code to record the "scope" of each variable, and make it available inside config callbacks, similar to how we give access to the filename. The scope is the starting source for a particular parsing operation, and remains the same even if we include other files (so a .git/config which includes another file will remain CONFIG_SCOPE_REPO, as it would be similarly untrusted). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- cache.h | 11 +++++++++++ config.c | 23 +++++++++++++++++++++++ t/helper/test-config.c | 16 ++++++++++++++++ t/t1308-config-set.sh | 3 +++ 4 files changed, 53 insertions(+) diff --git a/cache.h b/cache.h index 1bce212e88..2e3b377ae8 100644 --- a/cache.h +++ b/cache.h @@ -1604,6 +1604,16 @@ extern const char *get_log_output_encoding(void); extern const char *get_commit_output_encoding(void); extern int git_config_parse_parameter(const char *, config_fn_t fn, void *data); + +enum config_scope { + CONFIG_SCOPE_UNKNOWN = 0, + CONFIG_SCOPE_SYSTEM, + CONFIG_SCOPE_GLOBAL, + CONFIG_SCOPE_REPO, + CONFIG_SCOPE_CMDLINE, +}; + +extern enum config_scope current_config_scope(void); extern const char *current_config_origin_type(void); extern const char *current_config_name(void); @@ -1697,6 +1707,7 @@ struct key_value_info { const char *filename; int linenr; const char *origin_type; + enum config_scope scope; }; extern NORETURN void git_die_config(const char *key, const char *err, ...) __attribute__((format(printf, 2, 3))); diff --git a/config.c b/config.c index d555deeb9a..87629164b3 100644 --- a/config.c +++ b/config.c @@ -57,6 +57,15 @@ struct config_source { static struct config_source *cf; static struct key_value_info *current_config_kvi; +/* + * Similar to the variables above, this gives access to the "scope" of the + * current value (repo, global, etc). For cached values, it can be found via + * the current_config_kvi as above. During parsing, the current value can be + * found in this variable. It's not part of "cf" because it transcends a single + * file (i.e., a file included from .git/config is still in "repo" scope). + */ +static enum config_scope current_parsing_scope; + static int zlib_compression_seen; /* @@ -1229,22 +1238,27 @@ static int do_git_config_sequence(config_fn_t fn, void *data) char *user_config = expand_user_path("~/.gitconfig"); char *repo_config = git_pathdup("config"); + current_parsing_scope = CONFIG_SCOPE_SYSTEM; if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0)) ret += git_config_from_file(fn, git_etc_gitconfig(), data); + current_parsing_scope = CONFIG_SCOPE_GLOBAL; if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK)) ret += git_config_from_file(fn, xdg_config, data); if (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK)) ret += git_config_from_file(fn, user_config, data); + current_parsing_scope = CONFIG_SCOPE_REPO; if (repo_config && !access_or_die(repo_config, R_OK, 0)) ret += git_config_from_file(fn, repo_config, data); + current_parsing_scope = CONFIG_SCOPE_CMDLINE; if (git_config_from_parameters(fn, data) < 0) die(_("unable to parse command-line config")); + current_parsing_scope = CONFIG_SCOPE_UNKNOWN; free(xdg_config); free(user_config); free(repo_config); @@ -1383,6 +1397,7 @@ static int configset_add_value(struct config_set *cs, const char *key, const cha kv_info->linenr = -1; kv_info->origin_type = NULL; } + kv_info->scope = current_parsing_scope; si->util = kv_info; return 0; @@ -2482,3 +2497,11 @@ const char *current_config_name(void) die("BUG: current_config_name called outside config callback"); return name ? name : ""; } + +enum config_scope current_config_scope(void) +{ + if (current_config_kvi) + return current_config_kvi->scope; + else + return current_parsing_scope; +} diff --git a/t/helper/test-config.c b/t/helper/test-config.c index 3605ef8ee6..509aeef400 100644 --- a/t/helper/test-config.c +++ b/t/helper/test-config.c @@ -35,6 +35,21 @@ * */ +static const char *scope_name(enum config_scope scope) +{ + switch (scope) { + case CONFIG_SCOPE_SYSTEM: + return "system"; + case CONFIG_SCOPE_GLOBAL: + return "global"; + case CONFIG_SCOPE_REPO: + return "repo"; + case CONFIG_SCOPE_CMDLINE: + return "cmdline"; + default: + return "unknown"; + } +} static int iterate_cb(const char *var, const char *value, void *data) { static int nr; @@ -46,6 +61,7 @@ static int iterate_cb(const char *var, const char *value, void *data) printf("value=%s\n", value ? value : "(null)"); printf("origin=%s\n", current_config_origin_type()); printf("name=%s\n", current_config_name()); + printf("scope=%s\n", scope_name(current_config_scope())); return 0; } diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh index d345a885a3..065d5ebb09 100755 --- a/t/t1308-config-set.sh +++ b/t/t1308-config-set.sh @@ -238,16 +238,19 @@ test_expect_success 'iteration shows correct origins' ' value=from-home origin=file name=$(pwd)/.gitconfig + scope=global key=foo.bar value=from-repo origin=file name=.git/config + scope=repo key=foo.bar value=from-cmdline origin=command line name= + scope=cmdline EOF GIT_CONFIG_PARAMETERS=$cmdline_config test-config iterate >actual && test_cmp expect actual From 58461bdf15a66f428f5ca6042cafbcc64c82c64d Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 2 Jun 2016 15:06:55 -0700 Subject: [PATCH 6/7] t1308: do not get fooled by symbolic links to the source tree When your $PWD does not match $(/bin/pwd), e.g. you have your copy of the git source tree in one place, point it with a symbolic link, and then "cd" to that symbolic link before running 'make test', one of the tests in t1308 expects that the per-user configuration was reported to have been read from the true path (i.e. relative to the target of such a symbolic link), but the test-config program reports a path relative to $PWD (i.e. the symbolic link). Instead, expect a path relative to $HOME (aka $TRASH_DIRECTORY), as per-user configuration is read from $HOME/.gitconfig and the test framework sets these shell variables up in such a way to avoid this problem. Signed-off-by: Junio C Hamano --- t/t1308-config-set.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh index 065d5ebb09..cf716b469f 100755 --- a/t/t1308-config-set.sh +++ b/t/t1308-config-set.sh @@ -237,7 +237,7 @@ test_expect_success 'iteration shows correct origins' ' key=foo.bar value=from-home origin=file - name=$(pwd)/.gitconfig + name=$HOME/.gitconfig scope=global key=foo.bar From 20b20a22f8f7c1420e259c97ef790cb93091f475 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 18 May 2016 18:45:37 -0400 Subject: [PATCH 7/7] upload-pack: provide a hook for running pack-objects When upload-pack serves a client request, it turns to pack-objects to do the heavy lifting of creating a packfile. There's no easy way to intercept the call to pack-objects, but there are a few good reasons to want to do so: 1. If you're debugging a client or server issue with fetching, you may want to store a copy of the generated packfile. 2. If you're gathering data from real-world fetches for performance analysis or debugging, storing a copy of the arguments and stdin lets you replay the pack generation at your leisure. 3. You may want to insert a caching layer around pack-objects; it is the most CPU- and memory-intensive part of serving a fetch, and its output is a pure function[1] of its input, making it an ideal place to consolidate identical requests. This patch adds a simple "hook" interface to intercept calls to pack-objects. The new test demonstrates how it can be used for debugging (using it for caching is a straightforward extension; the tricky part is writing the actual caching layer). This hook is unlike the normal hook scripts found in the "hooks/" directory of a repository. Because we promise that upload-pack is safe to run in an untrusted repository, we cannot execute arbitrary code or commands found in the repository (neither in hooks/, nor in the config). So instead, this hook is triggered from a config variable that is explicitly ignored in the per-repo config. The config variable holds the actual shell command to run as the hook. Another approach would be to simply treat it as a boolean: "should I respect the upload-pack hooks in this repo?", and then run the script from "hooks/" as we usually do. However, that isn't as flexible; there's no way to run a hook approved by the site administrator (e.g., in "/etc/gitconfig") on a repository whose contents are not trusted. The approach taken by this patch is more fine-grained, if a little less conventional for git hooks (it does behave similar to other configured commands like diff.external, etc). [1] Pack-objects isn't _actually_ a pure function. Its output depends on the exact packing of the object database, and if multi-threading is used for delta compression, can even differ racily. But for the purposes of caching, that's OK; of the many possible outputs for a given input, it is sufficient only that we output one of them. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- Documentation/config.txt | 15 +++++++++ t/t5544-pack-objects-hook.sh | 62 ++++++++++++++++++++++++++++++++++++ upload-pack.c | 13 +++++++- 3 files changed, 89 insertions(+), 1 deletion(-) create mode 100755 t/t5544-pack-objects-hook.sh diff --git a/Documentation/config.txt b/Documentation/config.txt index 53f00dbc26..a7abcbeff3 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -2880,6 +2880,21 @@ uploadpack.keepAlive:: `uploadpack.keepAlive` seconds. Setting this option to 0 disables keepalive packets entirely. The default is 5 seconds. +uploadpack.packObjectsHook:: + If this option is set, when `upload-pack` would run + `git pack-objects` to create a packfile for a client, it will + run this shell command instead. The `pack-objects` command and + arguments it _would_ have run (including the `git pack-objects` + at the beginning) are appended to the shell command. The stdin + and stdout of the hook are treated as if `pack-objects` itself + was run. I.e., `upload-pack` will feed input intended for + `pack-objects` to the hook, and expects a completed packfile on + stdout. ++ +Note that this configuration variable is ignored if it is seen in the +repository-level config (this is a safety measure against fetching from +untrusted repositories). + url..insteadOf:: Any URL that starts with this value will be rewritten to start, instead, with . In cases where some site serves a diff --git a/t/t5544-pack-objects-hook.sh b/t/t5544-pack-objects-hook.sh new file mode 100755 index 0000000000..4357af1525 --- /dev/null +++ b/t/t5544-pack-objects-hook.sh @@ -0,0 +1,62 @@ +#!/bin/sh + +test_description='test custom script in place of pack-objects' +. ./test-lib.sh + +test_expect_success 'create some history to fetch' ' + test_commit one && + test_commit two +' + +test_expect_success 'create debugging hook script' ' + write_script .git/hook <<-\EOF + echo >&2 "hook running" + echo "$*" >hook.args + cat >hook.stdin + "$@" hook.stdout + cat hook.stdout + EOF +' + +clear_hook_results () { + rm -rf .git/hook.* dst.git +} + +test_expect_success 'hook runs via global config' ' + clear_hook_results && + test_config_global uploadpack.packObjectsHook ./hook && + git clone --no-local . dst.git 2>stderr && + grep "hook running" stderr +' + +test_expect_success 'hook outputs are sane' ' + # check that we recorded a usable pack + git index-pack --stdin <.git/hook.stdout && + + # check that we recorded args and stdin. We do not check + # the full argument list or the exact pack contents, as it would make + # the test brittle. So just sanity check that we could replay + # the packing procedure. + grep "^git" .git/hook.args && + $(cat .git/hook.args) <.git/hook.stdin >replay +' + +test_expect_success 'hook runs from -c config' ' + clear_hook_results && + git clone --no-local \ + -u "git -c uploadpack.packObjectsHook=./hook upload-pack" \ + . dst.git 2>stderr && + grep "hook running" stderr +' + +test_expect_success 'hook does not run from repo config' ' + clear_hook_results && + test_config uploadpack.packObjectsHook "./hook" && + git clone --no-local . dst.git 2>stderr && + ! grep "hook running" stderr && + test_path_is_missing .git/hook.args && + test_path_is_missing .git/hook.stdin && + test_path_is_missing .git/hook.stdout +' + +test_done diff --git a/upload-pack.c b/upload-pack.c index f19444df7b..8979be6394 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -52,6 +52,7 @@ static int keepalive = 5; static int use_sideband; static int advertise_refs; static int stateless_rpc; +static const char *pack_objects_hook; static void reset_timeout(void) { @@ -93,6 +94,14 @@ static void create_pack_file(void) int i; FILE *pipe_fd; + if (!pack_objects_hook) + pack_objects.git_cmd = 1; + else { + argv_array_push(&pack_objects.args, pack_objects_hook); + argv_array_push(&pack_objects.args, "git"); + pack_objects.use_shell = 1; + } + if (shallow_nr) { argv_array_push(&pack_objects.args, "--shallow-file"); argv_array_push(&pack_objects.args, ""); @@ -115,7 +124,6 @@ static void create_pack_file(void) pack_objects.in = -1; pack_objects.out = -1; pack_objects.err = -1; - pack_objects.git_cmd = 1; if (start_command(&pack_objects)) die("git upload-pack: unable to fork git-pack-objects"); @@ -812,6 +820,9 @@ static int upload_pack_config(const char *var, const char *value, void *unused) keepalive = git_config_int(var, value); if (!keepalive) keepalive = -1; + } else if (current_config_scope() != CONFIG_SCOPE_REPO) { + if (!strcmp("uploadpack.packobjectshook", var)) + return git_config_string(&pack_objects_hook, var, value); } return parse_hide_refs_config(var, value, "uploadpack"); }