From: Phillip Wood Date: Thu, 01 Jun 2023 09:50:55 GMT Subject: Re: [PATCH v2 03/14] (RFC-only) config: add kvi arg to config_fn_t Message-ID: <6faf1b17-a1ca-0c22-2e43-aee121c4e36a@gmail.com> In-Reply-To: <6834e37066e7877646fc7c37aa79704d14381251.1685472133.git.gitgitgadget@gmail.com> Hi Glen On 30/05/2023 19:42, Glen Choo via GitGitGadget wrote: > From: Glen Choo > > ..without actually changing any of its implementations. This commit does > not build - I've split this out for readability, but post-RFC I will > squash this with the rest of the refactor + cocci changes. While this ends up with a huge amount of churn, I think that is probably inevitable if we're going to get rid of global state (I see Ævar suggested adding a separate set of callbacks but I'm not sure how that would work with chaining up to git_default_config() and it would be nice to improve our error messages with filename and line information). I've not been following this thread all that closely but a couple of thoughts crossed by mind. - is it worth making struct key_value_info opaque and provide getters for the fields so we can change the implementation in the future without having to modify every user. We could rename it config_context or something generic like that if we think it might grow in scope in the future. - (probably impractical) could we stuff the key and value into struct key_value_info so config_fn_t becomes fn(const struct key_value_info, void *data) that would get rid of all the UNUSED annotations but would mean even more churn. The advantage is that one could add functions like kvi_bool_or_int(kvi, &is_bool) and get good error messages because all the config parsing functions would all have access to location information. Best Wishes Phillip > Signed-off-by: Glen Choo > --- > config.c | 8 +- > config.h | 16 +- > .../coccinelle/config_fn_kvi.pending.cocci | 146 ++++++++++++++++++ > 3 files changed, 158 insertions(+), 12 deletions(-) > create mode 100644 contrib/coccinelle/config_fn_kvi.pending.cocci > > diff --git a/config.c b/config.c > index 493f47df8ae..945f4f3b77e 100644 > --- a/config.c > +++ b/config.c > @@ -489,7 +489,7 @@ static int git_config_include(const char *var, const char *value, void *data) > * Pass along all values, including "include" directives; this makes it > * possible to query information on the includes themselves. > */ > - ret = inc->fn(var, value, inc->data); > + ret = inc->fn(var, value, NULL, inc->data); > if (ret < 0) > return ret; > > @@ -671,7 +671,7 @@ static int config_parse_pair(const char *key, const char *value, > if (git_config_parse_key(key, &canonical_name, NULL)) > return -1; > > - ret = (fn(canonical_name, value, data) < 0) ? -1 : 0; > + ret = (fn(canonical_name, value, NULL, data) < 0) ? -1 : 0; > free(canonical_name); > return ret; > } > @@ -959,7 +959,7 @@ static int get_value(struct config_source *cs, config_fn_t fn, void *data, > * accurate line number in error messages. > */ > cs->linenr--; > - ret = fn(name->buf, value, data); > + ret = fn(name->buf, value, NULL, data); > if (ret >= 0) > cs->linenr++; > return ret; > @@ -2303,7 +2303,7 @@ static void configset_iter(struct config_reader *reader, struct config_set *set, > > config_reader_set_kvi(reader, values->items[value_index].util); > > - if (fn(entry->key, values->items[value_index].string, data) < 0) > + if (fn(entry->key, values->items[value_index].string, NULL, data) < 0) > git_die_config_linenr(entry->key, > reader->config_kvi->filename, > reader->config_kvi->linenr); > diff --git a/config.h b/config.h > index 247b572b37b..9d052c52c3c 100644 > --- a/config.h > +++ b/config.h > @@ -111,6 +111,13 @@ struct config_options { > } error_action; > }; > > +struct key_value_info { > + const char *filename; > + int linenr; > + enum config_origin_type origin_type; > + enum config_scope scope; > +}; > + > /** > * A config callback function takes three parameters: > * > @@ -129,7 +136,7 @@ struct config_options { > * A config callback should return 0 for success, or -1 if the variable > * could not be parsed properly. > */ > -typedef int (*config_fn_t)(const char *, const char *, void *); > +typedef int (*config_fn_t)(const char *, const char *, struct key_value_info *, void *); > > int git_default_config(const char *, const char *, void *); > > @@ -667,13 +674,6 @@ int git_config_get_expiry(const char *key, const char **output); > /* parse either "this many days" integer, or "5.days.ago" approxidate */ > int git_config_get_expiry_in_days(const char *key, timestamp_t *, timestamp_t now); > > -struct key_value_info { > - const char *filename; > - int linenr; > - enum config_origin_type origin_type; > - enum config_scope scope; > -}; > - > /** > * First prints the error message specified by the caller in `err` and then > * dies printing the line number and the file name of the highest priority > diff --git a/contrib/coccinelle/config_fn_kvi.pending.cocci b/contrib/coccinelle/config_fn_kvi.pending.cocci > new file mode 100644 > index 00000000000..d4c84599afa > --- /dev/null > +++ b/contrib/coccinelle/config_fn_kvi.pending.cocci > @@ -0,0 +1,146 @@ > +// These are safe to apply to *.c *.h builtin/*.c > + > +@ get_fn @ > +identifier fn, R; > +@@ > +( > +( > +git_config_from_file > +| > +git_config_from_file_with_options > +| > +git_config_from_mem > +| > +git_config_from_blob_oid > +| > +read_early_config > +| > +read_very_early_config > +| > +config_with_options > +| > +git_config > +| > +git_protected_config > +| > +config_from_gitmodules > +) > + (fn, ...) > +| > +repo_config(R, fn, ...) > +) > + > +@ extends get_fn @ > +identifier C1, C2, D; > +@@ > +int fn(const char *C1, const char *C2, > ++ struct key_value_info *kvi, > + void *D); > + > +@ extends get_fn @ > +@@ > +int fn(const char *, const char *, > ++ struct key_value_info *, > + void *); > + > +@ extends get_fn @ > +// Don't change fns that look like callback fns but aren't > +identifier fn2 != tar_filter_config && != git_diff_heuristic_config && > + != git_default_submodule_config && != git_color_config && > + != bundle_list_update && != parse_object_filter_config; > +identifier C1, C2, D1, D2, S; > +attribute name UNUSED; > +@@ > +int fn(const char *C1, const char *C2, > ++ struct key_value_info *kvi, > + void *D1) { > +<+... > +( > +fn2(C1, C2, > ++ kvi, > +D2); > +| > +if(fn2(C1, C2, > ++ kvi, > +D2) < 0) { ... } > +| > +return fn2(C1, C2, > ++ kvi, > +D2); > +| > +S = fn2(C1, C2, > ++ kvi, > +D2); > +) > +...+> > + } > + > +@ extends get_fn@ > +identifier C1, C2, D; > +attribute name UNUSED; > +@@ > +int fn(const char *C1, const char *C2, > ++ struct key_value_info *kvi UNUSED, > + void *D) {...} > + > + > +// The previous rules don't catch all callbacks, especially if they're defined > +// in a separate file from the git_config() call. Fix these manually. > +@@ > +identifier C1, C2, D; > +attribute name UNUSED; > +@@ > +int > +( > +git_ident_config > +| > +urlmatch_collect_fn > +| > +write_one_config > +| > +forbid_remote_url > +| > +credential_config_callback > +) > + (const char *C1, const char *C2, > ++ struct key_value_info *kvi UNUSED, > + void *D) {...} > + > +@@ > +identifier C1, C2, D, D2, S, fn2; > +@@ > +int > +( > +http_options > +| > +git_status_config > +| > +git_commit_config > +| > +git_default_core_config > +| > +grep_config > +) > + (const char *C1, const char *C2, > ++ struct key_value_info *kvi, > + void *D) { > +<+... > +( > +fn2(C1, C2, > ++ kvi, > +D2); > +| > +if(fn2(C1, C2, > ++ kvi, > +D2) < 0) { ... } > +| > +return fn2(C1, C2, > ++ kvi, > +D2); > +| > +S = fn2(C1, C2, > ++ kvi, > +D2); > +) > +...+> > + }