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

[PATCH v3 7/8] config: report cached filenames in die_bad_number()

From
Glen Choo via GitGitGadget <gitgitgadget@gmail.com>
Date
Mar 28, 2023, 17:51 UTC
Message-ID
<eb843e6f08dcc143b71378750cca5efe553a377d.1680025914.git.gitgitgadget@gmail.com>
In-Reply-To
<pull.1463.v3.git.git.1680025914.gitgitgadget@gmail.com>
From: Glen Choo <chooglen@google.com>

If, when parsing numbers from config, die_bad_number() is called, it reports the filename and config source type if we were parsing a config file, but not if we were iterating a config_set (it defaults to a less specific error message). Most call sites don't parse config files because config is typically read once and cached, so we only report filename and config source type in "git config --type" (since "git config" always parses config files).

This could have been fixed when we taught the current_config_* functions to respect config_set values (0d44a2dacc (config: return configset value for current_config_ functions, 2016-05-26), but it was hard to spot then and we might have just missed it (I didn't find mention of die_bad_number() in the original ML discussion [1].)

Fix this by refactoring the current_config_* functions into variants that don't BUG() when we aren't reading config, and using the resulting functions in die_bad_number(). "git config --get[-regexp] --type=int" cannot use the non-refactored version because it parses the int value _after_ parsing the config file, which would run into the BUG().

Since the refactored functions aren't public, they use "struct config_reader".

1. https://lore.kernel.org/git/20160518223712.GA18317@sigill.intra.peff.net/
Signed-off-by: Glen Choo <chooglen@google.com>
---
 config.c               | 65 +++++++++++++++++++++++++++++-------------
 config.h               |  1 +
 t/helper/test-config.c | 17 +++++++++++
 t/t1308-config-set.sh  |  9 ++++++
 4 files changed, 72 insertions(+), 20 deletions(-)
diff --git a/config.c b/config.c
index 3756322ec96..46c5a9a7d51 100644
--- a/config.c
+++ b/config.c
@@ -1311,39 +1311,48 @@ int git_parse_ssize_t(const char *value, ssize_t *ret)
 	return 1;
 }
 
+static int reader_config_name(struct config_reader *reader, const char **out);
+static int reader_origin_type(struct config_reader *reader,
+			      enum config_origin_type *type);
 NORETURN
-static void die_bad_number(struct config_source *cf, const char *name,
+static void die_bad_number(struct config_reader *reader, const char *name,
 			   const char *value)
 {
 	const char *error_type = (errno == ERANGE) ?
 		N_("out of range") : N_("invalid unit");
 	const char *bad_numeric = N_("bad numeric config value '%s' for '%s': %s");
+	const char *config_name = NULL;
+	enum config_origin_type config_origin = CONFIG_ORIGIN_UNKNOWN;
 
 	if (!value)
 		value = "";
 
-	if (!(cf && cf->name))
+	/* Ignoring the return value is okay since we handle missing values. */
+	reader_config_name(reader, &config_name);
+	reader_origin_type(reader, &config_origin);
+
+	if (!config_name)
 		die(_(bad_numeric), value, name, _(error_type));
 
-	switch (cf->origin_type) {
+	switch (config_origin) {
 	case CONFIG_ORIGIN_BLOB:
 		die(_("bad numeric config value '%s' for '%s' in blob %s: %s"),
-		    value, name, cf->name, _(error_type));
+		    value, name, config_name, _(error_type));
 	case CONFIG_ORIGIN_FILE:
 		die(_("bad numeric config value '%s' for '%s' in file %s: %s"),
-		    value, name, cf->name, _(error_type));
+		    value, name, config_name, _(error_type));
 	case CONFIG_ORIGIN_STDIN:
 		die(_("bad numeric config value '%s' for '%s' in standard input: %s"),
 		    value, name, _(error_type));
 	case CONFIG_ORIGIN_SUBMODULE_BLOB:
 		die(_("bad numeric config value '%s' for '%s' in submodule-blob %s: %s"),
-		    value, name, cf->name, _(error_type));
+		    value, name, config_name, _(error_type));
 	case CONFIG_ORIGIN_CMDLINE:
 		die(_("bad numeric config value '%s' for '%s' in command line %s: %s"),
-		    value, name, cf->name, _(error_type));
+		    value, name, config_name, _(error_type));
 	default:
 		die(_("bad numeric config value '%s' for '%s' in %s: %s"),
-		    value, name, cf->name, _(error_type));
+		    value, name, config_name, _(error_type));
 	}
 }
 
