Re: [PATCH v2 03/14] (RFC-only) config: add kvi arg to config_fn_t

[Phillip Wood <phillip.wood123@xxxxxxxxx>]

Hi Glen

On 30/05/2023 19:42, Glen Choo via GitGitGadget wrote:
> From: Glen Choo <chooglen@xxxxxxxxxx>
>
> ..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@xxxxxxxxxx>
> ---
>   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);
> +)
> +...+>
> +  }