config-parse: create config parsing library

Config parsing no longer uses global state as of gc/config-context, so the natural next step for libification is to turn that into its own library. This series starts that process by moving config parsing into config-parse.[c|h] so that other programs can include this functionality without pulling in all of config.[c|h].

To use config-parse.[c|h], an external caller has to obey our convention of using "#include git-compat-util.h" at the start of the .c file. This is doable by including the in-tree git-compat-util.h and linking against the libgit.a Make target (which is admittedly cumbersome), and we've verified this by compiling and linking to the library using the Google-internal version of Bazel.

This series is not meant to distract from Calvin's git-std-lib series [1]. In fact, the two are complementary: git-std-lib will make it possible for external callers to compile a smaller subset of files in order to use a library. Doing this for config-parse will make config-parse easier to use, while testing that git-std-lib does what we want it to.

I considered calling the library config-ll (like we do in other parts of the codebase) instead of config-parse, with the intention of adding more "low level" config code to it in the future. A benefit to that is that by having fewer modules, dependency management is easier to reason about. However, I struggled to think of what other config code could be considered "low level" but doesn't make sense as its own module. (E.g. struct config_set is a low level implementation detail, but I think it's well-scoped enough to be its own config-set module.) I'd appreciate suggestions on how the config libraries could be organized.

[1]
https://lore.kernel.org/git/[email protected]/

Glen Choo (2):
config: return positive from git_config_parse_key()
config-parse: split library out of config.[c|h]

 Makefile           |   1 +
 builtin/config.c   |   3 +-
 config-parse.c     | 611 +++++++++++++++++++++++++++++++++++++++++++
 config-parse.h     | 182 +++++++++++++
 config.c           | 636 +--------------------------------------------
 config.h           | 146 +----------
 submodule-config.c |   4 +-
 t/t1300-config.sh  |  16 ++
 8 files changed, 816 insertions(+), 783 deletions(-)
 create mode 100644 config-parse.c
 create mode 100644 config-parse.h

base-commit: aa9166bcc0ba654fc21f198a30647ec087f733ed
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1551%2Fchooglen%2Fconfig%2Fparse-lib-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1551/chooglen/config/parse-lib-v1
Pull-Request: https://github.com/git/git/pull/1551

gitgitgadget

From: Glen Choo <[email protected]>

git_config_parse_key() returns #define-d error codes, but negated. This negation is merely a convenience to other parts of config.c that don't bother inspecting the return value before passing it along. But:

a) There's no good reason why those callers couldn't negate the value themselves.

b) In other callers, this value eventually gets fed to exit(3), and those callers need to sanitize the negative value (and they sometimes do so lossily, by overriding the return value with CONFIG_INVALID_KEY).

c) We want to move that into a separate library, and returning only negative values no longer makes as much sense.

Change git_config_parse_key() to return positive values instead, and adjust callers accordingly. Callers that sanitize the negative sign for exit(3) now pass the return value opaquely, fixing a bug where "git config <key with no section or name>" results in a different exit code depending on whether we are setting or getting config. Callers that wanted to pass along a negative value now negate the return value themselves.

---
 builtin/config.c   |  3 +--
 config.c           | 16 ++++++++--------
 config.h           |  2 +-
 submodule-config.c |  4 ++--
 t/t1300-config.sh  | 16 ++++++++++++++++
 5 files changed, 28 insertions(+), 13 deletions(-)

