threads / discuss / 30663

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

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

## tl;dr

88 messages between May 30, 2012 and Jul 16, 2012.

replies: 87people: 10as markdown or json

Huynh Khoi Nguyen NGUYEN· May 30, 2012, 21:19 UTC · lore
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.

Signed-off-by: Huynh Khoi Nguyen NGUYEN <Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr>
Signed-off-by: Lucien KONG <Lucien.Kong@ensimag.imag.fr>
Signed-off-by: Valentin DUPERRAY <Valentin.Duperray@ensimag.imag.fr>
Signed-off-by: Thomas NGUY <Thomas.Nguy@ensimag.imag.fr>
Signed-off-by: Franck JONAS <Franck.Jonas@ensimag.imag.fr>
---
 Documentation/git-config.txt    |    9 +++-
 builtin/config.c                |   48 +++++++++++++++++----
 config.c                        |   21 ++++++++--
 t/t1306-read-xdg-config-file.sh |   87 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 149 insertions(+), 16 deletions(-)
 create mode 100755 t/t1306-read-xdg-config-file.sh
diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index d9463cb..30418fa 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -99,8 +99,8 @@ OPTIONS
 	For writing options: write to global ~/.gitconfig file rather than
 	the repository .git/config.
 +
-For reading options: read only from global ~/.gitconfig rather than
-from all available files.
+For reading options: read only from global ~/.gitconfig then from
+$XDG_CONFIG_HOME/git/config rather than from all available files.
 +
 See also <<FILES>>.
 
@@ -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.
+
 $(prefix)/etc/gitconfig::
 	System-wide configuration file.
 
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)
+				xdg_global = xstrdup(mkpath("%s/git/config", xdg_config_home));
+			else
+				xdg_global = xstrdup(mkpath("%s/.config/git/config", home));
+
+			gitconfig_global = xstrdup(mkpath("%s/.gitconfig", home));
+		}
 		if (git_config_system())
 			system_wide = git_etc_gitconfig();
 	}
@@ -229,15 +236,19 @@ static int get_value(const char *key_, const char *regex_)
 
 	if (do_all && system_wide)
 		git_config_from_file(fn, system_wide, data);
-	if (do_all && global)
-		git_config_from_file(fn, global, data);
+	if (do_all && xdg_global)
+		git_config_from_file(fn, xdg_global, data);
+	if (do_all && gitconfig_global)
+		git_config_from_file(fn, gitconfig_global, data);
 	if (do_all)
 		git_config_from_file(fn, local, data);
 	git_config_from_parameters(fn, data);
 	if (!do_all && !seen)
 		git_config_from_file(fn, local, data);
-	if (!do_all && !seen && global)
-		git_config_from_file(fn, global, data);
+	if (!do_all && !seen && gitconfig_global)
+		git_config_from_file(fn, gitconfig_global, data);
+	if (!do_all && !seen && xdg_global)
+		git_config_from_file(fn, xdg_global, data);
 	if (!do_all && !seen && system_wide)
 		git_config_from_file(fn, system_wide, data);
 
@@ -254,7 +265,8 @@ static int get_value(const char *key_, const char *regex_)
 
 free_strings:
 	free(repo_config);
-	free(global);
+	free(gitconfig_global);
+	free(xdg_global);
 	return ret;
 }
 
@@ -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");
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));
+
+		if (!access(xdg_config_path, R_OK)) {
+			ret += git_config_from_file(fn, xdg_config_path, data);
+			found += 1;
+		}
+
+		if (!access(gitconfig_path, R_OK)) {
+			ret += git_config_from_file(fn, gitconfig_path, data);
 			found += 1;
 		}
 	}
diff --git a/t/t1306-read-xdg-config-file.sh b/t/t1306-read-xdg-config-file.sh
new file mode 100755
index 0000000..4cab38b
--- /dev/null
+++ b/t/t1306-read-xdg-config-file.sh
@@ -0,0 +1,87 @@
+#!/bin/sh
+#
+# Copyright (c) 2012 Valentin Duperray, Lucien Kong, Franck Jonas,
+#		     Thomas Nguy, Khoi Nguyen
+#		     Grenoble INP Ensimag
+#
+
+test_description='possibility to read $XDG_CONFIG_HOME/git/config file'
+
+. ./test-lib.sh
+
+test_expect_success 'read automatically: xdg file exists and ~/.gitconfig doesn'\''t' '
+	mkdir -p .config/git &&
+	echo "[advice]" >.config/git/config &&
+	echo "	statushints = false" >>.config/git/config &&
+	cat >expect <<-\EOF &&
+	# On branch master
+	#
+	# Initial commit
+	#
+	# Untracked files:
+	#	.config/
+	#	expect
+	#	output
+	nothing added to commit but untracked files present
+	EOF
+	git status >output &&
+	test_cmp expect output
+'
+
+test_expect_success 'read automatically: xdg file exists and ~/.gitconfig exists' '
+	>.gitconfig &&
+	echo "[advice]" >.gitconfig &&
+	echo "	statushints = true" >>.gitconfig &&
+	cat >expect <<-\EOF &&
+	# On branch master
+	#
+	# Initial commit
+	#
+	# Untracked files:
+	#   (use "git add <file>..." to include in what will be committed)
+	#
+	#	.config/
+	#	.gitconfig
+	#	expect
+	#	output
+	nothing added to commit but untracked files present (use "git add" to track)
+	EOF
+	git status >output &&
+	test_cmp expect output
+'
+
+test_expect_success 'read with --get: xdg file exists and ~/.gitconfig doesn'\''t' '
+	rm .gitconfig &&
+	echo "[user]" >.config/git/config &&
+	echo "	name = read_config" >>.config/git/config &&
+	echo read_config >expect &&
+	git config --get user.name >output &&
+	test_cmp expect output
+'
+
+test_expect_success 'read with --get: xdg file exists and ~/.gitconfig exists' '
+	>.gitconfig &&
+	echo "[user]" >.gitconfig &&
+	echo "	name = read_gitconfig" >>.gitconfig &&
+	echo read_gitconfig >expect &&
+	git config --get user.name >output &&
+	test_cmp expect output
+'
+
+test_expect_success 'read with --list: xdg file exists and ~/.gitconfig doesn'\''t' '
+	rm .gitconfig &&
+	echo user.name=read_config >expect &&
+	git config --global --list >output &&
+	test_cmp expect output
+'
+
+test_expect_success 'read with --list: xdg file exists and ~/.gitconfig exists' '
+	>.gitconfig &&
+	echo "[user]" >.gitconfig &&
+	echo "	name = read_gitconfig" >>.gitconfig &&
+	echo user.name=read_gitconfig >expect &&
+	git config --global --list >output &&
+	test_cmp expect output
+'
+
+test_done
-- 
1.7.8
Junio C Hamano· May 30, 2012, 21:54 UTC · re: Huynh Khoi Nguyen NGUYEN · lore

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

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.

Ramsay Jones· May 31, 2012, 22:06 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:
> Huynh Khoi Nguyen NGUYEN  <Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr>
[...]
Show 23 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.

I have not tried this patch (or the v3 version, which I haven't read yet), but is it likely that this has re-introduced the bug addressed by commit 05bab3ea ("config.c: Fix a static buffer overwrite bug by avoiding mkpath()", 19-11-2011)?.

I don't know the answer, but I suspect that it may have done just that. (indeed, it may well have made the bug more likely to appear).

Show 7 quoted lines
> 
> 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.

I agree. Also, using mksnpath() in the refactored code (rather than mkpath()) would be a good idea. :-P

ATB, Ramsay Jones

Huynh Khoi Nguyen NGUYEN· May 31, 2012, 14:40 UTC · re: Huynh Khoi Nguyen NGUYEN · lore

[PATCHv3] Read from XDG configuration file, not write

From: NGUYEN Huynh Khoi Nguyen <nguyenhu@ensibm.imag.fr>

Git will be able to read in $XDG_CONFIG_HOME/git/config, a new configuration file following XDG specification. In the order of reading, this file is between global configuration file and system wide configuration file. Git will not be able to write in this new configuration file. If $XDG_CONFIG_HOME is either not set or empty, $HOME/.config/git/config will be used as new configuration file.

Signed-off-by: Huynh Khoi Nguyen NGUYEN <Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr>
Signed-off-by: Lucien KONG <Lucien.Kong@ensimag.imag.fr>
Signed-off-by: Valentin DUPERRAY <Valentin.Duperray@ensimag.imag.fr>
Signed-off-by: Thomas NGUY <Thomas.Nguy@ensimag.imag.fr>
Signed-off-by: Franck JONAS <Franck.Jonas@ensimag.imag.fr>
---
 Documentation/git-config.txt    |   12 ++++-
 builtin/config.c                |   37 +++++++++++-----
 cache.h                         |    1 +
 config.c                        |    5 ++
 path.c                          |   12 +++++
 t/t1306-read-xdg-config-file.sh |   87 +++++++++++++++++++++++++++++++++++++++
 6 files changed, 139 insertions(+), 15 deletions(-)
 create mode 100755 t/t1306-read-xdg-config-file.sh
diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index d9463cb..004015a 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -99,8 +99,8 @@ OPTIONS
 	For writing options: write to global ~/.gitconfig file rather than
 	the repository .git/config.
 +
-For reading options: read only from global ~/.gitconfig rather than
-from all available files.
+For reading options: read only from global ~/.gitconfig then from
+$XDG_CONFIG_HOME/git/config rather than from all available files.
 +
 See also <<FILES>>.
 
@@ -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,12 @@ $GIT_DIR/config::
 	User-specific configuration file. Also called "global"
 	configuration file.
 
+$XDG_CONFIG_HOME/git/config::
+	Second user-specific configuration file. If $XDG_CONFIG_HOME is not set
+	or empty, $HOME/.config/git/config will be used. Any single-valued
+	variable set in this file will be overwritten by whatever is in
+	~/.gitconfig.
+
 $(prefix)/etc/gitconfig::
 	System-wide configuration file.
 
diff --git a/builtin/config.c b/builtin/config.c
index 33c8820..3fb23a1 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;
@@ -172,7 +172,9 @@ static int get_value(const char *key_, const char *regex_)
 		const char *home = getenv("HOME");
 		local = repo_config = git_pathdup("config");
 		if (home)
-			global = xstrdup(mkpath("%s/.gitconfig", home));
+			gitconfig_global = xstrdup(mkpath("%s/.gitconfig", home));
+		if (xdg_git_path("config"))
+			xdg_global = xdg_git_path("config");
 		if (git_config_system())
 			system_wide = git_etc_gitconfig();
 	}
@@ -229,15 +231,19 @@ static int get_value(const char *key_, const char *regex_)
 
 	if (do_all && system_wide)
 		git_config_from_file(fn, system_wide, data);
-	if (do_all && global)
-		git_config_from_file(fn, global, data);
+	if (do_all && xdg_global)
+		git_config_from_file(fn, xdg_global, data);
+	if (do_all && gitconfig_global)
+		git_config_from_file(fn, gitconfig_global, data);
 	if (do_all)
 		git_config_from_file(fn, local, data);
 	git_config_from_parameters(fn, data);
 	if (!do_all && !seen)
 		git_config_from_file(fn, local, data);
-	if (!do_all && !seen && global)
-		git_config_from_file(fn, global, data);
+	if (!do_all && !seen && gitconfig_global)
+		git_config_from_file(fn, gitconfig_global, data);
+	if (!do_all && !seen && xdg_global)
+		git_config_from_file(fn, xdg_global, data);
 	if (!do_all && !seen && system_wide)
 		git_config_from_file(fn, system_wide, data);
 
@@ -254,7 +260,8 @@ static int get_value(const char *key_, const char *regex_)
 
 free_strings:
 	free(repo_config);
-	free(global);
+	free(gitconfig_global);
+	free(xdg_global);
 	return ret;
 }
 
@@ -380,12 +387,18 @@ 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));
-			given_config_file = user_config;
-		} else {
+		const char *gitconfig_path = mkpath("%s/.gitconfig", home);
+		const char *xdg_config_path = xdg_git_path("config");
+
+		if (access(gitconfig_path, R_OK) && !access(xdg_config_path, R_OK) &&
+		    (actions == ACTION_LIST ||
+		     actions == ACTION_GET_COLOR ||
+		     actions == ACTION_GET_COLORBOOL))
+			given_config_file = xdg_config_path;
+		else if (home)
+			given_config_file = xstrdup(gitconfig_path);
+		else
 			die("$HOME not set");
-		}
 	}
 	else if (use_system_config)
 		given_config_file = git_etc_gitconfig();
diff --git a/cache.h b/cache.h
index 06413e1..b86d083 100644
--- a/cache.h
+++ b/cache.h
@@ -708,6 +708,7 @@ int set_shared_perm(const char *path, int mode);
 int safe_create_leading_directories(char *path);
 int safe_create_leading_directories_const(const char *path);
 int mkdir_in_gitdir(const char *path);
+extern char *xdg_git_path(char *file);
 extern char *expand_user_path(const char *path);
 const char *enter_repo(const char *path, int strict);
 static inline int is_absolute_path(const char *path)
diff --git a/config.c b/config.c
index 71ef171..e08632f 100644
--- a/config.c
+++ b/config.c
@@ -937,6 +937,11 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 		found += 1;
 	}
 
+	if (!access(xdg_git_path("config"), R_OK)) {
+		ret += git_config_from_file(fn, xdg_git_path("config"), data);
+		found += 1;
+	}
+
 	home = getenv("HOME");
 	if (home) {
 		char buf[PATH_MAX];
diff --git a/path.c b/path.c
index 6f2aa69..7dbedb2 100644
--- a/path.c
+++ b/path.c
@@ -122,6 +122,18 @@ char *git_path(const char *fmt, ...)
 	return cleanup_path(pathname);
 }
 
+char *xdg_git_path(char *file)
+{
+	const char *home = getenv("HOME");
+	const char *xdg_config_home = getenv("XDG_CONFIG_HOME");
+
+	if (xdg_config_home)
+		return xstrdup(mkpath("%s/git/%s", xdg_config_home, file));
+	if (home)
+		return xstrdup(mkpath("%s/.config/git/%s", home, file));
+	return NULL;
+}
+
 char *git_path_submodule(const char *path, const char *fmt, ...)
 {
 	char *pathname = get_pathname();
diff --git a/t/t1306-read-xdg-config-file.sh b/t/t1306-read-xdg-config-file.sh
new file mode 100755
index 0000000..4cab38b
--- /dev/null
+++ b/t/t1306-read-xdg-config-file.sh
@@ -0,0 +1,87 @@
+#!/bin/sh
+#
+# Copyright (c) 2012 Valentin Duperray, Lucien Kong, Franck Jonas,
+#		     Thomas Nguy, Khoi Nguyen
+#		     Grenoble INP Ensimag
+#
+
+test_description='possibility to read $XDG_CONFIG_HOME/git/config file'
+
+. ./test-lib.sh
+
+test_expect_success 'read automatically: xdg file exists and ~/.gitconfig doesn'\''t' '
+	mkdir -p .config/git &&
+	echo "[advice]" >.config/git/config &&
+	echo "	statushints = false" >>.config/git/config &&
+	cat >expect <<-\EOF &&
+	# On branch master
+	#
+	# Initial commit
+	#
+	# Untracked files:
+	#	.config/
+	#	expect
+	#	output
+	nothing added to commit but untracked files present
+	EOF
+	git status >output &&
+	test_cmp expect output
+'
+
+test_expect_success 'read automatically: xdg file exists and ~/.gitconfig exists' '
+	>.gitconfig &&
+	echo "[advice]" >.gitconfig &&
+	echo "	statushints = true" >>.gitconfig &&
+	cat >expect <<-\EOF &&
+	# On branch master
+	#
+	# Initial commit
+	#
+	# Untracked files:
+	#   (use "git add <file>..." to include in what will be committed)
+	#
+	#	.config/
+	#	.gitconfig
+	#	expect
+	#	output
+	nothing added to commit but untracked files present (use "git add" to track)
+	EOF
+	git status >output &&
+	test_cmp expect output
+'
+
+test_expect_success 'read with --get: xdg file exists and ~/.gitconfig doesn'\''t' '
+	rm .gitconfig &&
+	echo "[user]" >.config/git/config &&
+	echo "	name = read_config" >>.config/git/config &&
+	echo read_config >expect &&
+	git config --get user.name >output &&
+	test_cmp expect output
+'
+
+test_expect_success 'read with --get: xdg file exists and ~/.gitconfig exists' '
+	>.gitconfig &&
+	echo "[user]" >.gitconfig &&
+	echo "	name = read_gitconfig" >>.gitconfig &&
+	echo read_gitconfig >expect &&
+	git config --get user.name >output &&
+	test_cmp expect output
+'
+
+test_expect_success 'read with --list: xdg file exists and ~/.gitconfig doesn'\''t' '
+	rm .gitconfig &&
+	echo user.name=read_config >expect &&
+	git config --global --list >output &&
+	test_cmp expect output
+'
+
+test_expect_success 'read with --list: xdg file exists and ~/.gitconfig exists' '
+	>.gitconfig &&
+	echo "[user]" >.gitconfig &&
+	echo "	name = read_gitconfig" >>.gitconfig &&
+	echo user.name=read_gitconfig >expect &&
+	git config --global --list >output &&
+	test_cmp expect output
+'
+
+test_done
-- 
1.7.8
Junio C Hamano· May 31, 2012, 20:13 UTC · re: Huynh Khoi Nguyen NGUYEN · lore

Re: [PATCHv3] Read from XDG configuration file, not write

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

> From: NGUYEN Huynh Khoi Nguyen <nguyenhu@ensibm.imag.fr>
>
> Git will be able to read in $XDG_CONFIG_HOME/git/config, a new
Getting nicer.
Show 11 quoted lines
> @@ -172,7 +172,9 @@ static int get_value(const char *key_, const char *regex_)
>  		const char *home = getenv("HOME");
>  		local = repo_config = git_pathdup("config");
>  		if (home)
> -			global = xstrdup(mkpath("%s/.gitconfig", home));
> +			gitconfig_global = xstrdup(mkpath("%s/.gitconfig", home));
> +		if (xdg_git_path("config"))
> +			xdg_global = xdg_git_path("config");
>  		if (git_config_system())
>  			system_wide = git_etc_gitconfig();
>  	}

Hrm, xdg_git_path() returns allocated memory, and each call site leaks its return value, no?

I didn't mean a micro-helper function like xdg_git_path() when I suggested refactoring. I meant a helper that figures out all the necessary bits in one go. For example, can't the above call site look more like this?

        static int get_value(const char *key_, const char *regex_)
        {
                int ret = -1;
                char *global = NULL, *xdg = NULL, *repo_config = NULL;
                const char *system_wide = NULL, *local;
                struct config_include_data inc = CONFIG_INCLUDE_INIT;
                config_fn_t fn;
                void *data;
                local = given_config_file;
                if (!local) {
                        local = repo_config = git_pathdup("config");
                        if (git_config_system())
                                system_wide = git_etc_gitconfig();
			home_config_paths(&global, &xdg);
                }
		...
And then the config.c::home_config_paths() may look like:
	void home_config_paths(char **global, char **xdg)
	{
		char *xdg_home = getenv("XDG_CONFIG_HOME");
                char *home = getenv("HOME");
                char *to_free = NULL;
		
                if (!home) {
			*global = NULL;
		} else {
			if (!xdg_home) {
	                        to_free = strdup(mkpath("%s/.config", home));
				xdg_home = to_free;
			}
			*global = xstrdup(mkpath("%s/.gitconfig", home));
		}
                
                if (!xdg_home)
                	*xdg = NULL;
		else
			*xdg = xstrdup(mkpath("%s/git/config", xdg_home));
		free(to_free);
	}
Show 25 quoted lines
> diff --git a/t/t1306-read-xdg-config-file.sh b/t/t1306-read-xdg-config-file.sh
> new file mode 100755
> index 0000000..4cab38b
> --- /dev/null
> +++ b/t/t1306-read-xdg-config-file.sh
> @@ -0,0 +1,87 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2012 Valentin Duperray, Lucien Kong, Franck Jonas,
> +#		     Thomas Nguy, Khoi Nguyen
> +#		     Grenoble INP Ensimag
> +#
> +
> +test_description='possibility to read $XDG_CONFIG_HOME/git/config file'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'read automatically: xdg file exists and ~/.gitconfig doesn'\''t' '
> +	mkdir -p .config/git &&
> +	echo "[advice]" >.config/git/config &&
> +	echo "	statushints = false" >>.config/git/config &&
> +	cat >expect <<-\EOF &&
> +	# On branch master
> +	#
> +	# Initial commit

Use of "status" output as test vector is not such a good idea for multiple reasons.

 - You are aware that the "status hints" feature is currently in
   flux (somebody nearby is working on a patch);
 - Even without another topic that currently works on it, "git
   status" output is for human consumption and subject to
   improvement and also l10n.
 - "status" output lists tracked and untracked paths, which makes it
   harder to later introduce new file to the test directory that are
   needed for later tests, or rename ones that were used in earlier
   versions of the test, without having to update all of these test
   vectors.
Show 5 quoted lines
> +	#
> +	# Untracked files:
> +	#	.config/
> +	#	expect
> +	#	output

This is a good example that shows why using "status" output as a test vector is not a good idea.

Notice you listed "expect" and "output" here. As my next suggestion is to rename "output" to "actual" (we typically "test_cmp expect actual"), use of "git status" output as a test vector would mean you would have to update them whenever such a change is needed.

Show 5 quoted lines
> +	nothing added to commit but untracked files present
> +	EOF
> +	git status >output &&
> +	test_cmp expect output
> +'
Huynh Khoi Nguyen NGUYEN· Jun 1, 2012, 21:23 UTC · re: Huynh Khoi Nguyen NGUYEN · lore

[PATCHv4] Read (but not write) from XDG configuration, XDG attributes and XDG ignore files

From: NGUYEN Huynh Khoi Nguyen <nguyenhu@ensibm.imag.fr>

Git will be able to read in $XDG_CONFIG_HOME/git/config, a new configuration file following XDG specification. In the order of reading, this file is between global configuration file and system wide configuration file. Git will not be able to write in this new configuration file. If core.excludesfile is not define, Git will read the global exclude files in $XDG_CONFIG_HOME/git/ignore. Same goes for core.attributesfile in $XDG_CONFIG_HOME/git/attributes. If $XDG_CONFIG_HOME is either not set or empty, $HOME/.config will be used.

Signed-off-by: Huynh Khoi Nguyen NGUYEN <Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr>
Signed-off-by: Lucien KONG <Lucien.Kong@ensimag.imag.fr>
Signed-off-by: Valentin DUPERRAY <Valentin.Duperray@ensimag.imag.fr>
Signed-off-by: Thomas NGUY <Thomas.Nguy@ensimag.imag.fr>
Signed-off-by: Franck JONAS <Franck.Jonas@ensimag.imag.fr>
Signed-off-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
---
 Documentation/git-config.txt    |   12 +++-
 attr.c                          |   10 +++
 builtin/config.c                |   28 ++++++---
 cache.h                         |    1 +
 config.c                        |   21 ++++---
 dir.c                           |    4 +
 path.c                          |   26 ++++++++
 t/t1306-read-xdg-config-file.sh |  133 +++++++++++++++++++++++++++++++++++++++
 8 files changed, 214 insertions(+), 21 deletions(-)
 create mode 100755 t/t1306-read-xdg-config-file.sh
diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index d9463cb..7e344a2 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -99,8 +99,8 @@ OPTIONS
 	For writing options: write to global ~/.gitconfig file rather than
 	the repository .git/config.
 +
-For reading options: read only from global ~/.gitconfig rather than
-from all available files.
+For reading options: read only from global ~/.gitconfig and from
+$XDG_CONFIG_HOME/git/config rather than from all available files.
 +
 See also <<FILES>>.
 
@@ -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,12 @@ $GIT_DIR/config::
 	User-specific configuration file. Also called "global"
 	configuration file.
 
+$XDG_CONFIG_HOME/git/config::
+	Second user-specific configuration file. If $XDG_CONFIG_HOME is not set
+	or empty, $HOME/.config/git/config will be used. Any single-valued
+	variable set in this file will be overwritten by whatever is in
+	~/.gitconfig.
+
 $(prefix)/etc/gitconfig::
 	System-wide configuration file.
 
diff --git a/attr.c b/attr.c
index 303751f..441387f 100644
--- a/attr.c
+++ b/attr.c
@@ -497,6 +497,9 @@ static int git_attr_system(void)
 static void bootstrap_attr_stack(void)
 {
 	struct attr_stack *elem;
+	char *xdg_attributes_file;
+
+	home_config_paths(NULL, &xdg_attributes_file, "attributes");
 
 	if (attr_stack)
 		return;
@@ -522,6 +525,13 @@ static void bootstrap_attr_stack(void)
 			elem->prev = attr_stack;
 			attr_stack = elem;
 		}
+	} else if (!access(xdg_attributes_file, R_OK)) {
+		elem = read_attr_from_file(xdg_attributes_file, 1);
+		if (elem) {
+			elem->origin = NULL;
+			elem->prev = attr_stack;
+			attr_stack = elem;
+		}
 	}
 
 	if (!is_bare_repository() || direction == GIT_ATTR_INDEX) {
diff --git a/builtin/config.c b/builtin/config.c
index 33c8820..da54fd1 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 *global = NULL, *xdg = NULL, *repo_config = NULL;
 	const char *system_wide = NULL, *local;
 	struct config_include_data inc = CONFIG_INCLUDE_INIT;
 	config_fn_t fn;
@@ -169,12 +169,10 @@ static int get_value(const char *key_, const char *regex_)
 
 	local = given_config_file;
 	if (!local) {
-		const char *home = getenv("HOME");
 		local = repo_config = git_pathdup("config");
-		if (home)
-			global = xstrdup(mkpath("%s/.gitconfig", home));
 		if (git_config_system())
 			system_wide = git_etc_gitconfig();
+		home_config_paths(&global, &xdg, "config");
 	}
 
 	if (use_key_regexp) {
@@ -229,6 +227,8 @@ static int get_value(const char *key_, const char *regex_)
 
 	if (do_all && system_wide)
 		git_config_from_file(fn, system_wide, data);
+	if (do_all && xdg)
+		git_config_from_file(fn, xdg, data);
 	if (do_all && global)
 		git_config_from_file(fn, global, data);
 	if (do_all)
@@ -238,6 +238,8 @@ static int get_value(const char *key_, const char *regex_)
 		git_config_from_file(fn, local, data);
 	if (!do_all && !seen && global)
 		git_config_from_file(fn, global, data);
+	if (!do_all && !seen && xdg)
+		git_config_from_file(fn, xdg, data);
 	if (!do_all && !seen && system_wide)
 		git_config_from_file(fn, system_wide, data);
 
@@ -255,6 +257,7 @@ static int get_value(const char *key_, const char *regex_)
 free_strings:
 	free(repo_config);
 	free(global);
+	free(xdg);
 	return ret;
 }
 
@@ -379,13 +382,20 @@ 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 = NULL;
+		char *xdg_config = NULL;
+
+		home_config_paths(&user_config, &xdg_config, "config");
+
+		if (access(user_config, R_OK) && !access(xdg_config, R_OK) &&
+		    (actions == ACTION_LIST ||
+		     actions == ACTION_GET_COLOR ||
+		     actions == ACTION_GET_COLORBOOL))
+			given_config_file = xdg_config;
+		else if (user_config)
 			given_config_file = user_config;
-		} else {
+		else
 			die("$HOME not set");
-		}
 	}
 	else if (use_system_config)
 		given_config_file = git_etc_gitconfig();
diff --git a/cache.h b/cache.h
index 06413e1..0632503 100644
--- a/cache.h
+++ b/cache.h
@@ -708,6 +708,7 @@ int set_shared_perm(const char *path, int mode);
 int safe_create_leading_directories(char *path);
 int safe_create_leading_directories_const(const char *path);
 int mkdir_in_gitdir(const char *path);
+extern void home_config_paths(char **global, char **xdg, char *file);
 extern char *expand_user_path(const char *path);
 const char *enter_repo(const char *path, int strict);
 static inline int is_absolute_path(const char *path)
diff --git a/config.c b/config.c
index 71ef171..d1393b8 100644
--- a/config.c
+++ b/config.c
@@ -929,7 +929,10 @@ int git_config_system(void)
 int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 {
 	int ret = 0, found = 0;
-	const char *home = NULL;
+	char *xdg_config = NULL;
+	char *user_config = NULL;
+
+	home_config_paths(&user_config, &xdg_config, "config");
 
 	if (git_config_system() && !access(git_etc_gitconfig(), R_OK)) {
 		ret += git_config_from_file(fn, git_etc_gitconfig(),
@@ -937,14 +940,14 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 		found += 1;
 	}
 
-	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);
-			found += 1;
-		}
+	if (!access(xdg_config, R_OK)) {
+		ret += git_config_from_file(fn, xdg_config, data);
+		found += 1;
+	}
+
+	if (!access(user_config, R_OK)) {
+		ret += git_config_from_file(fn, user_config, data);
+		found += 1;
 	}
 
 	if (repo_config && !access(repo_config, R_OK)) {
diff --git a/dir.c b/dir.c
index ed1510f..e0c3589 100644
--- a/dir.c
+++ b/dir.c
@@ -1234,13 +1234,17 @@ int remove_dir_recursively(struct strbuf *path, int flag)
 void setup_standard_excludes(struct dir_struct *dir)
 {
 	const char *path;
+	char *xdg_path;
 
 	dir->exclude_per_dir = ".gitignore";
 	path = git_path("info/exclude");
+	home_config_paths(NULL, &xdg_path, "ignore");
 	if (!access(path, R_OK))
 		add_excludes_from_file(dir, path);
 	if (excludes_file && !access(excludes_file, R_OK))
 		add_excludes_from_file(dir, excludes_file);
+	else if (!access(xdg_path, R_OK))
+		add_excludes_from_file(dir, xdg_path);
 }
 
 int remove_path(const char *name)
diff --git a/path.c b/path.c
index 6f2aa69..53f3f53 100644
--- a/path.c
+++ b/path.c
@@ -122,6 +122,32 @@ char *git_path(const char *fmt, ...)
 	return cleanup_path(pathname);
 }
 
