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

[PATCH v3 3/8] config.c: create config_reader and the_reader

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

Create "struct config_reader" to hold the state of the config source currently being read. Then, create a static instance of it, "the_reader", and use "the_reader.source" to replace references to "cf_global" in public functions.

This doesn't create much immediate benefit (since we're mostly replacing static variables with a bigger static variable), but it prepares us for a future where this state doesn't have to be global; "struct config_reader" (or a similar struct) could be provided by the caller, or constructed internally by a function like "do_config_from()".

A more typical approach would be to put this struct on "the_repository", but that's a worse fit for this use case since config reading is not scoped to a repository. E.g. we can read config before the repository is known ("read_very_early_config()"), blatantly ignore the repo ("read_protected_config()"), or read only from a file ("git_config_from_file()"). This is especially evident in t5318 and t9210, where test-tool and scalar parse config but don't fully initialize "the_repository".

We could have also replaced the references to "cf_global" in callback functions (which are the only ones left), but we'll eventually plumb "the_reader" through the callback "*data" arg, so that would be unnecessary churn. Until we remove "cf_global" altogether, add logic to "config_reader_*_source()" to keep "cf_global" and "the_reader.source" in sync.

Signed-off-by: Glen Choo <chooglen@google.com>
---
 config.c | 82 +++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 51 insertions(+), 31 deletions(-)
diff --git a/config.c b/config.c
index 6627fad71cf..3a28b397c4d 100644
--- a/config.c
+++ b/config.c
@@ -51,6 +51,16 @@ struct config_source {
 };
 #define CONFIG_SOURCE_INIT { 0 }
 
+struct config_reader {
+	struct config_source *source;
+};
+/*
+ * Where possible, prefer to accept "struct config_reader" as an arg than to use
+ * "the_reader". "the_reader" should only be used if that is infeasible, e.g. in
+ * a public function.
+ */
+static struct config_reader the_reader;
+
 /*
  * These variables record the "current" config source, which
  * can be accessed by parsing callbacks.
@@ -66,6 +76,9 @@ struct config_source {
  * 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).
+ *
+ * FIXME "cf_global" has been replaced by "the_reader.source", remove
+ * "cf_global" once we plumb "the_reader" through all of the callback functions.
  */
 static struct config_source *cf_global;
 static struct key_value_info *current_config_kvi;
@@ -80,19 +93,24 @@ static struct key_value_info *current_config_kvi;
  */
 static enum config_scope current_parsing_scope;
 
-static inline void config_reader_push_source(struct config_source *top)
+static inline void config_reader_push_source(struct config_reader *reader,
+					     struct config_source *top)
 {
-	top->prev = cf_global;
-	cf_global = top;
+	top->prev = reader->source;
+	reader->source = top;
+	/* FIXME remove this when cf_global is removed. */
+	cf_global = reader->source;
 }
 
-static inline struct config_source *config_reader_pop_source()
+static inline struct config_source *config_reader_pop_source(struct config_reader *reader)
 {
 	struct config_source *ret;
-	if (!cf_global)
+	if (!reader->source)
 		BUG("tried to pop config source, but we weren't reading config");
-	ret = cf_global;
-	cf_global = cf_global->prev;
+	ret = reader->source;
+	reader->source = reader->source->prev;
+	/* FIXME remove this when cf_global is removed. */
+	cf_global = reader->source;
 	return ret;
 }
 
@@ -732,7 +750,7 @@ int git_config_from_parameters(config_fn_t fn, void *data)
 	struct config_source source = CONFIG_SOURCE_INIT;
 
 	source.origin_type = CONFIG_ORIGIN_CMDLINE;
-	config_reader_push_source(&source);
+	config_reader_push_source(&the_reader, &source);
 
 	env = getenv(CONFIG_COUNT_ENVIRONMENT);
 	if (env) {
@@ -790,7 +808,7 @@ out:
 	strbuf_release(&envvar);
 	strvec_clear(&to_free);
 	free(envw);
-	config_reader_pop_source();
+	config_reader_pop_source(&the_reader);
 	return ret;
 }
 
@@ -1325,7 +1343,7 @@ int git_config_int(const char *name, const char *value)
 {
 	int ret;
 	if (!git_parse_int(value, &ret))
-		die_bad_number(cf_global, name, value);
+		die_bad_number(the_reader.source, name, value);
 	return ret;
 }
 
@@ -1333,7 +1351,7 @@ int64_t git_config_int64(const char *name, const char *value)
 {
 	int64_t ret;
 	if (!git_parse_int64(value, &ret))
-		die_bad_number(cf_global, name, value);
+		die_bad_number(the_reader.source, name, value);
 	return ret;
 }
 
@@ -1341,7 +1359,7 @@ unsigned long git_config_ulong(const char *name, const char *value)
 {
 	unsigned long ret;
 	if (!git_parse_ulong(value, &ret))
-		die_bad_number(cf_global, name, value);
+		die_bad_number(the_reader.source, name, value);
 	return ret;
 }
 
