[RFC PATCH 5/5] config API: add and use a repo_config_kvi()

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

Introduce a repo_config_kvi(), which is a repo_config() which calls a
"config_kvi_fn_t", rather than the "config_fn_t" that repo_config()
uses.

This allows us to pass along the "struct key_value_info *" directly,
rather than having the callback grab it from the global
"current_config_kvi" that we've been maintaining in
"configset_iter()".

This change is an alternate direction to the topic at [1], this
expands on the vague suggestions I made in [2] to go in this
direction.

As this shows we can split apart the "config_fn_t", and thus avoid
having to change the hundreds of existing "config_fn_t" callers. By
doing this we can already get rid of the current_config_kvi()
function, as "builtin/remote.c" and "t/helper/test-config.c" were the
only users of it.

The change to "t/t5505-remote.sh" ensures that the change here to
config_read_push_default() isn't breaking things. It's the only test
that would go through that codepath, but nothing asserted that we'd
get the correct line number. Let's sanity check that, as well as the
other callback data.

This leaves the other current_config_*() functions. Subsequent commits
will need to deal with those.

1. https://lore.kernel.org/git/pull.1463.v2.git.git.1678925506.gitgitgadget@gmail.com/
2. https://lore.kernel.org/git/230307.86wn3szrzu.gmgdl@evledraar.gmail.com/
3. https://lore.kernel.org/git/230308.867cvrziac.gmgdl@evledraar.gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/remote.c       | 11 ++++++-----
 config.c               | 32 ++++++++++++++++++--------------
 config.h               |  9 ++++++++-
 t/helper/test-config.c | 13 +++++++------
 t/t5505-remote.sh      |  7 +++++--
 5 files changed, 44 insertions(+), 28 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index 729f6f3643a..c65bce05034 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -644,17 +644,18 @@ struct push_default_info
 };
 
 static int config_read_push_default(const char *key, const char *value,
-	void *cb)
+				    struct key_value_info *kvi, void *cb)
 {
 	struct push_default_info* info = cb;
 	if (strcmp(key, "remote.pushdefault") ||
 	    !value || strcmp(value, info->old_name))
 		return 0;
 
-	info->scope = current_config_scope();
+	info->scope = kvi->scope;
 	strbuf_reset(&info->origin);
-	strbuf_addstr(&info->origin, current_config_name());
-	info->linenr = current_config_line();
+	if (kvi->filename)
+		strbuf_addstr(&info->origin, kvi->filename);
+	info->linenr = kvi->linenr;
 
 	return 0;
 }
