threads / patch / 11911

patchRe: [PATCH] Make tests independent of global config files

Subject: Re: [PATCH] Make tests independent of global config files

## tl;dr

8 messages between Feb 6, 2008 and Feb 6, 2008. Diffs are folded; open one to read it.

replies: 7people: 2as markdown or json

Jeff King· Feb 6, 2008, 08:38 UTC · lore

On Thu, Feb 15, 2007 at 11:43:56AM +0100, Johannes Schindelin wrote:

Show 6 quoted lines

> This was done by setting $HOME to somewhere bogus. A better method is
> to reuse $GIT_CONFIG, which was invented for ignoring the global
> config file explicitely.
> 
> Technically, setting GIT_CONFIG=.git/config could be wrong, but it
> passes all the tests, and we can keep the tests that way.

[Yes, this is a reply to an ancient message, but your "could be wrong" turned out to be true.]

I ran across a bug in the test suite today, and it is caused by this change. The problem is that setting GIT_CONFIG=.git/config means that if we change directories, then we may silently fail to read the config file. Oops.

In particular, this is covering up an incorrect test in t1500. One of the things it does is more or less this:

  cd .git && git rev-parse --is-bare-repository

and expects it to say "true". The test passes. All fine and dandy, except that it _isn't_ a bare repository, and if you run the test by hand, it returns "false."

The problem is that it's looking for .git/.git/config and doesn't find it, meaning that it never sees the "core.bare = false" setting, and makes a guess from its location inside the git dir.

We could set GIT_CONFIG=$(pwd)/.git/config, but then that fails if the test creates other repos.

We could go back to setting HOME to something bogus, but then we have no way to suppress the reading of /etc/gitconfig (the only way to do so, I think, is to set GIT_CONFIG).

So I think we are stuck adding in some environment magic to suppress the reading of ETC_GITCONFIG, and doing something like:

unset GIT_CONFIG GIT_ETC_CONFIG=$(pwd)/.git/nonexistant GIT_LOCAL_CONFIG=$(pwd)/.git/nonexistant

Thoughts?

-Peff

Jeff King· Feb 6, 2008, 09:04 UTC · re: Jeff King · lore

On Wed, Feb 06, 2008 at 03:38:17AM -0500, Jeff King wrote:

Show 6 quoted lines

> So I think we are stuck adding in some environment magic to suppress the
> reading of ETC_GITCONFIG, and doing something like:
> 
> unset GIT_CONFIG
> GIT_ETC_CONFIG=$(pwd)/.git/nonexistant
> GIT_LOCAL_CONFIG=$(pwd)/.git/nonexistant

Actually, we don't need to suppress GIT_LOCAL_CONFIG in the same way; we just need to make sure it isn't set. But we do need to set HOME to avoid that lookup. Patch series will follow.

-Peff

Jeff King· Feb 6, 2008, 09:07 UTC · re: Jeff King · lore

[PATCH 1/2] allow GIT_CONFIG_ETC to override compile-time setting

This is not expected to be used by mere mortals, since the GIT_CONFIG_LOCAL variable already accomplishes more or less the same thing. However, to keep the testing environment clean, it is necessary to be able to suppress the use of this file (which may have settings that interfere with the tests).

Signed-off-by: Jeff King <peff@peff.net>
---
Junio, more rationale for the series is available in the first message
of the thread.

 cache.h  |    1 +
 config.c |    4 +++-
 2 files changed, 4 insertions(+), 1 deletions(-)

Show changes to 2 files +4 −1

cache.h, config.c

diff --git a/cache.h b/cache.h
index 549f4bb..d8322a1 100644
--- a/cache.h
+++ b/cache.h
@@ -208,6 +208,7 @@ static inline enum object_type object_type(unsigned int mode)
 #define TEMPLATE_DIR_ENVIRONMENT "GIT_TEMPLATE_DIR"
 #define CONFIG_ENVIRONMENT "GIT_CONFIG"
 #define CONFIG_LOCAL_ENVIRONMENT "GIT_CONFIG_LOCAL"
+#define CONFIG_ETC_ENVIRONMENT "GIT_CONFIG_ETC"
 #define EXEC_PATH_ENVIRONMENT "GIT_EXEC_PATH"
 #define GITATTRIBUTES_FILE ".gitattributes"
 #define INFOATTRIBUTES_FILE "info/attributes"
