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

[Glen Choo <chooglen@google.com>]

Jonathan Tan <jonathantanmy@google.com> writes:

> "Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>  * We could create a new kind of "config_fn_t" that accepts "struct
>>    config_reader", e.g.
>>    
>>    typedef int (*config_fn_t)(const char *var, const char *val, void *data);
>>    +typedef int (*config_state_fn_t)(const struct config_reader *reader,
>>    const char *var, const char *val, void *data);
>>    
>>    and only adjust the callers that would actually reference "reader". This
>>    is less churn, but I couldn't find a great way to do this kind of
>>    'switching between config callback types' elegantly.
>
> To reduce churn, one thing that could be done alongside is to convert
> config-using code (which is...practically the rest of Git) to start
> using the configset interface (we seem to be using configsets internally
> anyway, as evidenced by repo_config()). That way, we would reduce the
> number of implementations of config_fn_t.

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.

>> = Questions
>> 
>>  * Is this worth merging without the extra work? There are some cleanups in
>>    this series that could make it valuable, but there are also some hacks
>>    (see 4/6) that aren't so great.
> I don't see 4/6 as a hack. It is true that the nature of the config_fn_t
> callback could change so that passing the reader would end up being
> done in yet another different way, but firstly, I don't think that will
> happen for quite some time, and secondly, it might not happen at all
> (right now, I think what's most likely to happen is that the rest of the
> Git code moves to configsets and only a fraction of the Git code would
> need to do low-level parsing, which would not have a problem passing the
> reader through the data object since they would probably need to pass
> other context anyway).

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.

So I think you're right that we'd want to start by removing as many
config_fn_t implementations as possible. Perhaps we'd also want to
consider creating new abstractions for other situations where the key is
not known, e.g. "remote.<name>" and "branch.<name>".