Subject: Re: [WIP v2 1/2] config: make git_config_include() static

From: Emily Shaffer <[email-masked]>
Date: Fri, 5 Nov 2021 12:45:43 -0700

On Fri, Oct 29, 2021 at 10:31:09AM -0700, Jonathan Tan wrote:
> 
> It is not used from outside the file in which it is declared.
> 
> Signed-off-by: Jonathan Tan <[email-masked]>
> ---
>  config.c | 12 +++++++++++-
>  config.h | 37 ++++---------------------------------
>  2 files changed, 15 insertions(+), 34 deletions(-)
> 
> diff --git a/config.c b/config.c
> index 2dcbe901b6..94ad5ce913 100644
> --- a/config.c
> +++ b/config.c
> @@ -120,6 +120,16 @@ static long config_buf_ftell(struct config_source *conf)
>  	return conf->u.buf.pos;
>  }
>  
> +struct config_include_data {
> +	int depth;
> +	config_fn_t fn;
> +	void *data;
> +	const struct config_options *opts;
> +};
> +#define CONFIG_INCLUDE_INIT { 0 }
> +
> +static int git_config_include(const char *var, const char *value, void *data);
> +
>  #define MAX_INCLUDE_DEPTH 10
>  static const char include_depth_advice[] = N_(
>  "exceeded maximum include depth (%d) while including\n"
> @@ -306,7 +316,7 @@ static int include_condition_is_true(const struct config_options *opts,
>  	return 0;
>  }
>  
> -int git_config_include(const char *var, const char *value, void *data)
> +static int git_config_include(const char *var, const char *value, void *data)
>  {
>  	struct config_include_data *inc = data;
>  	const char *cond, *key;
> diff --git a/config.h b/config.h
> index f119de0130..48a5e472ca 100644
> --- a/config.h
> +++ b/config.h
> @@ -126,6 +126,8 @@ int git_default_config(const char *, const char *, void *);
>  /**
>   * Read a specific file in git-config format.
>   * This function takes the same callback and data parameters as `git_config`.
> + *
> + * Unlike git_config(), this function does not respect includes.
>   */
>  int git_config_from_file(config_fn_t fn, const char *, void *);
>  
> @@ -158,6 +160,8 @@ void read_very_early_config(config_fn_t cb, void *data);
>   * will first feed the user-wide one to the callback, and then the
>   * repo-specific one; by overwriting, the higher-priority repo-specific
>   * value is left at the end).
> + *
> + * Unlike git_config_from_file(), this function respects includes.
>   */
>  void git_config(config_fn_t fn, void *);
>  
> @@ -338,39 +342,6 @@ const char *current_config_origin_type(void);
>  const char *current_config_name(void);
>  int current_config_line(void);
>  
> -/**
> - * Include Directives
> - * ------------------
> - *
> - * By default, the config parser does not respect include directives.
> - * However, a caller can use the special `git_config_include` wrapper
> - * callback to support them. To do so, you simply wrap your "real" callback
> - * function and data pointer in a `struct config_include_data`, and pass
> - * the wrapper to the regular config-reading functions. For example:
> - *
> - * -------------------------------------------
> - * int read_file_with_include(const char *file, config_fn_t fn, void *data)
> - * {
> - * struct config_include_data inc = CONFIG_INCLUDE_INIT;
> - * inc.fn = fn;
> - * inc.data = data;
> - * return git_config_from_file(git_config_include, file, &inc);
> - * }
> - * -------------------------------------------
> - *
> - * `git_config` respects includes automatically. The lower-level
> - * `git_config_from_file` does not.
> - *
> - */

It is a shame to lose the comprehensive usage documentation. Can we move
it into the source near the static definition instead?

> -struct config_include_data {
> -	int depth;
> -	config_fn_t fn;
> -	void *data;
> -	const struct config_options *opts;
> -};
> -#define CONFIG_INCLUDE_INIT { 0 }
> -int git_config_include(const char *name, const char *value, void *data);

I wondered why we even had this here, if we were only calling
'git_config_include()' from config.c. The last time this definition was
touched was when it was moved out of cache.h (e67a57fc518) and the time
before that was when it was introduced in 2012 (9b25a0b52e0). At the
time of its introduction it was only called in config.c, anyways. So I
guess it is just a matter of history.

It's still a WIP so I won't leave a reviewed-by line, but this patch
looks fine.

 - Emily

> -
>  /*
>   * Match and parse a config key of the form:
>   *
> -- 
> 2.33.1.1089.g2158813163f-goog
>