threads / patch / 32615

patcharchive-tar: fix sanity check in config parsing

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

## tl;dr

31 messages between Jan 13, 2013 and Jan 23, 2013. Diffs are folded; open one to read it.

replies: 30people: 6as markdown or json

René Scharfe· Jan 13, 2013, 17:42 UTC · lore

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(-)
Show changes to 2 files +3 −2

archive-tar.c, t/t5000-tar-tree.sh

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
Jeff King· Jan 13, 2013, 20:00 UTC · re: René Scharfe · lore

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

On Sun, Jan 13, 2013 at 06:42:01PM +0100, René Scharfe wrote:
Show 10 quoted lines
> 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.
Show 9 quoted lines
> --- 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
Joachim Schmitz· Jan 14, 2013, 08:17 UTC · re: Jeff King · lore

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

Jeff King wrote:
Show 35 quoted lines
> 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 
Jeff King· Jan 14, 2013, 12:44 UTC · re: Joachim Schmitz · lore

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

On Mon, Jan 14, 2013 at 09:17:57AM +0100, Joachim Schmitz wrote:
Show 13 quoted lines
> >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:

Show changes to archive-tar.c +5 −4
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
Jeff King· Jan 14, 2013, 14:58 UTC · re: Jeff King · lore

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

On Mon, Jan 14, 2013 at 04:44:24AM -0800, Jeff King wrote:
Show 5 quoted lines
> > 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
Jeff King· Jan 14, 2013, 15:00 UTC · re: Jeff King · lore

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

The config callback functions get keys of the general form:
  section.subsection.key

(where the subsection may be contain arbitrary data, or may be missing). For matching keys without subsections, it is simple enough to call "strcmp". Matching keys with subsections is a little more complicated, and each callback does it in an ad-hoc way, usually involving error-prone pointer arithmetic.

Let's provide a helper that keeps the pointer arithmetic all in one place.

Signed-off-by: Jeff King <peff@peff.net>
---
No users yet; they come in future patches.
 cache.h  | 15 +++++++++++++++
 config.c | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+)
Show changes to 2 files +48 −0

cache.h, config.c

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
Junio C Hamano· Jan 14, 2013, 18:08 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 45 quoted lines
> 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).
Jeff King· Jan 15, 2013, 16:04 UTC · re: Junio C Hamano · lore

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

On Mon, Jan 14, 2013 at 10:08:47AM -0800, Junio C Hamano wrote:
Show 7 quoted lines
> > +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:
Show 9 quoted lines
> 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
Junio C Hamano· Jan 15, 2013, 17:07 UTC · re: Jeff King · lore

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

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.

Junio C Hamano· Jan 18, 2013, 20:53 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 12 quoted lines
> 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.

Jeff King· Jan 23, 2013, 06:21 UTC · re: Junio C Hamano · lore

[PATCHv2 0/8] config key-parsing cleanups

On Fri, Jan 18, 2013 at 12:53:32PM -0800, Junio C Hamano wrote:
Show 17 quoted lines
> >> ... 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
Jeff King· Jan 23, 2013, 06:23 UTC · re: Jeff King · lore

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

The config callback functions get keys of the general form:
  section.subsection.key

(where the subsection may be contain arbitrary data, or may be missing). For matching keys without subsections, it is simple enough to call "strcmp". Matching keys with subsections is a little more complicated, and each callback does it in an ad-hoc way, usually involving error-prone pointer arithmetic.

Let's provide a helper that keeps the pointer arithmetic all in one place.

Signed-off-by: Jeff King <peff@peff.net>
---
 cache.h  | 15 +++++++++++++++
 config.c | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+)
Show changes to 2 files +48 −0

cache.h, config.c

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
Jeff King· Jan 23, 2013, 06:23 UTC · re: Jeff King · lore

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

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(-)
Show changes to 2 files +3 −10

archive-tar.c, t/t5000-tar-tree.sh

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
Jeff King· Jan 23, 2013, 06:24 UTC · re: Jeff King · lore

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

These callers can drop some inline pointer arithmetic and magic offset constants, making them more readable and less error-prone (those constants had to match the lengths of strings, but there is no automatic verification of that fact).

The "ep" pointer (presumably for "end pointer"), which points to the final key segment of the config variable, is given the more standard name "key" to describe its function rather than its derivation.

Signed-off-by: Jeff King <peff@peff.net>
---
 convert.c  | 14 +++++---------
 ll-merge.c | 14 +++++---------
 userdiff.c | 13 +++----------
 3 files changed, 13 insertions(+), 28 deletions(-)
Show changes to 3 files +13 −28

convert.c, ll-merge.c, userdiff.c

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
Jeff King· Jan 23, 2013, 06:25 UTC · re: Jeff King · lore

[PATCHv2 4/8] userdiff: drop parse_driver function

When we parse userdiff config, we generally assume that
  diff.name.key

will affect the "key" value of the "name" driver. However, without confirming that the key is a valid userdiff key, we may accidentally conflict with the ancient "diff.color.*" namespace. The current code is careful not to even create a driver struct if we do not see a key that is known by the diff-driver code.

