Subject: Re: [PATCH v2 03/14] (RFC-only) config: add kvi arg to config_fn_t

From: Phillip Wood <[email-masked]>
Date: Fri, 2 Jun 2023 10:54:25 +0100

On 01/06/2023 17:22, Glen Choo wrote:
> Phillip Wood <[email-masked]> writes:
> 
>>    - is it worth making struct key_value_info opaque and provide getters
>>      for the fields so we can change the implementation in the future
>>      without having to modify every user. We could rename it
>>      config_context or something generic like that if we think it might
>>      grow in scope in the future.
> 
> Yes! I planned to do the key_value_info -> config_context conversion
> when I send the first non-RFC version for this exact reason.

That's great

>>    - (probably impractical) could we stuff the key and value into struct
>>      key_value_info so config_fn_t becomes
>>      fn(const struct key_value_info, void *data)
>>      that would get rid of all the UNUSED annotations but would mean even
>>      more churn.
> 
> Some of my colleagues also suggested this off-list. I think it is
> impractical for this series because I don't think anyone could
> reasonably review with all of the added churn. At least its current
> form, the churn is mostly concentrated in the signatures, but performing
> ^this change would make the bodies quite churny too.

I agree that keeping the churn to the function signatures makes it 
bearable. I wonder though if we could make the change by doing

-git_default_config(const char *key, const char *value, void *data)
+git_default_config(const struct key_value_info *kvi, void *data)
  {
+	const char *key = kvi_key(kvi);
+	const char *value = kvi_value(kvi);
+

That would add to the diffstat but I think it wouldn't really be any 
harder to review than just changing the signature as we're not modifying 
any existing lines in the function body, just adding a couple of 
variable declarations to the start of the function. If there is an error 
in either of the variable declarations then the compiler will complain 
as "key" or "value" will end up not being declared. It would pave the 
way for gradually changing the function bodies to use "kvi" directly and 
removing "key" and "value"

> After this series, I think it becomes somewhat feasible with coccinelle.
> My .cocci files were difficult to craft because we couldn't rely on the
> signature of config_fn_t alone to tell us if the function is actually
> used as config_fn_t, but after this series, we can just use the
> signature since config_fn_t has a struct key_value_info param.

That's an interesting possibility, I worry though that two huge changes 
to the config callbacks might be one too many though.

>>      The advantage is that one could add functions like
>>      kvi_bool_or_int(kvi, &is_bool) and get good error messages because
>>      all the config parsing functions would all have access to location
>>      information.
> 
> Interesting, I hadn't considered this possibility. This seems like a
> pretty good abstraction to me, though I worry about the feasibility
> since this is yet again more churn.

It could be done gradually though, converting one config callback at a 
time once the relevant changes have been made to config.c.

As an aside, I think we'd also want a couple of helpers for matching 
keys so we can just write kvi_match_key(kvi, "user.name") or 
kvi_skip_key_prefix(kvi, "core.", &p) rather than having to extract the 
key name first.

Best Wishes

Phillip