[v2,1/4] config: split out config_parse_options

Commit Message

Josh Steadmon Aug. 23, 2023, 9:53 p.m. UTC

From: Glen Choo <[e-mail]>

"struct config_options" is a disjoint set of options options used by the
config parser (e.g. event listners) 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

Junio C Hamano Aug. 23, 2023, 11:26 p.m. UTC | #1

Josh Steadmon <[e-mail]> writes:

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

There is some punctuation missing on the first line.  Perhaps an em-dash
between "options---options" or something like that?

> 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(-)

> 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,
>  	};

OK, and this one only needs the parse_options half, and presumably
all hunks (other than the one that splits the struct into two in
config.h) are about turning the users of config_options that only
need config_parse_options half.

As we do not see any funny casts in the patch text, compilers should
catch all questionable conversion in this step, if there were any.

OK.

Josh Steadmon Sept. 21, 2023, 9:08 p.m. UTC | #2

On 2023.08.23 16:26, Junio C Hamano wrote:
> Josh Steadmon <[e-mail]> writes:
> 
> > From: Glen Choo <[e-mail]>
> >
> > "struct config_options" is a disjoint set of options options used by the
> > config parser (e.g. event listners) and options used by
> > config_with_options() (e.g. to handle includes, choose which config
> > files to parse).
> 
> There is some punctuation missing on the first line.  Perhaps an em-dash
> between "options---options" or something like that?

Cleaned up this and an additional typo in the description.

> > 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(-)
> 
> > 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,
> >  	};
> 
> OK, and this one only needs the parse_options half, and presumably
> all hunks (other than the one that splits the struct into two in
> config.h) are about turning the users of config_options that only
> need config_parse_options half.
> 
> As we do not see any funny casts in the patch text, compilers should
> catch all questionable conversion in this step, if there were any.
> 
> OK.

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);