[RFC PATCH 0/5] bypass config.c global state with configset

On Thu, Mar 16 2023, Glen Choo via GitGitGadget wrote:

That libification url (https://github.com/git/git/compare/master...chooglen:git:config-lib-parsing) doesn't work for me, and I didn't find a branch with that name in your published repo. So maybe you've already done all this post-libification work...

...in any case. This RFC expands a bit on my comments on the v1 (at [1] and upthread). It doesn't get all the way there, but with the small change in 5/5 we've gotten rid of current_config_line(), the 1-4/5 are trivial pre-refactorings to make that diff smaller (e.g. moving the "struct key_value_info" around in config.h).

Maybe it still makes sense to go for this "the_reader" intermediate step, but I can't help but think that we could just go for it all in one leap, and that you've just got stuck on thinking that you needed to change "config_fn_t" for all its callers.

As the 5/5 here shows we have various orthagonal uses of the "config_fn_t" in config.c, and can just implement a new callback type for the edge cases where we need the file & line info.

This still leave the current_config_name() etc, which e.g. builtin/config.c still uses. In your series you've needed to add the new "reader" parameter for everything from do_config_from(), but if we're doing that can't we instead just go straight to passing a "struct key_value_info *" (perhaps with an added "name" field) all the way down, replacing "cf->linenr" etc?

Instead you end up extending "the_reader" everywhere, including to e.g. configset_iter, which I think as the 5/5 here shows isn't needed, but maybe I've missed something.

Similarly, you mention git_parse_int() wanting to report a filename and/or line number. I'm aware that it can do that, but it doesn't do so in the common case, e.g.:

	git -c format.filenameMaxLength=abc log
	fatal: bad numeric config value 'abc' for 'format.filenamemaxlength': invalid unit

And the same goes for writing it to e.g. ~/.gitconfig. It's only if you use "git config --file" or similar that we'll report a filename.

So just as with the current_config_line() I wonder if you just grepped for e.g. git_config_int() and thought because we have a lot of users of it that all of them would require this data, but for e.g. this log.c caller (and most or all of the others) we'll be reading the normal config, and aren't getting any useful info from die_bad_number() that we wouldn't get from an error function that didn't need the "linenr" etc.

I think we can make it even simpler than that, and from playing around with builtin/config.c a bit after the 5/5 here I got a POC working (but am not posting it here, didn't have time to clean it up).

We can just make config_set_callback() and configset_iter() non-static, so e.g. the builtin/config.c caller that implements "--show-origin" can keep its config_with_options(...) call, but instead of "streaming" the config, it'll buffer it up into a configset.

The advantage of that is that with the configset API we'll get a "struct key_value_info *" for free on the other end. I.e. we'll configset_iter() with a fn=NULL, but with a defined "config_kvi_fn_t", which 5/5 is adding.

But I haven't done that work (and am not planning to finish this), but maybe this helps.

We'll also need to track the equivalent of "cf->linenr" etc. while we do the actual initial parse. I think it might be simpler to start by converting those "linenr" to a "struct key_value_info" that's placed in the "config_source" right away.

I.e. when we pass it to the error handlers we'll need to give them access to the "linenr", but we don't want to provide e.g. "eof" (which is internal-only state).

I wonder how much else you're converting here is actually dead code in the end (or can trivially be made dead). E.g. the current_config_line() change you make in 1/8 is never going to use the "cf_global" if combined with the 5/5 change here.

1. https://lore.kernel.org/git/230308.867cvrziac.gmgdl@evledraar.gmail.com/

Ævar Arnfjörð Bjarmason (5):
  config.h: move up "struct key_value_info"
  config.c: use "enum config_origin_type", not "int"
  config API: add a config_origin_type_name() helper
  config.c: refactor configset_iter()
  config API: add and use a repo_config_kvi()

 builtin/remote.c       | 11 +++----
 config.c               | 67 ++++++++++++++++++++++++++----------------
 config.h               | 29 +++++++++++++-----
 t/helper/test-config.c | 13 ++++----
 t/t5505-remote.sh      |  7 +++--
 5 files changed, 80 insertions(+), 47 deletions(-)