[prev in list] [next in list] [prev in thread] [next in thread]
List: git
Subject: [RFC PATCH v1.5 0/5] config-parse: create config parsing library
From: Glen Choo <chooglen () google ! com>
Date: 2023-07-31 23:46:37
Message-ID: 20230731234910.94149-1-chooglen () google ! com
[Download RAW message or body]
I'll be leaving Google soon, so I won't be pushing this series through to
completion :/ As such, here's an rfc-quality 'reroll' to demonstrate how I
intended to respond to the comments on v1 2/2 that discussed where the library
boundary should be [1]. In particular:
> - "respect_includes" is in the library, but callers might want to opt
> out of it or provide an alternative way to resolve includes.
I've punted on this altogether by removing "respect_includes" from the options
accepted by the library functions. As 2/5 describes, config_options is quite
bloated, so this split already makes sense even without config-parse.
Initially, I considered turning the "includes" machinery into a first-class
citizen in config-parse instead of having it implemented as a separate config
callback, but it turned out to be much more complicated than it first appears.
The biggest challenge is that as "hasconfig:remote.*url" performs the additional
pass over the config files, it performs different actions - from the starting
file, it collects remote urls, then in files that could be imported via
"hasconfig:remote.*url", it forbid remote urls. This 'action switching' didn't
translate well into the lower level machinery, so I opted to leave this for
later.
> - There is a lot of error reporting capability with respect to the
> source of config, and not all sources are applicable to library
> users. How should we proceed? E.g. should we expect that all library
> users conform to the list here (e.g. even if the source is something
> like but not exactly STDIN, they should pick it), or allow users to
> customize sources?
I've opted to move the error reporting to a callback function (3/5), so in-tree
callers will use the existing logic, but other callers can provide their own
error handling. I've reused the config 'event listeners' for this purpose since
they seemed pretty well-suited to the task - they already respond to errors, and
the in-tree error handling doesn't do anything that an event listener can't
express.
As a result, the library now excludes functions that use the in-tree error
handling; we either teach the function to accept event listeners or exclude it
altogether.
[1] https://lore.kernel.org/git/20230721003107.3095493-1-jonathantanmy@google.com
Glen Choo (5):
config: return positive from git_config_parse_key()
config: split out config_parse_options
config: report config parse errors using cb
config.c: accept config_parse_options in git_config_from_stdin
config-parse: split library out of config.[c|h]
Makefile | 1 +
builtin/config.c | 7 +-
bundle-uri.c | 4 +-
config-parse.c | 561 ++++++++++++++++++++++++++++++++++++++
config-parse.h | 155 +++++++++++
config.c | 660 ++++-----------------------------------------
config.h | 134 +--------
fsck.c | 4 +-
submodule-config.c | 13 +-
t/t1300-config.sh | 16 ++
10 files changed, 816 insertions(+), 739 deletions(-)
create mode 100644 config-parse.c
create mode 100644 config-parse.h
Range-diff:
1: 9924481630 = 1: 9924481630 config: return positive from git_config_parse_key()
-: ---------- > 2: 8807be8319 config: split out config_parse_options
-: ---------- > 3: 3d4a2357f3 config: report config parse errors using cb
-: ---------- > 4: ff03ee1de7 config.c: accept config_parse_options in git_config_from_stdin
2: 4461d2163e ! 5: 377acbfbb5 config-parse: split library out of config.[c|h]
@@ config-parse.c (new)
+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,
@@ config-parse.c (new)
+
+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;
+ struct strbuf *var = &cs->var;
-+ int error_return = 0;
-+ char *error_msg = NULL;
+
+ /* U+FEFF Byte Order Mark in UTF8 */
+ const char *bomptr = utf8_bom;
@@ config-parse.c (new)
+ if (get_value(cs, kvi, fn, data, var) < 0)
+ break;
+ }
-+
-+ if (do_event(cs, CONFIG_EVENT_ERROR, &event_data) < 0)
-+ return -1;
-+
-+ switch (cs->origin_type) {
-+ case CONFIG_ORIGIN_BLOB:
-+ error_msg = xstrfmt(_("bad config line %d in blob %s"),
-+ cs->linenr, cs->name);
-+ break;
-+ case CONFIG_ORIGIN_FILE:
-+ error_msg = xstrfmt(_("bad config line %d in file %s"),
-+ cs->linenr, cs->name);
-+ break;
-+ case CONFIG_ORIGIN_STDIN:
-+ error_msg = xstrfmt(_("bad config line %d in standard input"),
-+ cs->linenr);
-+ break;
-+ case CONFIG_ORIGIN_SUBMODULE_BLOB:
-+ error_msg = xstrfmt(_("bad config line %d in submodule-blob %s"),
-+ cs->linenr, cs->name);
-+ break;
-+ case CONFIG_ORIGIN_CMDLINE:
-+ error_msg = xstrfmt(_("bad config line %d in command line %s"),
-+ cs->linenr, cs->name);
-+ break;
-+ default:
-+ error_msg = xstrfmt(_("bad config line %d in %s"),
-+ cs->linenr, cs->name);
-+ }
-+
-+ switch (opts && opts->error_action ?
-+ opts->error_action :
-+ cs->default_error_action) {
-+ case CONFIG_ERROR_DIE:
-+ die("%s", error_msg);
-+ break;
-+ case CONFIG_ERROR_ERROR:
-+ error_return = error("%s", error_msg);
-+ break;
-+ case CONFIG_ERROR_SILENT:
-+ error_return = -1;
-+ break;
-+ case CONFIG_ERROR_UNSET:
-+ BUG("config error action unset");
-+ }
-+
-+ free(error_msg);
-+ return error_return;
++ /*
++ * FIXME for whatever reason, do_event passes the _previous_ event, so
++ * in order for our callback to receive the error event, we have to call
++ * do_event twice
++ */
++ do_event(cs, CONFIG_EVENT_ERROR, &event_data);
++ do_event(cs, CONFIG_EVENT_ERROR, &event_data);
++ return -1;
+}
+
+/*
@@ config-parse.c (new)
+ */
+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;
@@ config-parse.c (new)
+ 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;
@@ config-parse.c (new)
+ top.origin_type = origin_type;
+ top.name = name;
+ top.path = path;
-+ top.default_error_action = CONFIG_ERROR_DIE;
+ top.do_fgetc = config_file_fgetc;
+ top.do_ungetc = config_file_ungetc;
+ top.do_ftell = config_file_ftell;
@@ config-parse.c (new)
+ return ret;
+}
+
-+int git_config_from_stdin(config_fn_t fn, void *data,
-+ enum config_scope scope)
++int git_config_from_stdin(config_fn_t fn, void *data, enum config_scope scope,
++ const struct config_parse_options *config_opts)
+{
+ return do_config_from_file(fn, CONFIG_ORIGIN_STDIN, "", NULL, stdin,
-+ data, scope, NULL);
++ data, scope, config_opts);
+}
+
+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;
@@ config-parse.c (new)
+ return ret;
+}
+
-+int git_config_from_file(config_fn_t fn, const char *filename, void *data)
-+{
-+ return git_config_from_file_with_options(fn, filename, data,
-+ CONFIG_SCOPE_UNKNOWN, NULL);
-+}
-+
+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;
+
@@ config-parse.c (new)
+ top.origin_type = origin_type;
+ top.name = name;
+ top.path = NULL;
-+ top.default_error_action = CONFIG_ERROR_ERROR;
+ top.do_fgetc = config_buf_fgetc;
+ top.do_ungetc = config_buf_ungetc;
+ top.do_ftell = config_buf_ftell;
@@ config-parse.h (new)
+/*
+ * Low level config parsing.
+ */
-+#ifndef CONFIG_LIB_H
-+#define CONFIG_LIB_H
++#ifndef CONFIG_PARSE_H
++#define CONFIG_PARSE_H
+
+#include "strbuf.h"
+
@@ config-parse.h (new)
+ struct config_source *cs,
+ void *event_fn_data);
+
-+struct config_options {
-+ unsigned int respect_includes : 1;
-+ unsigned int ignore_repo : 1;
-+ unsigned int ignore_worktree : 1;
-+ unsigned int ignore_cmdline : 1;
-+ unsigned int system_gently : 1;
-+
-+ /*
-+ * For internal use. Include all includeif.hasremoteurl paths without
-+ * checking if the repo has that remote URL, and when doing so, verify
-+ * that files included in this way do not configure any remote URLs
-+ * themselves.
-+ */
-+ unsigned int unconditional_remote_url : 1;
-+
-+ const char *commondir;
-+ const char *git_dir;
++struct config_parse_options {
+ /*
+ * 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;
+};
+
+struct config_source {
@@ config-parse.h (new)
+ enum config_origin_type origin_type;
+ const char *name;
+ const char *path;
-+ enum config_error_action default_error_action;
+ int linenr;
+ int eof;
+ size_t total_len;
@@ config-parse.h (new)
+typedef int (*config_fn_t)(const char *, const char *,
+ const struct config_context *, void *);
+
-+/**
-+ * Read a specific file in git-config format.
-+ */
-+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_stdin(config_fn_t fn, void *data, enum config_scope scope);
++int git_config_from_stdin(config_fn_t fn, void *data, enum config_scope scope,
++ const struct config_parse_options *config_opts);
+
-+#endif /* CONFIG_LIB_H */
++#endif /* CONFIG_PARSE_H */
## config.c ##
@@
@@ config.c
- enum config_origin_type origin_type;
- const char *name;
- const char *path;
-- enum config_error_action default_error_action;
- int linenr;
- int eof;
- size_t total_len;
@@ config.c: int git_config_from_parameters(config_fn_t fn, void *data)
-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,
@@ config.c: int git_config_from_parameters(config_fn_t fn, void *data)
- out->path = cs->path;
-}
-
+ int git_config_err_fn(enum config_event_t type, size_t begin_offset UNUSED,
+ size_t end_offset UNUSED, struct config_source *cs,
+ void *data)
+@@ config.c: int git_config_err_fn(enum config_event_t type, size_t begin_offset UNUSED,
+ return error_return;
+ }
+
-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;
- struct strbuf *var = &cs->var;
-- int error_return = 0;
-- char *error_msg = NULL;
-
- /* U+FEFF Byte Order Mark in UTF8 */
- const char *bomptr = utf8_bom;
@@ config.c: int git_config_from_parameters(config_fn_t fn, void *data)
- break;
- }
-
-- if (do_event(cs, CONFIG_EVENT_ERROR, &event_data) < 0)
-- return -1;
--
-- switch (cs->origin_type) {
-- case CONFIG_ORIGIN_BLOB:
-- error_msg = xstrfmt(_("bad config line %d in blob %s"),
-- cs->linenr, cs->name);
-- break;
-- case CONFIG_ORIGIN_FILE:
-- error_msg = xstrfmt(_("bad config line %d in file %s"),
-- cs->linenr, cs->name);
-- break;
-- case CONFIG_ORIGIN_STDIN:
-- error_msg = xstrfmt(_("bad config line %d in standard input"),
-- cs->linenr);
-- break;
-- case CONFIG_ORIGIN_SUBMODULE_BLOB:
-- error_msg = xstrfmt(_("bad config line %d in submodule-blob %s"),
-- cs->linenr, cs->name);
-- break;
-- case CONFIG_ORIGIN_CMDLINE:
-- error_msg = xstrfmt(_("bad config line %d in command line %s"),
-- cs->linenr, cs->name);
-- break;
-- default:
-- error_msg = xstrfmt(_("bad config line %d in %s"),
-- cs->linenr, cs->name);
-- }
--
-- switch (opts && opts->error_action ?
-- opts->error_action :
-- cs->default_error_action) {
-- case CONFIG_ERROR_DIE:
-- die("%s", error_msg);
-- break;
-- case CONFIG_ERROR_ERROR:
-- error_return = error("%s", error_msg);
-- break;
-- case CONFIG_ERROR_SILENT:
-- error_return = -1;
-- break;
-- case CONFIG_ERROR_UNSET:
-- BUG("config error action unset");
-- }
--
-- free(error_msg);
-- return error_return;
+- /*
+- * FIXME for whatever reason, do_event passes the _previous_ event, so
+- * in order for our callback to receive the error event, we have to call
+- * do_event twice
+- */
+- do_event(cs, CONFIG_EVENT_ERROR, &event_data);
+- do_event(cs, CONFIG_EVENT_ERROR, &event_data);
+- return -1;
-}
-
static uintmax_t get_unit_factor(const char *end)
@@ config.c: 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;
@@ config.c: int git_default_config(const char *var, const char *value,
- 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;
@@ config.c: int git_default_config(const char *var, const char *value,
- top.origin_type = origin_type;
- top.name = name;
- top.path = path;
-- top.default_error_action = CONFIG_ERROR_DIE;
- top.do_fgetc = config_file_fgetc;
- top.do_ungetc = config_file_ungetc;
- top.do_ftell = config_file_ftell;
@@ config.c: int git_default_config(const char *var, const char *value,
-}
-
-static int git_config_from_stdin(config_fn_t fn, void *data,
-- enum config_scope scope)
+- enum config_scope scope,
+- const struct config_parse_options *config_opts)
-{
- return do_config_from_file(fn, CONFIG_ORIGIN_STDIN, "", NULL, stdin,
-- data, scope, NULL);
+- data, scope, config_opts);
-}
-
-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;
@@ config.c: int git_default_config(const char *var, const char *value,
- return ret;
-}
-
--int git_config_from_file(config_fn_t fn, const char *filename, void *data)
--{
-- return git_config_from_file_with_options(fn, filename, data,
-- CONFIG_SCOPE_UNKNOWN, NULL);
--}
--
+ int git_config_from_file(config_fn_t fn, const char *filename, void *data)
+ {
+ struct config_parse_options config_opts = CP_OPTS_INIT(CONFIG_ERROR_DIE);
+@@ config.c: int git_config_from_file(config_fn_t fn, const char *filename, void *data)
+ CONFIG_SCOPE_UNKNOWN, &config_opts);
+ }
+
-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;
-
@@ config.c: int git_default_config(const char *var, const char *value,
- top.origin_type = origin_type;
- top.name = name;
- top.path = NULL;
-- top.default_error_action = CONFIG_ERROR_ERROR;
- top.do_fgetc = config_buf_fgetc;
- top.do_ungetc = config_buf_ungetc;
- top.do_ftell = config_buf_ftell;
@@ config.h: struct git_config_source {
- struct config_source *cs,
- void *event_fn_data);
-
--struct config_options {
-- unsigned int respect_includes : 1;
-- unsigned int ignore_repo : 1;
-- unsigned int ignore_worktree : 1;
-- unsigned int ignore_cmdline : 1;
-- unsigned int system_gently : 1;
--
-- /*
-- * For internal use. Include all includeif.hasremoteurl paths without
-- * checking if the repo has that remote URL, and when doing so, verify
-- * that files included in this way do not configure any remote URLs
-- * themselves.
-- */
-- unsigned int unconditional_remote_url : 1;
--
-- const char *commondir;
-- const char *git_dir;
+-struct config_parse_options {
- /*
- * 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;
-};
+-
+ #define CP_OPTS_INIT(error_action) { \
+ .event_fn = git_config_err_fn, \
+ .event_fn_data = (enum config_error_action []){(error_action)}, \
+@@ config.h: enum config_error_action {
+ int git_config_err_fn(enum config_event_t type, size_t begin_offset,
+ size_t end_offset, struct config_source *cs,
+ void *event_fn_data);
-
-/* Config source metadata for a given config key-value pair */
-struct key_value_info {
@@ config.h: struct git_config_source {
-
int git_default_config(const char *, const char *,
const struct config_context *, void *);
-
--/**
-- * Read a specific file in git-config format.
-- * This function takes the same callback and data parameters as `git_config`.
-- *
-- * Unlike git_config(), this function does not respect includes.
-- */
--int git_config_from_file(config_fn_t fn, const char *, void *);
+-
+ /**
+ * Read a specific file in git-config format.
+ * This function takes the same callback and data parameters as `git_config`.
+@@ config.h: int git_default_config(const char *, const char *,
+ * Unlike git_config(), this function does not respect includes.
+ */
+ 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,
base-commit: aa9166bcc0ba654fc21f198a30647ec087f733ed
--
2.41.0.585.gd2178a4bd4-goog
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic