[v2,7/8] config: report cached filenames in die_bad_number()

Commit Message

Glen Choo March 16, 2023, 12:11 a.m. UTC

From: Glen Choo <[e-mail]>

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(). Refactoring is needed because "git config
--get[-regexp] --type=int" parses the int value _after_ parsing the
config file, which will run into the BUG().

Also, plumb "struct config_reader" into the new functions. This isn't
necessary per se, but this generalizes better, so it might help us avoid
yet another refactor.

1. https://lore.kernel.org/git/[e-mail]/

Signed-off-by: Glen Choo <[e-mail]>
---
 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(-)

Comments

Jonathan Tan March 16, 2023, 10:22 p.m. UTC | #1

Ah, thanks for spotting this bug! It is a minor one, but this now makes
me think that we should definitely do this refactoring of a struct
containing all the relevant config state and passing it to functions as
much as possible (as opposed to merely leaning towards the idea).

"Glen Choo via GitGitGadget" <[e-mail]> writes:
> 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(). Refactoring is needed because "git config
> --get[-regexp] --type=int" parses the int value _after_ parsing the
> config file, which will run into the BUG().

You say "fix this", but are there actually 2 bugs (so, "fix these")?
Firstly, that BUG() is run into when invoking "git config" the way
you describe, and secondly, die_bad_number() only reading cf and not
checking kvi to see if anything's there. (I'm not sure how to reproduce
the latter, though.)

> Also, plumb "struct config_reader" into the new functions. This isn't
> necessary per se, but this generalizes better, so it might help us avoid
> yet another refactor.

Hmm...I thought this would be desired because we don't want the_reader
to be used from non-public functions anyway, so we can just state
that that is the reason (and not worry about using future refactors as
a justification).

The code itself looks good.

Glen Choo March 16, 2023, 11:05 p.m. UTC | #2

Jonathan Tan <[e-mail]> writes:

>>                                Refactoring is needed because "git config
>> --get[-regexp] --type=int" parses the int value _after_ parsing the
>> config file, which will run into the BUG().
>
> You say "fix this", but are there actually 2 bugs (so, "fix these")?
> Firstly, that BUG() is run into when invoking "git config" the way
> you describe, and secondly, die_bad_number() only reading cf and not
> checking kvi to see if anything's there. (I'm not sure how to reproduce
> the latter, though.)

There is actually only one bug (the latter). That is tested by the new
test I added in this patch. To reproduce it, we need:

- To iterate a config_set (git_config() or repo_config() will suffice),
  in which case the config_kvi is set, but not cf.
- Then in the config_fn_t we pass to it, we call git_parse_int() on an
  invalid number, which will result in die_bad_number(), which prints
  the less specific message.

The former case isn't a bug. We never ran into the BUG() when invoking
"git config" because die_bad_number() doesn't use current_* prior to
this patch (which is where the BUG() is). t1300:'invalid unit'
demonstrates that we print the correct message (and that we don't
BUG()):

  test_expect_success 'invalid unit' '
    git config aninvalid.unit "1auto" &&
    test_cmp_config 1auto aninvalid.unit &&
    test_must_fail git config --int --get aninvalid.unit 2>actual &&
    test_i18ngrep "bad numeric config value .1auto. for .aninvalid.unit. in file .git/config: invalid unit" actual
  '


(which is a good signal that I should probably reword the commit
message)

>
>> Also, plumb "struct config_reader" into the new functions. This isn't
>> necessary per se, but this generalizes better, so it might help us avoid
>> yet another refactor.
>
> Hmm...I thought this would be desired because we don't want the_reader
> to be used from non-public functions anyway, so we can just state
> that that is the reason (and not worry about using future refactors as
> a justification).

Ah, good point, thanks.

Patch

diff --git a/config.c b/config.c
index 460326ae21e..da5f6381cde 100644
--- a/config.c
+++ b/config.c
@@ -1312,39 +1312,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));
 	}
 }
 
@@ -1352,7 +1361,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;
 }
 
@@ -1360,7 +1369,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;
 }
 
@@ -1368,7 +1377,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;
 }
 
@@ -1376,7 +1385,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;
 }
 
@@ -3840,14 +3849,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) {
@@ -3886,14 +3904,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 &&