Re: [PATCH 1/6] config.c: plumb config_source through static fns

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 3, 2023, 18:02 UTC
Message-ID: <xmqqh6v1g1d3.fsf@gitster.g>
In-Reply-To: <ad513d832d8267f9e4805db5e7a9e8915fc62a23.1677631097.git.gitgitgadget@gmail.com>

"Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> writes:

> -static int handle_path_include(const char *path, struct config_include_data *inc)
> +static int handle_path_include(struct config_source *cs, const char *path,
> +			       struct config_include_data *inc)

Because handle_path_include() has no remaining reference to cf after this patch, it may make sense to name the parameter "cf", instead of "cs", taking advantage of the fact that it will cover/hide the global.

> -static int prepare_include_condition_pattern(struct strbuf *pat)
> +static int prepare_include_condition_pattern(struct config_source *cs,
> +					     struct strbuf *pat)

Ditto.

> -static int include_by_gitdir(const struct config_options *opts,
> +static int include_by_gitdir(struct config_source *cs,
> +			     const struct config_options *opts,
>  			     const char *cond, size_t cond_len, int icase)

Ditto.

> +static int include_condition_is_true(struct config_source *cs,
> +				     struct config_include_data *inc,
>  				     const char *cond, size_t cond_len)

Ditto.

> @@ -441,16 +445,16 @@ static int git_config_include(const char *var, const char *value, void *data)

Adding a member to the callback data struct to pass cf around would be the natural next step, I presume. I wonder if that makes the result too big if it is done in this same commit. I suspect that it would be easier to grok the whole picture if it were done in the same commit, though.

If not (IOW, if we deliberately leave some use of the global in the callchains unfixed with this step), it may make the resulting patch much easier to read if you (1) rename the global to a longer name that stands out more, e.g. cf_global, and (2) add a new parameter 'cf' to these helper functions and pass 'cf_global' through to the callchain.

> -static int get_next_char(void)
> +static int get_next_char(struct config_source *cs)

Ditto for "cs" -> "cf".

> -static char *parse_value(void)
> +static char *parse_value(struct config_source *cs)

Ditto.

> -static int get_value(config_fn_t fn, void *data, struct strbuf *name)
> +static int get_value(struct config_source *cs, config_fn_t fn, void *data,
> +		     struct strbuf *name)

Ditto.

> -static int get_extended_base_var(struct strbuf *name, int c)
> +static int get_extended_base_var(struct config_source *cs, struct strbuf *name,
> +				 int c)

Ditto.

> -static int get_base_var(struct strbuf *name)
> +static int get_base_var(struct config_source *cs, struct strbuf *name)

Ditto.

> -static int do_event(enum config_event_t type, struct parse_event_data *data)
> +static int do_event(struct config_source *cs, enum config_event_t type,
> +		    struct parse_event_data *data)

Ditto.

Show 5 quoted lines

> -static int git_parse_source(config_fn_t fn, void *data,
> -			    const struct config_options *opts)
> +static int git_parse_source(struct config_source *cs, config_fn_t fn,
> +			    void *data, const struct config_options *opts)
>  {

Ditto.

> -static void die_bad_number(const char *name, const char *value)
> +static void die_bad_number(struct config_source *cs, const char *name,
> +			   const char *value)

Ditto.

Show 6 quoted lines

> @@ -1304,7 +1312,7 @@ int git_config_int(const char *name, const char *value)
>  {
>  	int ret;
>  	if (!git_parse_int(value, &ret))
> -		die_bad_number(name, value);
> +		die_bad_number(cf, name, value);

And using a more visible name like cf_global will leave us a reminder here what is remaining to be converted, like this place and the callback function driven by config_with_options().