From 1b86bbb0ade4641c2da0dc74ebca1c09ec772bb4 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 23 Jan 2013 01:23:05 -0500 Subject: [PATCH 1/8] config: add helper function for parsing key names The config callback functions get keys of the general form: section.subsection.key (where the subsection may be contain arbitrary data, or may be missing). For matching keys without subsections, it is simple enough to call "strcmp". Matching keys with subsections is a little more complicated, and each callback does it in an ad-hoc way, usually involving error-prone pointer arithmetic. Let's provide a helper that keeps the pointer arithmetic all in one place. Signed-off-by: Jeff King Reviewed-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- cache.h | 15 +++++++++++++++ config.c | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+) diff --git a/cache.h b/cache.h index c257953fa7..b19305bca2 100644 --- a/cache.h +++ b/cache.h @@ -1164,6 +1164,21 @@ struct config_include_data { #define CONFIG_INCLUDE_INIT { 0 } extern int git_config_include(const char *name, const char *value, void *data); +/* + * Match and parse a config key of the form: + * + * section.(subsection.)?key + * + * (i.e., what gets handed to a config_fn_t). The caller provides the section; + * we return -1 if it does not match, 0 otherwise. The subsection and key + * out-parameters are filled by the function (and subsection is NULL if it is + * missing). + */ +extern int parse_config_key(const char *var, + const char *section, + const char **subsection, int *subsection_len, + const char **key); + extern int committer_ident_sufficiently_given(void); extern int author_ident_sufficiently_given(void); diff --git a/config.c b/config.c index 7b444b68ab..11bd4d8058 100644 --- a/config.c +++ b/config.c @@ -1667,3 +1667,36 @@ int config_error_nonbool(const char *var) { return error("Missing value for '%s'", var); } + +int parse_config_key(const char *var, + const char *section, + const char **subsection, int *subsection_len, + const char **key) +{ + int section_len = strlen(section); + const char *dot; + + /* Does it start with "section." ? */ + if (prefixcmp(var, section) || var[section_len] != '.') + return -1; + + /* + * Find the key; we don't know yet if we have a subsection, but we must + * parse backwards from the end, since the subsection may have dots in + * it, too. + */ + dot = strrchr(var, '.'); + *key = dot + 1; + + /* Did we have a subsection at all? */ + if (dot == var + section_len) { + *subsection = NULL; + *subsection_len = 0; + } + else { + *subsection = var + section_len + 1; + *subsection_len = dot - *subsection; + } + + return 0; +} From 785a04298177e155dc7391e7234945b38b624e34 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 23 Jan 2013 01:23:27 -0500 Subject: [PATCH 2/8] archive-tar: use parse_config_key when parsing config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is fewer lines of code, but more importantly, fixes a bogus pointer offset. We are looking for "tar." in the section, but later assume that the dot we found is at offset 9, not 3. This is a holdover from an earlier iteration of 767cf45 which called the section "tarfilter". As a result, we could erroneously reject some filters with dots in their name, as well as read uninitialized memory. Reported by (and test by) René Scharfe. Signed-off-by: Jeff King Reviewed-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- archive-tar.c | 10 +--------- t/t5000-tar-tree.sh | 3 ++- 2 files changed, 3 insertions(+), 10 deletions(-) diff --git a/archive-tar.c b/archive-tar.c index d1cce46e33..719b6298e6 100644 --- a/archive-tar.c +++ b/archive-tar.c @@ -327,20 +327,12 @@ static struct archiver *find_tar_filter(const char *name, int len) static int tar_filter_config(const char *var, const char *value, void *data) { struct archiver *ar; - const char *dot; const char *name; const char *type; int namelen; - if (prefixcmp(var, "tar.")) + if (parse_config_key(var, "tar", &name, &namelen, &type) < 0 || !name) return 0; - dot = strrchr(var, '.'); - if (dot == var + 9) - return 0; - - name = var + 4; - namelen = dot - name; - type = dot + 1; ar = find_tar_filter(name, namelen); if (!ar) { diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh index e7c240fc1f..3fbd366ec3 100755 --- a/t/t5000-tar-tree.sh +++ b/t/t5000-tar-tree.sh @@ -212,7 +212,8 @@ test_expect_success 'git-archive --prefix=olde-' ' test_expect_success 'setup tar filters' ' git config tar.tar.foo.command "tr ab ba" && git config tar.bar.command "tr ab ba" && - git config tar.bar.remote true + git config tar.bar.remote true && + git config tar.invalid baz ' test_expect_success 'archive --list mentions user filter' ' From d731f0ade129a71237eff5a17f3196002cb439fb Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 23 Jan 2013 01:24:23 -0500 Subject: [PATCH 3/8] convert some config callbacks to parse_config_key These callers can drop some inline pointer arithmetic and magic offset constants, making them more readable and less error-prone (those constants had to match the lengths of strings, but there is no automatic verification of that fact). The "ep" pointer (presumably for "end pointer"), which points to the final key segment of the config variable, is given the more standard name "key" to describe its function rather than its derivation. Signed-off-by: Jeff King Reviewed-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- convert.c | 14 +++++--------- ll-merge.c | 14 +++++--------- userdiff.c | 13 +++---------- 3 files changed, 13 insertions(+), 28 deletions(-) diff --git a/convert.c b/convert.c index 66021550c3..3520252d3a 100644 --- a/convert.c +++ b/convert.c @@ -457,7 +457,7 @@ static struct convert_driver { static int read_convert_config(const char *var, const char *value, void *cb) { - const char *ep, *name; + const char *key, *name; int namelen; struct convert_driver *drv; @@ -465,10 +465,8 @@ static int read_convert_config(const char *var, const char *value, void *cb) * External conversion drivers are configured using * "filter..variable". */ - if (prefixcmp(var, "filter.") || (ep = strrchr(var, '.')) == var + 6) + if (parse_config_key(var, "filter", &name, &namelen, &key) < 0 || !name) return 0; - name = var + 7; - namelen = ep - name; for (drv = user_convert; drv; drv = drv->next) if (!strncmp(drv->name, name, namelen) && !drv->name[namelen]) break; @@ -479,8 +477,6 @@ static int read_convert_config(const char *var, const char *value, void *cb) user_convert_tail = &(drv->next); } - ep++; - /* * filter..smudge and filter..clean specifies * the command line: @@ -490,13 +486,13 @@ static int read_convert_config(const char *var, const char *value, void *cb) * The command-line will not be interpolated in any way. */ - if (!strcmp("smudge", ep)) + if (!strcmp("smudge", key)) return git_config_string(&drv->smudge, var, value); - if (!strcmp("clean", ep)) + if (!strcmp("clean", key)) return git_config_string(&drv->clean, var, value); - if (!strcmp("required", ep)) { + if (!strcmp("required", key)) { drv->required = git_config_bool(var, value); return 0; } diff --git a/ll-merge.c b/ll-merge.c index acea33bf1b..fb61ea66a1 100644 --- a/ll-merge.c +++ b/ll-merge.c @@ -222,7 +222,7 @@ static const char *default_ll_merge; static int read_merge_config(const char *var, const char *value, void *cb) { struct ll_merge_driver *fn; - const char *ep, *name; + const char *key, *name; int namelen; if (!strcmp(var, "merge.default")) { @@ -236,15 +236,13 @@ static int read_merge_config(const char *var, const char *value, void *cb) * especially, we do not want to look at variables such as * "merge.summary", "merge.tool", and "merge.verbosity". */ - if (prefixcmp(var, "merge.") || (ep = strrchr(var, '.')) == var + 5) + if (parse_config_key(var, "merge", &name, &namelen, &key) < 0 || !name) return 0; /* * Find existing one as we might be processing merge..var2 * after seeing merge..var1. */ - name = var + 6; - namelen = ep - name; for (fn = ll_user_merge; fn; fn = fn->next) if (!strncmp(fn->name, name, namelen) && !fn->name[namelen]) break; @@ -256,16 +254,14 @@ static int read_merge_config(const char *var, const char *value, void *cb) ll_user_merge_tail = &(fn->next); } - ep++; - - if (!strcmp("name", ep)) { + if (!strcmp("name", key)) { if (!value) return error("%s: lacks value", var); fn->description = xstrdup(value); return 0; } - if (!strcmp("driver", ep)) { + if (!strcmp("driver", key)) { if (!value) return error("%s: lacks value", var); /* @@ -289,7 +285,7 @@ static int read_merge_config(const char *var, const char *value, void *cb) return 0; } - if (!strcmp("recursive", ep)) { + if (!strcmp("recursive", key)) { if (!value) return error("%s: lacks value", var); fn->recursive = xstrdup(value); diff --git a/userdiff.c b/userdiff.c index ed958ef6b8..a4ea1e97b7 100644 --- a/userdiff.c +++ b/userdiff.c @@ -188,20 +188,13 @@ static struct userdiff_driver *parse_driver(const char *var, const char *value, const char *type) { struct userdiff_driver *drv; - const char *dot; - const char *name; + const char *name, *key; int namelen; - if (prefixcmp(var, "diff.")) - return NULL; - dot = strrchr(var, '.'); - if (dot == var + 4) - return NULL; - if (strcmp(type, dot+1)) + if (parse_config_key(var, "diff", &name, &namelen, &key) < 0 || + !name || strcmp(type, key)) return NULL; - name = var + 5; - namelen = dot - name; drv = userdiff_find_by_namelen(name, namelen); if (!drv) { ALLOC_GROW(drivers, ndrivers+1, drivers_alloc); From 0a5987fe5e0cea23245e0beab6eb47131864b276 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 23 Jan 2013 01:25:07 -0500 Subject: [PATCH 4/8] userdiff: drop parse_driver function When we parse userdiff config, we generally assume that diff.name.key will affect the "key" value of the "name" driver. However, without confirming that the key is a valid userdiff key, we may accidentally conflict with the ancient "diff.color.*" namespace. The current code is careful not to even create a driver struct if we do not see a key that is known by the diff-driver code. However, this carefulness is unnecessary; the default driver with no keys set behaves exactly the same as having no driver at all. We can simply set up the driver struct as soon as we see we have a config key that looks like a driver. This makes the code a bit more readable. Signed-off-by: Jeff King Reviewed-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- userdiff.c | 50 +++++++++++++++++++++----------------------------- 1 file changed, 21 insertions(+), 29 deletions(-) diff --git a/userdiff.c b/userdiff.c index a4ea1e97b7..ea43a0306f 100644 --- a/userdiff.c +++ b/userdiff.c @@ -184,28 +184,6 @@ static struct userdiff_driver *userdiff_find_by_namelen(const char *k, int len) return NULL; } -static struct userdiff_driver *parse_driver(const char *var, - const char *value, const char *type) -{ - struct userdiff_driver *drv; - const char *name, *key; - int namelen; - - if (parse_config_key(var, "diff", &name, &namelen, &key) < 0 || - !name || strcmp(type, key)) - return NULL; - - drv = userdiff_find_by_namelen(name, namelen); - if (!drv) { - ALLOC_GROW(drivers, ndrivers+1, drivers_alloc); - drv = &drivers[ndrivers++]; - memset(drv, 0, sizeof(*drv)); - drv->name = xmemdupz(name, namelen); - drv->binary = -1; - } - return drv; -} - static int parse_funcname(struct userdiff_funcname *f, const char *k, const char *v, int cflags) { @@ -233,20 +211,34 @@ static int parse_bool(int *b, const char *k, const char *v) int userdiff_config(const char *k, const char *v) { struct userdiff_driver *drv; + const char *name, *type; + int namelen; - if ((drv = parse_driver(k, v, "funcname"))) + if (parse_config_key(k, "diff", &name, &namelen, &type) || !name) + return 0; + + drv = userdiff_find_by_namelen(name, namelen); + if (!drv) { + ALLOC_GROW(drivers, ndrivers+1, drivers_alloc); + drv = &drivers[ndrivers++]; + memset(drv, 0, sizeof(*drv)); + drv->name = xmemdupz(name, namelen); + drv->binary = -1; + } + + if (!strcmp(type, "funcname")) return parse_funcname(&drv->funcname, k, v, 0); - if ((drv = parse_driver(k, v, "xfuncname"))) + if (!strcmp(type, "xfuncname")) return parse_funcname(&drv->funcname, k, v, REG_EXTENDED); - if ((drv = parse_driver(k, v, "binary"))) + if (!strcmp(type, "binary")) return parse_tristate(&drv->binary, k, v); - if ((drv = parse_driver(k, v, "command"))) + if (!strcmp(type, "command")) return git_config_string(&drv->external, k, v); - if ((drv = parse_driver(k, v, "textconv"))) + if (!strcmp(type, "textconv")) return git_config_string(&drv->textconv, k, v); - if ((drv = parse_driver(k, v, "cachetextconv"))) + if (!strcmp(type, "cachetextconv")) return parse_bool(&drv->textconv_want_cache, k, v); - if ((drv = parse_driver(k, v, "wordregex"))) + if (!strcmp(type, "wordregex")) return git_config_string(&drv->word_regex, k, v); return 0; From 9edbb8b1c1fdb199b47650f50fc432b1bfcb9039 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 23 Jan 2013 01:25:22 -0500 Subject: [PATCH 5/8] submodule: use parse_config_key when parsing config This makes the code a lot simpler to read by dropping a whole bunch of constant offsets. As a bonus, it means we also feed the whole config variable name to our error functions: [before] $ git -c submodule.foo.fetchrecursesubmodules=bogus checkout fatal: bad foo.fetchrecursesubmodules argument: bogus [after] $ git -c submodule.foo.fetchrecursesubmodules=bogus checkout fatal: bad submodule.foo.fetchrecursesubmodules argument: bogus Signed-off-by: Jeff King Reviewed-by: Jonathan Nieder Acked-by: Jens Lehmann Signed-off-by: Junio C Hamano --- submodule.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/submodule.c b/submodule.c index 2f55436234..25413deb1f 100644 --- a/submodule.c +++ b/submodule.c @@ -126,15 +126,16 @@ void gitmodules_config(void) int parse_submodule_config_option(const char *var, const char *value) { - int len; struct string_list_item *config; struct strbuf submodname = STRBUF_INIT; + const char *name, *key; + int namelen; - var += 10; /* Skip "submodule." */ + if (parse_config_key(var, "submodule", &name, &namelen, &key) < 0 || !name) + return 0; - len = strlen(var); - if ((len > 5) && !strcmp(var + len - 5, ".path")) { - strbuf_add(&submodname, var, len - 5); + if (!strcmp(key, "path")) { + strbuf_add(&submodname, name, namelen); config = unsorted_string_list_lookup(&config_name_for_path, value); if (config) free(config->util); @@ -142,22 +143,22 @@ int parse_submodule_config_option(const char *var, const char *value) config = string_list_append(&config_name_for_path, xstrdup(value)); config->util = strbuf_detach(&submodname, NULL); strbuf_release(&submodname); - } else if ((len > 23) && !strcmp(var + len - 23, ".fetchrecursesubmodules")) { - strbuf_add(&submodname, var, len - 23); + } else if (!strcmp(key, "fetchrecursesubmodules")) { + strbuf_add(&submodname, name, namelen); config = unsorted_string_list_lookup(&config_fetch_recurse_submodules_for_name, submodname.buf); if (!config) config = string_list_append(&config_fetch_recurse_submodules_for_name, strbuf_detach(&submodname, NULL)); config->util = (void *)(intptr_t)parse_fetch_recurse_submodules_arg(var, value); strbuf_release(&submodname); - } else if ((len > 7) && !strcmp(var + len - 7, ".ignore")) { + } else if (!strcmp(key, "ignore")) { if (strcmp(value, "untracked") && strcmp(value, "dirty") && strcmp(value, "all") && strcmp(value, "none")) { warning("Invalid parameter \"%s\" for config option \"submodule.%s.ignore\"", value, var); return 0; } - strbuf_add(&submodname, var, len - 7); + strbuf_add(&submodname, name, namelen); config = unsorted_string_list_lookup(&config_ignore_for_name, submodname.buf); if (config) free(config->util); From 6bfe19ee168cd47295e9d25b4343ec318fab3790 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 23 Jan 2013 01:26:42 -0500 Subject: [PATCH 6/8] submodule: simplify memory handling in config parsing We keep a strbuf for the name of the submodule, even though we only ever add one string to it. Let's just use xmemdupz instead, which is slightly more efficient and makes it easier to follow what is going on. Unfortunately, we still end up having to deal with some memory ownership issues in some code branches, as we have to allocate the string in order to do a string list lookup, and then only sometimes want to hand ownership of that string over to the string_list. Still, making that explicit in the code (as opposed to sometimes detaching the strbuf, and then always releasing it) makes it a little more obvious what is going on. Signed-off-by: Jeff King Reviewed-by: Jonathan Nieder Acked-by: Jens Lehmann Signed-off-by: Junio C Hamano --- submodule.c | 30 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/submodule.c b/submodule.c index 25413deb1f..9ba1496543 100644 --- a/submodule.c +++ b/submodule.c @@ -127,7 +127,6 @@ void gitmodules_config(void) int parse_submodule_config_option(const char *var, const char *value) { struct string_list_item *config; - struct strbuf submodname = STRBUF_INIT; const char *name, *key; int namelen; @@ -135,37 +134,36 @@ int parse_submodule_config_option(const char *var, const char *value) return 0; if (!strcmp(key, "path")) { - strbuf_add(&submodname, name, namelen); config = unsorted_string_list_lookup(&config_name_for_path, value); if (config) free(config->util); else config = string_list_append(&config_name_for_path, xstrdup(value)); - config->util = strbuf_detach(&submodname, NULL); - strbuf_release(&submodname); + config->util = xmemdupz(name, namelen); } else if (!strcmp(key, "fetchrecursesubmodules")) { - strbuf_add(&submodname, name, namelen); - config = unsorted_string_list_lookup(&config_fetch_recurse_submodules_for_name, submodname.buf); + char *name_cstr = xmemdupz(name, namelen); + config = unsorted_string_list_lookup(&config_fetch_recurse_submodules_for_name, name_cstr); if (!config) - config = string_list_append(&config_fetch_recurse_submodules_for_name, - strbuf_detach(&submodname, NULL)); + config = string_list_append(&config_fetch_recurse_submodules_for_name, name_cstr); + else + free(name_cstr); config->util = (void *)(intptr_t)parse_fetch_recurse_submodules_arg(var, value); - strbuf_release(&submodname); } else if (!strcmp(key, "ignore")) { + char *name_cstr; + if (strcmp(value, "untracked") && strcmp(value, "dirty") && strcmp(value, "all") && strcmp(value, "none")) { warning("Invalid parameter \"%s\" for config option \"submodule.%s.ignore\"", value, var); return 0; } - strbuf_add(&submodname, name, namelen); - config = unsorted_string_list_lookup(&config_ignore_for_name, submodname.buf); - if (config) + name_cstr = xmemdupz(name, namelen); + config = unsorted_string_list_lookup(&config_ignore_for_name, name_cstr); + if (config) { free(config->util); - else - config = string_list_append(&config_ignore_for_name, - strbuf_detach(&submodname, NULL)); - strbuf_release(&submodname); + free(name_cstr); + } else + config = string_list_append(&config_ignore_for_name, name_cstr); config->util = xstrdup(value); return 0; } From 4d5c6cefd5c3e37fdf096c955abe9f9ac5938a53 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 23 Jan 2013 01:27:09 -0500 Subject: [PATCH 7/8] help: use parse_config_key for man config The resulting code ends up about the same length, but it is a little more self-explanatory. It now explicitly documents and checks the pre-condition that the incoming var starts with "man.", and drops the magic offset "4". Signed-off-by: Jeff King Reviewed-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- builtin/help.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/builtin/help.c b/builtin/help.c index bd86253d83..04cb77d67c 100644 --- a/builtin/help.c +++ b/builtin/help.c @@ -237,21 +237,21 @@ static int add_man_viewer_cmd(const char *name, static int add_man_viewer_info(const char *var, const char *value) { - const char *name = var + 4; - const char *subkey = strrchr(name, '.'); + const char *name, *subkey; + int namelen; - if (!subkey) + if (parse_config_key(var, "man", &name, &namelen, &subkey) < 0 || !name) return 0; - if (!strcmp(subkey, ".path")) { + if (!strcmp(subkey, "path")) { if (!value) return config_error_nonbool(var); - return add_man_viewer_path(name, subkey - name, value); + return add_man_viewer_path(name, namelen, value); } - if (!strcmp(subkey, ".cmd")) { + if (!strcmp(subkey, "cmd")) { if (!value) return config_error_nonbool(var); - return add_man_viewer_cmd(name, subkey - name, value); + return add_man_viewer_cmd(name, namelen, value); } return 0; From b3873c336cc2eb5a6eb2b10981a2ca0b65b8c987 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 23 Jan 2013 01:27:37 -0500 Subject: [PATCH 8/8] reflog: use parse_config_key in config callback This doesn't save any lines, but does keep us from doing error-prone pointer arithmetic with constants. Signed-off-by: Jeff King Reviewed-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- builtin/reflog.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/builtin/reflog.c b/builtin/reflog.c index b3c9e27bde..1fedf66329 100644 --- a/builtin/reflog.c +++ b/builtin/reflog.c @@ -510,26 +510,27 @@ static int parse_expire_cfg_value(const char *var, const char *value, unsigned l static int reflog_expire_config(const char *var, const char *value, void *cb) { - const char *lastdot = strrchr(var, '.'); + const char *pattern, *key; + int pattern_len; unsigned long expire; int slot; struct reflog_expire_cfg *ent; - if (!lastdot || prefixcmp(var, "gc.")) + if (parse_config_key(var, "gc", &pattern, &pattern_len, &key) < 0) return git_default_config(var, value, cb); - if (!strcmp(lastdot, ".reflogexpire")) { + if (!strcmp(key, "reflogexpire")) { slot = EXPIRE_TOTAL; if (parse_expire_cfg_value(var, value, &expire)) return -1; - } else if (!strcmp(lastdot, ".reflogexpireunreachable")) { + } else if (!strcmp(key, "reflogexpireunreachable")) { slot = EXPIRE_UNREACH; if (parse_expire_cfg_value(var, value, &expire)) return -1; } else return git_default_config(var, value, cb); - if (lastdot == var + 2) { + if (!pattern) { switch (slot) { case EXPIRE_TOTAL: default_reflog_expire = expire; @@ -541,7 +542,7 @@ static int reflog_expire_config(const char *var, const char *value, void *cb) return 0; } - ent = find_cfg_ent(var + 3, lastdot - (var+3)); + ent = find_cfg_ent(pattern, pattern_len); if (!ent) return -1; switch (slot) {