index 1c75cbc43df..8a2840f0a8c 100644
@@ -362,8 +362,7 @@ static int get_value(const char *key_, const char *regex_, unsigned flags)
 			goto free_strings;
 		}
 	} else {
-		if (git_config_parse_key(key_, &key, NULL)) {
-			ret = CONFIG_INVALID_KEY;
+		if ((ret = git_config_parse_key(key_, &key, NULL))) {
 			goto free_strings;
 		}
 	}

index 85c5f35132c..ca77ca17a47 100644
@@ -534,8 +534,9 @@ static inline int iskeychar(int c)
  * Auxiliary function to sanity-check and split the key into the section
  * identifier and variable name.
  *
- * Returns 0 on success, -1 when there is an invalid character in the key and
- * -2 if there is no section name in the key.
+ * Returns 0 on success, CONFIG_INVALID_KEY when there is an invalid character
+ * in the key and CONFIG_NO_SECTION_OR_NAME if there is no section name in the
+ * key.
  *
  * store_key - pointer to char* which will hold a copy of the key with
  *             lowercase section and variable name

@@ -555,12 +556,12 @@ int git_config_parse_key(const char *key, char **store_key, size_t *baselen_)
 
 	if (last_dot == NULL || last_dot == key) {
 		error(_("key does not contain a section: %s"), key);
-		return -CONFIG_NO_SECTION_OR_NAME;
+		return CONFIG_NO_SECTION_OR_NAME;
 	}
 
 	if (!last_dot[1]) {
 		error(_("key does not contain variable name: %s"), key);
-		return -CONFIG_NO_SECTION_OR_NAME;
+		return CONFIG_NO_SECTION_OR_NAME;
 	}
 
 	baselen = last_dot - key;

@@ -596,7 +597,7 @@ int git_config_parse_key(const char *key, char **store_key, size_t *baselen_)
 
 out_free_ret_1:
 	FREE_AND_NULL(*store_key);
-	return -CONFIG_INVALID_KEY;
+	return CONFIG_INVALID_KEY;
 }
 
 static int config_parse_pair(const char *key, const char *value,

@@ -2346,7 +2347,7 @@ static int configset_find_element(struct config_set *set, const char *key,
 	 * `key` may come from the user, so normalize it before using it
 	 * for querying entries from the hashmap.
 	 */
-	ret = git_config_parse_key(key, &normalized_key, NULL);
+	ret = -git_config_parse_key(key, &normalized_key, NULL);
 	if (ret)
 		return ret;
 

@@ -3334,8 +3335,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
 	size_t contents_sz;
 	struct config_store_data store = CONFIG_STORE_INIT;
 
-	/* parse-key returns negative; flip the sign to feed exit(3) */
-	ret = 0 - git_config_parse_key(key, &store.key, &store.baselen);
+	ret = git_config_parse_key(key, &store.key, &store.baselen);
 	if (ret)
 		goto out_free;
 

index 6332d749047..40966cb6828 100644
@@ -23,7 +23,7 @@
 
 struct object_id;
 
-/* git_config_parse_key() returns these negated: */
+/* git_config_parse_key() returns these: */
 #define CONFIG_INVALID_KEY 1
 #define CONFIG_NO_SECTION_OR_NAME 2
 /* git_config_set_gently(), git_config_set_multivar_gently() return the above or these: */

index b6908e295f4..2aafc7f9cbe 100644
@@ -824,8 +824,8 @@ int print_config_from_gitmodules(struct repository *repo, const char *key)
 	char *store_key;
 
 	ret = git_config_parse_key(key, &store_key, NULL);
-	if (ret < 0)
-		return CONFIG_INVALID_KEY;
+	if (ret)
+		return ret;
 
 	config_from_gitmodules(config_print_callback, repo, store_key);
 

index 387d336c91f..3202b0f8843 100755
@@ -2590,4 +2590,20 @@ test_expect_success 'includeIf.hasconfig:remote.*.url forbids remote url in such
 	grep "fatal: remote URLs cannot be configured in file directly or indirectly included by includeIf.hasconfig:remote.*.url" err
 '
 
+# Exit codes
+test_expect_success '--get with bad key' '
+	# Also exits with 1 if the value is not found
+	test_expect_code 1 git config --get "bad.name\n" 2>err &&
+	grep "error: invalid key" err &&
+	test_expect_code 2 git config --get "bad." 2>err &&
+	grep "error: key does not contain variable name" err
+'
+
+test_expect_success 'set with bad key' '
+	test_expect_code 1 git config "bad.name\n" var 2>err &&
+	grep "error: invalid key" err &&
+	test_expect_code 2 git config "bad." var 2>err &&
+	grep "error: key does not contain variable name" err
+'
+
 test_done

gitgitgadget

I'm not sure if we ever concluded that functions returning errors should return positive integers, but in this case I think it makes sense. We can document what's returned as being the same as what's documented in the config manpage.

The negative return was as early as when the function was first introduced in b09c53a3e3 (Sanity-check config variable names, 2011-01- 30), but there's no indication there as to why the author chose negative values.

Can you be more precise as to which bug is being fixed? (I think somewhere, a 1 is returned when it should be a 2.)

OK.

That is not a good reason to break from a widely adopted convention in UNIXy library functions to signal success with 0 and failure with negative values. The callers if they want to have a positive values can flip the polarity themselves, by the way.

There is no reason to think that each and every minute difference the direct callers of a library function may want to notice by different error return values needs to be propagated to the calling process via its exit value. It is perfectly fine and expected for the status values of the entire process is more coarse grained than individual library calls, the latter may convey not just "I failed" but "I failed why" to their callers, while the former may not want to say "I made a call to some library functions and got this error code", let alone "I called library function X and got error code Y".

In other words, if your program does

	err = library_call_about_some_filesystem_operation();
	if (err)
		exit(err); /* or exit(-err) */
	err = library_call_about_some_database_operation();
	if (err)
		exit(err); /* or exit(-err) */
	err = library_call_about_some_parsing();
	if (err)
		exit(err); /* or exit(-err) */

there is something wrong. The error codes from these different library functions share the same "integer" namespace without being segregated, and expecting the calling process to be able to tell what error we discovered by relaying the literal translation of low level library error code would not work. The exit() codes would need to be wider (i.e. not limited only to the possible errors from a single library function) and would be coarser (i.e. a filesystem operation may say "open failed due to permission error" or "open failed because there was no such file"; at the end-user level, it may be more appropriate to say "configuration file could not be read", regardless of the reason why the filesystem operation failed).

	err = library_call_about_some_filesystem_operation();
	if (err) {
		error("cannot open the filesystem entity");
		exit(ERR_FILESYSTEM);
	err = library_call_about_some_database_operation();
	if (err)
		exit(ERR_DATABASE);
	err = library_call_about_some_parsing();
	if (err)
		exit(ERR_PARSING);

So, this is not a good reason, either.

Quite the contrary, if it were a purely internal convention, we may not care too much, as we have only ourselves to confuse by picking an unusual convention, but if we are making more parts of our code available in a library-ish interface, I would expect they follow some convention, and that convention would be "0 for success, negative for failure".

Thanks.

Oh, interesting. I was trying to follow the conventions of the surrounding config.c code and many other parts of the codebase, which returns positive values. Why do we choose to return postive values throughout the codebase, by the way? Is it because they were really intended for exit(3), and not to be used as a library.

Yes, I fully agree. I didn't intend to be a statement on how things should be, but rather how they already are. The oddities in this case are:

- No callers actually care about the sign of git_config_parse_key() since it can only return values of one sign. Only configset_find_element() benefits from the sign being negative (since it returns negative on config key errors), but instead of putting the burden on the function it depends on, it could just return the negative value itself. And this "burden" is real, in that other callers have to worry about the negative value.

- For better or worse, we've already wired git_config_parse_key() directly to the exit values, e.g. if one peeks into builtin/config.c:cmd_config(), we'll see "return git_config_set_multivar_in_file_gently()", which in turn may return the value from git_config_parse_key(). (And as a result, we also try hard to separate the error values returnable by git_config_parse_key() vs the others returnable by git_config_set_multivar_in_file_gently().)

I would strongly prefer if builtin/config.c took more responsibility for noticing config.c errors and sanitizing them accordingly, but it seemed like too much churn for this series. If you think it is the right time for it, though (which I think it might be), I could try to make that change.

Right. I do not care what the convention is, only that we pick one. I chose the one that I saw in the surrounding code (positive), but I'm happy to go with UNIXy (negative) if others think it is worth the churn.

If config.c does that, I'd say that was poorly designed oddball. Looking at read-cache.c (which is older parts of the codebase written back when the developer base was smaller) may give you a better examples to follow. After all, error() returns negative exactly because we want to follow the usual "negative is an error" convention.

From: Glen Choo <[email protected]>

The config parsing machinery (besides "include" directives) is usable by programs other than Git - it works with works with any file written in Git config syntax (IOW it doesn't rely on 'core' Git features like a repository), and as of the series ending at 6e8e7981eb (config: pass source to config_parser_event_fn_t, 2023-06-28), it no longer relies on global state. Thus, we can and should start turning it into a library other programs can use.

Begin this process by splitting the config parsing code out of config.[c|h] and into config-parse.[c|h]. Do not change interfaces or function bodies, but tweak visibility and includes where appropriate, namely:

- git_config_from_stdin() is now non-static so that it can be seen by config.c.

- "struct config_source" is now defined in the .h file so that it can be seen by config.c. And as a result, config-lib.h needs to "#include strbuf.h".

In theory, this makes it possible for in-tree files to decide whether they only need all of the config functionality or only config parsing, and bring in the smallest bit of functionality needed. But for now, there are no in-tree files that can swap "#include config.h" for "#include config-parse.h". E.g. Bundle URIs would only need config parsing to parse bundle lists, but bundle-uri.c uses other config.h functionality like key parsing and reading repo settings.

The resulting library is usable, though it is unergonomic to do so, e.g. the caller needs to "#include git-compat-util.h" and other dependencies, and we don't have an easy way of linking in the required objects. This isn't the end state we want for our libraries, but at least we have _some_ library whose usability we can improve in future series.

---
 Makefile       |   1 +
 config-parse.c | 611 +++++++++++++++++++++++++++++++++++++++++++++++
 config-parse.h | 182 ++++++++++++++
 config.c       | 632 -------------------------------------------------
 config.h       | 146 +-----------
 5 files changed, 795 insertions(+), 777 deletions(-)
 create mode 100644 config-parse.c
 create mode 100644 config-parse.h

index fb541dedc9f..67e05bcee57 100644
@@ -992,6 +992,7 @@ LIB_OBJS += compat/obstack.o
 LIB_OBJS += compat/terminal.o
 LIB_OBJS += compat/zlib-uncompress2.o
 LIB_OBJS += config.o
+LIB_OBJS += config-parse.o
 LIB_OBJS += connect.o
 LIB_OBJS += connected.o
 LIB_OBJS += convert.o

gitgitgadget

I think we need to be more careful in how we split. It would be easier if there is a concrete use case, but some preliminary questions:

- "respect_includes" is in the library, but callers might want to opt out of it or provide an alternative way to resolve includes.
- 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?

In the absence of more information, the split I would envision is either something that can only parse a buffer, its error messages being very generic (the caller should turn them into something more specific before showing them to the user) (but one problem here is that we must postprocess includes, which might be a problem if the output of parsing is a flat hashtable, since we wouldn't know which keys are overridden by the includes and which are not); or something that can take in a callback that is invoked whenever something is included and maybe also a callback for access to the object database and that has full knowledge of all sources for error reporting (or allows the caller to customize the sources).

Makes sense. Or alternatively, we could choose not to support "respect_includes" initially, and exclude it to avoid confusion.

Good point. I would also prefer to have the list of sources constrained to the list of sources available via the library. Some possibilities I can see are:

1. Move the Git-program-specific error message reporting to a level above the library (i.e. config.c).

2. Proceed as-is (with the additional sources in the library) and leave a FIXME to address this when we find a Git library-idiomatic way to handle errors. This won't be the last time we'll have to untangle Git-program-specific error reporting from the library - it might be useful to try to figure out all of that in one fell swoop.

3. Figure out library-idiomatic error handling mentioned in 2. right now.

I think 1. is the best option, but if that fails, 2. is also reasonable.
3. is too difficult to do with a sample size of 1.

Hm, how does the include mechanism here this differ from what's in this patch? This also only parses a single file and ignores includes. I'm not sure why this requires us to postprocess includes - in config.c, includes are handled by 'pausing' parsing of the current source, evaluating the included files, then 'resuming' parsing.

Ah, I like this callback idea quite a lot. This lets config-parse.c easily support unconditional includes and provides entry points for program-specific behavior (like checking the odb). I will try this.

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:

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.

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/[email protected]

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

git_config_parse_key() returns #define-d error codes, but negated. This negation is merely a convenience to other parts of config.c that don't bother inspecting the return value before passing it along. But:

a) There's no good reason why those callers couldn't negate the value themselves.

b) In other callers, this value eventually gets fed to exit(3), and those callers need to sanitize the negative value (and they sometimes do so lossily, by overriding the return value with CONFIG_INVALID_KEY).

c) We want to move that into a separate library, and returning only negative values no longer makes as much sense.

Change git_config_parse_key() to return positive values instead, and adjust callers accordingly. Callers that sanitize the negative sign for exit(3) now pass the return value opaquely, fixing a bug where "git config <key with no section or name>" results in a different exit code depending on whether we are setting or getting config. Callers that wanted to pass along a negative value now negate the return value themselves.

---
 builtin/config.c   |  3 +--
 config.c           | 16 ++++++++--------
 config.h           |  2 +-
 submodule-config.c |  4 ++--
 t/t1300-config.sh  | 16 ++++++++++++++++
 5 files changed, 28 insertions(+), 13 deletions(-)

index 1c75cbc43d..8a2840f0a8 100644
@@ -362,8 +362,7 @@ static int get_value(const char *key_, const char *regex_, unsigned flags)
 			goto free_strings;
 		}
 	} else {
-		if (git_config_parse_key(key_, &key, NULL)) {
-			ret = CONFIG_INVALID_KEY;
+		if ((ret = git_config_parse_key(key_, &key, NULL))) {
 			goto free_strings;
 		}
 	}

index 85c5f35132..ca77ca17a4 100644
@@ -534,8 +534,9 @@ static inline int iskeychar(int c)
  * Auxiliary function to sanity-check and split the key into the section
  * identifier and variable name.
  *
- * Returns 0 on success, -1 when there is an invalid character in the key and
- * -2 if there is no section name in the key.
+ * Returns 0 on success, CONFIG_INVALID_KEY when there is an invalid character
+ * in the key and CONFIG_NO_SECTION_OR_NAME if there is no section name in the
+ * key.
  *
  * store_key - pointer to char* which will hold a copy of the key with
  *             lowercase section and variable name

@@ -555,12 +556,12 @@ int git_config_parse_key(const char *key, char **store_key, size_t *baselen_)
 
 	if (last_dot == NULL || last_dot == key) {
 		error(_("key does not contain a section: %s"), key);
-		return -CONFIG_NO_SECTION_OR_NAME;
+		return CONFIG_NO_SECTION_OR_NAME;
 	}
 
 	if (!last_dot[1]) {
 		error(_("key does not contain variable name: %s"), key);
-		return -CONFIG_NO_SECTION_OR_NAME;
+		return CONFIG_NO_SECTION_OR_NAME;
 	}
 
 	baselen = last_dot - key;

@@ -596,7 +597,7 @@ int git_config_parse_key(const char *key, char **store_key, size_t *baselen_)
 
 out_free_ret_1:
 	FREE_AND_NULL(*store_key);
-	return -CONFIG_INVALID_KEY;
+	return CONFIG_INVALID_KEY;
 }
 
 static int config_parse_pair(const char *key, const char *value,

@@ -2346,7 +2347,7 @@ static int configset_find_element(struct config_set *set, const char *key,
 	 * `key` may come from the user, so normalize it before using it
 	 * for querying entries from the hashmap.
 	 */
-	ret = git_config_parse_key(key, &normalized_key, NULL);
+	ret = -git_config_parse_key(key, &normalized_key, NULL);
 	if (ret)
 		return ret;
 

@@ -3334,8 +3335,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
 	size_t contents_sz;
 	struct config_store_data store = CONFIG_STORE_INIT;
 
-	/* parse-key returns negative; flip the sign to feed exit(3) */
-	ret = 0 - git_config_parse_key(key, &store.key, &store.baselen);
+	ret = git_config_parse_key(key, &store.key, &store.baselen);
 	if (ret)
 		goto out_free;
 

index 6332d74904..40966cb682 100644
@@ -23,7 +23,7 @@
 
 struct object_id;
 
-/* git_config_parse_key() returns these negated: */
+/* git_config_parse_key() returns these: */
 #define CONFIG_INVALID_KEY 1
 #define CONFIG_NO_SECTION_OR_NAME 2
 /* git_config_set_gently(), git_config_set_multivar_gently() return the above or these: */

index b6908e295f..2aafc7f9cb 100644
@@ -824,8 +824,8 @@ int print_config_from_gitmodules(struct repository *repo, const char *key)
 	char *store_key;
 
 	ret = git_config_parse_key(key, &store_key, NULL);
-	if (ret < 0)
-		return CONFIG_INVALID_KEY;
+	if (ret)
+		return ret;
 
 	config_from_gitmodules(config_print_callback, repo, store_key);
 

index 387d336c91..3202b0f884 100755
@@ -2590,4 +2590,20 @@ test_expect_success 'includeIf.hasconfig:remote.*.url forbids remote url in such
 	grep "fatal: remote URLs cannot be configured in file directly or indirectly included by includeIf.hasconfig:remote.*.url" err
 '
 
+# Exit codes
+test_expect_success '--get with bad key' '
+	# Also exits with 1 if the value is not found
+	test_expect_code 1 git config --get "bad.name\n" 2>err &&
+	grep "error: invalid key" err &&
+	test_expect_code 2 git config --get "bad." 2>err &&
+	grep "error: key does not contain variable name" err
+'
+
+test_expect_success 'set with bad key' '
+	test_expect_code 1 git config "bad.name\n" var 2>err &&
+	grep "error: invalid key" err &&
+	test_expect_code 2 git config "bad." var 2>err &&
+	grep "error: key does not contain variable name" err
+'
+
 test_done

2.41.0.585.gd2178a4bd4-goog

"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.

---
 bundle-uri.c |  2 +-
 config.c     | 14 +++++++-------
 config.h     | 37 ++++++++++++++++++++-----------------
 fsck.c       |  2 +-
 4 files changed, 29 insertions(+), 26 deletions(-)

index 4b5c49b93d..f93ca6a486 100644
@@ -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,
 	};
 

index ca77ca17a4..dc6cda03aa 100644
@@ -983,7 +983,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,

@@ -1031,7 +1031,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;

@@ -1968,7 +1968,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;

@@ -1993,7 +1993,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;

@@ -2022,7 +2022,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;

@@ -2048,7 +2048,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;

index 40966cb682..b13586307d 100644
@@ -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,

index 3be86616c5..522ee1c18a 100644
@@ -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);

