[PATCH] archive-tar: fix sanity check in config parsing

[PATCH] archive-tar: fix sanity check in config parsing

[René Scharfe]

git archive supports passing generated tar archives through filter
commands like gzip.  Additional filters can be set up using the
configuration variables tar.<name>.command and tar.<name>.remote.

When parsing these config variable names, we currently check that
the second dot is found nine characters into the name, disallowing
filter names with a length of five characters.  Additionally,
git archive crashes when the second dot is omitted:

	$ ./git -c tar.foo=bar archive HEAD >/dev/null
	fatal: Data too large to fit into virtual memory space.

Instead we should check if the second dot exists at all, or if
we only found the first one.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 archive-tar.c       | 2 +-
 t/t5000-tar-tree.sh | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/archive-tar.c b/archive-tar.c
index d1cce46..093d10e 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -335,7 +335,7 @@ static int tar_filter_config(const char *var, const char *value, void *data)
 	if (prefixcmp(var, "tar."))
 		return 0;
 	dot = strrchr(var, '.');
-	if (dot == var + 9)
+	if (dot == var + 3)
 		return 0;
 
 	name = var + 4;
diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index e7c240f..3fbd366 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' '
-- 
1.8.0

Re: [PATCH] archive-tar: fix sanity check in config parsing

[Jeff King]

On Sun, Jan 13, 2013 at 06:42:01PM +0100, René Scharfe wrote:

> When parsing these config variable names, we currently check that
> the second dot is found nine characters into the name, disallowing
> filter names with a length of five characters.  Additionally,
> git archive crashes when the second dot is omitted:
> 
> 	$ ./git -c tar.foo=bar archive HEAD >/dev/null
> 	fatal: Data too large to fit into virtual memory space.
> 
> Instead we should check if the second dot exists at all, or if
> we only found the first one.

Eek. Thanks for finding it. Your fix is obviously correct.

> --- a/archive-tar.c
> +++ b/archive-tar.c
> @@ -335,7 +335,7 @@ static int tar_filter_config(const char *var, const char *value, void *data)
>  	if (prefixcmp(var, "tar."))
>  		return 0;
>  	dot = strrchr(var, '.');
> -	if (dot == var + 9)
> +	if (dot == var + 3)
>  		return 0;

For the curious, the original version of the patch[1] read:

+       if (prefixcmp(var, "tarfilter."))
+               return 0;
+       dot = strrchr(var, '.');
+       if (dot == var + 9)
+               return 0;

and when I shortened the config section to "tar" in a re-roll of the
series, I missed the corresponding change to the offset.

-Peff

[1] http://thread.gmane.org/gmane.comp.version-control.git/175785/focus=175858

Re: [PATCH] archive-tar: fix sanity check in config parsing

[Joachim Schmitz]

Jeff King wrote:
> On Sun, Jan 13, 2013 at 06:42:01PM +0100, René Scharfe wrote:
>
>> When parsing these config variable names, we currently check that
>> the second dot is found nine characters into the name, disallowing
>> filter names with a length of five characters.  Additionally,
>> git archive crashes when the second dot is omitted:
>>
>> $ ./git -c tar.foo=bar archive HEAD >/dev/null
>> fatal: Data too large to fit into virtual memory space.
>>
>> Instead we should check if the second dot exists at all, or if
>> we only found the first one.
>
> Eek. Thanks for finding it. Your fix is obviously correct.
>
>> --- a/archive-tar.c
>> +++ b/archive-tar.c
>> @@ -335,7 +335,7 @@ static int tar_filter_config(const char *var,
>>  const char *value, void *data) if (prefixcmp(var, "tar."))
>>  return 0;
>>  dot = strrchr(var, '.');
>> - if (dot == var + 9)
>> + if (dot == var + 3)
>>  return 0;
>
> For the curious, the original version of the patch[1] read:
>
> +       if (prefixcmp(var, "tarfilter."))
> +               return 0;
> +       dot = strrchr(var, '.');
> +       if (dot == var + 9)
> +               return 0;
>
> and when I shortened the config section to "tar" in a re-roll of the
> series, I missed the corresponding change to the offset.

Wouldn't it then be better ti use strlen("tar") rather than a 3? Or at least 
a comment?