@@ -1351,7 +1360,7 @@ int git_config_int(const char *name, const char *value)
 {
 	int ret;
 	if (!git_parse_int(value, &ret))
-		die_bad_number(the_reader.source, name, value);
+		die_bad_number(&the_reader, name, value);
 	return ret;
 }
 
@@ -1359,7 +1368,7 @@ int64_t git_config_int64(const char *name, const char *value)
 {
 	int64_t ret;
 	if (!git_parse_int64(value, &ret))
-		die_bad_number(the_reader.source, name, value);
+		die_bad_number(&the_reader, name, value);
 	return ret;
 }
 
@@ -1367,7 +1376,7 @@ unsigned long git_config_ulong(const char *name, const char *value)
 {
 	unsigned long ret;
 	if (!git_parse_ulong(value, &ret))
-		die_bad_number(the_reader.source, name, value);
+		die_bad_number(&the_reader, name, value);
 	return ret;
 }
 
@@ -1375,7 +1384,7 @@ ssize_t git_config_ssize_t(const char *name, const char *value)
 {
 	ssize_t ret;
 	if (!git_parse_ssize_t(value, &ret))
-		die_bad_number(the_reader.source, name, value);
+		die_bad_number(&the_reader, name, value);
 	return ret;
 }
 
@@ -3839,14 +3848,23 @@ int parse_config_key(const char *var,
 	return 0;
 }
 
-const char *current_config_origin_type(void)
+static int reader_origin_type(struct config_reader *reader,
+			      enum config_origin_type *type)
 {
-	int type;
 	if (the_reader.config_kvi)
-		type = the_reader.config_kvi->origin_type;
+		*type = reader->config_kvi->origin_type;
 	else if(the_reader.source)
-		type = the_reader.source->origin_type;
+		*type = reader->source->origin_type;
 	else
+		return 1;
+	return 0;
+}
+
+const char *current_config_origin_type(void)
+{
+	enum config_origin_type type = CONFIG_ORIGIN_UNKNOWN;
+
+	if (reader_origin_type(&the_reader, &type))
 		BUG("current_config_origin_type called outside config callback");
 
 	switch (type) {
@@ -3885,14 +3903,21 @@ const char *config_scope_name(enum config_scope scope)
 	}
 }
 
-const char *current_config_name(void)
+static int reader_config_name(struct config_reader *reader, const char **out)
 {
-	const char *name;
 	if (the_reader.config_kvi)
-		name = the_reader.config_kvi->filename;
+		*out = reader->config_kvi->filename;
 	else if (the_reader.source)
-		name = the_reader.source->name;
+		*out = reader->source->name;
 	else
+		return 1;
+	return 0;
+}
+
+const char *current_config_name(void)
+{
+	const char *name;
+	if (reader_config_name(&the_reader, &name))
 		BUG("current_config_name called outside config callback");
 	return name ? name : "";
 }
diff --git a/config.h b/config.h
index 7606246531a..66c8b996e15 100644
--- a/config.h
+++ b/config.h
@@ -56,6 +56,7 @@ struct git_config_source {
 };
 
 enum config_origin_type {
+	CONFIG_ORIGIN_UNKNOWN = 0,
 	CONFIG_ORIGIN_BLOB,
 	CONFIG_ORIGIN_FILE,
 	CONFIG_ORIGIN_STDIN,
diff --git a/t/helper/test-config.c b/t/helper/test-config.c
index 4ba9eb65606..26e79168f6a 100644
--- a/t/helper/test-config.c
+++ b/t/helper/test-config.c
@@ -30,6 +30,9 @@
  * iterate -> iterate over all values using git_config(), and print some
  *            data for each
  *
+ * git_config_int -> iterate over all values using git_config() and print the
+ *                   integer value for the entered key or die
+ *
  * Examples:
  *
  * To print the value with highest priority for key "foo.bAr Baz.rock":
@@ -54,6 +57,17 @@ static int iterate_cb(const char *var, const char *value, void *data UNUSED)
 	return 0;
 }
 
