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

From: Junio C Hamano <gitster@pobox.com>
Date: May 30, 2012, 21:54 UTC
Message-ID: <7vr4u1xrkp.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1338412775-22840-1-git-send-email-Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr>

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

Show 5 quoted lines

> 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.

Show 16 quoted lines

> @@ -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.

Show 22 quoted lines

> 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?

Show 27 quoted lines

> @@ -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?

Show 21 quoted lines

> 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.