Re: [PATCH v1 3/4] config: factor out global config file retrieval

From: Patrick Steinhardt <ps@pks.im>
Date: Oct 23, 2023, 09:58 UTC
Message-ID: <ZTZDqToqcsDiS5AP@tanuki>
In-Reply-To: <147c767443c35b3b4a5516bf40557f41bb201078.1697660181.git.code@khaugsbakk.name>

On Wed, Oct 18, 2023 at 10:28:40PM +0200, Kristoffer Haugsbakk wrote:

Show 68 quoted lines

> 
> Factor out code that retrieves the global config file so that we can use
> it in `gc.c` as well.
> 
> Use the old name from the previous commit since this function acts
> functionally the same as `git_system_config` but for “global”.
> 
> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
> ---
>  builtin/config.c | 25 ++-----------------------
>  config.c         | 24 ++++++++++++++++++++++++
>  config.h         |  1 +
>  3 files changed, 27 insertions(+), 23 deletions(-)
> 
> diff --git a/builtin/config.c b/builtin/config.c
> index 6fff2655816..df06b766fad 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -708,30 +708,9 @@ int cmd_config(int argc, const char **argv, const char *prefix)
>  	}
>  
>  	if (use_global_config) {
> -		char *user_config, *xdg_config;
> -
> -		git_global_config_paths(&user_config, &xdg_config);
> -		if (!user_config)
> -			/*
> -			 * It is unknown if HOME/.gitconfig exists, so
> -			 * we do not know if we should write to XDG
> -			 * location; error out even if XDG_CONFIG_HOME
> -			 * is set and points at a sane location.
> -			 */
> -			die(_("$HOME not set"));
> -
> +		given_config_source.file = git_global_config();
>  		given_config_source.scope = CONFIG_SCOPE_GLOBAL;
> -
> -		if (access_or_warn(user_config, R_OK, 0) &&
> -		    xdg_config && !access_or_warn(xdg_config, R_OK, 0)) {
> -			given_config_source.file = xdg_config;
> -			free(user_config);
> -		} else {
> -			given_config_source.file = user_config;
> -			free(xdg_config);
> -		}
> -	}
> -	else if (use_system_config) {
> +	} else if (use_system_config) {
>  		given_config_source.file = git_system_config();
>  		given_config_source.scope = CONFIG_SCOPE_SYSTEM;
>  	} else if (use_local_config) {
> diff --git a/config.c b/config.c
> index d2cdda96edd..2ff766c56ff 100644
> --- a/config.c
> +++ b/config.c
> @@ -2111,6 +2111,30 @@ char *git_system_config(void)
>  	return system_config;
>  }
>  
> +char *git_global_config(void)
> +{
> +	char *user_config, *xdg_config;
> +
> +	git_global_config_paths(&user_config, &xdg_config);
> +	if (!user_config)
> +		/*
> +		 * It is unknown if HOME/.gitconfig exists, so
> +		 * we do not know if we should write to XDG

Nit: we don't know about the intent of the caller, so they may not want
to write to the file but only read it.

> +		 * location; error out even if XDG_CONFIG_HOME
> +		 * is set and points at a sane location.
> +		 */
> +		die(_("$HOME not set"));

Is it sensible to `die()` here in this new function that behaves more like a library function? I imagine it would be more sensible to indicate the error to the user and let them handle it accordingly.

Patrick

Show 29 quoted lines

> +
> +	if (access_or_warn(user_config, R_OK, 0) && xdg_config &&
> +	    !access_or_warn(xdg_config, R_OK, 0)) {
> +		free(user_config);
> +		return xdg_config;
> +	} else {
> +		free(xdg_config);
> +		return user_config;
> +	}
> +}
> +
>  void git_global_config_paths(char **user_out, char **xdg_out)
>  {
>  	char *user_config = xstrdup_or_null(getenv("GIT_CONFIG_GLOBAL"));
> diff --git a/config.h b/config.h
> index 9f04de8ee3e..5cf961b548d 100644
> --- a/config.h
> +++ b/config.h
> @@ -394,6 +394,7 @@ int config_error_nonbool(const char *);
>  #endif
>  
>  char *git_system_config(void);
> +char *git_global_config(void);
>  void git_global_config_paths(char **user, char **xdg);
>  
>  int git_config_parse_parameter(const char *, config_fn_t fn, void *data);
> -- 
> 2.42.0.2.g879ad04204
>