+void home_config_paths(char **global, char **xdg, char *file)
+{
+	char *xdg_home = getenv("XDG_CONFIG_HOME");
+	char *home = getenv("HOME");
+	char *to_free = NULL;
+
+	if (!home) {
+		if (global)
+			*global = NULL;
+	} else {
+		if (!xdg_home) {
+			to_free = strdup(mkpath("%s/.config", home));
+			xdg_home = to_free;
+		}
+		if (global)
+			*global = xstrdup(mkpath("%s/.gitconfig", home));
+	}
+
+	if (!xdg_home)
+		*xdg = NULL;
+	else
+		*xdg = xstrdup(mkpath("%s/git/%s", xdg_home, file));
+
+	free(to_free);
+}
+
 char *git_path_submodule(const char *path, const char *fmt, ...)
 {
 	char *pathname = get_pathname();
diff --git a/t/t1306-read-xdg-config-file.sh b/t/t1306-read-xdg-config-file.sh
new file mode 100755
index 0000000..a8775c4
--- /dev/null
+++ b/t/t1306-read-xdg-config-file.sh
@@ -0,0 +1,133 @@
+#!/bin/sh
+#
+# Copyright (c) 2012 Valentin Duperray, Lucien Kong, Franck Jonas,
+#		     Thomas Nguy, Khoi Nguyen
+#		     Grenoble INP Ensimag
+#
+
+test_description='Compatibility with $XDG_CONFIG_HOME/git/config /ignore and /attributes files'
+
+. ./test-lib.sh
+
+test_expect_success 'read config: xdg file exists and ~/.gitconfig doesn'\''t' '
+	mkdir -p .config/git &&
+	echo "[alias]" >.config/git/config &&
+	echo "	myalias = !echo in_config" >>.config/git/config &&
+	echo in_config >expected &&
+	git myalias >actual &&
+	test_cmp expected actual
+'
+
+
+test_expect_success 'read config: xdg file exists and ~/.gitconfig exists' '
+	>.gitconfig &&
+	echo "[alias]" >.gitconfig &&
+	echo "	myalias = !echo in_gitconfig" >>.gitconfig &&
+	echo in_gitconfig >expected &&
+	git myalias >actual &&
+	test_cmp expected actual
+'
+
+
+test_expect_success 'read with --get: xdg file exists and ~/.gitconfig doesn'\''t' '
+	rm .gitconfig &&
+	echo "[user]" >.config/git/config &&
+	echo "	name = read_config" >>.config/git/config &&
+	echo read_config >expected &&
+	git config --get user.name >actual &&
+	test_cmp expected actual
+'
+
+
+test_expect_success 'read with --get: xdg file exists and ~/.gitconfig exists' '
+	>.gitconfig &&
+	echo "[user]" >.gitconfig &&
+	echo "	name = read_gitconfig" >>.gitconfig &&
+	echo read_gitconfig >expected &&
+	git config --get user.name >actual &&
+	test_cmp expected actual
+'
+
+
+test_expect_success 'read with --list: xdg file exists and ~/.gitconfig doesn'\''t' '
+	rm .gitconfig &&
+	echo user.name=read_config >expected &&
+	git config --global --list >actual &&
+	test_cmp expected actual
+'
+
+
+test_expect_success 'read with --list: xdg file exists and ~/.gitconfig exists' '
+	>.gitconfig &&
+	echo "[user]" >.gitconfig &&
+	echo "	name = read_gitconfig" >>.gitconfig &&
+	echo user.name=read_gitconfig >expected &&
+	git config --global --list >actual &&
+	test_cmp expected actual
+'
+
+
+test_expect_success 'Exclusion of a file in the XDG ignore file' '
+	git init git &&
+	cd git &&
+	echo foo >to_be_excluded &&
+	git add to_be_excluded &&
+	git rm --cached to_be_excluded &&
+	mkdir -p ../.config/git/ &&
+	echo to_be_excluded >../.config/git/ignore &&
+	test_must_fail git add to_be_excluded
+'
+
+
+test_expect_success 'Exclusion in both XDG and local ignore files' '
+	echo to_be_excluded >.gitignore &&
+	test_must_fail git add to_be_excluded
+'
+
+
+test_expect_success 'Exclusion in a non-XDG global ignore file' '
+	echo >../.config/git/ignore &&
+	rm .gitignore &&
+	echo to_be_excluded >../my_gitignore &&
+	git config core.excludesfile ../my_gitignore &&
+	test_when_finished "cd .. && rm -rf git && rm -r .config" &&
+	test_must_fail git add to_be_excluded
+'
+
+
+test_expect_success 'Checking attributes in the XDG attributes file' '
+	git init git &&
+	cd git &&
+	echo foo >f &&
+	git check-attr -a f >actual &&
+	test_line_count -eq 0 actual &&
+	cd .. &&
+	mkdir -p .config/git/ &&
+	echo "f attr_f" >.config/git/attributes &&
+	cd git &&
+	echo "f: attr_f: set" >expected &&
+	git check-attr -a f >actual &&
+	test_cmp expected actual
+'
+
+
+test_expect_success 'Checking attributes in both XDG and local ignore files' '
+	echo  "f -attr_f" >.gitattributes &&
+	echo "f: attr_f: unset" >expected &&
+	git check-attr -a f >actual &&
+	test_cmp expected actual
+'
+
+
+test_expect_success 'Checking attributes in a non-XDG global attributes file' '
+	rm .gitattributes &&
+	echo "f attr_f=test" >../my_gitattributes &&
+	git config core.attributesfile ../my_gitattributes &&
+	test_when_finished "cd .. && rm -rf git && rm -r .config" &&
+	echo "f: attr_f: test" >expected &&
+	git check-attr -a f >actual &&
+	test_cmp expected actual
+'
+
+
+test_done
-- 
1.7.8
Matthieu Moy· Jun 2, 2012, 11:20 UTC · re: Huynh Khoi Nguyen NGUYEN · lore

Re: [PATCHv4] Read (but not write) from XDG configuration, XDG attributes and XDG ignore files

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

> Git will not be able to write in this new configuration file.

The use of the future is ambiguous, and since another patch will allow writing, I'd rather rephrase it as e.g.

Git currently does not write to this new configuration file.
> If core.excludesfile is not define, Git will read the global exclude
s/define/defined/
> files in $XDG_CONFIG_HOME/git/ignore. Same goes for
> core.attributesfile in $XDG_CONFIG_HOME/git/attributes. If
> $XDG_CONFIG_HOME is either not set or empty, $HOME/.config will be
> used.

I'd prefer having two separate patches for the config file and for the two others. If ignore and attributes are simple enough, they may go to the same patch, but ideally, there would be two separate patches again.

> --- a/Documentation/git-config.txt
> +++ b/Documentation/git-config.txt
No doc for core.excludesfile and core.attributesfile change :-(.
Show 26 quoted lines
> --- a/attr.c
> +++ b/attr.c
> @@ -497,6 +497,9 @@ static int git_attr_system(void)
>  static void bootstrap_attr_stack(void)
>  {
>  	struct attr_stack *elem;
> +	char *xdg_attributes_file;
> +
> +	home_config_paths(NULL, &xdg_attributes_file, "attributes");
>  
>  	if (attr_stack)
>  		return;
> @@ -522,6 +525,13 @@ static void bootstrap_attr_stack(void)
>  			elem->prev = attr_stack;
>  			attr_stack = elem;
>  		}
> +	} else if (!access(xdg_attributes_file, R_OK)) {
> +		elem = read_attr_from_file(xdg_attributes_file, 1);
> +		if (elem) {
> +			elem->origin = NULL;
> +			elem->prev = attr_stack;
> +			attr_stack = elem;
> +		}
>  	}
>  
>  	if (!is_bare_repository() || direction == GIT_ATTR_INDEX) {
The logic seems overly complex, and you duplicate the if() uselessly.

Why not just set the variable git_attributes_file before entering the if? Something like this:

diff --git a/attr.c b/attr.c
index 303751f..71dc472 100644
--- a/attr.c
+++ b/attr.c
@@ -515,6 +515,9 @@ static void bootstrap_attr_stack(void)
 		}
 	}
 
