[PATCH 1/3] test-lib.sh: unset XDG_CONFIG_HOME

[PATCH 1/3] test-lib.sh: unset XDG_CONFIG_HOME

[Jeff King]

Now that git respects XDG_CONFIG_HOME for some lookups, we
must be sure to cleanse the test environment. Otherwise, the
user's XDG_CONFIG_HOME could influence the test results.

Signed-off-by: Jeff King <peff@peff.net>
---
[oops, re-sending because I forgot to cc git@vger]

You can test this trivially with:

  XDG_CONFIG_HOME=/whatever ./t1306-xdg-files.sh

 t/test-lib.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index acda33d..5e7f435 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -61,6 +61,7 @@ unset VISUAL EMAIL LANGUAGE COLUMNS $(perl -e '
 	my @vars = grep(/^GIT_/ && !/^GIT_($ok)/o, @env);
 	print join("\n", @vars);
 ')
+unset XDG_CONFIG_HOME
 GIT_AUTHOR_EMAIL=author@example.com
 GIT_AUTHOR_NAME='A U Thor'
 GIT_COMMITTER_EMAIL=committer@example.com
-- 
1.7.11.3.4.g9f70dbb

[PATCH 2/3] attr: make sure we have an xdg path before using it

[Jeff King]

If we don't have a core.attributesfile configured, we fall
back to checking XDG config. This is usually
$HOME/.config/attributes. However, if $HOME is unset, then
home_config_paths will return NULL, and we end up calling
fopen(NULL).

Depending on your system, this may or may not cause the
accompanying test to fail (e.g., on Linux and glibc, the
address will go straight to open, which will return EFAULT).
However, valgrind will reliably notice the error.

Signed-off-by: Jeff King <peff@peff.net>
---
[oops, resending because I forgot to cc git@vger]

This is another instance of Matthieu's e3ebc35 (config: fix several
access(NULL) calls, 2012-07-12). I guess it wasn't caught
because of the lack of a test without HOME set (I found it
because t5541 can trigger it in a very roundabout way).

 attr.c               | 12 +++++++-----
 t/t1306-xdg-files.sh |  6 ++++++
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/attr.c b/attr.c
index aef93d8..b52efb5 100644
--- a/attr.c
+++ b/attr.c
@@ -520,11 +520,13 @@ static void bootstrap_attr_stack(void)
 		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 (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 (!is_bare_repository() || direction == GIT_ATTR_INDEX) {
diff --git a/t/t1306-xdg-files.sh b/t/t1306-xdg-files.sh
index 3c75c3f..1569596 100755
--- a/t/t1306-xdg-files.sh
+++ b/t/t1306-xdg-files.sh
@@ -106,6 +106,12 @@ test_expect_success 'Checking attributes in the XDG attributes file' '
 	test_cmp expected actual
 '
 
+test_expect_success 'Checking XDG attributes when HOME is unset' '
+	>expected &&
+	(sane_unset HOME &&
+	 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 &&
-- 
1.7.11.3.4.g9f70dbb

[PATCH 3/3] t1306: check that XDG_CONFIG_HOME works

[Jeff King]

This should override $HOME/.config, but we never actually
tested it.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t1306-xdg-files.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/t/t1306-xdg-files.sh b/t/t1306-xdg-files.sh
index 1569596..4447949 100755
--- a/t/t1306-xdg-files.sh
+++ b/t/t1306-xdg-files.sh
@@ -113,6 +113,14 @@ test_expect_success 'Checking XDG attributes when HOME is unset' '
 	test_cmp expected actual
 '
 
+test_expect_success 'Prefer XDG_CONFIG_HOME to HOME/.config' '
+	echo "f: attr_f: foo" >expected &&
+	mkdir -p foo/git &&
+	echo "f attr_f=foo" >foo/git/attributes &&
+	XDG_CONFIG_HOME="foo" 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 &&
-- 
1.7.11.3.4.g9f70dbb

Re: [PATCH 1/3] test-lib.sh: unset XDG_CONFIG_HOME

[Matthieu Moy]

Thanks (for the 3 patches, all of them look good).

the "unset XDG_CONFIG_HOME" part was already discussed here:

  http://thread.gmane.org/gmane.comp.version-control.git/201609

But Michael did not continue the thread. I think your solution (unset
$XDG_CONFIG_HOME instead of setting it to $HOME/.config/git) is better.

In the thread above, I also proposed checking that $XDG_CONFIG_HOME was
taken into account, but for the "git config" part (while you test the
attributes part).

I think it makes sense to add stg like this to your PATCH 3:


diff --git a/t/t1306-xdg-files.sh b/t/t1306-xdg-files.sh
index 3c75c3f..f1ea9f1 100755
--- a/t/t1306-xdg-files.sh
+++ b/t/t1306-xdg-files.sh
@@ -38,6 +38,19 @@ test_expect_success 'read with --get: xdg file exists and ~/.gitconfig doesn'\''
        test_cmp expected actual
 '

+test_expect_success '"$XDG_CONFIG_HOME overrides $HOME/.config/git' '
+       mkdir -p "$HOME"/xdg/git/ &&
+       echo "[user]" >"$HOME"/xdg/git/config &&
+       echo "  name = in_xdg" >>"$HOME"/xdg/git/config &&
+       echo in_xdg >expected &&
+       (
+               XDG_CONFIG_HOME="$HOME"/xdg/ &&
+               export XDG_CONFIG_HOME &&
+               git config --get-all user.name >actual
+       ) &&
+       test_cmp expected actual
+'
+

 test_expect_success 'read with --get: xdg file exists and ~/.gitconfig exists' '
        >.gitconfig &&

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

[PATCH] ignore: make sure we have an xdg path before using it

[Matthieu Moy]

Commit e3ebc35 (config: fix several access(NULL) calls, 2012-07-12) was
fixing access(NULL) calls when trying to access $HOME/.config/git/config,
but missed the ones when trying to access $HOME/.config/git/ignore. Fix
and test this.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
This can be appended to Jeff's serie. I thought if we had 3 bug
instances and already fixed 2, why not fix the (hopefully last)
one ;-).

 dir.c                | 2 +-
 t/t1306-xdg-files.sh | 7 +++++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/dir.c b/dir.c
index a772c6d..240bf0c 100644
--- a/dir.c
+++ b/dir.c
@@ -1313,7 +1313,7 @@ void setup_standard_excludes(struct dir_struct *dir)
 	}
 	if (!access(path, R_OK))
 		add_excludes_from_file(dir, path);
-	if (!access(excludes_file, R_OK))
+	if (excludes_file && !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 097184b..6d9e8cd 100755
--- a/t/t1306-xdg-files.sh
+++ b/t/t1306-xdg-files.sh
@@ -108,6 +108,13 @@ test_expect_success 'Exclusion in a non-XDG global ignore file' '
 	test_must_fail git add to_be_excluded
 '
 
+test_expect_success 'Checking XDG ignore file when HOME is unset' '
+	>expected &&
+	(sane_unset HOME &&
+	 git config --unset core.excludesfile &&
+	 git ls-files --exclude-standard --ignored >actual) &&
+	test_cmp expected actual
+'
 
 test_expect_success 'Checking attributes in the XDG attributes file' '
 	echo foo >f &&
-- 
1.7.11.2.402.g04c525e.dirty

Re: [PATCH 1/3] test-lib.sh: unset XDG_CONFIG_HOME

[Jeff King]

On Tue, Jul 24, 2012 at 02:06:43PM +0200, Matthieu Moy wrote:

> Thanks (for the 3 patches, all of them look good).
> 
> the "unset XDG_CONFIG_HOME" part was already discussed here:
> 
>   http://thread.gmane.org/gmane.comp.version-control.git/201609
> 
> But Michael did not continue the thread. I think your solution (unset
> $XDG_CONFIG_HOME instead of setting it to $HOME/.config/git) is better.

Yeah, setting it to $HOME/.config/git is actively wrong; I agree with
the reasoning in that thread (which I did not read until just now).

> In the thread above, I also proposed checking that $XDG_CONFIG_HOME was
> taken into account, but for the "git config" part (while you test the
> attributes part).

Yeah, I see.

> I think it makes sense to add stg like this to your PATCH 3:

Agreed. And one for the exclude section, too, just for completeness.

Revised patch 3 is below.

-- >8 --
Subject: [PATCHv2 3/3] t1306: check that XDG_CONFIG_HOME works

This should override $HOME/.config, but we never actually
tested it.
---
 t/t1306-xdg-files.sh | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/t/t1306-xdg-files.sh b/t/t1306-xdg-files.sh
index 1569596..a62c3fb 100755
--- a/t/t1306-xdg-files.sh
+++ b/t/t1306-xdg-files.sh
@@ -38,6 +38,13 @@ test_expect_success 'read with --get: xdg file exists and ~/.gitconfig doesn'\''
 	test_cmp expected actual
 '
 
+test_expect_success '"$XDG_CONFIG_HOME overrides $HOME/.config/git' '
+	mkdir -p "$HOME"/xdg/git &&
+	echo "[user]name = in_xdg" >"$HOME"/xdg/git/config &&
+	echo in_xdg >expected &&
+	XDG_CONFIG_HOME="$HOME"/xdg git config --get-all user.name >actual &&
+	test_cmp expected actual
+'
 
 test_expect_success 'read with --get: xdg file exists and ~/.gitconfig exists' '
 	>.gitconfig &&
@@ -80,6 +87,17 @@ test_expect_success 'Exclusion of a file in the XDG ignore file' '
 	test_must_fail git add to_be_excluded
 '
 
+test_expect_success '$XDG_CONFIG_HOME overrides $HOME/.config/ignore' '
+	mkdir -p "$HOME"/xdg/git &&
+	echo content >excluded_by_xdg_only &&
+	echo excluded_by_xdg_only >"$HOME"/xdg/git/ignore &&
+	test_when_finished "git read-tree --empty" &&
+	(XDG_CONFIG_HOME="$HOME/xdg" &&
+	 export XDG_CONFIG_HOME &&
+	 git add to_be_excluded &&
+	 test_must_fail git add excluded_by_xdg_only
+	)
+'
 
 test_expect_success 'Exclusion in both XDG and local ignore files' '
 	echo to_be_excluded >.gitignore &&
@@ -113,6 +131,14 @@ test_expect_success 'Checking XDG attributes when HOME is unset' '
 	test_cmp expected actual
 '
 
+test_expect_success '$XDG_CONFIG_HOME overrides $HOME/.config/attributes' '
+	mkdir -p "$HOME"/xdg/git &&
+	echo "f attr_f=xdg" >"$HOME"/xdg/git/attributes &&
+	echo "f: attr_f: xdg" >expected &&
+	XDG_CONFIG_HOME="$HOME/xdg" 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 &&

Re: [PATCH] ignore: make sure we have an xdg path before using it

[Jeff King]

On Tue, Jul 24, 2012 at 02:26:51PM +0200, Matthieu Moy wrote:

> Commit e3ebc35 (config: fix several access(NULL) calls, 2012-07-12) was
> fixing access(NULL) calls when trying to access $HOME/.config/git/config,
> but missed the ones when trying to access $HOME/.config/git/ignore. Fix
> and test this.
> 
> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
> ---
> This can be appended to Jeff's serie. I thought if we had 3 bug
> instances and already fixed 2, why not fix the (hopefully last)
> one ;-).

Thanks. Looks obviously correct to me.

-Peff