@@ -1349,7 +1367,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(cf_global, name, value);
+		die_bad_number(the_reader.source, name, value);
 	return ret;
 }
 
@@ -1955,7 +1973,8 @@ int git_default_config(const char *var, const char *value, void *cb)
  * fgetc, ungetc, ftell of top need to be initialized before calling
  * this function.
  */
-static int do_config_from(struct config_source *top, config_fn_t fn, void *data,
+static int do_config_from(struct config_reader *reader,
+			  struct config_source *top, config_fn_t fn, void *data,
 			  const struct config_options *opts)
 {
 	int ret;
@@ -1966,22 +1985,23 @@ static int do_config_from(struct config_source *top, config_fn_t fn, void *data,
 	top->total_len = 0;
 	strbuf_init(&top->value, 1024);
 	strbuf_init(&top->var, 1024);
-	config_reader_push_source(top);
+	config_reader_push_source(reader, top);
 
 	ret = git_parse_source(top, fn, data, opts);
 
 	/* pop config-file parsing state stack */
 	strbuf_release(&top->value);
 	strbuf_release(&top->var);
-	config_reader_pop_source();
+	config_reader_pop_source(reader);
 
 	return ret;
 }
 
-static int do_config_from_file(config_fn_t fn,
-		const enum config_origin_type origin_type,
-		const char *name, const char *path, FILE *f,
-		void *data, const struct config_options *opts)
+static int do_config_from_file(struct config_reader *reader,
+			       config_fn_t fn,
+			       const enum config_origin_type origin_type,
+			       const char *name, const char *path, FILE *f,
+			       void *data, const struct config_options *opts)
 {
 	struct config_source top = CONFIG_SOURCE_INIT;
 	int ret;
@@ -1996,15 +2016,15 @@ static int do_config_from_file(config_fn_t fn,
 	top.do_ftell = config_file_ftell;
 
 	flockfile(f);
-	ret = do_config_from(&top, fn, data, opts);
+	ret = do_config_from(reader, &top, fn, data, opts);
 	funlockfile(f);
 	return ret;
 }
 
 static int git_config_from_stdin(config_fn_t fn, void *data)
 {
-	return do_config_from_file(fn, CONFIG_ORIGIN_STDIN, "", NULL, stdin,
-				   data, NULL);
+	return do_config_from_file(&the_reader, fn, CONFIG_ORIGIN_STDIN, "",
+				   NULL, stdin, data, NULL);
 }
 
 int git_config_from_file_with_options(config_fn_t fn, const char *filename,
@@ -2018,8 +2038,8 @@ int git_config_from_file_with_options(config_fn_t fn, const char *filename,
 		BUG("filename cannot be NULL");
 	f = fopen_or_warn(filename, "r");
 	if (f) {
-		ret = do_config_from_file(fn, CONFIG_ORIGIN_FILE, filename,
-					  filename, f, data, opts);
+		ret = do_config_from_file(&the_reader, fn, CONFIG_ORIGIN_FILE,
+					  filename, filename, f, data, opts);
 		fclose(f);
 	}
 	return ret;
@@ -2048,7 +2068,7 @@ int git_config_from_mem(config_fn_t fn,
 	top.do_ungetc = config_buf_ungetc;
 	top.do_ftell = config_buf_ftell;
 
-	return do_config_from(&top, fn, data, opts);
+	return do_config_from(&the_reader, &top, fn, data, opts);
 }
 
 int git_config_from_blob_oid(config_fn_t fn,
@@ -3797,8 +3817,8 @@ const char *current_config_origin_type(void)
 	int type;
 	if (current_config_kvi)
 		type = current_config_kvi->origin_type;
-	else if(cf_global)
-		type = cf_global->origin_type;
+	else if(the_reader.source)
+		type = the_reader.source->origin_type;
 	else
 		BUG("current_config_origin_type called outside config callback");
 
@@ -3843,8 +3863,8 @@ const char *current_config_name(void)
 	const char *name;
 	if (current_config_kvi)
 		name = current_config_kvi->filename;
-	else if (cf_global)
-		name = cf_global->name;
+	else if (the_reader.source)
+		name = the_reader.source->name;
 	else
 		BUG("current_config_name called outside config callback");
 	return name ? name : "";
@@ -3863,7 +3883,7 @@ int current_config_line(void)
 	if (current_config_kvi)
 		return current_config_kvi->linenr;
 	else
-		return cf_global->linenr;
+		return the_reader.source->linenr;
 }
 
 int lookup_config(const char **mapping, int nr_mapping, const char *var)

-- 
gitgitgadget
Previous: Glen Choo via GitGitGadgetNext: Ævar Arnfjörð Bjarmason
Message 60 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.