Re: [PATCHv2] Possibility to read both from ~/.gitconfig and from $XDG_CONFIG_HOME/git/config

[Junio C Hamano <gitster@pobox.com>]

Huynh Khoi Nguyen NGUYEN  <Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr>
writes:

> From: NGUYEN Huynh Khoi Nguyen <nguyenhu@ensibm.imag.fr>
>
> Git will read both in $XDG_CONFIG_HOME/git/config and in ~/.gitconfig in this order:
> .git/config > ~/.gitconfig > $XDG_CONFIG_HOME/git/config > /etc/gitconfig
> If $XDG_CONFIG_HOME is either not set or empty, $HOME/.config/git/config will be used.

Is it just me who finds the above three lines extremely unreadable?

Also can you give this patch a bit more sensible title?
"Possibility to" does not tell us much---anything is possible if you
change code after all.

I see the patch does not touch the writing codepath, which is
probably a good thing, but the log message should explicitly state
that.

> @@ -194,7 +194,7 @@ See also <<FILES>>.
>  FILES
>  -----
>  
> -If not set explicitly with '--file', there are three files where
> +If not set explicitly with '--file', there are four files where
>  'git config' will search for configuration options:
>  
>  $GIT_DIR/config::
> @@ -204,6 +204,9 @@ $GIT_DIR/config::
>  	User-specific configuration file. Also called "global"
>  	configuration file.
>  
> +$XDG_CONFIG_HOME/git/config::
> +	Second user-specific configuration file. ~/.gitconfig has priority.
> +

I am not sure in what way $HOME/.gitconfig has "priority".

Your proposed log message says that You read from $HOME/.gitconfig
and then from $XDG_CONFIG_HOME/git/config, which means that any
single-valued variable set in $HOME/.gitconfig will be overwritten
by whatever is in $XDG_CONFIG_HOME/git/config, no?  That sounds like
you are giving priority to the latter to me.

And for multi-valued variables, settings from both files are read,
so there isn't much inherent priority between the two, except for
variables for which the definition order matters, of course.

If you read only from $HOME/.gitconfig if exists, and read from
$XDG_CONFIG_HOME/git/config only when $HOME/.gitconfig does not,
then you are giving $HOME/.gitconfig a priority, but that is not
what the patch is doing as far as I can tell.

> diff --git a/builtin/config.c b/builtin/config.c
> index 33c8820..38dba4f 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -161,7 +161,7 @@ static int show_config(const char *key_, const char *value_, void *cb)
>  static int get_value(const char *key_, const char *regex_)
>  {
>  	int ret = -1;
> -	char *global = NULL, *repo_config = NULL;
> +	char *gitconfig_global = NULL, *xdg_global = NULL, *repo_config = NULL;
>  	const char *system_wide = NULL, *local;
>  	struct config_include_data inc = CONFIG_INCLUDE_INIT;
>  	config_fn_t fn;
> @@ -171,8 +171,15 @@ static int get_value(const char *key_, const char *regex_)
>  	if (!local) {
>  		const char *home = getenv("HOME");
>  		local = repo_config = git_pathdup("config");
> -		if (home)
> -			global = xstrdup(mkpath("%s/.gitconfig", home));
> +		if (home) {
> +			const char *xdg_config_home = getenv("XDG_CONFIG_HOME");
> +			if (xdg_config_home)

This is logically wrong; even when you fail to read $HOME, you may
be able to read $XDG_CONFIG_HOME, no?  It shouldn't be nested inside
"if (home)" at all, methinks.

It would be more like

	global = xdg_global = NULL;
        if (HOME exists?)
        	global = $HOME/.gitconfig
	if (XDG_CONFIG_HOME exists?)
        	xdg_global = $XDG_CONFIG_HOME/git/config
	else if (HOME exists?)
        	xdg_global = $HOME/.config/git/config

no?

> @@ -381,7 +393,25 @@ int cmd_config(int argc, const char **argv, const char *prefix)
>  	if (use_global_config) {
>  		char *home = getenv("HOME");
>  		if (home) {
> -			char *user_config = xstrdup(mkpath("%s/.gitconfig", home));
> +			char *user_config;
> +			const char *gitconfig_path = mkpath("%s/.gitconfig", home);
> +			const char *xdg_config_path = NULL;
> +			const char *xdg_config_home = NULL;
> +
> +			xdg_config_home = getenv("XDG_CONFIG_HOME");
> +			if (xdg_config_home)
> +				xdg_config_path = mkpath("%s/git/config", xdg_config_home);
> +			else
> +				xdg_config_path = mkpath("%s/.config/git/config", home);
> +
> +			if (access(gitconfig_path, R_OK) && !access(xdg_config_path, R_OK) &&
> +			    (actions == ACTION_LIST ||
> +			     actions == ACTION_GET_COLOR ||
> +			     actions == ACTION_GET_COLORBOOL))
> +				user_config = xstrdup(xdg_config_path);
> +			else
> +				user_config = xstrdup(gitconfig_path);
> +
>  			given_config_file = user_config;
>  		} else {
>  			die("$HOME not set");

Exactly the same comment applies here.

You seem to always write to $HOME/.gitconfig, so missing $HOME may
be an error if the action is to store, but if you are reading and if
$XDG_CONFIG_HOME is set, you do not have to have $HOME set, no?
Even when there is $HOME, if there is no $HOME/.gitconfig file, you
wouldn't want to give an error, so missing $HOME environment should
be treated pretty much the same way as missing $HOME/.gitconfig file
for the purpose of reading, no?

> diff --git a/config.c b/config.c
> index 71ef171..53557dc 100644
> --- a/config.c
> +++ b/config.c
> @@ -939,10 +939,23 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
>  
>  	home = getenv("HOME");
>  	if (home) {
> -		char buf[PATH_MAX];
> -		char *user_config = mksnpath(buf, sizeof(buf), "%s/.gitconfig", home);
> -		if (!access(user_config, R_OK)) {
> -			ret += git_config_from_file(fn, user_config, data);
> +		const char *gitconfig_path = xstrdup(mkpath("%s/.gitconfig", home));
> +		const char *xdg_config_path = NULL;
> +		const char *xdg_config_home = NULL;
> +
> +		xdg_config_home = getenv("XDG_CONFIG_HOME");
> +		if (xdg_config_home)
> +			xdg_config_path = xstrdup(mkpath("%s/git/config", xdg_config_home));
> +		else
> +			xdg_config_path = xstrdup(mkpath("%s/.config/git/config", home));

Exactly the same comment applies here, too.

The original that read from $HOME/.gitconfig was simple enough so
having three copies of getenv("HOME") was perfectly fine, but as you
are introduce this much complexity to to decide which two files to
read from, the code added this patch needs to be refactored and
three copies of the same logic need to be consolidated, I would have
to say.