Phillip Wood <phillip.wood123@xxxxxxxxx> writes:
>> 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.