+	if (!git_attributes_file)
+		git_attributes_file = "foo";
+
 	if (git_attributes_file) {
 		elem = read_attr_from_file(git_attributes_file, 1);
 		if (elem) {

(obviously replacing "foo" by the actual code involving
home_config_paths(..., "attributes")).

Doing so, you may even get rid of the "if (git_attributes_file)" on the
next line.

Then, the logic is really "core.excludesfile defaults to the XDG file",
not "if such variable is set then do that else do something else".

> --- a/dir.c
> +++ b/dir.c
> @@ -1234,13 +1234,17 @@ int remove_dir_recursively(struct strbuf *path, int flag)
>  void setup_standard_excludes(struct dir_struct *dir)
>  {
>  	const char *path;
> +	char *xdg_path;
>  
>  	dir->exclude_per_dir = ".gitignore";
>  	path = git_path("info/exclude");
> +	home_config_paths(NULL, &xdg_path, "ignore");
>  	if (!access(path, R_OK))
>  		add_excludes_from_file(dir, path);
>  	if (excludes_file && !access(excludes_file, R_OK))
>  		add_excludes_from_file(dir, excludes_file);
> +	else if (!access(xdg_path, R_OK))
> +		add_excludes_from_file(dir, xdg_path);
>  }

Same remark here. Look at the patch I sent earlier to give a default
value:

http://thread.gmane.org/gmane.comp.version-control.git/133343/focus=133415

For example, you version reads from XDG file if core.excludesfile is
set, but the file it points to doesn't exist. I don't think this is
expected.

> +	echo foo >to_be_excluded &&
> +	git add to_be_excluded &&
> +	git rm --cached to_be_excluded &&

Err, why add and remove it? You just need to create it, right?

> +	cd .. &&
> +	mkdir -p .config/git/ &&

I don't like these relative references to $HOME. If you mean $HOME, why
not say

mkdir -p $HOME/.config/git/
echo "f attr_f" >$HOME/.config/git/

?

(you don't even need to "cd")
-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
nguyenhu@minatec.inpg.fr· Jun 2, 2012, 15:52 UTC · re: Matthieu Moy · lore

Re: [PATCHv4] Read (but not write) from XDG configuration, XDG attributes and XDG ignore files

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> a écrit :
> I'd prefer having two separate patches for the config file and for the
> two others. If ignore and attributes are simple enough, they may go to
> the same patch, but ideally, there would be two separate patches again.
We will separate this patch in three different patches.
> No doc for core.excludesfile and core.attributesfile change :-(.
It will be done for the next patch ;)
Show 52 quoted lines
>> --- a/attr.c
>> +++ b/attr.c
>> @@ -497,6 +497,9 @@ static int git_attr_system(void)
>>  static void bootstrap_attr_stack(void)
>>  {
>>  	struct attr_stack *elem;
>> +	char *xdg_attributes_file;
>> +
>> +	home_config_paths(NULL, &xdg_attributes_file, "attributes");
>>
>>  	if (attr_stack)
>>  		return;
>> @@ -522,6 +525,13 @@ static void bootstrap_attr_stack(void)
>>  			elem->prev = attr_stack;
>>  			attr_stack = elem;
>>  		}
>> +	} else if (!access(xdg_attributes_file, R_OK)) {
>> +		elem = read_attr_from_file(xdg_attributes_file, 1);
>> +		if (elem) {
>> +			elem->origin = NULL;
>> +			elem->prev = attr_stack;
>> +			attr_stack = elem;
>> +		}
>>  	}
>>
>>  	if (!is_bare_repository() || direction == GIT_ATTR_INDEX) {
>
> The logic seems overly complex, and you duplicate the if() uselessly.
>
> Why not just set the variable git_attributes_file before entering the
> if? Something like this:
>
> diff --git a/attr.c b/attr.c
> index 303751f..71dc472 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -515,6 +515,9 @@ static void bootstrap_attr_stack(void)
>  		}
>  	}
>
> +	if (!git_attributes_file)
> +		git_attributes_file = "foo";
> +
>  	if (git_attributes_file) {
>  		elem = read_attr_from_file(git_attributes_file, 1);
>  		if (elem) {
>
> (obviously replacing "foo" by the actual code involving
> home_config_paths(..., "attributes")).
>
> Doing so, you may even get rid of the "if (git_attributes_file)" on the
> next line.

We first thought to use an "else if" in order not to pointlessly check the existence of the xdg_attributes_file (or double checking git_attributes_file) if git_attributes_file exists. BTW after checking the code more closely, we do not need to verify the existence of the xdg_attributes_file so it is indeed more clean to use your version, Thank.

Show 27 quoted lines
>> --- a/dir.c
>> +++ b/dir.c
>> @@ -1234,13 +1234,17 @@ int remove_dir_recursively(struct strbuf  
>> *path, int flag)
>>  void setup_standard_excludes(struct dir_struct *dir)
>>  {
>>  	const char *path;
>> +	char *xdg_path;
>>
>>  	dir->exclude_per_dir = ".gitignore";
>>  	path = git_path("info/exclude");
>> +	home_config_paths(NULL, &xdg_path, "ignore");
>>  	if (!access(path, R_OK))
>>  		add_excludes_from_file(dir, path);
>>  	if (excludes_file && !access(excludes_file, R_OK))
>>  		add_excludes_from_file(dir, excludes_file);
>> +	else if (!access(xdg_path, R_OK))
>> +		add_excludes_from_file(dir, xdg_path);
>>  }
> Same remark here. Look at the patch I sent earlier to give a default
> value:
>
> http://thread.gmane.org/gmane.comp.version-control.git/133343/focus=133415
>
> For example, you version reads from XDG file if core.excludesfile is
> set, but the file it points to doesn't exist. I don't think this is
> expected.

Actually, it's the opposite. Our version only read from XDG file if core.excludesfile is not set. After checking your patch, it may be more logical to inialize the default value of excludes_file to the xdg_path as done for the attributes file.

Show 5 quoted lines
>> +	echo foo >to_be_excluded &&
>> +	git add to_be_excluded &&
>> +	git rm --cached to_be_excluded &&
>
> Err, why add and remove it? You just need to create it, right?

It was to check if to_be_excluded is indeed not ignored at the beginning of the test before ignoring it but that's seem a bit over-testing, I'll remove it.

Show 10 quoted lines
>> +	cd .. &&
>> +	mkdir -p .config/git/ &&
>
> I don't like these relative references to $HOME. If you mean $HOME, why
> not say
>
> mkdir -p $HOME/.config/git/
> echo "f attr_f" >$HOME/.config/git/
>
> ?

It will be fixed but BTW where the tests are executed, $HOME has a weird behaviour:

echo $HOME and echo "$HOME"
      both returns /.../t/trash directory.t1306-read-xdg-config-file
but   echo foo >$HOME   writes in ../t/trash
while echo foo >"$HOME" writes in t/trash directory.t1306-read-xdg-config-file
so "$HOME" is needed for the tests to work.
Matthieu Moy· Jun 2, 2012, 21:05 UTC · re: nguyenhu@minatec.inpg.fr · lore

Re: [PATCHv4] Read (but not write) from XDG configuration, XDG attributes and XDG ignore files

nguyenhu@minatec.inpg.fr writes:
Show 30 quoted lines
>>> --- a/dir.c
>>> +++ b/dir.c
>>> @@ -1234,13 +1234,17 @@ int remove_dir_recursively(struct strbuf  
>>> *path, int flag)
>>>  void setup_standard_excludes(struct dir_struct *dir)
>>>  {
>>>  	const char *path;
>>> +	char *xdg_path;
>>>
>>>  	dir->exclude_per_dir = ".gitignore";
>>>  	path = git_path("info/exclude");
>>> +	home_config_paths(NULL, &xdg_path, "ignore");
>>>  	if (!access(path, R_OK))
>>>  		add_excludes_from_file(dir, path);
>>>  	if (excludes_file && !access(excludes_file, R_OK))
>>>  		add_excludes_from_file(dir, excludes_file);
>>> +	else if (!access(xdg_path, R_OK))
>>> +		add_excludes_from_file(dir, xdg_path);
>>>  }
>> Same remark here. Look at the patch I sent earlier to give a default
>> value:
>>
>> http://thread.gmane.org/gmane.comp.version-control.git/133343/focus=133415
>>
>> For example, you version reads from XDG file if core.excludesfile is
>> set, but the file it points to doesn't exist. I don't think this is
>> expected.
>
> Actually, it's the opposite. Our version only read from XDG file if  
> core.excludesfile is not set.

It's what you want to do, but not what I read from the code. Your "else if" above is reachable if "excludes_file && !access(excludes_file, R_OK)" is false, which includes the case when excludes_file is set but does not exist.

Anyway, this just shows that the logic is too complex, we shouldn't need this discussion with simple enough code.

Show 5 quoted lines
> echo $HOME and echo "$HOME"
>       both returns /.../t/trash directory.t1306-read-xdg-config-file
> but   echo foo >$HOME   writes in ../t/trash
> while echo foo >"$HOME" writes in t/trash directory.t1306-read-xdg-config-file
> so "$HOME" is needed for the tests to work.

In general, $HOME means "evaluate $HOME and do the whitespace splitting after", and "$HOME" means "evaluate $HOME and don't do whitespace splitting". Since "trash directory" contains a space, you need the quoting.

There are places where quoting is not needed, though, and I think redirect is part of them. But I wouldn't be surprised if the different levels of quoting when ran in test-lib broke that.

My advice: always quote variables, unless you have a very good reason not to do so.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Huynh Khoi Nguyen NGUYEN· Jun 3, 2012, 20:14 UTC · re: Huynh Khoi Nguyen NGUYEN · lore

[PATCHv5 1/4] Read (but not write) from $XDG_CONFIG_HOME/git/config file

From: NGUYEN Huynh Khoi Nguyen <nguyenhu@ensibm.imag.fr>

Git will be able to read from $XDG_CONFIG_HOME/git/config, a new configuration file following XDG specification. In the order of reading, this file is between global configuration file and system wide configuration file. Git currently does not write to this new configuration file. If $XDG_CONFIG_HOME is either not set or empty, $HOME/.config will be used.

Signed-off-by: Huynh Khoi Nguyen NGUYEN <Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr>
Signed-off-by: Valentin DUPERRAY <Valentin.DUPERRAY@ensimag.imag.fr>
Signed-off-by: Franck JONAS <Franck.JONAS@ensimag.imag.fr>
Signed-off-by: Thomas NGUY <Thomas.NGUY@ensimag.imag.fr>
Signed-off-by: Lucien KONG <Lucien.KONG@ensimag.imag.fr>
Signed-off-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
---
 Documentation/git-config.txt |   12 ++++++--
 builtin/config.c             |   28 ++++++++++++------
 cache.h                      |    1 +
 config.c                     |   21 ++++++++------
 path.c                       |   26 +++++++++++++++++
 t/t1306-xdg-files.sh         |   64 ++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 131 insertions(+), 21 deletions(-)
 create mode 100755 t/t1306-xdg-files.sh
diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index d9463cb..7e344a2 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -99,8 +99,8 @@ OPTIONS
 	For writing options: write to global ~/.gitconfig file rather than
 	the repository .git/config.
 +
-For reading options: read only from global ~/.gitconfig rather than
-from all available files.
+For reading options: read only from global ~/.gitconfig and from
+$XDG_CONFIG_HOME/git/config rather than from all available files.
 +
 See also <<FILES>>.
 
@@ -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,12 @@ $GIT_DIR/config::
 	User-specific configuration file. Also called "global"
 	configuration file.
 
+$XDG_CONFIG_HOME/git/config::
+	Second user-specific configuration file. If $XDG_CONFIG_HOME is not set
+	or empty, $HOME/.config/git/config will be used. Any single-valued
+	variable set in this file will be overwritten by whatever is in
+	~/.gitconfig.
+
 $(prefix)/etc/gitconfig::
 	System-wide configuration file.
 
diff --git a/builtin/config.c b/builtin/config.c
index 33c8820..da54fd1 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 *global = NULL, *xdg = NULL, *repo_config = NULL;
 	const char *system_wide = NULL, *local;
 	struct config_include_data inc = CONFIG_INCLUDE_INIT;
 	config_fn_t fn;
@@ -169,12 +169,10 @@ static int get_value(const char *key_, const char *regex_)
 
 	local = given_config_file;
 	if (!local) {
-		const char *home = getenv("HOME");
 		local = repo_config = git_pathdup("config");
-		if (home)
-			global = xstrdup(mkpath("%s/.gitconfig", home));
 		if (git_config_system())
 			system_wide = git_etc_gitconfig();
+		home_config_paths(&global, &xdg, "config");
 	}
 
 	if (use_key_regexp) {
@@ -229,6 +227,8 @@ static int get_value(const char *key_, const char *regex_)
 
 	if (do_all && system_wide)
 		git_config_from_file(fn, system_wide, data);
+	if (do_all && xdg)
+		git_config_from_file(fn, xdg, data);
 	if (do_all && global)
 		git_config_from_file(fn, global, data);
 	if (do_all)
@@ -238,6 +238,8 @@ static int get_value(const char *key_, const char *regex_)
 		git_config_from_file(fn, local, data);
 	if (!do_all && !seen && global)
 		git_config_from_file(fn, global, data);
+	if (!do_all && !seen && xdg)
+		git_config_from_file(fn, xdg, data);
 	if (!do_all && !seen && system_wide)
 		git_config_from_file(fn, system_wide, data);
 
@@ -255,6 +257,7 @@ static int get_value(const char *key_, const char *regex_)
 free_strings:
 	free(repo_config);
 	free(global);
+	free(xdg);
 	return ret;
 }
 
@@ -379,13 +382,20 @@ 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 = NULL;
+		char *xdg_config = NULL;
+
+		home_config_paths(&user_config, &xdg_config, "config");
+
+		if (access(user_config, R_OK) && !access(xdg_config, R_OK) &&
+		    (actions == ACTION_LIST ||
+		     actions == ACTION_GET_COLOR ||
+		     actions == ACTION_GET_COLORBOOL))
+			given_config_file = xdg_config;
+		else if (user_config)
 			given_config_file = user_config;
-		} else {
+		else
 			die("$HOME not set");
-		}
 	}
 	else if (use_system_config)
 		given_config_file = git_etc_gitconfig();
diff --git a/cache.h b/cache.h
index 06413e1..0632503 100644
--- a/cache.h
+++ b/cache.h
@@ -708,6 +708,7 @@ int set_shared_perm(const char *path, int mode);
 int safe_create_leading_directories(char *path);
 int safe_create_leading_directories_const(const char *path);
 int mkdir_in_gitdir(const char *path);
+extern void home_config_paths(char **global, char **xdg, char *file);
 extern char *expand_user_path(const char *path);
 const char *enter_repo(const char *path, int strict);
 static inline int is_absolute_path(const char *path)
diff --git a/config.c b/config.c
index 71ef171..d1393b8 100644
--- a/config.c
+++ b/config.c
@@ -929,7 +929,10 @@ int git_config_system(void)
 int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 {
 	int ret = 0, found = 0;
-	const char *home = NULL;
+	char *xdg_config = NULL;
+	char *user_config = NULL;
+
+	home_config_paths(&user_config, &xdg_config, "config");
 
 	if (git_config_system() && !access(git_etc_gitconfig(), R_OK)) {
 		ret += git_config_from_file(fn, git_etc_gitconfig(),
@@ -937,14 +940,14 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 		found += 1;
 	}
 
-	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);
-			found += 1;
-		}
+	if (!access(xdg_config, R_OK)) {
+		ret += git_config_from_file(fn, xdg_config, data);
+		found += 1;
+	}
+
+	if (!access(user_config, R_OK)) {
+		ret += git_config_from_file(fn, user_config, data);
+		found += 1;
 	}
 
 	if (repo_config && !access(repo_config, R_OK)) {
diff --git a/path.c b/path.c
index 6f2aa69..53f3f53 100644
--- a/path.c
+++ b/path.c
@@ -122,6 +122,32 @@ char *git_path(const char *fmt, ...)
 	return cleanup_path(pathname);
 }
 
+void home_config_paths(char **global, char **xdg, char *file)
+{
+	char *xdg_home = getenv("XDG_CONFIG_HOME");
+	char *home = getenv("HOME");
+	char *to_free = NULL;
+
+	if (!home) {
+		if (global)
+			*global = NULL;
+	} else {
+		if (!xdg_home) {
+			to_free = strdup(mkpath("%s/.config", home));
+			xdg_home = to_free;
+		}
+		if (global)
+			*global = xstrdup(mkpath("%s/.gitconfig", home));
+	}
+
+	if (!xdg_home)
+		*xdg = NULL;
+	else
+		*xdg = xstrdup(mkpath("%s/git/%s", xdg_home, file));
+
+	free(to_free);
+}
+
 char *git_path_submodule(const char *path, const char *fmt, ...)
 {
 	char *pathname = get_pathname();
diff --git a/t/t1306-xdg-files.sh b/t/t1306-xdg-files.sh
new file mode 100755
index 0000000..77823fc
--- /dev/null
+++ b/t/t1306-xdg-files.sh
@@ -0,0 +1,64 @@
+#!/bin/sh
+#
+# Copyright (c) 2012 Valentin Duperray, Lucien Kong, Franck Jonas,
+#		     Thomas Nguy, Khoi Nguyen
+#		     Grenoble INP Ensimag
+#
+
+test_description='Compatibility with $XDG_CONFIG_HOME/git/ files'
+
+. ./test-lib.sh
+
+test_expect_success 'read config: xdg file exists and ~/.gitconfig doesn'\''t' '
+	mkdir -p .config/git &&
+	echo "[alias]" >.config/git/config &&
+	echo "	myalias = !echo in_config" >>.config/git/config &&
+	echo in_config >expected &&
+	git myalias >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'read config: xdg file exists and ~/.gitconfig exists' '
+	>.gitconfig &&
+	echo "[alias]" >.gitconfig &&
+	echo "	myalias = !echo in_gitconfig" >>.gitconfig &&
+	echo in_gitconfig >expected &&
+	git myalias >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'read with --get: xdg file exists and ~/.gitconfig doesn'\''t' '
+	rm .gitconfig &&
+	echo "[user]" >.config/git/config &&
+	echo "	name = read_config" >>.config/git/config &&
+	echo read_config >expected &&
+	git config --get user.name >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'read with --get: xdg file exists and ~/.gitconfig exists' '
+	>.gitconfig &&
+	echo "[user]" >.gitconfig &&
+	echo "	name = read_gitconfig" >>.gitconfig &&
+	echo read_gitconfig >expected &&
+	git config --get user.name >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'read with --list: xdg file exists and ~/.gitconfig doesn'\''t' '
+	rm .gitconfig &&
+	echo user.name=read_config >expected &&
+	git config --global --list >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'read with --list: xdg file exists and ~/.gitconfig exists' '
+	>.gitconfig &&
+	echo "[user]" >.gitconfig &&
+	echo "	name = read_gitconfig" >>.gitconfig &&
+	echo user.name=read_gitconfig >expected &&
+	git config --global --list >actual &&
+	test_cmp expected actual
+'
+
+test_done
-- 
1.7.8
Huynh Khoi Nguyen NGUYEN· Jun 3, 2012, 20:14 UTC · re: Huynh Khoi Nguyen NGUYEN · lore

[PATCHv5 2/4] Let core.excludesfile default to $XDG_CONFIG_HOME/git/ignore

From: NGUYEN Huynh Khoi Nguyen <nguyenhu@ensibm.imag.fr>

If core.excludesfile is not defined, its default value will be $XDG_CONFIG_HOME/git/ignore in order to follow XDG specification. If $XDG_CONFIG_HOME is either not set or emty, $HOME/.config will be used.

Signed-off-by: Huynh Khoi Nguyen NGUYEN <Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr>
Signed-off-by: Valentin DUPERRAY <Valentin.DUPERRAY@ensimag.imag.fr>
Signed-off-by: Franck JONAS <Franck.JONAS@ensimag.imag.fr>
Signed-off-by: Thomas NGUY <Thomas.NGUY@ensimag.imag.fr>
Signed-off-by: Lucien KONG <Lucien.KONG@ensimag.imag.fr>
Signed-off-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
---
 Documentation/config.txt    |    4 +++-
 Documentation/gitignore.txt |    4 +++-
 dir.c                       |    7 ++++++-
 t/t1306-xdg-files.sh        |   23 +++++++++++++++++++++++
 4 files changed, 35 insertions(+), 3 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 915cb5a..bfaef2c 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -483,7 +483,9 @@ core.excludesfile::
 	'.git/info/exclude', git looks into this file for patterns
 	of files which are not meant to be tracked.  "`~/`" is expanded
 	to the value of `$HOME` and "`~user/`" to the specified user's
-	home directory.  See linkgit:gitignore[5].
+	home directory. Its default value is $XDG_CONFIG_HOME/git/ignore.
+	If $XDG_CONFIG_HOME is either not set or empty, $HOME/.config will
+	be used. See linkgit:gitignore[5].
 
 core.askpass::
 	Some commands (e.g. svn and http interfaces) that interactively
diff --git a/Documentation/gitignore.txt b/Documentation/gitignore.txt
index 2e7328b..05ee33f 100644
--- a/Documentation/gitignore.txt
+++ b/Documentation/gitignore.txt
@@ -50,7 +50,9 @@ the repository but are specific to one user's workflow) should go into
 the `$GIT_DIR/info/exclude` file.  Patterns which a user wants git to
 ignore in all situations (e.g., backup or temporary files generated by
 the user's editor of choice) generally go into a file specified by
-`core.excludesfile` in the user's `~/.gitconfig`.
+`core.excludesfile` in the user's `~/.gitconfig`. Its default value is
+$XDG_CONFIG_HOME/git/ignore. If $XDG_CONFIG_HOME is either not set or empty,
+$HOME/.config will be used.
 
 The underlying git plumbing tools, such as
 'git ls-files' and 'git read-tree', read
diff --git a/dir.c b/dir.c
index ed1510f..8c6f47f 100644
--- a/dir.c
+++ b/dir.c
@@ -1234,12 +1234,17 @@ int remove_dir_recursively(struct strbuf *path, int flag)
 void setup_standard_excludes(struct dir_struct *dir)
 {
 	const char *path;
+	char *xdg_path;
 
 	dir->exclude_per_dir = ".gitignore";
 	path = git_path("info/exclude");
+	if (!excludes_file) {
+		home_config_paths(NULL, &xdg_path, "ignore");
+		excludes_file = xdg_path;
+	}
 	if (!access(path, R_OK))
 		add_excludes_from_file(dir, path);
-	if (excludes_file && !access(excludes_file, R_OK))
+	if (!access(excludes_file, R_OK))
 		add_excludes_from_file(dir, excludes_file);
 }
 
diff --git a/t/t1306-xdg-files.sh b/t/t1306-xdg-files.sh
index 77823fc..1a1f07e 100755
--- a/t/t1306-xdg-files.sh
+++ b/t/t1306-xdg-files.sh
@@ -61,4 +61,27 @@ test_expect_success 'read with --list: xdg file exists and ~/.gitconfig exists'
 	test_cmp expected actual
 '
 
+test_expect_success 'Exclusion of a file in the XDG ignore file' '
+	git init git &&
+	cd git &&
+	echo foo >to_be_excluded &&
+	mkdir -p "$HOME"/.config/git/ &&
+	echo to_be_excluded >"$HOME"/.config/git/ignore &&
+	test_must_fail git add to_be_excluded
+'
+
+test_expect_success 'Exclusion in both XDG and local ignore files' '
+	echo to_be_excluded >.gitignore &&
+	test_must_fail git add to_be_excluded
+'
+
+test_expect_success 'Exclusion in a non-XDG global ignore file' '
+	echo >"$HOME"/.config/git/ignore &&
+	rm .gitignore &&
+	echo to_be_excluded >"$HOME"/my_gitignore &&
+	git config core.excludesfile "$HOME"/my_gitignore &&
+	test_when_finished "cd \"$HOME\" && rm -rf git && rm -r .config" &&
+	test_must_fail git add to_be_excluded
+'
+
 test_done
-- 
1.7.8
Matthieu Moy· Jun 4, 2012, 11:43 UTC · re: Huynh Khoi Nguyen NGUYEN · lore

Re: [PATCHv5 2/4] Let core.excludesfile default to $XDG_CONFIG_HOME/git/ignore

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

> If core.excludesfile is not defined, its default value will be
> $XDG_CONFIG_HOME/git/ignore in order to follow XDG specification. If
> $XDG_CONFIG_HOME is either not set or emty, $HOME/.config will be
> used.

It's not clear whether $HOME/.config will be used to replace $XDG_CONFIG_HOME or $XDG_CONFIG_HOME/git/ignore. I'd say explicitely $HOME/.config/git/ignore to avoid this.

Show 10 quoted lines
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -483,7 +483,9 @@ core.excludesfile::
>  	'.git/info/exclude', git looks into this file for patterns
>  	of files which are not meant to be tracked.  "`~/`" is expanded
>  	to the value of `$HOME` and "`~user/`" to the specified user's
> -	home directory.  See linkgit:gitignore[5].
> +	home directory. Its default value is $XDG_CONFIG_HOME/git/ignore.
> +	If $XDG_CONFIG_HOME is either not set or empty, $HOME/.config will
> +	be used. See linkgit:gitignore[5].
Likewise.
Show 10 quoted lines
> --- a/Documentation/gitignore.txt
> +++ b/Documentation/gitignore.txt
> @@ -50,7 +50,9 @@ the repository but are specific to one user's workflow) should go into
>  the `$GIT_DIR/info/exclude` file.  Patterns which a user wants git to
>  ignore in all situations (e.g., backup or temporary files generated by
>  the user's editor of choice) generally go into a file specified by
> -`core.excludesfile` in the user's `~/.gitconfig`.
> +`core.excludesfile` in the user's `~/.gitconfig`. Its default value is
> +$XDG_CONFIG_HOME/git/ignore. If $XDG_CONFIG_HOME is either not set or empty,
> +$HOME/.config will be used.
Likewise.
Otherwise, sounds good.
-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
nguyenhu@minatec.inpg.fr· Jun 5, 2012, 13:17 UTC · re: Matthieu Moy · lore

Re: [PATCHv5 2/4] Let core.excludesfile default to $XDG_CONFIG_HOME/git/ignore

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> a écrit :
Show 27 quoted lines
> Huynh Khoi Nguyen NGUYEN <Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr>
> writes:
>
>> If core.excludesfile is not defined, its default value will be
>> $XDG_CONFIG_HOME/git/ignore in order to follow XDG specification. If
>> $XDG_CONFIG_HOME is either not set or emty, $HOME/.config will be
>> used.
>
> It's not clear whether $HOME/.config will be used to replace
> $XDG_CONFIG_HOME or $XDG_CONFIG_HOME/git/ignore. I'd say explicitely
> $HOME/.config/git/ignore to avoid this.
>
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>
> Likewise.
>
>> --- a/Documentation/gitignore.txt
>> +++ b/Documentation/gitignore.txt
>
> Likewise.
>
> Otherwise, sounds good.
>
> --
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/

We will also do the same changes with core.attributesfile, in our next v6 version.

Huynh Khoi Nguyen NGUYEN· Jun 3, 2012, 20:14 UTC · re: Huynh Khoi Nguyen NGUYEN · lore

[PATCHv5 3/4] Let core.attributesfile default to $XDG_CONFIG_HOME/git/attributes

From: NGUYEN Huynh Khoi Nguyen <nguyenhu@ensibm.imag.fr>

If core.attributesfile is not defined, its default value will be $XDG_CONFIG_HOME/git/attributes in order to follow XDG specification. If $XDG_CONFIG_HOME is either not set or emty, $HOME/.config will be used.

Signed-off-by: Huynh Khoi Nguyen NGUYEN <Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr>
Signed-off-by: Valentin DUPERRAY <Valentin.DUPERRAY@ensimag.imag.fr>
Signed-off-by: Franck JONAS <Franck.JONAS@ensimag.imag.fr>
Signed-off-by: Thomas NGUY <Thomas.NGUY@ensimag.imag.fr>
Signed-off-by: Lucien KONG <Lucien.KONG@ensimag.imag.fr>
Signed-off-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
---
 Documentation/config.txt        |    4 +++-
 Documentation/gitattributes.txt |    2 ++
 attr.c                          |   17 ++++++++++-------
 t/t1306-xdg-files.sh            |   30 ++++++++++++++++++++++++++++++
 4 files changed, 45 insertions(+), 8 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index bfaef2c..9dc274c 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -500,7 +500,9 @@ core.attributesfile::
 	In addition to '.gitattributes' (per-directory) and
 	'.git/info/attributes', git looks into this file for attributes
 	(see linkgit:gitattributes[5]). Path expansions are made the same
-	way as for `core.excludesfile`.
+	way as for `core.excludesfile`. Its default value is
+	$XDG_CONFIG_HOME/git/attributes. If $XDG_CONFIG_HOME is either not
+	set or empty, $HOME/.config will be used.
 
 core.editor::
 	Commands such as `commit` and `tag` that lets you edit
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 80120ea..3339eef 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -75,6 +75,8 @@ repositories (i.e., attributes of interest to all users) should go into
 `.gitattributes` files. Attributes that should affect all repositories
 for a single user should be placed in a file specified by the
 `core.attributesfile` configuration option (see linkgit:git-config[1]).
+Its default value is $XDG_CONFIG_HOME/git/attributes. If $XDG_CONFIG_HOME
+is either not set or empty, $HOME/.config will be used.
 Attributes for all users on a system should be placed in the
 `$(prefix)/etc/gitattributes` file.
 
diff --git a/attr.c b/attr.c
index 303751f..aef93d8 100644
--- a/attr.c
+++ b/attr.c
@@ -497,6 +497,7 @@ static int git_attr_system(void)
 static void bootstrap_attr_stack(void)
 {
 	struct attr_stack *elem;
+	char *xdg_attributes_file;
 
 	if (attr_stack)
 		return;
@@ -515,13 +516,15 @@ static void bootstrap_attr_stack(void)
 		}
 	}
 
-	if (git_attributes_file) {
-		elem = read_attr_from_file(git_attributes_file, 1);
-		if (elem) {
-			elem->origin = NULL;
-			elem->prev = attr_stack;
-			attr_stack = elem;
-		}
+	if (!git_attributes_file) {
+		home_config_paths(NULL, &xdg_attributes_file, "attributes");
+		git_attributes_file = xdg_attributes_file;
+	}
+	elem = read_attr_from_file(git_attributes_file, 1);
+	if (elem) {
+		elem->origin = NULL;
+		elem->prev = attr_stack;
+		attr_stack = elem;
 	}
 
 	if (!is_bare_repository() || direction == GIT_ATTR_INDEX) {
diff --git a/t/t1306-xdg-files.sh b/t/t1306-xdg-files.sh
index 1a1f07e..80621ed 100755
--- a/t/t1306-xdg-files.sh
+++ b/t/t1306-xdg-files.sh
@@ -84,4 +84,34 @@ test_expect_success 'Exclusion in a non-XDG global ignore file' '
 	test_must_fail git add to_be_excluded
 '
 
+test_expect_success 'Checking attributes in the XDG attributes file' '
+	git init git &&
+	cd git &&
+	echo foo >f &&
+	git check-attr -a f >actual &&
+	test_line_count -eq 0 actual &&
+	mkdir -p "$HOME"/.config/git/ &&
+	echo "f attr_f" >"$HOME"/.config/git/attributes &&
+	echo "f: attr_f: set" >expected &&
+	git check-attr -a f >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'Checking attributes in both XDG and local ignore files' '
+	echo "f -attr_f" >.gitattributes &&
+	echo "f: attr_f: unset" >expected &&
+	git check-attr -a f >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'Checking attributes in a non-XDG global attributes file' '
+	rm .gitattributes &&
+	echo "f attr_f=test" >"$HOME"/my_gitattributes &&
+	git config core.attributesfile "$HOME"/my_gitattributes &&
+	test_when_finished "cd \"$HOME\" && rm -rf git && rm -r .config" &&
+	echo "f: attr_f: test" >expected &&
+	git check-attr -a f >actual &&
+	test_cmp expected actual
+'
+
 test_done
-- 
1.7.8
Huynh Khoi Nguyen NGUYEN· Jun 3, 2012, 20:14 UTC · re: Huynh Khoi Nguyen NGUYEN · lore

[PATCHv5 4/4] Write to $XDG_CONFIG_HOME/git/config file

From: NGUYEN Huynh Khoi Nguyen <nguyenhu@ensibm.imag.fr>
Git will be able to write to $XDG_CONFIG_HOME/git/config, a new
configuration file following XDG specification, if:
- this file already exists, and
- $HOME/.gitconfig file doesn't, and
- global option is used.
Otherwise Git writes to $HOME/.gitconfig with global option, as usual.
If you don't create $XDG_CONFIG_HOME/git/config, there is ABSOLUTELY
NO change. Users can use this new file ONLY if they want.
If $XDG_CONFIG_HOME is either not set or empty, $HOME/.config will be
used.
Advice for users who often come back to an old version of GIT:
you shouldn't create this file.
Signed-off-by: Huynh Khoi Nguyen NGUYEN <Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr>
Signed-off-by: Valentin DUPERRAY <Valentin.DUPERRAY@ensimag.imag.fr>
Signed-off-by: Franck JONAS <Franck.JONAS@ensimag.imag.fr>
Signed-off-by: Thomas NGUY <Thomas.NGUY@ensimag.imag.fr>
Signed-off-by: Lucien KONG <Lucien.KONG@ensimag.imag.fr>
Signed-off-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
---
 Documentation/git-config.txt |    3 ++-
 builtin/config.c             |    5 +----
 t/t1306-xdg-files.sh         |   18 ++++++++++++++++++
 3 files changed, 21 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 7e344a2..a2b4786 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -97,7 +97,8 @@ OPTIONS
 
 --global::
 	For writing options: write to global ~/.gitconfig file rather than
-	the repository .git/config.
+	the repository .git/config, write to $XDG_CONFIG_HOME/git/config file
+	if this file exists and ~/.gitconfig file doesn't.
 +
 For reading options: read only from global ~/.gitconfig and from
 $XDG_CONFIG_HOME/git/config rather than from all available files.
diff --git a/builtin/config.c b/builtin/config.c
index da54fd1..e8e1c0a 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -387,10 +387,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 
 		home_config_paths(&user_config, &xdg_config, "config");
 
-		if (access(user_config, R_OK) && !access(xdg_config, R_OK) &&
-		    (actions == ACTION_LIST ||
-		     actions == ACTION_GET_COLOR ||
-		     actions == ACTION_GET_COLORBOOL))
+		if (access(user_config, R_OK) && !access(xdg_config, R_OK))
 			given_config_file = xdg_config;
 		else if (user_config)
 			given_config_file = user_config;
diff --git a/t/t1306-xdg-files.sh b/t/t1306-xdg-files.sh
index 80621ed..1ff709d 100755
--- a/t/t1306-xdg-files.sh
+++ b/t/t1306-xdg-files.sh
@@ -114,4 +114,22 @@ test_expect_success 'Checking attributes in a non-XDG global attributes file' '
 	test_cmp expected actual
 '
 
+test_expect_success 'write: xdg file exists and ~/.gitconfig doesn'\''t' '
+	mkdir -p .config/git &&
+	>.config/git/config &&
+	rm .gitconfig &&
+	git config --global user.name "write_config" &&
+	echo "[user]" >expected &&
+	echo "	name = write_config" >>expected &&
+	test_cmp expected .config/git/config
+'
+
+test_expect_success 'write: xdg file exists and ~/.gitconfig exists' '
+	>.gitconfig &&
+	git config --global user.name "write_gitconfig" &&
+	echo "[user]" >expected &&
+	echo "	name = write_gitconfig" >>expected &&
+	test_cmp expected .gitconfig
+'
+
 test_done
-- 
1.7.8
Matthieu Moy· Jun 4, 2012, 21:17 UTC · re: Huynh Khoi Nguyen NGUYEN · lore

Re: [PATCHv5 4/4] Write to $XDG_CONFIG_HOME/git/config file

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

Show 9 quoted lines
> +test_expect_success 'write: xdg file exists and ~/.gitconfig doesn'\''t' '
> +	mkdir -p .config/git &&
> +	>.config/git/config &&
> +	rm .gitconfig &&
> +	git config --global user.name "write_config" &&
> +	echo "[user]" >expected &&
> +	echo "	name = write_config" >>expected &&
> +	test_cmp expected .config/git/config
> +'

It was suggested that you also write to the XDG file if the XDG _directory_ exists. After thinking about it, I tend to disagree with this, because creating ~/.config/git/ignore would let Git write to the XDG configuration file as a side effect, which I find strange.

Anyway, you may add the case where ~/.config/git/ exists but not the config file in it to your tests, to materialize the decision you have taken on that point.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
nguyenhu@minatec.inpg.fr· Jun 5, 2012, 13:04 UTC · re: Matthieu Moy · lore

Re: [PATCHv5 4/4] Write to $XDG_CONFIG_HOME/git/config file

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> a écrit :
Show 25 quoted lines
> Huynh Khoi Nguyen NGUYEN <Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr>
> writes:
>
>> +test_expect_success 'write: xdg file exists and ~/.gitconfig doesn'\''t' '
>> +	mkdir -p .config/git &&
>> +	>.config/git/config &&
>> +	rm .gitconfig &&
>> +	git config --global user.name "write_config" &&
>> +	echo "[user]" >expected &&
>> +	echo "	name = write_config" >>expected &&
>> +	test_cmp expected .config/git/config
>> +'
>
> It was suggested that you also write to the XDG file if the XDG
> _directory_ exists. After thinking about it, I tend to disagree with
> this, because creating ~/.config/git/ignore would let Git write to the
> XDG configuration file as a side effect, which I find strange.
>
> Anyway, you may add the case where ~/.config/git/ exists but not the
> config file in it to your tests, to materialize the decision you have
> taken on that point.
>
> --
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/
I agree. This case will be added in our next v6 version.
Huynh Khoi Nguyen NGUYEN· Jun 6, 2012, 13:21 UTC · re: Huynh Khoi Nguyen NGUYEN · lore

[PATCHv6 1/4] Read (but not write) from $XDG_CONFIG_HOME/git/config file

From: NGUYEN Huynh Khoi Nguyen <nguyenhu@ensibm.imag.fr>

Git will be able to read from $XDG_CONFIG_HOME/git/config, a new configuration file following XDG specification. In the order of reading, this file is between global configuration file and system wide configuration file. Git currently does not write to this new configuration file. If $XDG_CONFIG_HOME is either not set or empty, $HOME/.config/git/config will be used.

Signed-off-by: Huynh Khoi Nguyen NGUYEN <Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr>
Signed-off-by: Valentin Duperray <Valentin.Duperray@ensimag.imag.fr>
Signed-off-by: Franck Jonas <Franck.Jonas@ensimag.imag.fr>
Signed-off-by: Lucien Kong <Lucien.Kong@ensimag.imag.fr>
Signed-off-by: Thomas Nguy <Thomas.Nguy@ensimag.imag.fr>
Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
Signed-off-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
---
 Documentation/git-config.txt |   12 +++++--
 builtin/config.c             |   28 +++++++++++-----
 cache.h                      |    3 ++
 config.c                     |   23 ++++++++-----
 path.c                       |   40 ++++++++++++++++++++++++
 t/t1306-xdg-files.sh         |   70 ++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 155 insertions(+), 21 deletions(-)
 create mode 100755 t/t1306-xdg-files.sh
diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index d9463cb..7e344a2 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -99,8 +99,8 @@ OPTIONS
 	For writing options: write to global ~/.gitconfig file rather than
 	the repository .git/config.
 +
-For reading options: read only from global ~/.gitconfig rather than
-from all available files.
+For reading options: read only from global ~/.gitconfig and from
+$XDG_CONFIG_HOME/git/config rather than from all available files.
 +
 See also <<FILES>>.
 
@@ -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,12 @@ $GIT_DIR/config::
 	User-specific configuration file. Also called "global"
 	configuration file.
 
+$XDG_CONFIG_HOME/git/config::
+	Second user-specific configuration file. If $XDG_CONFIG_HOME is not set
+	or empty, $HOME/.config/git/config will be used. Any single-valued
+	variable set in this file will be overwritten by whatever is in
+	~/.gitconfig.
+
 $(prefix)/etc/gitconfig::
 	System-wide configuration file.
 
diff --git a/builtin/config.c b/builtin/config.c
index 33c8820..da54fd1 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 *global = NULL, *xdg = NULL, *repo_config = NULL;
 	const char *system_wide = NULL, *local;
 	struct config_include_data inc = CONFIG_INCLUDE_INIT;
 	config_fn_t fn;
@@ -169,12 +169,10 @@ static int get_value(const char *key_, const char *regex_)
 
 	local = given_config_file;
 	if (!local) {
-		const char *home = getenv("HOME");
 		local = repo_config = git_pathdup("config");
-		if (home)
-			global = xstrdup(mkpath("%s/.gitconfig", home));
 		if (git_config_system())
 			system_wide = git_etc_gitconfig();
+		home_config_paths(&global, &xdg, "config");
 	}
 
 	if (use_key_regexp) {
@@ -229,6 +227,8 @@ static int get_value(const char *key_, const char *regex_)
 
 	if (do_all && system_wide)
 		git_config_from_file(fn, system_wide, data);
+	if (do_all && xdg)
+		git_config_from_file(fn, xdg, data);
 	if (do_all && global)
 		git_config_from_file(fn, global, data);
 	if (do_all)
@@ -238,6 +238,8 @@ static int get_value(const char *key_, const char *regex_)
 		git_config_from_file(fn, local, data);
 	if (!do_all && !seen && global)
 		git_config_from_file(fn, global, data);
+	if (!do_all && !seen && xdg)
+		git_config_from_file(fn, xdg, data);
 	if (!do_all && !seen && system_wide)
 		git_config_from_file(fn, system_wide, data);
 
@@ -255,6 +257,7 @@ static int get_value(const char *key_, const char *regex_)
 free_strings:
 	free(repo_config);
 	free(global);
+	free(xdg);
 	return ret;
 }
 
@@ -379,13 +382,20 @@ 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 = NULL;
+		char *xdg_config = NULL;
+
+		home_config_paths(&user_config, &xdg_config, "config");
+
+		if (access(user_config, R_OK) && !access(xdg_config, R_OK) &&
+		    (actions == ACTION_LIST ||
+		     actions == ACTION_GET_COLOR ||
+		     actions == ACTION_GET_COLORBOOL))
+			given_config_file = xdg_config;
+		else if (user_config)
 			given_config_file = user_config;
-		} else {
+		else
 			die("$HOME not set");
-		}
 	}
 	else if (use_system_config)
 		given_config_file = git_etc_gitconfig();
diff --git a/cache.h b/cache.h
index 06413e1..fbba2d6 100644
--- a/cache.h
+++ b/cache.h
@@ -619,6 +619,8 @@ extern char *git_snpath(char *buf, size_t n, const char *fmt, ...)
 	__attribute__((format (printf, 3, 4)));
 extern char *git_pathdup(const char *fmt, ...)
 	__attribute__((format (printf, 1, 2)));
+extern char *mkpathdup(const char *fmt, ...)
+	__attribute__((format (printf, 1, 2)));
 
 /* Return a statically allocated filename matching the sha1 signature */
 extern char *mkpath(const char *fmt, ...) __attribute__((format (printf, 1, 2)));
@@ -708,6 +710,7 @@ int set_shared_perm(const char *path, int mode);
 int safe_create_leading_directories(char *path);
 int safe_create_leading_directories_const(const char *path);
 int mkdir_in_gitdir(const char *path);
+extern void home_config_paths(char **global, char **xdg, char *file);
 extern char *expand_user_path(const char *path);
 const char *enter_repo(const char *path, int strict);
 static inline int is_absolute_path(const char *path)
diff --git a/config.c b/config.c
index 71ef171..d28a499 100644
--- a/config.c
+++ b/config.c
@@ -929,7 +929,10 @@ int git_config_system(void)
 int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 {
 	int ret = 0, found = 0;
-	const char *home = NULL;
+	char *xdg_config = NULL;
+	char *user_config = NULL;
+
+	home_config_paths(&user_config, &xdg_config, "config");
 
 	if (git_config_system() && !access(git_etc_gitconfig(), R_OK)) {
 		ret += git_config_from_file(fn, git_etc_gitconfig(),
@@ -937,14 +940,14 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 		found += 1;
 	}
 
-	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);
-			found += 1;
-		}
+	if (!access(xdg_config, R_OK)) {
+		ret += git_config_from_file(fn, xdg_config, data);
+		found += 1;
+	}
+
+	if (!access(user_config, R_OK)) {
+		ret += git_config_from_file(fn, user_config, data);
+		found += 1;
 	}
 
 	if (repo_config && !access(repo_config, R_OK)) {
@@ -963,6 +966,8 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 		break;
 	}
 
+	free(xdg_config);
+	free(user_config);
 	return ret == 0 ? found : ret;
 }
 
diff --git a/path.c b/path.c
index 6f2aa69..ca29bdd 100644
--- a/path.c
+++ b/path.c
@@ -87,6 +87,20 @@ char *git_pathdup(const char *fmt, ...)
 	return xstrdup(path);
 }
 
+char *mkpathdup(const char *fmt, ...)
+{
+	char path[PATH_MAX];
+	va_list args;
+	unsigned len;
+
+	va_start(args, fmt);
+	len = vsnprintf(path, sizeof(path), fmt, args);
+	va_end(args);
+	if (len >= sizeof(path))
+		return xstrdup(bad_path);
+	return xstrdup(cleanup_path(path));
+}
+
 char *mkpath(const char *fmt, ...)
 {
 	va_list args;
@@ -122,6 +136,32 @@ char *git_path(const char *fmt, ...)
 	return cleanup_path(pathname);
 }
 
+void home_config_paths(char **global, char **xdg, char *file)
+{
+	char *xdg_home = getenv("XDG_CONFIG_HOME");
+	char *home = getenv("HOME");
+	char *to_free = NULL;
+
+	if (!home) {
+		if (global)
+			*global = NULL;
+	} else {
+		if (!xdg_home) {
+			to_free = mkpathdup("%s/.config", home);
+			xdg_home = to_free;
+		}
+		if (global)
+			*global = mkpathdup("%s/.gitconfig", home);
+	}
+
+	if (!xdg_home)
+		*xdg = NULL;
+	else
+		*xdg = mkpathdup("%s/git/%s", xdg_home, file);
+
+	free(to_free);
+}
+
 char *git_path_submodule(const char *path, const char *fmt, ...)
 {
 	char *pathname = get_pathname();
diff --git a/t/t1306-xdg-files.sh b/t/t1306-xdg-files.sh
new file mode 100755
index 0000000..5b971d9
--- /dev/null
+++ b/t/t1306-xdg-files.sh
@@ -0,0 +1,70 @@
+#!/bin/sh
+#
+# Copyright (c) 2012 Valentin Duperray, Lucien Kong, Franck Jonas,
+#		     Thomas Nguy, Khoi Nguyen
+#		     Grenoble INP Ensimag
+#
+
+test_description='Compatibility with $XDG_CONFIG_HOME/git/ files'
+
+. ./test-lib.sh
+
+test_expect_success 'read config: xdg file exists and ~/.gitconfig doesn'\''t' '
+	mkdir -p .config/git &&
+	echo "[alias]" >.config/git/config &&
+	echo "	myalias = !echo in_config" >>.config/git/config &&
+	echo in_config >expected &&
+	git myalias >actual &&
+	test_cmp expected actual
+'
+
+
+test_expect_success 'read config: xdg file exists and ~/.gitconfig exists' '
+	>.gitconfig &&
+	echo "[alias]" >.gitconfig &&
+	echo "	myalias = !echo in_gitconfig" >>.gitconfig &&
+	echo in_gitconfig >expected &&
+	git myalias >actual &&
+	test_cmp expected actual
+'
+
+
+test_expect_success 'read with --get: xdg file exists and ~/.gitconfig doesn'\''t' '
+	rm .gitconfig &&
+	echo "[user]" >.config/git/config &&
+	echo "	name = read_config" >>.config/git/config &&
+	echo read_config >expected &&
+	git config --get user.name >actual &&
+	test_cmp expected actual
+'
+
+
+test_expect_success 'read with --get: xdg file exists and ~/.gitconfig exists' '
+	>.gitconfig &&
+	echo "[user]" >.gitconfig &&
+	echo "	name = read_gitconfig" >>.gitconfig &&
+	echo read_gitconfig >expected &&
+	git config --get user.name >actual &&
+	test_cmp expected actual
+'
+
+
+test_expect_success 'read with --list: xdg file exists and ~/.gitconfig doesn'\''t' '
+	rm .gitconfig &&
+	echo user.name=read_config >expected &&
+	git config --global --list >actual &&
+	test_cmp expected actual
+'
+
+
+test_expect_success 'read with --list: xdg file exists and ~/.gitconfig exists' '
+	>.gitconfig &&
+	echo "[user]" >.gitconfig &&
+	echo "	name = read_gitconfig" >>.gitconfig &&
+	echo user.name=read_gitconfig >expected &&
+	git config --global --list >actual &&
+	test_cmp expected actual
+'
+
+
+test_done
-- 
1.7.8
Huynh Khoi Nguyen NGUYEN· Jun 6, 2012, 13:21 UTC · re: Huynh Khoi Nguyen NGUYEN · lore

[PATCHv6 2/4] Let core.excludesfile default to $XDG_CONFIG_HOME/git/ignore

From: NGUYEN Huynh Khoi Nguyen <nguyenhu@ensibm.imag.fr>

If core.excludesfile is not defined, its default value will be $XDG_CONFIG_HOME/git/ignore in order to follow XDG specification. If $XDG_CONFIG_HOME is either not set or emty, $HOME/.config/git/ignore will be used.

Signed-off-by: Huynh Khoi Nguyen NGUYEN <Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr>
Signed-off-by: Valentin Duperray <Valentin.Duperray@ensimag.imag.fr>
Signed-off-by: Franck Jonas <Franck.Jonas@ensimag.imag.fr>
Signed-off-by: Lucien Kong <Lucien.Kong@ensimag.imag.fr>
Signed-off-by: Thomas Nguy <Thomas.Nguy@ensimag.imag.fr>
Signed-off-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
---
 Documentation/config.txt    |    4 +++-
 Documentation/gitignore.txt |    4 +++-
 dir.c                       |    7 ++++++-
 t/t1306-xdg-files.sh        |   40 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 52 insertions(+), 3 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 915cb5a..509bf25 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -483,7 +483,9 @@ core.excludesfile::
 	'.git/info/exclude', git looks into this file for patterns
 	of files which are not meant to be tracked.  "`~/`" is expanded
 	to the value of `$HOME` and "`~user/`" to the specified user's
-	home directory.  See linkgit:gitignore[5].
+	home directory. Its default value is $XDG_CONFIG_HOME/git/ignore.
+	If $XDG_CONFIG_HOME is either not set or empty, $HOME/.config/git/ignore
+	will be used. See linkgit:gitignore[5].
 
 core.askpass::
 	Some commands (e.g. svn and http interfaces) that interactively
diff --git a/Documentation/gitignore.txt b/Documentation/gitignore.txt
index 2e7328b..a5c9b93 100644
--- a/Documentation/gitignore.txt
+++ b/Documentation/gitignore.txt
@@ -50,7 +50,9 @@ the repository but are specific to one user's workflow) should go into
 the `$GIT_DIR/info/exclude` file.  Patterns which a user wants git to
 ignore in all situations (e.g., backup or temporary files generated by
 the user's editor of choice) generally go into a file specified by
-`core.excludesfile` in the user's `~/.gitconfig`.
+`core.excludesfile` in the user's `~/.gitconfig`. Its default value is
+$XDG_CONFIG_HOME/git/ignore. If $XDG_CONFIG_HOME is either not set or empty,
+$HOME/.config/git/ignore will be used.
 
 The underlying git plumbing tools, such as
 'git ls-files' and 'git read-tree', read
diff --git a/dir.c b/dir.c
index ed1510f..8c6f47f 100644
--- a/dir.c
+++ b/dir.c
@@ -1234,12 +1234,17 @@ int remove_dir_recursively(struct strbuf *path, int flag)
 void setup_standard_excludes(struct dir_struct *dir)
 {
 	const char *path;
+	char *xdg_path;
 
 	dir->exclude_per_dir = ".gitignore";
 	path = git_path("info/exclude");
+	if (!excludes_file) {
+		home_config_paths(NULL, &xdg_path, "ignore");
+		excludes_file = xdg_path;
+	}
 	if (!access(path, R_OK))
 		add_excludes_from_file(dir, path);
-	if (excludes_file && !access(excludes_file, R_OK))
+	if (!access(excludes_file, R_OK))
 		add_excludes_from_file(dir, excludes_file);
 }
 
diff --git a/t/t1306-xdg-files.sh b/t/t1306-xdg-files.sh
index 5b971d9..873bbcf 100755
--- a/t/t1306-xdg-files.sh
+++ b/t/t1306-xdg-files.sh
@@ -67,4 +67,44 @@ test_expect_success 'read with --list: xdg file exists and ~/.gitconfig exists'
 '
 
 
+test_expect_success 'Exclusion of a file in the XDG ignore file' '
+	test_when_finished "rm -rf git" &&
+	git init git &&
+	(
+		cd git &&
+		echo foo >to_be_excluded &&
+		mkdir -p "$HOME"/.config/git/ &&
+		echo to_be_excluded >"$HOME"/.config/git/ignore &&
+		test_must_fail git add to_be_excluded
+	)
+'
+
+
+test_expect_success 'Exclusion in both XDG and local ignore files' '
+	test_when_finished "rm -rf git" &&
+	git init git &&
+	(
+		cd git &&
+		echo foo >to_be_excluded &&
+		echo to_be_excluded >"$HOME"/.config/git/ignore &&
+		echo to_be_excluded >.gitignore &&
+		test_must_fail git add to_be_excluded
+	)
+'
+
+
+test_expect_success 'Exclusion in a non-XDG global ignore file' '
+	test_when_finished "rm -rf git" &&
+	git init git &&
+	(
+		cd git &&
+		echo foo >to_be_excluded &&
+		echo >"$HOME"/.config/git/ignore &&
+		echo to_be_excluded >"$HOME"/my_gitignore &&
+		git config core.excludesfile "$HOME"/my_gitignore &&
+		test_must_fail git add to_be_excluded
+	)
+'
+
+
 test_done
-- 
1.7.8
Junio C Hamano· Jun 7, 2012, 23:31 UTC · re: Huynh Khoi Nguyen NGUYEN · lore

Re: [PATCHv6 2/4] Let core.excludesfile default to $XDG_CONFIG_HOME/git/ignore

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

> From: NGUYEN Huynh Khoi Nguyen <nguyenhu@ensibm.imag.fr>
>
> If core.excludesfile is not defined, its default value will be
> $XDG_CONFIG_HOME/git/ignore in order to follow XDG specification.

I find the above statement hard to understand and harder to agree with.

Right now, when core.excludesfile is not defined, the user does not have to worry about any "global" ignore file to get in the way while working on a specific project. With this change, if the user does not want random paths in the project that happens to match a pattern in this XDG_CONFIG_HOME/git/ignore file that is totally outside of the project's control, the user needs to make sure core.excludesfile is set to an empty file or something.

That sounds like a huge downside; does XDG specification mandates such a regression? Do they say "you must treat XDG_CONFIG_HOME/git/ignore as your global ignore file"? It is hard to believe.

Perhaps you are doing two or more unrelated things and stating what you are doing poorly, which is what is confusing me?

Perhaps the problem you are solving is something like this (by the way, you didn't state what problem you are trying to solve at all):

 - Use of core.excludesfile is an opt-in feature to keep ignore
   patterns in a file and use it across projects defined per user.
   To use this feature, the user needs to create such a file, and
   add configuration variable to point at it.  The feature needs two
   user actions to enable.
 - Instead, we can define a filename that is unlikely to already
   exist on the system, and use the presense of the file as a cue
   that the user wants to use that feature.  The user can simply
   create the file without adding configuration variable to point at
   it. Now the feature needs only one user action to enable, which
   is the primary problem I wanted to solve.
 - Now we need to decide what that magic filename should be.  We
   could pick any random filename that user can create, and no other
   user can create for the user, which means that it has to be under
   $HOME.  Let's use "${XDG_CONFIG_HOME:-$HOME/.config/git}/ignore";
   it is as good as any random filename in $HOME/ and follows the
   XDG guideline.
But that may not be what you meant to say.  I dunno.
Matthieu Moy· Jun 8, 2012, 08:47 UTC · re: Junio C Hamano · lore

Re: [PATCHv6 2/4] Let core.excludesfile default to $XDG_CONFIG_HOME/git/ignore

Junio C Hamano <gitster@pobox.com> writes:
> Perhaps the problem you are solving is something like this (by the
> way, you didn't state what problem you are trying to solve at all):
> [...]

That is a very good summary of what the patch tries to do. Following XDG is a nice side effect, but not the primary goal.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
nguyenhu@minatec.inpg.fr· Jun 8, 2012, 09:02 UTC · re: Junio C Hamano · lore

Re: [PATCHv6 2/4] Let core.excludesfile default to $XDG_CONFIG_HOME/git/ignore

Junio C Hamano <gitster@pobox.com> a écrit :
> But that may not be what you meant to say.  I dunno.

It is exactly what we want to do. It is true the statement was very short and we didn't state what problem we are trying to solve. Your points raise the issues and explain clearly our solution.

Huynh Khoi Nguyen NGUYEN· Jun 6, 2012, 13:21 UTC · re: Huynh Khoi Nguyen NGUYEN · lore

[PATCHv6 3/4] Let core.attributesfile default to $XDG_CONFIG_HOME/git/attributes

From: NGUYEN Huynh Khoi Nguyen <nguyenhu@ensibm.imag.fr>

If core.attributesfile is not defined, its default value will be $XDG_CONFIG_HOME/git/attributes in order to follow XDG specification. If $XDG_CONFIG_HOME is either not set or emty, $HOME/.config/git/attributes will be used.

Signed-off-by: Huynh Khoi Nguyen NGUYEN <Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr>
Signed-off-by: Valentin Duperray <Valentin.Duperray@ensimag.imag.fr>
Signed-off-by: Franck Jonas <Franck.Jonas@ensimag.imag.fr>
Signed-off-by: Lucien Kong <Lucien.Kong@ensimag.imag.fr>
Signed-off-by: Thomas Nguy <Thomas.Nguy@ensimag.imag.fr>
Signed-off-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
---
 Documentation/config.txt        |    4 ++-
 Documentation/gitattributes.txt |    2 +
 attr.c                          |   17 ++++++++-----
 t/t1306-xdg-files.sh            |   47 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 62 insertions(+), 8 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 509bf25..f691d8e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -500,7 +500,9 @@ core.attributesfile::
 	In addition to '.gitattributes' (per-directory) and
 	'.git/info/attributes', git looks into this file for attributes
 	(see linkgit:gitattributes[5]). Path expansions are made the same
-	way as for `core.excludesfile`.
+	way as for `core.excludesfile`. Its default value is
+	$XDG_CONFIG_HOME/git/attributes. If $XDG_CONFIG_HOME is either not
+	set or empty, $HOME/.config/git/attributes will be used.
 
 core.editor::
 	Commands such as `commit` and `tag` that lets you edit
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 80120ea..484c614 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -75,6 +75,8 @@ repositories (i.e., attributes of interest to all users) should go into
 `.gitattributes` files. Attributes that should affect all repositories
 for a single user should be placed in a file specified by the
 `core.attributesfile` configuration option (see linkgit:git-config[1]).
+Its default value is $XDG_CONFIG_HOME/git/attributes. If $XDG_CONFIG_HOME
+is either not set or empty, $HOME/.config/git/attributes will be used.
 Attributes for all users on a system should be placed in the
 `$(prefix)/etc/gitattributes` file.
 
diff --git a/attr.c b/attr.c
index 303751f..aef93d8 100644
--- a/attr.c
+++ b/attr.c
@@ -497,6 +497,7 @@ static int git_attr_system(void)
 static void bootstrap_attr_stack(void)
 {
 	struct attr_stack *elem;
+	char *xdg_attributes_file;
 
 	if (attr_stack)
 		return;
@@ -515,13 +516,15 @@ static void bootstrap_attr_stack(void)
 		}
 	}
 
-	if (git_attributes_file) {
-		elem = read_attr_from_file(git_attributes_file, 1);
-		if (elem) {
-			elem->origin = NULL;
-			elem->prev = attr_stack;
-			attr_stack = elem;
-		}
+	if (!git_attributes_file) {
+		home_config_paths(NULL, &xdg_attributes_file, "attributes");
+		git_attributes_file = xdg_attributes_file;
+	}
+	elem = read_attr_from_file(git_attributes_file, 1);
+	if (elem) {
+		elem->origin = NULL;
+		elem->prev = attr_stack;
+		attr_stack = elem;
 	}
 
 	if (!is_bare_repository() || direction == GIT_ATTR_INDEX) {
diff --git a/t/t1306-xdg-files.sh b/t/t1306-xdg-files.sh
index 873bbcf..eb0f40b 100755
--- a/t/t1306-xdg-files.sh
+++ b/t/t1306-xdg-files.sh
@@ -107,4 +107,51 @@ test_expect_success 'Exclusion in a non-XDG global ignore file' '
 '
 
 
+test_expect_success 'Checking attributes in the XDG attributes file' '
+	test_when_finished "rm -rf git" &&
+	git init git &&
+	(
+		cd git &&
+		echo foo >f &&
+		git check-attr -a f >actual &&
+		test_line_count -eq 0 actual &&
+		echo "f attr_f" >"$HOME"/.config/git/attributes &&
+		echo "f: attr_f: set" >expected &&
+		git check-attr -a f >actual &&
+		test_cmp expected actual
+	)
+'
+
+
+test_expect_success 'Checking attributes in both XDG and local ignore files' '
+	test_when_finished "rm -rf git" &&
+	git init git &&
+	(
+		cd git &&
+		echo foo >f &&
+		echo "f attr_f" >"$HOME"/.config/git/attributes &&
+		echo "f -attr_f" >.gitattributes &&
+		echo "f: attr_f: unset" >expected &&
+		git check-attr -a f >actual &&
+		test_cmp expected actual
+	)
+'
+
+
+test_expect_success 'Checking attributes in a non-XDG global attributes file' '
+	test_when_finished "rm -rf git" &&
+	git init git &&
+	(
+		cd git &&
+		echo foo >f &&
+		echo "f attr_f" >"$HOME"/.config/git/attributes &&
+		echo "f attr_f=test" >"$HOME"/my_gitattributes &&
+		git config core.attributesfile "$HOME"/my_gitattributes &&
+		echo "f: attr_f: test" >expected &&
+		git check-attr -a f >actual &&
+		test_cmp expected actual
+	)
+'
+
+
 test_done
-- 
1.7.8
Huynh Khoi Nguyen NGUYEN· Jun 6, 2012, 13:21 UTC · re: Huynh Khoi Nguyen NGUYEN · lore

[PATCHv6 4/4] Write to $XDG_CONFIG_HOME/git/config file

From: NGUYEN Huynh Khoi Nguyen <nguyenhu@ensibm.imag.fr>
Git will be able to write to $XDG_CONFIG_HOME/git/config, a new
configuration file following XDG specification, if:
- this file already exists, and
- $HOME/.gitconfig file doesn't, and
- global option is used.
Otherwise Git writes to $HOME/.gitconfig with global option, as usual.
If you don't create $XDG_CONFIG_HOME/git/config, there is ABSOLUTELY
NO change. Users can use this new file ONLY if they want.
If $XDG_CONFIG_HOME is either not set or empty, $HOME/.config/git/config
will be used.
Advice for users who often come back to an old version of GIT:
you shouldn't create this file.
Signed-off-by: Huynh Khoi Nguyen NGUYEN <Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr>
Signed-off-by: Valentin Duperray <Valentin.Duperray@ensimag.imag.fr>
Signed-off-by: Franck Jonas <Franck.Jonas@ensimag.imag.fr>
Signed-off-by: Lucien Kong <Lucien.Kong@ensimag.imag.fr>
Signed-off-by: Thomas Nguy <Thomas.Nguy@ensimag.imag.fr>
Signed-off-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
---
 Documentation/git-config.txt |    3 ++-
 builtin/config.c             |    5 +----
 t/t1306-xdg-files.sh         |   30 ++++++++++++++++++++++++++++++
 3 files changed, 33 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 7e344a2..a2b4786 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -97,7 +97,8 @@ OPTIONS
 
 --global::
 	For writing options: write to global ~/.gitconfig file rather than
-	the repository .git/config.
+	the repository .git/config, write to $XDG_CONFIG_HOME/git/config file
+	if this file exists and ~/.gitconfig file doesn't.
 +
 For reading options: read only from global ~/.gitconfig and from
 $XDG_CONFIG_HOME/git/config rather than from all available files.
diff --git a/builtin/config.c b/builtin/config.c
index da54fd1..e8e1c0a 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -387,10 +387,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 
 		home_config_paths(&user_config, &xdg_config, "config");
 
-		if (access(user_config, R_OK) && !access(xdg_config, R_OK) &&
-		    (actions == ACTION_LIST ||
-		     actions == ACTION_GET_COLOR ||
-		     actions == ACTION_GET_COLORBOOL))
+		if (access(user_config, R_OK) && !access(xdg_config, R_OK))
 			given_config_file = xdg_config;
 		else if (user_config)
 			given_config_file = user_config;
diff --git a/t/t1306-xdg-files.sh b/t/t1306-xdg-files.sh
index eb0f40b..dbf236a 100755
--- a/t/t1306-xdg-files.sh
+++ b/t/t1306-xdg-files.sh
@@ -154,4 +154,34 @@ test_expect_success 'Checking attributes in a non-XDG global attributes file' '
 '
 
 
+test_expect_success 'write: xdg file exists and ~/.gitconfig doesn'\''t' '
+	mkdir -p .config/git &&
+	>.config/git/config &&
+	rm .gitconfig &&
+	git config --global user.name "write_config" &&
+	echo "[user]" >expected &&
+	echo "	name = write_config" >>expected &&
+	test_cmp expected .config/git/config
+'
+
+
+test_expect_success 'write: xdg file exists and ~/.gitconfig exists' '
+	>.gitconfig &&
+	git config --global user.name "write_gitconfig" &&
+	echo "[user]" >expected &&
+	echo "	name = write_gitconfig" >>expected &&
+	test_cmp expected .gitconfig
+'
+
+
+test_expect_success 'write: ~/.config/git/ exists and config file doesn'\''t' '
+	rm .gitconfig &&
+	rm .config/git/config &&
+	git config --global user.name "write_gitconfig" &&
+	echo "[user]" >expected &&
+	echo "	name = write_gitconfig" >>expected &&
+	test_cmp expected .gitconfig
+'
+
+
 test_done
-- 
1.7.8
David Aguilar· Jun 9, 2012, 03:48 UTC · re: Huynh Khoi Nguyen NGUYEN · lore

Re: [PATCHv6 4/4] Write to $XDG_CONFIG_HOME/git/config file

On Wed, Jun 6, 2012 at 6:21 AM, Huynh Khoi Nguyen NGUYEN <Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr> wrote:

Show 22 quoted lines
> From: NGUYEN Huynh Khoi Nguyen <nguyenhu@ensibm.imag.fr>
>
> Git will be able to write to $XDG_CONFIG_HOME/git/config, a new
> configuration file following XDG specification, if:
> - this file already exists, and
> - $HOME/.gitconfig file doesn't, and
> - global option is used.
> Otherwise Git writes to $HOME/.gitconfig with global option, as usual.
> If you don't create $XDG_CONFIG_HOME/git/config, there is ABSOLUTELY
> NO change. Users can use this new file ONLY if they want.
> If $XDG_CONFIG_HOME is either not set or empty, $HOME/.config/git/config
> will be used.
> Advice for users who often come back to an old version of GIT:
> you shouldn't create this file.
>
> Signed-off-by: Huynh Khoi Nguyen NGUYEN <Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr>
> Signed-off-by: Valentin Duperray <Valentin.Duperray@ensimag.imag.fr>
> Signed-off-by: Franck Jonas <Franck.Jonas@ensimag.imag.fr>
> Signed-off-by: Lucien Kong <Lucien.Kong@ensimag.imag.fr>
> Signed-off-by: Thomas Nguy <Thomas.Nguy@ensimag.imag.fr>
> Signed-off-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
> ---
I'm all for this change too.
I did a little research RE: the OSX and Windows question.

glib makes no differentiation between OSX and Linux, but Windows does have its own #ifdef.

http://git.gnome.org/browse/glib/tree/glib/gutils.c#n1251

I certainly don't think this should affect this series, I'm just noting it as something that the Windows folks might care about. Perhaps something for compat/ in the future?

The downside to doing the same as glib is more documentation. The upside is... consistency?

Thanks, David

Show 76 quoted lines
>  Documentation/git-config.txt |    3 ++-
>  builtin/config.c             |    5 +----
>  t/t1306-xdg-files.sh         |   30 ++++++++++++++++++++++++++++++
>  3 files changed, 33 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> index 7e344a2..a2b4786 100644
> --- a/Documentation/git-config.txt
> +++ b/Documentation/git-config.txt
> @@ -97,7 +97,8 @@ OPTIONS
>
>  --global::
>        For writing options: write to global ~/.gitconfig file rather than
> -       the repository .git/config.
> +       the repository .git/config, write to $XDG_CONFIG_HOME/git/config file
> +       if this file exists and ~/.gitconfig file doesn't.
>  +
>  For reading options: read only from global ~/.gitconfig and from
>  $XDG_CONFIG_HOME/git/config rather than from all available files.
> diff --git a/builtin/config.c b/builtin/config.c
> index da54fd1..e8e1c0a 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -387,10 +387,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
>
>                home_config_paths(&user_config, &xdg_config, "config");
>
> -               if (access(user_config, R_OK) && !access(xdg_config, R_OK) &&
> -                   (actions == ACTION_LIST ||
> -                    actions == ACTION_GET_COLOR ||
> -                    actions == ACTION_GET_COLORBOOL))
> +               if (access(user_config, R_OK) && !access(xdg_config, R_OK))
>                        given_config_file = xdg_config;
>                else if (user_config)
>                        given_config_file = user_config;
> diff --git a/t/t1306-xdg-files.sh b/t/t1306-xdg-files.sh
> index eb0f40b..dbf236a 100755
> --- a/t/t1306-xdg-files.sh
> +++ b/t/t1306-xdg-files.sh
> @@ -154,4 +154,34 @@ test_expect_success 'Checking attributes in a non-XDG global attributes file' '
>  '
>
>
> +test_expect_success 'write: xdg file exists and ~/.gitconfig doesn'\''t' '
> +       mkdir -p .config/git &&
> +       >.config/git/config &&
> +       rm .gitconfig &&
> +       git config --global user.name "write_config" &&
> +       echo "[user]" >expected &&
> +       echo "  name = write_config" >>expected &&
> +       test_cmp expected .config/git/config
> +'
> +
> +
> +test_expect_success 'write: xdg file exists and ~/.gitconfig exists' '
> +       >.gitconfig &&
> +       git config --global user.name "write_gitconfig" &&
> +       echo "[user]" >expected &&
> +       echo "  name = write_gitconfig" >>expected &&
> +       test_cmp expected .gitconfig
> +'
> +
> +
> +test_expect_success 'write: ~/.config/git/ exists and config file doesn'\''t' '
> +       rm .gitconfig &&
> +       rm .config/git/config &&
> +       git config --global user.name "write_gitconfig" &&
> +       echo "[user]" >expected &&
> +       echo "  name = write_gitconfig" >>expected &&
> +       test_cmp expected .gitconfig
> +'
> +
> +
>  test_done
> --
> 1.7.8
Junio C Hamano· Jun 9, 2012, 06:19 UTC · re: David Aguilar · lore

Re: [PATCHv6 4/4] Write to $XDG_CONFIG_HOME/git/config file

David Aguilar <davvid@gmail.com> writes:
> I'm all for this change too.

Do you mean just the general direction, or including the implementation?

Show 13 quoted lines
> I did a little research RE: the OSX and Windows question.
>
> glib makes no differentiation between OSX and Linux,
> but Windows does have its own #ifdef.
>
> http://git.gnome.org/browse/glib/tree/glib/gutils.c#n1251
>
> I certainly don't think this should affect this series,
> I'm just noting it as something that the Windows folks might
> care about.  Perhaps something for compat/ in the future?
>
> The downside to doing the same as glib is more documentation.
> The upside is... consistency?

Probably. I think we should follow whatever existing and prevalent practice is, and my gut feeling is that we would end up first doing POSIX only thing in my tree, and msysgit folks will quickly feed me updates to tweak the behaviour to match what of Windows version.

David Aguilar· Jun 9, 2012, 17:25 UTC · re: Junio C Hamano · lore

Re: [PATCHv6 4/4] Write to $XDG_CONFIG_HOME/git/config file

On Fri, Jun 8, 2012 at 11:19 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 6 quoted lines
> David Aguilar <davvid@gmail.com> writes:
>
>> I'm all for this change too.
>
> Do you mean just the general direction, or including the
> implementation?

General direction, including the backwards-compatibility concerns described in the commit message.

Show 18 quoted lines
>> I did a little research RE: the OSX and Windows question.
>>
>> glib makes no differentiation between OSX and Linux,
>> but Windows does have its own #ifdef.
>>
>> http://git.gnome.org/browse/glib/tree/glib/gutils.c#n1251
>>
>> I certainly don't think this should affect this series,
>> I'm just noting it as something that the Windows folks might
>> care about.  Perhaps something for compat/ in the future?
>>
>> The downside to doing the same as glib is more documentation.
>> The upside is... consistency?
>
> Probably.  I think we should follow whatever existing and prevalent
> practice is, and my gut feeling is that we would end up first doing
> POSIX only thing in my tree, and msysgit folks will quickly feed me
> updates to tweak the behaviour to match what of Windows version.

That sounds good to me. Thanks,

-- 
David
Matthieu Moy· Jun 10, 2012, 13:21 UTC · re: Junio C Hamano · lore

Re: [PATCHv6 4/4] Write to $XDG_CONFIG_HOME/git/config file

Junio C Hamano <gitster@pobox.com> writes:
> Probably.  I think we should follow whatever existing and prevalent
> practice is, and my gut feeling is that we would end up first doing
> POSIX only thing in my tree, and msysgit folks will quickly feed me
> updates to tweak the behaviour to match what of Windows version.

I don't really have opinion on what Git should do on windows, but in any case, the current patches only changes the behavior if the XDG file exists, so it may be suboptimal on windows, but shouldn't harm windows users.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
nguyenhu@minatec.inpg.fr· Jun 11, 2012, 23:45 UTC · re: Matthieu Moy · lore

Re: [PATCHv6 4/4] Write to $XDG_CONFIG_HOME/git/config file

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> a écrit :
> I don't really have opinion on what Git should do on windows, but in any
> case, the current patches only changes the behavior if the XDG file
> exists, so it may be suboptimal on windows, but shouldn't harm windows
> users.

Yes, if the XDG file doesn't exist, there is no change, and the behaviour is the same, with Linux, OSX or Windows. But it is true that, although windows users will not have problems with the current patches even if they use the XDG file, it is not opimized for Windows, and it would be nice to optimize the case of Windows. For the time being, it is not the goal of this series of patches. Another patch will certainly add this optimization, I hope.

Junio C Hamano· Jun 7, 2012, 22:58 UTC · re: Huynh Khoi Nguyen NGUYEN · lore

Re: [PATCHv6 1/4] Read (but not write) from $XDG_CONFIG_HOME/git/config file

Show 13 quoted lines
> +char *mkpathdup(const char *fmt, ...)
> +{
> +	char path[PATH_MAX];
> +	va_list args;
> +	unsigned len;
> +
> +	va_start(args, fmt);
> +	len = vsnprintf(path, sizeof(path), fmt, args);
> +	va_end(args);
> +	if (len >= sizeof(path))
> +		return xstrdup(bad_path);
> +	return xstrdup(cleanup_path(path));
> +}

Hrmph. If a new helper is introduced anyway, wouldn't it be a better idea to get rid of the hardcoded PATH_MAX limitation, perhaps using strbuf_vaddf() or something in the implementation of this function?

Show 9 quoted lines
> diff --git a/t/t1306-xdg-files.sh b/t/t1306-xdg-files.sh
> new file mode 100755
> index 0000000..5b971d9
> --- /dev/null
> +++ b/t/t1306-xdg-files.sh
> @@ -0,0 +1,70 @@
> ...
> +test_expect_success 'read with --get: xdg file exists and ~/.gitconfig doesn'\''t' '
> +	rm .gitconfig &&

"rm -f .gitconfig"; always consider the possibility that any part of the previous test could have been omitted or failed.

nguyenhu@minatec.inpg.fr· Jun 8, 2012, 09:57 UTC · re: Junio C Hamano · lore

Re: [PATCHv6 1/4] Read (but not write) from $XDG_CONFIG_HOME/git/config file

Junio C Hamano <gitster@pobox.com> a écrit :
Show 17 quoted lines
>> +char *mkpathdup(const char *fmt, ...)
>> +{
>> +	char path[PATH_MAX];
>> +	va_list args;
>> +	unsigned len;
>> +
>> +	va_start(args, fmt);
>> +	len = vsnprintf(path, sizeof(path), fmt, args);
>> +	va_end(args);
>> +	if (len >= sizeof(path))
>> +		return xstrdup(bad_path);
>> +	return xstrdup(cleanup_path(path));
>> +}
>
> Hrmph. If a new helper is introduced anyway, wouldn't it be a better
> idea to get rid of the hardcoded PATH_MAX limitation, perhaps using
> strbuf_vaddf() or something in the implementation of this function?
Ramsay Jones, what do you think about this ?
Ramsay Jones· Jun 12, 2012, 17:42 UTC · re: nguyenhu@minatec.inpg.fr · lore

Re: [PATCHv6 1/4] Read (but not write) from $XDG_CONFIG_HOME/git/config file

nguyenhu@minatec.inpg.fr wrote:
Show 20 quoted lines
> Junio C Hamano <gitster@pobox.com> a écrit :
> 
>>> +char *mkpathdup(const char *fmt, ...)
>>> +{
>>> +	char path[PATH_MAX];
>>> +	va_list args;
>>> +	unsigned len;
>>> +
>>> +	va_start(args, fmt);
>>> +	len = vsnprintf(path, sizeof(path), fmt, args);
>>> +	va_end(args);
>>> +	if (len >= sizeof(path))
>>> +		return xstrdup(bad_path);
>>> +	return xstrdup(cleanup_path(path));
>>> +}
>> Hrmph. If a new helper is introduced anyway, wouldn't it be a better
>> idea to get rid of the hardcoded PATH_MAX limitation, perhaps using
>> strbuf_vaddf() or something in the implementation of this function?
> 
> Ramsay Jones, what do you think about this ?
I think that I'm sorry for the late reply, I've been away ...  :-D

I noticed your new series (v7 I think) which looked good (as far as the mkpathdup() implementation is concerned) and I don't think it will tickle the cygwin bug. (I haven't actually fetched that mail yet, I've only read it using my ISPs web-mail interface, but will do so soon and test it on cygwin).

Thanks!

ATB, Ramsay Jones

nguyenhu@minatec.inpg.fr· Jun 8, 2012, 12:26 UTC · re: Junio C Hamano · lore

Re: [PATCHv6 1/4] Read (but not write) from $XDG_CONFIG_HOME/git/config file

Junio C Hamano <gitster@pobox.com> a écrit :
Show 17 quoted lines
>> +char *mkpathdup(const char *fmt, ...)
>> +{
>> +	char path[PATH_MAX];
>> +	va_list args;
>> +	unsigned len;
>> +
>> +	va_start(args, fmt);
>> +	len = vsnprintf(path, sizeof(path), fmt, args);
>> +	va_end(args);
>> +	if (len >= sizeof(path))
>> +		return xstrdup(bad_path);
>> +	return xstrdup(cleanup_path(path));
>> +}
>
> Hrmph. If a new helper is introduced anyway, wouldn't it be a better
> idea to get rid of the hardcoded PATH_MAX limitation, perhaps using
> strbuf_vaddf() or something in the implementation of this function?
What about this ?
char *mkpathdup(const char *fmt, ...)
{
	char *path;
	struct strbuf sb = STRBUF_INIT;
	va_list args;
	va_start(args, fmt);
	strbuf_vaddf(&sb, fmt, args);
	va_end(args);
	path = sb.buf;
	strbuf_release(&sb);
	return xstrdup(cleanup_path(path));
}
Erik Faye-Lund· Jun 8, 2012, 12:33 UTC · re: nguyenhu@minatec.inpg.fr · lore

Re: [PATCHv6 1/4] Read (but not write) from $XDG_CONFIG_HOME/git/config file

On Fri, Jun 8, 2012 at 2:26 PM,  <nguyenhu@minatec.inpg.fr> wrote:
Show 41 quoted lines
>
> Junio C Hamano <gitster@pobox.com> a écrit :
>
>>> +char *mkpathdup(const char *fmt, ...)
>>> +{
>>> +       char path[PATH_MAX];
>>> +       va_list args;
>>> +       unsigned len;
>>> +
>>> +       va_start(args, fmt);
>>> +       len = vsnprintf(path, sizeof(path), fmt, args);
>>> +       va_end(args);
>>> +       if (len >= sizeof(path))
>>> +               return xstrdup(bad_path);
>>> +       return xstrdup(cleanup_path(path));
>>> +}
>>
>>
>> Hrmph. If a new helper is introduced anyway, wouldn't it be a better
>> idea to get rid of the hardcoded PATH_MAX limitation, perhaps using
>> strbuf_vaddf() or something in the implementation of this function?
>
>
> What about this ?
>
>
> char *mkpathdup(const char *fmt, ...)
> {
>        char *path;
>        struct strbuf sb = STRBUF_INIT;
>        va_list args;
>
>        va_start(args, fmt);
>        strbuf_vaddf(&sb, fmt, args);
>        va_end(args);
>        path = sb.buf;
>
>        strbuf_release(&sb);
>        return xstrdup(cleanup_path(path));
>
> }

No, strbuf_release(&sb) frees 'sb.buf', causing 'path' to point to unallocated memory. You can fix that by doing something along these lines on top:

 	va_end(args);
-	path = sb.buf;
+	path = xstrdup(cleanup_path(path));
 	strbuf_release(&sb);
-	return xstrdup(cleanup_path(path));
+	return path;
 }
nguyenhu@minatec.inpg.fr· Jun 8, 2012, 12:54 UTC · re: Erik Faye-Lund · lore

Re: [PATCHv6 1/4] Read (but not write) from $XDG_CONFIG_HOME/git/config file

Erik Faye-Lund <kusmabite@gmail.com> a écrit :
Show 28 quoted lines
>> char *mkpathdup(const char *fmt, ...)
>> {
>>        char *path;
>>        struct strbuf sb = STRBUF_INIT;
>>        va_list args;
>>
>>        va_start(args, fmt);
>>        strbuf_vaddf(&sb, fmt, args);
>>        va_end(args);
>>        path = sb.buf;
>>
>>        strbuf_release(&sb);
>>        return xstrdup(cleanup_path(path));
>>
>> }
>
> No, strbuf_release(&sb) frees 'sb.buf', causing 'path' to point to
> unallocated memory. You can fix that by doing something along these
> lines on top:
>
>  	va_end(args);
> -	path = sb.buf;
> +	path = xstrdup(cleanup_path(path));
>
>  	strbuf_release(&sb);
> -	return xstrdup(cleanup_path(path));
> +	return path;
>  }
You are right, but I think you rather mean this, no?
  	va_end(args);
-	path = sb.buf;
+	path = xstrdup(cleanup_path(sb.buf));
  	strbuf_release(&sb);
-	return xstrdup(cleanup_path(path));
+	return path;
  }
Erik Faye-Lund· Jun 8, 2012, 12:57 UTC · re: nguyenhu@minatec.inpg.fr · lore

Re: [PATCHv6 1/4] Read (but not write) from $XDG_CONFIG_HOME/git/config file

On Fri, Jun 8, 2012 at 2:54 PM,  <nguyenhu@minatec.inpg.fr> wrote:
Show 48 quoted lines
>
> Erik Faye-Lund <kusmabite@gmail.com> a écrit :
>
>
>>> char *mkpathdup(const char *fmt, ...)
>>> {
>>>        char *path;
>>>        struct strbuf sb = STRBUF_INIT;
>>>        va_list args;
>>>
>>>        va_start(args, fmt);
>>>        strbuf_vaddf(&sb, fmt, args);
>>>        va_end(args);
>>>        path = sb.buf;
>>>
>>>        strbuf_release(&sb);
>>>        return xstrdup(cleanup_path(path));
>>>
>>> }
>>
>>
>> No, strbuf_release(&sb) frees 'sb.buf', causing 'path' to point to
>> unallocated memory. You can fix that by doing something along these
>> lines on top:
>>
>>        va_end(args);
>> -       path = sb.buf;
>> +       path = xstrdup(cleanup_path(path));
>>
>>        strbuf_release(&sb);
>> -       return xstrdup(cleanup_path(path));
>> +       return path;
>>  }
>
>
> You are right, but I think you rather mean this, no?
>
>
>        va_end(args);
> -       path = sb.buf;
> +       path = xstrdup(cleanup_path(sb.buf));
>
>
>        strbuf_release(&sb);
> -       return xstrdup(cleanup_path(path));
> +       return path;
>  }
>
Yes, sorry for the fat fingers :)
Junio C Hamano· Jun 8, 2012, 15:08 UTC · re: nguyenhu@minatec.inpg.fr · lore

