From c72ee44bf4bc7549ee8710037ae8adfae6d8efc2 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 18 May 2016 18:39:02 -0400 Subject: 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(-) (limited to 'config.c') 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. -- cgit v1.2.3 From a77d6db69b2cbd16e98763f33ceaa76ff5ca2c54 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 18 May 2016 18:39:49 -0400 Subject: 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(-) (limited to 'config.c') 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) -- cgit v1.2.3 From 3258258f51f45efeff5ce0e9f825f347a1404efa Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 18 May 2016 18:41:08 -0400 Subject: 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(-) (limited to 'config.c') 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 : ""; } -- cgit v1.2.3 From 0d44a2dacc84fb7dcb5d684800e976f3b3c76d00 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 26 May 2016 20:32:23 -0400 Subject: 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 --- config.c | 51 ++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 42 insertions(+), 9 deletions(-) (limited to 'config.c') 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 : ""; } -- cgit v1.2.3 From 9acc5911119ec0209877fbaa0a1e68aa714c191e Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 18 May 2016 18:44:23 -0400 Subject: 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 --- config.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) (limited to 'config.c') 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; +} -- cgit v1.2.3