Bye, Jojo 

Re: [PATCH] archive-tar: fix sanity check in config parsing

[Jeff King]

On Mon, Jan 14, 2013 at 09:17:57AM +0100, Joachim Schmitz wrote:

> >For the curious, the original version of the patch[1] read:
> >
> >+       if (prefixcmp(var, "tarfilter."))
> >+               return 0;
> >+       dot = strrchr(var, '.');
> >+       if (dot == var + 9)
> >+               return 0;
> >
> >and when I shortened the config section to "tar" in a re-roll of the
> >series, I missed the corresponding change to the offset.
> 
> Wouldn't it then be better ti use strlen("tar") rather than a 3? Or
> at least a comment?

Then you are relying on the two strings being the same, rather than the
string and the length being the same. If you wanted to DRY it up, it
would look like:

diff --git a/archive-tar.c b/archive-tar.c
index d1cce46..a7c0690 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -332,15 +332,17 @@ static int tar_filter_config(const char *var, const char *value, void *data)
 	const char *type;
 	int namelen;
 
-	if (prefixcmp(var, "tar."))
+#define SECTION "tar"
+	if (prefixcmp(var, SECTION "."))
 		return 0;
 	dot = strrchr(var, '.');
-	if (dot == var + 9)
+	if (dot == var + strlen(SECTION))
 		return 0;
 
-	name = var + 4;
+	name = var + strlen(SECTION) + 1;
 	namelen = dot - name;
 	type = dot + 1;
+#undef SECTION
 
 	ar = find_tar_filter(name, namelen);
 	if (!ar) {


(of course there are other variants where you do not use a macro, but
then you need to manually check for the "." after the prefixcmp call).
I dunno. It is technically more robust in that the offsets are computed,
but I think it is a little harder to read. Of course, I wrote the
original so I am probably not a good judge.

We could also potentially encapsulate it in a function. I think the diff
code has a very similar block.

-Peff

Re: [PATCH] archive-tar: fix sanity check in config parsing

[Jeff King]

On Mon, Jan 14, 2013 at 04:44:24AM -0800, Jeff King wrote:

> > Wouldn't it then be better ti use strlen("tar") rather than a 3? Or
> > at least a comment?
> [...]
> We could also potentially encapsulate it in a function. I think the diff
> code has a very similar block.

Here's a series that does that, with a few other cleanups on top. The
diffstat actually ends up a few lines longer, but that is mostly because
of comments and function declarations. More importantly, though, the
call-sites are much easier to read.

Having written this series, though, I can't help but wonder if the world
would be a better place if config_fn_t looked more like:

  typedef int (*config_fn_t)(const char *full_var,
                             const char *section,
                             const char *subsection,
                             const char *key,
                             const char *value,
                             void *data);

It's just as easy for the config parser to do this ahead of time, and by
handing off real C-strings (instead of ending up with a ptr/len pair for
the subsection), it makes the lives of the callbacks much easier (e.g.,
the final patch below contorts a bit to use string_list with the
subsection).

I can look into that, but here is the less invasive cleanup:

  [1/6]: config: add helper function for parsing key names
  [2/6]: archive-tar: use match_config_key when parsing config
  [3/6]: convert some config callbacks to match_config_key
  [4/6]: userdiff: drop parse_driver function
  [5/6]: submodule: use match_config_key when parsing config
  [6/6]: submodule: simplify memory handling in config parsing

-Peff

[PATCH 1/6] config: add helper function for parsing key names

[Jeff King]

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 <peff@peff.net>
---
No users yet; they come in future patches.

 cache.h  | 15 +++++++++++++++
 config.c | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+)

diff --git a/cache.h b/cache.h
index c257953..14003b8 100644
--- a/cache.h
+++ b/cache.h
@@ -1164,6 +1164,21 @@ extern int git_config_include(const char *name, const char *value, void *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 match_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 7b444b6..18d9c0e 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 match_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;
+}
-- 
1.8.1.rc1.10.g7d71f7b

[PATCH 2/6] archive-tar: use match_config_key when parsing config