@@ -663,7 +664,7 @@ static void handle_push_default(const char* old_name, const char* new_name)
 {
 	struct push_default_info push_default = {
 		old_name, CONFIG_SCOPE_UNKNOWN, STRBUF_INIT, -1 };
-	git_config(config_read_push_default, &push_default);
+	repo_config_kvi(the_repository, config_read_push_default, &push_default);
 	if (push_default.scope >= CONFIG_SCOPE_COMMAND)
 		; /* pass */
 	else if (push_default.scope >= CONFIG_SCOPE_LOCAL) {
diff --git a/config.c b/config.c
index 230a98b0631..1b3f534757c 100644
--- a/config.c
+++ b/config.c
@@ -2219,7 +2219,8 @@ int config_with_options(config_fn_t fn, void *data,
 	return ret;
 }
 
-static void configset_iter(struct config_set *cs, config_fn_t fn, void *data)
+static void configset_iter(struct config_set *cs, config_fn_t fn,
+			   config_kvi_fn_t fn_kvi, void *data)
 {
 	int i, value_index;
 	struct string_list *values;
@@ -2230,6 +2231,7 @@ static void configset_iter(struct config_set *cs, config_fn_t fn, void *data)
 		const char *key;
 		const char *val;
 		struct key_value_info *kvi;
+		int ret;
 
 		entry = list->items[i].e;
 		value_index = list->items[i].value_index;
@@ -2239,11 +2241,15 @@ static void configset_iter(struct config_set *cs, config_fn_t fn, void *data)
 		val = values->items[value_index].string;
 		kvi = values->items[value_index].util;
 
-		current_config_kvi = kvi;
-		if (fn(key, val, data) < 0)
+		if (!fn_kvi)
+			current_config_kvi = kvi;
+		ret = fn_kvi ? fn_kvi(key, val, kvi, data) :
+			fn(key, val, data);
+		current_config_kvi = NULL;
+
+		if (ret < 0)
 			git_die_config_linenr(entry->key, kvi->filename,
 					      kvi->linenr);
-		current_config_kvi = NULL;
 	}
 }
 
@@ -2567,7 +2573,13 @@ static void repo_config_clear(struct repository *repo)
 void repo_config(struct repository *repo, config_fn_t fn, void *data)
 {
 	git_config_check_init(repo);
-	configset_iter(repo->config, fn, data);
+	configset_iter(repo->config, fn, NULL, data);
+}
+
+void repo_config_kvi(struct repository *repo, config_kvi_fn_t fn, void *data)
+{
+	git_config_check_init(repo);
+	configset_iter(repo->config, NULL, fn, data);
 }
 
 int repo_config_get_value(struct repository *repo,
@@ -2670,7 +2682,7 @@ void git_protected_config(config_fn_t fn, void *data)
 {
 	if (!protected_config.hash_initialized)
 		read_protected_config();
-	configset_iter(&protected_config, fn, data);
+	configset_iter(&protected_config, fn, NULL, data);
 }
 
 /* Functions used historically to read configuration from 'the_repository' */
@@ -3843,14 +3855,6 @@ enum config_scope current_config_scope(void)
 		return current_parsing_scope;
 }
 
-int current_config_line(void)
-{
-	if (current_config_kvi)
-		return current_config_kvi->linenr;
-	else
-		return cf->linenr;
-}
-
 int lookup_config(const char **mapping, int nr_mapping, const char *var)
 {
 	int i;
diff --git a/config.h b/config.h
index a9cb01e9405..de5350dbee5 100644
--- a/config.h
+++ b/config.h
@@ -143,6 +143,13 @@ const char *config_origin_type_name(enum config_origin_type type);
  */
 typedef int (*config_fn_t)(const char *, const char *, void *);
 
+/**
+ * Like config_fn_t, but before the callback-specific data we'll get a
+ * "struct key_value_info" indicating the origin of the config.
+ */
+typedef int (*config_kvi_fn_t)(const char *key, const char *var,
+			       struct key_value_info *kvi, void *data);
+
 int git_default_config(const char *, const char *, void *);
 
 /**
@@ -371,7 +378,6 @@ int git_config_parse_parameter(const char *, config_fn_t fn, void *data);
 enum config_scope current_config_scope(void);
 const char *current_config_origin_type(void);
 const char *current_config_name(void);
-int current_config_line(void);
 
 /*
  * Match and parse a config key of the form:
@@ -498,6 +504,7 @@ int git_configset_get_pathname(struct config_set *cs, const char *key, const cha
 /* Functions for reading a repository's config */
 struct repository;
 void repo_config(struct repository *repo, config_fn_t fn, void *data);
+void repo_config_kvi(struct repository *repo, config_kvi_fn_t fn, void *data);
 int repo_config_get_value(struct repository *repo,
 			  const char *key, const char **value);
 const struct string_list *repo_config_get_value_multi(struct repository *repo,
diff --git a/t/helper/test-config.c b/t/helper/test-config.c
index 4ba9eb65606..2ef67b18a0b 100644
--- a/t/helper/test-config.c
+++ b/t/helper/test-config.c
@@ -37,7 +37,8 @@
  *
  */
 
-static int iterate_cb(const char *var, const char *value, void *data UNUSED)
+static int iterate_cb(const char *var, const char *value,
+		      struct key_value_info *kvi, void *data UNUSED)
 {
 	static int nr;
 
@@ -46,10 +47,10 @@ static int iterate_cb(const char *var, const char *value, void *data UNUSED)
 
 	printf("key=%s\n", var);
 	printf("value=%s\n", value ? value : "(null)");
-	printf("origin=%s\n", current_config_origin_type());
-	printf("name=%s\n", current_config_name());
-	printf("lno=%d\n", current_config_line());
-	printf("scope=%s\n", config_scope_name(current_config_scope()));
+	printf("origin=%s\n", config_origin_type_name(kvi->origin_type));
+	printf("name=%s\n", kvi->filename ? kvi->filename : "");
+	printf("lno=%d\n", kvi->linenr);
+	printf("scope=%s\n", config_scope_name(kvi->scope));
 
 	return 0;
 }
@@ -174,7 +175,7 @@ int cmd__config(int argc, const char **argv)
 			goto exit1;
 		}
 	} else if (!strcmp(argv[1], "iterate")) {
-		git_config(iterate_cb, NULL);
+		repo_config_kvi(the_repository, iterate_cb, NULL);
 		goto exit0;
 	}
 
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 43b7bcd7159..d9659b9c65e 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -853,8 +853,11 @@ test_expect_success 'rename a remote' '
 	(
 		cd four &&
 		git config branch.main.pushRemote origin &&
-		GIT_TRACE2_EVENT=$(pwd)/trace \
-			git remote rename --progress origin upstream &&
+		GIT_TRACE2_EVENT=$PWD/trace \
+			git remote rename --progress origin upstream 2>warn &&
+		grep -F "The global configuration remote.pushDefault" warn &&
+		grep "/\.gitconfig:2$" warn &&
+		grep "remote '\''origin'\''" warn &&
 		test_region progress "Renaming remote references" trace &&
 		grep "pushRemote" .git/config &&
 		test -z "$(git for-each-ref refs/remotes/origin)" &&
-- 
2.40.0.rc1.1034.g5867a1b10c5