Subject: Re: [PATCH v3 1/4] cache.h: rename "xdg_config_home" to "xdg_config_home_git"

From: Đoàn Trần Công Danh <[email-masked]>
Date: Fri, 21 May 2021 06:44:24 +0700

On 2021-05-21 00:13:56+0200, Lénaïc Huard <[email-masked]> wrote:
> Current implementation of `xdg_config_home(str)` returns
> `$XDG_CONFIG_HOME/git/$str`, with the `git` subdirectory inserted
> between the `XDG_CONFIG_HOME` environment variable and the parameter.
> 
> This patch re-purposes `xdg_config_home(…)` to be more generic. It now
> only concatenates "$XDG_CONFIG_HOME", or "$HOME/.config" if the former
> isn’t defined, with the parameter, without adding `git` in between.
> Its parameter is now a format string.

Intended or not, this change is going to make a logical conflict,
should other topics also call to xdg_config_home, i.e. no textual
conflicts, programs compiled successfully but run into failure.

I think we shouldn't re-purpose xdg_config_home, we should add a new
function named something like
xdg_config_home_{for,nongit,other,generic} instead.

> -char *xdg_config_home(const char *filename)
> +char *xdg_config_home(const char *fmt, ...)

In my opinion, we don't even need to over-engineer this function
with variadic arguments, I think below function should be enough for
most (all?) cases:

	char *xdg_config_home_prog(const char *prog, const char *filename)

>  {
>  	const char *home, *config_home;
> +	struct strbuf buf = STRBUF_INIT;
> +	char *out = NULL;
> +	va_list args;
> +
> +	va_start(args, fmt);
> +	strbuf_vaddf(&buf, fmt, args);
> +	va_end(args);

If my imagination is sensible, it's not necessary to use a temporary
strbuf and strbuf_vaddf here ...

>  
> -	assert(filename);
>  	config_home = getenv("XDG_CONFIG_HOME");
> -	if (config_home && *config_home)
> -		return mkpathdup("%s/git/%s", config_home, filename);
> +	if (config_home && *config_home) {
> +		out = mkpathdup("%s/%s", config_home, buf.buf);
> +		goto done;
> +	}
>  
>  	home = getenv("HOME");
> -	if (home)
> -		return mkpathdup("%s/.config/git/%s", home, filename);
> -	return NULL;
> +	if (home) {
> +		out = mkpathdup("%s/.config/%s", home, buf.buf);
> +		goto done;
> +	}
> +
> +done:
> +	strbuf_release(&buf);

... and go though the restructure of this function.

-- 
Danh