[PATCH] Fix wrong failures in config test

[PATCH] Fix wrong failures in config test

[Ingo Brückl]

The tests after '--set in alternative GIT_CONFIG' failed because
variable GIT_CONFIG was still set.

Signed-off-by: Ingo Brückl <ib@wupperonline.de>
---

Is it only me (bash 3.2.48(1)-release) experiencing these failures?

 t/t1300-repo-config.sh |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index d0e5546..d1c9a8f 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -428,6 +428,8 @@ EOF

 test_expect_success '--set in alternative GIT_CONFIG' 'cmp other-config expect'

+unset GIT_CONFIG
+
 cat > .git/config << EOF
 # Hallo
 	#Bello
--
1.7.3.5

Re: [PATCH] Fix wrong failures in config test

[Jonathan Nieder]

Ingo Brückl wrote:

> The tests after '--set in alternative GIT_CONFIG' failed because
> variable GIT_CONFIG was still set.
[...]
> Is it only me (bash 3.2.48(1)-release) experiencing these failures?

Could you explain the nature of the failures in more detail?  What
version of git are you building?  Is GIT_CONFIG already set in the
environment before you run t1300-repo-config.sh (it shouldn't matter)?
How does output from "sh t1300-repo-config.sh -v -i" end?  If that
doesn't end up being helpful, how about "GIT_TRACE=1 sh -x
t1300-repo-config.sh -v -i"?

Regards,
Jonathan

Re: [PATCH] Fix wrong failures in config test

[Ingo Brückl]

Jonathan Nieder wrote on Mon, 10 Jan 2011 10:52:51 -0600:

> Could you explain the nature of the failures in more detail?

Yeah, sorry.

> What version of git are you building?

1.7.3.5

> Is GIT_CONFIG already set in the environment before you run
> t1300-repo-config.sh (it shouldn't matter)?

No.

> How does output from "sh t1300-repo-config.sh -v -i" end?

  expecting success: git config --rename-section branch.eins branch.zwei
  fatal: No such section!
  not ok - 50 rename section
  #       git config --rename-section branch.eins branch.zwei

The problem is that the last 'git config' worked on other-config due to
variable GIT_CONFIG. Test 50 should work now on .git/config, but as
GIT_CONFIG is still set, it doesn't and fails.

Ingo

Re: [PATCH] Fix wrong failures in config test

[Jonathan Nieder]

Ingo Brückl wrote:
> Jonathan Nieder wrote on Mon, 10 Jan 2011 10:52:51 -0600:

>> How does output from "sh t1300-repo-config.sh -v -i" end?
>
>   expecting success: git config --rename-section branch.eins branch.zwei
>   fatal: No such section!
>   not ok - 50 rename section
>   #       git config --rename-section branch.eins branch.zwei

Thanks.

> The problem is that the last 'git config' worked on other-config due to
> variable GIT_CONFIG.

I'm still missing something.

	GIT_CONFIG=other-config git config -l > output
	echo $GIT_CONFIG

should result in no output to stdout, right?  In other words, the
construct

	envvar=value git command

is not supposed to pollute the current environment.

It sounds like you've checked that "unset GIT_CONFIG" fixes it; what's
left is to explain why GIT_CONFIG had a value in the first place.

Confused,
Jonathan

Re: [PATCH] Fix wrong failures in config test

[Junio C Hamano]

Ingo Brückl <ib@wupperonline.de> writes:

> The tests after '--set in alternative GIT_CONFIG' failed because
> variable GIT_CONFIG was still set.
>
> Signed-off-by: Ingo Brückl <ib@wupperonline.de>
> ---
>
> Is it only me (bash 3.2.48(1)-release) experiencing these failures?

>
>  t/t1300-repo-config.sh |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)

t1300 first sources test-lib.sh that explicitly unsets GIT_CONFIG and the
tests that might touch GIT_CONFIG all do so by a single-shot assignment to
be exported, i.e.

	GIT_CONFIG=other-config git config anwohner.park ausweis

that shouldn't affect the later test, unless the shell is broken.

With this patch, can you check which one of the new tests barf on you?

 t/t1300-repo-config.sh |   21 +++++++++++++++++++++
 1 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index d0e5546..c91d166 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -7,6 +7,10 @@ test_description='Test git config in different settings'
 
 . ./test-lib.sh
 
+test_expect_success 'is GIT_CONFIG set (0)?' '
+	test "z${GIT_CONFIG+set}" = z
+'
+
 test -f .git/config && rm .git/config
 
 git config core.penguin "little blue"
@@ -399,8 +403,17 @@ cat > expect << EOF
 ein.bahn=strasse
 EOF
 
+
+test_expect_success 'is GIT_CONFIG set (1)?' '
+	test "z${GIT_CONFIG+set}" = z
+'
+
 GIT_CONFIG=other-config git config -l > output
 
+test_expect_success 'is GIT_CONFIG set (2)?' '
+	test "z${GIT_CONFIG+set}" = z
+'
+
 test_expect_success 'alternative GIT_CONFIG' 'cmp output expect'
 
 test_expect_success 'alternative GIT_CONFIG (--file)' \
@@ -419,6 +432,10 @@ test_expect_success 'refer config from subdirectory' '
 
 GIT_CONFIG=other-config git config anwohner.park ausweis
 
+test_expect_success 'is GIT_CONFIG set (3)?' '
+	test "z${GIT_CONFIG+set}" = z
+'
+
 cat > expect << EOF
 [ein]
 	bahn = strasse
@@ -426,6 +443,10 @@ cat > expect << EOF
 	park = ausweis
 EOF
 
+test_expect_success 'is GIT_CONFIG set (4)?' '
+	test "z${GIT_CONFIG+set}" = z
+'
+
 test_expect_success '--set in alternative GIT_CONFIG' 'cmp other-config expect'
 
 cat > .git/config << EOF

Re: [PATCH] Fix wrong failures in config test

[Ingo Brückl]

I wrote:

> Is it only me [...] experiencing these failures?

And the answer is yes. Sorry.

As Jonathan and Junio stated,

>  envvar=value git command

>  GIT_CONFIG=other-config git config anwohner.park ausweis

shouldn't affect the environment of the tests.

Unfortunately, I had a shell alias function named git that interfered. In
fact it passes to the git program (command git "$@") but sadly does not know
about the newly set PATH and (still inexplicably to me) makes the variable
set.

So it's all my problem.

Ingo

Re: [PATCH] Fix wrong failures in config test

[Jonathan Nieder]

Ingo Brückl wrote:
> As Jonathan and Junio stated,

>>  envvar=value git command
>
>>  GIT_CONFIG=other-config git config anwohner.park ausweis
>
> shouldn't affect the environment of the tests.
>
> Unfortunately, I had a shell alias function named git that interfered. In
> fact it passes to the git program (command git "$@") but sadly does not know
> about the newly set PATH and (still inexplicably to me) makes the variable
> set.

For what it's worth, here's what POSIX[1] has to say:

	When a given simple command is required to be executed [...] the
	following expansions, assignments, and redirections shall all be
	performed from the beginning of the command text to the end:
[...]
	If no command name results, variable assignments shall affect
	the current execution environment. Otherwise, the variable
	assignments shall be exported for the execution environment of
	the command and shall not affect the current execution
	environment (except for special built-ins). 

I am guessing the expansion of your 'git' alias starts with a special
builtin.  For the future, it is probably best to guard settings for
interactive use with

	if test "${PS1+set}"
	then
		CDPATH=something
		alias foo=bar
		alias baz=qux
		...
	fi

or even better,

	case $- in
	*i*)
		CDPATH=something
		...
	esac

