Phillip Wood <phillip.wood123@xxxxxxxxx> writes: > On 02/06/2023 17:46, Glen Choo wrote: >> Phillip Wood <phillip.wood123@xxxxxxxxx> writes: >>> 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. > > That's probably the best way to see if it is an improvement - you can > always blame me if it turns out not to be! Hopefully it isn't too much > work to add enough api to be able to convert a couple of config_fn_t > functions to see how it pans out. > I was preparing a reroll that incorporated some of the suggestions here, but when I was done, I didn't really feel like it was better (or at the very least, worth the extra churn) compared to the original approach. The 3 versions I was comparing are: 1. Adding a new "struct config_context" arg to config_fn_t that only contains a .kvi member. This is basically the approach in v1 and v2 (adding an extra args) but wrapping it in a "struct config_context" so that we can grow it later. 2. Stuffing "key" and "value" into "struct key_value_info" and removing "key" and "value" from config_fn_t. 3. Creating a "struct config_context" that contains "key", "value" and "struct key_value_info" as separate members, and removing "key" and "value" from config_fn_t. I didn't try using opaque getters - I think that's nice to have, but isn't crucial to this series and introduces a lot of churn. I've uploaded an implementation of option 3. [1]. The result is quite ugly in some places. In particular, config callbacks are used to calling other config callbacks with slightly different args (e.g. massaging the "key" but keeping the "value" the same, see patches 3,5/14 for examples.), so to make this work, I ended up creating a new "config_context", copying over the relevant members, then changing the members that need to be different. I didn't pursue the refactor to its end state (e.g. some of the CLI config machinery in the earlier patches and git_config_int()/ functions that end in die_bad_number() in later patches, take "key", "value", "key_value_info" as separate args instead of a single "config_context" arg) so I haven't reaped all of the benefits, but I thought the range-diff was getting churny enough that I wasn't interested in pursuing the idea further. I toyed with option 2 for a bit. Unsurprisingly, it requires even more copying of members, though in some ways, it's better than option 3 because callers only have to initialize one "struct key_value_info" instead of "struct key_value_info" _and_ "struct config_context". However, it requires reworking the machinery quite a lot because "key_value_info" is currently used _just_ for config source information, and there are several cases where we need to propagate the config source information separately from the key/value (e.g. the configsets store "key" in a hashmap key, "value" in a string_list_item.string, and "key_value_info" in a string_list_item.util). I'm convinced that this is better interface-wise than option 3, but the required effort is high enough that I don't think we should do this unless we see a real need for it. With that in mind, I'm tempted to continue with option 1 for now, but let me know if I'm missing something. [1] https://github.com/git/git/compare/master...chooglen:git:config/no-global-combined-kv-context?expand=1