[Jeff King]

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 <peff@peff.net>
---
This obviously replaces René's patch. It might make more sense to just
put his patch onto maint, and build this on top; then this patch can get
squashed into the next one (which just updates the uninteresting
callsites).

 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 0ba3f25..68dbe59 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -325,20 +325,12 @@ static int tar_filter_config(const char *var, const char *value, void *data)
 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 (match_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 ecf00ed..517ae04 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -283,7 +283,8 @@ test_expect_success 'setup tar filters' '
 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' '
-- 
1.8.1.rc1.10.g7d71f7b

[PATCH 3/6] convert some config callbacks to match_config_key

[Jeff King]

This is easier to read and avoids magic offset constants
which need to be in sync with the section-name we provide.

Signed-off-by: Jeff King <peff@peff.net>
---
 convert.c  |  6 +-----
 ll-merge.c |  6 +-----
 userdiff.c | 13 +++----------
 3 files changed, 5 insertions(+), 20 deletions(-)

diff --git a/convert.c b/convert.c
index 6602155..e3ecb30 100644
--- a/convert.c
+++ b/convert.c
@@ -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 (match_config_key(var, "filter", &name, &namelen, &ep) < 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:
diff --git a/ll-merge.c b/ll-merge.c
index acea33b..d4c4ff6 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -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 (match_config_key(var, "merge", &name, &namelen, &ep) < 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,8 +254,6 @@ 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 (!value)
 			return error("%s: lacks value", var);
diff --git a/userdiff.c b/userdiff.c
index ed958ef..1a6a0fa 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 (match_config_key(var, "diff", &name, &namelen, &key) < 0 ||
+	    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);
-- 
1.8.1.rc1.10.g7d71f7b

[PATCH 4/6] userdiff: drop parse_driver function

[Jeff King]

When we parse userdiff config, we generally assume that

  diff.name.key

will affect the "key" value of the "name" driver. However,
without checking the key, we 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 <peff@peff.net>
---
This is not strictly related to the series, but I noticed it as a
cleanup while doing the previous patch.

 userdiff.c | 50 +++++++++++++++++++++-----------------------------
 1 file changed, 21 insertions(+), 29 deletions(-)

diff --git a/userdiff.c b/userdiff.c
index 1a6a0fa..c6cdec4 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 (match_config_key(var, "diff", &name, &namelen, &key) < 0 ||
-	    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 @@ int userdiff_config(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 (match_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;
-- 
1.8.1.rc1.10.g7d71f7b

[PATCH 5/6] submodule: use match_config_key when parsing config

[Jeff King]

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]
  fatal: bad submodule.foo.fetchrecursesubmodules argument: bogus

Signed-off-by: Jeff King <peff@peff.net>
---
 submodule.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/submodule.c b/submodule.c
index 2f55436..4361207 100644
--- a/submodule.c
+++ b/submodule.c
@@ -126,15 +126,17 @@ int parse_submodule_config_option(const char *var, const char *value)
 
 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 (match_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 +144,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);
-- 
1.8.1.rc1.10.g7d71f7b

[PATCH 6/6] submodule: simplify memory handling in config parsing

[Jeff King]

We keep a strbuf for the name of the submodule, even though
we only ever add one buffer (which we know the length of) 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 <peff@peff.net>
---
I'm undecided on this one. I think the result is easier to follow, but
others might find the original easier. This would be a lot more
readable if the config parser gave us a broken-out representation with
real C strings. Then we could pass the subsection straight to the
string_list functions for lookup, and using the "dup" flag of string
list, avoid even having to deal with memory duplication at all.

 submodule.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/submodule.c b/submodule.c
index 4361207..23a8490 100644
--- a/submodule.c
+++ b/submodule.c
@@ -127,7 +127,6 @@ int parse_submodule_config_option(const char *var, const char *value)
 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;
 
@@ -136,37 +135,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;
 	}
-- 
1.8.1.rc1.10.g7d71f7b

Re: [PATCH 3/6] convert some config callbacks to match_config_key

[Jonathan Nieder]

Jeff King wrote:

> --- a/convert.c
> +++ b/convert.c
> @@ -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 (match_config_key(var, "filter", &name, &namelen, &ep) < 0 || !name)
>  		return 0;

Hm, I actually find the preimage more readable here.

I like the idea of having a function to do this, though.  Here are a
couple of ideas for making the meaning obvious again for people like
me:

Rename match_config_key() to something like parse_config_key()?
match_ makes it sound like its main purpose is to match it against a
pattern, but really it is more about decomposing into constituent
parts.

Rename ep to something like 'key' or 'filtertype'?  Without the
explicit string processing, it is not obvious what ep is the end of.

[...]
> --- 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 (match_config_key(var, "diff", &name, &namelen, &key) < 0 ||
> +	    strcmp(type, key))
>  		return NULL;
>  
> -	name = var + 5;
> -	namelen = dot - name;
>  	drv = userdiff_find_by_namelen(name, namelen);

What happens in the !name case?  (Honest question --- I haven't checked.)

Generally I like the cleanup.  Thanks for tasteful patch.

Jonathan

Re: [PATCH 3/6] convert some config callbacks to match_config_key

[Jeff King]

On Mon, Jan 14, 2013 at 08:55:28AM -0800, Jonathan Nieder wrote:

> Jeff King wrote:
> 
> > --- a/convert.c
> > +++ b/convert.c
> > @@ -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 (match_config_key(var, "filter", &name, &namelen, &ep) < 0 || !name)
> >  		return 0;
> 
> Hm, I actually find the preimage more readable here.

The big thing to me is getting rid of the manual "6" there, which is bad
for maintainability (it must match the length of "filter", and there is
no compile-time verification).

> Rename match_config_key() to something like parse_config_key()?
> match_ makes it sound like its main purpose is to match it against a
> pattern, but really it is more about decomposing into constituent
> parts.

There is already a git_config_parse_key, but it does something else. I
wanted to avoid confusion there. And I was trying to indicate that this
was not just about parsing, but also about matching the section.
Basically I was trying to encapsulate the current technique and have as
little code change as possible. But maybe:

  struct config_key k;
  parse_config_key(&k, var);
  if (strcmp(k.section, "filter") || k.subsection))
          return 0;

would be a better start (or having git_config do the first two lines
itself before triggering the callback).

> Rename ep to something like 'key' or 'filtertype'?  Without the
> explicit string processing, it is not obvious what ep is the end of.

Ah, so that is what "ep" stands for. I was thinking it is a terrible
variable name, but I was trying to keep the patch minimal.

> > -	if (prefixcmp(var, "diff."))
> > -		return NULL;
> > -	dot = strrchr(var, '.');
> > -	if (dot == var + 4)
> > -		return NULL;
> > -	if (strcmp(type, dot+1))
> > +	if (match_config_key(var, "diff", &name, &namelen, &key) < 0 ||
> > +	    strcmp(type, key))
> >  		return NULL;
> >  
> > -	name = var + 5;
> > -	namelen = dot - name;
> >  	drv = userdiff_find_by_namelen(name, namelen);
> 
> What happens in the !name case?  (Honest question --- I haven't checked.)

Segfault, I expect. Thanks for catching.

I actually wrote this correctly once, coupled with patch 4, but screwed
it up when teasing it apart into two patches. It should be:

  if (match_config_key(var, "diff", &name, &namelen, &key) < 0 ||
      !name ||
      strcmp(type, key))
          return NULL;

Patch 4 replaces this with correct code (as it moves it into
userdiff_config).

-Peff

Re: [PATCH 3/6] convert some config callbacks to match_config_key

[Jeff King]

On Mon, Jan 14, 2013 at 09:06:10AM -0800, Jeff King wrote:

>   struct config_key k;
>   parse_config_key(&k, var);
>   if (strcmp(k.section, "filter") || k.subsection))
>           return 0;
> 
> would be a better start (or having git_config do the first two lines
> itself before triggering the callback).

Here's what that looks like, along with the cleanups in submodule.c that
are made possible by it.

I "cheat" a little and use a static buffer when parsing the config key,
so that the caller does not have to deal with freeing it. It makes using
the parser literally as simple as the lines above, but it does mean it
isn't re-entrant (and worse, it has to be invoked from a config
callback, since the static buffer is tied to the config file stack).

None of that is a problem for the use here, but it is not a
generally-callable function. For that reason, it might make more sense
to have the config parser just provide the config_key, and not have a
public function at all. The downside to that is that we have to update
the function signature of all of the config callbacks.

---
 cache.h     |  7 ++++++
 config.c    | 35 ++++++++++++++++++++++++++++++
 submodule.c | 41 ++++++++++++++---------------------
 3 files changed, 58 insertions(+), 25 deletions(-)

diff --git a/cache.h b/cache.h
index c257953..df756e6 100644
--- a/cache.h
+++ b/cache.h
@@ -1119,6 +1119,13 @@ extern int update_server_info(int);
 #define CONFIG_INVALID_PATTERN 6
 #define CONFIG_GENERIC_ERROR 7
 
+struct config_key {
+	const char *section;
+	const char *subsection;
+	const char *key;
+};
+void config_key_parse(struct config_key *, const char *);
+
 typedef int (*config_fn_t)(const char *, const char *, void *);
 extern int git_default_config(const char *, const char *, void *);
 extern int git_config_from_file(config_fn_t fn, const char *, void *);
diff --git a/config.c b/config.c
index 7b444b6..7b8df3e 100644
--- a/config.c
+++ b/config.c
@@ -18,6 +18,7 @@ typedef struct config_file {
 	int eof;
 	struct strbuf value;
 	struct strbuf var;
+	struct strbuf key_parse_buf;
 } config_file;
 
 static config_file *cf;
@@ -899,6 +900,7 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)
 		top.eof = 0;
 		strbuf_init(&top.value, 1024);
 		strbuf_init(&top.var, 1024);
+		strbuf_init(&top.key_parse_buf, 1024);
 		cf = ⊤
 
 		ret = git_parse_file(fn, data);
@@ -906,6 +908,7 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)
 		/* pop config-file parsing state stack */
 		strbuf_release(&top.value);
 		strbuf_release(&top.var);
+		strbuf_release(&top.key_parse_buf);
 		cf = top.prev;
 
 		fclose(f);
@@ -1667,3 +1670,35 @@ int config_error_nonbool(const char *var)
 {
 	return error("Missing value for '%s'", var);
 }
+
+void config_key_parse(struct config_key *key, const char *var)
+{
+	/*
+	 * We want to use a static buffer so the caller does not have to worry
+	 * about memory ownership. But since config parsing can happen
+	 * recursively, we must use storage from the stack of config files.
+	 */
+	struct strbuf *sb = &cf->key_parse_buf;
+	char *dot;
+	char *rdot;
+
+	strbuf_reset(sb);
+	strbuf_addstr(sb, var);
+
+	dot = strchr(sb->buf, '.');
+	rdot = strrchr(sb->buf, '.');
+	/* Should never happen because our keys come from git_parse_file. */
+	if (!dot)
+		die("BUG: config_key_parse was fed a bogus key");
+	key->section = sb->buf;
+	*dot = '\0';
+	key->key = rdot + 1;
+
+	if (rdot == dot)
+		key->subsection = NULL;
+	else {
+		*rdot = '\0';
+		key->subsection = dot + 1;
+	}
+
+}
diff --git a/submodule.c b/submodule.c
index 2f55436..4894718 100644
--- a/submodule.c
+++ b/submodule.c
@@ -11,9 +11,9 @@
 #include "sha1-array.h"
 #include "argv-array.h"
 
-static struct string_list config_name_for_path;
-static struct string_list config_fetch_recurse_submodules_for_name;
-static struct string_list config_ignore_for_name;
+static struct string_list config_name_for_path = STRING_LIST_INIT_DUP;
+static struct string_list config_fetch_recurse_submodules_for_name = STRING_LIST_INIT_DUP;
+static struct string_list config_ignore_for_name = STRING_LIST_INIT_DUP;
 static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;
 static struct string_list changed_submodule_paths;
 static int initialized_fetch_ref_tips;
@@ -126,47 +126,38 @@ int parse_submodule_config_option(const char *var, const char *value)
 
 int parse_submodule_config_option(const char *var, const char *value)
 {
-	int len;
+	struct config_key key;
 	struct string_list_item *config;
-	struct strbuf submodname = STRBUF_INIT;
 
-	var += 10;		/* Skip "submodule." */
+	config_key_parse(&key, var);
+	if (strcmp(key.section, "submodule") || !key.subsection)
+		return 0;
 
-	len = strlen(var);
-	if ((len > 5) && !strcmp(var + len - 5, ".path")) {
-		strbuf_add(&submodname, var, len - 5);
+	if (!strcmp(key.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 = string_list_append(&config_name_for_path, value);
+		config->util = xstrdup(key.subsection);
+	} else if (!strcmp(key.key, "fetchrecursesubmodules")) {
+		config = unsorted_string_list_lookup(&config_fetch_recurse_submodules_for_name, key.subsection);
 		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, key.subsection);
 		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.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);
-		config = unsorted_string_list_lookup(&config_ignore_for_name, submodname.buf);
+		config = unsorted_string_list_lookup(&config_ignore_for_name, key.subsection);
 		if (config)
 			free(config->util);
 		else
-			config = string_list_append(&config_ignore_for_name,
-						    strbuf_detach(&submodname, NULL));
-		strbuf_release(&submodname);
+			config = string_list_append(&config_ignore_for_name, key.subsection);
 		config->util = xstrdup(value);
-		return 0;
 	}
 	return 0;
 }

Re: [PATCH 1/6] config: add helper function for parsing key names

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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 <peff@peff.net>
> ---
> No users yet; they come in future patches.
>
>  cache.h  | 15 +++++++++++++++
>  config.c | 33 +++++++++++++++++++++++++++++++++
>  2 files changed, 48 insertions(+)
>
> diff --git a/cache.h b/cache.h
> index c257953..14003b8 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1164,6 +1164,21 @@ extern int git_config_include(const char *name, const char *value, void *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 match_config_key(const char *var,
> +		     const char *section,
> +		     const char **subsection, int *subsection_len,
> +		     const char **key);
> +

I agree with Jonathan about the naming s/match/parse/.

After looking at the callers in your later patches, I think the
counted interface to subsection is probably fine.  The caller can
check !subsection to see if it is a two- or three- level name, and

    if (parse_config_key(var, "submodule", &name, &namelen,  &key) < 0 ||
	!name)
	return 0;

is very easy to follow (that is the result of your 5th step).

Re: [PATCH 1/6] config: add helper function for parsing key names

[Jeff King]

On Mon, Jan 14, 2013 at 10:08:47AM -0800, Junio C Hamano wrote:

> > +extern int match_config_key(const char *var,
> > +		     const char *section,
> > +		     const char **subsection, int *subsection_len,
> > +		     const char **key);
> > +
> 
> I agree with Jonathan about the naming s/match/parse/.

I see this is marked for re-roll in WC. I'm happy to re-roll it with the
suggestions from Jonathan, but before I do, did you have any comment on
the "struct config_key" alternative I sent as a follow-up?

You said:

> After looking at the callers in your later patches, I think the
> counted interface to subsection is probably fine.  The caller can
> check !subsection to see if it is a two- or three- level name, and
> 
>     if (parse_config_key(var, "submodule", &name, &namelen,  &key) < 0 ||
> 	!name)
> 	return 0;
> 
> is very easy to follow (that is the result of your 5th step).

but I wasn't sure if that was "it is not worth the trouble of the other
one" or "I did not yet read the other one".

-Peff

Re: [PATCH 1/6] config: add helper function for parsing key names

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> ... did you have any comment on
> the "struct config_key" alternative I sent as a follow-up?

I did read it but I cannot say I did so very carefully.  My gut
reaction was that the "take the variable name and section name,
return the subsection name pointer and length, if there is any, and
the key" made it readable enough.  The proposed interface to make
and lend a copy to the caller does make it more readble, but I do
not know if that is worth doing.  Neutral-to-slightly-in-favor, I
would say.

Re: [PATCH 1/6] config: add helper function for parsing key names

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jeff King <peff@peff.net> writes:
>
>> ... did you have any comment on
>> the "struct config_key" alternative I sent as a follow-up?
>
> I did read it but I cannot say I did so very carefully.  My gut
> reaction was that the "take the variable name and section name,
> return the subsection name pointer and length, if there is any, and
> the key" made it readable enough.  The proposed interface to make
> and lend a copy to the caller does make it more readble, but I do
> not know if that is worth doing.  Neutral-to-slightly-in-favor, I
> would say.

Now I re-read that "struct config_key" thing, I would have to say
that the idea of giving split and NUL-terminated strings to the
callers is good, but the "cheat" looks somewhat brittle for all the
reasons that come from using a static buffer (which you already
mentioned).  As I do not offhand think of a better alternative, I'd
say we leave it for another day.

[PATCHv2 0/8] config key-parsing cleanups

[Jeff King]

On Fri, Jan 18, 2013 at 12:53:32PM -0800, Junio C Hamano wrote:

> >> ... did you have any comment on
> >> the "struct config_key" alternative I sent as a follow-up?
> >
> > I did read it but I cannot say I did so very carefully.  My gut
> > reaction was that the "take the variable name and section name,
> > return the subsection name pointer and length, if there is any, and
> > the key" made it readable enough.  The proposed interface to make
> > and lend a copy to the caller does make it more readble, but I do
> > not know if that is worth doing.  Neutral-to-slightly-in-favor, I
> > would say.
> 
> Now I re-read that "struct config_key" thing, I would have to say
> that the idea of giving split and NUL-terminated strings to the
> callers is good, but the "cheat" looks somewhat brittle for all the
> reasons that come from using a static buffer (which you already
> mentioned).  As I do not offhand think of a better alternative, I'd
> say we leave it for another day.

OK. I had the feeling if the config parser provided it to the caller
that more sites could take advantage of it (without adding too many
lines to call the parsing function). But looking again, there aren't
that many sites that would benefit. E.g., git_daemon_config in daemon.c
could use it to avoid using a constant offset. But the current code
there is not hard to read, and saving a few characters there is not
worth the complexity.

So I've re-rolled the original version, taking into account the comments
from you and Jonathan. I also clarified a few of the commit messages,
and modified two more sites to use the new function.

  [1/8]: config: add helper function for parsing key names

Same as before, but now called parse_config_key.

  [2/8]: archive-tar: use parse_config_key when parsing config

Same (rebased for new name, of course).

  [3/8]: convert some config callbacks to parse_config_key

Tweaked confusing "ep" variable name. Fixed missing "!name" check in
userdiff code (which gets removed in the next patch anyway).

  [4/8]: userdiff: drop parse_driver function
  [5/8]: submodule: use parse_config_key when parsing config
  [6/8]: submodule: simplify memory handling in config parsing

Same.

  [7/8]: help: use parse_config_key for man config
  [8/8]: reflog: use parse_config_key in config callback

Two new callsites. I split these out because unlike the ones in 3/8,
they do not benefit from a reduction in lines of code. However, I think
the results are still more readable. You can judge for yourself; drop
them if you disagree. Or feel free to squash them into 3/8 if that makes
more sense.

-Peff

[PATCHv2 1/8] config: add helper function for parsing key names

[Jeff King]

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 <peff@peff.net>
---
 cache.h  | 15 +++++++++++++++
 config.c | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+)

diff --git a/cache.h b/cache.h
index c257953..b19305b 100644
--- a/cache.h
+++ b/cache.h
@@ -1164,6 +1164,21 @@ extern int git_config_include(const char *name, const char *value, void *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 7b444b6..11bd4d8 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;
+}
-- 
1.8.0.2.15.g815dc66

[PATCHv2 2/8] archive-tar: use parse_config_key when parsing config

[Jeff King]

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 <peff@peff.net>
---
 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 d1cce46..719b629 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -327,20 +327,12 @@ static int tar_filter_config(const char *var, const char *value, void *data)
 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 e7c240f..3fbd366 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -212,7 +212,8 @@ test_expect_success 'setup tar filters' '
 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' '
-- 
1.8.0.2.15.g815dc66

[PATCHv2 3/8] convert some config callbacks to parse_config_key

[Jeff King]

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 <peff@peff.net>
---
 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 6602155..3520252 100644
--- a/convert.c
+++ b/convert.c
@@ -457,7 +457,7 @@ static int read_convert_config(const char *var, const char *value, void *cb)
 
 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 acea33b..fb61ea6 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -222,7 +222,7 @@ static int read_merge_config(const char *var, const char *value, void *cb)
 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/userdiff.c b/userdiff.c
index ed958ef..a4ea1e9 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);
-- 
1.8.0.2.15.g815dc66