Re: [PATCHv6 1/4] Read (but not write) from $XDG_CONFIG_HOME/git/config file

nguyenhu@minatec.inpg.fr writes:
> What about this ?
What about that ;-)?
Show 14 quoted lines
> char *mkpathdup(const char *fmt, ...)
> {
> 	char *path;
> 	struct strbuf sb = STRBUF_INIT;
> 	va_list args;
>
> 	va_start(args, fmt);
> 	strbuf_vaddf(&sb, fmt, args);
> 	va_end(args);
> 	path = sb.buf;
>
> 	strbuf_release(&sb);
> 	return xstrdup(cleanup_path(path));
> }
Modulo
	path = strbuf_detach(&sb, NULL);
that is more or less what I meant.
nguyenhu@minatec.inpg.fr· Jun 9, 2012, 10:53 UTC · re: Junio C Hamano · lore

Re: [PATCHv6 1/4] Read (but not write) from $XDG_CONFIG_HOME/git/config file

Junio C Hamano <gitster@pobox.com> a écrit :
Show 20 quoted lines
>> char *mkpathdup(const char *fmt, ...)
>> {
>> 	char *path;
>> 	struct strbuf sb = STRBUF_INIT;
>> 	va_list args;
>>
>> 	va_start(args, fmt);
>> 	strbuf_vaddf(&sb, fmt, args);
>> 	va_end(args);
>> 	path = sb.buf;
>>
>> 	strbuf_release(&sb);
>> 	return xstrdup(cleanup_path(path));
>> }
>
> Modulo
>
> 	path = strbuf_detach(&sb, NULL);
>
> that is more or less what I meant.
So now the mkpathdup() function looks like:
char *mkpathdup(const char *fmt, ...)
{
	char *path;
	struct strbuf sb = STRBUF_INIT;
	va_list args;
	va_start(args, fmt);
	strbuf_vaddf(&sb, fmt, args);
	va_end(args);
	path = strbuf_detach(&sb, NULL);
	strbuf_release(&sb);
	return path;
}

