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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Jun 2, 2023, 09:54 UTC
Message-ID: <4de362b4-dcce-3b5a-0011-73dc7dec79c3@gmail.com>
In-Reply-To: <kl6l4jnr17am.fsf@chooglen-macbookpro.roam.corp.google.com>

On 01/06/2023 17:22, Glen Choo wrote:

Show 10 quoted lines

> Phillip Wood <phillip.wood123@gmail.com> 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

Show 11 quoted lines

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

Show 5 quoted lines

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

Show 8 quoted lines

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