patch, 3 partstests: unsetting variables that influence the outcome

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(-)

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(-)

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(-)

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

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

Jeff King wrote:

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(-)

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(-)

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(-)

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(-)

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(-)

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

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.

Makes sense to me.

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

Jeff King wrote:

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(-)

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(-)

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(-)

Jonathan Nieder <jrnieder@gmail.com> writes:

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.

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

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.

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