2.41.0.585.gd2178a4bd4-goog

In a subsequent commit, config parsing will become its own library, and it's likely that the caller will want flexibility in handling errors (instead of being limited to the error handling we have in-tree).

Move the Git-specific error handling into a config_parser_event_fn_t that responds to config errors, and make git_parse_source() always return -1 (careful inspection shows that it was always returning -1 already). This makes CONFIG_ERROR_SILENT obsolete since that is equivalent to not specifying an error event listener. Also, remove CONFIG_ERROR_UNSET and the config_source 'default', since all callers are now expected to specify the error handling they want.

---
 builtin/config.c   |   4 +-
 bundle-uri.c       |   4 +-
 config.c           | 175 ++++++++++++++++++++++++++-------------------
 config.h           |  20 ++++--
 fsck.c             |   4 +-
 submodule-config.c |   9 ++-
 6 files changed, 129 insertions(+), 87 deletions(-)

index 8a2840f0a8..de1878b947 100644
@@ -42,7 +42,9 @@ static int actions, type;
 static char *default_value;
 static int end_nul;
 static int respect_includes_opt = -1;
-static struct config_options config_options;
+static struct config_options config_options = {
+	.parse_options = CP_OPTS_INIT(CONFIG_ERROR_DIE)
+};
 static int show_origin;
 static int show_scope;
 static int fixed_value;

