[02/14] config.c: use kvi for CLI config

Commit Message

Glen Choo April 21, 2023, 7:13 p.m. UTC

From: Glen Choo <[e-mail]>

Plumb "struct key_value_info" and use "kvi_fn()" when parsing CLI
config. Do this by refactoring out and reusing the logic that sets the
"struct key_value_info" members when caching CLI config in a configset.

This lets us get rid of the fake "struct config_source" in
"git_config_from_parameters()", so we now only have to maintain one
implementation. Additionally, this plumbing also reveals that
"git_config_parse_parameter()" hasn't been setting either
"the_reader.source" or "the_reader.config_kvi", so any calls to
"current_*" would either BUG() or return *_UNKNOWN values.

Also, get rid of the BUG() checks that forbid setting ".config_kvi" and
".source" at the same time, since we will run afoul of that check. They
will soon be unnecessary when we remove ".source".

Signed-off-by: Glen Choo <[e-mail]>
---
 config.c | 58 ++++++++++++++++++++++++++++----------------------------
 1 file changed, 29 insertions(+), 29 deletions(-)

Comments

Ævar Arnfjörð Bjarmason May 1, 2023, 11:06 a.m. UTC | #1

On Fri, Apr 21 2023, Glen Choo via GitGitGadget wrote:

> @@ -682,17 +677,30 @@ static int config_parse_pair(const char *key, const char *value,
>  	if (git_config_parse_key(key, &canonical_name, NULL))
>  		return -1;
>  
> -	ret = (fn(canonical_name, value, data) < 0) ? -1 : 0;
> +	ret = (kvi_fn(fn, canonical_name, value, kvi, data) < 0) ? -1 : 0;
>  	free(canonical_name);
>  	return ret;
>  }

This is pre-existing, but I'd much prefer as we're doing more
formalization of this interface if this were just:

	ret = kvi_fn(...);
	...;
	return ret;

I.e. a look at the current code shows us that the API users of
git_config_parse_parameter() are already doing this coercion themselves,
i.e. they only care about "ret < 0".

So let's just hand them the actual return value, rather than doing the
needless coercion.