[PATCHv2 4/8] userdiff: drop parse_driver function

[Jeff King]

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 <peff@peff.net>
---
 userdiff.c | 50 +++++++++++++++++++++-----------------------------
 1 file changed, 21 insertions(+), 29 deletions(-)

diff --git a/userdiff.c b/userdiff.c
index a4ea1e9..ea43a03 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 @@ int userdiff_config(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;
-- 
1.8.0.2.15.g815dc66

[PATCHv2 5/8] submodule: use parse_config_key when parsing config

[Jeff King]

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 <peff@peff.net>
---
 submodule.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/submodule.c b/submodule.c
index 2f55436..25413de 100644
--- a/submodule.c
+++ b/submodule.c
@@ -126,15 +126,16 @@ int parse_submodule_config_option(const char *var, const char *value)
 
 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);
-- 
1.8.0.2.15.g815dc66

[PATCHv2 6/8] submodule: simplify memory handling in config parsing

[Jeff King]

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 <peff@peff.net>
---
 submodule.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/submodule.c b/submodule.c
index 25413de..9ba1496 100644
--- a/submodule.c
+++ b/submodule.c
@@ -127,7 +127,6 @@ int parse_submodule_config_option(const char *var, const char *value)
 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;
 	}
-- 
1.8.0.2.15.g815dc66

