From: Phillip Wood <phillip.wood123@gmail.com>
To: Glen Choo via GitGitGadget <gitgitgadget@gmail.com>, git@vger.kernel.org
Cc: "Jonathan Tan" <jonathantanmy@google.com>,
"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
"Emily Shaffer" <nasamuffin@google.com>,
"Glen Choo" <chooglen@google.com>
Subject: Re: [PATCH v2 03/14] (RFC-only) config: add kvi arg to config_fn_t
Date: Thu, 1 Jun 2023 10:50:55 +0100 [thread overview]
Message-ID: <6faf1b17-a1ca-0c22-2e43-aee121c4e36a@gmail.com> (raw)
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 <chooglen@google.com>
>
> ..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 <chooglen@google.com>
> ---
> 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);
> +)
> +...+>
> + }
next prev parent reply other threads:[~2023-06-01 9:51 UTC|newest]
Thread overview: 115+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-04-21 19:13 [PATCH 00/14] [RFC] config: remove global state from config iteration Glen Choo via GitGitGadget
2023-04-21 19:13 ` [PATCH 01/14] config.c: introduce kvi_fn(), use it for configsets Glen Choo via GitGitGadget
2023-04-21 19:13 ` [PATCH 02/14] config.c: use kvi for CLI config Glen Choo via GitGitGadget
2023-05-01 11:06 ` Ævar Arnfjörð Bjarmason
2023-04-21 19:13 ` [PATCH 03/14] config: use kvi for config files Glen Choo via GitGitGadget
2023-04-21 19:13 ` [PATCH 04/14] config: add kvi.path, use it to evaluate includes Glen Choo via GitGitGadget
2023-04-21 19:13 ` [PATCH 05/14] config: pass source to config_parser_event_fn_t Glen Choo via GitGitGadget
2023-04-21 19:13 ` [PATCH 06/14] config: inline git_color_default_config Glen Choo via GitGitGadget
2023-04-21 19:13 ` [PATCH 07/14] urlmatch.h: use config_fn_t type Glen Choo via GitGitGadget
2023-04-21 19:13 ` [PATCH 08/14] (RFC-only) config: add kvi arg to config_fn_t Glen Choo via GitGitGadget
2023-04-21 19:13 ` [PATCH 09/14] (RFC-only) config: apply cocci to config_fn_t implementations Glen Choo via GitGitGadget
2023-04-21 19:13 ` [PATCH 10/14] (RFC-only) config: finish config_fn_t refactor Glen Choo via GitGitGadget
2023-05-01 11:19 ` Ævar Arnfjörð Bjarmason
2023-05-05 21:07 ` Jonathan Tan
2023-05-09 22:46 ` Glen Choo
2023-05-11 16:21 ` Jonathan Tan
2023-05-08 21:00 ` Glen Choo
2023-04-21 19:13 ` [PATCH 11/14] config: remove current_config_(line|name|origin_type) Glen Choo via GitGitGadget
2023-04-21 19:13 ` [PATCH 12/14] config: remove current_config_scope() Glen Choo via GitGitGadget
2023-04-21 19:13 ` [PATCH 13/14] config: pass kvi to die_bad_number() Glen Choo via GitGitGadget
2023-04-21 19:13 ` [PATCH 14/14] config: remove config_reader from configset_add_value Glen Choo via GitGitGadget
2023-05-30 18:41 ` [PATCH v2 00/14] [RFC] config: remove global state from config iteration Glen Choo via GitGitGadget
2023-05-30 18:41 ` [PATCH v2 01/14] config: inline git_color_default_config Glen Choo via GitGitGadget
2023-05-30 18:42 ` [PATCH v2 02/14] urlmatch.h: use config_fn_t type Glen Choo via GitGitGadget
2023-05-30 18:42 ` [PATCH v2 03/14] (RFC-only) config: add kvi arg to config_fn_t Glen Choo via GitGitGadget
2023-06-01 9:50 ` Phillip Wood [this message]
2023-06-01 16:22 ` Glen Choo
2023-06-02 9:54 ` Phillip Wood
2023-06-02 16:46 ` Glen Choo
2023-06-05 9:38 ` Phillip Wood
2023-06-09 23:19 ` Glen Choo
2023-05-30 18:42 ` [PATCH v2 04/14] (RFC-only) config: apply cocci to config_fn_t implementations Glen Choo via GitGitGadget
2023-05-30 18:42 ` [PATCH v2 05/14] (RFC-only) config: finish config_fn_t refactor Glen Choo via GitGitGadget
2023-06-01 22:17 ` Jonathan Tan
2023-05-30 18:42 ` [PATCH v2 06/14] config.c: pass kvi in configsets Glen Choo via GitGitGadget
2023-06-01 22:21 ` Jonathan Tan
2023-05-30 18:42 ` [PATCH v2 07/14] config: provide kvi with config files Glen Choo via GitGitGadget
2023-06-01 22:41 ` Jonathan Tan
2023-06-01 23:54 ` Jonathan Tan
2023-05-30 18:42 ` [PATCH v2 08/14] builtin/config.c: test misuse of format_config() Glen Choo via GitGitGadget
2023-05-30 18:42 ` [PATCH v2 09/14] config.c: provide kvi with CLI config Glen Choo via GitGitGadget
2023-06-01 23:35 ` Jonathan Tan
2023-06-02 17:26 ` Glen Choo
2023-05-30 18:42 ` [PATCH v2 10/14] trace2: plumb config kvi Glen Choo via GitGitGadget
2023-06-01 23:38 ` Jonathan Tan
2023-05-30 18:42 ` [PATCH v2 11/14] config: pass kvi to die_bad_number() Glen Choo via GitGitGadget
2023-06-01 23:48 ` Jonathan Tan
2023-06-02 17:23 ` Glen Choo
2023-05-30 18:42 ` [PATCH v2 12/14] config.c: remove config_reader from configsets Glen Choo via GitGitGadget
2023-05-30 18:42 ` [PATCH v2 13/14] config: add kvi.path, use it to evaluate includes Glen Choo via GitGitGadget
2023-06-02 0:06 ` Jonathan Tan
2023-05-30 18:42 ` [PATCH v2 14/14] config: pass source to config_parser_event_fn_t Glen Choo via GitGitGadget
2023-06-02 0:08 ` Jonathan Tan
2023-06-02 17:20 ` Glen Choo
2023-06-20 19:43 ` [PATCH v3 00/12] config: remove global state from config iteration Glen Choo via GitGitGadget
2023-06-20 19:43 ` [PATCH v3 01/12] config: inline git_color_default_config Glen Choo via GitGitGadget
2023-06-20 21:01 ` Junio C Hamano
2023-06-20 19:43 ` [PATCH v3 02/12] urlmatch.h: use config_fn_t type Glen Choo via GitGitGadget
2023-06-20 21:02 ` Junio C Hamano
2023-06-20 19:43 ` [PATCH v3 03/12] config: add ctx arg to config_fn_t Glen Choo via GitGitGadget
2023-06-20 19:43 ` [PATCH v3 04/12] config.c: pass ctx in configsets Glen Choo via GitGitGadget
2023-06-20 21:19 ` Junio C Hamano
2023-06-20 19:43 ` [PATCH v3 05/12] config: pass ctx with config files Glen Choo via GitGitGadget
2023-06-20 19:43 ` [PATCH v3 06/12] builtin/config.c: test misuse of format_config() Glen Choo via GitGitGadget
2023-06-20 21:35 ` Junio C Hamano
2023-06-20 23:06 ` Glen Choo
2023-06-23 20:32 ` Jonathan Tan
2023-06-24 1:31 ` Jeff King
2023-06-28 17:28 ` Glen Choo
2023-06-20 19:43 ` [PATCH v3 07/12] config.c: pass ctx with CLI config Glen Choo via GitGitGadget
2023-06-23 20:35 ` Jonathan Tan
2023-06-23 21:41 ` Glen Choo
2023-06-20 19:43 ` [PATCH v3 08/12] trace2: plumb config kvi Glen Choo via GitGitGadget
2023-06-23 20:40 ` Jonathan Tan
2023-06-20 19:43 ` [PATCH v3 09/12] config: pass kvi to die_bad_number() Glen Choo via GitGitGadget
2023-06-20 19:43 ` [PATCH v3 10/12] config.c: remove config_reader from configsets Glen Choo via GitGitGadget
2023-06-23 20:57 ` Jonathan Tan
2023-06-23 21:33 ` Junio C Hamano
2023-06-20 19:43 ` [PATCH v3 11/12] config: add kvi.path, use it to evaluate includes Glen Choo via GitGitGadget
2023-06-20 19:43 ` [PATCH v3 12/12] config: pass source to config_parser_event_fn_t Glen Choo via GitGitGadget
2023-06-20 21:46 ` Junio C Hamano
2023-06-21 21:46 ` [PATCH v3 00/12] config: remove global state from config iteration Junio C Hamano
2023-06-21 23:06 ` Glen Choo
2023-06-23 21:02 ` Jonathan Tan
2023-06-23 21:33 ` Junio C Hamano
2023-06-23 21:45 ` Glen Choo
2023-06-26 18:11 ` [PATCH v4 " Glen Choo via GitGitGadget
2023-06-26 18:11 ` [PATCH v4 01/12] config: inline git_color_default_config Glen Choo via GitGitGadget
2023-06-26 18:11 ` [PATCH v4 02/12] urlmatch.h: use config_fn_t type Glen Choo via GitGitGadget
2023-06-26 18:11 ` [PATCH v4 03/12] config: add ctx arg to config_fn_t Glen Choo via GitGitGadget
2023-06-26 18:11 ` [PATCH v4 04/12] config.c: pass ctx in configsets Glen Choo via GitGitGadget
2023-06-26 18:11 ` [PATCH v4 05/12] config: pass ctx with config files Glen Choo via GitGitGadget
2023-06-26 18:11 ` [PATCH v4 06/12] builtin/config.c: test misuse of format_config() Glen Choo via GitGitGadget
2023-06-26 18:11 ` [PATCH v4 07/12] config.c: pass ctx with CLI config Glen Choo via GitGitGadget
2023-06-26 18:11 ` [PATCH v4 08/12] trace2: plumb config kvi Glen Choo via GitGitGadget
2023-06-26 18:11 ` [PATCH v4 09/12] config: pass kvi to die_bad_number() Glen Choo via GitGitGadget
2023-06-26 18:11 ` [PATCH v4 10/12] config.c: remove config_reader from configsets Glen Choo via GitGitGadget
2023-06-26 18:11 ` [PATCH v4 11/12] config: add kvi.path, use it to evaluate includes Glen Choo via GitGitGadget
2023-06-26 18:11 ` [PATCH v4 12/12] config: pass source to config_parser_event_fn_t Glen Choo via GitGitGadget
2023-06-26 20:45 ` [PATCH v4 00/12] config: remove global state from config iteration Junio C Hamano
2023-06-28 19:26 ` [PATCH v5 00/11] " Glen Choo via GitGitGadget
2023-06-28 19:26 ` [PATCH v5 01/11] config: inline git_color_default_config Glen Choo via GitGitGadget
2023-06-28 19:26 ` [PATCH v5 02/11] urlmatch.h: use config_fn_t type Glen Choo via GitGitGadget
2023-06-28 19:26 ` [PATCH v5 03/11] config: add ctx arg to config_fn_t Glen Choo via GitGitGadget
2023-06-28 19:26 ` [PATCH v5 04/11] config.c: pass ctx in configsets Glen Choo via GitGitGadget
2023-06-28 19:26 ` [PATCH v5 05/11] config: pass ctx with config files Glen Choo via GitGitGadget
2023-06-28 19:26 ` [PATCH v5 06/11] config.c: pass ctx with CLI config Glen Choo via GitGitGadget
2023-06-28 19:26 ` [PATCH v5 07/11] trace2: plumb config kvi Glen Choo via GitGitGadget
2023-06-28 19:26 ` [PATCH v5 08/11] config: pass kvi to die_bad_number() Glen Choo via GitGitGadget
2023-06-28 19:26 ` [PATCH v5 09/11] config.c: remove config_reader from configsets Glen Choo via GitGitGadget
2023-06-28 19:26 ` [PATCH v5 10/11] config: add kvi.path, use it to evaluate includes Glen Choo via GitGitGadget
2023-06-28 19:26 ` [PATCH v5 11/11] config: pass source to config_parser_event_fn_t Glen Choo via GitGitGadget
2023-06-28 22:23 ` [PATCH v5 00/11] config: remove global state from config iteration Jonathan Tan
2023-06-28 22:47 ` Junio C Hamano
2023-07-11 18:41 ` Phillip Wood
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6faf1b17-a1ca-0c22-2e43-aee121c4e36a@gmail.com \
--to=phillip.wood123@gmail.com \
--cc=avarab@gmail.com \
--cc=chooglen@google.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=jonathantanmy@google.com \
--cc=nasamuffin@google.com \
--cc=phillip.wood@dunelm.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).