This new variation of mkpathdup() function both fix the bug addressed by commit 05bab3ea and avoid the use of bounded buffer.

Junio C Hamano· Jun 10, 2012, 06:41 UTC · re: nguyenhu@minatec.inpg.fr · lore

Re: [PATCHv6 1/4] Read (but not write) from $XDG_CONFIG_HOME/git/config file

nguyenhu@minatec.inpg.fr writes:
Show 25 quoted lines
>> Modulo
>>
>> 	path = strbuf_detach(&sb, NULL);
>>
>> that is more or less what I meant.
>
> So now the mkpathdup() function looks like:
>
> char *mkpathdup(const char *fmt, ...)
> {
> 	char *path;
> 	struct strbuf sb = STRBUF_INIT;
> 	va_list args;
>
> 	va_start(args, fmt);
> 	strbuf_vaddf(&sb, fmt, args);
> 	va_end(args);
> 	path = strbuf_detach(&sb, NULL);
>
> 	strbuf_release(&sb);
> 	return path;
> }
>
> This new variation of mkpathdup() function both fix the bug addressed
> by commit 05bab3ea and avoid the use of bounded buffer.

I didn't mean to suggest removing the call to clean-up-path function. What I meant was that strbuf_detach() is a way to take the ownership of the buffer, so that you do not have to call strbuf_release() on it.

nguyenhu@minatec.inpg.fr· Jun 10, 2012, 13:48 UTC · re: Junio C Hamano · lore

Re: [PATCHv6 1/4] Read (but not write) from $XDG_CONFIG_HOME/git/config file

Junio C Hamano <gitster@pobox.com> a écrit :
Show 19 quoted lines
>> char *mkpathdup(const char *fmt, ...)
>> {
>> 	char *path;
>> 	struct strbuf sb = STRBUF_INIT;
>> 	va_list args;
>>
>> 	va_start(args, fmt);
>> 	strbuf_vaddf(&sb, fmt, args);
>> 	va_end(args);
>> 	path = strbuf_detach(&sb, NULL);
>>
>> 	strbuf_release(&sb);
>> 	return path;
>> }
>
> I didn't mean to suggest removing the call to clean-up-path
> function.  What I meant was that strbuf_detach() is a way to take
> the ownership of the buffer, so that you do not have to call
> strbuf_release() on it.

So with the call to clean-up-path function and without the call to strbuf_release(), mkpathdup() function becomes :

char *mkpathdup(const char *fmt, ...)
{
	struct strbuf sb = STRBUF_INIT;
	va_list args;
	va_start(args, fmt);
	strbuf_vaddf(&sb, fmt, args);
	va_end(args);
	return cleanup_path(strbuf_detach(&sb, NULL));
}
Erik Faye-Lund· Jun 10, 2012, 18:44 UTC · re: nguyenhu@minatec.inpg.fr · lore

Re: [PATCHv6 1/4] Read (but not write) from $XDG_CONFIG_HOME/git/config file

On Sun, Jun 10, 2012 at 3:48 PM,  <nguyenhu@minatec.inpg.fr> wrote:
Show 41 quoted lines
>
> Junio C Hamano <gitster@pobox.com> a écrit :
>
>>> char *mkpathdup(const char *fmt, ...)
>>> {
>>>        char *path;
>>>        struct strbuf sb = STRBUF_INIT;
>>>        va_list args;
>>>
>>>        va_start(args, fmt);
>>>        strbuf_vaddf(&sb, fmt, args);
>>>        va_end(args);
>>>        path = strbuf_detach(&sb, NULL);
>>>
>>>        strbuf_release(&sb);
>>>        return path;
>>> }
>>
>>
>> I didn't mean to suggest removing the call to clean-up-path
>> function.  What I meant was that strbuf_detach() is a way to take
>> the ownership of the buffer, so that you do not have to call
>> strbuf_release() on it.
>
>
> So with the call to clean-up-path function and without the call to
> strbuf_release(), mkpathdup() function becomes :
>
>
> char *mkpathdup(const char *fmt, ...)
> {
>        struct strbuf sb = STRBUF_INIT;
>        va_list args;
>
>        va_start(args, fmt);
>        strbuf_vaddf(&sb, fmt, args);
>        va_end(args);
>
>        return cleanup_path(strbuf_detach(&sb, NULL));
>
> }

The awkward thing about doing this, is that the memory allocated by the strbuf cannot be reclaimed if you go with this. A pointer that has been adjusted (like cleanup_path can do) cannot be successfully fed to free.

nguyenhu@minatec.inpg.fr· Jun 10, 2012, 20:02 UTC · re: Erik Faye-Lund · lore

Re: [PATCHv6 1/4] Read (but not write) from $XDG_CONFIG_HOME/git/config file

Erik Faye-Lund <kusmabite@gmail.com> a écrit :
Show 17 quoted lines
>> char *mkpathdup(const char *fmt, ...)
>> {
>>        struct strbuf sb = STRBUF_INIT;
>>        va_list args;
>>
>>        va_start(args, fmt);
>>        strbuf_vaddf(&sb, fmt, args);
>>        va_end(args);
>>
>>        return cleanup_path(strbuf_detach(&sb, NULL));
>>
>> }
>
> The awkward thing about doing this, is that the memory allocated by
> the strbuf cannot be reclaimed if you go with this. A pointer that has
> been adjusted (like cleanup_path can do) cannot be successfully fed to
> free.

Do you mean that the previous version is preferable in keeping clean-up-path function ?

Erik Faye-Lund· Jun 10, 2012, 20:27 UTC · re: nguyenhu@minatec.inpg.fr · lore

Re: [PATCHv6 1/4] Read (but not write) from $XDG_CONFIG_HOME/git/config file

On Sun, Jun 10, 2012 at 10:02 PM,  <nguyenhu@minatec.inpg.fr> wrote:
Show 26 quoted lines
>
> Erik Faye-Lund <kusmabite@gmail.com> a écrit :
>
>>> char *mkpathdup(const char *fmt, ...)
>>> {
>>>        struct strbuf sb = STRBUF_INIT;
>>>        va_list args;
>>>
>>>        va_start(args, fmt);
>>>        strbuf_vaddf(&sb, fmt, args);
>>>        va_end(args);
>>>
>>>        return cleanup_path(strbuf_detach(&sb, NULL));
>>>
>>> }
>>
>>
>> The awkward thing about doing this, is that the memory allocated by
>> the strbuf cannot be reclaimed if you go with this. A pointer that has
>> been adjusted (like cleanup_path can do) cannot be successfully fed to
>> free.
>
>
> Do you mean that the previous version is preferable in keeping clean-up-path
> function ?
>

No, that wasn't my intention. Since this is only used for a few config files, I don't think leaking the memory is a big deal. But it's probably worth putting a comment in the code about it, to warn potential future users.

Junio C Hamano· Jun 11, 2012, 15:50 UTC · re: Erik Faye-Lund · lore

Re: [PATCHv6 1/4] Read (but not write) from $XDG_CONFIG_HOME/git/config file

Erik Faye-Lund <kusmabite@gmail.com> writes:
> The awkward thing about doing this, is that the memory allocated by
> the strbuf cannot be reclaimed if you go with this. A pointer that has
> been adjusted (like cleanup_path can do) cannot be successfully fed to
> free.
Yeah, I wouldn't recommend doing that.  Either
	path = strbuf_detach(&sb, NULL);
	retval = xstrdup(cleanup_path(path));
        free(path);
        return retval;
or
	path = xstrdup(cleanup_path(sb.buf));
        strbuf_release(&sb);
        return path;
	
would be more sensible.
nguyenhu@minatec.inpg.fr· Jun 11, 2012, 16:53 UTC · re: Junio C Hamano · lore

Re: [PATCHv6 1/4] Read (but not write) from $XDG_CONFIG_HOME/git/config file

Junio C Hamano <gitster@pobox.com> a écrit :
Show 21 quoted lines
> Erik Faye-Lund <kusmabite@gmail.com> writes:
>
>> The awkward thing about doing this, is that the memory allocated by
>> the strbuf cannot be reclaimed if you go with this. A pointer that has
>> been adjusted (like cleanup_path can do) cannot be successfully fed to
>> free.
>
> Yeah, I wouldn't recommend doing that.  Either
>
> 	path = strbuf_detach(&sb, NULL);
> 	retval = xstrdup(cleanup_path(path));
>         free(path);
>         return retval;
>
> or
>
> 	path = xstrdup(cleanup_path(sb.buf));
>         strbuf_release(&sb);
>         return path;
>
> would be more sensible.
Erik, what do you prefer ? You can have the final answer.
nguyenhu@minatec.inpg.fr· Jun 11, 2012, 22:59 UTC · re: nguyenhu@minatec.inpg.fr · lore

Re: [PATCHv6 1/4] Read (but not write) from $XDG_CONFIG_HOME/git/config file

nguyenhu@minatec.inpg.fr a écrit :
Show 23 quoted lines
> Junio C Hamano <gitster@pobox.com> a écrit :
>
>> Erik Faye-Lund <kusmabite@gmail.com> writes:
>>
>>> The awkward thing about doing this, is that the memory allocated by
>>> the strbuf cannot be reclaimed if you go with this. A pointer that has
>>> been adjusted (like cleanup_path can do) cannot be successfully fed to
>>> free.
>>
>> Yeah, I wouldn't recommend doing that.  Either
>>
>> 	path = strbuf_detach(&sb, NULL);
>> 	retval = xstrdup(cleanup_path(path));
>>        free(path);
>>        return retval;
>>
>> or
>>
>> 	path = xstrdup(cleanup_path(sb.buf));
>>        strbuf_release(&sb);
>>        return path;
>>
>> would be more sensible.
In our next patch, mkpathdup() function will be
char *mkpathdup(const char *fmt, ...)
{
	char *path;
	struct strbuf sb = STRBUF_INIT;
	va_list args;
	va_start(args, fmt);
	strbuf_vaddf(&sb, fmt, args);
	va_end(args);
	path = xstrdup(cleanup_path(sb.buf));
	strbuf_release(&sb);
	return path;
}

which looks like our previous proposal with xstrdup() and cleanup_path() functions at the right place.

Erik Faye-Lund· Jun 11, 2012, 23:03 UTC · re: nguyenhu@minatec.inpg.fr · lore

Re: [PATCHv6 1/4] Read (but not write) from $XDG_CONFIG_HOME/git/config file

On Mon, Jun 11, 2012 at 6:53 PM,  <nguyenhu@minatec.inpg.fr> wrote:
Show 29 quoted lines
>
> Junio C Hamano <gitster@pobox.com> a écrit :
>
>> Erik Faye-Lund <kusmabite@gmail.com> writes:
>>
>>> The awkward thing about doing this, is that the memory allocated by
>>> the strbuf cannot be reclaimed if you go with this. A pointer that has
>>> been adjusted (like cleanup_path can do) cannot be successfully fed to
>>> free.
>>
>>
>> Yeah, I wouldn't recommend doing that.  Either
>>
>>        path = strbuf_detach(&sb, NULL);
>>        retval = xstrdup(cleanup_path(path));
>>        free(path);
>>        return retval;
>>
>> or
>>
>>        path = xstrdup(cleanup_path(sb.buf));
>>        strbuf_release(&sb);
>>        return path;
>>
>> would be more sensible.
>
>
> Erik, what do you prefer ? You can have the final answer.
>

I think the latter is the neater one, but I don't really care too much either way :)

Huynh Khoi Nguyen Nguyen· Jun 12, 2012, 02:49 UTC · re: Huynh Khoi Nguyen NGUYEN · lore

[PATCHv7 1/4] Read (but not write) from $XDG_CONFIG_HOME/git/config file

Git will be able to read from $XDG_CONFIG_HOME/git/config, a new configuration file following XDG specification. In the order of reading, this file is between global configuration file and system wide configuration file. Git currently does not write to this new configuration file. If $XDG_CONFIG_HOME is either not set or empty, $HOME/.config/git/config will be used.

Signed-off-by: Huynh Khoi Nguyen Nguyen <Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr>
Signed-off-by: Valentin Duperray <Valentin.Duperray@ensimag.imag.fr>
Signed-off-by: Franck Jonas <Franck.Jonas@ensimag.imag.fr>
Signed-off-by: Lucien Kong <Lucien.Kong@ensimag.imag.fr>
Signed-off-by: Thomas Nguy <Thomas.Nguy@ensimag.imag.fr>
Signed-off-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
---
 Documentation/git-config.txt |   12 +++++--
 builtin/config.c             |   28 +++++++++++-----
 cache.h                      |    3 ++
 config.c                     |   23 ++++++++-----
 path.c                       |   41 ++++++++++++++++++++++++
 t/t1306-xdg-files.sh         |   70 ++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 156 insertions(+), 21 deletions(-)
 create mode 100755 t/t1306-xdg-files.sh
diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index d9463cb..7e344a2 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -99,8 +99,8 @@ OPTIONS
 	For writing options: write to global ~/.gitconfig file rather than
 	the repository .git/config.
 +
-For reading options: read only from global ~/.gitconfig rather than
-from all available files.
+For reading options: read only from global ~/.gitconfig and from
+$XDG_CONFIG_HOME/git/config rather than from all available files.
 +
 See also <<FILES>>.
 
@@ -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,12 @@ $GIT_DIR/config::
 	User-specific configuration file. Also called "global"
 	configuration file.
 
+$XDG_CONFIG_HOME/git/config::
+	Second user-specific configuration file. If $XDG_CONFIG_HOME is not set
+	or empty, $HOME/.config/git/config will be used. Any single-valued
+	variable set in this file will be overwritten by whatever is in
+	~/.gitconfig.
+
 $(prefix)/etc/gitconfig::
 	System-wide configuration file.
 
diff --git a/builtin/config.c b/builtin/config.c
index 33c8820..da54fd1 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 *global = NULL, *xdg = NULL, *repo_config = NULL;
 	const char *system_wide = NULL, *local;
 	struct config_include_data inc = CONFIG_INCLUDE_INIT;
 	config_fn_t fn;
@@ -169,12 +169,10 @@ static int get_value(const char *key_, const char *regex_)
 
 	local = given_config_file;
 	if (!local) {
-		const char *home = getenv("HOME");
 		local = repo_config = git_pathdup("config");
-		if (home)
-			global = xstrdup(mkpath("%s/.gitconfig", home));
 		if (git_config_system())
 			system_wide = git_etc_gitconfig();
+		home_config_paths(&global, &xdg, "config");
 	}
 
 	if (use_key_regexp) {
@@ -229,6 +227,8 @@ static int get_value(const char *key_, const char *regex_)
 
 	if (do_all && system_wide)
 		git_config_from_file(fn, system_wide, data);
+	if (do_all && xdg)
+		git_config_from_file(fn, xdg, data);
 	if (do_all && global)
 		git_config_from_file(fn, global, data);
 	if (do_all)
@@ -238,6 +238,8 @@ static int get_value(const char *key_, const char *regex_)
 		git_config_from_file(fn, local, data);
 	if (!do_all && !seen && global)
 		git_config_from_file(fn, global, data);
+	if (!do_all && !seen && xdg)
+		git_config_from_file(fn, xdg, data);
 	if (!do_all && !seen && system_wide)
 		git_config_from_file(fn, system_wide, data);
 
@@ -255,6 +257,7 @@ static int get_value(const char *key_, const char *regex_)
 free_strings:
 	free(repo_config);
 	free(global);
+	free(xdg);
 	return ret;
 }
 
@@ -379,13 +382,20 @@ 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 = NULL;
+		char *xdg_config = NULL;
+
+		home_config_paths(&user_config, &xdg_config, "config");
+
+		if (access(user_config, R_OK) && !access(xdg_config, R_OK) &&
+		    (actions == ACTION_LIST ||
+		     actions == ACTION_GET_COLOR ||
+		     actions == ACTION_GET_COLORBOOL))
+			given_config_file = xdg_config;
+		else if (user_config)
 			given_config_file = user_config;
-		} else {
+		else
 			die("$HOME not set");
-		}
 	}
 	else if (use_system_config)
 		given_config_file = git_etc_gitconfig();
diff --git a/cache.h b/cache.h
index 06413e1..fbba2d6 100644
--- a/cache.h
+++ b/cache.h
@@ -619,6 +619,8 @@ extern char *git_snpath(char *buf, size_t n, const char *fmt, ...)
 	__attribute__((format (printf, 3, 4)));
 extern char *git_pathdup(const char *fmt, ...)
 	__attribute__((format (printf, 1, 2)));
+extern char *mkpathdup(const char *fmt, ...)
+	__attribute__((format (printf, 1, 2)));
 
 /* Return a statically allocated filename matching the sha1 signature */
 extern char *mkpath(const char *fmt, ...) __attribute__((format (printf, 1, 2)));
@@ -708,6 +710,7 @@ int set_shared_perm(const char *path, int mode);
 int safe_create_leading_directories(char *path);
 int safe_create_leading_directories_const(const char *path);
 int mkdir_in_gitdir(const char *path);
+extern void home_config_paths(char **global, char **xdg, char *file);
 extern char *expand_user_path(const char *path);
 const char *enter_repo(const char *path, int strict);
 static inline int is_absolute_path(const char *path)
diff --git a/config.c b/config.c
index 71ef171..d28a499 100644
--- a/config.c
+++ b/config.c
@@ -929,7 +929,10 @@ int git_config_system(void)
 int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 {
 	int ret = 0, found = 0;
-	const char *home = NULL;
+	char *xdg_config = NULL;
+	char *user_config = NULL;
+
+	home_config_paths(&user_config, &xdg_config, "config");
 
 	if (git_config_system() && !access(git_etc_gitconfig(), R_OK)) {
 		ret += git_config_from_file(fn, git_etc_gitconfig(),
@@ -937,14 +940,14 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 		found += 1;
 	}
 
-	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);
-			found += 1;
-		}
+	if (!access(xdg_config, R_OK)) {
+		ret += git_config_from_file(fn, xdg_config, data);
+		found += 1;
+	}
+
+	if (!access(user_config, R_OK)) {
+		ret += git_config_from_file(fn, user_config, data);
+		found += 1;
 	}
 
 	if (repo_config && !access(repo_config, R_OK)) {
@@ -963,6 +966,8 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 		break;
 	}
 
+	free(xdg_config);
+	free(user_config);
 	return ret == 0 ? found : ret;
 }
 
diff --git a/path.c b/path.c
index 6f2aa69..66acd24 100644
--- a/path.c
+++ b/path.c
@@ -87,6 +87,21 @@ char *git_pathdup(const char *fmt, ...)
 	return xstrdup(path);
 }
 
+char *mkpathdup(const char *fmt, ...)
+{
+	char *path;
+	struct strbuf sb = STRBUF_INIT;
+	va_list args;
+
+	va_start(args, fmt);
+	strbuf_vaddf(&sb, fmt, args);
+	va_end(args);
+	path = xstrdup(cleanup_path(sb.buf));
+
+	strbuf_release(&sb);
+	return path;
+}
+
 char *mkpath(const char *fmt, ...)
 {
 	va_list args;
@@ -122,6 +137,32 @@ char *git_path(const char *fmt, ...)
 	return cleanup_path(pathname);
 }
 