> @@ -2423,19 +2429,13 @@ static int configset_add_value(struct config_reader *reader,
>  	l_item->e = e;
>  	l_item->value_index = e->value_list.nr - 1;
>  
> -	if (!reader->source)
> -		BUG("configset_add_value has no source");
> -	if (reader->source->name) {
> +	if (reader->source && reader->source->name) {
>  		kv_info->filename = strintern(reader->source->name);
>  		kv_info->linenr = reader->source->linenr;
>  		kv_info->origin_type = reader->source->origin_type;
> -	} else {
> -		/* for values read from `git_config_from_parameters()` */
> -		kv_info->filename = NULL;
> -		kv_info->linenr = -1;
> -		kv_info->origin_type = CONFIG_ORIGIN_CMDLINE;
> -	}
> -	kv_info->scope = reader->parsing_scope;
> +		kv_info->scope = reader->parsing_scope;
> +	} else
> +		kvi_from_param(kv_info);

Missing a {} here.

Patch

diff --git a/config.c b/config.c
index daad892e4fd..4e9e8e7abb9 100644
--- a/config.c
+++ b/config.c
@@ -99,8 +99,6 @@  static struct config_reader the_reader;
 static inline void config_reader_push_source(struct config_reader *reader,
 					     struct config_source *top)
 {
-	if (reader->config_kvi)
-		BUG("source should not be set while iterating a config set");
 	top->prev = reader->source;
 	reader->source = top;
 }
@@ -118,16 +116,12 @@  static inline struct config_source *config_reader_pop_source(struct config_reade
 static inline void config_reader_set_kvi(struct config_reader *reader,
 					 struct key_value_info *kvi)
 {
-	if (kvi && (reader->source || reader->parsing_scope))
-		BUG("kvi should not be set while parsing a config source");
 	reader->config_kvi = kvi;
 }
 
 static inline void config_reader_set_scope(struct config_reader *reader,
 					   enum config_scope scope)
 {
-	if (scope && reader->config_kvi)
-		BUG("scope should only be set when iterating through a config source");
 	reader->parsing_scope = scope;
 }
 
@@ -672,7 +666,8 @@  static int kvi_fn(config_fn_t fn, const char *key, const char *value,
 }
 
 static int config_parse_pair(const char *key, const char *value,
-			  config_fn_t fn, void *data)
+			     struct key_value_info *kvi,
+			     config_fn_t fn, void *data)
 {
 	char *canonical_name;
 	int ret;
@@ -682,17 +677,30 @@  static int config_parse_pair(const char *key, const char *value,
 	if (git_config_parse_key(key, &canonical_name, NULL))
 		return -1;
 
-	ret = (fn(canonical_name, value, data) < 0) ? -1 : 0;
+	ret = (kvi_fn(fn, canonical_name, value, kvi, data) < 0) ? -1 : 0;
 	free(canonical_name);
 	return ret;
 }
 
+
+/* for values read from `git_config_from_parameters()` */
+static void kvi_from_param(struct key_value_info *out)
+{
+	out->filename = NULL;
+	out->linenr = -1;
+	out->origin_type = CONFIG_ORIGIN_CMDLINE;
+	out->scope = CONFIG_SCOPE_COMMAND;
+}
+
 int git_config_parse_parameter(const char *text,
 			       config_fn_t fn, void *data)
 {
 	const char *value;
 	struct strbuf **pair;
 	int ret;
+	struct key_value_info kvi = { 0 };
+
+	kvi_from_param(&kvi);
 
 	pair = strbuf_split_str(text, '=', 2);
 	if (!pair[0])
@@ -711,12 +719,13 @@  int git_config_parse_parameter(const char *text,
 		return error(_("bogus config parameter: %s"), text);
 	}
 
-	ret = config_parse_pair(pair[0]->buf, value, fn, data);
+	ret = config_parse_pair(pair[0]->buf, value, &kvi, fn, data);
 	strbuf_list_free(pair);
 	return ret;
 }
 
-static int parse_config_env_list(char *env, config_fn_t fn, void *data)
+static int parse_config_env_list(char *env, struct key_value_info *kvi,
+				 config_fn_t fn, void *data)
 {
 	char *cur = env;
 	while (cur && *cur) {
@@ -750,7 +759,7 @@  static int parse_config_env_list(char *env, config_fn_t fn, void *data)
 					     CONFIG_DATA_ENVIRONMENT);
 			}
 
-			if (config_parse_pair(key, value, fn, data) < 0)
+			if (config_parse_pair(key, value, kvi, fn, data) < 0)
 				return -1;
 		}
 		else {
@@ -774,11 +783,9 @@  int git_config_from_parameters(config_fn_t fn, void *data)
 	struct strvec to_free = STRVEC_INIT;
 	int ret = 0;
 	char *envw = NULL;
-	struct config_source source = CONFIG_SOURCE_INIT;
-
-	source.origin_type = CONFIG_ORIGIN_CMDLINE;
-	config_reader_push_source(&the_reader, &source);
+	struct key_value_info kvi = { 0 };
 
+	kvi_from_param(&kvi);
 	env = getenv(CONFIG_COUNT_ENVIRONMENT);
 	if (env) {
 		unsigned long count;
@@ -814,7 +821,7 @@  int git_config_from_parameters(config_fn_t fn, void *data)
 			}
 			strbuf_reset(&envvar);
 
-			if (config_parse_pair(key, value, fn, data) < 0) {
+			if (config_parse_pair(key, value, &kvi, fn, data) < 0) {
 				ret = -1;
 				goto out;
 			}
@@ -825,7 +832,7 @@  int git_config_from_parameters(config_fn_t fn, void *data)
 	if (env) {
 		/* sq_dequote will write over it */
 		envw = xstrdup(env);
-		if (parse_config_env_list(envw, fn, data) < 0) {
+		if (parse_config_env_list(envw, &kvi, fn, data) < 0) {
 			ret = -1;
 			goto out;
 		}
@@ -835,7 +842,6 @@  out:
 	strbuf_release(&envvar);
 	strvec_clear(&to_free);
 	free(envw);
-	config_reader_pop_source(&the_reader);
 	return ret;
 }
 
@@ -2241,7 +2247,7 @@  static int do_git_config_sequence(struct config_reader *reader,
 		free(path);
 	}
 
-	config_reader_set_scope(reader, CONFIG_SCOPE_COMMAND);
+	config_reader_set_scope(reader, 0);
 	if (!opts->ignore_cmdline && git_config_from_parameters(fn, data) < 0)
 		die(_("unable to parse command-line config"));
 
@@ -2423,19 +2429,13 @@  static int configset_add_value(struct config_reader *reader,
 	l_item->e = e;
 	l_item->value_index = e->value_list.nr - 1;
 
-	if (!reader->source)
-		BUG("configset_add_value has no source");
-	if (reader->source->name) {
+	if (reader->source && reader->source->name) {
 		kv_info->filename = strintern(reader->source->name);
 		kv_info->linenr = reader->source->linenr;
 		kv_info->origin_type = reader->source->origin_type;
-	} else {
-		/* for values read from `git_config_from_parameters()` */
-		kv_info->filename = NULL;
-		kv_info->linenr = -1;
-		kv_info->origin_type = CONFIG_ORIGIN_CMDLINE;
-	}
-	kv_info->scope = reader->parsing_scope;
+		kv_info->scope = reader->parsing_scope;
+	} else
+		kvi_from_param(kv_info);
 	si->util = kv_info;
 
 	return 0;