+static int parse_int_cb(const char *var, const char *value, void *data)
+{
+	const char *key_to_match = data;
+
+	if (!strcmp(key_to_match, var)) {
+		int parsed = git_config_int(value, value);
+		printf("%d\n", parsed);
+	}
+	return 0;
+}
+
 static int early_config_cb(const char *var, const char *value, void *vdata)
 {
 	const char *key = vdata;
@@ -176,6 +190,9 @@ int cmd__config(int argc, const char **argv)
 	} else if (!strcmp(argv[1], "iterate")) {
 		git_config(iterate_cb, NULL);
 		goto exit0;
+	} else if (argc == 3 && !strcmp(argv[1], "git_config_int")) {
+		git_config(parse_int_cb, (void *) argv[2]);
+		goto exit0;
 	}
 
 	die("%s: Please check the syntax and the function name", argv[0]);
diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
index b38e158d3b2..9733bed30a9 100755
--- a/t/t1308-config-set.sh
+++ b/t/t1308-config-set.sh
@@ -120,6 +120,10 @@ test_expect_success 'find integer value for a key' '
 	check_config get_int lamb.chop 65
 '
 
+test_expect_success 'parse integer value during iteration' '
+	check_config git_config_int lamb.chop 65
+'
+
 test_expect_success 'find string value for a key' '
 	check_config get_string case.baz hask &&
 	check_config expect_code 1 get_string case.ba "Value not found for \"case.ba\""
@@ -134,6 +138,11 @@ test_expect_success 'find integer if value is non parse-able' '
 	check_config expect_code 128 get_int lamb.head
 '
 
+test_expect_success 'non parse-able integer value during iteration' '
+	check_config expect_code 128 git_config_int lamb.head 2>result &&
+	grep "fatal: bad numeric config value .* in file \.git/config" result
+'
+
 test_expect_success 'find bool value for the entered key' '
 	check_config get_bool goat.head 1 &&
 	check_config get_bool goat.skin 0 &&