Thanks for explaining.
Jonathan

[1] http://unix.org/2008edition/

Re: [PATCH] Fix wrong failures in config test

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

>> Unfortunately, I had a shell alias function named git that interfered. In
>> fact it passes to the git program (command git "$@") but sadly does not know
>> about the newly set PATH and (still inexplicably to me) makes the variable
>> set.

Yuck.  I really do not want to do something like this X-<.

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

diff --git a/t/test-lib.sh b/t/test-lib.sh
index cb1ca97..df1b4f2 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -77,10 +77,10 @@ export GIT_COMMITTER_EMAIL GIT_COMMITTER_NAME
 export EDITOR
 
 # Protect ourselves from common misconfiguration to export
-# CDPATH into the environment
+# CDPATH into the environment and such
 unset CDPATH
-
 unset GREP_OPTIONS
+unalias git >/dev/null 2>&1 || :
 
 case $(echo $GIT_TRACE |tr "[A-Z]" "[a-z]") in
 	1|2|true)

Re: [PATCH] Fix wrong failures in config test

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>>> Unfortunately, I had a shell alias function named git that interfered. In
>>> fact it passes to the git program (command git "$@") but sadly does not know
>>> about the newly set PATH and (still inexplicably to me) makes the variable
>>> set.
>
> Yuck.  I really do not want to do something like this X-<.

Please don't. :)

Re: [PATCH] Fix wrong failures in config test

[Ingo Brückl]

Junio C Hamano wrote:

> Yuck.  I really do not want to do something like this X-<.

And you shouldn't. :-)

It's my personal problem (now that I know) to unset the alias.

Ingo

Re: [PATCH] Fix wrong failures in config test

[Jonathan Nieder]

Ingo Brückl wrote:

> It's a function (available in login shells and thus during the test suite):

The test suite doesn't run in a login shell.  As I hinted before, you can
put

	case "$-" in
	*i*)	# interactive shell
		;;
	*)
		return 0
	esac

in your .bashrc before the function definition and all should be well.

> From what I've learned from you now, if 'git' is an exported bash function,
> 'VAR=val git' will always automatically result in VAR being exported

I didn't understand at first why this particular vintage of bash makes
VAR leak into the current environment.  I tried to reproduce it with
Debian bash 3.2-4 (which is based on bash 3.2.39(1)-release) with no
success.

In any event git avoids

	VAR=val fn

when fn is a function for this and possibly other reasons (see [1]).

I do not think git ought to guard against a git function (or alias) in
the user's environment, even though doing so might lead to a better
user experience and less confusion on the mailing list.  git does not
protect against 'rm' being an alias to 'rm -i' or 'svn' being an
alias, either.

Regards,
Jonathan

[1] http://thread.gmane.org/gmane.comp.version-control.git/135766/focus=137095