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

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

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

> After reflecting on Ævar's responses on v1, I'm fairly convinced that
> "struct config_reader" shouldn't exist in the long term. I've written my
> thoughts on a good long term direction in the "Leftover bits" section. Based
> on that, I've also updated my WIP libification patches [1] to remove "struct
> config_reader" from the library interface, and think it looks a lot better
> as a result.

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

> = Leftover bits
>
> We still need a global "the_reader" because config callbacks are reading
> auxiliary information about the config (e.g. line number, file name) via
> global functions (e.g. current_config_line(), current_config_name()). This
> is either because the callback uses this info directly (like
> builtin/config.c printing the filename and scope of the value) or for error
> reporting (like git_parse_int() reporting the filename of the value it
> failed to parse).
>
> If we had a way to plumb the state from "struct config_reader" to the config
> callback functions, we could initialize "struct config_reader" in the config
> machinery whenever we read config (instead of asking the caller to
> initialize "struct config_reader" themselves), and config reading could
> become a thread-safe operation. There isn't an obvious way to plumb this
> state to config callbacks without adding an additional arg to config_fn_t
> and incurring a lot of churn, but if we start replacing "config_fn_t" with
> the configset API (which we've independently wanted for some time), this may
> become feasible.

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

> And if we do this, "struct config_reader" itself will probably become
> obsolete, because we'd be able to plumb only the relevant state for the
> current operation, e.g. if we are parsing a config file, we'd pass only the
> config file parsing state, instead of "struct config_reader", which also
> contains config set iterating state. In such a scenario, we'd probably want
> to pass "struct key_value_info" to the config callback, since that's all the
> callback should be interested in anyway. Interestingly, this was proposed by
> Junio back in [4], and we didn't do this back then out of concern for the
> churn (just like in v1).

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

-- 
2.40.0.rc1.1034.g5867a1b10c5