-- 
gitgitgadget
Previous: Glen Choo via GitGitGadgetNext: Glen Choo via GitGitGadget
Message 68 of 72 in “[RFC] config.c: use struct for config reading state”
  1. 0/6 [RFC] config.c: use struct for config reading stateGlen Choo via GitGitGadget, Mar 1, 2023
  2. 2/6 config.c: don't assign to "cf" directlyGlen Choo via GitGitGadget, Mar 1, 2023
  3. 1/6 config.c: plumb config_source through static fnsGlen Choo via GitGitGadget, Mar 1, 2023
  4. Junio C HamanoMar 3, 2023
  5. 3/6 config.c: create config_reader and the_readerGlen Choo via GitGitGadget, Mar 1, 2023
  6. Junio C HamanoMar 3, 2023
  7. 5/6 config.c: remove current_config_kviGlen Choo via GitGitGadget, Mar 1, 2023
  8. Calvin WanMar 6, 2023
  9. 4/6 config.c: plumb the_reader through callbacksGlen Choo via GitGitGadget, Mar 1, 2023
  10. Ævar Arnfjörð BjarmasonMar 8, 2023
  11. Glen ChooMar 8, 2023
  12. Junio C HamanoMar 8, 2023
  13. 6/6 config.c: remove current_parsing_scopeGlen Choo via GitGitGadget, Mar 1, 2023
  14. Jonathan TanMar 6, 2023
  15. Glen ChooMar 6, 2023
  16. Jonathan TanMar 6, 2023
  17. Ævar Arnfjörð BjarmasonMar 8, 2023
  18. Glen ChooMar 8, 2023
  19. Ævar Arnfjörð BjarmasonMar 7, 2023
  20. Glen ChooMar 7, 2023
  21. Ævar Arnfjörð BjarmasonMar 7, 2023
  22. Junio C HamanoMar 7, 2023
  23. Glen ChooMar 7, 2023
  24. Ævar Arnfjörð BjarmasonMar 8, 2023
  25. Glen ChooMar 8, 2023
  26. 0/8 config.c: use struct for config reading stateGlen Choo via GitGitGadget, Mar 16, 2023
  27. 2/8 config.c: don't assign to "cf_global" directlyGlen Choo via GitGitGadget, Mar 16, 2023
  28. Jonathan TanMar 16, 2023
  29. Junio C HamanoMar 16, 2023
  30. Glen ChooMar 16, 2023
  31. 1/8 config.c: plumb config_source through static fnsGlen Choo via GitGitGadget, Mar 16, 2023
  32. Jonathan TanMar 16, 2023
  33. 3/8 config.c: create config_reader and the_readerGlen Choo via GitGitGadget, Mar 16, 2023
  34. Jonathan TanMar 16, 2023
  35. 4/8 config.c: plumb the_reader through callbacksGlen Choo via GitGitGadget, Mar 16, 2023
  36. 6/8 config.c: remove current_parsing_scopeGlen Choo via GitGitGadget, Mar 16, 2023
  37. 5/8 config.c: remove current_config_kviGlen Choo via GitGitGadget, Mar 16, 2023
  38. 7/8 config: report cached filenames in die_bad_number()Glen Choo via GitGitGadget, Mar 16, 2023
  39. Jonathan TanMar 16, 2023
  40. Glen ChooMar 16, 2023
  41. 8/8 config.c: rename "struct config_source cf"Glen Choo via GitGitGadget, Mar 16, 2023
  42. Glen ChooMar 16, 2023
  43. Jonathan TanMar 16, 2023
  44. 0/5 bypass config.c global state with configsetÆvar Arnfjörð Bjarmason, Mar 17, 2023
  45. 1/5 config.h: move up "struct key_value_info"Ævar Arnfjörð Bjarmason, Mar 17, 2023
  46. 2/5 config.c: use "enum config_origin_type", not "int"Ævar Arnfjörð Bjarmason, Mar 17, 2023
  47. 3/5 config API: add a config_origin_type_name() helperÆvar Arnfjörð Bjarmason, Mar 17, 2023
  48. 4/5 config.c: refactor configset_iter()Ævar Arnfjörð Bjarmason, Mar 17, 2023
  49. 5/5 config API: add and use a repo_config_kvi()Ævar Arnfjörð Bjarmason, Mar 17, 2023
  50. Junio C HamanoMar 17, 2023
  51. Jonathan TanMar 17, 2023
  52. Junio C HamanoMar 17, 2023
  53. Glen ChooMar 17, 2023
  54. Glen ChooMar 17, 2023
  55. Glen ChooMar 17, 2023
  56. Ævar Arnfjörð BjarmasonMar 29, 2023
  57. 0/8 config.c: use struct for config reading stateGlen Choo via GitGitGadget, Mar 28, 2023
  58. 1/8 config.c: plumb config_source through static fnsGlen Choo via GitGitGadget, Mar 28, 2023
  59. 2/8 config.c: don't assign to "cf_global" directlyGlen Choo via GitGitGadget, Mar 28, 2023
  60. 3/8 config.c: create config_reader and the_readerGlen Choo via GitGitGadget, Mar 28, 2023
  61. Ævar Arnfjörð BjarmasonMar 29, 2023
  62. Junio C HamanoMar 29, 2023
  63. Glen ChooMar 29, 2023
  64. Glen ChooMar 30, 2023
  65. 4/8 config.c: plumb the_reader through callbacksGlen Choo via GitGitGadget, Mar 28, 2023
  66. 5/8 config.c: remove current_config_kviGlen Choo via GitGitGadget, Mar 28, 2023
  67. 6/8 config.c: remove current_parsing_scopeGlen Choo via GitGitGadget, Mar 28, 2023
  68. 7/8 config: report cached filenames in die_bad_number()Glen Choo via GitGitGadget, Mar 28, 2023
  69. 8/8 config.c: rename "struct config_source cf"Glen Choo via GitGitGadget, Mar 28, 2023
  70. Glen ChooMar 28, 2023
  71. Junio C HamanoMar 28, 2023
  72. Glen ChooMar 28, 2023

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.