[v3,1/5] config: split out config_parse_options

Commit Message

Josh Steadmon Sept. 21, 2023, 9:17 p.m. UTC

From: Glen Choo <[e-mail]>

"struct config_options" is a disjoint set of options used by the config
parser (e.g. event listeners) and options used by config_with_options()
(e.g. to handle includes, choose which config files to parse). Split
parser-only options into config_parse_options.

Signed-off-by: Glen Choo <[e-mail]>
Signed-off-by: Josh Steadmon <[e-mail]>
---
 bundle-uri.c |  2 +-
 config.c     | 14 +++++++-------
 config.h     | 37 ++++++++++++++++++++-----------------
 fsck.c       |  2 +-
 4 files changed, 29 insertions(+), 26 deletions(-)

Comments

Jonathan Tan Oct. 23, 2023, 5:52 p.m. UTC | #1

Josh Steadmon <[e-mail]> writes:
> From: Glen Choo <[e-mail]>
> 
> "struct config_options" is a disjoint set of options used by the config
> parser (e.g. event listeners) and options used by config_with_options()
> (e.g. to handle includes, choose which config files to parse).

Can this sentence be reworded? In particular, "disjoint" is a word
normally applied to two or more sets (meaning that they have no elements
in common), but here it is used for only one.

Everything else looks good, and the reasoning (some functions only use
a subset of the fields, and this subset is easily explained conceptually
as those related to parsing) makes sense.

Taylor Blau Oct. 23, 2023, 6:46 p.m. UTC | #2

On Mon, Oct 23, 2023 at 10:52:17AM -0700, Jonathan Tan wrote:
> Josh Steadmon <[e-mail]> writes:
> > From: Glen Choo <[e-mail]>
> >
> > "struct config_options" is a disjoint set of options used by the config
> > parser (e.g. event listeners) and options used by config_with_options()
> > (e.g. to handle includes, choose which config files to parse).
>
> Can this sentence be reworded? In particular, "disjoint" is a word
> normally applied to two or more sets (meaning that they have no elements
> in common), but here it is used for only one.

The pedant in me agrees with you. I do think that the sentence reads a
little awkwardly. Perhaps instead:

    "struct config_options" has members which serve two distinct
    purposes. There are a set of members used by the configuration parse
    (e.g. event listeners). There is also a set used by
    config_with_options() (e.g to handle includes, choose which config
    files to parse).

> Everything else looks good, and the reasoning (some functions only use
> a subset of the fields, and this subset is easily explained conceptually
> as those related to parsing) makes sense.

Yup.

Thanks,
Taylor

Patch

