git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Jeff King <peff@peff.net>
Date
Jan 14, 2013, 18:05 UTC
Message-ID
<20130114180550.GA12961@sigill.intra.peff.net>
In-Reply-To
<20130114170610.GB22098@sigill.intra.peff.net>
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(-)
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;
 }
Previous: Jeff KingNext: Jeff King
Message 28 of 31 in “archive-tar: fix sanity check in config parsing”
  1. archive-tar: fix sanity check in config parsingRené Scharfe, Jan 13, 2013
  2. Jeff KingJan 13, 2013
  3. Joachim SchmitzJan 14, 2013
  4. Jeff KingJan 14, 2013
  5. Jeff KingJan 14, 2013
  6. 1/6 config: add helper function for parsing key namesJeff King, Jan 14, 2013
  7. Junio C HamanoJan 14, 2013
  8. Jeff KingJan 15, 2013
  9. Junio C HamanoJan 15, 2013
  10. Junio C HamanoJan 18, 2013
  11. 0/8 config key-parsing cleanupsJeff King, Jan 23, 2013
  12. 1/8 config: add helper function for parsing key namesJeff King, Jan 23, 2013
  13. 2/8 archive-tar: use parse_config_key when parsing configJeff King, Jan 23, 2013
  14. 3/8 convert some config callbacks to parse_config_keyJeff King, Jan 23, 2013
  15. 4/8 userdiff: drop parse_driver functionJeff King, Jan 23, 2013
  16. 5/8 submodule: use parse_config_key when parsing configJeff King, Jan 23, 2013
  17. Jens LehmannJan 23, 2013
  18. 6/8 submodule: simplify memory handling in config parsingJeff King, Jan 23, 2013
  19. Jens LehmannJan 23, 2013
  20. 7/8 help: use parse_config_key for man configJeff King, Jan 23, 2013
  21. 8/8 reflog: use parse_config_key in config callbackJeff King, Jan 23, 2013
  22. Junio C HamanoJan 23, 2013
  23. Jonathan NiederJan 23, 2013
  24. 2/6 archive-tar: use match_config_key when parsing configJeff King, Jan 14, 2013
  25. 3/6 convert some config callbacks to match_config_keyJeff King, Jan 14, 2013
  26. Jonathan NiederJan 14, 2013
  27. Jeff KingJan 14, 2013
  28. Jeff KingJan 14, 2013
  29. 4/6 userdiff: drop parse_driver functionJeff King, Jan 14, 2013
  30. 5/6 submodule: use match_config_key when parsing configJeff King, Jan 14, 2013
  31. 6/6 submodule: simplify memory handling in config parsingJeff King, Jan 14, 2013

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.