Jonathan Tan <jonathantanmy@xxxxxxxxxx> writes: > "Glen Choo via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes: >> * We could create a new kind of "config_fn_t" that accepts "struct >> config_reader", e.g. >> >> typedef int (*config_fn_t)(const char *var, const char *val, void *data); >> +typedef int (*config_state_fn_t)(const struct config_reader *reader, >> const char *var, const char *val, void *data); >> >> and only adjust the callers that would actually reference "reader". This >> is less churn, but I couldn't find a great way to do this kind of >> 'switching between config callback types' elegantly. > > To reduce churn, one thing that could be done alongside is to convert > config-using code (which is...practically the rest of Git) to start > using the configset interface (we seem to be using configsets internally > anyway, as evidenced by repo_config()). That way, we would reduce the > number of implementations of config_fn_t. By configset interface, I believe you mean the O(1) lookup functions like git_config_get_int() (which rely on the value being cached, but don't necessarily accept "struct config_set" as an arg)? I think that makes sense both from a performance and maintenance perspective. >> = Questions >> >> * Is this worth merging without the extra work? There are some cleanups in >> this series that could make it valuable, but there are also some hacks >> (see 4/6) that aren't so great. > I don't see 4/6 as a hack. It is true that the nature of the config_fn_t > callback could change so that passing the reader would end up being > done in yet another different way, but firstly, I don't think that will > happen for quite some time, and secondly, it might not happen at all > (right now, I think what's most likely to happen is that the rest of the > Git code moves to configsets and only a fraction of the Git code would > need to do low-level parsing, which would not have a problem passing the > reader through the data object since they would probably need to pass > other context anyway). Given how painful it is to change the config_fn_t signature, I think it is important to get as right as possible the first time. After I sent this out, I thought of yet another possible config_fn_t signature (since most callbacks only need diagnostic information, we could pass "struct key_value_info" instead of the more privileged "struct config_reader"), but given how many functions we'd have to change, it seemed extremely difficult to even begin experimenting with this different signature. So I think you're right that we'd want to start by removing as many config_fn_t implementations as possible. Perhaps we'd also want to consider creating new abstractions for other situations where the key is not known, e.g. "remote.<name>" and "branch.<name>".