diff --git a/bundle-uri.c b/bundle-uri.c
index 4b5c49b93d..f93ca6a486 100644
--- a/bundle-uri.c
+++ b/bundle-uri.c
@@ -237,7 +237,7 @@  int bundle_uri_parse_config_format(const char *uri,
 				   struct bundle_list *list)
 {
 	int result;
-	struct config_options opts = {
+	struct config_parse_options opts = {
 		.error_action = CONFIG_ERROR_ERROR,
 	};
 
diff --git a/config.c b/config.c
index 85c5f35132..1518f70fc2 100644
--- a/config.c
+++ b/config.c
@@ -982,7 +982,7 @@  static int get_base_var(struct config_source *cs, struct strbuf *name)
 struct parse_event_data {
 	enum config_event_t previous_type;
 	size_t previous_offset;
-	const struct config_options *opts;
+	const struct config_parse_options *opts;
 };
 
 static int do_event(struct config_source *cs, enum config_event_t type,
@@ -1030,7 +1030,7 @@  static void kvi_from_source(struct config_source *cs,
 
 static int git_parse_source(struct config_source *cs, config_fn_t fn,
 			    struct key_value_info *kvi, void *data,
-			    const struct config_options *opts)
+			    const struct config_parse_options *opts)
 {
 	int comment = 0;
 	size_t baselen = 0;
@@ -1967,7 +1967,7 @@  int git_default_config(const char *var, const char *value,
  */
 static int do_config_from(struct config_source *top, config_fn_t fn,
 			  void *data, enum config_scope scope,
-			  const struct config_options *opts)
+			  const struct config_parse_options *opts)
 {
 	struct key_value_info kvi = KVI_INIT;
 	int ret;
@@ -1992,7 +1992,7 @@  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, enum config_scope scope,
-			       const struct config_options *opts)
+			       const struct config_parse_options *opts)
 {
 	struct config_source top = CONFIG_SOURCE_INIT;
 	int ret;
@@ -2021,7 +2021,7 @@  static int git_config_from_stdin(config_fn_t fn, void *data,
 
 int git_config_from_file_with_options(config_fn_t fn, const char *filename,
 				      void *data, enum config_scope scope,
-				      const struct config_options *opts)
+				      const struct config_parse_options *opts)
 {
 	int ret = -1;
 	FILE *f;
@@ -2047,7 +2047,7 @@  int git_config_from_mem(config_fn_t fn,
 			const enum config_origin_type origin_type,
 			const char *name, const char *buf, size_t len,
 			void *data, enum config_scope scope,
-			const struct config_options *opts)
+			const struct config_parse_options *opts)
 {
 	struct config_source top = CONFIG_SOURCE_INIT;
 
@@ -3380,7 +3380,7 @@  int git_config_set_multivar_in_file_gently(const char *config_filename,
 		struct stat st;
 		size_t copy_begin, copy_end;
 		int i, new_line = 0;
-		struct config_options opts;
+		struct config_parse_options opts;
 
 		if (!value_pattern)
 			store.value_pattern = NULL;
diff --git a/config.h b/config.h
index 6332d74904..2537516446 100644
--- a/config.h
+++ b/config.h
@@ -85,6 +85,21 @@  typedef int (*config_parser_event_fn_t)(enum config_event_t type,
 					struct config_source *cs,
 					void *event_fn_data);
 
+struct config_parse_options {
+	enum config_error_action {
+		CONFIG_ERROR_UNSET = 0, /* use source-specific default */
+		CONFIG_ERROR_DIE, /* die() on error */
+		CONFIG_ERROR_ERROR, /* error() on error, return -1 */
+		CONFIG_ERROR_SILENT, /* return -1 */
+	} error_action;
+	/*
+	 * event_fn and event_fn_data are for internal use only. Handles events
+	 * emitted by the config parser.
+	 */
+	config_parser_event_fn_t event_fn;
+	void *event_fn_data;
+};
+
 struct config_options {
 	unsigned int respect_includes : 1;
 	unsigned int ignore_repo : 1;
@@ -92,6 +107,9 @@  struct config_options {
 	unsigned int ignore_cmdline : 1;
 	unsigned int system_gently : 1;
 
+	const char *commondir;
+	const char *git_dir;
+	struct config_parse_options parse_options;
 	/*
 	 * For internal use. Include all includeif.hasremoteurl paths without
 	 * checking if the repo has that remote URL, and when doing so, verify
@@ -99,21 +117,6 @@  struct config_options {
 	 * themselves.
 	 */
 	unsigned int unconditional_remote_url : 1;
-
-	const char *commondir;
-	const char *git_dir;
-	/*
-	 * event_fn and event_fn_data are for internal use only. Handles events
-	 * emitted by the config parser.
-	 */
-	config_parser_event_fn_t event_fn;
-	void *event_fn_data;
-	enum config_error_action {
-		CONFIG_ERROR_UNSET = 0, /* use source-specific default */
-		CONFIG_ERROR_DIE, /* die() on error */
-		CONFIG_ERROR_ERROR, /* error() on error, return -1 */
-		CONFIG_ERROR_SILENT, /* return -1 */
-	} error_action;
 };
 
 /* Config source metadata for a given config key-value pair */
@@ -178,13 +181,13 @@  int git_config_from_file(config_fn_t fn, const char *, void *);
 
 int git_config_from_file_with_options(config_fn_t fn, const char *,
 				      void *, enum config_scope,
-				      const struct config_options *);
+				      const struct config_parse_options *);
 int git_config_from_mem(config_fn_t fn,
 			const enum config_origin_type,
 			const char *name,
 			const char *buf, size_t len,
 			void *data, enum config_scope scope,
-			const struct config_options *opts);
+			const struct config_parse_options *opts);
 int git_config_from_blob_oid(config_fn_t fn, const char *name,
 			     struct repository *repo,
 			     const struct object_id *oid, void *data,
diff --git a/fsck.c b/fsck.c
index 3be86616c5..522ee1c18a 100644
--- a/fsck.c
+++ b/fsck.c
@@ -1219,7 +1219,7 @@  static int fsck_blob(const struct object_id *oid, const char *buf,
 		return 0;
 
 	if (oidset_contains(&options->gitmodules_found, oid)) {
-		struct config_options config_opts = { 0 };
+		struct config_parse_options config_opts = { 0 };
 		struct fsck_gitmodules_data data;
 
 		oidset_insert(&options->gitmodules_done, oid);