[PATCH 0/3] tests: unsetting variables that influence the outcome

[PATCH 0/3] tests: unsetting variables that influence the outcome

[Jonathan Nieder]

Hi,

These patches were rotting in my tree so I thought I should
try sending them out.  Maybe they can be useful.

The theme: settings from the inherited environment like GIT_PAGER can
break tests in undesirable ways.  For example,

 GIT_PAGER=more sh t5400-send-pack.sh -v -i

hangs.

Jonathan Nieder (3):
  tests: protect against GIT_ATTR_NOGLOBAL from environment
  tests: suppress global and system gitattributes
  tests: scrub environment of GIT_* variables

 t/t0003-attributes.sh |    1 +
 t/test-lib.sh         |   36 +++++++++++++++++++++++++++++++++++-
 2 files changed, 36 insertions(+), 1 deletions(-)

[PATCH 1/3] tests: protect against GIT_ATTR_NOGLOBAL from environment

[Jonathan Nieder]

The GIT_ATTR_NOGLOBAL environment variable simulates a totally clean
environment with no ~/.gitattributes present.  The intent is to
make test results more easily reproducible, but it currently has
the opposite effect --- if GIT_ATTR_NOGLOBAL is set to 1, t0003.3
(core.attributesfile) fails.

Unset GIT_ATTR_NOGLOBAL in the affected test script.  test-lib already
sets $HOME to protect against pollution from user settings, so this
should be safe.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t0003-attributes.sh |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index ebbc755..3cfc824 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -18,6 +18,7 @@ attr_check () {
 
 test_expect_success 'setup' '
 
+	sane_unset GIT_ATTR_NOGLOBAL &&
 	mkdir -p a/b/d a/c &&
 	(
 		echo "[attr]notest !test"
-- 
1.7.4.1

[PATCH 2/3] tests: suppress global and system gitattributes

[Jonathan Nieder]

Set GIT_ATTR_NOGLOBAL and GIT_ATTR_NOSYSTEM in test-lib to make
tests more reliable in two ways:

 - an invalid GIT_ATTR_NOGLOBAL or GIT_ATTR_NOSYSTEM setting
   should not cause tests to fail with

	fatal: bad config value for 'GIT_ATTR_NOGLOBAL'

 - /etc/gitattributes should not change the outcome of tests.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/test-lib.sh |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 0fdc541..8ae03c7 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -955,7 +955,10 @@ GIT_TEMPLATE_DIR="$GIT_BUILD_DIR"/templates/blt
 unset GIT_CONFIG
 GIT_CONFIG_NOSYSTEM=1
 GIT_CONFIG_NOGLOBAL=1
-export PATH GIT_EXEC_PATH GIT_TEMPLATE_DIR GIT_CONFIG_NOSYSTEM GIT_CONFIG_NOGLOBAL
+GIT_ATTR_NOSYSTEM=1
+GIT_ATTR_NOGLOBAL=1
+export PATH GIT_EXEC_PATH GIT_TEMPLATE_DIR
+export GIT_CONFIG_NOSYSTEM GIT_CONFIG_NOGLOBAL GIT_ATTR_NOSYSTEM GIT_ATTR_NOGLOBAL
 
 . "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
 
-- 
1.7.4.1

[PATCH 3/3] tests: scrub environment of GIT_* variables

[Jonathan Nieder]

Variables from the inherited environment that are meaningful to git
can break tests in undesirable ways.  For example,

 GIT_PAGER=more sh t5400-send-pack.sh -v -i

hangs.  So unset them in test-lib.

The variables to unset were found with 'git grep -F -e '"GIT_'.

Exception: leave the GIT_USE_LOOKUP variable from v1.5.6-rc0~134^2~1
(sha1-lookup: more memory efficient search in sorted list of SHA-1,
2007-12-29) alone, since it is about trying an alternate
implementation strategy rather than changing semantics and it can be
useful to compare performance with and without it set.

Longer term, it would be nice to just unset all GIT_* variables (with
exceptions like GIT_TRACE and GIT_USE_LOOKUP) with some magic using
the "env" command.  That is less straightforward than one might hope
because the values of environment variables can contain embedded
newlines and equal signs and "env -0" is not portable.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
IIRC the patch is based on v1.7.4.1.  So it does not handle
GIT_ASK_YESNO or GIT_TRACE_PACKET; they're too new.

Thanks for reading.  Hopefully it was not too dull.

 t/test-lib.sh |   31 +++++++++++++++++++++++++++++++
 1 files changed, 31 insertions(+), 0 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 8ae03c7..26c6707 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -43,7 +43,11 @@ TERM=dumb
 export LANG LC_ALL PAGER TERM TZ
 EDITOR=:
 unset VISUAL
+unset GIT_ASKPASS
 unset GIT_EDITOR
+unset GIT_PAGER
+unset GIT_MAN_VIEWER
+unset GIT_SSH
 unset AUTHOR_DATE
 unset AUTHOR_EMAIL
 unset AUTHOR_NAME
@@ -62,16 +66,43 @@ unset GIT_DIR
 unset GIT_WORK_TREE
 unset GIT_EXTERNAL_DIFF
 unset GIT_INDEX_FILE
+unset GIT_GRAFT_FILE
 unset GIT_OBJECT_DIRECTORY
 unset GIT_CEILING_DIRECTORIES
+unset GIT_DISCOVERY_ACROSS_FILESYSTEM
 unset SHA1_FILE_DIRECTORIES
 unset SHA1_FILE_DIRECTORY
+unset GIT_NO_REPLACE_OBJECTS
+unset GIT_CONFIG_PARAMETERS
 unset GIT_NOTES_REF
 unset GIT_NOTES_DISPLAY_REF
 unset GIT_NOTES_REWRITE_REF
 unset GIT_NOTES_REWRITE_MODE
 unset GIT_REFLOG_ACTION
 unset GIT_CHERRY_PICK_HELP
+unset GIT_CURL_FTP_NO_EPSV
+unset GIT_CURL_VERBOSE
+unset GIT_PROJECT_ROOT
+unset GIT_PROXY_COMMAND
+unset GIT_HTTP_EXPORT_ALL
+unset GIT_HTTP_LOW_SPEED_LIMIT
+unset GIT_HTTP_LOW_SPEED_TIME
+unset GIT_HTTP_MAX_REQUESTS
+unset GIT_HTTP_USER_AGENT
+unset GIT_SSL_CAINFO
+unset GIT_SSL_CAPATH
+unset GIT_SSL_CERT
+unset GIT_SSL_CERT_PASSWORD_PROTECTED
+unset GIT_SSL_KEY
+unset GIT_SSL_NO_VERIFY
+unset GIT_FLUSH
+unset GIT_PAGER_IN_USE
+unset GIT_SEND_EMAIL_NOTTY
+unset GIT_DEBUG_LOOKUP
+unset GIT_DEBUG_SEND_PACK
+unset GIT_DEBUG_TESTGIT
+unset GIT_TRANSLOOP_DEBUG
+unset GIT_TRANSPORT_HELPER_DEBUG
 unset GIT_QUIET
 GIT_MERGE_VERBOSITY=5
 export GIT_MERGE_VERBOSITY
-- 
1.7.4.1

Re: [PATCH 2/3] tests: suppress global and system gitattributes

[Jeff King]

On Tue, Mar 15, 2011 at 01:56:43AM -0500, Jonathan Nieder wrote:

> Set GIT_ATTR_NOGLOBAL and GIT_ATTR_NOSYSTEM in test-lib to make
> tests more reliable in two ways:
> 
>  - an invalid GIT_ATTR_NOGLOBAL or GIT_ATTR_NOSYSTEM setting
>    should not cause tests to fail with
> 
> 	fatal: bad config value for 'GIT_ATTR_NOGLOBAL'
> 
>  - /etc/gitattributes should not change the outcome of tests.

We already munge $HOME, as you note in 1/3, I don't know that there is
much point in setting GIT_ATTR_NOGLOBAL. The alternative would be to
drop your 1/3 and unset GIT_ATTR_NOGLOBAL in test-lib.sh.

I don't care much either way. Having it set prevents others tests from
accidentally triggering global attributes (since we have the odd case of
$HOME and the repo in the same directory). But IIRC, they'd have to set
core.attributesfile anyway, so that is not likely to happen.

And what you're doing at least matches what GIT_CONFIG_* does (I think
GIT_CONFIG_NOGLOBAL is also redundant in the tests at this point).

So I'm fine with either strategy. But definitely it should be protected,
so thanks for looking into it.

-Peff

Re: [PATCH 3/3] tests: scrub environment of GIT_* variables

[Jeff King]

On Tue, Mar 15, 2011 at 02:04:45AM -0500, Jonathan Nieder wrote:

> Variables from the inherited environment that are meaningful to git
> can break tests in undesirable ways.  For example,
> 
>  GIT_PAGER=more sh t5400-send-pack.sh -v -i
> 
> hangs.  So unset them in test-lib.
> 
> The variables to unset were found with 'git grep -F -e '"GIT_'.

Thanks. I fixed a few of these in a1231de recently, but was too lazy to
do an exhaustive search. Your list and your method look sane to me.

> Exception: leave the GIT_USE_LOOKUP variable from v1.5.6-rc0~134^2~1
> (sha1-lookup: more memory efficient search in sorted list of SHA-1,
> 2007-12-29) alone, since it is about trying an alternate
> implementation strategy rather than changing semantics and it can be
> useful to compare performance with and without it set.

Makes sense to me.

> Longer term, it would be nice to just unset all GIT_* variables (with
> exceptions like GIT_TRACE and GIT_USE_LOOKUP) with some magic using
> the "env" command.  That is less straightforward than one might hope
> because the values of environment variables can contain embedded
> newlines and equal signs and "env -0" is not portable.

According to posix, just running "set" provides an unambiguous,
parseable output. The problem is that it's actual shell, so it's a
little tricky to parse (it's single-quoted, and you have to follow
values across embedded newlines). So it's probably not worth the
headache.

Having a big list of "clear these variables" is probably not the end of
the world.  If it breaks somebody's test run, they'll probably report it
and we'll fix it. We don't add variables like this all that often.

-Peff

[PATCH 1-2/3 v2 0/3] Re: tests: suppress global and system gitattributes

[Jonathan Nieder]

Jeff King wrote:

> The alternative would be to
> drop your 1/3 and unset GIT_ATTR_NOGLOBAL in test-lib.sh.

Hmm, how about this?  These are meant as a replacement for 1/3
and 2/3.

Jonathan Nieder (3):
  gitattributes: drop support for GIT_ATTR_NOGLOBAL
  config: drop support for GIT_CONFIG_NOGLOBAL
  tests: suppress system gitattributes

 attr.c                          |    7 +------
 builtin/config.c                |    2 +-
 cache.h                         |    1 -
 config.c                        |    7 +------
 t/t0001-init.sh                 |    5 +----
 t/t5601-clone.sh                |    1 -
 t/t9130-git-svn-authors-file.sh |    1 -
 t/test-lib.sh                   |    4 ++--
 8 files changed, 6 insertions(+), 22 deletions(-)

[PATCH 1/3] gitattributes: drop support for GIT_ATTR_NOGLOBAL

[Jonathan Nieder]

test-lib sets $HOME to protect against pollution from user settings,
so setting GIT_ATTR_NOGLOBAL would be redundant.  Simplify by
eliminating support for that environment variable altogether.

GIT_ATTR_NOGLOBAL was introduced in v1.7.4-rc0~208^2 (Add global and
system-wide gitattributes, 2010-09-01) as an undocumented feature for
use by the test suite.  It never ended up being used (neither within
git.git nor in other projects).

This patch does not affect GIT_ATTR_NOSYSTEM, which should still be
useful.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 attr.c |    7 +------
 1 files changed, 1 insertions(+), 6 deletions(-)

diff --git a/attr.c b/attr.c
index 6aff695..0e28ba8 100644
--- a/attr.c
+++ b/attr.c
@@ -478,11 +478,6 @@ int git_attr_system(void)
 	return !git_env_bool("GIT_ATTR_NOSYSTEM", 0);
 }
 
