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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Mar 8, 2023, 10:32 UTC
Message-ID: <230308.86y1o7y0jc.gmgdl@evledraar.gmail.com>
In-Reply-To: <20230306223804.3414251-1-jonathantanmy@google.com>

On Mon, Mar 06 2023, Jonathan Tan wrote:

Show 22 quoted lines

> Glen Choo <chooglen@google.com> 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.