Subject: Re: [PATCH 0/6] [RFC] config.c: use struct for config reading state

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Wed, 08 Mar 2023 11:32:16 +0100

On Mon, Mar 06 2023, Jonathan Tan wrote:

> Glen Choo <[email-masked]> writes:
>> By configset interface, I believe you mean the O(1) lookup functions
>> like git_config_get_int() (which rely on the value being cached, but
>> don't necessarily accept "struct config_set" as an arg)? I think that
>> makes sense both from a performance and maintenance perspective.
>
> Ah, yes. (More precisely, not the one where you call something like
> repo_config(), passing a callback function that.)
>
>> Given how painful it is to change the config_fn_t signature, I think it
>> is important to get as right as possible the first time. After I sent
>> this out, I thought of yet another possible config_fn_t signature
>> (since most callbacks only need diagnostic information, we could pass
>> "struct key_value_info" instead of the more privileged "struct
>> config_reader"), but given how many functions we'd have to change, it
>> seemed extremely difficult to even begin experimenting with this
>> different signature.
>
> Yeah, the first change is the hardest. I think passing it a single
> struct (so, instead of key, value, data, reader, and then any future
> fields we would need) to which we can add fields later would mean that
> we wouldn't need any changes beyond the first, though.

The more I've looked at this the more I'm convinced this is the wrong
direction.

For the configset API users we already have the line number, source
etc. in the "util" member, i.e. when we have an error in any API user
that uses the configset they can error about the specific line that
config came from.

I think this may have been conflated because e.g. for the configset to
get the "scope" we need to go from do_git_config_sequence(), which will
currently set "current_parsing_scope", all the way down to
configset_add_value(), and there we'll make use of the
"config_set_callback", which is a config_fn_t.

But that's all internal "static" functions, except
git_config_from_file() and git_config_from_file_with_options(), but
those have only a handful of callers.

But that's *different* than the user callbacks, which will be invoked
through a loop in configset_iter(), i.e. *after* we've parsed the
config, and are just getting the line number, scope etc. from the
configset.

There's other edge cases, e.g. current_config_line() will access the
global, but it only has two callers (one if we exclude the test
helper). But I think the answer there is to change the
config_read_push_default() code, not to give every current "config_fn_t"
implementation an extra parameter.