-int git_attr_global(void)
-{
-	return !git_env_bool("GIT_ATTR_NOGLOBAL", 0);
-}
-
 static int git_attr_config(const char *var, const char *value, void *dummy)
 {
 	if (!strcmp(var, "core.attributesfile"))
@@ -511,7 +506,7 @@ static void bootstrap_attr_stack(void)
 		}
 
 		git_config(git_attr_config, NULL);
-		if (git_attr_global() && attributes_file) {
+		if (attributes_file) {
 			elem = read_attr_from_file(attributes_file, 1);
 			if (elem) {
 				elem->origin = NULL;
-- 
1.7.4.1

[PATCH 2/3] config: drop support for GIT_CONFIG_NOGLOBAL

[Jonathan Nieder]

Now that test-lib sets $HOME to protect against pollution from user
settings, GIT_CONFIG_NOGLOBAL is not needed for use by the test
suite any more.  And as luck would have it, a quick code search
reveals no other users in the wild.

This patch does not affect GIT_CONFIG_NOSYSTEM, which is still
needed.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 builtin/config.c                |    2 +-
 cache.h                         |    1 -
 config.c                        |    7 +------
 t/t0001-init.sh                 |    5 +----
 t/t5601-clone.sh                |    1 -
 t/t9130-git-svn-authors-file.sh |    1 -
 t/test-lib.sh                   |    3 +--
 7 files changed, 4 insertions(+), 16 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 76be0b7..3e3c528 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -160,7 +160,7 @@ static int get_value(const char *key_, const char *regex_)
 	if (!local) {
 		const char *home = getenv("HOME");
 		local = repo_config = git_pathdup("config");
-		if (git_config_global() && home)
+		if (home)
 			global = xstrdup(mkpath("%s/.gitconfig", home));
 		if (git_config_system())
 			system_wide = git_etc_gitconfig();
diff --git a/cache.h b/cache.h
index d0bbc91..a8ef37a 100644
--- a/cache.h
+++ b/cache.h
@@ -1020,7 +1020,6 @@ extern const char *git_etc_gitconfig(void);
 extern int check_repository_format_version(const char *var, const char *value, void *cb);
 extern int git_env_bool(const char *, int);
 extern int git_config_system(void);
-extern int git_config_global(void);
 extern int config_error_nonbool(const char *);
 extern const char *get_log_output_encoding(void);
 extern const char *get_commit_output_encoding(void);
diff --git a/config.c b/config.c
index b94de8f..217a77d 100644
--- a/config.c
+++ b/config.c
@@ -826,11 +826,6 @@ 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_from_parameters(config_fn_t fn, void *data)
 {
 	static int loaded_environment;
@@ -862,7 +857,7 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 	}
 
 	home = getenv("HOME");
-	if (git_config_global() && home) {
+	if (home) {
 		char *user_config = xstrdup(mkpath("%s/.gitconfig", home));
 		if (!access(user_config, R_OK)) {
 			ret += git_config_from_file(fn, user_config, data);
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index f684993..ce4ba13 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -47,7 +47,7 @@ test_expect_success 'plain nested in bare' '
 
 test_expect_success 'plain through aliased command, outside any git repo' '
 	(
-		sane_unset GIT_DIR GIT_WORK_TREE GIT_CONFIG_NOGLOBAL &&
+		sane_unset GIT_DIR GIT_WORK_TREE &&
 		HOME=$(pwd)/alias-config &&
 		export HOME &&
 		mkdir alias-config &&
@@ -231,7 +231,6 @@ test_expect_success 'init with init.templatedir set' '
 		git config -f "$test_config"  init.templatedir "${HOME}/templatedir-source" &&
 		mkdir templatedir-set &&
 		cd templatedir-set &&
-		sane_unset GIT_CONFIG_NOGLOBAL &&
 		sane_unset GIT_TEMPLATE_DIR &&
 		NO_SET_GIT_TEMPLATE_DIR=t &&
 		export NO_SET_GIT_TEMPLATE_DIR &&
@@ -243,7 +242,6 @@ test_expect_success 'init with init.templatedir set' '
 test_expect_success 'init --bare/--shared overrides system/global config' '
 	(
 		test_config="$HOME"/.gitconfig &&
-		sane_unset GIT_CONFIG_NOGLOBAL &&
 		git config -f "$test_config" core.bare false &&
 		git config -f "$test_config" core.sharedRepository 0640 &&
 		mkdir init-bare-shared-override &&
@@ -258,7 +256,6 @@ test_expect_success 'init --bare/--shared overrides system/global config' '
 test_expect_success 'init honors global core.sharedRepository' '
 	(
 		test_config="$HOME"/.gitconfig &&
-		sane_unset GIT_CONFIG_NOGLOBAL &&
 		git config -f "$test_config" core.sharedRepository 0666 &&
 		mkdir shared-honor-global &&
 		cd shared-honor-global &&
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 987e0c8..3ca275c 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -164,7 +164,6 @@ test_expect_success 'clone a void' '
 test_expect_success 'clone respects global branch.autosetuprebase' '
 	(
 		test_config="$HOME/.gitconfig" &&
-		unset GIT_CONFIG_NOGLOBAL &&
 		git config -f "$test_config" branch.autosetuprebase remote &&
 		rm -fr dst &&
 		git clone src dst &&
diff --git a/t/t9130-git-svn-authors-file.sh b/t/t9130-git-svn-authors-file.sh
index ec0a106..b324c49 100755
--- a/t/t9130-git-svn-authors-file.sh
+++ b/t/t9130-git-svn-authors-file.sh
@@ -96,7 +96,6 @@ test_expect_success 'fresh clone with svn.authors-file in config' '
 		rm -r "$GIT_DIR" &&
 		test x = x"$(git config svn.authorsfile)" &&
 		test_config="$HOME"/.gitconfig &&
-		unset GIT_CONFIG_NOGLOBAL &&
 		unset GIT_DIR &&
 		unset GIT_CONFIG &&
 		git config --global \
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 0fdc541..94595e3 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -954,8 +954,7 @@ fi
 GIT_TEMPLATE_DIR="$GIT_BUILD_DIR"/templates/blt
 unset GIT_CONFIG
 GIT_CONFIG_NOSYSTEM=1
-GIT_CONFIG_NOGLOBAL=1
-export PATH GIT_EXEC_PATH GIT_TEMPLATE_DIR GIT_CONFIG_NOSYSTEM GIT_CONFIG_NOGLOBAL
+export PATH GIT_EXEC_PATH GIT_TEMPLATE_DIR GIT_CONFIG_NOSYSTEM
 
 . "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
 
-- 
1.7.4.1

[PATCH 3/3] tests: suppress system gitattributes

[Jonathan Nieder]

Set GIT_ATTR_NOSYSTEM in test-lib to make tests more reliable in two
ways:

 - an invalid GIT_ATTR_NOSYSTEM setting should not cause tests to fail
   with "fatal: bad config value for 'GIT_ATTR_NOSYSTEM'".

 - /etc/gitattributes should not change the outcome of tests.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Improved-by: Jeff King <peff@peff.net>
---
 t/test-lib.sh |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 94595e3..f35ba6f 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -954,7 +954,8 @@ fi
 GIT_TEMPLATE_DIR="$GIT_BUILD_DIR"/templates/blt
 unset GIT_CONFIG
 GIT_CONFIG_NOSYSTEM=1
-export PATH GIT_EXEC_PATH GIT_TEMPLATE_DIR GIT_CONFIG_NOSYSTEM
+GIT_ATTR_NOSYSTEM=1
+export PATH GIT_EXEC_PATH GIT_TEMPLATE_DIR GIT_CONFIG_NOSYSTEM GIT_ATTR_NOSYSTEM
 
 . "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
 
-- 
1.7.4.1

[RFC/PATCH 0/2] tests: stop hard-coding the list of GIT_* vars to scrub

[Jonathan Nieder]

Jeff King wrote:

> According to posix, just running "set" provides an unambiguous,
> parseable output. The problem is that it's actual shell, so it's a
> little tricky to parse (it's single-quoted, and you have to follow
> values across embedded newlines). So it's probably not worth the
> headache.
>
> Having a big list of "clear these variables" is probably not the end of
> the world.

Here's one way (using perl), for what it's worth.

Jonathan Nieder (2):
  tests: stop worrying about obsolete environment variables
  tests: scrub environment of GIT_* variables

 t/test-lib.sh |   30 +++++-------------------------
 1 files changed, 5 insertions(+), 25 deletions(-)

[PATCH 1/2] tests: stop worrying about obsolete environment variables

[Jonathan Nieder]

After v0.99.7~99 (Retire support for old environment variables,
2005-09-09), there is no more need to unset a stray AUTHOR_NAME
variable that might have entered the test environment.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/test-lib.sh |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index f35ba6f..8893406 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -44,11 +44,6 @@ export LANG LC_ALL PAGER TERM TZ
 EDITOR=:
 unset VISUAL
 unset GIT_EDITOR
-unset AUTHOR_DATE
-unset AUTHOR_EMAIL
-unset AUTHOR_NAME
-unset COMMIT_AUTHOR_EMAIL
-unset COMMIT_AUTHOR_NAME
 unset EMAIL
 unset GIT_ALTERNATE_OBJECT_DIRECTORIES
 unset GIT_AUTHOR_DATE
@@ -64,8 +59,6 @@ unset GIT_EXTERNAL_DIFF
 unset GIT_INDEX_FILE
 unset GIT_OBJECT_DIRECTORY
 unset GIT_CEILING_DIRECTORIES
-unset SHA1_FILE_DIRECTORIES
-unset SHA1_FILE_DIRECTORY
 unset GIT_NOTES_REF
 unset GIT_NOTES_DISPLAY_REF
 unset GIT_NOTES_REWRITE_REF
-- 
1.7.4.1

[RFC/PATCH 2/2] tests: scrub environment of GIT_* variables

[Jonathan Nieder]

Variables from the inherited environment that are meaningful to git
can break tests in undesirable ways.  For example,

	GIT_PAGER=more sh t5400-send-pack.sh -v -i

hangs.  So unset all environment variables in the GIT_ namespace in
test-lib, with a few exceptions:

- GIT_TRACE* are useful for tracking down bugs exhibited by a failing
  test;

- GIT_DEBUG* are GIT_TRACE variables by another name, practically
  speaking.  They should probably be tweaked to follow the
  GIT_TRACE_foo scheme and use trace_printf machinery some time.

- GIT_USE_LOOKUP from v1.5.6-rc0~134^2~1 (sha1-lookup: more memory
  efficient search in sorted list of SHA-1, 2007-12-29) is about
  trying an alternate implementation strategy rather than changing
  semantics and it can be useful to compare performance with and
  without it set.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Thanks again.

 t/test-lib.sh |   23 +++++------------------
 1 files changed, 5 insertions(+), 18 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 8893406..8aac727 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -43,29 +43,16 @@ TERM=dumb
 export LANG LC_ALL PAGER TERM TZ
 EDITOR=:
 unset VISUAL
-unset GIT_EDITOR
 unset EMAIL
-unset GIT_ALTERNATE_OBJECT_DIRECTORIES
-unset GIT_AUTHOR_DATE
+unset $(perl -e '
+	my @env = keys %ENV;
+	my @vars = grep(/^GIT_/ && !/^GIT_(TRACE|DEBUG|USE_LOOKUP)/, @env);
+	print join("\n", @vars);
+')
 GIT_AUTHOR_EMAIL=author@example.com
 GIT_AUTHOR_NAME='A U Thor'
-unset GIT_COMMITTER_DATE
 GIT_COMMITTER_EMAIL=committer@example.com
 GIT_COMMITTER_NAME='C O Mitter'
-unset GIT_DIFF_OPTS
-unset GIT_DIR
-unset GIT_WORK_TREE
-unset GIT_EXTERNAL_DIFF
-unset GIT_INDEX_FILE
-unset GIT_OBJECT_DIRECTORY
-unset GIT_CEILING_DIRECTORIES
-unset GIT_NOTES_REF
-unset GIT_NOTES_DISPLAY_REF
-unset GIT_NOTES_REWRITE_REF
-unset GIT_NOTES_REWRITE_MODE
-unset GIT_REFLOG_ACTION
-unset GIT_CHERRY_PICK_HELP
-unset GIT_QUIET
 GIT_MERGE_VERBOSITY=5
 export GIT_MERGE_VERBOSITY
 export GIT_AUTHOR_EMAIL GIT_AUTHOR_NAME
-- 
1.7.4.1

Re: [RFC/PATCH 2/2] tests: scrub environment of GIT_* variables

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Variables from the inherited environment that are meaningful to git
> can break tests in undesirable ways.  For example,
>
> 	GIT_PAGER=more sh t5400-send-pack.sh -v -i
>
> hangs.  So unset all environment variables in the GIT_ namespace in
> test-lib, with a few exceptions:
>
> - GIT_TRACE* are useful for tracking down bugs exhibited by a failing
>   test;
>
> - GIT_DEBUG* are GIT_TRACE variables by another name, practically
>   speaking.  They should probably be tweaked to follow the
>   GIT_TRACE_foo scheme and use trace_printf machinery some time.
>
> - GIT_USE_LOOKUP from v1.5.6-rc0~134^2~1 (sha1-lookup: more memory
>   efficient search in sorted list of SHA-1, 2007-12-29) is about
>   trying an alternate implementation strategy rather than changing
>   semantics and it can be useful to compare performance with and
>   without it set.
>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> Thanks again.

Sounds sane.

I am perfectly fine with the use of perl but do we know the "perl" binary
found on $PATH (as opposed to $PERL_PATH) is good to use for this purpose?
The features used in the scriptlet seem so bread-and-butter that I don't
think it would make too much of a difference, but we may want to be
consistent.

I personally think USE_LOOKUP outlived its usefulness. It was meant to be
an easy way to experiment if the sha1_entry_pos() lookup gives better
performance while looking up a pack entry, to choose one implementation
and discard the other, but I don't think anybody actually did meaningful
benchmarks to decide which one to keep.

Perhaps we should discard the codepath USE_LOOKUP turns on, which I
suspect hasn't been exercised since v1.5.6 days by anybody. An obvious
alternative is for somebody to try using USE_LOOKUP and see if it really
gives a better performance with large packs (and if so, always use it and
discard the other codepath).

Obviously that is outside the scope of this patch series, but I thought I
should mention it before I forget.

Re: [RFC/PATCH 2/2] tests: scrub environment of GIT_* variables

[Jeff King]

On Tue, Mar 15, 2011 at 10:20:46AM -0700, Junio C Hamano wrote:

> I am perfectly fine with the use of perl but do we know the "perl" binary
> found on $PATH (as opposed to $PERL_PATH) is good to use for this purpose?
> The features used in the scriptlet seem so bread-and-butter that I don't
> think it would make too much of a difference, but we may want to be
> consistent.

We already make the assumption elsewhere in the tests that perl in $PATH
is some minimal sane version (which probably means some version of perl5
in practice). This little script looks like it should run under any
perl5 to me.

> I personally think USE_LOOKUP outlived its usefulness. It was meant to be
> an easy way to experiment if the sha1_entry_pos() lookup gives better
> performance while looking up a pack entry, to choose one implementation
> and discard the other, but I don't think anybody actually did meaningful
> benchmarks to decide which one to keep.
> 
> Perhaps we should discard the codepath USE_LOOKUP turns on, which I
> suspect hasn't been exercised since v1.5.6 days by anybody. An obvious
> alternative is for somebody to try using USE_LOOKUP and see if it really
> gives a better performance with large packs (and if so, always use it and
> discard the other codepath).

Vicent asked me about it the other day with respect to doing something
similar in libgit2. I tried a few basic things and never ended up
getting any substantially different timing (or even page faults) between
the two strategies. Vicent, did you do any timings where it made a
difference?

-Peff