diff --git a/config.c b/config.c
index 498259e..2a0d005 100644
--- a/config.c
+++ b/config.c
@@ -481,7 +481,9 @@ const char *git_etc_gitconfig(void)
 {
 	static const char *system_wide;
 	if (!system_wide) {
-		system_wide = ETC_GITCONFIG;
+		system_wide = getenv(CONFIG_ETC_ENVIRONMENT);
+	        if (!system_wide)
+			system_wide = ETC_GITCONFIG;
 		if (!is_absolute_path(system_wide)) {
 			/* interpret path relative to exec-dir */
 			struct strbuf d = STRBUF_INIT;

-- 
1.5.4.26.g9be9

Jeff King· Feb 6, 2008, 09:07 UTC · re: Jeff King · lore

[PATCH 2/2] fix config reading in tests

Previously, we set the GIT_CONFIG environment variable in our tests so that only that file was read. However, setting it to a static value is not correct, since we are not necessarily always in the same directory; instead, we want the usual git config file lookup to happen.

To do this, we stop setting GIT_CONFIG, which means that we must now suppress the reading of the system-wide and user configs.

This exposes an incorrect test in t1500, which is also fixed (the incorrect test worked because we were failing to read the core.bare value from the config file, since the GIT_CONFIG variable was pointing us to the wrong file).

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t1500-rev-parse.sh |    4 ++--
 t/test-lib.sh        |    7 +++++--
 2 files changed, 7 insertions(+), 4 deletions(-)

Show changes to 2 files +7 −4

t/t1500-rev-parse.sh, t/test-lib.sh

diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index e474b3f..38a2bf0 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -33,9 +33,9 @@ test_rev_parse() {
 test_rev_parse toplevel false false true ''
 
 cd .git || exit 1
-test_rev_parse .git/ true true false ''
+test_rev_parse .git/ false true false ''
 cd objects || exit 1
-test_rev_parse .git/objects/ true true false ''
+test_rev_parse .git/objects/ false true false ''
 cd ../.. || exit 1
 
 mkdir -p sub/dir || exit 1
diff --git a/t/test-lib.sh b/t/test-lib.sh
index da47bd7..40db00c 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -324,8 +324,11 @@ test_done () {
 PATH=$(pwd)/..:$PATH
 GIT_EXEC_PATH=$(pwd)/..
 GIT_TEMPLATE_DIR=$(pwd)/../templates/blt
-GIT_CONFIG=.git/config
-export PATH GIT_EXEC_PATH GIT_TEMPLATE_DIR GIT_CONFIG
+unset GIT_CONFIG
+unset GIT_CONFIG_LOCAL
+HOME=$(pwd)
+GIT_CONFIG_ETC=$GIT_CONFIG_LOCAL
+export PATH GIT_EXEC_PATH GIT_TEMPLATE_DIR HOME GIT_CONFIG_ETC
 
 GITPERLLIB=$(pwd)/../perl/blib/lib:$(pwd)/../perl/blib/arch/auto/Git
 export GITPERLLIB

-- 
1.5.4.26.g9be9

Junio C Hamano· Feb 6, 2008, 09:14 UTC · re: Jeff King · lore

Jeff King <peff@peff.net> writes:

Show 12 quoted lines

> We could go back to setting HOME to something bogus, but then we have no
> way to suppress the reading of /etc/gitconfig (the only way to do so, I
> think, is to set GIT_CONFIG).
>
> So I think we are stuck adding in some environment magic to suppress the
> reading of ETC_GITCONFIG, and doing something like:
>
> unset GIT_CONFIG
> GIT_ETC_CONFIG=$(pwd)/.git/nonexistant
> GIT_LOCAL_CONFIG=$(pwd)/.git/nonexistant
>
> Thoughts?

Instead how about "GIT_NO_ETC_CONFIG" when set _ignores_ /etc/gitconfig altogether?

Then you do not have to make excuses like "eh, you can do this more or less with local config but...".

Jeff King· Feb 6, 2008, 09:27 UTC · re: Junio C Hamano · lore

On Wed, Feb 06, 2008 at 01:14:18AM -0800, Junio C Hamano wrote:

Show 7 quoted lines

> > GIT_ETC_CONFIG=$(pwd)/.git/nonexistant
> 
> Instead how about "GIT_NO_ETC_CONFIG" when set _ignores_ /etc/gitconfig
> altogether?
> 
> Then you do not have to make excuses like "eh, you can do this
> more or less with local config but...".

I'm fine with that, as well. And for our purposes, a little cleaner, since we don't have to use the hack-ish "nonexistent" file (which is clearly a spelling nightmare).

Updated patch series to follow.

-Peff

Jeff King· Feb 6, 2008, 10:11 UTC · re: Jeff King · lore

[PATCH 1/2] allow suppressing of global and system config

The GIT_CONFIG_NOGLOBAL and GIT_CONFIG_NOSYSTEM environment variables are magic undocumented switches that can be used to ensure a totally clean environment. This is necessary for running reliable tests, since those config files may contain settings that change the outcome of tests.

Signed-off-by: Jeff King <peff@peff.net>
---
You can see that the config file lookup is more or less duplicated in
config.c and in builtin-config.c. I tried to keep the patch minimal, but
it's possible that those can be refactored into a single lookup point.

 builtin-config.c |    5 +++--
 cache.h          |    3 +++
 config.c         |   20 ++++++++++++++++++--
 3 files changed, 24 insertions(+), 4 deletions(-)

Show changes to 3 files +24 −4

builtin-config.c, cache.h, config.c

diff --git a/builtin-config.c b/builtin-config.c
index e4a12e3..404bb44 100644
--- a/builtin-config.c
+++ b/builtin-config.c
@@ -79,9 +79,10 @@ static int get_value(const char* key_, const char* regex_)
 		local = getenv(CONFIG_LOCAL_ENVIRONMENT);
 		if (!local)
 			local = repo_config = xstrdup(git_path("config"));
-		if (home)
+		if (git_config_global() && home)
 			global = xstrdup(mkpath("%s/.gitconfig", home));
-		system_wide = git_etc_gitconfig();
+		if (git_config_system())
+			system_wide = git_etc_gitconfig();
 	}
 
 	key = xstrdup(key_);
diff --git a/cache.h b/cache.h
index 549f4bb..af74e20 100644
--- a/cache.h
+++ b/cache.h
@@ -589,6 +589,9 @@ extern int git_config_set_multivar(const char *, const char *, const char *, int
 extern int git_config_rename_section(const char *, const char *);
 extern const char *git_etc_gitconfig(void);
 extern int check_repository_format_version(const char *var, const char *value);
+extern int git_env_bool(const char *, int);
+extern int git_config_system(void);
+extern int git_config_global(void);
 
 #define MAX_GITNAME (1000)
 extern char git_default_email[MAX_GITNAME];
diff --git a/config.c b/config.c
index 498259e..2f85e9d 100644
--- a/config.c
+++ b/config.c
@@ -492,6 +492,22 @@ const char *git_etc_gitconfig(void)
 	return system_wide;
 }
 
+int git_env_bool(const char *k, int def)
+{
+	const char *v = getenv(k);
+	return v ? git_config_bool(k, v) : def;
+}
+
+int git_config_system(void)
+{
+	return !git_env_bool("GIT_CONFIG_NOSYSTEM", 0);
+}
+
+int git_config_global(void)
+{
+	return !git_env_bool("GIT_CONFIG_NOGLOBAL", 0);
+}
+
 int git_config(config_fn_t fn)
 {
 	int ret = 0;
@@ -504,7 +520,7 @@ int git_config(config_fn_t fn)
 	 * config file otherwise. */
 	filename = getenv(CONFIG_ENVIRONMENT);
 	if (!filename) {
-		if (!access(git_etc_gitconfig(), R_OK))
+		if (git_config_system() && !access(git_etc_gitconfig(), R_OK))
 			ret += git_config_from_file(fn, git_etc_gitconfig());
 		home = getenv("HOME");
 		filename = getenv(CONFIG_LOCAL_ENVIRONMENT);
@@ -512,7 +528,7 @@ int git_config(config_fn_t fn)
 			filename = repo_config = xstrdup(git_path("config"));
 	}
 
-	if (home) {
+	if (git_config_global() && home) {
 		char *user_config = xstrdup(mkpath("%s/.gitconfig", home));
 		if (!access(user_config, R_OK))
 			ret = git_config_from_file(fn, user_config);

-- 
1.5.4.25.g251c56-dirty

Jeff King· Feb 6, 2008, 10:11 UTC · re: Jeff King · lore

[PATCH 2/2] fix config reading in tests

To do this, we stop setting GIT_CONFIG, which means that we must now suppress the reading of the system-wide and user configs.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t1500-rev-parse.sh |    4 ++--
 t/test-lib.sh        |    7 +++++--
 2 files changed, 7 insertions(+), 4 deletions(-)

Show changes to 2 files +7 −4

t/t1500-rev-parse.sh, t/test-lib.sh

diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index e474b3f..38a2bf0 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -33,9 +33,9 @@ test_rev_parse() {
 test_rev_parse toplevel false false true ''
 
 cd .git || exit 1
-test_rev_parse .git/ true true false ''
+test_rev_parse .git/ false true false ''
 cd objects || exit 1
-test_rev_parse .git/objects/ true true false ''
+test_rev_parse .git/objects/ false true false ''
 cd ../.. || exit 1
 
 mkdir -p sub/dir || exit 1
diff --git a/t/test-lib.sh b/t/test-lib.sh
index da47bd7..83889c4 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -324,8 +324,11 @@ test_done () {
 PATH=$(pwd)/..:$PATH
 GIT_EXEC_PATH=$(pwd)/..
 GIT_TEMPLATE_DIR=$(pwd)/../templates/blt
-GIT_CONFIG=.git/config
-export PATH GIT_EXEC_PATH GIT_TEMPLATE_DIR GIT_CONFIG
+unset GIT_CONFIG
+unset GIT_CONFIG_LOCAL
+GIT_CONFIG_NOSYSTEM=1
+GIT_CONFIG_NOGLOBAL=1
+export PATH GIT_EXEC_PATH GIT_TEMPLATE_DIR GIT_CONFIG_NOSYSTEM GIT_CONFIG_NOGLOBAL
 
 GITPERLLIB=$(pwd)/../perl/blib/lib:$(pwd)/../perl/blib/arch/auto/Git
 export GITPERLLIB

-- 
1.5.4.25.g251c56-dirty