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

From: Glen Choo <chooglen@google.com>
Date: Jun 2, 2023, 16:46 UTC
Message-ID: <kl6lv8g5zu9v.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <4de362b4-dcce-3b5a-0011-73dc7dec79c3@gmail.com>

Phillip Wood <phillip.wood123@gmail.com> writes:

Show 14 quoted lines

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

Ah, yes that seems reasonable to review (and most importantly for me, it is also doable with coccinelle once I figure out how :P). I also agree that it's better to do it all in one change than two.

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

Yes, and that would also abstract over implementation details like matching keys using strcasecmp() and not strcmp(). For reasons like this, I think your proposal paves the way for a harder-to-misuse API.

I still have some nagging, probably irrational fear that consolidating all of the config_fn_t args is trickier to manage than adding a single key_value_info arg. It definitely *sounds* trickier, but I can't really think of a real downside.

Maybe I just have to try it and send the result for others to consider.