Merge branch 'jk/config-parsing-cleanup'

Configuration parsing for tar.* configuration variables were
broken. Introduce a new config-keyname parser API to make the
callers much less error prone.

* jk/config-parsing-cleanup:
reflog: use parse_config_key in config callback
help: use parse_config_key for man config
submodule: simplify memory handling in config parsing
submodule: use parse_config_key when parsing config
userdiff: drop parse_driver function
convert some config callbacks to parse_config_key
archive-tar: use parse_config_key when parsing config
config: add helper function for parsing key names

diff --git a/archive-tar.c b/archive-tar.c
index d1cce46e3310d64af32b231de697449ae90ddccf..719b6298e6abf9c9e9e8009ec49dfc76d0d9e49b 100644 (file)
--- 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/builtin/help.c b/builtin/help.c
index 6067a6134b58f2464b243f47112226a0a013f5c4..d1d71816a9df67721578bc29665c15887575caec 100644 (file)
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -236,21 +236,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;

diff --git a/builtin/reflog.c b/builtin/reflog.c
index b3c9e27bde653bf01acc6126deeb5f508fa0b26e..1fedf66329d73f192d80d3d42789c290a839d7b0 100644 (file)
--- 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) {

diff --git a/cache.h b/cache.h
index 94ffcda8f6cbb603b79eda1ae441a21450a16cb5..9038fb500e45e926a696598b91a3759c22e3b306 100644 (file)
--- a/cache.h
+++ b/cache.h
@@ -1170,6 +1170,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 5a20de3fa62b46d73cc0b13bf5d50aefac78fac6..aefd80b12a079d4a3c91d43c8a2c33ed6fbd0a38 100644 (file)
--- a/config.c
+++ b/config.c
@@ -1681,3 +1681,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;
+}

diff --git a/convert.c b/convert.c
index 66021550c32f86e662fe5da84c852e80ae790450..3520252d3abaf49d4b7682ad083da8b7ae6be4c6 100644 (file)
--- 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.<name>.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.<name>.smudge and filter.<name>.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 acea33bf1babfe541c319081f14625ac779bb582..fb61ea66a13eba3a8e91a05dbe1c37de98cec853 100644 (file)
--- 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.<name>.var2
         * after seeing merge.<name>.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/submodule.c b/submodule.c
index 2f554362347674df1a5ee9904d83a0de530304a6..9ba149654322840323cf2d8f4980fa09e56f4068 100644 (file)
--- a/submodule.c
+++ b/submodule.c
@@ -126,45 +126,44 @@ 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")) {
                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);
-       } else if ((len > 23) && !strcmp(var + len - 23, ".fetchrecursesubmodules")) {
-               strbuf_add(&submodname, var, len - 23);
-               config = unsorted_string_list_lookup(&config_fetch_recurse_submodules_for_name, submodname.buf);
+               config->util = xmemdupz(name, namelen);
+       } else if (!strcmp(key, "fetchrecursesubmodules")) {
+               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 ((len > 7) && !strcmp(var + len - 7, ".ignore")) {
+       } 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, var, len - 7);
-               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;
        }

diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index e7c240fc1f8333c933f22bc1af6bf57eb9ae27cb..3fbd366ec3bf95020959041462721291d6b92cf2 100755 (executable)
--- 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' '

diff --git a/userdiff.c b/userdiff.c
index ed958ef6b8b912436ad0145fe650adea083412e3..ea43a0306f1a017f719e0d6b7c1de8b063760184 100644 (file)
--- a/userdiff.c
+++ b/userdiff.c
@@ -184,35 +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 *dot;
-       const char *name;
-       int namelen;
-
-       if (prefixcmp(var, "diff."))
-               return NULL;
-       dot = strrchr(var, '.');
-       if (dot == var + 4)
-               return NULL;
-       if (strcmp(type, dot+1))
-               return NULL;
-
-       name = var + 5;
-       namelen = dot - name;
-       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)
 {
@@ -240,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 (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 ((drv = parse_driver(k, v, "funcname")))
+       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;