+void home_config_paths(char **global, char **xdg, char *file)
+{
+	char *xdg_home = getenv("XDG_CONFIG_HOME");
+	char *home = getenv("HOME");
+	char *to_free = NULL;
+
+	if (!home) {
+		if (global)
+			*global = NULL;
+	} else {
+		if (!xdg_home) {
+			to_free = mkpathdup("%s/.config", home);
+			xdg_home = to_free;
+		}
+		if (global)
+			*global = mkpathdup("%s/.gitconfig", home);
+	}
+
+	if (!xdg_home)
+		*xdg = NULL;
+	else
+		*xdg = mkpathdup("%s/git/%s", xdg_home, file);
+
+	free(to_free);
+}
+
 char *git_path_submodule(const char *path, const char *fmt, ...)
 {
 	char *pathname = get_pathname();
diff --git a/t/t1306-xdg-files.sh b/t/t1306-xdg-files.sh
new file mode 100755
index 0000000..5b971d9
--- /dev/null
+++ b/t/t1306-xdg-files.sh
@@ -0,0 +1,70 @@
+#!/bin/sh
+#
+# Copyright (c) 2012 Valentin Duperray, Lucien Kong, Franck Jonas,
+#		     Thomas Nguy, Khoi Nguyen
+#		     Grenoble INP Ensimag
+#
+
+test_description='Compatibility with $XDG_CONFIG_HOME/git/ files'
+
+. ./test-lib.sh
+
+test_expect_success 'read config: xdg file exists and ~/.gitconfig doesn'\''t' '
+	mkdir -p .config/git &&
+	echo "[alias]" >.config/git/config &&
+	echo "	myalias = !echo in_config" >>.config/git/config &&
+	echo in_config >expected &&
+	git myalias >actual &&
+	test_cmp expected actual
+'
+
+
+test_expect_success 'read config: xdg file exists and ~/.gitconfig exists' '
+	>.gitconfig &&
+	echo "[alias]" >.gitconfig &&
+	echo "	myalias = !echo in_gitconfig" >>.gitconfig &&
+	echo in_gitconfig >expected &&
+	git myalias >actual &&
+	test_cmp expected actual
+'
+
+
+test_expect_success 'read with --get: xdg file exists and ~/.gitconfig doesn'\''t' '
+	rm .gitconfig &&
+	echo "[user]" >.config/git/config &&
+	echo "	name = read_config" >>.config/git/config &&
+	echo read_config >expected &&
+	git config --get user.name >actual &&
+	test_cmp expected actual
+'
+
+
+test_expect_success 'read with --get: xdg file exists and ~/.gitconfig exists' '
+	>.gitconfig &&
+	echo "[user]" >.gitconfig &&
+	echo "	name = read_gitconfig" >>.gitconfig &&
+	echo read_gitconfig >expected &&
+	git config --get user.name >actual &&
+	test_cmp expected actual
+'
+
+
+test_expect_success 'read with --list: xdg file exists and ~/.gitconfig doesn'\''t' '
+	rm .gitconfig &&
+	echo user.name=read_config >expected &&
+	git config --global --list >actual &&
+	test_cmp expected actual
+'
+
+
+test_expect_success 'read with --list: xdg file exists and ~/.gitconfig exists' '
+	>.gitconfig &&
+	echo "[user]" >.gitconfig &&
+	echo "	name = read_gitconfig" >>.gitconfig &&
+	echo user.name=read_gitconfig >expected &&
+	git config --global --list >actual &&
+	test_cmp expected actual
+'
+
+
+test_done
-- 
1.7.8
Huynh Khoi Nguyen Nguyen· Jun 12, 2012, 02:49 UTC · re: Huynh Khoi Nguyen Nguyen · lore

[PATCHv7 2/4] Let core.excludesfile default to $XDG_CONFIG_HOME/git/ignore

To use the feature of core.excludesfile, the user needs:
- to create such a file,
- and add configuration variable to point at it.
Instead, we can define a filename that is unlikely to already exist on
the system, and only use the presence of the file as a cue that the user
wants to use that feature.
We could pick any random filename that only user can create, which
means that it has to be under $HOME. Let's use
"${XDG_CONFIG_HOME:-$HOME/.config/git}/ignore". It is as good as any
random filename in $HOME/ and follows the XDG specification.
Signed-off-by: Huynh Khoi Nguyen Nguyen <Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr>
Signed-off-by: Valentin Duperray <Valentin.Duperray@ensimag.imag.fr>
Signed-off-by: Franck Jonas <Franck.Jonas@ensimag.imag.fr>
Signed-off-by: Lucien Kong <Lucien.Kong@ensimag.imag.fr>
Signed-off-by: Thomas Nguy <Thomas.Nguy@ensimag.imag.fr>
Signed-off-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
---
 Documentation/config.txt    |    4 +++-
 Documentation/gitignore.txt |    4 +++-
 dir.c                       |    7 ++++++-
 t/t1306-xdg-files.sh        |   29 +++++++++++++++++++++++++++++
 4 files changed, 41 insertions(+), 3 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 915cb5a..509bf25 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -483,7 +483,9 @@ core.excludesfile::
 	'.git/info/exclude', git looks into this file for patterns
 	of files which are not meant to be tracked.  "`~/`" is expanded
 	to the value of `$HOME` and "`~user/`" to the specified user's
-	home directory.  See linkgit:gitignore[5].
+	home directory. Its default value is $XDG_CONFIG_HOME/git/ignore.
+	If $XDG_CONFIG_HOME is either not set or empty, $HOME/.config/git/ignore
+	will be used. See linkgit:gitignore[5].
 
 core.askpass::
 	Some commands (e.g. svn and http interfaces) that interactively
diff --git a/Documentation/gitignore.txt b/Documentation/gitignore.txt
index 2e7328b..a5c9b93 100644
--- a/Documentation/gitignore.txt
+++ b/Documentation/gitignore.txt
@@ -50,7 +50,9 @@ the repository but are specific to one user's workflow) should go into
 the `$GIT_DIR/info/exclude` file.  Patterns which a user wants git to
 ignore in all situations (e.g., backup or temporary files generated by
 the user's editor of choice) generally go into a file specified by
-`core.excludesfile` in the user's `~/.gitconfig`.
+`core.excludesfile` in the user's `~/.gitconfig`. Its default value is
+$XDG_CONFIG_HOME/git/ignore. If $XDG_CONFIG_HOME is either not set or empty,
+$HOME/.config/git/ignore will be used.
 
 The underlying git plumbing tools, such as
 'git ls-files' and 'git read-tree', read
diff --git a/dir.c b/dir.c
index ed1510f..8c6f47f 100644
--- a/dir.c
+++ b/dir.c
@@ -1234,12 +1234,17 @@ int remove_dir_recursively(struct strbuf *path, int flag)
 void setup_standard_excludes(struct dir_struct *dir)
 {
 	const char *path;
+	char *xdg_path;
 
 	dir->exclude_per_dir = ".gitignore";
 	path = git_path("info/exclude");
+	if (!excludes_file) {
+		home_config_paths(NULL, &xdg_path, "ignore");
+		excludes_file = xdg_path;
+	}
 	if (!access(path, R_OK))
 		add_excludes_from_file(dir, path);
-	if (excludes_file && !access(excludes_file, R_OK))
+	if (!access(excludes_file, R_OK))
 		add_excludes_from_file(dir, excludes_file);
 }
 
diff --git a/t/t1306-xdg-files.sh b/t/t1306-xdg-files.sh
index 5b971d9..05103f5 100755
--- a/t/t1306-xdg-files.sh
+++ b/t/t1306-xdg-files.sh
@@ -67,4 +67,33 @@ test_expect_success 'read with --list: xdg file exists and ~/.gitconfig exists'
 '
 
 
+test_expect_success 'Setup' '
+	git init git &&
+	cd git &&
+	echo foo >to_be_excluded
+'
+
+
+test_expect_success 'Exclusion of a file in the XDG ignore file' '
+	mkdir -p "$HOME"/.config/git/ &&
+	echo to_be_excluded >"$HOME"/.config/git/ignore &&
+	test_must_fail git add to_be_excluded
+'
+
+
+test_expect_success 'Exclusion in both XDG and local ignore files' '
+	echo to_be_excluded >.gitignore &&
+	test_must_fail git add to_be_excluded
+'
+
+
+test_expect_success 'Exclusion in a non-XDG global ignore file' '
+	rm .gitignore &&
+	echo >"$HOME"/.config/git/ignore &&
+	echo to_be_excluded >"$HOME"/my_gitignore &&
+	git config core.excludesfile "$HOME"/my_gitignore &&
+	test_must_fail git add to_be_excluded
+'
+
+
 test_done
-- 
1.7.8
Huynh Khoi Nguyen Nguyen· Jun 12, 2012, 02:49 UTC · re: Huynh Khoi Nguyen Nguyen · lore

[PATCHv7 3/4] Let core.attributesfile default to $XDG_CONFIG_HOME/git/attributes

To use the feature of core.attributesfile, the user needs:
- to create such a file,
- and add configuration variable to point at it.
Instead, we can define a filename that is unlikely to already exist on
the system, and only use the presence of the file as a cue that the user
wants to use that feature.
We could pick any random filename that only user can create, which
means that it has to be under $HOME. Let's use
"${XDG_CONFIG_HOME:-$HOME/.config/git}/attributes". It is as good as any
random filename in $HOME/ and follows the XDG specification.
Signed-off-by: Huynh Khoi Nguyen Nguyen <Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr>
Signed-off-by: Valentin Duperray <Valentin.Duperray@ensimag.imag.fr>
Signed-off-by: Franck Jonas <Franck.Jonas@ensimag.imag.fr>
Signed-off-by: Lucien Kong <Lucien.Kong@ensimag.imag.fr>
Signed-off-by: Thomas Nguy <Thomas.Nguy@ensimag.imag.fr>
Signed-off-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
---
 Documentation/config.txt        |    4 +++-
 Documentation/gitattributes.txt |    2 ++
 attr.c                          |   17 ++++++++++-------
 t/t1306-xdg-files.sh            |   29 +++++++++++++++++++++++++++++
 4 files changed, 44 insertions(+), 8 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 509bf25..f691d8e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -500,7 +500,9 @@ core.attributesfile::
 	In addition to '.gitattributes' (per-directory) and
 	'.git/info/attributes', git looks into this file for attributes
 	(see linkgit:gitattributes[5]). Path expansions are made the same
-	way as for `core.excludesfile`.
+	way as for `core.excludesfile`. Its default value is
+	$XDG_CONFIG_HOME/git/attributes. If $XDG_CONFIG_HOME is either not
+	set or empty, $HOME/.config/git/attributes will be used.
 
 core.editor::
 	Commands such as `commit` and `tag` that lets you edit
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 80120ea..484c614 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -75,6 +75,8 @@ repositories (i.e., attributes of interest to all users) should go into
 `.gitattributes` files. Attributes that should affect all repositories
 for a single user should be placed in a file specified by the
 `core.attributesfile` configuration option (see linkgit:git-config[1]).
+Its default value is $XDG_CONFIG_HOME/git/attributes. If $XDG_CONFIG_HOME
+is either not set or empty, $HOME/.config/git/attributes will be used.
 Attributes for all users on a system should be placed in the
 `$(prefix)/etc/gitattributes` file.
 
diff --git a/attr.c b/attr.c
index 303751f..aef93d8 100644
--- a/attr.c
+++ b/attr.c
@@ -497,6 +497,7 @@ static int git_attr_system(void)
 static void bootstrap_attr_stack(void)
 {
 	struct attr_stack *elem;
+	char *xdg_attributes_file;
 
 	if (attr_stack)
 		return;
@@ -515,13 +516,15 @@ static void bootstrap_attr_stack(void)
 		}
 	}
 
-	if (git_attributes_file) {
-		elem = read_attr_from_file(git_attributes_file, 1);
-		if (elem) {
-			elem->origin = NULL;
-			elem->prev = attr_stack;
-			attr_stack = elem;
-		}
+	if (!git_attributes_file) {
+		home_config_paths(NULL, &xdg_attributes_file, "attributes");
+		git_attributes_file = xdg_attributes_file;
+	}
+	elem = read_attr_from_file(git_attributes_file, 1);
+	if (elem) {
+		elem->origin = NULL;
+		elem->prev = attr_stack;
+		attr_stack = elem;
 	}
 
 	if (!is_bare_repository() || direction == GIT_ATTR_INDEX) {
diff --git a/t/t1306-xdg-files.sh b/t/t1306-xdg-files.sh
index 05103f5..e8cd78a 100755
--- a/t/t1306-xdg-files.sh
+++ b/t/t1306-xdg-files.sh
@@ -96,4 +96,33 @@ test_expect_success 'Exclusion in a non-XDG global ignore file' '
 '
 
 
+test_expect_success 'Checking attributes in the XDG attributes file' '
+	echo foo >f &&
+	git check-attr -a f >actual &&
+	test_line_count -eq 0 actual &&
+	echo "f attr_f" >"$HOME"/.config/git/attributes &&
+	echo "f: attr_f: set" >expected &&
+	git check-attr -a f >actual &&
+	test_cmp expected actual
+'
+
+
+test_expect_success 'Checking attributes in both XDG and local attributes files' '
+	echo "f -attr_f" >.gitattributes &&
+	echo "f: attr_f: unset" >expected &&
+	git check-attr -a f >actual &&
+	test_cmp expected actual
+'
+
+
+test_expect_success 'Checking attributes in a non-XDG global attributes file' '
+	test_might_fail rm .gitattributes &&
+	echo "f attr_f=test" >"$HOME"/my_gitattributes &&
+	git config core.attributesfile "$HOME"/my_gitattributes &&
+	echo "f: attr_f: test" >expected &&
+	git check-attr -a f >actual &&
+	test_cmp expected actual
+'
+
+
 test_done
-- 
1.7.8
Huynh Khoi Nguyen Nguyen· Jun 12, 2012, 02:49 UTC · re: Huynh Khoi Nguyen Nguyen · lore

[PATCHv7 4/4] Write to $XDG_CONFIG_HOME/git/config file

Git will be able to write to $XDG_CONFIG_HOME/git/config, a new
configuration file following XDG specification, if:
- this file already exists, and
- $HOME/.gitconfig file doesn't, and
- global option is used.
Otherwise Git writes to $HOME/.gitconfig with global option, as usual.
If you don't create $XDG_CONFIG_HOME/git/config, there is absolutely
no change. Users can use this new file only if they want.
If $XDG_CONFIG_HOME is either not set or empty, $HOME/.config/git/config
will be used.
Advice for users who often come back to an old version of Git:
you shouldn't create this file.
Signed-off-by: Huynh Khoi Nguyen Nguyen <Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr>
Signed-off-by: Valentin Duperray <Valentin.Duperray@ensimag.imag.fr>
Signed-off-by: Franck Jonas <Franck.Jonas@ensimag.imag.fr>
Signed-off-by: Lucien Kong <Lucien.Kong@ensimag.imag.fr>
Signed-off-by: Thomas Nguy <Thomas.Nguy@ensimag.imag.fr>
Signed-off-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
---
 Documentation/git-config.txt |    3 ++-
 builtin/config.c             |    5 +----
 t/t1306-xdg-files.sh         |   30 ++++++++++++++++++++++++++++++
 3 files changed, 33 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 7e344a2..a2b4786 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -97,7 +97,8 @@ OPTIONS
 
 --global::
 	For writing options: write to global ~/.gitconfig file rather than
-	the repository .git/config.
+	the repository .git/config, write to $XDG_CONFIG_HOME/git/config file
+	if this file exists and ~/.gitconfig file doesn't.
 +
 For reading options: read only from global ~/.gitconfig and from
 $XDG_CONFIG_HOME/git/config rather than from all available files.
diff --git a/builtin/config.c b/builtin/config.c
index da54fd1..e8e1c0a 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -387,10 +387,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 
 		home_config_paths(&user_config, &xdg_config, "config");
 
-		if (access(user_config, R_OK) && !access(xdg_config, R_OK) &&
-		    (actions == ACTION_LIST ||
-		     actions == ACTION_GET_COLOR ||
-		     actions == ACTION_GET_COLORBOOL))
+		if (access(user_config, R_OK) && !access(xdg_config, R_OK))
 			given_config_file = xdg_config;
 		else if (user_config)
 			given_config_file = user_config;
diff --git a/t/t1306-xdg-files.sh b/t/t1306-xdg-files.sh
index e8cd78a..3c75c3f 100755
--- a/t/t1306-xdg-files.sh
+++ b/t/t1306-xdg-files.sh
@@ -125,4 +125,34 @@ test_expect_success 'Checking attributes in a non-XDG global attributes file' '
 '
 
 
+test_expect_success 'write: xdg file exists and ~/.gitconfig doesn'\''t' '
+	mkdir -p "$HOME"/.config/git &&
+	>"$HOME"/.config/git/config &&
+	test_might_fail rm "$HOME"/.gitconfig &&
+	git config --global user.name "write_config" &&
+	echo "[user]" >expected &&
+	echo "	name = write_config" >>expected &&
+	test_cmp expected "$HOME"/.config/git/config
+'
+
+
+test_expect_success 'write: xdg file exists and ~/.gitconfig exists' '
+	>"$HOME"/.gitconfig &&
+	git config --global user.name "write_gitconfig" &&
+	echo "[user]" >expected &&
+	echo "	name = write_gitconfig" >>expected &&
+	test_cmp expected "$HOME"/.gitconfig
+'
+
+
+test_expect_success 'write: ~/.config/git/ exists and config file doesn'\''t' '
+	test_might_fail rm "$HOME"/.gitconfig &&
+	test_might_fail rm "$HOME"/.config/git/config &&
+	git config --global user.name "write_gitconfig" &&
+	echo "[user]" >expected &&
+	echo "	name = write_gitconfig" >>expected &&
+	test_cmp expected "$HOME"/.gitconfig
+'
+
+
 test_done
-- 
1.7.8
Ramsay Jones· Jun 14, 2012, 17:31 UTC · re: Huynh Khoi Nguyen Nguyen · lore

Re: [PATCHv7 1/4] Read (but not write) from $XDG_CONFIG_HOME/git/config file

Huynh Khoi Nguyen Nguyen wrote:
Show 22 quoted lines
> Git will be able to read from $XDG_CONFIG_HOME/git/config, a new
> configuration file following XDG specification. In the order of
> reading, this file is between global configuration file and system
> wide configuration file. Git currently does not write to this new
> configuration file. If $XDG_CONFIG_HOME is either not set or empty,
> $HOME/.config/git/config will be used.
> 
> Signed-off-by: Huynh Khoi Nguyen Nguyen <Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr>
> Signed-off-by: Valentin Duperray <Valentin.Duperray@ensimag.imag.fr>
> Signed-off-by: Franck Jonas <Franck.Jonas@ensimag.imag.fr>
> Signed-off-by: Lucien Kong <Lucien.Kong@ensimag.imag.fr>
> Signed-off-by: Thomas Nguy <Thomas.Nguy@ensimag.imag.fr>
> Signed-off-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
> ---
>  Documentation/git-config.txt |   12 +++++--
>  builtin/config.c             |   28 +++++++++++-----
>  cache.h                      |    3 ++
>  config.c                     |   23 ++++++++-----
>  path.c                       |   41 ++++++++++++++++++++++++
>  t/t1306-xdg-files.sh         |   70 ++++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 156 insertions(+), 21 deletions(-)
>  create mode 100755 t/t1306-xdg-files.sh
[...]
Show 23 quoted lines
> diff --git a/path.c b/path.c
> index 6f2aa69..66acd24 100644
> --- a/path.c
> +++ b/path.c
> @@ -87,6 +87,21 @@ char *git_pathdup(const char *fmt, ...)
>  	return xstrdup(path);
>  }
>  
> +char *mkpathdup(const char *fmt, ...)
> +{
> +	char *path;
> +	struct strbuf sb = STRBUF_INIT;
> +	va_list args;
> +
> +	va_start(args, fmt);
> +	strbuf_vaddf(&sb, fmt, args);
> +	va_end(args);
> +	path = xstrdup(cleanup_path(sb.buf));
> +
> +	strbuf_release(&sb);
> +	return path;
> +}
> +
As expected, this version avoids re-introducing the bug on Cygwin.

I tested the series on Cygwin, MinGW and Linux and it passes it's own tests (t1306-xdg-files.sh) on all platforms. (Well, on MinGW I had to run it thus:

    $ GIT_TEST_CMP='diff -ub' ./t1306-xdg-files.sh
otherwise the first two tests fail because of CRLF vs LF issues).
Thanks!

ATB, Ramsay Jones

Matthieu Moy· Jun 21, 2012, 16:55 UTC · re: Huynh Khoi Nguyen Nguyen · lore

Re: [PATCHv7 1/4] Read (but not write) from $XDG_CONFIG_HOME/git/config file

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

Show 6 quoted lines
> Git will be able to read from $XDG_CONFIG_HOME/git/config, a new
> configuration file following XDG specification. In the order of
> reading, this file is between global configuration file and system
> wide configuration file. Git currently does not write to this new
> configuration file. If $XDG_CONFIG_HOME is either not set or empty,
> $HOME/.config/git/config will be used.

What happened to this serie? I think all comments have been addressed, but I don't see it in pu... Should I resend and Cc Junio ?

Thanks,
-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Junio C Hamano· Jun 21, 2012, 17:22 UTC · re: Matthieu Moy · lore

Re: [PATCHv7 1/4] Read (but not write) from $XDG_CONFIG_HOME/git/config file

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
Show 12 quoted lines
> Huynh Khoi Nguyen Nguyen <Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr>
> writes:
>
>> Git will be able to read from $XDG_CONFIG_HOME/git/config, a new
>> configuration file following XDG specification. In the order of
>> reading, this file is between global configuration file and system
>> wide configuration file. Git currently does not write to this new
>> configuration file. If $XDG_CONFIG_HOME is either not set or empty,
>> $HOME/.config/git/config will be used.
>
> What happened to this serie? I think all comments have been addressed,
> but I don't see it in pu... Should I resend and Cc Junio ?

If it is ready for testing, please do. I wasn't interested and was not paying attention during the feature freeze.

Matthieu Moy· Jun 22, 2012, 09:03 UTC · re: Junio C Hamano · lore

[PATCH 0/4 v8] Git configuration directory

This is a resend of a serie already discussed here:
  http://thread.gmane.org/gmane.comp.version-control.git/198837

I took the liberty of rewording the commit messages, and change a few words in the documentation (changed some "will be used" to "is used instead", which sounded more consistant with the context).

A reminder of the context and objectives of the serie:

Git currently stores its configuration file in ~/.gitconfig, which is nice and customary on Unix, as long as one has only one configuration file.

But a typical user may want to store not only the config file, but also the files pointed to by core.excludesfile and core.attributesfile, which currently have no default values. To store several configuration files, it makes sense to have a configuration _directory_ instead of a set of configuration files, all right into $HOME. Calling this configuration directory ~/.git or ~/.gitconfig is not an option, since these paths already have another meaning. Using the XDG specification (in short: ~/.config/git) doesn't have this drawback, and allows the user to store his configuration files right next to files from other applications following the XDG standard.

The first 3 patches add read support for this "configuration directory", and the last one gives opt-in write support, allowing users to make this "configuration directory" their way of life, and never hear again about ~/.gitconfig if (and only if) they do not want to.

None of the patches change the behavior for people who do not create the new configuration files.

Huynh Khoi Nguyen Nguyen (4):
  config: read (but not write) from $XDG_CONFIG_HOME/git/config file
  Let core.excludesfile default to $XDG_CONFIG_HOME/git/ignore
  Let core.attributesfile default to $XDG_CONFIG_HOME/git/ignore
  config: write to $XDG_CONFIG_HOME/git/config file if appropriate
 Documentation/config.txt        |   8 +-
 Documentation/git-config.txt    |  15 +++-
 Documentation/gitattributes.txt |   2 +
 Documentation/gitignore.txt     |   4 +-
 attr.c                          |  17 +++--
 builtin/config.c                |  25 ++++---
 cache.h                         |   3 +
 config.c                        |  23 +++---
 dir.c                           |   7 +-
 path.c                          |  41 +++++++++++
 t/t1306-xdg-files.sh            | 158 ++++++++++++++++++++++++++++++++++++++++
 11 files changed, 270 insertions(+), 33 deletions(-)
 create mode 100755 t/t1306-xdg-files.sh
-- 
1.7.11.rc3.235.gd0d1d08
Matthieu Moy· Jun 22, 2012, 09:03 UTC · re: Matthieu Moy · lore

[PATCH 1/4 v8] config: read (but not write) from $XDG_CONFIG_HOME/git/config file

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

Git will be able to read from $XDG_CONFIG_HOME/git/config, a new configuration file following XDG specification. In the order of reading, this file is between global configuration file and system wide configuration file. Git currently does not write to this new configuration file. If $XDG_CONFIG_HOME is either not set or empty, $HOME/.config/git/config will be used. If the new file does not exist, the behavior is unchanged.

Signed-off-by: Huynh Khoi Nguyen Nguyen <Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr>
Signed-off-by: Valentin Duperray <Valentin.Duperray@ensimag.imag.fr>
Signed-off-by: Franck Jonas <Franck.Jonas@ensimag.imag.fr>
Signed-off-by: Lucien Kong <Lucien.Kong@ensimag.imag.fr>
Signed-off-by: Thomas Nguy <Thomas.Nguy@ensimag.imag.fr>
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 Documentation/git-config.txt | 12 ++++++--
 builtin/config.c             | 28 ++++++++++++------
 cache.h                      |  3 ++
 config.c                     | 23 +++++++++------
 path.c                       | 41 ++++++++++++++++++++++++++
 t/t1306-xdg-files.sh         | 70 ++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 156 insertions(+), 21 deletions(-)
 create mode 100755 t/t1306-xdg-files.sh
diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index d9463cb..7e344a2 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -99,8 +99,8 @@ OPTIONS
 	For writing options: write to global ~/.gitconfig file rather than
 	the repository .git/config.
 +
-For reading options: read only from global ~/.gitconfig rather than
-from all available files.
+For reading options: read only from global ~/.gitconfig and from
+$XDG_CONFIG_HOME/git/config rather than from all available files.
 +
 See also <<FILES>>.
 
@@ -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,12 @@ $GIT_DIR/config::
 	User-specific configuration file. Also called "global"
 	configuration file.
 
+$XDG_CONFIG_HOME/git/config::
+	Second user-specific configuration file. If $XDG_CONFIG_HOME is not set
+	or empty, $HOME/.config/git/config will be used. Any single-valued
+	variable set in this file will be overwritten by whatever is in
+	~/.gitconfig.
+
 $(prefix)/etc/gitconfig::
 	System-wide configuration file.
 
diff --git a/builtin/config.c b/builtin/config.c
index 33c8820..da54fd1 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 *global = NULL, *xdg = NULL, *repo_config = NULL;
 	const char *system_wide = NULL, *local;
 	struct config_include_data inc = CONFIG_INCLUDE_INIT;
 	config_fn_t fn;
@@ -169,12 +169,10 @@ static int get_value(const char *key_, const char *regex_)
 
 	local = given_config_file;
 	if (!local) {
-		const char *home = getenv("HOME");
 		local = repo_config = git_pathdup("config");
-		if (home)
-			global = xstrdup(mkpath("%s/.gitconfig", home));
 		if (git_config_system())
 			system_wide = git_etc_gitconfig();
+		home_config_paths(&global, &xdg, "config");
 	}
 
 	if (use_key_regexp) {
@@ -229,6 +227,8 @@ static int get_value(const char *key_, const char *regex_)
 
 	if (do_all && system_wide)
 		git_config_from_file(fn, system_wide, data);
+	if (do_all && xdg)
+		git_config_from_file(fn, xdg, data);
 	if (do_all && global)
 		git_config_from_file(fn, global, data);
 	if (do_all)
@@ -238,6 +238,8 @@ static int get_value(const char *key_, const char *regex_)
 		git_config_from_file(fn, local, data);
 	if (!do_all && !seen && global)
 		git_config_from_file(fn, global, data);
+	if (!do_all && !seen && xdg)
+		git_config_from_file(fn, xdg, data);
 	if (!do_all && !seen && system_wide)
 		git_config_from_file(fn, system_wide, data);
 
@@ -255,6 +257,7 @@ static int get_value(const char *key_, const char *regex_)
 free_strings:
 	free(repo_config);
 	free(global);
+	free(xdg);
 	return ret;
 }
 
@@ -379,13 +382,20 @@ 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 = NULL;
+		char *xdg_config = NULL;
+
+		home_config_paths(&user_config, &xdg_config, "config");
+
+		if (access(user_config, R_OK) && !access(xdg_config, R_OK) &&
+		    (actions == ACTION_LIST ||
+		     actions == ACTION_GET_COLOR ||
+		     actions == ACTION_GET_COLORBOOL))
+			given_config_file = xdg_config;
+		else if (user_config)
 			given_config_file = user_config;
-		} else {
+		else
 			die("$HOME not set");
-		}
 	}
 	else if (use_system_config)
 		given_config_file = git_etc_gitconfig();
diff --git a/cache.h b/cache.h
index 06413e1..fbba2d6 100644
--- a/cache.h
+++ b/cache.h
@@ -619,6 +619,8 @@ extern char *git_snpath(char *buf, size_t n, const char *fmt, ...)
 	__attribute__((format (printf, 3, 4)));
 extern char *git_pathdup(const char *fmt, ...)
 	__attribute__((format (printf, 1, 2)));
+extern char *mkpathdup(const char *fmt, ...)
+	__attribute__((format (printf, 1, 2)));
 
 /* Return a statically allocated filename matching the sha1 signature */
 extern char *mkpath(const char *fmt, ...) __attribute__((format (printf, 1, 2)));
@@ -708,6 +710,7 @@ int set_shared_perm(const char *path, int mode);
 int safe_create_leading_directories(char *path);
 int safe_create_leading_directories_const(const char *path);
 int mkdir_in_gitdir(const char *path);
+extern void home_config_paths(char **global, char **xdg, char *file);
 extern char *expand_user_path(const char *path);
 const char *enter_repo(const char *path, int strict);
 static inline int is_absolute_path(const char *path)
diff --git a/config.c b/config.c
index 71ef171..d28a499 100644
--- a/config.c
+++ b/config.c
@@ -929,7 +929,10 @@ int git_config_system(void)
 int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 {
 	int ret = 0, found = 0;
-	const char *home = NULL;
+	char *xdg_config = NULL;
+	char *user_config = NULL;
+
+	home_config_paths(&user_config, &xdg_config, "config");
 
 	if (git_config_system() && !access(git_etc_gitconfig(), R_OK)) {
 		ret += git_config_from_file(fn, git_etc_gitconfig(),
@@ -937,14 +940,14 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 		found += 1;
 	}
 
-	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);
-			found += 1;
-		}
+	if (!access(xdg_config, R_OK)) {
+		ret += git_config_from_file(fn, xdg_config, data);
+		found += 1;
+	}
+
+	if (!access(user_config, R_OK)) {
+		ret += git_config_from_file(fn, user_config, data);
+		found += 1;
 	}
 
 	if (repo_config && !access(repo_config, R_OK)) {
@@ -963,6 +966,8 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 		break;
 	}
 
+	free(xdg_config);
+	free(user_config);
 	return ret == 0 ? found : ret;
 }
 
diff --git a/path.c b/path.c
index 6f2aa69..66acd24 100644
--- a/path.c
+++ b/path.c
@@ -87,6 +87,21 @@ char *git_pathdup(const char *fmt, ...)
 	return xstrdup(path);
 }
 
+char *mkpathdup(const char *fmt, ...)
+{
+	char *path;
+	struct strbuf sb = STRBUF_INIT;
+	va_list args;
+
+	va_start(args, fmt);
+	strbuf_vaddf(&sb, fmt, args);
+	va_end(args);
+	path = xstrdup(cleanup_path(sb.buf));
+
+	strbuf_release(&sb);
+	return path;
+}
+
 char *mkpath(const char *fmt, ...)
 {
 	va_list args;
@@ -122,6 +137,32 @@ char *git_path(const char *fmt, ...)
 	return cleanup_path(pathname);
 }
 