However, this carefulness is unnecessary; the default driver with no keys set behaves exactly the same as having no driver at all. We can simply set up the driver struct as soon as we see we have a config key that looks like a driver. This makes the code a bit more readable.

Signed-off-by: Jeff King <peff@peff.net>
---
 userdiff.c | 50 +++++++++++++++++++++-----------------------------
 1 file changed, 21 insertions(+), 29 deletions(-)
Show changes to userdiff.c +21 −29
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
Jeff King· Jan 23, 2013, 06:25 UTC · re: Jeff King · lore

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

This makes the code a lot simpler to read by dropping a whole bunch of constant offsets.

As a bonus, it means we also feed the whole config variable name to our error functions:

  [before]
  $ git -c submodule.foo.fetchrecursesubmodules=bogus checkout
  fatal: bad foo.fetchrecursesubmodules argument: bogus
  [after]
  $ git -c submodule.foo.fetchrecursesubmodules=bogus checkout
  fatal: bad submodule.foo.fetchrecursesubmodules argument: bogus
Signed-off-by: Jeff King <peff@peff.net>
---
 submodule.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)
Show changes to submodule.c +10 −9
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
Jens Lehmann· Jan 23, 2013, 20:45 UTC · re: Jeff King · lore

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

Am 23.01.2013 07:25, schrieb Jeff King:
Show 13 quoted lines
> 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>
Show 59 quoted lines
> 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);
> 
Jeff King· Jan 23, 2013, 06:26 UTC · re: Jeff King · lore

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

We keep a strbuf for the name of the submodule, even though we only ever add one string to it. Let's just use xmemdupz instead, which is slightly more efficient and makes it easier to follow what is going on.

Unfortunately, we still end up having to deal with some memory ownership issues in some code branches, as we have to allocate the string in order to do a string list lookup, and then only sometimes want to hand ownership of that string over to the string_list. Still, making that explicit in the code (as opposed to sometimes detaching the strbuf, and then always releasing it) makes it a little more obvious what is going on.

Signed-off-by: Jeff King <peff@peff.net>
---
 submodule.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)
Show changes to submodule.c +14 −16
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
Jens Lehmann· Jan 23, 2013, 20:51 UTC · re: Jeff King · lore

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

Am 23.01.2013 07:26, schrieb Jeff King:
Show 13 quoted lines
> 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>
Show 70 quoted lines
> 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;
>  	}
> 
Jeff King· Jan 23, 2013, 06:27 UTC · re: Jeff King · lore

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

The resulting code ends up about the same length, but it is a little more self-explanatory. It now explicitly documents and checks the pre-condition that the incoming var starts with "man.", and drops the magic offset "4".

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/help.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)
Show changes to builtin/help.c +7 −7
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
Jeff King· Jan 23, 2013, 06:27 UTC · re: Jeff King · lore

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

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

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/reflog.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)
Show changes to builtin/reflog.c +7 −6
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
Junio C Hamano· Jan 23, 2013, 07:04 UTC · re: Jeff King · lore

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

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.
Jeff King· Jan 14, 2013, 15:02 UTC · re: Jeff King · lore

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

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(-)
Show changes to 2 files +3 −10

archive-tar.c, t/t5000-tar-tree.sh

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
Jeff King· Jan 14, 2013, 15:03 UTC · re: Jeff King · lore

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

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(-)
Show changes to 3 files +5 −20

convert.c, ll-merge.c, userdiff.c

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
Jonathan Nieder· Jan 14, 2013, 16:55 UTC · re: Jeff King · lore

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

Jeff King wrote:
Show 9 quoted lines
> --- 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.

[...]
Show 24 quoted lines
> --- 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
Jeff King· Jan 14, 2013, 17:06 UTC · re: Jonathan Nieder · lore

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

On Mon, Jan 14, 2013 at 08:55:28AM -0800, Jonathan Nieder wrote:
Show 13 quoted lines
> 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.

Show 15 quoted lines
> > -	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
Jeff King· Jan 14, 2013, 18:05 UTC · re: Jeff King · lore

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

On Mon, Jan 14, 2013 at 09:06:10AM -0800, Jeff King wrote:
Show 7 quoted lines
>   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(-)
Show changes to 3 files +58 −25

cache.h, config.c, submodule.c

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 = &top;
 
 		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;
 }
Jeff King· Jan 14, 2013, 15:04 UTC · re: Jeff King · lore

[PATCH 4/6] userdiff: drop parse_driver function

When we parse userdiff config, we generally assume that
  diff.name.key

will affect the "key" value of the "name" driver. However, without 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(-)
Show changes to userdiff.c +21 −29
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
Jeff King· Jan 14, 2013, 15:04 UTC · re: Jeff King · lore

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

This makes the code a lot simpler to read by dropping a whole bunch of constant offsets.

As a bonus, it means we also feed the whole config variable name to our error functions:

  [before]
  $ git -c submodule.foo.fetchrecursesubmodules=bogus checkout
  fatal: bad foo.fetchrecursesubmodules argument: bogus
  [after]
  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(-)
Show changes to submodule.c +11 −9
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
Jeff King· Jan 14, 2013, 15:07 UTC · re: Jeff King · lore

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

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(-)
Show changes to submodule.c +14 −16
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

← back to recent threads