diff mbox series

[5/6] config.c: remove current_config_kvi

Message ID c05b33ab29dbce931f9d0ad968fe9eeedc653456.1677631097.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series config.c: use struct for config reading state | expand

Commit Message

Glen Choo March 1, 2023, 12:38 a.m. UTC 
From: Glen Choo <chooglen@google.com>

Add ".config_kvi" to "struct config_reader" and replace
"current_config_kvi" with "the_reader.config_kvi", either in-place (in
public functions) or by passing "the_reader" to the "*data" arg of
callback functions.

Also, introduce a setter function for ".config_kvi", which allows us to
enforce the contraint that only one of ".source" and ".config_kvi" can
be set at a time (as documented in the comments). Because of this
constraint, we know that "populate_remote_urls()" was never touching
"current_config_kvi" when iterating through config files, so it doesn't
need to store and restore that value.

Signed-off-by: Glen Choo <chooglen@google.com>
---
 config.c | 103 ++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 61 insertions(+), 42 deletions(-)

Comments

Calvin Wan March 6, 2023, 8:12 p.m. UTC | #1 
"Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Glen Choo <chooglen@google.com>
> 
> Add ".config_kvi" to "struct config_reader" and replace
> "current_config_kvi" with "the_reader.config_kvi", either in-place (in
> public functions) or by passing "the_reader" to the "*data" arg of
> callback functions.
> 
> Also, introduce a setter function for ".config_kvi", which allows us to
> enforce the contraint that only one of ".source" and ".config_kvi" can
> be set at a time (as documented in the comments). Because of this
> constraint, we know that "populate_remote_urls()" was never touching
> "current_config_kvi" when iterating through config files, so it doesn't
> need to store and restore that value.
> 
> Signed-off-by: Glen Choo <chooglen@google.com>
> ---
>  config.c | 103 ++++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 61 insertions(+), 42 deletions(-)
> 
> diff --git a/config.c b/config.c
> index 9676734b1b7..c7995148165 100644
> --- a/config.c
> +++ b/config.c
> @@ -52,32 +52,28 @@ struct config_source {
>  #define CONFIG_SOURCE_INIT { 0 }
>  
>  struct config_reader {
> +	/*
> +	 * These members record the "current" config source, which can be
> +	 * accessed by parsing callbacks.
> +	 *
> +	 * The "source" variable will be non-NULL only when we are actually
> +	 * parsing a real config source (file, blob, cmdline, etc).
> +	 *
> +	 * The "config_kvi" variable will be non-NULL only when we are feeding
> +	 * cached config from a configset into a callback.
> +	 *
> +	 * They cannot be non-NULL at the same time. If they are both NULL, then
> +	 * we aren't parsing anything (and depending on the function looking at
> +	 * the variables, it's either a bug for it to be called in the first
> +	 * place, or it's a function which can be reused for non-config
> +	 * purposes, and should fall back to some sane behavior).
> +	 */
>  	struct config_source *source;
> +	struct key_value_info *config_kvi;
>  };
>  /* Only public functions should reference the_reader. */
>  static struct config_reader the_reader;
>  
> -/*
> - * FIXME The comments are temporarily out of date since "cf" been moved to
> - * the_reader, but not current_*.
> - *
> - * These variables record the "current" config source, which
> - * can be accessed by parsing callbacks.
> - *
> - * The "cf" variable will be non-NULL only when we are actually parsing a real
> - * config source (file, blob, cmdline, etc).
> - *
> - * The "current_config_kvi" variable will be non-NULL only when we are feeding
> - * cached config from a configset into a callback.
> - *
> - * They should generally never be non-NULL at the same time. If they are both
> - * NULL, then we aren't parsing anything (and depending on the function looking
> - * at the variables, it's either a bug for it to be called in the first place,
> - * or it's a function which can be reused for non-config purposes, and should
> - * fall back to some sane behavior).
> - */
> -static struct key_value_info *current_config_kvi;
> -
>  /*
>   * Similar to the variables above, this gives access to the "scope" of the
>   * current value (repo, global, etc). For cached values, it can be found via
> @@ -90,6 +86,8 @@ static enum config_scope current_parsing_scope;
>  static inline void config_reader_push_source(struct config_reader *reader,
>  					     struct config_source *top)
>  {
> +	if (reader->config_kvi)
> +		BUG("source should only be set when parsing a config source");
>  	if (reader->source)
>  		top->prev = reader->source;
>  	reader->source = top;

A developer who accidentally sets config_kvi probably has the right
intention of trying to push source. I think a clearer BUG message here
(which also doubly functions as a useful comment) would be something
along the lines of: "configset should not be set when parsing a config
source".

> @@ -105,6 +103,14 @@ static inline struct config_source *config_reader_pop_source(struct config_reade
>  	return ret;
>  }
>  
> +static inline void config_reader_set_kvi(struct config_reader *reader,
> +					 struct key_value_info *kvi)
> +{
> +	if (kvi && reader->source)
> +		BUG("kvi should only be set when iterating through configset");
> +	reader->config_kvi = kvi;
> +}
> +

ditto

>  static int pack_compression_seen;
>  static int zlib_compression_seen;
>  
> @@ -373,20 +379,17 @@ static void populate_remote_urls(struct config_include_data *inc)
>  {
>  	struct config_options opts;
>  
> -	struct key_value_info *store_kvi = current_config_kvi;
>  	enum config_scope store_scope = current_parsing_scope;
>  
>  	opts = *inc->opts;
>  	opts.unconditional_remote_url = 1;
>  
> -	current_config_kvi = NULL;
>  	current_parsing_scope = 0;
>  
>  	inc->remote_urls = xmalloc(sizeof(*inc->remote_urls));
>  	string_list_init_dup(inc->remote_urls);
>  	config_with_options(add_remote_url, inc->remote_urls, inc->config_source, &opts);
>  
> -	current_config_kvi = store_kvi;
>  	current_parsing_scope = store_scope;
>  }
>  
> @@ -2253,26 +2256,34 @@ int config_with_options(config_fn_t fn, void *data,
>  	return ret;
>  }
>  
> +struct configset_iter_data {
> +	struct config_reader *config_reader;
> +	void *inner;
> +};
> +#define CONFIGSET_ITER_INIT { 0 }
> +
>  static void configset_iter(struct config_set *cs, config_fn_t fn, void *data)
>  {
>  	int i, value_index;
>  	struct string_list *values;
>  	struct config_set_element *entry;
>  	struct configset_list *list = &cs->list;
> +	struct configset_iter_data *iter_data = data;
>  
>  	for (i = 0; i < list->nr; i++) {
> +		struct key_value_info *kvi;
>  		entry = list->items[i].e;
>  		value_index = list->items[i].value_index;
>  		values = &entry->value_list;
>  
> -		current_config_kvi = values->items[value_index].util;
> +		kvi = values->items[value_index].util;
> +		config_reader_set_kvi(iter_data->config_reader, kvi);
>  
> -		if (fn(entry->key, values->items[value_index].string, data) < 0)
> -			git_die_config_linenr(entry->key,
> -					      current_config_kvi->filename,
> -					      current_config_kvi->linenr);
> +		if (fn(entry->key, values->items[value_index].string, iter_data->inner) < 0)
> +			git_die_config_linenr(entry->key, kvi->filename,
> +					      kvi->linenr);
>  
> -		current_config_kvi = NULL;
> +		config_reader_set_kvi(iter_data->config_reader, NULL);
>  	}
>  }
>  
> @@ -2607,10 +2618,14 @@ static void repo_config_clear(struct repository *repo)
>  	git_configset_clear(repo->config);
>  }
>  
> -void repo_config(struct repository *repo, config_fn_t fn, void *data)
> +void repo_config(struct repository *repo, config_fn_t fn, void *data_inner)
>  {
> +	struct configset_iter_data data = CONFIGSET_ITER_INIT;
> +	data.inner = data_inner;
> +	data.config_reader = &the_reader;
> +
>  	git_config_check_init(repo);
> -	configset_iter(repo->config, fn, data);
> +	configset_iter(repo->config, fn, &data);
>  }

I'm not sure I agree with changing *data to *data_inner. An API caller
would be wondering why this function has a *data_inner signature vs
*data for other functions in config. You could instead change `struct
configset_iter_data data` to `struct configset_iter_data data_outer` to
preserve the function signature.

>  
>  int repo_config_get_value(struct repository *repo,
> @@ -2712,11 +2727,15 @@ static void read_protected_config(void)
>  	config_with_options(config_set_callback, &data, NULL, &opts);
>  }
>  
> -void git_protected_config(config_fn_t fn, void *data)
> +void git_protected_config(config_fn_t fn, void *data_inner)
>  {
> +	struct configset_iter_data data = CONFIGSET_ITER_INIT;
>  	if (!protected_config.hash_initialized)
>  		read_protected_config();
> -	configset_iter(&protected_config, fn, data);
> +	data.inner = data_inner;
> +	data.config_reader = &the_reader;
> +
> +	configset_iter(&protected_config, fn, &data);
>  }

ditto

>  
>  /* Functions used historically to read configuration from 'the_repository' */
> @@ -3823,8 +3842,8 @@ int parse_config_key(const char *var,
>  const char *current_config_origin_type(void)
>  {
>  	int type;
> -	if (current_config_kvi)
> -		type = current_config_kvi->origin_type;
> +	if (the_reader.config_kvi)
> +		type = the_reader.config_kvi->origin_type;
>  	else if(the_reader.source)
>  		type = the_reader.source->origin_type;
>  	else
> @@ -3869,8 +3888,8 @@ const char *config_scope_name(enum config_scope scope)
>  const char *current_config_name(void)
>  {
>  	const char *name;
> -	if (current_config_kvi)
> -		name = current_config_kvi->filename;
> +	if (the_reader.config_kvi)
> +		name = the_reader.config_kvi->filename;
>  	else if (the_reader.source)
>  		name = the_reader.source->name;
>  	else
> @@ -3880,16 +3899,16 @@ const char *current_config_name(void)
>  
>  enum config_scope current_config_scope(void)
>  {
> -	if (current_config_kvi)
> -		return current_config_kvi->scope;
> +	if (the_reader.config_kvi)
> +		return the_reader.config_kvi->scope;
>  	else
>  		return current_parsing_scope;
>  }
>  
>  int current_config_line(void)
>  {
> -	if (current_config_kvi)
> -		return current_config_kvi->linenr;
> +	if (the_reader.config_kvi)
> +		return the_reader.config_kvi->linenr;
>  	else
>  		return the_reader.source->linenr;
>  }
> -- 
> gitgitgadget
> 
> 
>
diff mbox series

Patch

diff --git a/config.c b/config.c
index 9676734b1b7..c7995148165 100644
--- a/config.c
+++ b/config.c
@@ -52,32 +52,28 @@  struct config_source {
 #define CONFIG_SOURCE_INIT { 0 }
 
 struct config_reader {
+	/*
+	 * These members record the "current" config source, which can be
+	 * accessed by parsing callbacks.
+	 *
+	 * The "source" variable will be non-NULL only when we are actually
+	 * parsing a real config source (file, blob, cmdline, etc).
+	 *
+	 * The "config_kvi" variable will be non-NULL only when we are feeding
+	 * cached config from a configset into a callback.
+	 *
+	 * They cannot be non-NULL at the same time. If they are both NULL, then
+	 * we aren't parsing anything (and depending on the function looking at
+	 * the variables, it's either a bug for it to be called in the first
+	 * place, or it's a function which can be reused for non-config
+	 * purposes, and should fall back to some sane behavior).
+	 */
 	struct config_source *source;
+	struct key_value_info *config_kvi;
 };
 /* Only public functions should reference the_reader. */
 static struct config_reader the_reader;
 
-/*
- * FIXME The comments are temporarily out of date since "cf" been moved to
- * the_reader, but not current_*.
- *
- * These variables record the "current" config source, which
- * can be accessed by parsing callbacks.
- *
- * The "cf" variable will be non-NULL only when we are actually parsing a real
- * config source (file, blob, cmdline, etc).
- *
- * The "current_config_kvi" variable will be non-NULL only when we are feeding
- * cached config from a configset into a callback.
- *
- * They should generally never be non-NULL at the same time. If they are both
- * NULL, then we aren't parsing anything (and depending on the function looking
- * at the variables, it's either a bug for it to be called in the first place,
- * or it's a function which can be reused for non-config purposes, and should
- * fall back to some sane behavior).
- */
-static struct key_value_info *current_config_kvi;
-
 /*
  * Similar to the variables above, this gives access to the "scope" of the
  * current value (repo, global, etc). For cached values, it can be found via
@@ -90,6 +86,8 @@  static enum config_scope current_parsing_scope;
 static inline void config_reader_push_source(struct config_reader *reader,
 					     struct config_source *top)
 {
+	if (reader->config_kvi)
+		BUG("source should only be set when parsing a config source");
 	if (reader->source)
 		top->prev = reader->source;
 	reader->source = top;
@@ -105,6 +103,14 @@  static inline struct config_source *config_reader_pop_source(struct config_reade
 	return ret;
 }
 
+static inline void config_reader_set_kvi(struct config_reader *reader,
+					 struct key_value_info *kvi)
+{
+	if (kvi && reader->source)
+		BUG("kvi should only be set when iterating through configset");
+	reader->config_kvi = kvi;
+}
+
 static int pack_compression_seen;
 static int zlib_compression_seen;
 
@@ -373,20 +379,17 @@  static void populate_remote_urls(struct config_include_data *inc)
 {
 	struct config_options opts;
 
-	struct key_value_info *store_kvi = current_config_kvi;
 	enum config_scope store_scope = current_parsing_scope;
 
 	opts = *inc->opts;
 	opts.unconditional_remote_url = 1;
 
-	current_config_kvi = NULL;
 	current_parsing_scope = 0;
 
 	inc->remote_urls = xmalloc(sizeof(*inc->remote_urls));
 	string_list_init_dup(inc->remote_urls);
 	config_with_options(add_remote_url, inc->remote_urls, inc->config_source, &opts);
 
-	current_config_kvi = store_kvi;
 	current_parsing_scope = store_scope;
 }
 
@@ -2253,26 +2256,34 @@  int config_with_options(config_fn_t fn, void *data,
 	return ret;
 }
 
+struct configset_iter_data {
+	struct config_reader *config_reader;
+	void *inner;
+};
+#define CONFIGSET_ITER_INIT { 0 }
+
 static void configset_iter(struct config_set *cs, config_fn_t fn, void *data)
 {
 	int i, value_index;
 	struct string_list *values;
 	struct config_set_element *entry;
 	struct configset_list *list = &cs->list;
+	struct configset_iter_data *iter_data = data;
 
 	for (i = 0; i < list->nr; i++) {
+		struct key_value_info *kvi;
 		entry = list->items[i].e;
 		value_index = list->items[i].value_index;
 		values = &entry->value_list;
 
-		current_config_kvi = values->items[value_index].util;
+		kvi = values->items[value_index].util;
+		config_reader_set_kvi(iter_data->config_reader, kvi);
 
-		if (fn(entry->key, values->items[value_index].string, data) < 0)
-			git_die_config_linenr(entry->key,
-					      current_config_kvi->filename,
-					      current_config_kvi->linenr);
+		if (fn(entry->key, values->items[value_index].string, iter_data->inner) < 0)
+			git_die_config_linenr(entry->key, kvi->filename,
+					      kvi->linenr);
 
-		current_config_kvi = NULL;
+		config_reader_set_kvi(iter_data->config_reader, NULL);
 	}
 }
 
@@ -2607,10 +2618,14 @@  static void repo_config_clear(struct repository *repo)
 	git_configset_clear(repo->config);
 }
 
-void repo_config(struct repository *repo, config_fn_t fn, void *data)
+void repo_config(struct repository *repo, config_fn_t fn, void *data_inner)
 {
+	struct configset_iter_data data = CONFIGSET_ITER_INIT;
+	data.inner = data_inner;
+	data.config_reader = &the_reader;
+
 	git_config_check_init(repo);
-	configset_iter(repo->config, fn, data);
+	configset_iter(repo->config, fn, &data);
 }
 
 int repo_config_get_value(struct repository *repo,
@@ -2712,11 +2727,15 @@  static void read_protected_config(void)
 	config_with_options(config_set_callback, &data, NULL, &opts);
 }
 
-void git_protected_config(config_fn_t fn, void *data)
+void git_protected_config(config_fn_t fn, void *data_inner)
 {
+	struct configset_iter_data data = CONFIGSET_ITER_INIT;
 	if (!protected_config.hash_initialized)
 		read_protected_config();
-	configset_iter(&protected_config, fn, data);
+	data.inner = data_inner;
+	data.config_reader = &the_reader;
+
+	configset_iter(&protected_config, fn, &data);
 }
 
 /* Functions used historically to read configuration from 'the_repository' */
@@ -3823,8 +3842,8 @@  int parse_config_key(const char *var,
 const char *current_config_origin_type(void)
 {
 	int type;
-	if (current_config_kvi)
-		type = current_config_kvi->origin_type;
+	if (the_reader.config_kvi)
+		type = the_reader.config_kvi->origin_type;
 	else if(the_reader.source)
 		type = the_reader.source->origin_type;
 	else
@@ -3869,8 +3888,8 @@  const char *config_scope_name(enum config_scope scope)
 const char *current_config_name(void)
 {
 	const char *name;
-	if (current_config_kvi)
-		name = current_config_kvi->filename;
+	if (the_reader.config_kvi)
+		name = the_reader.config_kvi->filename;
 	else if (the_reader.source)
 		name = the_reader.source->name;
 	else
@@ -3880,16 +3899,16 @@  const char *current_config_name(void)
 
 enum config_scope current_config_scope(void)
 {
-	if (current_config_kvi)
-		return current_config_kvi->scope;
+	if (the_reader.config_kvi)
+		return the_reader.config_kvi->scope;
 	else
 		return current_parsing_scope;
 }
 
 int current_config_line(void)
 {
-	if (current_config_kvi)
-		return current_config_kvi->linenr;
+	if (the_reader.config_kvi)
+		return the_reader.config_kvi->linenr;
 	else
 		return the_reader.source->linenr;
 }