+void home_config_paths(char **global, char **xdg, char *file)
+{
+	char *xdg_home = getenv("XDG_CONFIG_HOME");
+	char *home = getenv("HOME");
+	char *to_free = NULL;
+
+	if (!home) {
+		if (global)
+			*global = NULL;
+	} else {
+		if (!xdg_home) {
+			to_free = mkpathdup("%s/.config", home);
+			xdg_home = to_free;
+		}
+		if (global)
+			*global = mkpathdup("%s/.gitconfig", home);
+	}
+
+	if (!xdg_home)
+		*xdg = NULL;
+	else
+		*xdg = mkpathdup("%s/git/%s", xdg_home, file);
+
+	free(to_free);
+}
+
 char *git_path_submodule(const char *path, const char *fmt, ...)
 {
 	char *pathname = get_pathname();
diff --git a/t/t1306-xdg-files.sh b/t/t1306-xdg-files.sh
new file mode 100755
index 0000000..5b971d9
--- /dev/null
+++ b/t/t1306-xdg-files.sh
@@ -0,0 +1,70 @@
+#!/bin/sh
+#
+# Copyright (c) 2012 Valentin Duperray, Lucien Kong, Franck Jonas,
+#		     Thomas Nguy, Khoi Nguyen
+#		     Grenoble INP Ensimag
+#
+
+test_description='Compatibility with $XDG_CONFIG_HOME/git/ files'
+
+. ./test-lib.sh
+
+test_expect_success 'read config: xdg file exists and ~/.gitconfig doesn'\''t' '
+	mkdir -p .config/git &&
+	echo "[alias]" >.config/git/config &&
+	echo "	myalias = !echo in_config" >>.config/git/config &&
+	echo in_config >expected &&
+	git myalias >actual &&
+	test_cmp expected actual
+'
+
+
+test_expect_success 'read config: xdg file exists and ~/.gitconfig exists' '
+	>.gitconfig &&
+	echo "[alias]" >.gitconfig &&
+	echo "	myalias = !echo in_gitconfig" >>.gitconfig &&
+	echo in_gitconfig >expected &&
+	git myalias >actual &&
+	test_cmp expected actual
+'
+
+
+test_expect_success 'read with --get: xdg file exists and ~/.gitconfig doesn'\''t' '
+	rm .gitconfig &&
+	echo "[user]" >.config/git/config &&
+	echo "	name = read_config" >>.config/git/config &&
+	echo read_config >expected &&
+	git config --get user.name >actual &&
+	test_cmp expected actual
+'
+
+
+test_expect_success 'read with --get: xdg file exists and ~/.gitconfig exists' '
+	>.gitconfig &&
+	echo "[user]" >.gitconfig &&
+	echo "	name = read_gitconfig" >>.gitconfig &&
+	echo read_gitconfig >expected &&
+	git config --get user.name >actual &&
+	test_cmp expected actual
+'
+
+
+test_expect_success 'read with --list: xdg file exists and ~/.gitconfig doesn'\''t' '
+	rm .gitconfig &&
+	echo user.name=read_config >expected &&
+	git config --global --list >actual &&
+	test_cmp expected actual
+'
+
+
+test_expect_success 'read with --list: xdg file exists and ~/.gitconfig exists' '
+	>.gitconfig &&
+	echo "[user]" >.gitconfig &&
+	echo "	name = read_gitconfig" >>.gitconfig &&
+	echo user.name=read_gitconfig >expected &&
+	git config --global --list >actual &&
+	test_cmp expected actual
+'
+
+
+test_done
-- 
1.7.11.rc3.235.gd0d1d08
Thomas Rast· Jul 12, 2012, 07:55 UTC · re: Matthieu Moy · lore

Re: [PATCH 1/4 v8] config: read (but not write) from $XDG_CONFIG_HOME/git/config file

Hi all,

This patch causes valgrind warnings in t1300.81 (get --path copes with unset $HOME) about passing NULL to access():

==25286== Syscall param access(pathname) points to unaddressable byte(s) ==25286== at 0x56E2227: access (in /lib64/libc-2.14.1.so) ==25286== by 0x48CA42: git_config_early (config.c:948) ==25286== by 0x4F0C20: check_repository_format_gently (setup.c:369) ==25286== by 0x4F1A52: setup_git_directory_gently_1 (setup.c:531) ==25286== by 0x4F24ED: setup_git_directory_gently (setup.c:725) ==25286== by 0x405D8C: run_builtin (git.c:287) ==25286== by 0x40548E: main (git.c:467) ==25286== Address 0x0 is not stack'd, malloc'd or (recently) free'd

AFAICT it is this bit of code:
Matthieu Moy <Matthieu.Moy@imag.fr> writes:
Show 9 quoted lines
> +void home_config_paths(char **global, char **xdg, char *file)
> +{
> +	char *xdg_home = getenv("XDG_CONFIG_HOME");
> +	char *home = getenv("HOME");
> +	char *to_free = NULL;
> +
> +	if (!home) {
> +		if (global)
> +			*global = NULL;
[...]
Show 32 quoted lines
>  int git_config_early(config_fn_t fn, void *data, const char *repo_config)
>  {
>  	int ret = 0, found = 0;
> -	const char *home = NULL;
> +	char *xdg_config = NULL;
> +	char *user_config = NULL;
> +
> +	home_config_paths(&user_config, &xdg_config, "config");
>  
>  	if (git_config_system() && !access(git_etc_gitconfig(), R_OK)) {
>  		ret += git_config_from_file(fn, git_etc_gitconfig(),
> @@ -937,14 +940,14 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
>  		found += 1;
>  	}
>  
> -	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);
> -			found += 1;
> -		}
> +	if (!access(xdg_config, R_OK)) {
> +		ret += git_config_from_file(fn, xdg_config, data);
> +		found += 1;
> +	}
> +
> +	if (!access(user_config, R_OK)) {
> +		ret += git_config_from_file(fn, user_config, data);
> +		found += 1;
>  	}

That is, while the old code was careful about home==NULL, the new code checks this only in home_config_paths(); after that, it does no further NULL check.

There's a similar instance in your changes to cmd_config(), found by running 'unset HOME; valgrind git config --global --get foo.bar':

==27841== Syscall param access(pathname) points to unaddressable byte(s) ==27841== at 0x56E2227: access (in /lib64/libc-2.14.1.so) ==27841== by 0x425280: cmd_config (config.c:390) ==27841== by 0x405C34: run_builtin (git.c:306) ==27841== by 0x40548E: main (git.c:467) ==27841== Address 0x0 is not stack'd, malloc'd or (recently) free'd

around here:
> @@ -379,13 +382,20 @@ int cmd_config(int argc, const char **argv, const char *prefix)
[...]
Show 6 quoted lines
> +		char *user_config = NULL;
> +		char *xdg_config = NULL;
> +
> +		home_config_paths(&user_config, &xdg_config, "config");
> +
> +		if (access(user_config, R_OK) && !access(xdg_config, R_OK) &&
Can you fix this?
-- 
Thomas Rast
trast@{inf,student}.ethz.ch
Matthieu Moy· Jul 12, 2012, 12:04 UTC · re: Thomas Rast · lore

[PATCH] config: fix several access(NULL) calls

When $HOME is unset, home_config_paths fails and returns NULL pointers for user_config and xdg_config. Valgrind complains with Syscall param access(pathname) points to unaddressable byte(s).

Don't call blindly access() on these variables, but test them for NULL-ness before.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
> This patch causes valgrind warnings in t1300.81 (get --path copes with
> unset $HOME) about passing NULL to access():
Indeed. The following patch should fix it.
 builtin/config.c | 3 ++-
 config.c         | 4 ++--
 2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index e8e1c0a..67945b2 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -387,7 +387,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 
 		home_config_paths(&user_config, &xdg_config, "config");
 
-		if (access(user_config, R_OK) && !access(xdg_config, R_OK))
+		if (user_config && access(user_config, R_OK) &&
+		    xdg_config && !access(xdg_config, R_OK))
 			given_config_file = xdg_config;
 		else if (user_config)
 			given_config_file = user_config;
diff --git a/config.c b/config.c
index d28a499..6b97503 100644
--- a/config.c
+++ b/config.c
@@ -940,12 +940,12 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 		found += 1;
 	}
 
-	if (!access(xdg_config, R_OK)) {
+	if (xdg_config && !access(xdg_config, R_OK)) {
 		ret += git_config_from_file(fn, xdg_config, data);
 		found += 1;
 	}
 
-	if (!access(user_config, R_OK)) {
+	if (user_config && !access(user_config, R_OK)) {
 		ret += git_config_from_file(fn, user_config, data);
 		found += 1;
 	}
-- 
1.7.11.1.29.g0e8593d.dirty
Thomas Rast· Jul 12, 2012, 12:39 UTC · re: Matthieu Moy · lore

Re: [PATCH] config: fix several access(NULL) calls

Matthieu Moy <Matthieu.Moy@imag.fr> writes:
Show 13 quoted lines
> When $HOME is unset, home_config_paths fails and returns NULL pointers
> for user_config and xdg_config. Valgrind complains with Syscall param
> access(pathname) points to unaddressable byte(s).
>
> Don't call blindly access() on these variables, but test them for
> NULL-ness before.
>
> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
> ---
>> This patch causes valgrind warnings in t1300.81 (get --path copes with
>> unset $HOME) about passing NULL to access():
>
> Indeed. The following patch should fix it.
Thanks!
Tested-by: Thomas Rast <trast@student.ethz.ch>
-- 
Thomas Rast
trast@{inf,student}.ethz.ch
Junio C Hamano· Jul 12, 2012, 17:14 UTC · re: Matthieu Moy · lore

Re: [PATCH] config: fix several access(NULL) calls

Matthieu Moy <Matthieu.Moy@imag.fr> writes:
Show 30 quoted lines
> When $HOME is unset, home_config_paths fails and returns NULL pointers
> for user_config and xdg_config. Valgrind complains with Syscall param
> access(pathname) points to unaddressable byte(s).
>
> Don't call blindly access() on these variables, but test them for
> NULL-ness before.
>
> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
> ---
>> This patch causes valgrind warnings in t1300.81 (get --path copes with
>> unset $HOME) about passing NULL to access():
>
> Indeed. The following patch should fix it.
>
>  builtin/config.c | 3 ++-
>  config.c         | 4 ++--
>  2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/config.c b/builtin/config.c
> index e8e1c0a..67945b2 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -387,7 +387,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
>  
>  		home_config_paths(&user_config, &xdg_config, "config");
>  
> -		if (access(user_config, R_OK) && !access(xdg_config, R_OK))
> +		if (user_config && access(user_config, R_OK) &&
> +		    xdg_config && !access(xdg_config, R_OK))
>  			given_config_file = xdg_config;

Shouldn't we be using xdg_config, if user_config is NULL and xdg_config is defined and accessible?

In other words, isn't the objective of this "fix" is to replace any call to access(PATH, PERM) whose PATH can potentially be NULL with

	(PATH ? access(PATH, PERM) : -1)

because we want to pretend access(PATH, PERM) returned an error, saying "The variable PATH holds a path to the file that is not accessible", when PATH is NULL?

And that is equivalent to
	(!PATH || access(PATH, PERM))
in the context of boolean.  The original
	if (access(user_config, R_OK) && !access(xdg_config, R_OK))
becomes 
	if ((!user_config || access(user_config, R_OK)) &&
	    !(!xdg_config || access(xdg_config, R_OK)))
and the latter half of it is the same as
	    (xdg_config && !access(xdg_config, R_OK))
but the former half is not. Shouldn't it be
	if ((!user_config || access(user_config, R_OK)) &&
	    (xdg_config && !access(xdg_config, R_OK)))
Confused.
Show 19 quoted lines
> diff --git a/config.c b/config.c
> index d28a499..6b97503 100644
> --- a/config.c
> +++ b/config.c
> @@ -940,12 +940,12 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
>  		found += 1;
>  	}
>  
> -	if (!access(xdg_config, R_OK)) {
> +	if (xdg_config && !access(xdg_config, R_OK)) {
>  		ret += git_config_from_file(fn, xdg_config, data);
>  		found += 1;
>  	}
>  
> -	if (!access(user_config, R_OK)) {
> +	if (user_config && !access(user_config, R_OK)) {
>  		ret += git_config_from_file(fn, user_config, data);
>  		found += 1;
>  	}
Matthieu Moy· Jul 12, 2012, 19:34 UTC · re: Junio C Hamano · lore

Re: [PATCH] config: fix several access(NULL) calls

Junio C Hamano <gitster@pobox.com> writes:
Show 35 quoted lines
> Matthieu Moy <Matthieu.Moy@imag.fr> writes:
>
>> When $HOME is unset, home_config_paths fails and returns NULL pointers
>> for user_config and xdg_config. Valgrind complains with Syscall param
>> access(pathname) points to unaddressable byte(s).
>>
>> Don't call blindly access() on these variables, but test them for
>> NULL-ness before.
>>
>> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
>> ---
>>> This patch causes valgrind warnings in t1300.81 (get --path copes with
>>> unset $HOME) about passing NULL to access():
>>
>> Indeed. The following patch should fix it.
>>
>>  builtin/config.c | 3 ++-
>>  config.c         | 4 ++--
>>  2 files changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/builtin/config.c b/builtin/config.c
>> index e8e1c0a..67945b2 100644
>> --- a/builtin/config.c
>> +++ b/builtin/config.c
>> @@ -387,7 +387,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
>>  
>>  		home_config_paths(&user_config, &xdg_config, "config");
>>  
>> -		if (access(user_config, R_OK) && !access(xdg_config, R_OK))
>> +		if (user_config && access(user_config, R_OK) &&
>> +		    xdg_config && !access(xdg_config, R_OK))
>>  			given_config_file = xdg_config;
>
> Shouldn't we be using xdg_config, if user_config is NULL and
> xdg_config is defined and accessible?

I don't think so. If user_config is NULL, it means something went wrong, because $HOME is unset. In this case, I'd rather die than using some other configuration file silently (which would be possible if $XDG_CONFIG_HOME is set), and this is what the code does:

		if (user_config && access(user_config, R_OK) &&
		    xdg_config && !access(xdg_config, R_OK))
			given_config_file = xdg_config;
		else if (user_config)
			given_config_file = user_config;
		else
			die("$HOME not set");
Perhaps it could actually be made even clearer with
		if (!user_config)
			die("$HOME not set");
		else if (access(user_config, R_OK) &&
			 xdg_config && !access(xdg_config, R_OK))
			given_config_file = xdg_config;
		else
			given_config_file = user_config;
That said, I don't care very strongly about it.
-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Junio C Hamano· Jul 12, 2012, 20:12 UTC · re: Matthieu Moy · lore

Re: [PATCH] config: fix several access(NULL) calls

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
Show 29 quoted lines
>>> +		if (user_config && access(user_config, R_OK) &&
>>> +		    xdg_config && !access(xdg_config, R_OK))
>>>  			given_config_file = xdg_config;
>>
>> Shouldn't we be using xdg_config, if user_config is NULL and
>> xdg_config is defined and accessible?
>
> I don't think so. If user_config is NULL, it means something went wrong,
> because $HOME is unset. In this case, I'd rather die than using some
> other configuration file silently (which would be possible if
> $XDG_CONFIG_HOME is set), and this is what the code does:
>
> 		if (user_config && access(user_config, R_OK) &&
> 		    xdg_config && !access(xdg_config, R_OK))
> 			given_config_file = xdg_config;
> 		else if (user_config)
> 			given_config_file = user_config;
> 		else
> 			die("$HOME not set");
>
> Perhaps it could actually be made even clearer with
>
> 		if (!user_config)
> 			die("$HOME not set");
> 		else if (access(user_config, R_OK) &&
> 			 xdg_config && !access(xdg_config, R_OK))
> 			given_config_file = xdg_config;
> 		else
> 			given_config_file = user_config;
At least the code needs to break lines at different point and a comment.
	if (user_config &&
		access(user_config, R_OK) &&
                (xdg_config && !access(xdg_config, R_OK)))
                /*
                 * Even if we have usable XDG stuff, we want to
		 * error out on missing HOME!!!
                 */
		use xdg config;
	else if (user_config)
		use user config;
	else
		unusable situation;

But is it really true that we want to error out on missing HOME if we have usable XDG stuff?

Matthieu Moy· Jul 13, 2012, 08:48 UTC · re: Junio C Hamano · lore

Re: [PATCH] config: fix several access(NULL) calls

Junio C Hamano <gitster@pobox.com> writes:
> But is it really true that we want to error out on missing HOME if
> we have usable XDG stuff?
Anyone else have an opinion on this?
In short, the question is whether
  export XDG_CONFIG_HOME=some-existing-dir
  unset HOME
  git config foo.baz boz
should die("$HOME is unset") or use the XDG config file.
-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Matthieu Moy· Jul 13, 2012, 08:59 UTC · re: Matthieu Moy · lore

[PATCH v2] config: fix several access(NULL) calls

When $HOME is unset, home_config_paths fails and returns NULL pointers for user_config and xdg_config. Valgrind complains with Syscall param access(pathname) points to unaddressable byte(s).

Don't call blindly access() on these variables, but test them for NULL-ness before.

The when the XDG configuration file can be found but not $HOME/.gitconfig requires a bit of attention. We chose to error out in "git config --set" if $HOME is unset anyway.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---

Before I forget about it, here's the patch assuming people do want to error out when $HOME is unset. It should be functionally equivalent to the previous one, but the code should be clearer.

 builtin/config.c | 15 +++++++++++----
 config.c         |  4 ++--
 2 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index e8e1c0a..f064d6e 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -387,12 +387,19 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 
 		home_config_paths(&user_config, &xdg_config, "config");
 
-		if (access(user_config, R_OK) && !access(xdg_config, R_OK))
+		if (!user_config)
+			/*
+			 * Don't even try to access the xdg_config, as
+			 * unset $HOME means something is really
+			 * broken and should be fixed. Silently
+			 * writing to xdg_config may be confusing.
+			 */
+			die("$HOME not set");
+		else if (access(user_config, R_OK) &&
+			 xdg_config && !access(xdg_config, R_OK))
 			given_config_file = xdg_config;
-		else if (user_config)
-			given_config_file = user_config;
 		else
-			die("$HOME not set");
+			given_config_file = user_config;
 	}
 	else if (use_system_config)
 		given_config_file = git_etc_gitconfig();
diff --git a/config.c b/config.c
index d28a499..6b97503 100644
--- a/config.c
+++ b/config.c
@@ -940,12 +940,12 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 		found += 1;
 	}
 
-	if (!access(xdg_config, R_OK)) {
+	if (xdg_config && !access(xdg_config, R_OK)) {
 		ret += git_config_from_file(fn, xdg_config, data);
 		found += 1;
 	}
 
-	if (!access(user_config, R_OK)) {
+	if (user_config && !access(user_config, R_OK)) {
 		ret += git_config_from_file(fn, user_config, data);
 		found += 1;
 	}
-- 
1.7.11.1.30.g7e1baf9.dirty
Jeff King· Jul 13, 2012, 13:00 UTC · re: Matthieu Moy · lore

Re: [PATCH] config: fix several access(NULL) calls

On Fri, Jul 13, 2012 at 10:48:18AM +0200, Matthieu Moy wrote:
Show 14 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > But is it really true that we want to error out on missing HOME if
> > we have usable XDG stuff?
> 
> Anyone else have an opinion on this?
> 
> In short, the question is whether
> 
>   export XDG_CONFIG_HOME=some-existing-dir
>   unset HOME
>   git config foo.baz boz
> 
> should die("$HOME is unset") or use the XDG config file.

What did previous versions of git do? From my reading of 21cf32279, the previous behavior was that if $HOME was not set, git would silently avoid reading from $HOME/.gitconfig entirely. Wouldn't dying be a huge regression?

-Peff
Matthieu Moy· Jul 13, 2012, 13:15 UTC · re: Jeff King · lore

Re: [PATCH] config: fix several access(NULL) calls

Jeff King <peff@peff.net> writes:
Show 20 quoted lines
> On Fri, Jul 13, 2012 at 10:48:18AM +0200, Matthieu Moy wrote:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>> 
>> > But is it really true that we want to error out on missing HOME if
>> > we have usable XDG stuff?
>> 
>> Anyone else have an opinion on this?
>> 
>> In short, the question is whether
>> 
>>   export XDG_CONFIG_HOME=some-existing-dir
>>   unset HOME
>>   git config foo.baz boz
>> 
>> should die("$HOME is unset") or use the XDG config file.
>
> What did previous versions of git do? From my reading of 21cf32279, the
> previous behavior was that if $HOME was not set, git would silently
> avoid reading from $HOME/.gitconfig entirely.

Yes, and this is still the case for _reading_. But the current case is about deciding which file to use when _writing_. Git was already dying when writing with an unset $HOME. There is no behavior change in this case.

With Junio's suggestion, we would have a behavior change in that we would write to the XDG file if we can find it (using XDG_CONFIG_HOME, obviously, since $HOME is unset in this case).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Thomas Rast· Jul 13, 2012, 14:05 UTC · re: Matthieu Moy · lore

Re: [PATCH] config: fix several access(NULL) calls

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
Show 26 quoted lines
> Jeff King <peff@peff.net> writes:
>
>> On Fri, Jul 13, 2012 at 10:48:18AM +0200, Matthieu Moy wrote:
>>
>>> Junio C Hamano <gitster@pobox.com> writes:
>>> 
>>> > But is it really true that we want to error out on missing HOME if
>>> > we have usable XDG stuff?
>>> 
>>> Anyone else have an opinion on this?
>>> 
>>> In short, the question is whether
>>> 
>>>   export XDG_CONFIG_HOME=some-existing-dir
>>>   unset HOME
>>>   git config foo.baz boz
>>> 
>>> should die("$HOME is unset") or use the XDG config file.
>>
>> What did previous versions of git do? From my reading of 21cf32279, the
>> previous behavior was that if $HOME was not set, git would silently
>> avoid reading from $HOME/.gitconfig entirely.
>
> Yes, and this is still the case for _reading_. But the current case is
> about deciding which file to use when _writing_. Git was already dying
> when writing with an unset $HOME.

Umm, are you sure? I may be somewhat confused about this, but the tests I used to trigger the access(NULL) were IIRC

  unset HOME
  git config --get foo.bar
  git config --global --get foo.bar
none of which is writing....
-- 
Thomas Rast
trast@{inf,student}.ethz.ch
Matthieu Moy· Jul 13, 2012, 14:23 UTC · re: Thomas Rast · lore

Re: [PATCH] config: fix several access(NULL) calls

Thomas Rast <trast@student.ethz.ch> writes:
Show 8 quoted lines
> Umm, are you sure?  I may be somewhat confused about this, but the tests
> I used to trigger the access(NULL) were IIRC
>
>   unset HOME
>   git config --get foo.bar
>   git config --global --get foo.bar
>
> none of which is writing....

I was inaccurate, but that doesn't change the conclusion: the question is not reading Vs writting, but use of --global Vs no use of it (and most of the time, --global is used for writing, hence my confusion).

Both the patch that introduce the NULL bug and the one that fix it touch two files: $git/config.c, and $git/builtin/config.c.

My changes to $git/config.c, are straightforward because they touch positive tests for file existance. The controversial one is $git/builtin/config.c, which is inside a "if (use_global_config)".

This piece of code was already dying on unset $HOME, and in my proposal it still is.

The old code was:
        if (use_global_config) {
                char *home = getenv("HOME");
                if (home) {
                        char *user_config = xstrdup(mkpath("%s/.gitconfig", home));
                        given_config_file = user_config;
                } else {
                        die("$HOME not set");
                }
        }
-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Junio C Hamano· Jul 13, 2012, 16:49 UTC · re: Matthieu Moy · lore

Re: [PATCH] config: fix several access(NULL) calls

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
Show 11 quoted lines
> The old code was:
>
>         if (use_global_config) {
>                 char *home = getenv("HOME");
>                 if (home) {
>                         char *user_config = xstrdup(mkpath("%s/.gitconfig", home));
>                         given_config_file = user_config;
>                 } else {
>                         die("$HOME not set");
>                 }
>         }

I think the above is a good illustration of the crux of the issue, as the natural way to introduce XDG that is used when the one at $HOME/.gitconfig is not there seems at least to me to do this:

	if (use_global_config) {
                if (is $HOME/.gitconfig usable?) {
			use it;
		} else if (is $XDG/git/config usable?) {
                	use $XDG location;
		} else {
			die("Neither $XDG or $HOME is usable");
		}
	}

In other words, we are not trying to say "You do not have $HOME -- there is something wrong, so we will give up" in this codepath. We were saying "Can we use _the_ place we place personal config? It is an error if we cannot find it, so we die." and the above says "Can we use this place? Can we use that place? If no place is found to be usable, we die."

And the intent of the "NULL-buggy" version seems to want the same "If we cannot use home/.gitconfig but xdg_config one is usable, use xdg one" semantics to me.

		if (access(user_config, R_OK) && !access(xdg_config, R_OK))
 			given_config_file = xdg_config;
 		else if (user_config)
 			given_config_file = user_config;
Matthieu Moy· Jul 16, 2012, 09:45 UTC · re: Junio C Hamano · lore

Re: [PATCH] config: fix several access(NULL) calls

Junio C Hamano <gitster@pobox.com> writes:
> 	if (use_global_config) {
>                 if (is $HOME/.gitconfig usable?) {
> 			use it;

Yes, but when $HOME is unset, the question doesn't really make sense. Maybe the file exists, but we can't know since the user broke his configuration by unsetting $HOME. The intent was to avoid writing to the XDG file unless it was very clear that the user wanted it, so in doubt, dying seems the best option.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Junio C Hamano· Jul 16, 2012, 16:35 UTC · re: Matthieu Moy · lore

Re: [PATCH] config: fix several access(NULL) calls

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
Show 11 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
>> 	if (use_global_config) {
>>                 if (is $HOME/.gitconfig usable?) {
>> 			use it;
>
> Yes, but when $HOME is unset, the question doesn't really make sense.
> Maybe the file exists, but we can't know since the user broke his
> configuration by unsetting $HOME. The intent was to avoid writing to the
> XDG file unless it was very clear that the user wanted it, so in doubt,
> dying seems the best option.

I would think that it is plausible that the user wanted to write into XDG one and used "unset HOME" as a way to signal that wish.

Are there ways to force writing into XDG ones without having to remove the $HOME/ ones (perhaps the user wants to keep them for use with older versions of Git on a different machine that shares the same $HOME directory)? Temporarily unsetting HOME may be how a user might achieve it.

If you want to disallow such a use case, that is fine, but at least the logic needs to be described in comment. Perhaps based on one of your rewrites earlier in the thread, like this?

 builtin/config.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index 67945b2..a788409 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -387,13 +387,26 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 
 		home_config_paths(&user_config, &xdg_config, "config");
 
-		if (user_config && access(user_config, R_OK) &&
+		if (!user_config)
+			/*
+			 * We do not know HOME/.gitconfig exists or
+			 * not, hence we do not know if we should
+			 * write to XDG location, so we error out even
+			 * if XDG_CONFIG_HOME is set and points at a
+			 * sane location.
+			 *
+			 * In other words, we forbid the user from
+			 * telling us to write to XDG location,
+			 * pretending that $HOME/.gitconfig does not
+			 * exist, by temporarily unsetting HOME.
+			 */
+			die("$HOME not set");
+
+		if (access(user_config, R_OK) &&
 		    xdg_config && !access(xdg_config, R_OK))
 			given_config_file = xdg_config;
-		else if (user_config)
-			given_config_file = user_config;
 		else
-			die("$HOME not set");
+			given_config_file = user_config;
 	}
 	else if (use_system_config)
 		given_config_file = git_etc_gitconfig();
Matthieu Moy· Jul 16, 2012, 16:39 UTC · re: Junio C Hamano · lore

Re: [PATCH] config: fix several access(NULL) calls

Junio C Hamano <gitster@pobox.com> writes:
> I would think that it is plausible that the user wanted to write
> into XDG one and used "unset HOME" as a way to signal that wish. 
I didn't think of this case, but it makes sense.

Anyway, I don't really care either way, so I leave it up to you (either your patch below, or the fixup in pu).

Show 24 quoted lines
> -		if (user_config && access(user_config, R_OK) &&
> +		if (!user_config)
> +			/*
> +			 * We do not know HOME/.gitconfig exists or
> +			 * not, hence we do not know if we should
> +			 * write to XDG location, so we error out even
> +			 * if XDG_CONFIG_HOME is set and points at a
> +			 * sane location.
> +			 *
> +			 * In other words, we forbid the user from
> +			 * telling us to write to XDG location,
> +			 * pretending that $HOME/.gitconfig does not
> +			 * exist, by temporarily unsetting HOME.
> +			 */
> +			die("$HOME not set");
> +
> +		if (access(user_config, R_OK) &&
>  		    xdg_config && !access(xdg_config, R_OK))
>  			given_config_file = xdg_config;
> -		else if (user_config)
> -			given_config_file = user_config;
>  		else
> -			die("$HOME not set");
> +			given_config_file = user_config;
-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Junio C Hamano· Jul 16, 2012, 16:56 UTC · re: Matthieu Moy · lore

Re: [PATCH] config: fix several access(NULL) calls

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
Show 9 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
>> I would think that it is plausible that the user wanted to write
>> into XDG one and used "unset HOME" as a way to signal that wish. 
>
> I didn't think of this case, but it makes sense.
>
> Anyway, I don't really care either way, so I leave it up to you (either
> your patch below, or the fixup in pu).

I do not care deeply either, as there always is "config -f $file" if somebody wants to write into a specific location.

But if you do something that is different from a straight-and-dumb conversion from access(A, B) to (A ? access(A, B) : -1), the conversion is bound to be questioned by somebody in 2 years "isn't this a misconversion?". If you did that on purpose, a comment to explain why it is not a misconversion would help them understand the reasoning behind it.

Matthieu Moy· Jun 22, 2012, 09:03 UTC · re: Matthieu Moy · lore

[PATCH 2/4 v8] Let core.excludesfile default to $XDG_CONFIG_HOME/git/ignore

From: Huynh Khoi Nguyen Nguyen <Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr>
To use the feature of core.excludesfile, the user needs:
 1. to create such a file,
 2. and add configuration variable to point at it.

Instead, we can make this a one-step process by choosing a default value which points to a filename in the user's $HOME, that is unlikely to already exist on the system, and only use the presence of the file as a cue that the user wants to use that feature.

We use "${XDG_CONFIG_HOME:-$HOME/.config/git}/ignore" as a default value. It goes in the same directory as the newly added configuration file ("${XDG_CONFIG_HOME:-$HOME/.config/git}/config), and follows the XDG specification.

Signed-off-by: Huynh Khoi Nguyen Nguyen <Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr>
Signed-off-by: Valentin Duperray <Valentin.Duperray@ensimag.imag.fr>
Signed-off-by: Franck Jonas <Franck.Jonas@ensimag.imag.fr>
Signed-off-by: Lucien Kong <Lucien.Kong@ensimag.imag.fr>
Signed-off-by: Thomas Nguy <Thomas.Nguy@ensimag.imag.fr>
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 Documentation/config.txt    |  4 +++-
 Documentation/gitignore.txt |  4 +++-
 dir.c                       |  7 ++++++-
 t/t1306-xdg-files.sh        | 29 +++++++++++++++++++++++++++++
 4 files changed, 41 insertions(+), 3 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 915cb5a..20e9531 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -483,7 +483,9 @@ core.excludesfile::
 	'.git/info/exclude', git looks into this file for patterns
 	of files which are not meant to be tracked.  "`~/`" is expanded
 	to the value of `$HOME` and "`~user/`" to the specified user's
-	home directory.  See linkgit:gitignore[5].
+	home directory. Its default value is $XDG_CONFIG_HOME/git/ignore.
+	If $XDG_CONFIG_HOME is either not set or empty, $HOME/.config/git/ignore
+	is used instead. See linkgit:gitignore[5].
 
 core.askpass::
 	Some commands (e.g. svn and http interfaces) that interactively
diff --git a/Documentation/gitignore.txt b/Documentation/gitignore.txt
index 2e7328b..c1f692a 100644
--- a/Documentation/gitignore.txt
+++ b/Documentation/gitignore.txt
@@ -50,7 +50,9 @@ the repository but are specific to one user's workflow) should go into
 the `$GIT_DIR/info/exclude` file.  Patterns which a user wants git to
 ignore in all situations (e.g., backup or temporary files generated by
 the user's editor of choice) generally go into a file specified by
-`core.excludesfile` in the user's `~/.gitconfig`.
+`core.excludesfile` in the user's `~/.gitconfig`. Its default value is
+$XDG_CONFIG_HOME/git/ignore. If $XDG_CONFIG_HOME is either not set or empty,
+$HOME/.config/git/ignore is used instead.
 
 The underlying git plumbing tools, such as
 'git ls-files' and 'git read-tree', read
diff --git a/dir.c b/dir.c
index ed1510f..8c6f47f 100644
--- a/dir.c
+++ b/dir.c
@@ -1234,12 +1234,17 @@ int remove_dir_recursively(struct strbuf *path, int flag)
 void setup_standard_excludes(struct dir_struct *dir)
 {
 	const char *path;
+	char *xdg_path;
 
 	dir->exclude_per_dir = ".gitignore";
 	path = git_path("info/exclude");
+	if (!excludes_file) {
+		home_config_paths(NULL, &xdg_path, "ignore");
+		excludes_file = xdg_path;
+	}
 	if (!access(path, R_OK))
 		add_excludes_from_file(dir, path);
-	if (excludes_file && !access(excludes_file, R_OK))
+	if (!access(excludes_file, R_OK))
 		add_excludes_from_file(dir, excludes_file);
 }
 
diff --git a/t/t1306-xdg-files.sh b/t/t1306-xdg-files.sh
index 5b971d9..05103f5 100755
--- a/t/t1306-xdg-files.sh
+++ b/t/t1306-xdg-files.sh
@@ -67,4 +67,33 @@ test_expect_success 'read with --list: xdg file exists and ~/.gitconfig exists'
 '
 
 
+test_expect_success 'Setup' '
+	git init git &&
+	cd git &&
+	echo foo >to_be_excluded
+'
+
+
+test_expect_success 'Exclusion of a file in the XDG ignore file' '
+	mkdir -p "$HOME"/.config/git/ &&
+	echo to_be_excluded >"$HOME"/.config/git/ignore &&
+	test_must_fail git add to_be_excluded
+'
+
+
+test_expect_success 'Exclusion in both XDG and local ignore files' '
+	echo to_be_excluded >.gitignore &&
+	test_must_fail git add to_be_excluded
+'
+
+
+test_expect_success 'Exclusion in a non-XDG global ignore file' '
+	rm .gitignore &&
+	echo >"$HOME"/.config/git/ignore &&
+	echo to_be_excluded >"$HOME"/my_gitignore &&
+	git config core.excludesfile "$HOME"/my_gitignore &&
+	test_must_fail git add to_be_excluded
+'
+
+
 test_done
-- 
1.7.11.rc3.235.gd0d1d08
Matthieu Moy· Jun 22, 2012, 09:03 UTC · re: Matthieu Moy · lore

[PATCH 3/4 v8] Let core.attributesfile default to $XDG_CONFIG_HOME/git/ignore

From: Huynh Khoi Nguyen Nguyen <Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr>
To use the feature of core.attributesfile, the user needs:
 1. to create such a file,
 2. and add configuration variable to point at it.

Instead, we can make this a one-step process by choosing a default value which points to a filename in the user's $HOME, that is unlikely to already exist on the system, and only use the presence of the file as a cue that the user wants to use that feature.

We use "${XDG_CONFIG_HOME:-$HOME/.config/git}/ignore" as a default value. It goes in the same directory as the newly added configuration file ("${XDG_CONFIG_HOME:-$HOME/.config/git}/config), and follows the XDG specification.

Signed-off-by: Huynh Khoi Nguyen Nguyen <Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr>
Signed-off-by: Valentin Duperray <Valentin.Duperray@ensimag.imag.fr>
Signed-off-by: Franck Jonas <Franck.Jonas@ensimag.imag.fr>
Signed-off-by: Lucien Kong <Lucien.Kong@ensimag.imag.fr>
Signed-off-by: Thomas Nguy <Thomas.Nguy@ensimag.imag.fr>
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---

The commit message is amlost identical to the previous one to make the patch self-contained, but this can be changed to something shorter like "like the previous patch, we set the default avlue ..." if needed.

 Documentation/config.txt        |  4 +++-
 Documentation/gitattributes.txt |  2 ++
 attr.c                          | 17 ++++++++++-------
 t/t1306-xdg-files.sh            | 29 +++++++++++++++++++++++++++++
 4 files changed, 44 insertions(+), 8 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 20e9531..db07459 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -500,7 +500,9 @@ core.attributesfile::
 	In addition to '.gitattributes' (per-directory) and
 	'.git/info/attributes', git looks into this file for attributes
 	(see linkgit:gitattributes[5]). Path expansions are made the same
-	way as for `core.excludesfile`.
+	way as for `core.excludesfile`. Its default value is
+	$XDG_CONFIG_HOME/git/attributes. If $XDG_CONFIG_HOME is either not
+	set or empty, $HOME/.config/git/attributes is used instead.
 
 core.editor::
 	Commands such as `commit` and `tag` that lets you edit
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 80120ea..e16f3e1 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -75,6 +75,8 @@ repositories (i.e., attributes of interest to all users) should go into
 `.gitattributes` files. Attributes that should affect all repositories
 for a single user should be placed in a file specified by the
 `core.attributesfile` configuration option (see linkgit:git-config[1]).
+Its default value is $XDG_CONFIG_HOME/git/attributes. If $XDG_CONFIG_HOME
+is either not set or empty, $HOME/.config/git/attributes is used instead.
 Attributes for all users on a system should be placed in the
 `$(prefix)/etc/gitattributes` file.
 
diff --git a/attr.c b/attr.c
index 303751f..aef93d8 100644
--- a/attr.c
+++ b/attr.c
@@ -497,6 +497,7 @@ static int git_attr_system(void)
 static void bootstrap_attr_stack(void)
 {
 	struct attr_stack *elem;
+	char *xdg_attributes_file;
 
 	if (attr_stack)
 		return;
@@ -515,13 +516,15 @@ static void bootstrap_attr_stack(void)
 		}
 	}
 
-	if (git_attributes_file) {
-		elem = read_attr_from_file(git_attributes_file, 1);
-		if (elem) {
-			elem->origin = NULL;
-			elem->prev = attr_stack;
-			attr_stack = elem;
-		}
+	if (!git_attributes_file) {
+		home_config_paths(NULL, &xdg_attributes_file, "attributes");
+		git_attributes_file = xdg_attributes_file;
+	}
+	elem = read_attr_from_file(git_attributes_file, 1);
+	if (elem) {
+		elem->origin = NULL;
+		elem->prev = attr_stack;
+		attr_stack = elem;
 	}
 
 	if (!is_bare_repository() || direction == GIT_ATTR_INDEX) {
diff --git a/t/t1306-xdg-files.sh b/t/t1306-xdg-files.sh
index 05103f5..e8cd78a 100755
--- a/t/t1306-xdg-files.sh
+++ b/t/t1306-xdg-files.sh
@@ -96,4 +96,33 @@ test_expect_success 'Exclusion in a non-XDG global ignore file' '
 '
 
 
+test_expect_success 'Checking attributes in the XDG attributes file' '
+	echo foo >f &&
+	git check-attr -a f >actual &&
+	test_line_count -eq 0 actual &&
+	echo "f attr_f" >"$HOME"/.config/git/attributes &&
+	echo "f: attr_f: set" >expected &&
+	git check-attr -a f >actual &&
+	test_cmp expected actual
+'
+
+
+test_expect_success 'Checking attributes in both XDG and local attributes files' '
+	echo "f -attr_f" >.gitattributes &&
+	echo "f: attr_f: unset" >expected &&
+	git check-attr -a f >actual &&
+	test_cmp expected actual
+'
+
+
+test_expect_success 'Checking attributes in a non-XDG global attributes file' '
+	test_might_fail rm .gitattributes &&
+	echo "f attr_f=test" >"$HOME"/my_gitattributes &&
+	git config core.attributesfile "$HOME"/my_gitattributes &&
+	echo "f: attr_f: test" >expected &&
+	git check-attr -a f >actual &&
+	test_cmp expected actual
+'
+
+
 test_done
-- 
1.7.11.rc3.235.gd0d1d08
Junio C Hamano· Jun 22, 2012, 21:20 UTC · re: Matthieu Moy · lore

Re: [PATCH 3/4 v8] Let core.attributesfile default to $XDG_CONFIG_HOME/git/ignore

Matthieu Moy <Matthieu.Moy@imag.fr> writes:
Show 17 quoted lines
> From: Huynh Khoi Nguyen Nguyen <Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr>
>
> To use the feature of core.attributesfile, the user needs:
>
>  1. to create such a file,
>
>  2. and add configuration variable to point at it.
>
> Instead, we can make this a one-step process by choosing a default value
> which points to a filename in the user's $HOME, that is unlikely to
> already exist on the system, and only use the presence of the file as a
> cue that the user wants to use that feature.
>
> We use "${XDG_CONFIG_HOME:-$HOME/.config/git}/ignore" as a default value.
> It goes in the same directory as the newly added configuration
> file ("${XDG_CONFIG_HOME:-$HOME/.config/git}/config), and follows the XDG
> specification.

The patch text may be OK but look at the Subject: line and notice something funny ;-)

Given the root cause of that "something funny", I'll reduce the log message of this one down to:

    Let core.attributesfile default to $XDG_CONFIG_HOME/git/attributes
    
    This gives the default value for the core.attributesfile variable
    following the exact same logic of the previous change for the
    core.excludesfile setting.
    
    Signed-off-by: Huynh Khoi Nguyen Nguyen <Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr>
    ...
    Signed-off-by: Junio C Hamano <gitster@pobox.com>
I am not sure about use of git/ignore in [2/2], though.

Shouldn't the default value for core.excludesfile be git/exclude, not git/ignore?

Matthieu Moy· Jun 25, 2012, 06:32 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/4 v8] Let core.attributesfile default to $XDG_CONFIG_HOME/git/ignore

Junio C Hamano <gitster@pobox.com> writes:
> The patch text may be OK but look at the Subject: line and notice
> something funny ;-)
Nice catch.
Show 12 quoted lines
> Given the root cause of that "something funny", I'll reduce the log
> message of this one down to:
>
>     Let core.attributesfile default to $XDG_CONFIG_HOME/git/attributes
>     
>     This gives the default value for the core.attributesfile variable
>     following the exact same logic of the previous change for the
>     core.excludesfile setting.
>     
>     Signed-off-by: Huynh Khoi Nguyen Nguyen <Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr>
>     ...
>     Signed-off-by: Junio C Hamano <gitster@pobox.com>
Good (I see you did the change yourself in pu).
> I am not sure about use of git/ignore in [2/2], though.
>
> Shouldn't the default value for core.excludesfile be git/exclude,
> not git/ignore?

Both are valid options, and unfortunately, Git currently use both "ignore" and "exclude" to mean the same thing.

I have a slight preference for "ignore", because this file is the user-wide version of the .gitignore file. I think most non-advanced users have already been exposed to the wording "ignore" with this file, but much less have been exposed to "exclude" (for example, I don't remember having ever used the per-directory .git/info/exclude). So, going for "ignore" means that the wording used for the most common operations will be consistant, and only relatively advanced users (who use .git/info/exclude, or "git ls-files --exclude-standard", or need to set core.excludesfile to some value other than the default) will need to learn that "exclude" is synonym for "ignore" in the Git world.

Actually, it would even make sense to reduce the number of occurences of "excludes" in the UI, e.g. support something like core.ignoresfile as an alias for core.excludesfile, along the lines of:

--- a/config.c
+++ b/config.c
@@ -723,7 +723,7 @@ static int git_default_core_config(const char *var, const char *value)
        if (!strcmp(var, "core.askpass"))
                return git_config_string(&askpass_program, var, value);
 
-       if (!strcmp(var, "core.excludesfile"))
+       if (!strcmp(var, "core.excludesfile") || !strcmp(var, "core.ignoresfile"))
                return git_config_pathname(&excludes_file, var, value);
 
        if (!strcmp(var, "core.whitespace")) {

(which would off course require some documentation)
-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Junio C Hamano· Jun 25, 2012, 07:22 UTC · re: Matthieu Moy · lore

Re: [PATCH 3/4 v8] Let core.attributesfile default to $XDG_CONFIG_HOME/git/ignore

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
Show 13 quoted lines
> Actually, it would even make sense to reduce the number of occurences of
> "excludes" in the UI, e.g. support something like core.ignoresfile as an
> alias for core.excludesfile, along the lines of:
>
> --- a/config.c
> +++ b/config.c
> @@ -723,7 +723,7 @@ static int git_default_core_config(const char *var, const char *value)
>         if (!strcmp(var, "core.askpass"))
>                 return git_config_string(&askpass_program, var, value);
>  
> -       if (!strcmp(var, "core.excludesfile"))
> +       if (!strcmp(var, "core.excludesfile") || !strcmp(var, "core.ignoresfile"))
>                 return git_config_pathname(&excludes_file, var, value);

I somehow suspect this is going backwards. ".gitignore" may have been the original mistake; it is much more than ".cvsignore".

But I do not deeply care either way. It just felt $path/git/ignore was out of place given $path/.git/info/exclude has been with us almost forever.

Matthieu Moy· Jun 25, 2012, 07:56 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/4 v8] Let core.attributesfile default to $XDG_CONFIG_HOME/git/ignore

Junio C Hamano <gitster@pobox.com> writes:
> I somehow suspect this is going backwards.  ".gitignore" may have
> been the original mistake; it is much more than ".cvsignore".

I don't think they are _that_ different, and if we're talking about how other VCS call this, I can think of ".hgignore", ".bzrignore", ...

Even within the Git world, see how "man git-add" describes the behavior of ignore/exclude:

       The git add command will not add ignored files by default. If any
       ignored files were explicitly specified on the command line, git
       add will fail with a list of ignored files. Ignored files reached
       by directory recursion or filename globbing performed by Git
       (quote your globs before the shell) will be silently ignored. The
       git add command can be used to add ignored files with the -f
       (force) option.
Another fun fact: google "git exclude" gives this as first result
  http://www.kernel.org/pub/software/scm/git/docs/gitignore.html
;-)
-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Matthieu Moy· Jun 22, 2012, 09:03 UTC · re: Matthieu Moy · lore

[PATCH 4/4 v8] config: write to $XDG_CONFIG_HOME/git/config file if appropriate

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

Git will be able to write to $XDG_CONFIG_HOME/git/config, a new configuration file following XDG specification, if:

 1. this file already exists, and
 2. $HOME/.gitconfig file doesn't, and
 3. The --global option is used.

Otherwise Git writes to $HOME/.gitconfig with the --global option, as usual. If the user doesn't create $XDG_CONFIG_HOME/git/config, there is absolutely no change. Users can use this new file only if they want.

If $XDG_CONFIG_HOME is either not set or empty, $HOME/.config/git/config will be used.

Advice for users who often come back to an old version of Git: you shouldn't create this file.

Signed-off-by: Huynh Khoi Nguyen Nguyen <Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr>
Signed-off-by: Valentin Duperray <Valentin.Duperray@ensimag.imag.fr>
Signed-off-by: Franck Jonas <Franck.Jonas@ensimag.imag.fr>
Signed-off-by: Lucien Kong <Lucien.Kong@ensimag.imag.fr>
Signed-off-by: Thomas Nguy <Thomas.Nguy@ensimag.imag.fr>
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 Documentation/git-config.txt |  3 ++-
 builtin/config.c             |  5 +----
 t/t1306-xdg-files.sh         | 30 ++++++++++++++++++++++++++++++
 3 files changed, 33 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 7e344a2..fb10082 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -97,7 +97,8 @@ OPTIONS
 
 --global::
 	For writing options: write to global ~/.gitconfig file rather than
-	the repository .git/config.
+	the repository .git/config, write to $XDG_CONFIG_HOME/git/config file
+	if this file exists and the ~/.gitconfig file doesn't.
 +
 For reading options: read only from global ~/.gitconfig and from
 $XDG_CONFIG_HOME/git/config rather than from all available files.
diff --git a/builtin/config.c b/builtin/config.c
index da54fd1..e8e1c0a 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -387,10 +387,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 
 		home_config_paths(&user_config, &xdg_config, "config");
 
-		if (access(user_config, R_OK) && !access(xdg_config, R_OK) &&
-		    (actions == ACTION_LIST ||
-		     actions == ACTION_GET_COLOR ||
-		     actions == ACTION_GET_COLORBOOL))
+		if (access(user_config, R_OK) && !access(xdg_config, R_OK))
 			given_config_file = xdg_config;
 		else if (user_config)
 			given_config_file = user_config;
diff --git a/t/t1306-xdg-files.sh b/t/t1306-xdg-files.sh
index e8cd78a..3c75c3f 100755
--- a/t/t1306-xdg-files.sh
+++ b/t/t1306-xdg-files.sh
@@ -125,4 +125,34 @@ test_expect_success 'Checking attributes in a non-XDG global attributes file' '
 '
 
 
+test_expect_success 'write: xdg file exists and ~/.gitconfig doesn'\''t' '
+	mkdir -p "$HOME"/.config/git &&
+	>"$HOME"/.config/git/config &&
+	test_might_fail rm "$HOME"/.gitconfig &&
+	git config --global user.name "write_config" &&
+	echo "[user]" >expected &&
+	echo "	name = write_config" >>expected &&
+	test_cmp expected "$HOME"/.config/git/config
+'
+
+
+test_expect_success 'write: xdg file exists and ~/.gitconfig exists' '
+	>"$HOME"/.gitconfig &&
+	git config --global user.name "write_gitconfig" &&
+	echo "[user]" >expected &&
+	echo "	name = write_gitconfig" >>expected &&
+	test_cmp expected "$HOME"/.gitconfig
+'
+
+
+test_expect_success 'write: ~/.config/git/ exists and config file doesn'\''t' '
+	test_might_fail rm "$HOME"/.gitconfig &&
+	test_might_fail rm "$HOME"/.config/git/config &&
+	git config --global user.name "write_gitconfig" &&
+	echo "[user]" >expected &&
+	echo "	name = write_gitconfig" >>expected &&
+	test_cmp expected "$HOME"/.gitconfig
+'
+
+
 test_done
-- 
1.7.11.rc3.235.gd0d1d08
Junio C Hamano· Jun 22, 2012, 21:20 UTC · re: Matthieu Moy · lore

Re: [PATCH 4/4 v8] config: write to $XDG_CONFIG_HOME/git/config file if appropriate

Matthieu Moy <Matthieu.Moy@imag.fr> writes:
Show 6 quoted lines
> From: Huynh Khoi Nguyen Nguyen <Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr>
>
> Git will be able to write to $XDG_CONFIG_HOME/git/config, a new
> ...
> Advice for users who often come back to an old version of Git: you
> shouldn't create this file.

Hrm, is there a better way to give this advice to the _users_? The log message is primarily for people who need to dig the development history of Git, and they are not the people who will be hurt by this change. Perhaps the change to Documentation/git-config.txt in this patch needs to be a bit more detailed?

Ideally, it would be nice if we could do the usual "advise" thing by detecting a possible confusion, but I do not offhand see a clever way to do so.

Matthieu Moy· Jun 25, 2012, 06:45 UTC · re: Junio C Hamano · lore

Re: [PATCH 4/4 v8] config: write to $XDG_CONFIG_HOME/git/config file if appropriate

Junio C Hamano <gitster@pobox.com> writes:
Show 14 quoted lines
> Matthieu Moy <Matthieu.Moy@imag.fr> writes:
>
>> From: Huynh Khoi Nguyen Nguyen <Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr>
>>
>> Git will be able to write to $XDG_CONFIG_HOME/git/config, a new
>> ...
>> Advice for users who often come back to an old version of Git: you
>> shouldn't create this file.
>
> Hrm, is there a better way to give this advice to the _users_?  The
> log message is primarily for people who need to dig the development
> history of Git, and they are not the people who will be hurt by this
> change.  Perhaps the change to Documentation/git-config.txt in this
> patch needs to be a bit more detailed?
We can squash this into PATCH 1:
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -209,7 +209,9 @@ $XDG_CONFIG_HOME/git/config::
        Second user-specific configuration file. If $XDG_CONFIG_HOME is not set
        or empty, $HOME/.config/git/config will be used. Any single-valued
        variable set in this file will be overwritten by whatever is in
-       ~/.gitconfig.
+       ~/.gitconfig. Support for this file was added after Git
+       1.7.11, so if you sometimes use older versions of Git, it's a
+       good idea not to create this file.
 
 $(prefix)/etc/gitconfig::
        System-wide configuration file.

(If your cristal ball tells you what version number will follow 1.7.11
(1.7.12 or 1.8.0?), we can change "after 1.7.11" to "in X". Do you want
to squash it yourself, or shall I resend?

> Ideally, it would be nice if we could do the usual "advise" thing
> by detecting a possible confusion, but I do not offhand see a clever
> way to do so.

I don't see either, since the advice should be given only to people who
use various versions of Git.

Anyway, I don't think we should bother too much with this: it's really
an opt-in feature, Git will never create the new file for you (it will
only write in it if it exists). So, people potentially harmed by this
change are only those who read the documentation. If the documentation
warns them, I think it's sufficient. It's not very different from any
new feature actually.
-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Junio C Hamano· Jun 25, 2012, 18:08 UTC · re: Matthieu Moy · lore

Re: [PATCH 4/4 v8] config: write to $XDG_CONFIG_HOME/git/config file if appropriate

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
Show 12 quoted lines
> We can squash this into PATCH 1:
>
> --- a/Documentation/git-config.txt
> +++ b/Documentation/git-config.txt
> @@ -209,7 +209,9 @@ $XDG_CONFIG_HOME/git/config::
>         Second user-specific configuration file. If $XDG_CONFIG_HOME is not set
>         or empty, $HOME/.config/git/config will be used. Any single-valued
>         variable set in this file will be overwritten by whatever is in
> -       ~/.gitconfig.
> +       ~/.gitconfig. Support for this file was added after Git
> +       1.7.11, so if you sometimes use older versions of Git, it's a
> +       good idea not to create this file.
Okay.  Thanks.
Junio C Hamano· Jun 22, 2012, 21:19 UTC · re: Matthieu Moy · lore

Re: [PATCH 0/4 v8] Git configuration directory

Matthieu Moy <Matthieu.Moy@imag.fr> writes:
> I took the liberty of rewording the commit messages, and change a few
> words in the documentation (changed some "will be used" to "is used
> instead", which sounded more consistant with the context).
Thanks; the series overall looks good.

I may have made further tweaks by the time I push the result out. Please watch 'pu' and holler if you find anything objectionable.

Show 44 quoted lines
> A reminder of the context and objectives of the serie:
>
> Git currently stores its configuration file in ~/.gitconfig, which is
> nice and customary on Unix, as long as one has only one configuration
> file.
>
> But a typical user may want to store not only the config file, but
> also the files pointed to by core.excludesfile and
> core.attributesfile, which currently have no default values. To store
> several configuration files, it makes sense to have a configuration
> _directory_ instead of a set of configuration files, all right into
> $HOME. Calling this configuration directory ~/.git or ~/.gitconfig is
> not an option, since these paths already have another meaning. Using
> the XDG specification (in short: ~/.config/git) doesn't have this
> drawback, and allows the user to store his configuration files right
> next to files from other applications following the XDG standard.
>
> The first 3 patches add read support for this "configuration
> directory", and the last one gives opt-in write support, allowing
> users to make this "configuration directory" their way of life, and
> never hear again about ~/.gitconfig if (and only if) they do not want to.
>
> None of the patches change the behavior for people who do not create
> the new configuration files.
>
> Huynh Khoi Nguyen Nguyen (4):
>   config: read (but not write) from $XDG_CONFIG_HOME/git/config file
>   Let core.excludesfile default to $XDG_CONFIG_HOME/git/ignore
>   Let core.attributesfile default to $XDG_CONFIG_HOME/git/ignore
>   config: write to $XDG_CONFIG_HOME/git/config file if appropriate
>
>  Documentation/config.txt        |   8 +-
>  Documentation/git-config.txt    |  15 +++-
>  Documentation/gitattributes.txt |   2 +
>  Documentation/gitignore.txt     |   4 +-
>  attr.c                          |  17 +++--
>  builtin/config.c                |  25 ++++---
>  cache.h                         |   3 +
>  config.c                        |  23 +++---
>  dir.c                           |   7 +-
>  path.c                          |  41 +++++++++++
>  t/t1306-xdg-files.sh            | 158 ++++++++++++++++++++++++++++++++++++++++
>  11 files changed, 270 insertions(+), 33 deletions(-)
>  create mode 100755 t/t1306-xdg-files.sh
Ramsay Jones· Jun 4, 2012, 17:54 UTC · re: Huynh Khoi Nguyen NGUYEN · lore

Re: [PATCHv4] Read (but not write) from XDG configuration, XDG attributes and XDG ignore files

Huynh Khoi Nguyen NGUYEN wrote:
Show 30 quoted lines
> From: NGUYEN Huynh Khoi Nguyen <nguyenhu@ensibm.imag.fr>
> 
> Git will be able to read in $XDG_CONFIG_HOME/git/config, a new
> configuration file following XDG specification. In the order of
> reading, this file is between global configuration file and system
> wide configuration file. Git will not be able to write in this new
> configuration file. If core.excludesfile is not define, Git will read
> the global exclude files in $XDG_CONFIG_HOME/git/ignore. Same goes for
> core.attributesfile in $XDG_CONFIG_HOME/git/attributes. If
> $XDG_CONFIG_HOME is either not set or empty, $HOME/.config will be
> used.
> 
> Signed-off-by: Huynh Khoi Nguyen NGUYEN <Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr>
> Signed-off-by: Lucien KONG <Lucien.Kong@ensimag.imag.fr>
> Signed-off-by: Valentin DUPERRAY <Valentin.Duperray@ensimag.imag.fr>
> Signed-off-by: Thomas NGUY <Thomas.Nguy@ensimag.imag.fr>
> Signed-off-by: Franck JONAS <Franck.Jonas@ensimag.imag.fr>
> Signed-off-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
> ---
>  Documentation/git-config.txt    |   12 +++-
>  attr.c                          |   10 +++
>  builtin/config.c                |   28 ++++++---
>  cache.h                         |    1 +
>  config.c                        |   21 ++++---
>  dir.c                           |    4 +
>  path.c                          |   26 ++++++++
>  t/t1306-read-xdg-config-file.sh |  133 +++++++++++++++++++++++++++++++++++++++
>  8 files changed, 214 insertions(+), 21 deletions(-)
>  create mode 100755 t/t1306-read-xdg-config-file.sh
> 
[...]
Show 11 quoted lines
> diff --git a/attr.c b/attr.c
> index 303751f..441387f 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -497,6 +497,9 @@ static int git_attr_system(void)
>  static void bootstrap_attr_stack(void)
>  {
>  	struct attr_stack *elem;
> +	char *xdg_attributes_file;
> +
> +	home_config_paths(NULL, &xdg_attributes_file, "attributes");
who free()'s xdg_attributes_file ?
[...]
Show 13 quoted lines
> diff --git a/config.c b/config.c
> index 71ef171..d1393b8 100644
> --- a/config.c
> +++ b/config.c
> @@ -929,7 +929,10 @@ int git_config_system(void)
>  int git_config_early(config_fn_t fn, void *data, const char *repo_config)
>  {
>  	int ret = 0, found = 0;
> -	const char *home = NULL;
> +	char *xdg_config = NULL;
> +	char *user_config = NULL;
> +
> +	home_config_paths(&user_config, &xdg_config, "config");
who free()'s user_config and xdg_config?
[...]
Show 13 quoted lines
> diff --git a/dir.c b/dir.c
> index ed1510f..e0c3589 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -1234,13 +1234,17 @@ int remove_dir_recursively(struct strbuf *path, int flag)
>  void setup_standard_excludes(struct dir_struct *dir)
>  {
>  	const char *path;
> +	char *xdg_path;
>  
>  	dir->exclude_per_dir = ".gitignore";
>  	path = git_path("info/exclude");
> +	home_config_paths(NULL, &xdg_path, "ignore");
ditto xdg_path ;-)
Show 43 quoted lines
>  	if (!access(path, R_OK))
>  		add_excludes_from_file(dir, path);
>  	if (excludes_file && !access(excludes_file, R_OK))
>  		add_excludes_from_file(dir, excludes_file);
> +	else if (!access(xdg_path, R_OK))
> +		add_excludes_from_file(dir, xdg_path);
>  }
>  
>  int remove_path(const char *name)
> diff --git a/path.c b/path.c
> index 6f2aa69..53f3f53 100644
> --- a/path.c
> +++ b/path.c
> @@ -122,6 +122,32 @@ char *git_path(const char *fmt, ...)
>  	return cleanup_path(pathname);
>  }
>  
> +void home_config_paths(char **global, char **xdg, char *file)
> +{
> +	char *xdg_home = getenv("XDG_CONFIG_HOME");
> +	char *home = getenv("HOME");
> +	char *to_free = NULL;
> +
> +	if (!home) {
> +		if (global)
> +			*global = NULL;
> +	} else {
> +		if (!xdg_home) {
> +			to_free = strdup(mkpath("%s/.config", home));
> +			xdg_home = to_free;
> +		}
> +		if (global)
> +			*global = xstrdup(mkpath("%s/.gitconfig", home));
> +	}
> +
> +	if (!xdg_home)
> +		*xdg = NULL;
> +	else
> +		*xdg = xstrdup(mkpath("%s/git/%s", xdg_home, file));
> +
> +	free(to_free);
> +}
> +

So, this re-introduces the bug addressed by commit 05bab3ea. The test number is now 29 (rather than 21) but the same test is failing; namely t3200-branch.sh test #29 (git branch -m q q2 without config should succeed).

In order to fix the bug, I created the patch given below (on top of this patch). (Note that it does not address the above issues).

HTH

ATB, Ramsay Jones

-- >8 --
From: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
Subject: [PATCH] path.c: Fix a static buffer overwrite bug by avoiding mkpath()

The v4 version of the "Read (but not write) from XDG configuration, XDG attributes and XDG ignore files" patch, re-introduced the bug addressed by commit 05bab3ea ("config.c: Fix a static buffer overwrite bug by avoiding mkpath()", 19-11-2011). Note that the patch refactored the code to determine the user (or home) configuration filename into a new function (home_config_paths()). In doing so, the new code once again uses mkpath() rather than mksnpath().

In order to fix the bug, we introduce a new variation of the mkpath() function, mkpathdup(), which avoids the use of the internal static buffers. As the name implies, the new function returns a pointer to the pathname as a dynamically allocated string. It is the callers responsibility to free the memory for the returned string.

Having introduced the new function, we can now replace the calls to 'xstrdup(mkpath(...))' in the home_config_paths() function with a call to mkpathdup() to achieve the same effect, without tickling the original bug.

(Also, note that the 'xstrdup(mkpath(...))' pattern occurs in several other places in the source ...)

Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
---
 cache.h |    2 ++
 path.c  |   20 +++++++++++++++++---
 2 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/cache.h b/cache.h
index 0632503..fbba2d6 100644
--- a/cache.h
+++ b/cache.h
@@ -619,6 +619,8 @@ extern char *git_snpath(char *buf, size_t n, const char *fmt, ...)
 	__attribute__((format (printf, 3, 4)));
 extern char *git_pathdup(const char *fmt, ...)
 	__attribute__((format (printf, 1, 2)));
+extern char *mkpathdup(const char *fmt, ...)
+	__attribute__((format (printf, 1, 2)));
 
 /* Return a statically allocated filename matching the sha1 signature */
 extern char *mkpath(const char *fmt, ...) __attribute__((format (printf, 1, 2)));
diff --git a/path.c b/path.c
index 53f3f53..ca29bdd 100644
--- a/path.c
+++ b/path.c
@@ -87,6 +87,20 @@ char *git_pathdup(const char *fmt, ...)
 	return xstrdup(path);
 }
 
+char *mkpathdup(const char *fmt, ...)
+{
+	char path[PATH_MAX];
+	va_list args;
+	unsigned len;
+
+	va_start(args, fmt);
+	len = vsnprintf(path, sizeof(path), fmt, args);
+	va_end(args);
+	if (len >= sizeof(path))
+		return xstrdup(bad_path);
+	return xstrdup(cleanup_path(path));
+}
+
 char *mkpath(const char *fmt, ...)
 {
 	va_list args;
@@ -133,17 +147,17 @@ void home_config_paths(char **global, char **xdg, char *file)
 			*global = NULL;
 	} else {
 		if (!xdg_home) {
-			to_free = strdup(mkpath("%s/.config", home));
+			to_free = mkpathdup("%s/.config", home);
 			xdg_home = to_free;
 		}
 		if (global)
-			*global = xstrdup(mkpath("%s/.gitconfig", home));
+			*global = mkpathdup("%s/.gitconfig", home);
 	}
 
 	if (!xdg_home)
 		*xdg = NULL;
 	else
-		*xdg = xstrdup(mkpath("%s/git/%s", xdg_home, file));
+		*xdg = mkpathdup("%s/git/%s", xdg_home, file);
 
 	free(to_free);
 }
-- 
1.7.10
Junio C Hamano· Jun 4, 2012, 18:41 UTC · re: Ramsay Jones · lore

Re: [PATCHv4] Read (but not write) from XDG configuration, XDG attributes and XDG ignore files

Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:
Show 5 quoted lines
> Subject: [PATCH] path.c: Fix a static buffer overwrite bug by avoiding mkpath()
>
> In order to fix the bug, we introduce a new variation of the mkpath()
> function, mkpathdup(), which avoids the use of the internal static
> buffers.

Shouldn't we aim a bit higher to also avoid the use of bounded buffer? Instead of returning bad_path, retry with lengthened buffer until we succeed, or something.

Better yet, internally use strbuf_vaddf().
Ramsay Jones· Jun 12, 2012, 17:32 UTC · re: Junio C Hamano · lore

Re: [PATCHv4] Read (but not write) from XDG configuration, XDG attributes and XDG ignore files

Junio C Hamano wrote:
Show 13 quoted lines
> Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:
> 
>> Subject: [PATCH] path.c: Fix a static buffer overwrite bug by avoiding mkpath()
>>
>> In order to fix the bug, we introduce a new variation of the mkpath()
>> function, mkpathdup(), which avoids the use of the internal static
>> buffers.
> 
> Shouldn't we aim a bit higher to also avoid the use of bounded
> buffer?  Instead of returning bad_path, retry with lengthened buffer
> until we succeed, or something.
> 
> Better yet, internally use strbuf_vaddf().
Sorry for the late reply, I've been away ...

Yes, I wasn't aiming too high. In fact I was only aiming for "I spent the last 20 minutes fixing up your patch so that it doesn't tickle the bug on cygwin, and it looks like this..." :-P

While away, I did rewrite mkpathdup() to address your concerns (although I didn't use strbuf_vaddf()).

However, I see that Nguyen has not been idle and, with help from others, has fixed up the patch and re-rolled the series (v7 I think). I haven't fetched that mail yet, but it looked good and should not tickle the cygwin bug. I will test it soon.

ATB, Ramsay Jones

nguyenhu@minatec.inpg.fr· Jun 5, 2012, 12:19 UTC · re: Ramsay Jones · lore

Re: [PATCHv4] Read (but not write) from XDG configuration, XDG attributes and XDG ignore files

Ramsay Jones <ramsay@ramsay1.demon.co.uk> a écrit :
Show 113 quoted lines
>
> So, this re-introduces the bug addressed by commit 05bab3ea. The test number
> is now 29 (rather than 21) but the same test is failing; namely  
> t3200-branch.sh
> test #29 (git branch -m q q2 without config should succeed).
>
> In order to fix the bug, I created the patch given below (on top of  
> this patch).
> (Note that it does not address the above issues).
>
> HTH
>
> ATB,
> Ramsay Jones
>
> -- >8 --
> From: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
> Subject: [PATCH] path.c: Fix a static buffer overwrite bug by  
> avoiding mkpath()
>
> The v4 version of the "Read (but not write) from XDG configuration,
> XDG attributes and XDG ignore files" patch, re-introduced the bug
> addressed by commit 05bab3ea ("config.c: Fix a static buffer overwrite
> bug by avoiding mkpath()", 19-11-2011). Note that the patch refactored
> the code to determine the user (or home) configuration filename into
> a new function (home_config_paths()). In doing so, the new code once
> again uses mkpath() rather than mksnpath().
>
> In order to fix the bug, we introduce a new variation of the mkpath()
> function, mkpathdup(), which avoids the use of the internal static
> buffers. As the name implies, the new function returns a pointer to
> the pathname as a dynamically allocated string. It is the callers
> responsibility to free the memory for the returned string.
>
> Having introduced the new function, we can now replace the calls to
> 'xstrdup(mkpath(...))' in the home_config_paths() function with a
> call to mkpathdup() to achieve the same effect, without tickling the
> original bug.
>
> (Also, note that the 'xstrdup(mkpath(...))' pattern occurs in several
> other places in the source ...)
>
> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
> ---
>  cache.h |    2 ++
>  path.c  |   20 +++++++++++++++++---
>  2 files changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/cache.h b/cache.h
> index 0632503..fbba2d6 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -619,6 +619,8 @@ extern char *git_snpath(char *buf, size_t n,  
> const char *fmt, ...)
>  	__attribute__((format (printf, 3, 4)));
>  extern char *git_pathdup(const char *fmt, ...)
>  	__attribute__((format (printf, 1, 2)));
> +extern char *mkpathdup(const char *fmt, ...)
> +	__attribute__((format (printf, 1, 2)));
>
>  /* Return a statically allocated filename matching the sha1 signature */
>  extern char *mkpath(const char *fmt, ...) __attribute__((format  
> (printf, 1, 2)));
> diff --git a/path.c b/path.c
> index 53f3f53..ca29bdd 100644
> --- a/path.c
> +++ b/path.c
> @@ -87,6 +87,20 @@ char *git_pathdup(const char *fmt, ...)
>  	return xstrdup(path);
>  }
>
> +char *mkpathdup(const char *fmt, ...)
> +{
> +	char path[PATH_MAX];
> +	va_list args;
> +	unsigned len;
> +
> +	va_start(args, fmt);
> +	len = vsnprintf(path, sizeof(path), fmt, args);
> +	va_end(args);
> +	if (len >= sizeof(path))
> +		return xstrdup(bad_path);
> +	return xstrdup(cleanup_path(path));
> +}
> +
>  char *mkpath(const char *fmt, ...)
>  {
>  	va_list args;
> @@ -133,17 +147,17 @@ void home_config_paths(char **global, char  
> **xdg, char *file)
>  			*global = NULL;
>  	} else {
>  		if (!xdg_home) {
> -			to_free = strdup(mkpath("%s/.config", home));
> +			to_free = mkpathdup("%s/.config", home);
>  			xdg_home = to_free;
>  		}
>  		if (global)
> -			*global = xstrdup(mkpath("%s/.gitconfig", home));
> +			*global = mkpathdup("%s/.gitconfig", home);
>  	}
>
>  	if (!xdg_home)
>  		*xdg = NULL;
>  	else
> -		*xdg = xstrdup(mkpath("%s/git/%s", xdg_home, file));
> +		*xdg = mkpathdup("%s/git/%s", xdg_home, file);
>
>  	free(to_free);
>  }
> --
> 1.7.10
>

Thank you for having fixed this bug we have re-introduced with your patch, we will add your modifications in our next v6 version.

← back to recent threads