Re: [PATCH 10/14] (RFC-only) config: finish config_fn_t refactor

On Fri, Apr 21 2023, Glen Choo via GitGitGadget wrote:

I like the general goal of this series, i.e. to get rid of the "reader" callback, and pass stuff more explicitly.

But as I pointed out in https://lore.kernel.org/git/RFC-cover-0.5-00000000000-20230317T042408Z-avarab@gmail.com/ I think this part (and the preceding two commits) are really taking is down the wrong path.

To demonstrate why, here's a patch-on-top of this topic as a whole where
I renamed the "kvi" struct members. I've excluded config.c itself (as
its internals aren't interesting for the purposes of this discussion):
	
	diff --git a/builtin/config.c b/builtin/config.c
	index c9e80a4b500..00cf8ffd791 100644
	--- a/builtin/config.c
	+++ b/builtin/config.c
	@@ -198,8 +198,8 @@ static void show_config_origin(struct key_value_info *kvi, struct strbuf *buf)
	 
	-	strbuf_addstr(buf, config_origin_type_name(kvi->origin_type));
	+	strbuf_addstr(buf, config_origin_type_name(kvi->origin_type2));
	 	strbuf_addch(buf, ':');
	 	if (end_nul)
	-		strbuf_addstr(buf, kvi->filename ? kvi->filename : "");
	+		strbuf_addstr(buf, kvi->filename2 ? kvi->filename2 : "");
	 	else
	-		quote_c_style(kvi->filename ? kvi->filename : "", buf, NULL, 0);
	+		quote_c_style(kvi->filename2 ? kvi->filename2 : "", buf, NULL, 0);
	 	strbuf_addch(buf, term);
	@@ -210,3 +210,3 @@ static void show_config_scope(struct key_value_info *kvi, struct strbuf *buf)
	 	const char term = end_nul ? '\0' : '\t';
	-	const char *scope = config_scope_name(kvi->scope);
	+	const char *scope = config_scope_name(kvi->scope2);
	 
	diff --git a/builtin/remote.c b/builtin/remote.c
	index 034998a1205..81922af3f58 100644
	--- a/builtin/remote.c
	+++ b/builtin/remote.c
	@@ -655,6 +655,6 @@ static int config_read_push_default(const char *key, const char *value,
	 
	-	info->scope = kvi->scope;
	+	info->scope = kvi->scope2;
	 	strbuf_reset(&info->origin);
	-	strbuf_addstr(&info->origin, kvi->filename);
	-	info->linenr = kvi->linenr;
	+	strbuf_addstr(&info->origin, kvi->filename2);
	+	info->linenr = kvi->linenr2;
	 
	diff --git a/config.h b/config.h
	index ffb2d76823c..d9b0470e7b7 100644
	--- a/config.h
	+++ b/config.h
	@@ -120,8 +120,8 @@ struct config_options {
	 struct key_value_info {
	-	const char *filename;
	-	int linenr;
	-	enum config_origin_type origin_type;
	-	enum config_scope scope;
	-	const char *path;
	-	struct key_value_info *prev;
	+	const char *filename2;
	+	int linenr2;
	+	enum config_origin_type origin_type2;
	+	enum config_scope scope2;
	+	const char *path2;
	+	struct key_value_info *prev2;
	 };
	diff --git a/remote.c b/remote.c
	index 5239dfeab55..1cb465c6c17 100644
	--- a/remote.c
	+++ b/remote.c
	@@ -417,4 +417,4 @@ static int handle_config(const char *key, const char *value,
	 	remote->origin = REMOTE_CONFIG;
	-	if (kvi->scope == CONFIG_SCOPE_LOCAL ||
	-	    kvi->scope == CONFIG_SCOPE_WORKTREE)
	+	if (kvi->scope2 == CONFIG_SCOPE_LOCAL ||
	+	    kvi->scope2 == CONFIG_SCOPE_WORKTREE)
	 		remote->configured_in_repo = 1;
	diff --git a/t/helper/test-config.c b/t/helper/test-config.c
	index 737505583d4..fa89cdd084c 100644
	--- a/t/helper/test-config.c
	+++ b/t/helper/test-config.c
	@@ -54,6 +54,6 @@ static int iterate_cb(const char *var, const char *value,
	 	printf("value=%s\n", value ? value : "(null)");
	-	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));
	+	printf("origin=%s\n", config_origin_type_name(kvi->origin_type2));
	+	printf("name=%s\n", kvi->filename2 ? kvi->filename2 : "");
	+	printf("lno=%d\n", kvi->linenr2);
	+	printf("scope=%s\n", config_scope_name(kvi->scope2));
	 
	diff --git a/trace2/tr2_tgt_event.c b/trace2/tr2_tgt_event.c
	index 83db3c755bd..338c5a58bd9 100644
	--- a/trace2/tr2_tgt_event.c
	+++ b/trace2/tr2_tgt_event.c
	@@ -482,3 +482,3 @@ static void fn_param_fl(const char *file, int line, const char *param,
	 	struct json_writer jw = JSON_WRITER_INIT;
	-	enum config_scope scope = kvi->scope;
	+	enum config_scope scope = kvi->scope2;
	 	const char *scope_name = config_scope_name(scope);
	diff --git a/trace2/tr2_tgt_normal.c b/trace2/tr2_tgt_normal.c
	index 65e9be9c5a4..5a06042e7c3 100644
	--- a/trace2/tr2_tgt_normal.c
	+++ b/trace2/tr2_tgt_normal.c
	@@ -301,3 +301,3 @@ static void fn_param_fl(const char *file, int line, const char *param,
	 	struct strbuf buf_payload = STRBUF_INIT;
	-	enum config_scope scope = kvi->scope;
	+	enum config_scope scope = kvi->scope2;
	 	const char *scope_name = config_scope_name(scope);
	diff --git a/trace2/tr2_tgt_perf.c b/trace2/tr2_tgt_perf.c
	index f402f6e3813..96fa359183d 100644
	--- a/trace2/tr2_tgt_perf.c
	+++ b/trace2/tr2_tgt_perf.c
	@@ -445,3 +445,3 @@ static void fn_param_fl(const char *file, int line, const char *param,
	 	struct strbuf scope_payload = STRBUF_INIT;
	-	enum config_scope scope = kvi->scope;
	+	enum config_scope scope = kvi->scope2;
	 	const char *scope_name = config_scope_name(scope);

So, as this shows us your 08/14 has gone through the effort of passing this "kvi" info to every single callback, but it's only this handful that actually needs this information.

So, even if we *can* get this to work I don't think it's worth it, especially as this would preclude giving these config callbacks some "lighter" API that doesn't take the trouble of recording and ferrying this information to them.

Of course *now* we need to always prepare this information anyway, as anyone could access it via a global, but as the work you've done here shows we're always doing that, but only need it for these few cases.

So I really think we could leave the vast majority of the current callbacks alone, and just supply a new "kvi" callback. My https://lore.kernel.org/git/RFC-patch-5.5-2b80d293c83-20230317T042408Z-avarab@gmail.com/ showed one way forward with that.

I think this should also neatly answer some of your outstanding questions. Especially as the above shows that the only non-test caller that needs "linenr" is the builtin/config.c caller that my proposed RFC (linked above) tackled directly. Most of these callbacks just need the more basic "scope".

So, in particular:

Just adding a "CONFIG_SCOPE_IN_PROCESS", "CONFIG_SCOPE_SET" or whatever you'd want to call it seems to make much more sense here, no?

Another example of a caller which never actually cares about this data, so if it doesn't need to have it passed to it, it doesn't need to fake it up either.

No comment (internal to config.c, as noted above).

All of these problems go away if you don't insist on changing every single caller, you'll just have a step where you remove the current global in favor of some "config callback with kvi" info, and "make" will spot those callers that aren't converted yet.

Those changes will be trivial enough (just the callers I noted above) to not require the tricky cocci patch in 08/14.

ditto.