| Message ID | 1071e70c92892166e1ed2cf22bcd7eb49bdf3b20.1682104399.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | config: remove global state from config iteration | expand |
On Fri, Apr 21 2023, Glen Choo via GitGitGadget wrote: > From: Glen Choo <chooglen@google.com> I like the general goal of this series, i.e. to get rid of the "reader" callback, and pass stuff more explicitly. But as I pointed out in https://lore.kernel.org/git/RFC-cover-0.5-00000000000-20230317T042408Z-avarab@gmail.com/ I think this part (and the preceding two commits) are really taking is down the wrong path. To demonstrate why, here's a patch-on-top of this topic as a whole where I renamed the "kvi" struct members. I've excluded config.c itself (as its internals aren't interesting for the purposes of this discussion): diff --git a/builtin/config.c b/builtin/config.c index c9e80a4b500..00cf8ffd791 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -198,8 +198,8 @@ static void show_config_origin(struct key_value_info *kvi, struct strbuf *buf) - strbuf_addstr(buf, config_origin_type_name(kvi->origin_type)); + strbuf_addstr(buf, config_origin_type_name(kvi->origin_type2)); strbuf_addch(buf, ':'); if (end_nul) - strbuf_addstr(buf, kvi->filename ? kvi->filename : ""); + strbuf_addstr(buf, kvi->filename2 ? kvi->filename2 : ""); else - quote_c_style(kvi->filename ? kvi->filename : "", buf, NULL, 0); + quote_c_style(kvi->filename2 ? kvi->filename2 : "", buf, NULL, 0); strbuf_addch(buf, term); @@ -210,3 +210,3 @@ static void show_config_scope(struct key_value_info *kvi, struct strbuf *buf) const char term = end_nul ? '\0' : '\t'; - const char *scope = config_scope_name(kvi->scope); + const char *scope = config_scope_name(kvi->scope2); diff --git a/builtin/remote.c b/builtin/remote.c index 034998a1205..81922af3f58 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -655,6 +655,6 @@ static int config_read_push_default(const char *key, const char *value, - info->scope = kvi->scope; + info->scope = kvi->scope2; strbuf_reset(&info->origin); - strbuf_addstr(&info->origin, kvi->filename); - info->linenr = kvi->linenr; + strbuf_addstr(&info->origin, kvi->filename2); + info->linenr = kvi->linenr2; diff --git a/config.h b/config.h index ffb2d76823c..d9b0470e7b7 100644 --- a/config.h +++ b/config.h @@ -120,8 +120,8 @@ struct config_options { struct key_value_info { - const char *filename; - int linenr; - enum config_origin_type origin_type; - enum config_scope scope; - const char *path; - struct key_value_info *prev; + const char *filename2; + int linenr2; + enum config_origin_type origin_type2; + enum config_scope scope2; + const char *path2; + struct key_value_info *prev2; }; diff --git a/remote.c b/remote.c index 5239dfeab55..1cb465c6c17 100644 --- a/remote.c +++ b/remote.c @@ -417,4 +417,4 @@ static int handle_config(const char *key, const char *value, remote->origin = REMOTE_CONFIG; - if (kvi->scope == CONFIG_SCOPE_LOCAL || - kvi->scope == CONFIG_SCOPE_WORKTREE) + if (kvi->scope2 == CONFIG_SCOPE_LOCAL || + kvi->scope2 == CONFIG_SCOPE_WORKTREE) remote->configured_in_repo = 1; diff --git a/t/helper/test-config.c b/t/helper/test-config.c index 737505583d4..fa89cdd084c 100644 --- a/t/helper/test-config.c +++ b/t/helper/test-config.c @@ -54,6 +54,6 @@ static int iterate_cb(const char *var, const char *value, printf("value=%s\n", value ? value : "(null)"); - printf("origin=%s\n", config_origin_type_name(kvi->origin_type)); - printf("name=%s\n", kvi->filename ? kvi->filename : ""); - printf("lno=%d\n", kvi->linenr); - printf("scope=%s\n", config_scope_name(kvi->scope)); + printf("origin=%s\n", config_origin_type_name(kvi->origin_type2)); + printf("name=%s\n", kvi->filename2 ? kvi->filename2 : ""); + printf("lno=%d\n", kvi->linenr2); + printf("scope=%s\n", config_scope_name(kvi->scope2)); diff --git a/trace2/tr2_tgt_event.c b/trace2/tr2_tgt_event.c index 83db3c755bd..338c5a58bd9 100644 --- a/trace2/tr2_tgt_event.c +++ b/trace2/tr2_tgt_event.c @@ -482,3 +482,3 @@ static void fn_param_fl(const char *file, int line, const char *param, struct json_writer jw = JSON_WRITER_INIT; - enum config_scope scope = kvi->scope; + enum config_scope scope = kvi->scope2; const char *scope_name = config_scope_name(scope); diff --git a/trace2/tr2_tgt_normal.c b/trace2/tr2_tgt_normal.c index 65e9be9c5a4..5a06042e7c3 100644 --- a/trace2/tr2_tgt_normal.c +++ b/trace2/tr2_tgt_normal.c @@ -301,3 +301,3 @@ static void fn_param_fl(const char *file, int line, const char *param, struct strbuf buf_payload = STRBUF_INIT; - enum config_scope scope = kvi->scope; + enum config_scope scope = kvi->scope2; const char *scope_name = config_scope_name(scope); diff --git a/trace2/tr2_tgt_perf.c b/trace2/tr2_tgt_perf.c index f402f6e3813..96fa359183d 100644 --- a/trace2/tr2_tgt_perf.c +++ b/trace2/tr2_tgt_perf.c @@ -445,3 +445,3 @@ static void fn_param_fl(const char *file, int line, const char *param, struct strbuf scope_payload = STRBUF_INIT; - enum config_scope scope = kvi->scope; + enum config_scope scope = kvi->scope2; const char *scope_name = config_scope_name(scope); So, as this shows us your 08/14 has gone through the effort of passing this "kvi" info to every single callback, but it's only this handful that actually needs this information. So, even if we *can* get this to work I don't think it's worth it, especially as this would preclude giving these config callbacks some "lighter" API that doesn't take the trouble of recording and ferrying this information to them. Of course *now* we need to always prepare this information anyway, as anyone could access it via a global, but as the work you've done here shows we're always doing that, but only need it for these few cases. So I really think we could leave the vast majority of the current callbacks alone, and just supply a new "kvi" callback. My https://lore.kernel.org/git/RFC-patch-5.5-2b80d293c83-20230317T042408Z-avarab@gmail.com/ showed one way forward with that. I think this should also neatly answer some of your outstanding questions. Especially as the above shows that the only non-test caller that needs "linenr" is the builtin/config.c caller that my proposed RFC (linked above) tackled directly. Most of these callbacks just need the more basic "scope". So, in particular: > Here's an exhaustive list of all of the changes: > > * Cases that need a judgement call > > - trace2/tr2_cfg.c:tr2_cfg_set_fl() > > This function needs to account for tr2_cfg_cb() now using "kvi". > Since this is only called (indirectly) by git_config_set(), config > source information has never been available here, so just pass NULL. > It will be tr2_cfg_cb()'s responsibility to not use "kvi". Just adding a "CONFIG_SCOPE_IN_PROCESS", "CONFIG_SCOPE_SET" or whatever you'd want to call it seems to make much more sense here, no? > - builtin/checkout.c:checkout_main() > > This calls git_xmerge_config() as a shorthand for parsing a CLI arg. > "kvi" doesn't apply, so just pass NULL. This might be worth > refactoring away, since git_xmerge_config() can call > git_default_config(). Another example of a caller which never actually cares about this data, so if it doesn't need to have it passed to it, it doesn't need to fake it up either. > - config.c:git_config_include() > > Replace the local "kvi" variable with the "kvi" parameter. This > makes config_include_data.config_reader obsolete, so remove it. No comment (internal to config.c, as noted above). > * Hard for cocci to catch > > - urlmatch.c > > Manually refactor the custom config callbacks in "struct > urlmatch_config". > > - diff.h, fsck.h, grep.h, ident.h, xdiff-interface.h > > "struct key_value_info" hasn't been defined yet, so forward declare > it. Alternatively, maybe these files should "#include config.h". All of these problems go away if you don't insist on changing every single caller, you'll just have a step where you remove the current global in favor of some "config callback with kvi" info, and "make" will spot those callers that aren't converted yet. Those changes will be trivial enough (just the callers I noted above) to not require the tricky cocci patch in 08/14. > * Likely deficiencies in .cocci patch > > - submodule-config.c:gitmodules_cb() > > Manually refactor a parse_config() call that gets missed because it > uses a different "*data" arg. > > - grep.h, various > > Manually refactor grep_config() calls. Not sure why these don't get > picked up. > > - config.c:git_config_include(), http.c:http_options() > > Manually add "kvi" where it was missed. Not sure why they get missed. > > - builtin/clone.c:write_one_config() > > Manually refactor a git_clone_config() call. Probably got missed > because I didn't include git_config_parse_parameter(). > > - ident.h > > Remove the UNUSED attribute. Not sure why this is the only instance > of this. > > - git-compat-util.h, compat/mingw.[h|c] > > Manually refactor noop_core_config(), platform_core_config() and > mingw_core_config(). I can probably add these as "manual fixups" in > cocci. ditto.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > But as I pointed out in > https://lore.kernel.org/git/RFC-cover-0.5-00000000000-20230317T042408Z-avarab@gmail.com/ > I think this part (and the preceding two commits) are really taking is > down the wrong path. > > To demonstrate why, here's a patch-on-top of this topic as a whole where > I renamed the "kvi" struct members. I've excluded config.c itself (as > its internals aren't interesting for the purposes of this discussion): [snip patch showing where kvi is used] Ah, thanks for this patch. One of my objections to your proposal in the aforementioned thread is that we would end up with a lot of callback- taking config functions that have 2 variants, one for the kvi callback and one for the non-kvi callback, but here your patch shows that it won't be the case. Your approach does look like a reasonable one. As for my own opinions, (before Ævar sent this email) I took a look at these patches myself and had some issues with at least the first 2: in patch 1, kvi_fn() replaces fn() for some but not all invocations of fn() (in patch 2, you can see one such invocation that was not changed), and I was having difficulty thinking of what kind of bugs I should watch out for since not all invocations were changed; and in patch 2, the safeguard of not setting kvi and source together was removed and likewise I was having difficulty thinking of what kind of bugs could occur from both being set at once inadvertently. I was going to suggest reordering the patches such that the large-scale refactoring (and any supporting patches like [PATCH 06/14] config: inline git_color_default_config) should occur first (or waiting for a reviewer who is convinced that patches 1 and 2 are OK, I guess), but having now seen that sidestepping a large part of this makes sense, sidestepping seems like a good idea to me.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > On Fri, Apr 21 2023, Glen Choo via GitGitGadget wrote: > >> From: Glen Choo <chooglen@google.com> > > I like the general goal of this series, i.e. to get rid of the "reader" > callback, and pass stuff more explicitly. > > But as I pointed out in > https://lore.kernel.org/git/RFC-cover-0.5-00000000000-20230317T042408Z-avarab@gmail.com/ > I think this part (and the preceding two commits) are really taking is > down the wrong path. > > To demonstrate why, here's a patch-on-top of this topic as a whole where > I renamed the "kvi" struct members. I've excluded config.c itself (as > its internals aren't interesting for the purposes of this discussion): > > [Patch showing used kvi members...] > > So, as this shows us your 08/14 has gone through the effort of passing > this "kvi" info to every single callback, but it's only this handful > that actually needs this information. The attached patch misses the majority of "kvi" users, which use it in die_bad_number(). This change happens in Patch 13/14, where the whole "kvi" is passed into config.c machinery. It's still a much smaller number of config callbacks than the ones changed in these few patches, but it's more than a handful, I'd think. > So, even if we *can* get this to work I don't think it's worth it, > especially as this would preclude giving these config callbacks some > "lighter" API that doesn't take the trouble of recording and ferrying > this information to them. I assume the "lighter API" refers to the current API? If I'm reading this correctly, your concern isn't primarily about reducing churn, it's YAGNI - since most callers don't need this info, they should be able to reap the benefits of an API that doesn't provide that info. Thus, you're proposing to having both the current API and a new kvi-based one? If so, I don't think that's a good route to take: - Having both *_kvi() and "old" variants will add bloat to an already bloated config API. Even without further changes, we'd need to add *_kvi() to at least config_with_options() and repo_config(), as well as all of the functions that config_with_options() uses under the hood (some of which are public). To some extent, this can be managed by shrinking the config API (e.g. like your suggestion to get rid of git_config_from_file() [1]), but IMO that needs more discussion. - What benefit is the old API giving us vs the new one? It won't be any faster since the machinery will need to support the new API, and even if it were, the benefit would be tiny. In some cases it might be nice to use the config callback in a non-config setting, but this patch shows that those are rare and can be easily worked around. - I strongly suspect that we'd already like "kvi" in more places, and if we made it readily available, we would add these additional callers. E.g. we already use it to give additional diagnostics when failing to parse a number, and there's no reason why we shouldn't do this when doing other kinds of parsing (e.g. date in git_config_expiry_date() or color in color_parse_mem()). If your concern really is primarily about churn and we actually want to move everything to the new API eventually, wouldn't it be better to bite the bullet and take a one time, well-scoped churn cost instead of a longer migration? [1] https://lore.kernel.org/git/230307.86wn3szrzu.gmgdl@evledraar.gmail.com/ > I think this should also neatly answer some of your outstanding > questions. Especially as the above shows that the only non-test caller > that needs "linenr" is the builtin/config.c caller that my proposed RFC > (linked above) tackled directly. Most of these callbacks just need the > more basic "scope". I didn't see where builtin/config.c was handled in the above link. Perhaps you meant a different RFC, https://github.com/avar/git/commit/0233297a359bbda43a902dd0213aacdca82faa34? > So, in particular: > >> Here's an exhaustive list of all of the changes: >> >> * Cases that need a judgement call >> >> - trace2/tr2_cfg.c:tr2_cfg_set_fl() >> >> This function needs to account for tr2_cfg_cb() now using "kvi". >> Since this is only called (indirectly) by git_config_set(), config >> source information has never been available here, so just pass NULL. >> It will be tr2_cfg_cb()'s responsibility to not use "kvi". > > Just adding a "CONFIG_SCOPE_IN_PROCESS", "CONFIG_SCOPE_SET" or whatever > you'd want to call it seems to make much more sense here, no? CONFIG_SCOPE_SET makes sense. >> - builtin/checkout.c:checkout_main() >> >> This calls git_xmerge_config() as a shorthand for parsing a CLI arg. >> "kvi" doesn't apply, so just pass NULL. This might be worth >> refactoring away, since git_xmerge_config() can call >> git_default_config(). > > Another example of a caller which never actually cares about this data, > so if it doesn't need to have it passed to it, it doesn't need to fake > it up either. Here's a case of YAGNI I mentioned above and I agree it would be nice to not have to fake a "kvi". However, git_xmerge_config() can call git_defualt_config() so this seems more like it's abusing the config callback to parse a CLI arg. A better resolution would be to have a function dedicated to parsing "merge.conflictstyle". >> * Hard for cocci to catch >> >> - urlmatch.c >> >> Manually refactor the custom config callbacks in "struct >> urlmatch_config". >> >> - diff.h, fsck.h, grep.h, ident.h, xdiff-interface.h >> >> "struct key_value_info" hasn't been defined yet, so forward declare >> it. Alternatively, maybe these files should "#include config.h". > > All of these problems go away if you don't insist on changing every > single caller, you'll just have a step where you remove the current > global in favor of some "config callback with kvi" info, and "make" will > spot those callers that aren't converted yet. > > Those changes will be trivial enough (just the callers I noted above) to > not require the tricky cocci patch in 08/14. > >> * Likely deficiencies in .cocci patch >> >> - submodule-config.c:gitmodules_cb() >> >> Manually refactor a parse_config() call that gets missed because it >> uses a different "*data" arg. >> >> - grep.h, various >> >> Manually refactor grep_config() calls. Not sure why these don't get >> picked up. >> >> - config.c:git_config_include(), http.c:http_options() >> >> Manually add "kvi" where it was missed. Not sure why they get missed. >> >> - builtin/clone.c:write_one_config() >> >> Manually refactor a git_clone_config() call. Probably got missed >> because I didn't include git_config_parse_parameter(). >> >> - ident.h >> >> Remove the UNUSED attribute. Not sure why this is the only instance >> of this. >> >> - git-compat-util.h, compat/mingw.[h|c] >> >> Manually refactor noop_core_config(), platform_core_config() and >> mingw_core_config(). I can probably add these as "manual fixups" in >> cocci. > > ditto. Yeah, there was definitely more cocci trickery than I'd like. Btw, if you comments on the .cocci file itself, feel free to share. I obviously just hacked together something based on a very rudimentary understanding of cocci and I'd love suggestions on how to improve.
I've covered most your response to Ævar upthread, so I'll omit that. Jonathan Tan <jonathantanmy@google.com> writes: > As for my own opinions, (before Ævar sent this email) I took a look > at these patches myself and had some issues with at least the first > 2: in patch 1, kvi_fn() replaces fn() for some but not all invocations > of fn() (in patch 2, you can see one such invocation that was not > changed), and I was having difficulty thinking of what kind of bugs > I should watch out for since not all invocations were changed; and > in patch 2, the safeguard of not setting kvi and source together was > removed and likewise I was having difficulty thinking of what kind of > bugs could occur from both being set at once inadvertently. I was going > to suggest reordering the patches such that the large-scale refactoring > (and any supporting patches like [PATCH 06/14] config: inline > git_color_default_config) should occur first (or waiting for a reviewer > who is convinced that patches 1 and 2 are OK, I guess), but having now > seen that sidestepping a large part of this makes sense, sidestepping > seems like a good idea to me. In an off-list discussion, we described some plausible ways to organize the refactor that would make it easier for a reviewer to confirm safety. I haven't tried that yet because it sounds like you'd prefer the sidestepping approach. Do you prefer that primarily for safety reasons, or is it largely motivated by other concerns too (e.g. reducing churn or sidestepping produces a better API)? If the primary concern is just safety, I'm somewhat confident that we can find some way to organize this that makes it easier to review and I should just do it.
Glen Choo <chooglen@google.com> writes: > I've covered most your response to Ævar upthread, so I'll omit that. Thanks. Indeed, I missed the situation in which a caller used kvi not by accessing its fields directly but by passing kvi to a function in config.c. > In an off-list discussion, we described some plausible ways to organize > the refactor that would make it easier for a reviewer to confirm safety. > > I haven't tried that yet because it sounds like you'd prefer the > sidestepping approach. Do you prefer that primarily for safety reasons, > or is it largely motivated by other concerns too (e.g. reducing churn or > sidestepping produces a better API)? If the primary concern is just > safety, I'm somewhat confident that we can find some way to organize > this that makes it easier to review and I should just do it. My preference for the sidestepping approach was to reduce churn, but as you have pointed out, it doesn't actually reduce churn. So now I think that the patches should be reordered (but am open to being convinced otherwise, of course). As for safety and better API, I think both approaches (bulk modification of all functions to take the new config_fn_t and two sets of functions each taking a different function type) are equally safe, and it is bulk modification that results in a better API (as you've demonstrated, having kvi information is needed for good error messages, and I expect that to be more and more needed).
diff --git a/builtin/checkout.c b/builtin/checkout.c index 92017ba6696..9641423dc2f 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -1687,7 +1687,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix, if (opts->conflict_style) { opts->merge = 1; /* implied */ - git_xmerge_config("merge.conflictstyle", opts->conflict_style, NULL); + git_xmerge_config("merge.conflictstyle", opts->conflict_style, NULL, NULL); } if (opts->force) { opts->discard_changes = 1; diff --git a/builtin/clone.c b/builtin/clone.c index 1e1cf104194..e654757c45d 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -791,14 +791,14 @@ static int git_clone_config(const char *k, const char *v, } static int write_one_config(const char *key, const char *value, - struct key_value_info *kvi UNUSED, void *data) + struct key_value_info *kvi, void *data) { /* * give git_clone_config a chance to write config values back to the * environment, since git_config_set_multivar_gently only deals with * config-file writes */ - int apply_failed = git_clone_config(key, value, data); + int apply_failed = git_clone_config(key, value, kvi, data); if (apply_failed) return apply_failed; diff --git a/builtin/grep.c b/builtin/grep.c index 177befc3ed4..6e795f9f3a2 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -290,7 +290,7 @@ static int wait_all(void) static int grep_cmd_config(const char *var, const char *value, struct key_value_info *kvi, void *cb) { - int st = grep_config(var, value, cb); + int st = grep_config(var, value, kvi, cb); if (git_color_config(var, value, cb) < 0) st = -1; diff --git a/compat/mingw.c b/compat/mingw.c index 94c5a1daa40..c8181469a2f 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -242,7 +242,8 @@ static int core_restrict_inherited_handles = -1; static enum hide_dotfiles_type hide_dotfiles = HIDE_DOTFILES_DOTGITONLY; static char *unset_environment_variables; -int mingw_core_config(const char *var, const char *value, void *cb) +int mingw_core_config(const char *var, const char *value, + struct key_value_info *kvi UNUSED, void *cb) { if (!strcmp(var, "core.hidedotfiles")) { if (value && !strcasecmp(value, "dotgitonly")) diff --git a/compat/mingw.h b/compat/mingw.h index 209cf7cebad..4f2b489b883 100644 --- a/compat/mingw.h +++ b/compat/mingw.h @@ -11,7 +11,9 @@ typedef _sigset_t sigset_t; #undef _POSIX_THREAD_SAFE_FUNCTIONS #endif -int mingw_core_config(const char *var, const char *value, void *cb); +struct key_value_info; +int mingw_core_config(const char *var, const char *value, + struct key_value_info *, void *cb); #define platform_core_config mingw_core_config /* diff --git a/config.c b/config.c index 60f8c0c666b..aa183f6f244 100644 --- a/config.c +++ b/config.c @@ -147,7 +147,6 @@ struct config_include_data { void *data; const struct config_options *opts; struct git_config_source *config_source; - struct config_reader *config_reader; /* * All remote URLs discovered when reading all config files. @@ -433,10 +432,9 @@ static int include_condition_is_true(struct key_value_info *kvi, static int kvi_fn(config_fn_t fn, const char *key, const char *value, struct key_value_info *kvi, void *data); static int git_config_include(const char *var, const char *value, - struct key_value_info *kvi UNUSED, void *data) + struct key_value_info *kvi, void *data) { struct config_include_data *inc = data; - struct key_value_info *kvi = inc->config_reader->config_kvi; const char *cond, *key; size_t cond_len; int ret; @@ -2252,7 +2250,6 @@ int config_with_options(config_fn_t fn, void *data, inc.data = data; inc.opts = opts; inc.config_source = config_source; - inc.config_reader = &the_reader; fn = git_config_include; data = &inc; } diff --git a/diff.h b/diff.h index 6a3efa63753..2ceb0fd2d66 100644 --- a/diff.h +++ b/diff.h @@ -532,6 +532,7 @@ void free_diffstat_info(struct diffstat_t *diffstat); int parse_long_opt(const char *opt, const char **argv, const char **optarg); +struct key_value_info; int git_diff_basic_config(const char *var, const char *value, struct key_value_info *kvi, void *cb); int git_diff_heuristic_config(const char *var, const char *value, void *cb); diff --git a/fsck.h b/fsck.h index a06f202576c..914e67a067d 100644 --- a/fsck.h +++ b/fsck.h @@ -233,6 +233,7 @@ void fsck_put_object_name(struct fsck_options *options, const char *fsck_describe_object(struct fsck_options *options, const struct object_id *oid); +struct key_value_info; /* * git_config() callback for use by fsck-y tools that want to support * fsck.<msg> fsck.skipList etc. diff --git a/git-compat-util.h b/git-compat-util.h index 4a200a9fb41..6812b592c15 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -440,8 +440,10 @@ typedef uintmax_t timestamp_t; #endif #ifndef platform_core_config +struct key_value_info; static inline int noop_core_config(const char *var UNUSED, const char *value UNUSED, + struct key_value_info *kvi UNUSED, void *cb UNUSED) { return 0; diff --git a/grep.c b/grep.c index 1516b0689d0..2d3b9bf5d92 100644 --- a/grep.c +++ b/grep.c @@ -55,7 +55,7 @@ define_list_config_array_extra(color_grep_slots, {"match"}); * the grep_defaults template. */ int grep_config(const char *var, const char *value, - struct key_value_info *kvi UNUSED, void *cb) + struct key_value_info *kvi, void *cb) { struct grep_opt *opt = cb; const char *slot; @@ -90,9 +90,9 @@ int grep_config(const char *var, const char *value, if (!strcmp(var, "color.grep")) opt->color = git_config_colorbool(var, value); if (!strcmp(var, "color.grep.match")) { - if (grep_config("color.grep.matchcontext", value, cb) < 0) + if (grep_config("color.grep.matchcontext", value, kvi, cb) < 0) return -1; - if (grep_config("color.grep.matchselected", value, cb) < 0) + if (grep_config("color.grep.matchselected", value, kvi, cb) < 0) return -1; } else if (skip_prefix(var, "color.grep.", &slot)) { int i = LOOKUP_CONFIG(color_grep_slots, slot); diff --git a/grep.h b/grep.h index c59592e3bdb..6d2fb0ada54 100644 --- a/grep.h +++ b/grep.h @@ -202,7 +202,9 @@ struct grep_opt { .output = std_output, \ } -int grep_config(const char *var, const char *value, void *); +struct key_value_info; +int grep_config(const char *var, const char *value, struct key_value_info *kvi, + void *); void grep_init(struct grep_opt *, struct repository *repo); void append_grep_pat(struct grep_opt *opt, const char *pat, size_t patlen, const char *origin, int no, enum grep_pat_token t); diff --git a/http.c b/http.c index d5d82c5230f..3d4292eba6a 100644 --- a/http.c +++ b/http.c @@ -361,7 +361,8 @@ static void process_curl_messages(void) } } -static int http_options(const char *var, const char *value, void *cb) +static int http_options(const char *var, const char *value, + struct key_value_info *kvi, void *cb) { if (!strcmp("http.version", var)) { return git_config_string(&curl_http_version, var, value); @@ -532,7 +533,7 @@ static int http_options(const char *var, const char *value, void *cb) } /* Fall back on the default ones */ - return git_default_config(var, value, cb); + return git_default_config(var, value, kvi, cb); } static int curl_empty_auth_enabled(void) diff --git a/ident.h b/ident.h index f55db41ff99..4e3bd347c52 100644 --- a/ident.h +++ b/ident.h @@ -62,7 +62,8 @@ const char *fmt_name(enum want_ident); int committer_ident_sufficiently_given(void); int author_ident_sufficiently_given(void); +struct key_value_info; int git_ident_config(const char *, const char *, - struct key_value_info *UNUSED, void *); + struct key_value_info *, void *); #endif diff --git a/submodule-config.c b/submodule-config.c index 522c9cd3213..7d773f33621 100644 --- a/submodule-config.c +++ b/submodule-config.c @@ -675,7 +675,7 @@ out: } static int gitmodules_cb(const char *var, const char *value, - struct key_value_info *kvi UNUSED, void *data) + struct key_value_info *kvi, void *data) { struct repository *repo = data; struct parse_config_parameter parameter; @@ -685,7 +685,7 @@ static int gitmodules_cb(const char *var, const char *value, parameter.gitmodules_oid = null_oid(); parameter.overwrite = 1; - return parse_config(var, value, ¶meter); + return parse_config(var, value, kvi, ¶meter); } void repo_read_gitmodules(struct repository *repo, int skip_if_read) diff --git a/trace2/tr2_cfg.c b/trace2/tr2_cfg.c index 6871258d468..8ed139c69f4 100644 --- a/trace2/tr2_cfg.c +++ b/trace2/tr2_cfg.c @@ -146,5 +146,5 @@ void tr2_cfg_set_fl(const char *file, int line, const char *key, struct tr2_cfg_data data = { file, line }; if (tr2_cfg_load_patterns() > 0) - tr2_cfg_cb(key, value, &data); + tr2_cfg_cb(key, value, NULL, &data); } diff --git a/urlmatch.c b/urlmatch.c index 47683974d8c..c94500b61b3 100644 --- a/urlmatch.c +++ b/urlmatch.c @@ -552,7 +552,7 @@ static int cmp_matches(const struct urlmatch_item *a, } int urlmatch_config_entry(const char *var, const char *value, - struct key_value_info *kvi UNUSED, void *cb) + struct key_value_info *kvi, void *cb) { struct string_list_item *item; struct urlmatch_config *collect = cb; @@ -566,7 +566,7 @@ int urlmatch_config_entry(const char *var, const char *value, if (!skip_prefix(var, collect->section, &key) || *(key++) != '.') { if (collect->cascade_fn) - return collect->cascade_fn(var, value, cb); + return collect->cascade_fn(var, value, kvi, cb); return 0; /* not interested */ } dot = strrchr(key, '.'); @@ -610,7 +610,7 @@ int urlmatch_config_entry(const char *var, const char *value, strbuf_addstr(&synthkey, collect->section); strbuf_addch(&synthkey, '.'); strbuf_addstr(&synthkey, key); - retval = collect->collect_fn(synthkey.buf, value, collect->cb); + retval = collect->collect_fn(synthkey.buf, value, kvi, collect->cb); strbuf_release(&synthkey); return retval; diff --git a/xdiff-interface.h b/xdiff-interface.h index c1676b11702..749cdf77579 100644 --- a/xdiff-interface.h +++ b/xdiff-interface.h @@ -50,6 +50,8 @@ int buffer_is_binary(const char *ptr, unsigned long size); void xdiff_set_find_func(xdemitconf_t *xecfg, const char *line, int cflags); void xdiff_clear_find_func(xdemitconf_t *xecfg); + +struct key_value_info; int git_xmerge_config(const char *var, const char *value, struct key_value_info *kvi, void *cb); extern int git_xmerge_style;