index f93ca6a486..856bffdcad 100644
@@ -237,9 +237,7 @@ int bundle_uri_parse_config_format(const char *uri,
 				   struct bundle_list *list)
 {
 	int result;
-	struct config_parse_options opts = {
-		.error_action = CONFIG_ERROR_ERROR,
-	};
+	struct config_parse_options opts = CP_OPTS_INIT(CONFIG_ERROR_ERROR);
 
 	if (!list->baseURI) {
 		struct strbuf baseURI = STRBUF_INIT;

index b13586307d..1aed02cd5d 100644
@@ -86,12 +86,6 @@ typedef int (*config_parser_event_fn_t)(enum config_event_t type,
 					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.

@@ -100,6 +94,11 @@ struct config_parse_options {
 	void *event_fn_data;
 };
 
+#define CP_OPTS_INIT(error_action) { \
+	.event_fn = git_config_err_fn, \
+	.event_fn_data = (enum config_error_action []){(error_action)}, \
+}
+
 struct config_options {
 	unsigned int respect_includes : 1;
 	unsigned int ignore_repo : 1;

@@ -119,6 +118,15 @@ struct config_options {
 	unsigned int unconditional_remote_url : 1;
 };
 
+enum config_error_action {
+	CONFIG_ERROR_DIE, /* die() on error */
+	CONFIG_ERROR_ERROR, /* error() on error, return -1 */
+};
+
+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 {
 	const char *filename;

index 522ee1c18a..bc0ca11421 100644
@@ -1219,7 +1219,6 @@ static int fsck_blob(const struct object_id *oid, const char *buf,
 		return 0;
 
 	if (oidset_contains(&options->gitmodules_found, oid)) {
-		struct config_parse_options config_opts = { 0 };
 		struct fsck_gitmodules_data data;
 
 		oidset_insert(&options->gitmodules_done, oid);

@@ -1238,10 +1237,9 @@ static int fsck_blob(const struct object_id *oid, const char *buf,
 		data.oid = oid;
 		data.options = options;
 		data.ret = 0;
-		config_opts.error_action = CONFIG_ERROR_SILENT;
 		if (git_config_from_mem(fsck_gitmodules_fn, CONFIG_ORIGIN_BLOB,
 					".gitmodules", buf, size, &data,
-					CONFIG_SCOPE_UNKNOWN, &config_opts))
+					CONFIG_SCOPE_UNKNOWN, NULL))
 			data.ret |= report(options, oid, OBJ_BLOB,
 					FSCK_MSG_GITMODULES_PARSE,
 					"could not parse gitmodules blob");

index 2aafc7f9cb..bcdd6feefa 100644
@@ -565,6 +565,8 @@ static const struct submodule *config_from(struct submodule_cache *cache,
 	enum object_type type;
 	const struct submodule *submodule = NULL;
 	struct parse_config_parameter parameter;
+	struct config_parse_options config_opts = CP_OPTS_INIT(CONFIG_ERROR_ERROR);
+
 
 	/*
 	 * If any parameter except the cache is a NULL pointer just

@@ -608,7 +610,8 @@ static const struct submodule *config_from(struct submodule_cache *cache,
 	parameter.gitmodules_oid = &oid;
 	parameter.overwrite = 0;
 	git_config_from_mem(parse_config, CONFIG_ORIGIN_SUBMODULE_BLOB, rev.buf,
-			    config, config_size, &parameter, CONFIG_SCOPE_UNKNOWN, NULL);
+			    config, config_size, &parameter,
+			    CONFIG_SCOPE_UNKNOWN, &config_opts);
 	strbuf_release(&rev);
 	free(config);
 

@@ -652,7 +655,9 @@ static void config_from_gitmodules(config_fn_t fn, struct repository *repo, void
 		struct git_config_source config_source = {
 			0, .scope = CONFIG_SCOPE_SUBMODULE
 		};
-		const struct config_options opts = { 0 };
+		struct config_options opts = {
+			.parse_options = CP_OPTS_INIT(CONFIG_ERROR_DIE),
+		};
 		struct object_id oid;
 		char *file;
 		char *oidstr = NULL;

2.41.0.585.gd2178a4bd4-goog

A later commit will move git_config_from_stdin() to a library, so it will need to accept event listeners.

---
 config.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

index b6d0e16240..cf0785a258 100644
@@ -2025,12 +2025,11 @@ static int do_config_from_file(config_fn_t fn,
 }
 
 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)
 {
-	struct config_parse_options config_opts = CP_OPTS_INIT(CONFIG_ERROR_DIE);
-
 	return do_config_from_file(fn, CONFIG_ORIGIN_STDIN, "", NULL, stdin,
-				   data, scope, &config_opts);
+				   data, scope, config_opts);
 }
 
 int git_config_from_file_with_options(config_fn_t fn, const char *filename,

@@ -2265,7 +2264,8 @@ int config_with_options(config_fn_t fn, void *data,
 	 * regular lookup sequence.
 	 */
 	if (config_source && config_source->use_stdin) {
-		ret = git_config_from_stdin(fn, data, config_source->scope);
+		ret = git_config_from_stdin(fn, data, config_source->scope,
+					    &opts->parse_options);
 	} else if (config_source && config_source->file) {
 		ret = git_config_from_file_with_options(fn, config_source->file,
 							data, config_source->scope,

2.41.0.585.gd2178a4bd4-goog

The config parsing machinery (besides "include" directives) is usable by programs other than Git - it works with works with any file written in Git config syntax (IOW it doesn't rely on 'core' Git features like a repository), and as of the series ending at 6e8e7981eb (config: pass source to config_parser_event_fn_t, 2023-06-28), it no longer relies on global state. Thus, we can and should start turning it into a library other programs can use.

Begin this process by splitting the config parsing code out of config.[c|h] and into config-parse.[c|h]. Do not change interfaces or function bodies, but tweak visibility and includes where appropriate, namely:

- git_config_from_stdin() is now non-static so that it can be seen by config.c.

- "struct config_source" is now defined in the .h file so that it can be seen by config.c. And as a result, config-lib.h needs to "#include strbuf.h".

In theory, this makes it possible for in-tree files to decide whether they only need all of the config functionality or only config parsing, and bring in the smallest bit of functionality needed. But for now, there are no in-tree files that can swap "#include config.h" for "#include config-parse.h". E.g. Bundle URIs would only need config parsing to parse bundle lists, but bundle-uri.c uses other config.h functionality like key parsing and reading repo settings.

The resulting library is usable, though it is unergonomic to do so, e.g. the caller needs to "#include git-compat-util.h" and other dependencies, and we don't have an easy way of linking in the required objects. This isn't the end state we want for our libraries, but at least we have _some_ library whose usability we can improve in future series.

---
 Makefile       |   1 +
 config-parse.c | 561 +++++++++++++++++++++++++++++++++++++++++++++++
 config-parse.h | 155 +++++++++++++
 config.c       | 583 -------------------------------------------------
 config.h       | 119 +---------
 5 files changed, 718 insertions(+), 701 deletions(-)
 create mode 100644 config-parse.c
 create mode 100644 config-parse.h

index fb541dedc9..67e05bcee5 100644
@@ -992,6 +992,7 @@ LIB_OBJS += compat/obstack.o
 LIB_OBJS += compat/terminal.o
 LIB_OBJS += compat/zlib-uncompress2.o
 LIB_OBJS += config.o
+LIB_OBJS += config-parse.o
 LIB_OBJS += connect.o
 LIB_OBJS += connected.o
 LIB_OBJS += convert.o

2.41.0.585.gd2178a4bd4-goog

I'll be taking over this series from Glen (thank you for the work so far).

Here's what I see as the current open questions:
- Do we need a cleanup of return values in config.c? If so, does it need to be part of this series?
- Are we OK leaving the include machinery in config.c?
- If we expect non-Git-CLI projects to use config-parse as a library, should we provide alternative error-handling callbacks for (assumed) use cases, even if they won't be directly used in the Git project? (I lean to "No", but I want to make sure the option is considered.)
- Does this series need to include the config-parse.c:do_event() refactor Jonathan Tan mentioned in [1]?

[1]: https://lore.kernel.org/git/[email protected]/

Changes since v1.5:
- Dropped patch 1/5: config: return positive from git_config_parse_key()

Glen Choo (4):
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   |   4 +-
 bundle-uri.c       |   4 +-
 config-parse.c     | 561 ++++++++++++++++++++++++++++++++++++++
 config-parse.h     | 155 +++++++++++
 config.c           | 655 ++++-----------------------------------------
 config.h           | 134 +---------
 fsck.c             |   4 +-
 submodule-config.c |   9 +-
 9 files changed, 795 insertions(+), 732 deletions(-)
 create mode 100644 config-parse.c
 create mode 100644 config-parse.h

Range-diff against v1:

1:  9924481630 < -:  ---------- config: return positive from git_config_parse_key()
-:  ---------- > 1:  5c676fbac3 config: split out config_parse_options
-:  ---------- > 2:  cb92a1f2e3 config: report config parse errors using cb
-:  ---------- > 3:  e034d0780c config.c: accept config_parse_options in git_config_from_stdin
-:  ---------- > 4:  74c5dcd5a2 config-parse: split library out of config.[c|h]

base-commit: aa9166bcc0ba654fc21f198a30647ec087f733ed

2.42.0.rc1.204.g551eb34607-goog

From: Glen Choo <[email protected]>

"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.

---
 bundle-uri.c |  2 +-
 config.c     | 14 +++++++-------
 config.h     | 37 ++++++++++++++++++++-----------------
 fsck.c       |  2 +-
 4 files changed, 29 insertions(+), 26 deletions(-)

index 4b5c49b93d..f93ca6a486 100644
@@ -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,
 	};
 

index 85c5f35132..1518f70fc2 100644
@@ -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;

index 6332d74904..2537516446 100644
@@ -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,

index 3be86616c5..522ee1c18a 100644
@@ -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);

2.42.0.rc1.204.g551eb34607-goog

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.

From: Glen Choo <[email protected]>

In a subsequent commit, config parsing will become its own library, and it's likely that the caller will want flexibility in handling errors (instead of being limited to the error handling we have in-tree).

Move the Git-specific error handling into a config_parser_event_fn_t that responds to config errors, and make git_parse_source() always return -1 (careful inspection shows that it was always returning -1 already). This makes CONFIG_ERROR_SILENT obsolete since that is equivalent to not specifying an error event listener. Also, remove CONFIG_ERROR_UNSET and the config_source 'default', since all callers are now expected to specify the error handling they want.

---
 builtin/config.c   |   4 +-
 bundle-uri.c       |   4 +-
 config.c           | 175 ++++++++++++++++++++++++++-------------------
 config.h           |  20 ++++--
 fsck.c             |   4 +-
 submodule-config.c |   9 ++-
 6 files changed, 129 insertions(+), 87 deletions(-)

index 1c75cbc43d..e2cf49de7a 100644
@@ -42,7 +42,9 @@ static int actions, type;
 static char *default_value;
 static int end_nul;
 static int respect_includes_opt = -1;
-static struct config_options config_options;
+static struct config_options config_options = {
+	.parse_options = CP_OPTS_INIT(CONFIG_ERROR_DIE)
+};
 static int show_origin;
 static int show_scope;
 static int fixed_value;

index f93ca6a486..856bffdcad 100644
@@ -237,9 +237,7 @@ int bundle_uri_parse_config_format(const char *uri,
 				   struct bundle_list *list)
 {
 	int result;
-	struct config_parse_options opts = {
-		.error_action = CONFIG_ERROR_ERROR,
-	};
+	struct config_parse_options opts = CP_OPTS_INIT(CONFIG_ERROR_ERROR);
 
 	if (!list->baseURI) {
 		struct strbuf baseURI = STRBUF_INIT;

index 2537516446..8ad399580f 100644
@@ -86,12 +86,6 @@ typedef int (*config_parser_event_fn_t)(enum config_event_t type,
 					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.

@@ -100,6 +94,11 @@ struct config_parse_options {
 	void *event_fn_data;
 };
 
+#define CP_OPTS_INIT(error_action) { \
+	.event_fn = git_config_err_fn, \
+	.event_fn_data = (enum config_error_action []){(error_action)}, \
+}
+
 struct config_options {
 	unsigned int respect_includes : 1;
 	unsigned int ignore_repo : 1;

@@ -119,6 +118,15 @@ struct config_options {
 	unsigned int unconditional_remote_url : 1;
 };
 
+enum config_error_action {
+	CONFIG_ERROR_DIE, /* die() on error */
+	CONFIG_ERROR_ERROR, /* error() on error, return -1 */
+};
+
+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 {
 	const char *filename;

index 522ee1c18a..bc0ca11421 100644
@@ -1219,7 +1219,6 @@ static int fsck_blob(const struct object_id *oid, const char *buf,
 		return 0;
 
 	if (oidset_contains(&options->gitmodules_found, oid)) {
-		struct config_parse_options config_opts = { 0 };
 		struct fsck_gitmodules_data data;
 
 		oidset_insert(&options->gitmodules_done, oid);

@@ -1238,10 +1237,9 @@ static int fsck_blob(const struct object_id *oid, const char *buf,
 		data.oid = oid;
 		data.options = options;
 		data.ret = 0;
-		config_opts.error_action = CONFIG_ERROR_SILENT;
 		if (git_config_from_mem(fsck_gitmodules_fn, CONFIG_ORIGIN_BLOB,
 					".gitmodules", buf, size, &data,
-					CONFIG_SCOPE_UNKNOWN, &config_opts))
+					CONFIG_SCOPE_UNKNOWN, NULL))
 			data.ret |= report(options, oid, OBJ_BLOB,
 					FSCK_MSG_GITMODULES_PARSE,
 					"could not parse gitmodules blob");

index b6908e295f..d97135c917 100644
@@ -565,6 +565,8 @@ static const struct submodule *config_from(struct submodule_cache *cache,
 	enum object_type type;
 	const struct submodule *submodule = NULL;
 	struct parse_config_parameter parameter;
+	struct config_parse_options config_opts = CP_OPTS_INIT(CONFIG_ERROR_ERROR);
+
 
 	/*
 	 * If any parameter except the cache is a NULL pointer just

@@ -608,7 +610,8 @@ static const struct submodule *config_from(struct submodule_cache *cache,
 	parameter.gitmodules_oid = &oid;
 	parameter.overwrite = 0;
 	git_config_from_mem(parse_config, CONFIG_ORIGIN_SUBMODULE_BLOB, rev.buf,
-			    config, config_size, &parameter, CONFIG_SCOPE_UNKNOWN, NULL);
+			    config, config_size, &parameter,
+			    CONFIG_SCOPE_UNKNOWN, &config_opts);
 	strbuf_release(&rev);
 	free(config);
 

@@ -652,7 +655,9 @@ static void config_from_gitmodules(config_fn_t fn, struct repository *repo, void
 		struct git_config_source config_source = {
 			0, .scope = CONFIG_SCOPE_SUBMODULE
 		};
-		const struct config_options opts = { 0 };
+		struct config_options opts = {
+			.parse_options = CP_OPTS_INIT(CONFIG_ERROR_DIE),
+		};
 		struct object_id oid;
 		char *file;
 		char *oidstr = NULL;

2.42.0.rc1.204.g551eb34607-goog

And the in-tree error handling is abstracted out as the git_config_err_fn() function; in other words, we become the first client of the library interface, which makes sense.

I plan to look at this more fully later, but about this, I wrote about this in a reply to the previous version:
https://lore.kernel.org/git/[email protected]/

Thanks.

Also if you have time, can you comment on the latest round of the fix to the Bloom filter hash functions that Taylor sent the latest iteration for, either to give it a "looks perfect" to nudge it to merge it down, to give it a further polishing, or whatever. Thanks.

As Jonathan Tan mentioned in [1], on calling do_event() we set the start offset of the new event, and execute the callback for the previous event whose end offset we now know.

I refactored this into "start_event()" and "flush_event()" functions as suggested, and added a new "do_event_and_flush()" function for the case where we want to immediately execute a callback for an event.

[1]: https://lore.kernel.org/git/[email protected]/

Agreed, fixed here and above.

Very nicely done. Thanks, both.

From: Glen Choo <[email protected]>

A later commit will move git_config_from_stdin() to a library, so it will need to accept event listeners.

---
 config.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

index 6cf4dafc6c..40cc9dbc40 100644
@@ -2024,12 +2024,11 @@ static int do_config_from_file(config_fn_t fn,
 }
 
 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)
 {
-	struct config_parse_options config_opts = CP_OPTS_INIT(CONFIG_ERROR_DIE);
-
 	return do_config_from_file(fn, CONFIG_ORIGIN_STDIN, "", NULL, stdin,
-				   data, scope, &config_opts);
+				   data, scope, config_opts);
 }
 
 int git_config_from_file_with_options(config_fn_t fn, const char *filename,

@@ -2264,7 +2263,8 @@ int config_with_options(config_fn_t fn, void *data,
 	 * regular lookup sequence.
 	 */
 	if (config_source && config_source->use_stdin) {
-		ret = git_config_from_stdin(fn, data, config_source->scope);
+		ret = git_config_from_stdin(fn, data, config_source->scope,
+					    &opts->parse_options);
 	} else if (config_source && config_source->file) {
 		ret = git_config_from_file_with_options(fn, config_source->file,
 							data, config_source->scope,

2.42.0.rc1.204.g551eb34607-goog

From: Glen Choo <[email protected]>

The config parsing machinery (besides "include" directives) is usable by programs other than Git - it works with any file written in Git config syntax (IOW it doesn't rely on 'core' Git features like a repository), and as of the series ending at 6e8e7981eb (config: pass source to config_parser_event_fn_t, 2023-06-28), it no longer relies on global state. Thus, we can and should start turning it into a library other programs can use.

Begin this process by splitting the config parsing code out of config.[c|h] and into config-parse.[c|h]. Do not change interfaces or function bodies, but tweak visibility and includes where appropriate, namely:

- git_config_from_stdin() is now non-static so that it can be seen by config.c.

- "struct config_source" is now defined in the .h file so that it can be seen by config.c. And as a result, config-lib.h needs to "#include strbuf.h".

In theory, this makes it possible for in-tree files to decide whether they only need all of the config functionality or only config parsing, and bring in the smallest bit of functionality needed. But for now, there are no in-tree files that can swap "#include config.h" for "#include config-parse.h". E.g. Bundle URIs would only need config parsing to parse bundle lists, but bundle-uri.c uses other config.h functionality like key parsing and reading repo settings.

The resulting library is usable, though it is unergonomic to do so, e.g. the caller needs to "#include git-compat-util.h" and other dependencies, and we don't have an easy way of linking in the required objects. This isn't the end state we want for our libraries, but at least we have _some_ library whose usability we can improve in future series.

---
 Makefile       |   1 +
 config-parse.c | 561 +++++++++++++++++++++++++++++++++++++++++++++++
 config-parse.h | 155 +++++++++++++
 config.c       | 582 -------------------------------------------------
 config.h       | 119 +---------
 5 files changed, 718 insertions(+), 700 deletions(-)
 create mode 100644 config-parse.c
 create mode 100644 config-parse.h

index fb541dedc9..67e05bcee5 100644
@@ -992,6 +992,7 @@ LIB_OBJS += compat/obstack.o
 LIB_OBJS += compat/terminal.o
 LIB_OBJS += compat/zlib-uncompress2.o
 LIB_OBJS += config.o
+LIB_OBJS += config-parse.o
 LIB_OBJS += connect.o
 LIB_OBJS += connected.o
 LIB_OBJS += convert.o

2.42.0.rc1.204.g551eb34607-goog

BTW, I'm going to be AFK for a couple weeks, so it will be a while before I'm able to address feedback on this series. Thanks in advance.

Config parsing no longer uses global state as of gc/config-context, so the natural next step for libification is to turn that into its own library. This series starts that process by moving config parsing into config-parse.[c|h] so that other programs can include this functionality without pulling in all of config.[c|h].

Open questions:
- How do folks feel about the do_event() refactor in patches 2 & 3?

Changes since v2:
- Added patch 2/5 to refactor do_event() into start_event() and flush_event().
- In patch 3/5, we can now add do_event_and_flush() to immediately run an event callback, rather than having to do_event() twice in a row.

Changes since v1.5:
- Dropped patch 1/5: config: return positive from git_config_parse_key()

Glen Choo (4):
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]

Josh Steadmon (1):
config: split do_event() into start and flush operations

 Makefile           |   1 +
 builtin/config.c   |   4 +-
 bundle-uri.c       |   4 +-
 config-parse.c     | 601 +++++++++++++++++++++++++++++++++++++++++
 config-parse.h     | 155 +++++++++++
 config.c           | 658 ++++-----------------------------------------
 config.h           | 134 +--------
 fsck.c             |   4 +-
 submodule-config.c |   9 +-
 9 files changed, 836 insertions(+), 734 deletions(-)
 create mode 100644 config-parse.c
 create mode 100644 config-parse.h

Range-diff against v2:
1: 5c676fbac3 ! 1: fa55b7836f config: split out config_parse_options
@@ Metadata
## Commit message ##
config: split out config_parse_options

- "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.
+ "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.

## bundle-uri.c ##

-:  ---------- > 2:  8a1463c223 config: split do_event() into start and flush operations
2:  cb92a1f2e3 ! 3:  a888045c04 config: report config parse errors using cb
    @@ Commit message
         CONFIG_ERROR_UNSET and the config_source 'default', since all callers
         are now expected to specify the error handling they want.
     
    +    Add a new "do_event_and_flush" function for running event callbacks
    +    immediately, where the event does not need to calculate an end offset.
    +
         Signed-off-by: Glen Choo <[email protected]>
     
      ## builtin/config.c ##
    @@ config.c: static int add_remote_url(const char *var, const char *value,
      
      	opts = *inc->opts;
      	opts.unconditional_remote_url = 1;
    +@@ config.c: static int do_event(struct config_source *cs, enum config_event_t type,
    + 	return 0;
    + }
    + 
    ++static int do_event_and_flush(struct config_source *cs,
    ++			      enum config_event_t type,
    ++			      struct parse_event_data *data)
    ++{
    ++	int maybe_ret;
    ++
    ++	if ((maybe_ret = flush_event(cs, type, data)) < 1)
    ++		return maybe_ret;
    ++
    ++	start_event(cs, type, data);
    ++
    ++	if ((maybe_ret = flush_event(cs, type, data)) < 1)
    ++		return maybe_ret;
    ++
    ++	/*
    ++	 * Not actually EOF, but this indicates we don't have a valid event
    ++	 * to flush next time around.
    ++	 */
    ++	data->previous_type = CONFIG_EVENT_EOF;
    ++
    ++	return 0;
    ++}
    ++
    + static void kvi_from_source(struct config_source *cs,
    + 			    enum config_scope scope,
    + 			    struct key_value_info *out)
     @@ config.c: static void kvi_from_source(struct config_source *cs,
      	out->path = cs->path;
      }
    @@ config.c: static int git_parse_source(struct config_source *cs, config_fn_t fn,
     -
     -	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);
    ++	do_event_and_flush(cs, CONFIG_EVENT_ERROR, &event_data);
     +	return -1;
      }
      
    @@ config.c: void read_early_config(config_fn_t cb, void *data)
      void read_very_early_config(config_fn_t cb, void *data)
      {
     -	struct config_options opts = { 0 };
    +-
    +-	opts.respect_includes = 1;
    +-	opts.ignore_repo = 1;
    +-	opts.ignore_worktree = 1;
    +-	opts.ignore_cmdline = 1;
    +-	opts.system_gently = 1;
     +	struct config_options opts = {
    ++		.respect_includes = 1,
    ++		.ignore_repo = 1,
    ++		.ignore_worktree = 1,
    ++		.ignore_cmdline = 1,
    ++		.system_gently = 1,
     +		.parse_options = CP_OPTS_INIT(CONFIG_ERROR_DIE),
     +	};
      
    - 	opts.respect_includes = 1;
    - 	opts.ignore_repo = 1;
    + 	config_with_options(cb, data, NULL, NULL, &opts);
    + }
     @@ config.c: int git_configset_get_pathname(struct config_set *set, const char *key, const ch
      /* Functions use to read configuration from a repository */
      static void repo_read_config(struct repository *repo)
    @@ config.c: int git_configset_get_pathname(struct config_set *set, const char *key
      
      	opts.respect_includes = 1;
      	opts.commondir = repo->commondir;
    -@@ config.c: int repo_config_get_pathname(struct repository *repo,
    - static void read_protected_config(void)
    - {
    - 	struct config_options opts = {
    --		.respect_includes = 1,
    --		.ignore_repo = 1,
    --		.ignore_worktree = 1,
    --		.system_gently = 1,
    +@@ config.c: static void read_protected_config(void)
    + 		.ignore_repo = 1,
    + 		.ignore_worktree = 1,
    + 		.system_gently = 1,
     +		.parse_options = CP_OPTS_INIT(CONFIG_ERROR_DIE),
      	};
      
    -+	opts.respect_includes = 1;
    -+	opts.ignore_repo = 1;
    -+	opts.ignore_worktree = 1;
    -+	opts.system_gently = 1;
     +
      	git_configset_init(&protected_config);
      	config_with_options(config_set_callback, &protected_config, NULL,
3:  e034d0780c = 4:  49d4b64991 config.c: accept config_parse_options in git_config_from_stdin
4:  74c5dcd5a2 ! 5:  e59ca992d0 config-parse: split library out of config.[c|h]
    @@ config-parse.c (new)
     +	const struct config_parse_options *opts;
     +};
     +
    -+static int do_event(struct config_source *cs, enum config_event_t type,
    -+		    struct parse_event_data *data)
    ++static size_t get_corrected_offset(struct config_source *cs,
    ++				   enum config_event_t type)
     +{
    -+	size_t offset;
    -+
    -+	if (!data->opts || !data->opts->event_fn)
    -+		return 0;
    ++	size_t offset = cs->do_ftell(cs);
     +
    -+	if (type == CONFIG_EVENT_WHITESPACE &&
    -+	    data->previous_type == type)
    -+		return 0;
    -+
    -+	offset = cs->do_ftell(cs);
     +	/*
     +	 * At EOF, the parser always "inserts" an extra '\n', therefore
     +	 * the end offset of the event is the current file position, otherwise
    @@ config-parse.c (new)
     +	 */
     +	if (type != CONFIG_EVENT_EOF)
     +		offset--;
    ++	return offset;
    ++}
    ++
    ++static void start_event(struct config_source *cs, enum config_event_t type,
    ++		       struct parse_event_data *data)
    ++{
    ++	data->previous_type = type;
    ++	data->previous_offset = get_corrected_offset(cs, type);
    ++}
    ++
    ++static int flush_event(struct config_source *cs, enum config_event_t type,
    ++		       struct parse_event_data *data)
    ++{
    ++	if (!data->opts || !data->opts->event_fn)
    ++		return 0;
    ++
    ++	if (type == CONFIG_EVENT_WHITESPACE &&
    ++	    data->previous_type == type)
    ++		return 0;
     +
     +	if (data->previous_type != CONFIG_EVENT_EOF &&
     +	    data->opts->event_fn(data->previous_type, data->previous_offset,
    -+				 offset, cs, data->opts->event_fn_data) < 0)
    ++				 get_corrected_offset(cs, type), cs,
    ++				 data->opts->event_fn_data) < 0)
     +		return -1;
     +
    -+	data->previous_type = type;
    -+	data->previous_offset = offset;
    ++	return 1;
    ++}
    ++
    ++static int do_event(struct config_source *cs, enum config_event_t type,
    ++		    struct parse_event_data *data)
    ++{
    ++	int maybe_ret;
    ++
    ++	if ((maybe_ret = flush_event(cs, type, data)) < 1)
    ++		return maybe_ret;
    ++
    ++	start_event(cs, type, data);
    ++
    ++	return 0;
    ++}
    ++
    ++static int do_event_and_flush(struct config_source *cs,
    ++			      enum config_event_t type,
    ++			      struct parse_event_data *data)
    ++{
    ++	int maybe_ret;
    ++
    ++	if ((maybe_ret = flush_event(cs, type, data)) < 1)
    ++		return maybe_ret;
    ++
    ++	start_event(cs, type, data);
    ++
    ++	if ((maybe_ret = flush_event(cs, type, data)) < 1)
    ++		return maybe_ret;
    ++
    ++	/*
    ++	 * Not actually EOF, but this indicates we don't have a valid event
    ++	 * to flush next time around.
    ++	 */
    ++	data->previous_type = CONFIG_EVENT_EOF;
     +
     +	return 0;
     +}
    @@ config-parse.c (new)
     +		if (get_value(cs, kvi, fn, data, var) < 0)
     +			break;
     +	}
    -+	/*
    -+	 * 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);
    ++
    ++	do_event_and_flush(cs, CONFIG_EVENT_ERROR, &event_data);
     +	return -1;
     +}
     +
    @@ config.c: int git_config_from_parameters(config_fn_t fn, void *data)
     -	const struct config_parse_options *opts;
     -};
     -
    --static int do_event(struct config_source *cs, enum config_event_t type,
    --		    struct parse_event_data *data)
    +-static size_t get_corrected_offset(struct config_source *cs,
    +-				   enum config_event_t type)
     -{
    --	size_t offset;
    --
    --	if (!data->opts || !data->opts->event_fn)
    --		return 0;
    +-	size_t offset = cs->do_ftell(cs);
     -
    --	if (type == CONFIG_EVENT_WHITESPACE &&
    --	    data->previous_type == type)
    --		return 0;
    --
    --	offset = cs->do_ftell(cs);
     -	/*
     -	 * At EOF, the parser always "inserts" an extra '\n', therefore
     -	 * the end offset of the event is the current file position, otherwise
    @@ config.c: int git_config_from_parameters(config_fn_t fn, void *data)
     -	 */
     -	if (type != CONFIG_EVENT_EOF)
     -		offset--;
    +-	return offset;
    +-}
    +-
    +-static void start_event(struct config_source *cs, enum config_event_t type,
    +-		       struct parse_event_data *data)
    +-{
    +-	data->previous_type = type;
    +-	data->previous_offset = get_corrected_offset(cs, type);
    +-}
    +-
    +-static int flush_event(struct config_source *cs, enum config_event_t type,
    +-		       struct parse_event_data *data)
    +-{
    +-	if (!data->opts || !data->opts->event_fn)
    +-		return 0;
    +-
    +-	if (type == CONFIG_EVENT_WHITESPACE &&
    +-	    data->previous_type == type)
    +-		return 0;
     -
     -	if (data->previous_type != CONFIG_EVENT_EOF &&
     -	    data->opts->event_fn(data->previous_type, data->previous_offset,
    --				 offset, cs, data->opts->event_fn_data) < 0)
    +-				 get_corrected_offset(cs, type), cs,
    +-				 data->opts->event_fn_data) < 0)
     -		return -1;
     -
    --	data->previous_type = type;
    --	data->previous_offset = offset;
    +-	return 1;
    +-}
    +-
    +-static int do_event(struct config_source *cs, enum config_event_t type,
    +-		    struct parse_event_data *data)
    +-{
    +-	int maybe_ret;
    +-
    +-	if ((maybe_ret = flush_event(cs, type, data)) < 1)
    +-		return maybe_ret;
    +-
    +-	start_event(cs, type, data);
    +-
    +-	return 0;
    +-}
    +-
    +-static int do_event_and_flush(struct config_source *cs,
    +-			      enum config_event_t type,
    +-			      struct parse_event_data *data)
    +-{
    +-	int maybe_ret;
    +-
    +-	if ((maybe_ret = flush_event(cs, type, data)) < 1)
    +-		return maybe_ret;
    +-
    +-	start_event(cs, type, data);
    +-
    +-	if ((maybe_ret = flush_event(cs, type, data)) < 1)
    +-		return maybe_ret;
    +-
    +-	/*
    +-	 * Not actually EOF, but this indicates we don't have a valid event
    +-	 * to flush next time around.
    +-	 */
    +-	data->previous_type = CONFIG_EVENT_EOF;
     -
     -	return 0;
     -}
    @@ config.c: int git_config_err_fn(enum config_event_t type, size_t begin_offset UN
     -			break;
     -	}
     -
    --	/*
    --	 * 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);
    +-	do_event_and_flush(cs, CONFIG_EVENT_ERROR, &event_data);
     -	return -1;
     -}
     -

base-commit: aa9166bcc0ba654fc21f198a30647ec087f733ed

2.42.0.515.g380fc7ccd1-goog

From: Glen Choo <[email protected]>

"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.

---
 bundle-uri.c |  2 +-
 config.c     | 14 +++++++-------
 config.h     | 37 ++++++++++++++++++++-----------------
 fsck.c       |  2 +-
 4 files changed, 29 insertions(+), 26 deletions(-)

index 4b5c49b93d..f93ca6a486 100644
@@ -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,
 	};
 

index 85c5f35132..1518f70fc2 100644
@@ -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;

index 6332d74904..2537516446 100644
@@ -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,

index 3be86616c5..522ee1c18a 100644
@@ -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);

2.42.0.515.g380fc7ccd1-goog

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.

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

Yup.

Thanks,
Taylor

When handling config-parsing events, the current do_event() handler is a bit confusing; calling it with a specific event type records the initial offset where the event occurred, and runs the supplied callback against the previous event (whose end offset is now known).

Split this operation into "start_event" and "flush_event" functions. Then reimplement "do_event" (preserving the original behavior) using the newly split functions.

In a later change, we can use these building blocks to also handle "immediate" events, where we want to run the callback without having to calculate an end offset for the event.

---
 config.c | 50 ++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 36 insertions(+), 14 deletions(-)

index 1518f70fc2..ff138500a2 100644
@@ -985,19 +985,11 @@ struct parse_event_data {
 	const struct config_parse_options *opts;
 };
 
-static int do_event(struct config_source *cs, enum config_event_t type,
-		    struct parse_event_data *data)
+static size_t get_corrected_offset(struct config_source *cs,
+				   enum config_event_t type)
 {
-	size_t offset;
-
-	if (!data->opts || !data->opts->event_fn)
-		return 0;
-
-	if (type == CONFIG_EVENT_WHITESPACE &&
-	    data->previous_type == type)
-		return 0;
+	size_t offset = cs->do_ftell(cs);
 
-	offset = cs->do_ftell(cs);
 	/*
 	 * At EOF, the parser always "inserts" an extra '\n', therefore
 	 * the end offset of the event is the current file position, otherwise

@@ -1005,14 +997,44 @@ static int do_event(struct config_source *cs, enum config_event_t type,
 	 */
 	if (type != CONFIG_EVENT_EOF)
 		offset--;
+	return offset;
+}
+
+static void start_event(struct config_source *cs, enum config_event_t type,
+		       struct parse_event_data *data)
+{
+	data->previous_type = type;
+	data->previous_offset = get_corrected_offset(cs, type);
+}
+
+static int flush_event(struct config_source *cs, enum config_event_t type,
+		       struct parse_event_data *data)
+{
+	if (!data->opts || !data->opts->event_fn)
+		return 0;
+
+	if (type == CONFIG_EVENT_WHITESPACE &&
+	    data->previous_type == type)
+		return 0;
 
 	if (data->previous_type != CONFIG_EVENT_EOF &&
 	    data->opts->event_fn(data->previous_type, data->previous_offset,
-				 offset, cs, data->opts->event_fn_data) < 0)
+				 get_corrected_offset(cs, type), cs,
+				 data->opts->event_fn_data) < 0)
 		return -1;
 
-	data->previous_type = type;
-	data->previous_offset = offset;
+	return 1;
+}
+
+static int do_event(struct config_source *cs, enum config_event_t type,
+		    struct parse_event_data *data)
+{
+	int maybe_ret;
+
+	if ((maybe_ret = flush_event(cs, type, data)) < 1)
+		return maybe_ret;
+
+	start_event(cs, type, data);
 
 	return 0;
 }

2.42.0.515.g380fc7ccd1-goog

From: Glen Choo <[email protected]>

In a subsequent commit, config parsing will become its own library, and it's likely that the caller will want flexibility in handling errors (instead of being limited to the error handling we have in-tree).

Move the Git-specific error handling into a config_parser_event_fn_t that responds to config errors, and make git_parse_source() always return -1 (careful inspection shows that it was always returning -1 already). This makes CONFIG_ERROR_SILENT obsolete since that is equivalent to not specifying an error event listener. Also, remove CONFIG_ERROR_UNSET and the config_source 'default', since all callers are now expected to specify the error handling they want.

Add a new "do_event_and_flush" function for running event callbacks immediately, where the event does not need to calculate an end offset.

---
 builtin/config.c   |   4 +-
 bundle-uri.c       |   4 +-
 config.c           | 195 ++++++++++++++++++++++++++++-----------------
 config.h           |  20 +++--
 fsck.c             |   4 +-
 submodule-config.c |   9 ++-
 6 files changed, 147 insertions(+), 89 deletions(-)

index 1c75cbc43d..e2cf49de7a 100644
@@ -42,7 +42,9 @@ static int actions, type;
 static char *default_value;
 static int end_nul;
 static int respect_includes_opt = -1;
-static struct config_options config_options;
+static struct config_options config_options = {
+	.parse_options = CP_OPTS_INIT(CONFIG_ERROR_DIE)
+};
 static int show_origin;
 static int show_scope;
 static int fixed_value;

index f93ca6a486..856bffdcad 100644
@@ -237,9 +237,7 @@ int bundle_uri_parse_config_format(const char *uri,
 				   struct bundle_list *list)
 {
 	int result;
-	struct config_parse_options opts = {
-		.error_action = CONFIG_ERROR_ERROR,
-	};
+	struct config_parse_options opts = CP_OPTS_INIT(CONFIG_ERROR_ERROR);
 
 	if (!list->baseURI) {
 		struct strbuf baseURI = STRBUF_INIT;

index 2537516446..8ad399580f 100644
@@ -86,12 +86,6 @@ typedef int (*config_parser_event_fn_t)(enum config_event_t type,
 					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.

@@ -100,6 +94,11 @@ struct config_parse_options {
 	void *event_fn_data;
 };
 
+#define CP_OPTS_INIT(error_action) { \
+	.event_fn = git_config_err_fn, \
+	.event_fn_data = (enum config_error_action []){(error_action)}, \
+}
+
 struct config_options {
 	unsigned int respect_includes : 1;
 	unsigned int ignore_repo : 1;

@@ -119,6 +118,15 @@ struct config_options {
 	unsigned int unconditional_remote_url : 1;
 };
 
+enum config_error_action {
+	CONFIG_ERROR_DIE, /* die() on error */
+	CONFIG_ERROR_ERROR, /* error() on error, return -1 */
+};
+
+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 {
 	const char *filename;

index 522ee1c18a..bc0ca11421 100644
@@ -1219,7 +1219,6 @@ static int fsck_blob(const struct object_id *oid, const char *buf,
 		return 0;
 
 	if (oidset_contains(&options->gitmodules_found, oid)) {
-		struct config_parse_options config_opts = { 0 };
 		struct fsck_gitmodules_data data;
 
 		oidset_insert(&options->gitmodules_done, oid);

@@ -1238,10 +1237,9 @@ static int fsck_blob(const struct object_id *oid, const char *buf,
 		data.oid = oid;
 		data.options = options;
 		data.ret = 0;
-		config_opts.error_action = CONFIG_ERROR_SILENT;
 		if (git_config_from_mem(fsck_gitmodules_fn, CONFIG_ORIGIN_BLOB,
 					".gitmodules", buf, size, &data,
-					CONFIG_SCOPE_UNKNOWN, &config_opts))
+					CONFIG_SCOPE_UNKNOWN, NULL))
 			data.ret |= report(options, oid, OBJ_BLOB,
 					FSCK_MSG_GITMODULES_PARSE,
 					"could not parse gitmodules blob");

index b6908e295f..d97135c917 100644
@@ -565,6 +565,8 @@ static const struct submodule *config_from(struct submodule_cache *cache,
 	enum object_type type;
 	const struct submodule *submodule = NULL;
 	struct parse_config_parameter parameter;
+	struct config_parse_options config_opts = CP_OPTS_INIT(CONFIG_ERROR_ERROR);
+
 
 	/*
 	 * If any parameter except the cache is a NULL pointer just

@@ -608,7 +610,8 @@ static const struct submodule *config_from(struct submodule_cache *cache,
 	parameter.gitmodules_oid = &oid;
 	parameter.overwrite = 0;
 	git_config_from_mem(parse_config, CONFIG_ORIGIN_SUBMODULE_BLOB, rev.buf,
-			    config, config_size, &parameter, CONFIG_SCOPE_UNKNOWN, NULL);
+			    config, config_size, &parameter,
+			    CONFIG_SCOPE_UNKNOWN, &config_opts);
 	strbuf_release(&rev);
 	free(config);
 

@@ -652,7 +655,9 @@ static void config_from_gitmodules(config_fn_t fn, struct repository *repo, void
 		struct git_config_source config_source = {
 			0, .scope = CONFIG_SCOPE_SUBMODULE
 		};
-		const struct config_options opts = { 0 };
+		struct config_options opts = {
+			.parse_options = CP_OPTS_INIT(CONFIG_ERROR_DIE),
+		};
 		struct object_id oid;
 		char *file;
 		char *oidstr = NULL;

2.42.0.515.g380fc7ccd1-goog

I think this has to be better explained. So:

- There is already a config_parser_event_fn_t that can be configured by a user to receive emitted config events. This callback can return negative to halt further config parsing.

- Currently, it is git_parse_source() that detects when an error occurs, and it emits a CONFIG_EVENT_ERROR and either dies, prints an error, or swallows the error depending on error_action; no matter what error_action is, it halts config parsing, as one would expect. This commit moves the die/print/swallow handling to a config_parser_event_fn_t that will see the CONFIG_EVENT_ERROR and die/ print/swallow.

- This new config_parser_event_fn_t does not need to swallow, since that's the same as not passing in a callback. So it just needs to die/ print.

FWIW, I have always felt that the way STRING_LIST_INIT* was done was quite ugly. The new pattern does look superiour, as long as (1) it does not involve voodoo like token pasting, and (2) the parameters passed does not grow. The latter is especially important as there is no equivalent to designated initializers in C preprocessor macros.

Thanks.

From: Glen Choo <[email protected]>

A later commit will move git_config_from_stdin() to a library, so it will need to accept event listeners.

---
 config.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

index 0c4f1a2874..50188f469a 100644
@@ -2063,12 +2063,11 @@ static int do_config_from_file(config_fn_t fn,
 }
 
 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)
 {
-	struct config_parse_options config_opts = CP_OPTS_INIT(CONFIG_ERROR_DIE);
-
 	return do_config_from_file(fn, CONFIG_ORIGIN_STDIN, "", NULL, stdin,
-				   data, scope, &config_opts);
+				   data, scope, config_opts);
 }
 
 int git_config_from_file_with_options(config_fn_t fn, const char *filename,

@@ -2303,7 +2302,8 @@ int config_with_options(config_fn_t fn, void *data,
 	 * regular lookup sequence.
 	 */
 	if (config_source && config_source->use_stdin) {
-		ret = git_config_from_stdin(fn, data, config_source->scope);
+		ret = git_config_from_stdin(fn, data, config_source->scope,
+					    &opts->parse_options);
 	} else if (config_source && config_source->file) {
 		ret = git_config_from_file_with_options(fn, config_source->file,
 							data, config_source->scope,

2.42.0.515.g380fc7ccd1-goog

From: Glen Choo <[email protected]>

The config parsing machinery (besides "include" directives) is usable by programs other than Git - it works with any file written in Git config syntax (IOW it doesn't rely on 'core' Git features like a repository), and as of the series ending at 6e8e7981eb (config: pass source to config_parser_event_fn_t, 2023-06-28), it no longer relies on global state. Thus, we can and should start turning it into a library other programs can use.

Begin this process by splitting the config parsing code out of config.[c|h] and into config-parse.[c|h]. Do not change interfaces or function bodies, but tweak visibility and includes where appropriate, namely:

- git_config_from_stdin() is now non-static so that it can be seen by config.c.

- "struct config_source" is now defined in the .h file so that it can be seen by config.c. And as a result, config-lib.h needs to "#include strbuf.h".

In theory, this makes it possible for in-tree files to decide whether they only need all of the config functionality or only config parsing, and bring in the smallest bit of functionality needed. But for now, there are no in-tree files that can swap "#include config.h" for "#include config-parse.h". E.g. Bundle URIs would only need config parsing to parse bundle lists, but bundle-uri.c uses other config.h functionality like key parsing and reading repo settings.

The resulting library is usable, though it is unergonomic to do so, e.g. the caller needs to "#include git-compat-util.h" and other dependencies, and we don't have an easy way of linking in the required objects. This isn't the end state we want for our libraries, but at least we have _some_ library whose usability we can improve in future series.

---
 Makefile       |   1 +
 config-parse.c | 601 +++++++++++++++++++++++++++++++++++++++++++++++
 config-parse.h | 155 ++++++++++++
 config.c       | 621 -------------------------------------------------
 config.h       | 119 +---------
 5 files changed, 758 insertions(+), 739 deletions(-)
 create mode 100644 config-parse.c
 create mode 100644 config-parse.h

index fb541dedc9..67e05bcee5 100644
@@ -992,6 +992,7 @@ LIB_OBJS += compat/obstack.o
 LIB_OBJS += compat/terminal.o
 LIB_OBJS += compat/zlib-uncompress2.o
 LIB_OBJS += config.o
+LIB_OBJS += config-parse.o
 LIB_OBJS += connect.o
 LIB_OBJS += connected.o
 LIB_OBJS += convert.o

2.42.0.515.g380fc7ccd1-goog

Checking this with --color-moved looks good, but we'll need to take another look once my comments about the earlier patches have been addressed.

This has been in list archive collecting dust. It is unfortunate that not many people appear to be interested in reviewing others' patches?

I gave a quick re-read and found that the code after patch 2 made it easier to see how config.c::do_event() does its thing (even though the patch text of that exact step was somehow a bit hard to follow).

However, the helper added by patch 3, do_event_and_flush(), that duplicates exactly what do_event() does, is hard to reason about, at least for me. It returns early without setting .previous_type to EOF and the value returned from the helper signals if that is the case (the two early return points both return what flush_event() gave us), but the only caller of the helper does not even inspect the return value, unlike all the callers of do_event(), which also looks a bit fishy.

Thanks.

I had similar thoughts while reviewing.

But I am not sure that I agree that this series is moving us in the right direction necessarily. Or at least I am not convinced that shipping the intermediate state is worth doing before we have callers that could drop '#include "config.h"' for just the parser.

This feels like churn that does not yield a tangible pay-off, at least in the sense that the refactoring and code movement delivers us something that we can substantively use today.

I dunno.

Thanks,
Taylor

That matches something I felt but was too polite to say aloud ;-)

Thanks for calling this out. We do want our changes to be good for both the libification and the non-libification cases as much as possible. As it is, I do agree that since we won't have callers that can use the new parser header (I think the likeliest cause of having such a caller is if we have a "interpret-config" command, like "interpret-trailers"), we probably shouldn't merge this (at least, the last 2 patches).

I think patches 1-3 are still usable (they make some internals of config parsing less confusing) but I'm also OK if we hold off on them until we find a compelling use case that motivates refactoring on the config parser.

Thanks everyone for the revived discussion here. I think I agree, this series is not going in the right direction. Additionally, our internal use case for this change has evaporated, so let's just drop the series. We can pick it up again later if interest returns.

OK. Let's scrap it for now.

The "internal use case" behind a proposed feature changing so quickly is a bit worrying. What is good for this project should ideally be good for everybody, not only for satisfying a particular $CORP needs of the day. But I think the idea of giving enhanced visibility into stakeholder companies directions and priorities Emily (I think?) floated during the contributors' summit may help reduce such a risk, hopefully.

Thanks.