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

From: Glen Choo <chooglen@google.com>
Date: Mar 6, 2023, 21:45 UTC
Message-ID: <kl6l356heeqb.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <20230306195756.3399115-1-jonathantanmy@google.com>

Jonathan Tan <jonathantanmy@google.com> writes:

Show 17 quoted lines

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

Show 14 quoted lines

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