From 3df8fd625fba33a6525f61c85de39afb746db9bd Mon Sep 17 00:00:00 2001 From: Tanay Abhra Date: Thu, 7 Aug 2014 04:59:14 -0700 Subject: add line number and file name info to `config_set` Store file name and line number for each key-value pair in the cache during parsing of the configuration files. Signed-off-by: Tanay Abhra Reviewed-by: Matthieu Moy Signed-off-by: Junio C Hamano --- cache.h | 5 +++++ 1 file changed, 5 insertions(+) (limited to 'cache.h') diff --git a/cache.h b/cache.h index 7292aefa2e..0b1bdfd896 100644 --- a/cache.h +++ b/cache.h @@ -1383,6 +1383,11 @@ extern int git_config_get_bool_or_int(const char *key, int *is_bool, int *dest); extern int git_config_get_maybe_bool(const char *key, int *dest); extern int git_config_get_pathname(const char *key, const char **dest); +struct key_value_info { + const char *filename; + int linenr; +}; + extern int committer_ident_sufficiently_given(void); extern int author_ident_sufficiently_given(void); -- cgit v1.3 From aace4385027b0366f43961a94c8ed95ac9b3bd53 Mon Sep 17 00:00:00 2001 From: Tanay Abhra Date: Thu, 7 Aug 2014 04:59:15 -0700 Subject: change `git_config()` return value to void Currently `git_config()` returns an integer signifying an error code. During rewrites of the function most of the code was shifted to `git_config_with_options()`. `git_config_with_options()` normally returns positive values if its `config_source` parameter is set as NULL, as most errors are fatal, and non-fatal potential errors are guarded by "if" statements that are entered only when no error is possible. Still a negative value can be returned in case of race condition between `access_or_die()` & `git_config_from_file()`. Also, all callers of `git_config()` ignore the return value except for one case in branch.c. Change `git_config()` return value to void and make it die if it receives a negative value from `git_config_with_options()`. Original-patch-by: Matthieu Moy Signed-off-by: Tanay Abhra Reviewed-by: Matthieu Moy Signed-off-by: Junio C Hamano --- branch.c | 5 +---- cache.h | 2 +- config.c | 16 ++++++++++++++-- 3 files changed, 16 insertions(+), 7 deletions(-) (limited to 'cache.h') diff --git a/branch.c b/branch.c index 46e8aa86df..735767dd4a 100644 --- a/branch.c +++ b/branch.c @@ -161,10 +161,7 @@ int read_branch_desc(struct strbuf *buf, const char *branch_name) strbuf_addf(&name, "branch.%s.description", branch_name); cb.config_name = name.buf; cb.value = NULL; - if (git_config(read_branch_desc_cb, &cb) < 0) { - strbuf_release(&name); - return -1; - } + git_config(read_branch_desc_cb, &cb); if (cb.value) strbuf_addstr(buf, cb.value); strbuf_release(&name); diff --git a/cache.h b/cache.h index 0b1bdfd896..f11ce41956 100644 --- a/cache.h +++ b/cache.h @@ -1294,7 +1294,7 @@ extern int git_config_from_buf(config_fn_t fn, const char *name, const char *buf, size_t len, void *data); extern void git_config_push_parameter(const char *text); extern int git_config_from_parameters(config_fn_t fn, void *data); -extern int git_config(config_fn_t fn, void *); +extern void git_config(config_fn_t fn, void *); extern int git_config_with_options(config_fn_t fn, void *, struct git_config_source *config_source, int respect_includes); diff --git a/config.c b/config.c index 1ac85edce4..cd59c4a0f2 100644 --- a/config.c +++ b/config.c @@ -1232,9 +1232,21 @@ int git_config_with_options(config_fn_t fn, void *data, return ret; } -int git_config(config_fn_t fn, void *data) +void git_config(config_fn_t fn, void *data) { - return git_config_with_options(fn, data, NULL, 1); + 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 + * non-fatal potential errors are guarded by "if" + * statements that are entered only when no error is + * possible. + * + * If we ever encounter a non-fatal error, it means + * something went really wrong and we should stop + * immediately. + */ + die(_("unknown error occured while reading the configuration files")); } static struct config_set_element *configset_find_element(struct config_set *cs, const char *key) -- cgit v1.3 From 5a80e97c827e9d73884dbe4119bf97f6dd84b237 Mon Sep 17 00:00:00 2001 From: Tanay Abhra Date: Thu, 7 Aug 2014 04:59:16 -0700 Subject: config: add `git_die_config()` to the config-set API Add `git_die_config` that dies printing the line number and the file name of the highest priority value for the configuration variable `key`. A custom error message is also printed before dying, specified by the caller, which can be skipped if `err` argument is set to NULL. It has usage in non-callback based config value retrieval where we can raise an error and die if there is a semantic error. For example, if (!git_config_get_value(key, &value)){ if (!strcmp(value, "foo")) git_config_die(key, "value: `%s` is illegal", value); else /* do work */ } Signed-off-by: Tanay Abhra Reviewed-by: Matthieu Moy Signed-off-by: Junio C Hamano --- Documentation/technical/api-config.txt | 13 ++++++++++++ cache.h | 3 +++ config.c | 39 ++++++++++++++++++++++++++++++++-- 3 files changed, 53 insertions(+), 2 deletions(-) (limited to 'cache.h') diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt index 815c1eecc3..7108888b8a 100644 --- a/Documentation/technical/api-config.txt +++ b/Documentation/technical/api-config.txt @@ -155,6 +155,19 @@ as well as retrieval for the queried variable, including: Similar to `git_config_get_string`, but expands `~` or `~user` into the user's home directory when found at the beginning of the path. +`git_die_config(const char *key, const char *err, ...)`:: + + First prints the error message specified by the caller in `err` and then + dies printing the line number and the file name of the highest priority + value for the configuration variable `key`. + +`void git_die_config_linenr(const char *key, const char *filename, int linenr)`:: + + Helper function which formats the die error message according to the + parameters entered. Used by `git_die_config()`. It can be used by callers + handling `git_config_get_value_multi()` to print the correct error message + for the desired value. + See test-config.c for usage examples. Value Parsing Helpers diff --git a/cache.h b/cache.h index f11ce41956..89a0d51515 100644 --- a/cache.h +++ b/cache.h @@ -1388,6 +1388,9 @@ struct key_value_info { int linenr; }; +extern NORETURN void git_die_config(const char *key, const char *err, ...) __attribute__((format(printf, 2, 3))); +extern NORETURN void git_die_config_linenr(const char *key, const char *filename, int linenr); + extern int committer_ident_sufficiently_given(void); extern int author_ident_sufficiently_given(void); diff --git a/config.c b/config.c index cd59c4a0f2..8449a8ac45 100644 --- a/config.c +++ b/config.c @@ -1471,8 +1471,12 @@ const struct string_list *git_config_get_value_multi(const char *key) int git_config_get_string_const(const char *key, const char **dest) { + int ret; git_config_check_init(); - return git_configset_get_string_const(&the_config_set, key, dest); + ret = git_configset_get_string_const(&the_config_set, key, dest); + if (ret < 0) + git_die_config(key, NULL); + return ret; } int git_config_get_string(const char *key, char **dest) @@ -1513,8 +1517,39 @@ int git_config_get_maybe_bool(const char *key, int *dest) int git_config_get_pathname(const char *key, const char **dest) { + int ret; git_config_check_init(); - return git_configset_get_pathname(&the_config_set, key, dest); + ret = git_configset_get_pathname(&the_config_set, key, dest); + if (ret < 0) + git_die_config(key, NULL); + return ret; +} + +NORETURN +void git_die_config_linenr(const char *key, const char *filename, int linenr) +{ + if (!filename) + die(_("unable to parse '%s' from command-line config"), key); + else + die(_("bad config variable '%s' in file '%s' at line %d"), + key, filename, linenr); +} + +NORETURN __attribute__((format(printf, 2, 3))) +void git_die_config(const char *key, const char *err, ...) +{ + const struct string_list *values; + struct key_value_info *kv_info; + + if (err) { + va_list params; + va_start(params, err); + vreportf("error: ", err, params); + va_end(params); + } + values = git_config_get_value_multi(key); + kv_info = values->items[values->nr - 1].util; + git_die_config_linenr(key, kv_info->filename, kv_info->linenr); } /* -- cgit v1.3 From 155ef25f12329258c06b8875b5e372abec2d348d Mon Sep 17 00:00:00 2001 From: Tanay Abhra Date: Thu, 7 Aug 2014 04:59:17 -0700 Subject: rewrite git_config() to use the config-set API Of all the functions in `git_config*()` family, `git_config()` has the most invocations in the whole code base. Each `git_config()` invocation causes config file rereads which can be avoided using the config-set API. Use the config-set API to rewrite `git_config()` to use the config caching layer to avoid config file rereads on each invocation during a git process lifetime. First invocation constructs the cache, and after that for each successive invocation, `git_config()` feeds values from the config cache instead of rereading the configuration files. Signed-off-by: Tanay Abhra Reviewed-by: Matthieu Moy Signed-off-by: Junio C Hamano --- cache.h | 24 +++++++++++++++++++++++ config.c | 51 +++++++++++++++++++++++++++++++++++++++++-------- t/t4055-diff-context.sh | 2 +- 3 files changed, 68 insertions(+), 9 deletions(-) (limited to 'cache.h') diff --git a/cache.h b/cache.h index 89a0d51515..2693a3736a 100644 --- a/cache.h +++ b/cache.h @@ -8,6 +8,7 @@ #include "gettext.h" #include "convert.h" #include "trace.h" +#include "string-list.h" #include SHA1_HEADER #ifndef git_SHA_CTX @@ -1351,9 +1352,32 @@ extern int parse_config_key(const char *var, const char **subsection, int *subsection_len, const char **key); +struct config_set_element { + struct hashmap_entry ent; + char *key; + struct string_list value_list; +}; + +struct configset_list_item { + struct config_set_element *e; + int value_index; +}; + +/* + * the contents of the list are ordered according to their + * position in the config files and order of parsing the files. + * (i.e. key-value pair at the last position of .git/config will + * be at the last item of the list) + */ +struct configset_list { + struct configset_list_item *items; + unsigned int nr, alloc; +}; + struct config_set { struct hashmap config_hash; int hash_initialized; + struct configset_list list; }; extern void git_configset_init(struct config_set *cs); diff --git a/config.c b/config.c index 8449a8ac45..387e86bb6c 100644 --- a/config.c +++ b/config.c @@ -35,12 +35,6 @@ struct config_source { long (*do_ftell)(struct config_source *c); }; -struct config_set_element { - struct hashmap_entry ent; - char *key; - struct string_list value_list; -}; - static struct config_source *cf; static int zlib_compression_seen; @@ -1232,7 +1226,7 @@ int git_config_with_options(config_fn_t fn, void *data, return ret; } -void git_config(config_fn_t fn, void *data) +static void git_config_raw(config_fn_t fn, void *data) { if (git_config_with_options(fn, data, NULL, 1) < 0) /* @@ -1249,6 +1243,33 @@ void git_config(config_fn_t fn, void *data) die(_("unknown error occured while reading the configuration files")); } +static void configset_iter(struct config_set *cs, config_fn_t fn, void *data) +{ + int i, value_index; + 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); + } + } +} + +static void git_config_check_init(void); + +void git_config(config_fn_t fn, void *data) +{ + git_config_check_init(); + configset_iter(&the_config_set, fn, data); +} + static struct config_set_element *configset_find_element(struct config_set *cs, const char *key) { struct config_set_element k; @@ -1275,6 +1296,7 @@ static int configset_add_value(struct config_set *cs, const char *key, const cha { struct config_set_element *e; struct string_list_item *si; + struct configset_list_item *l_item; struct key_value_info *kv_info = xmalloc(sizeof(*kv_info)); e = configset_find_element(cs, key); @@ -1290,6 +1312,12 @@ static int configset_add_value(struct config_set *cs, const char *key, const cha hashmap_add(&cs->config_hash, e); } si = string_list_append_nodup(&e->value_list, value ? xstrdup(value) : NULL); + + ALLOC_GROW(cs->list.items, cs->list.nr + 1, cs->list.alloc); + l_item = &cs->list.items[cs->list.nr++]; + l_item->e = e; + l_item->value_index = e->value_list.nr - 1; + if (cf) { kv_info->filename = strintern(cf->name); kv_info->linenr = cf->linenr; @@ -1313,6 +1341,9 @@ void git_configset_init(struct config_set *cs) { hashmap_init(&cs->config_hash, (hashmap_cmp_fn)config_set_element_cmp, 0); cs->hash_initialized = 1; + cs->list.nr = 0; + cs->list.alloc = 0; + cs->list.items = NULL; } void git_configset_clear(struct config_set *cs) @@ -1329,6 +1360,10 @@ void git_configset_clear(struct config_set *cs) } hashmap_free(&cs->config_hash, 1); cs->hash_initialized = 0; + free(cs->list.items); + cs->list.nr = 0; + cs->list.alloc = 0; + cs->list.items = NULL; } static int config_set_callback(const char *key, const char *value, void *cb) @@ -1447,7 +1482,7 @@ static void git_config_check_init(void) if (the_config_set.hash_initialized) return; git_configset_init(&the_config_set); - git_config(config_set_callback, &the_config_set); + git_config_raw(config_set_callback, &the_config_set); } void git_config_clear(void) diff --git a/t/t4055-diff-context.sh b/t/t4055-diff-context.sh index cd0454356a..741e0803c1 100755 --- a/t/t4055-diff-context.sh +++ b/t/t4055-diff-context.sh @@ -79,7 +79,7 @@ test_expect_success 'non-integer config parsing' ' test_expect_success 'negative integer config parsing' ' git config diff.context -1 && test_must_fail git diff 2>output && - test_i18ngrep "bad config file" output + test_i18ngrep "bad config variable" output ' test_expect_success '-U0 is valid, so is diff.context=0' ' -- cgit v1.3