[PATCHv2 7/8] help: use parse_config_key for man config

[Jeff King]

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 <peff@peff.net>
---
 builtin/help.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/builtin/help.c b/builtin/help.c
index bd86253..04cb77d 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -237,21 +237,21 @@ static int add_man_viewer_info(const char *var, const char *value)
 
 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;
-- 
1.8.0.2.15.g815dc66

[PATCHv2 8/8] reflog: use parse_config_key in config callback

[Jeff King]

This doesn't save any lines, but does keep us from doing
error-prone pointer arithmetic with constants.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/reflog.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index b3c9e27..1fedf66 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -510,26 +510,27 @@ static int reflog_expire_config(const char *var, const char *value, void *cb)
 
 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) {
-- 
1.8.0.2.15.g815dc66

Re: [PATCHv2 8/8] reflog: use parse_config_key in config callback

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> This doesn't save any lines, but does keep us from doing
> error-prone pointer arithmetic with constants.

Yeah, this and 7/8 shows that the true value of the new parse
function is not line number reduction but clarity of the calling
code.  There is really no point making everybody implement "last_dot
is there, so the subsection name must be between the section name
and that last_dot" like the original before this patch.

Thanks.  Will read it over again and then apply.

Re: [PATCHv2 0/8] config key-parsing cleanups

[Jonathan Nieder]

Jeff King wrote:

> So I've re-rolled the original version

Lovely. :)
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Re: [PATCHv2 5/8] submodule: use parse_config_key when parsing config

[Jens Lehmann]

Am 23.01.2013 07:25, schrieb Jeff King:
> 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

Thanks, that makes lots of sense!

Acked-by: Jens Lehmann <Jens.Lehmann@web.de>

> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  submodule.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/submodule.c b/submodule.c
> index 2f55436..25413de 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -126,15 +126,16 @@ int parse_submodule_config_option(const char *var, const char *value)
>  
>  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);
> 

Re: [PATCHv2 6/8] submodule: simplify memory handling in config parsing

[Jens Lehmann]

Am 23.01.2013 07:26, schrieb Jeff King:
> 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.

Thanks, this helps until I some day find the time to refactor
that code into a more digestible shape ;-)

Acked-by: Jens Lehmann <Jens.Lehmann@web.de>

> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  submodule.c | 30 ++++++++++++++----------------
>  1 file changed, 14 insertions(+), 16 deletions(-)
> 
> diff --git a/submodule.c b/submodule.c
> index 25413de..9ba1496 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -127,7 +127,6 @@ int parse_submodule_config_option(const char *var, const char *value)
>  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;
>  	}
>