threads / patch / 26252

patchFix wrong failures in config test

Subject: [PATCH] Fix wrong failures in config test

## tl;dr

11 messages between Jan 10, 2011 and Jan 10, 2011. Diffs are folded; open one to read it.

replies: 10people: 3as markdown or json

Ingo Br ückl· Jan 10, 2011, 16:13 UTC · lore

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(-)
Show changes to t/t1300-repo-config.sh +2 −1
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
Jonathan Nieder· Jan 10, 2011, 16:52 UTC · re: Ingo Br ückl · lore

Re: [PATCH] Fix wrong failures in config test

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

Ingo Br ückl· Jan 10, 2011, 17:15 UTC · re: Jonathan Nieder · lore

Re: [PATCH] Fix wrong failures in config test

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
Jonathan Nieder· Jan 10, 2011, 17:29 UTC · re: Ingo Br ückl · lore

Re: [PATCH] Fix wrong failures in config test

Ingo Brückl wrote:
> Jonathan Nieder wrote on Mon, 10 Jan 2011 10:52:51 -0600:
Show 6 quoted lines
>> 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

Junio C Hamano· Jan 10, 2011, 18:30 UTC · re: Ingo Br ückl · lore

Re: [PATCH] Fix wrong failures in config test

Ingo Brückl <ib@wupperonline.de> writes:
Show 7 quoted lines
> 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(-)
Show changes to t/t1300-repo-config.sh +21 −0
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
Ingo Br ückl· Jan 10, 2011, 19:21 UTC · re: Junio C Hamano · lore

Re: [PATCH] Fix wrong failures in config test

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
Jonathan Nieder· Jan 10, 2011, 19:42 UTC · re: Ingo Br ückl · lore

Re: [PATCH] Fix wrong failures in config test

Ingo Brückl wrote:
> As Jonathan and Junio stated,
Show 10 quoted lines
>>  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/
Junio C Hamano· Jan 10, 2011, 21:30 UTC · re: Jonathan Nieder · lore

Re: [PATCH] Fix wrong failures in config test

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(-)
Show changes to t/test-lib.sh +2 −2
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)
Jonathan Nieder· Jan 10, 2011, 21:33 UTC · re: Junio C Hamano · lore

Re: [PATCH] Fix wrong failures in config test

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
Show 6 quoted lines
>>> 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. :)
Ingo Br ückl· Jan 10, 2011, 21:50 UTC · re: Junio C Hamano · lore

Re: [PATCH] Fix wrong failures in config test

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
Jonathan Nieder· Jan 10, 2011, 21:59 UTC · lore

Re: [PATCH] Fix wrong failures in config test

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

← back to recent threads