[PATCH v2] tests: set temp variables using 'env' in test function instead of subshell

[PATCH v2] tests: set temp variables using 'env' in test function instead of subshell

[David Tran]

Originally, the code used subshells instead of FOO=BAR command because
the variable would otherwise leak into the surrounding context of the POSIX
shell when 'command' is a shell function. The subshell was used to hold the
context for the test. Using 'env' in the test function sets the temp variables
without leaking, removing the need of a subshell.

Signed-off-by: David Tran <unsignedzero@gmail.com>

---

Let's see if I replied correctly with send-email. Retrying this again.
How do I 'reply' to a thread using send-email?

I am David Tran a graduating CS/Math senior from Sonoma State University,
United States. I would like to work with git for GSoC'14, specifically the line
options for git rebase --interactive [1]. I have used git for a few years and
know how destructive but important rebase is to git. I have created a few shell
scripts here and there to make life using bash/zsh easier. I would like to
apply these skills and work with the best.

Github: unsignedzero

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

> Oh, really ;-)?
Missed that.

> Thanks.  Getting closer, I think.
Slowly but surely.

Signed-off-by: David Tran <unsignedzero@gmail.com>
---
 t/t1300-repo-config.sh        |   17 ++--------
 t/t1510-repo-setup.sh         |    4 +--
 t/t3200-branch.sh             |   12 +------
 t/t3301-notes.sh              |   22 ++++----------
 t/t3404-rebase-interactive.sh |   65 ++++++++--------------------------------
 t/t3413-rebase-hook.sh        |    6 +---
 t/t4014-format-patch.sh       |   14 ++-------
 t/t5305-include-tag.sh        |    4 +--
 t/t5602-clone-remote-exec.sh  |   13 ++------
 t/t5801-remote-helpers.sh     |    6 +--
 t/t6006-rev-list-format.sh    |    9 ++----
 t/t7006-pager.sh              |   18 ++---------
 12 files changed, 42 insertions(+), 148 deletions(-)

diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index c9c426c..3e3f77b 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -974,24 +974,15 @@ test_expect_success SYMLINKS 'symlinked configuration' '
 '

 test_expect_success 'nonexistent configuration' '
-	(
-		GIT_CONFIG=doesnotexist &&
-		export GIT_CONFIG &&
-		test_must_fail git config --list &&
-		test_must_fail git config test.xyzzy
-	)
+	test_must_fail env GIT_CONFIG=doesnotexist git config --list &&
+	test_must_fail env GIT_CONFIG=doesnotexist git config test.xyzzy
 '

 test_expect_success SYMLINKS 'symlink to nonexistent configuration' '
 	ln -s doesnotexist linktonada &&
 	ln -s linktonada linktolinktonada &&
-	(
-		GIT_CONFIG=linktonada &&
-		export GIT_CONFIG &&
-		test_must_fail git config --list &&
-		GIT_CONFIG=linktolinktonada &&
-		test_must_fail git config --list
-	)
+	test_must_fail env GIT_CONFIG=linktonada git config --list &&
+	test_must_fail env GIT_CONFIG=linktolinktonada git config --list
 '

 test_expect_success 'check split_cmdline return' "
diff --git a/t/t1510-repo-setup.sh b/t/t1510-repo-setup.sh
index cf2ee78..e1b2a99 100755
--- a/t/t1510-repo-setup.sh
+++ b/t/t1510-repo-setup.sh
@@ -777,9 +777,7 @@ test_expect_success '#30: core.worktree and core.bare conflict (gitfile version)
 	setup_repo 30 "$here/30" gitfile true &&
 	(
 		cd 30 &&
-		GIT_DIR=.git &&
-		export GIT_DIR &&
-		test_must_fail git symbolic-ref HEAD 2>result
+		test_must_fail env GIT_DIR=.git git symbolic-ref HEAD 2>result
 	) &&
 	grep "core.bare and core.worktree" 30/result
 '
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index fcdb867..d45e95c 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -849,11 +849,7 @@ test_expect_success 'detect typo in branch name when using --edit-description' '
 	write_script editor <<-\EOF &&
 		echo "New contents" >"$1"
 	EOF
-	(
-		EDITOR=./editor &&
-		export EDITOR &&
-		test_must_fail git branch --edit-description no-such-branch
-	)
+	test_must_fail env EDITOR=./editor git branch --edit-description no-such-branch
 '

 test_expect_success 'refuse --edit-description on unborn branch for now' '
@@ -861,11 +857,7 @@ test_expect_success 'refuse --edit-description on unborn branch for now' '
 		echo "New contents" >"$1"
 	EOF
 	git checkout --orphan unborn &&
-	(
-		EDITOR=./editor &&
-		export EDITOR &&
-		test_must_fail git branch --edit-description
-	)
+	test_must_fail env EDITOR=./editor git branch --edit-description
 '

 test_expect_success '--merged catches invalid object names' '
diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index 3bb79a4..cfd67ff 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -17,7 +17,7 @@ GIT_EDITOR=./fake_editor.sh
 export GIT_EDITOR

 test_expect_success 'cannot annotate non-existing HEAD' '
-	(MSG=3 && export MSG && test_must_fail git notes add)
+	test_must_fail env MSG=3 git notes add
 '

 test_expect_success setup '
@@ -32,22 +32,16 @@ test_expect_success setup '
 '

 test_expect_success 'need valid notes ref' '
-	(MSG=1 GIT_NOTES_REF=/ && export MSG GIT_NOTES_REF &&
-	 test_must_fail git notes add) &&
-	(MSG=2 GIT_NOTES_REF=/ && export MSG GIT_NOTES_REF &&
-	 test_must_fail git notes show)
+	test_must_fail env MSG=1 GIT_NOTES_REF=/ git notes show &&
+	test_must_fail env MSG=2 GIT_NOTES_REF=/ git notes show
 '

 test_expect_success 'refusing to add notes in refs/heads/' '
-	(MSG=1 GIT_NOTES_REF=refs/heads/bogus &&
-	 export MSG GIT_NOTES_REF &&
-	 test_must_fail git notes add)
+	test_must_fail env MSG=1 GIT_NOTES_REF=refs/heads/bogus git notes add
 '

 test_expect_success 'refusing to edit notes in refs/remotes/' '
-	(MSG=1 GIT_NOTES_REF=refs/remotes/bogus &&
-	 export MSG GIT_NOTES_REF &&
-	 test_must_fail git notes edit)
+	test_must_fail env MSG=1 GIT_NOTES_REF=refs/heads/bogus git notes edit
 '

 # 1 indicates caught gracefully by die, 128 means git-show barked
@@ -865,11 +859,7 @@ test_expect_success 'create note from non-existing note with "git notes add -c"
 	git add a10 &&
 	test_tick &&
 	git commit -m 10th &&
-	(
-		MSG="yet another note" &&
-		export MSG &&
-		test_must_fail git notes add -c deadbeef
-	) &&
+	test_must_fail env MSG="yet another note" git notes add -c deadbeef &&
 	test_must_fail git notes list HEAD
 '

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 50e22b1..4c7364a 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -104,9 +104,7 @@ test_expect_success 'rebase -i with the exec command checks tree cleanness' '
 	git checkout master &&
 	(
 	set_fake_editor &&
-	FAKE_LINES="exec_echo_foo_>file1 1" &&
-	export FAKE_LINES &&
-	test_must_fail git rebase -i HEAD^
+	test_must_fail env FAKE_LINES="exec_echo_foo_>file1 1" git rebase -i HEAD^
 	) &&
 	test_cmp_rev master^ HEAD &&
 	git reset --hard &&
@@ -118,9 +116,8 @@ test_expect_success 'rebase -i with exec of inexistent command' '
 	test_when_finished "git rebase --abort" &&
 	(
 	set_fake_editor &&
-	FAKE_LINES="exec_this-command-does-not-exist 1" &&
-	export FAKE_LINES &&
-	test_must_fail git rebase -i HEAD^ >actual 2>&1
+	test_must_fail env FAKE_LINES="exec_this-command-does-not-exist 1" \
+	git rebase -i HEAD^ >actual 2>&1
 	) &&
 	! grep "Maybe git-rebase is broken" actual
 '
@@ -375,11 +372,7 @@ test_expect_success 'commit message used after conflict' '
 	git checkout -b conflict-fixup conflict-branch &&
 	base=$(git rev-parse HEAD~4) &&
 	set_fake_editor &&
-	(
-		FAKE_LINES="1 fixup 3 fixup 4" &&
-		export FAKE_LINES &&
-		test_must_fail git rebase -i $base
-	) &&
+	test_must_fail env FAKE_LINES="1 fixup 3 fixup 4" git rebase -i $base &&
 	echo three > conflict &&
 	git add conflict &&
 	FAKE_COMMIT_AMEND="ONCE" EXPECT_HEADER_COUNT=2 \
@@ -394,11 +387,7 @@ test_expect_success 'commit message retained after conflict' '
 	git checkout -b conflict-squash conflict-branch &&
 	base=$(git rev-parse HEAD~4) &&
 	set_fake_editor &&
-	(
-		FAKE_LINES="1 fixup 3 squash 4" &&
-		export FAKE_LINES &&
-		test_must_fail git rebase -i $base
-	) &&
+	test_must_fail env FAKE_LINES="1 fixup 3 squash 4" git rebase -i $base &&
 	echo three > conflict &&
 	git add conflict &&
 	FAKE_COMMIT_AMEND="TWICE" EXPECT_HEADER_COUNT=2 \
@@ -469,11 +458,7 @@ test_expect_success 'interrupted squash works as expected' '
 	git checkout -b interrupted-squash conflict-branch &&
 	one=$(git rev-parse HEAD~3) &&
 	set_fake_editor &&
-	(
-		FAKE_LINES="1 squash 3 2" &&
-		export FAKE_LINES &&
-		test_must_fail git rebase -i HEAD~3
-	) &&
+	test_must_fail env FAKE_LINES="1 squash 3 2" git rebase -i HEAD~3 &&
 	(echo one; echo two; echo four) > conflict &&
 	git add conflict &&
 	test_must_fail git rebase --continue &&
@@ -487,11 +472,7 @@ test_expect_success 'interrupted squash works as expected (case 2)' '
 	git checkout -b interrupted-squash2 conflict-branch &&
 	one=$(git rev-parse HEAD~3) &&
 	set_fake_editor &&
-	(
-		FAKE_LINES="3 squash 1 2" &&
-		export FAKE_LINES &&
-		test_must_fail git rebase -i HEAD~3
-	) &&
+	test_must_fail env FAKE_LINES="3 squash 1 2" git rebase -i HEAD~3 &&
 	(echo one; echo four) > conflict &&
 	git add conflict &&
 	test_must_fail git rebase --continue &&
@@ -528,11 +509,7 @@ test_expect_success 'aborted --continue does not squash commits after "edit"' '
 	FAKE_LINES="edit 1" git rebase -i HEAD^ &&
 	echo "edited again" > file7 &&
 	git add file7 &&
-	(
-		FAKE_COMMIT_MESSAGE=" " &&
-		export FAKE_COMMIT_MESSAGE &&
-		test_must_fail git rebase --continue
-	) &&
+	test_must_fail env FAKE_COMMIT_MESSAGE=" " git rebase --continue
 	test $old = $(git rev-parse HEAD) &&
 	git rebase --abort
 '
@@ -547,11 +524,7 @@ test_expect_success 'auto-amend only edited commits after "edit"' '
 	echo "and again" > file7 &&
 	git add file7 &&
 	test_tick &&
-	(
-		FAKE_COMMIT_MESSAGE="and again" &&
-		export FAKE_COMMIT_MESSAGE &&
-		test_must_fail git rebase --continue
-	) &&
+	test_must_fail env FAKE_COMMIT_MESSAGE="and again" git rebase --continue &&
 	git rebase --abort
 '

@@ -559,11 +532,7 @@ test_expect_success 'clean error after failed "exec"' '
 	test_tick &&
 	test_when_finished "git rebase --abort || :" &&
 	set_fake_editor &&
-	(
-		FAKE_LINES="1 exec_false" &&
-		export FAKE_LINES &&
-		test_must_fail git rebase -i HEAD^
-	) &&
+	test_must_fail env FAKE_LINES="1 exec_false" git rebase -i HEAD^ &&
 	echo "edited again" > file7 &&
 	git add file7 &&
 	test_must_fail git rebase --continue 2>error &&
@@ -947,12 +916,8 @@ test_expect_success 'rebase -i --root retain root commit author and message' '

 test_expect_success 'rebase -i --root temporary sentinel commit' '
 	git checkout B &&
-	(
-		set_fake_editor &&
-		FAKE_LINES="2" &&
-		export FAKE_LINES &&
-		test_must_fail git rebase -i --root
-	) &&
+	set_fake_editor &&
+	test_must_fail env FAKE_LINES="2" git rebase -i --root &&
 	git cat-file commit HEAD | grep "^tree 4b825dc642cb" &&
 	git rebase --abort
 '
@@ -1042,11 +1007,7 @@ test_expect_success 'rebase -i error on commits with \ in message' '
 	test_when_finished "git rebase --abort; git reset --hard $current_head; rm -f error" &&
 	test_commit TO-REMOVE will-conflict old-content &&
 	test_commit "\temp" will-conflict new-content dummy &&
-	(
-	EDITOR=true &&
-	export EDITOR &&
-	test_must_fail git rebase -i HEAD^ --onto HEAD^^ 2>error
-	) &&
+	test_must_fail env EDITOR=true git rebase -i HEAD^ --onto HEAD^^ 2>error &&
 	test_expect_code 1 grep  "	emp" error
 '

diff --git a/t/t3413-rebase-hook.sh b/t/t3413-rebase-hook.sh
index 098b755..b6833e9 100755
--- a/t/t3413-rebase-hook.sh
+++ b/t/t3413-rebase-hook.sh
@@ -118,11 +118,7 @@ test_expect_success 'pre-rebase hook stops rebase (1)' '
 test_expect_success 'pre-rebase hook stops rebase (2)' '
 	git checkout test &&
 	git reset --hard side &&
-	(
-		EDITOR=:
-		export EDITOR
-		test_must_fail git rebase -i master
-	) &&
+	test_must_fail env EDITOR=: git rebase -i master &&
 	test "z$(git symbolic-ref HEAD)" = zrefs/heads/test &&
 	test 0 = $(git rev-list HEAD...side | wc -l)
 '
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 73194b2..9c80633 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -764,22 +764,14 @@ test_expect_success 'format-patch --signature="" suppresses signatures' '

 test_expect_success TTY 'format-patch --stdout paginates' '
 	rm -f pager_used &&
-	(
-		GIT_PAGER="wc >pager_used" &&
-		export GIT_PAGER &&
-		test_terminal git format-patch --stdout --all
-	) &&
+	test_terminal env GIT_PAGER="wc >pager_used" git format-patch --stdout --all &&
 	test_path_is_file pager_used
 '

  test_expect_success TTY 'format-patch --stdout pagination can be disabled' '
 	rm -f pager_used &&
-	(
-		GIT_PAGER="wc >pager_used" &&
-		export GIT_PAGER &&
-		test_terminal git --no-pager format-patch --stdout --all &&
-		test_terminal git -c "pager.format-patch=false" format-patch --stdout --all
-	) &&
+	test_terminal env GIT_PAGER="wc >pager_used" git --no-pager format-patch --stdout --all &&
+	test_terminal env GIT_PAGER="wc >pager_used" git -c "pager.format-patch=false" format-patch --stdout --all &&
 	test_path_is_missing pager_used &&
 	test_path_is_missing .git/pager_used
 '
diff --git a/t/t5305-include-tag.sh b/t/t5305-include-tag.sh
index b061864..21517c7 100755
--- a/t/t5305-include-tag.sh
+++ b/t/t5305-include-tag.sh
@@ -45,9 +45,7 @@ test_expect_success 'unpack objects' '
 test_expect_success 'check unpacked result (have commit, no tag)' '
 	git rev-list --objects $commit >list.expect &&
 	(
-		GIT_DIR=clone.git &&
-		export GIT_DIR &&
-		test_must_fail git cat-file -e $tag &&
+		test_must_fail env GIT_DIR=clone.git git cat-file -e $tag &&
 		git rev-list --objects $commit
 	) >list.actual &&
 	test_cmp list.expect list.actual
diff --git a/t/t5602-clone-remote-exec.sh b/t/t5602-clone-remote-exec.sh
index 3f353d9..cbcceab 100755
--- a/t/t5602-clone-remote-exec.sh
+++ b/t/t5602-clone-remote-exec.sh
@@ -12,21 +12,14 @@ test_expect_success setup '
 '

 test_expect_success 'clone calls git upload-pack unqualified with no -u option' '
-	(
-		GIT_SSH=./not_ssh &&
-		export GIT_SSH &&
-		test_must_fail git clone localhost:/path/to/repo junk
-	) &&
+	test_must_fail env GIT_SSH=./not_ssh git clone localhost:/path/to/repo junk &&
 	echo "localhost git-upload-pack '\''/path/to/repo'\''" >expected &&
 	test_cmp expected not_ssh_output
 '

 test_expect_success 'clone calls specified git upload-pack with -u option' '
-	(
-		GIT_SSH=./not_ssh &&
-		export GIT_SSH &&
-		test_must_fail git clone -u ./something/bin/git-upload-pack localhost:/path/to/repo junk
-	) &&
+	test_must_fail env GIT_SSH=./not_ssh \
+		git clone -u ./something/bin/git-upload-pack localhost:/path/to/repo junk &&
 	echo "localhost ./something/bin/git-upload-pack '\''/path/to/repo'\''" >expected &&
 	test_cmp expected not_ssh_output
 '
diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index 613f69a..ca19838 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -218,10 +218,8 @@ test_expect_success 'proper failure checks for fetching' '
 '

 test_expect_success 'proper failure checks for pushing' '
-	(GIT_REMOTE_TESTGIT_FAILURE=1 &&
-	export GIT_REMOTE_TESTGIT_FAILURE &&
-	cd local &&
-	test_must_fail git push --all
+	(cd local &&
+	test_must_fail env GIT_REMOTE_TESTGIT_FAILURE=1 git push --all
 	)
 '

diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh
index 9874403..9d9d9de 100755
--- a/t/t6006-rev-list-format.sh
+++ b/t/t6006-rev-list-format.sh
@@ -190,12 +190,9 @@ test_expect_success '%C(auto) respects --no-color' '
 '

 test_expect_success TTY '%C(auto) respects --color=auto (stdout is tty)' '
-	(
-		TERM=vt100 && export TERM &&
-		test_terminal \
-			git log --format=$AUTO_COLOR -1 --color=auto >actual &&
-		has_color actual
-	)
+	test_terminal env TERM=vt100 \
+		git log --format=$AUTO_COLOR -1 --color=auto >actual &&
+	has_color actual
 '

 test_expect_success '%C(auto) respects --color=auto (stdout not tty)' '
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index b9365b4..da958a8 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -146,11 +146,7 @@ test_expect_success 'no color when stdout is a regular file' '
 test_expect_success TTY 'color when writing to a pager' '
 	rm -f paginated.out &&
 	test_config color.ui auto &&
-	(
-		TERM=vt100 &&
-		export TERM &&
-		test_terminal git log
-	) &&
+	test_terminal env TERM=vt100 git log &&
 	colorful paginated.out
 '

@@ -158,11 +154,7 @@ test_expect_success TTY 'colors are suppressed by color.pager' '
 	rm -f paginated.out &&
 	test_config color.ui auto &&
 	test_config color.pager false &&
-	(
-		TERM=vt100 &&
-		export TERM &&
-		test_terminal git log
-	) &&
+	test_terminal env TERM=vt100 git log &&
 	! colorful paginated.out
 '

@@ -181,11 +173,7 @@ test_expect_success 'color when writing to a file intended for a pager' '
 test_expect_success TTY 'colors are sent to pager for external commands' '
 	test_config alias.externallog "!git log" &&
 	test_config color.ui auto &&
-	(
-		TERM=vt100 &&
-		export TERM &&
-		test_terminal git -p externallog
-	) &&
+	test_terminal env TERM=vt100 git -p externallog &&
 	colorful paginated.out
 '

--
1.7.9

Re: [PATCH v2] tests: set temp variables using 'env' in test function instead of subshell

[Junio C Hamano]

David Tran <unsignedzero@gmail.com> writes:

> Originally, the code used subshells instead of FOO=BAR command
> because the variable would otherwise leak into the surrounding
> context of the POSIX shell when 'command' is a shell function.
> The subshell was used to hold the context for the test. Using
> 'env' in the test function sets the temp variables without
> leaking, removing the need of a subshell.

These are not "temp variables" ;-).

You are improving the way how commands are run under a different
settings to environment variables.

Hmm, let's try to see if I can do better:

	Subject: tests: use "env" to run commands with temporary env-var settings

	Ordinarily, we would say "VAR=VAL command" to execute a
	tested command with environment variable(s) set only for
	that command.  This however does not work if 'command' is a
	shell function (most notably 'test_must_fail'); the result
	of the assignment is retained and affects later commands.

	To avoid this, we used to assign and export environment
        variables and run such a test in a subshell,

		(
                	VAR=VAL && export VAR &&
                        test_must_fail git command to be tested
		)

        but with "env" utility, we should be able to say

        	test_must_fail env VAR=VAL git command to be tested

	which is much shorter and easier to read.

> Let's see if I replied correctly with send-email. Retrying this again.
> How do I 'reply' to a thread using send-email?

Look for --in-reply-to option in "man git-send-email".

> Signed-off-by: David Tran <unsignedzero@gmail.com>
> ---
>  t/t1300-repo-config.sh        |   17 ++--------
>  t/t1510-repo-setup.sh         |    4 +--
>  t/t3200-branch.sh             |   12 +------
>  t/t3301-notes.sh              |   22 ++++----------
>  t/t3404-rebase-interactive.sh |   65 ++++++++--------------------------------
>  t/t3413-rebase-hook.sh        |    6 +---
>  t/t4014-format-patch.sh       |   14 ++-------
>  t/t5305-include-tag.sh        |    4 +--
>  t/t5602-clone-remote-exec.sh  |   13 ++------
>  t/t5801-remote-helpers.sh     |    6 +--
>  t/t6006-rev-list-format.sh    |    9 ++----
>  t/t7006-pager.sh              |   18 ++---------
>  12 files changed, 42 insertions(+), 148 deletions(-)

Thanks.  The numbers look very good ;-)  We love code reduction.

> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
> index c9c426c..3e3f77b 100755
> --- a/t/t1300-repo-config.sh
> +++ b/t/t1300-repo-config.sh
> @@ -974,24 +974,15 @@ test_expect_success SYMLINKS 'symlinked configuration' '
>  '
>
>  test_expect_success 'nonexistent configuration' '
> -	(
> -		GIT_CONFIG=doesnotexist &&
> -		export GIT_CONFIG &&
> -		test_must_fail git config --list &&
> -		test_must_fail git config test.xyzzy
> -	)
> +	test_must_fail env GIT_CONFIG=doesnotexist git config --list &&
> +	test_must_fail env GIT_CONFIG=doesnotexist git config test.xyzzy
>  '
>
>  test_expect_success SYMLINKS 'symlink to nonexistent configuration' '
>  	ln -s doesnotexist linktonada &&
>  	ln -s linktonada linktolinktonada &&
> -	(
> -		GIT_CONFIG=linktonada &&
> -		export GIT_CONFIG &&
> -		test_must_fail git config --list &&
> -		GIT_CONFIG=linktolinktonada &&
> -		test_must_fail git config --list
> -	)
> +	test_must_fail env GIT_CONFIG=linktonada git config --list &&
> +	test_must_fail env GIT_CONFIG=linktolinktonada git config --list
>  '
>
>  test_expect_success 'check split_cmdline return' "
> diff --git a/t/t1510-repo-setup.sh b/t/t1510-repo-setup.sh
> index cf2ee78..e1b2a99 100755
> --- a/t/t1510-repo-setup.sh
> +++ b/t/t1510-repo-setup.sh
> @@ -777,9 +777,7 @@ test_expect_success '#30: core.worktree and core.bare conflict (gitfile version)
>  	setup_repo 30 "$here/30" gitfile true &&
>  	(
>  		cd 30 &&
> -		GIT_DIR=.git &&
> -		export GIT_DIR &&
> -		test_must_fail git symbolic-ref HEAD 2>result
> +		test_must_fail env GIT_DIR=.git git symbolic-ref HEAD 2>result
>  	) &&
>  	grep "core.bare and core.worktree" 30/result
>  '
> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index fcdb867..d45e95c 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -849,11 +849,7 @@ test_expect_success 'detect typo in branch name when using --edit-description' '
>  	write_script editor <<-\EOF &&
>  		echo "New contents" >"$1"
>  	EOF
> -	(
> -		EDITOR=./editor &&
> -		export EDITOR &&
> -		test_must_fail git branch --edit-description no-such-branch
> -	)
> +	test_must_fail env EDITOR=./editor git branch --edit-description no-such-branch
>  '
>
>  test_expect_success 'refuse --edit-description on unborn branch for now' '
> @@ -861,11 +857,7 @@ test_expect_success 'refuse --edit-description on unborn branch for now' '
>  		echo "New contents" >"$1"
>  	EOF
>  	git checkout --orphan unborn &&
> -	(
> -		EDITOR=./editor &&
> -		export EDITOR &&
> -		test_must_fail git branch --edit-description
> -	)
> +	test_must_fail env EDITOR=./editor git branch --edit-description
>  '
>
>  test_expect_success '--merged catches invalid object names' '
> diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
> index 3bb79a4..cfd67ff 100755
> --- a/t/t3301-notes.sh
> +++ b/t/t3301-notes.sh
> @@ -17,7 +17,7 @@ GIT_EDITOR=./fake_editor.sh
>  export GIT_EDITOR
>
>  test_expect_success 'cannot annotate non-existing HEAD' '
> -	(MSG=3 && export MSG && test_must_fail git notes add)
> +	test_must_fail env MSG=3 git notes add
>  '
>
>  test_expect_success setup '
> @@ -32,22 +32,16 @@ test_expect_success setup '
>  '
>
>  test_expect_success 'need valid notes ref' '
> -	(MSG=1 GIT_NOTES_REF=/ && export MSG GIT_NOTES_REF &&
> -	 test_must_fail git notes add) &&
> -	(MSG=2 GIT_NOTES_REF=/ && export MSG GIT_NOTES_REF &&
> -	 test_must_fail git notes show)
> +	test_must_fail env MSG=1 GIT_NOTES_REF=/ git notes show &&
> +	test_must_fail env MSG=2 GIT_NOTES_REF=/ git notes show
>  '
>
>  test_expect_success 'refusing to add notes in refs/heads/' '
> -	(MSG=1 GIT_NOTES_REF=refs/heads/bogus &&
> -	 export MSG GIT_NOTES_REF &&
> -	 test_must_fail git notes add)
> +	test_must_fail env MSG=1 GIT_NOTES_REF=refs/heads/bogus git notes add
>  '
>
>  test_expect_success 'refusing to edit notes in refs/remotes/' '
> -	(MSG=1 GIT_NOTES_REF=refs/remotes/bogus &&
> -	 export MSG GIT_NOTES_REF &&
> -	 test_must_fail git notes edit)
> +	test_must_fail env MSG=1 GIT_NOTES_REF=refs/heads/bogus git notes edit
>  '
>
>  # 1 indicates caught gracefully by die, 128 means git-show barked
> @@ -865,11 +859,7 @@ test_expect_success 'create note from non-existing note with "git notes add -c"
>  	git add a10 &&
>  	test_tick &&
>  	git commit -m 10th &&
> -	(
> -		MSG="yet another note" &&
> -		export MSG &&
> -		test_must_fail git notes add -c deadbeef
> -	) &&
> +	test_must_fail env MSG="yet another note" git notes add -c deadbeef &&
>  	test_must_fail git notes list HEAD
>  '
>
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 50e22b1..4c7364a 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -104,9 +104,7 @@ test_expect_success 'rebase -i with the exec command checks tree cleanness' '
>  	git checkout master &&
>  	(
>  	set_fake_editor &&
> -	FAKE_LINES="exec_echo_foo_>file1 1" &&
> -	export FAKE_LINES &&
> -	test_must_fail git rebase -i HEAD^
> +	test_must_fail env FAKE_LINES="exec_echo_foo_>file1 1" git rebase -i HEAD^
>  	) &&
>  	test_cmp_rev master^ HEAD &&
>  	git reset --hard &&
> @@ -118,9 +116,8 @@ test_expect_success 'rebase -i with exec of inexistent command' '
>  	test_when_finished "git rebase --abort" &&
>  	(
>  	set_fake_editor &&
> -	FAKE_LINES="exec_this-command-does-not-exist 1" &&
> -	export FAKE_LINES &&
> -	test_must_fail git rebase -i HEAD^ >actual 2>&1
> +	test_must_fail env FAKE_LINES="exec_this-command-does-not-exist 1" \
> +	git rebase -i HEAD^ >actual 2>&1
>  	) &&
>  	! grep "Maybe git-rebase is broken" actual
>  '
> @@ -375,11 +372,7 @@ test_expect_success 'commit message used after conflict' '
>  	git checkout -b conflict-fixup conflict-branch &&
>  	base=$(git rev-parse HEAD~4) &&
>  	set_fake_editor &&
> -	(
> -		FAKE_LINES="1 fixup 3 fixup 4" &&
> -		export FAKE_LINES &&
> -		test_must_fail git rebase -i $base
> -	) &&
> +	test_must_fail env FAKE_LINES="1 fixup 3 fixup 4" git rebase -i $base &&
>  	echo three > conflict &&
>  	git add conflict &&
>  	FAKE_COMMIT_AMEND="ONCE" EXPECT_HEADER_COUNT=2 \
> @@ -394,11 +387,7 @@ test_expect_success 'commit message retained after conflict' '
>  	git checkout -b conflict-squash conflict-branch &&
>  	base=$(git rev-parse HEAD~4) &&
>  	set_fake_editor &&
> -	(
> -		FAKE_LINES="1 fixup 3 squash 4" &&
> -		export FAKE_LINES &&
> -		test_must_fail git rebase -i $base
> -	) &&
> +	test_must_fail env FAKE_LINES="1 fixup 3 squash 4" git rebase -i $base &&
>  	echo three > conflict &&
>  	git add conflict &&
>  	FAKE_COMMIT_AMEND="TWICE" EXPECT_HEADER_COUNT=2 \
> @@ -469,11 +458,7 @@ test_expect_success 'interrupted squash works as expected' '
>  	git checkout -b interrupted-squash conflict-branch &&
>  	one=$(git rev-parse HEAD~3) &&
>  	set_fake_editor &&
> -	(
> -		FAKE_LINES="1 squash 3 2" &&
> -		export FAKE_LINES &&
> -		test_must_fail git rebase -i HEAD~3
> -	) &&
> +	test_must_fail env FAKE_LINES="1 squash 3 2" git rebase -i HEAD~3 &&
>  	(echo one; echo two; echo four) > conflict &&
>  	git add conflict &&
>  	test_must_fail git rebase --continue &&
> @@ -487,11 +472,7 @@ test_expect_success 'interrupted squash works as expected (case 2)' '
>  	git checkout -b interrupted-squash2 conflict-branch &&
>  	one=$(git rev-parse HEAD~3) &&
>  	set_fake_editor &&
> -	(
> -		FAKE_LINES="3 squash 1 2" &&
> -		export FAKE_LINES &&
> -		test_must_fail git rebase -i HEAD~3
> -	) &&
> +	test_must_fail env FAKE_LINES="3 squash 1 2" git rebase -i HEAD~3 &&
>  	(echo one; echo four) > conflict &&
>  	git add conflict &&
>  	test_must_fail git rebase --continue &&
> @@ -528,11 +509,7 @@ test_expect_success 'aborted --continue does not squash commits after "edit"' '
>  	FAKE_LINES="edit 1" git rebase -i HEAD^ &&
>  	echo "edited again" > file7 &&
>  	git add file7 &&
> -	(
> -		FAKE_COMMIT_MESSAGE=" " &&
> -		export FAKE_COMMIT_MESSAGE &&
> -		test_must_fail git rebase --continue
> -	) &&
> +	test_must_fail env FAKE_COMMIT_MESSAGE=" " git rebase --continue
>  	test $old = $(git rev-parse HEAD) &&
>  	git rebase --abort
>  '
> @@ -547,11 +524,7 @@ test_expect_success 'auto-amend only edited commits after "edit"' '
>  	echo "and again" > file7 &&
>  	git add file7 &&
>  	test_tick &&
> -	(
> -		FAKE_COMMIT_MESSAGE="and again" &&
> -		export FAKE_COMMIT_MESSAGE &&
> -		test_must_fail git rebase --continue
> -	) &&
> +	test_must_fail env FAKE_COMMIT_MESSAGE="and again" git rebase --continue &&
>  	git rebase --abort
>  '
>
> @@ -559,11 +532,7 @@ test_expect_success 'clean error after failed "exec"' '
>  	test_tick &&
>  	test_when_finished "git rebase --abort || :" &&
>  	set_fake_editor &&
> -	(
> -		FAKE_LINES="1 exec_false" &&
> -		export FAKE_LINES &&
> -		test_must_fail git rebase -i HEAD^
> -	) &&
> +	test_must_fail env FAKE_LINES="1 exec_false" git rebase -i HEAD^ &&
>  	echo "edited again" > file7 &&
>  	git add file7 &&
>  	test_must_fail git rebase --continue 2>error &&
> @@ -947,12 +916,8 @@ test_expect_success 'rebase -i --root retain root commit author and message' '
>
>  test_expect_success 'rebase -i --root temporary sentinel commit' '
>  	git checkout B &&
> -	(
> -		set_fake_editor &&
> -		FAKE_LINES="2" &&
> -		export FAKE_LINES &&
> -		test_must_fail git rebase -i --root
> -	) &&
> +	set_fake_editor &&
> +	test_must_fail env FAKE_LINES="2" git rebase -i --root &&
>  	git cat-file commit HEAD | grep "^tree 4b825dc642cb" &&
>  	git rebase --abort
>  '
> @@ -1042,11 +1007,7 @@ test_expect_success 'rebase -i error on commits with \ in message' '
>  	test_when_finished "git rebase --abort; git reset --hard $current_head; rm -f error" &&
>  	test_commit TO-REMOVE will-conflict old-content &&
>  	test_commit "\temp" will-conflict new-content dummy &&
> -	(
> -	EDITOR=true &&
> -	export EDITOR &&
> -	test_must_fail git rebase -i HEAD^ --onto HEAD^^ 2>error
> -	) &&
> +	test_must_fail env EDITOR=true git rebase -i HEAD^ --onto HEAD^^ 2>error &&
>  	test_expect_code 1 grep  "	emp" error
>  '
>
> diff --git a/t/t3413-rebase-hook.sh b/t/t3413-rebase-hook.sh
> index 098b755..b6833e9 100755
> --- a/t/t3413-rebase-hook.sh
> +++ b/t/t3413-rebase-hook.sh
> @@ -118,11 +118,7 @@ test_expect_success 'pre-rebase hook stops rebase (1)' '
>  test_expect_success 'pre-rebase hook stops rebase (2)' '
>  	git checkout test &&
>  	git reset --hard side &&
> -	(
> -		EDITOR=:
> -		export EDITOR
> -		test_must_fail git rebase -i master
> -	) &&
> +	test_must_fail env EDITOR=: git rebase -i master &&
>  	test "z$(git symbolic-ref HEAD)" = zrefs/heads/test &&
>  	test 0 = $(git rev-list HEAD...side | wc -l)
>  '
> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> index 73194b2..9c80633 100755
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -764,22 +764,14 @@ test_expect_success 'format-patch --signature="" suppresses signatures' '
>
>  test_expect_success TTY 'format-patch --stdout paginates' '
>  	rm -f pager_used &&
> -	(
> -		GIT_PAGER="wc >pager_used" &&
> -		export GIT_PAGER &&
> -		test_terminal git format-patch --stdout --all
> -	) &&
> +	test_terminal env GIT_PAGER="wc >pager_used" git format-patch --stdout --all &&
>  	test_path_is_file pager_used
>  '
>
>   test_expect_success TTY 'format-patch --stdout pagination can be disabled' '
>  	rm -f pager_used &&
> -	(
> -		GIT_PAGER="wc >pager_used" &&
> -		export GIT_PAGER &&
> -		test_terminal git --no-pager format-patch --stdout --all &&
> -		test_terminal git -c "pager.format-patch=false" format-patch --stdout --all
> -	) &&
> +	test_terminal env GIT_PAGER="wc >pager_used" git --no-pager format-patch --stdout --all &&
> +	test_terminal env GIT_PAGER="wc >pager_used" git -c "pager.format-patch=false" format-patch --stdout --all &&
>  	test_path_is_missing pager_used &&
>  	test_path_is_missing .git/pager_used
>  '
> diff --git a/t/t5305-include-tag.sh b/t/t5305-include-tag.sh
> index b061864..21517c7 100755
> --- a/t/t5305-include-tag.sh
> +++ b/t/t5305-include-tag.sh
> @@ -45,9 +45,7 @@ test_expect_success 'unpack objects' '
>  test_expect_success 'check unpacked result (have commit, no tag)' '
>  	git rev-list --objects $commit >list.expect &&
>  	(
> -		GIT_DIR=clone.git &&
> -		export GIT_DIR &&
> -		test_must_fail git cat-file -e $tag &&
> +		test_must_fail env GIT_DIR=clone.git git cat-file -e $tag &&
>  		git rev-list --objects $commit
>  	) >list.actual &&
>  	test_cmp list.expect list.actual
> diff --git a/t/t5602-clone-remote-exec.sh b/t/t5602-clone-remote-exec.sh
> index 3f353d9..cbcceab 100755
> --- a/t/t5602-clone-remote-exec.sh
> +++ b/t/t5602-clone-remote-exec.sh
> @@ -12,21 +12,14 @@ test_expect_success setup '
>  '
>
>  test_expect_success 'clone calls git upload-pack unqualified with no -u option' '
> -	(
> -		GIT_SSH=./not_ssh &&
> -		export GIT_SSH &&
> -		test_must_fail git clone localhost:/path/to/repo junk
> -	) &&
> +	test_must_fail env GIT_SSH=./not_ssh git clone localhost:/path/to/repo junk &&
>  	echo "localhost git-upload-pack '\''/path/to/repo'\''" >expected &&
>  	test_cmp expected not_ssh_output
>  '
>
>  test_expect_success 'clone calls specified git upload-pack with -u option' '
> -	(
> -		GIT_SSH=./not_ssh &&
> -		export GIT_SSH &&
> -		test_must_fail git clone -u ./something/bin/git-upload-pack localhost:/path/to/repo junk
> -	) &&
> +	test_must_fail env GIT_SSH=./not_ssh \
> +		git clone -u ./something/bin/git-upload-pack localhost:/path/to/repo junk &&
>  	echo "localhost ./something/bin/git-upload-pack '\''/path/to/repo'\''" >expected &&
>  	test_cmp expected not_ssh_output
>  '
> diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
> index 613f69a..ca19838 100755
> --- a/t/t5801-remote-helpers.sh
> +++ b/t/t5801-remote-helpers.sh
> @@ -218,10 +218,8 @@ test_expect_success 'proper failure checks for fetching' '
>  '
>
>  test_expect_success 'proper failure checks for pushing' '
> -	(GIT_REMOTE_TESTGIT_FAILURE=1 &&
> -	export GIT_REMOTE_TESTGIT_FAILURE &&
> -	cd local &&
> -	test_must_fail git push --all
> +	(cd local &&
> +	test_must_fail env GIT_REMOTE_TESTGIT_FAILURE=1 git push --all
>  	)
>  '
>
> diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh
> index 9874403..9d9d9de 100755
> --- a/t/t6006-rev-list-format.sh
> +++ b/t/t6006-rev-list-format.sh
> @@ -190,12 +190,9 @@ test_expect_success '%C(auto) respects --no-color' '
>  '
>
>  test_expect_success TTY '%C(auto) respects --color=auto (stdout is tty)' '
> -	(
> -		TERM=vt100 && export TERM &&
> -		test_terminal \
> -			git log --format=$AUTO_COLOR -1 --color=auto >actual &&
> -		has_color actual
> -	)
> +	test_terminal env TERM=vt100 \
> +		git log --format=$AUTO_COLOR -1 --color=auto >actual &&
> +	has_color actual
>  '
>
>  test_expect_success '%C(auto) respects --color=auto (stdout not tty)' '
> diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
> index b9365b4..da958a8 100755
> --- a/t/t7006-pager.sh
> +++ b/t/t7006-pager.sh
> @@ -146,11 +146,7 @@ test_expect_success 'no color when stdout is a regular file' '
>  test_expect_success TTY 'color when writing to a pager' '
>  	rm -f paginated.out &&
>  	test_config color.ui auto &&
> -	(
> -		TERM=vt100 &&
> -		export TERM &&
> -		test_terminal git log
> -	) &&
> +	test_terminal env TERM=vt100 git log &&
>  	colorful paginated.out
>  '
>
> @@ -158,11 +154,7 @@ test_expect_success TTY 'colors are suppressed by color.pager' '
>  	rm -f paginated.out &&
>  	test_config color.ui auto &&
>  	test_config color.pager false &&
> -	(
> -		TERM=vt100 &&
> -		export TERM &&
> -		test_terminal git log
> -	) &&
> +	test_terminal env TERM=vt100 git log &&
>  	! colorful paginated.out
>  '
>
> @@ -181,11 +173,7 @@ test_expect_success 'color when writing to a file intended for a pager' '
>  test_expect_success TTY 'colors are sent to pager for external commands' '
>  	test_config alias.externallog "!git log" &&
>  	test_config color.ui auto &&
> -	(
> -		TERM=vt100 &&
> -		export TERM &&
> -		test_terminal git -p externallog
> -	) &&
> +	test_terminal env TERM=vt100 git -p externallog &&
>  	colorful paginated.out
>  '
>
> --
> 1.7.9

Re: [PATCH v2] tests: set temp variables using 'env' in test function instead of subshell

[Eric Sunshine]

On Tue, Mar 18, 2014 at 8:08 AM, David Tran <unsignedzero@gmail.com> wrote:
> Originally, the code used subshells instead of FOO=BAR command because
> the variable would otherwise leak into the surrounding context of the POSIX
> shell when 'command' is a shell function. The subshell was used to hold the
> context for the test. Using 'env' in the test function sets the temp variables
> without leaking, removing the need of a subshell.
>
> Signed-off-by: David Tran <unsignedzero@gmail.com>
> ---
>> Oh, really ;-)?
> Missed that.
>
>> Thanks.  Getting closer, I think.
> Slowly but surely.

Getting better. See below.

> Signed-off-by: David Tran <unsignedzero@gmail.com>
> ---
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 50e22b1..4c7364a 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -104,9 +104,7 @@ test_expect_success 'rebase -i with the exec command checks tree cleanness' '
>         git checkout master &&
>         (
>         set_fake_editor &&
> -       FAKE_LINES="exec_echo_foo_>file1 1" &&
> -       export FAKE_LINES &&
> -       test_must_fail git rebase -i HEAD^
> +       test_must_fail env FAKE_LINES="exec_echo_foo_>file1 1" git rebase -i HEAD^
>         ) &&

In a previous review, I asked if this subshell could be dropped or if
it was required for set_fake_editor. I didn't quite understand your
response, so I tested it myself, and found that the subshell can be
eliminated safely without breaking this or later tests.

>         test_cmp_rev master^ HEAD &&
>         git reset --hard &&
> @@ -118,9 +116,8 @@ test_expect_success 'rebase -i with exec of inexistent command' '
>         test_when_finished "git rebase --abort" &&
>         (
>         set_fake_editor &&
> -       FAKE_LINES="exec_this-command-does-not-exist 1" &&
> -       export FAKE_LINES &&
> -       test_must_fail git rebase -i HEAD^ >actual 2>&1
> +       test_must_fail env FAKE_LINES="exec_this-command-does-not-exist 1" \
> +       git rebase -i HEAD^ >actual 2>&1
>         ) &&

Ditto for this subshell.

>         ! grep "Maybe git-rebase is broken" actual
>  '
> @@ -528,11 +509,7 @@ test_expect_success 'aborted --continue does not squash commits after "edit"' '
>         FAKE_LINES="edit 1" git rebase -i HEAD^ &&
>         echo "edited again" > file7 &&
>         git add file7 &&
> -       (
> -               FAKE_COMMIT_MESSAGE=" " &&
> -               export FAKE_COMMIT_MESSAGE &&
> -               test_must_fail git rebase --continue
> -       ) &&
> +       test_must_fail env FAKE_COMMIT_MESSAGE=" " git rebase --continue

Broken &&-chain.

>         test $old = $(git rev-parse HEAD) &&
>         git rebase --abort
>  '

Re: [PATCH v2] tests: set temp variables using 'env' in test function instead of subshell

[Jeff King]

On Tue, Mar 18, 2014 at 01:37:39PM -0700, Junio C Hamano wrote:

> > diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
> > index c9c426c..3e3f77b 100755
> > --- a/t/t1300-repo-config.sh
> > +++ b/t/t1300-repo-config.sh
> > @@ -974,24 +974,15 @@ test_expect_success SYMLINKS 'symlinked configuration' '
> >  '
> >
> >  test_expect_success 'nonexistent configuration' '
> > -	(
> > -		GIT_CONFIG=doesnotexist &&
> > -		export GIT_CONFIG &&
> > -		test_must_fail git config --list &&
> > -		test_must_fail git config test.xyzzy
> > -	)
> > +	test_must_fail env GIT_CONFIG=doesnotexist git config --list &&
> > +	test_must_fail env GIT_CONFIG=doesnotexist git config test.xyzzy
> >  '

Isn't GIT_CONFIG here another way of saying:

  test_must_fail git config -f doesnotexist --list

Perhaps that is shorter and more readable still (and there are a few
similar cases in this patch.

-Peff

Re: [PATCH v2] tests: set temp variables using 'env' in test function instead of subshell

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Mar 18, 2014 at 01:37:39PM -0700, Junio C Hamano wrote:
>
>> > diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
>> > index c9c426c..3e3f77b 100755
>> > --- a/t/t1300-repo-config.sh
>> > +++ b/t/t1300-repo-config.sh
>> > @@ -974,24 +974,15 @@ test_expect_success SYMLINKS 'symlinked configuration' '
>> >  '
>> >
>> >  test_expect_success 'nonexistent configuration' '
>> > -	(
>> > -		GIT_CONFIG=doesnotexist &&
>> > -		export GIT_CONFIG &&
>> > -		test_must_fail git config --list &&
>> > -		test_must_fail git config test.xyzzy
>> > -	)
>> > +	test_must_fail env GIT_CONFIG=doesnotexist git config --list &&
>> > +	test_must_fail env GIT_CONFIG=doesnotexist git config test.xyzzy
>> >  '
>
> Isn't GIT_CONFIG here another way of saying:
>
>   test_must_fail git config -f doesnotexist --list
>
> Perhaps that is shorter and more readable still (and there are a few
> similar cases in this patch.

Surely, but are we assuming that "git config" correctly honors the
equivalence between GIT_CONFIG=file and -f file, or is that also
something we are testing in these tests?

Re: [PATCH v2] tests: set temp variables using 'env' in test function instead of subshell

[Eric Sunshine]

On Tue, Mar 18, 2014 at 5:45 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Mar 18, 2014 at 01:37:39PM -0700, Junio C Hamano wrote:
>
>> > diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
>> > index c9c426c..3e3f77b 100755
>> > --- a/t/t1300-repo-config.sh
>> > +++ b/t/t1300-repo-config.sh
>> > @@ -974,24 +974,15 @@ test_expect_success SYMLINKS 'symlinked configuration' '
>> >  '
>> >
>> >  test_expect_success 'nonexistent configuration' '
>> > -   (
>> > -           GIT_CONFIG=doesnotexist &&
>> > -           export GIT_CONFIG &&
>> > -           test_must_fail git config --list &&
>> > -           test_must_fail git config test.xyzzy
>> > -   )
>> > +   test_must_fail env GIT_CONFIG=doesnotexist git config --list &&
>> > +   test_must_fail env GIT_CONFIG=doesnotexist git config test.xyzzy
>> >  '
>
> Isn't GIT_CONFIG here another way of saying:
>
>   test_must_fail git config -f doesnotexist --list
>
> Perhaps that is shorter and more readable still (and there are a few
> similar cases in this patch.

Such a change could be the subject of a separate cleanup patch, but is
tangental to the GSoC microproject which begat this submission.

Re: [PATCH v2] tests: set temp variables using 'env' in test function instead of subshell

[Jeff King]

On Tue, Mar 18, 2014 at 03:16:27PM -0700, Junio C Hamano wrote:

> > Isn't GIT_CONFIG here another way of saying:
> >
> >   test_must_fail git config -f doesnotexist --list
> >
> > Perhaps that is shorter and more readable still (and there are a few
> > similar cases in this patch.
> 
> Surely, but are we assuming that "git config" correctly honors the
> equivalence between GIT_CONFIG=file and -f file, or is that also
> something we are testing in these tests?

I think we can assume that they are equivalent, and it is not worth
testing (and they are equivalent in code since 270a344 (config: stop
using config_exclusive_filename, 2012-02-16).

My recollection is that GIT_CONFIG mostly exists as a historical
footnote. Recall that at one time it affected all commands, but that had
many problems and was done away with in dc87183 (Only use GIT_CONFIG in
"git config", not other programs, 2008-06-30). I think we left it in
place for git-config mostly for backward compatibility, but I didn't see
that point explicitly addressed in the list discussion (the main issue
was that setting it for things besides "git config" is a bad idea, as it
suppresses ~/.gitconfig).

-Peff

Re: [PATCH v2] tests: set temp variables using 'env' in test function instead of subshell

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Mar 18, 2014 at 03:16:27PM -0700, Junio C Hamano wrote:
>
>> > Isn't GIT_CONFIG here another way of saying:
>> >
>> >   test_must_fail git config -f doesnotexist --list
>> >
>> > Perhaps that is shorter and more readable still (and there are a few
>> > similar cases in this patch.
>> 
>> Surely, but are we assuming that "git config" correctly honors the
>> equivalence between GIT_CONFIG=file and -f file, or is that also
>> something we are testing in these tests?
>
> I think we can assume that they are equivalent, and it is not worth
> testing (and they are equivalent in code since 270a344 (config: stop
> using config_exclusive_filename, 2012-02-16).
>
> My recollection is that GIT_CONFIG mostly exists as a historical
> footnote. Recall that at one time it affected all commands, but that had
> many problems and was done away with in dc87183 (Only use GIT_CONFIG in
> "git config", not other programs, 2008-06-30). I think we left it in
> place for git-config mostly for backward compatibility,...

Thanks.  Then I think it makes sense to do such a conversion but it
probably should be done on top of this patch (we could do it before
this patch), not as a part of this patch.

[PATCH 0/12] GIT_CONFIG in the test suite

[Jeff King]

On Wed, Mar 19, 2014 at 10:28:46AM -0700, Junio C Hamano wrote:

> [git config --file versus GIT_CONFIG=]
>
> Thanks.  Then I think it makes sense to do such a conversion but it
> probably should be done on top of this patch (we could do it before
> this patch), not as a part of this patch.

Here's a series that goes on top of what you queued in
dt/tests-with-env-not-subshell. Once I started cleaning, I noticed a lot
of room for improvement and modernization in t0001. I hope I didn't get
too carried away.

  [01/12]: t/Makefile: stop setting GIT_CONFIG
  [02/12]: t/test-lib: drop redundant unset of GIT_CONFIG
  [03/12]: t: drop useless sane_unset GIT_* calls
  [04/12]: t: stop using GIT_CONFIG to cross repo boundaries
  [05/12]: t: prefer "git config --file" to GIT_CONFIG with test_must_fail
  [06/12]: t: prefer "git config --file" to GIT_CONFIG
  [07/12]: t0001: make symlink reinit test more careful
  [08/12]: t0001: use test_path_is_*
  [09/12]: t0001: use test_config_global
  [10/12]: t0001: use test_must_fail
  [11/12]: t0001: drop useless subshells
  [12/12]: t0001: drop subshells just for "cd"

 t/Makefile                      |   4 +-
 t/t0001-init.sh                 | 211 ++++++++++-------------------------
 t/t1300-repo-config.sh          |  28 ++---
 t/t1302-repo-version.sh         |   2 +-
 t/t5701-clone-local.sh          |   6 +-
 t/t7400-submodule-basic.sh      |   5 +-
 t/t9130-git-svn-authors-file.sh |   3 +-
 t/t9154-git-svn-fancy-glob.sh   |   6 +-
 t/t9400-git-cvsserver-server.sh |   1 -
 t/test-lib.sh                   |   1 -
 10 files changed, 87 insertions(+), 180 deletions(-)

-Peff

[PATCH 01/12] t/Makefile: stop setting GIT_CONFIG

[Jeff King]

Once upon a time, the setting of GIT_CONFIG in the
environment could affect how tests ran. Commit 9c3796f (Fix
setting config variables with an alternative GIT_CONFIG,
2006-06-20) unconditionally set GIT_CONFIG in the Makefile
when running tests to give us a known starting point.

This is insufficient for running the tests outside of the
Makefile, however, and 8565d2d (Make tests independent of
global config files, 2007-02-15) later set GIT_CONFIG
directly in test-lib.sh. At that point the Makefile setting
was redundant, but we never removed it. Let's do so now.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/Makefile b/t/Makefile
index 2373a04..8fd1a72 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -36,11 +36,11 @@ test: pre-clean $(TEST_LINT)
 	$(MAKE) aggregate-results-and-cleanup
 
 prove: pre-clean $(TEST_LINT)
-	@echo "*** prove ***"; GIT_CONFIG=.git/config $(PROVE) --exec '$(SHELL_PATH_SQ)' $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS)
+	@echo "*** prove ***"; $(PROVE) --exec '$(SHELL_PATH_SQ)' $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS)
 	$(MAKE) clean-except-prove-cache
 
 $(T):
-	@echo "*** $@ ***"; GIT_CONFIG=.git/config '$(SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS)
+	@echo "*** $@ ***"; '$(SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS)
 
 pre-clean:
 	$(RM) -r '$(TEST_RESULTS_DIRECTORY_SQ)'
-- 
1.9.0.560.g01ceb46

[PATCH 02/12] t/test-lib: drop redundant unset of GIT_CONFIG

[Jeff King]

This is already handled by the mass GIT_* unsetting added by
95a1d12 (tests: scrub environment of GIT_* variables,
2011-03-15).

Signed-off-by: Jeff King <peff@peff.net>
---
 t/test-lib.sh | 1 -
 1 file changed, 1 deletion(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 1531c24..625f06e 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -649,7 +649,6 @@ else # normal case, use ../bin-wrappers only unless $with_dashes:
 	fi
 fi
 GIT_TEMPLATE_DIR="$GIT_BUILD_DIR"/templates/blt
-unset GIT_CONFIG
 GIT_CONFIG_NOSYSTEM=1
 GIT_ATTR_NOSYSTEM=1
 export PATH GIT_EXEC_PATH GIT_TEMPLATE_DIR GIT_CONFIG_NOSYSTEM GIT_ATTR_NOSYSTEM
-- 
1.9.0.560.g01ceb46

[PATCH 03/12] t: drop useless sane_unset GIT_* calls

[Jeff King]

Several test scripts manually unset GIT_CONFIG and other
GIT_* variables. These are generally taken care of for us by
test-lib.sh already.

Unsetting these is not only useless, but can be confusing to
a reader, who may wonder why some tests in a script unset
them and others do not (t0001 is particularly guilty of this
inconsistency, probably because many of its tests predate
the test-lib.sh environment-cleansing).

Note that we cannot always get rid of such unsetting. For
example, t9130 can drop the GIT_CONFIG unset, but not the
GIT_DIR one, because lib-git-svn.sh sets the latter. And in
t1000, we unset GIT_TEMPLATE_DIR, which is explicitly
initialized by test-lib.sh.

Signed-off-by: Jeff King <peff@peff.net>
---
I suppose one could make an argument that test-lib.sh may later change
the set of variables it clears, and these unsets are documenting an
explicit need of each test. I'd find that more compelling if it were
actually applied consistently.

 t/t0001-init.sh                 | 15 ---------------
 t/t9130-git-svn-authors-file.sh |  1 -
 t/t9400-git-cvsserver-server.sh |  1 -
 3 files changed, 17 deletions(-)

diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 9fb582b..ddc8160 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -25,7 +25,6 @@ check_config () {
 
 test_expect_success 'plain' '
 	(
-		sane_unset GIT_DIR GIT_WORK_TREE &&
 		mkdir plain &&
 		cd plain &&
 		git init
@@ -35,7 +34,6 @@ test_expect_success 'plain' '
 
 test_expect_success 'plain nested in bare' '
 	(
-		sane_unset GIT_DIR GIT_WORK_TREE &&
 		git init --bare bare-ancestor.git &&
 		cd bare-ancestor.git &&
 		mkdir plain-nested &&
@@ -47,7 +45,6 @@ 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 &&
 		HOME=$(pwd)/alias-config &&
 		export HOME &&
 		mkdir alias-config &&
@@ -65,7 +62,6 @@ test_expect_success 'plain through aliased command, outside any git repo' '
 
 test_expect_failure 'plain nested through aliased command' '
 	(
-		sane_unset GIT_DIR GIT_WORK_TREE &&
 		git init plain-ancestor-aliased &&
 		cd plain-ancestor-aliased &&
 		echo "[alias] aliasedinit = init" >>.git/config &&
@@ -78,7 +74,6 @@ test_expect_failure 'plain nested through aliased command' '
 
 test_expect_failure 'plain nested in bare through aliased command' '
 	(
-		sane_unset GIT_DIR GIT_WORK_TREE &&
 		git init --bare bare-ancestor-aliased.git &&
 		cd bare-ancestor-aliased.git &&
 		echo "[alias] aliasedinit = init" >>config &&
@@ -91,7 +86,6 @@ test_expect_failure 'plain nested in bare through aliased command' '
 
 test_expect_success 'plain with GIT_WORK_TREE' '
 	if (
-		sane_unset GIT_DIR &&
 		mkdir plain-wt &&
 		cd plain-wt &&
 		GIT_WORK_TREE=$(pwd) git init
@@ -104,7 +98,6 @@ test_expect_success 'plain with GIT_WORK_TREE' '
 
 test_expect_success 'plain bare' '
 	(
-		sane_unset GIT_DIR GIT_WORK_TREE GIT_CONFIG &&
 		mkdir plain-bare-1 &&
 		cd plain-bare-1 &&
 		git --bare init
@@ -114,7 +107,6 @@ test_expect_success 'plain bare' '
 
 test_expect_success 'plain bare with GIT_WORK_TREE' '
 	if (
-		sane_unset GIT_DIR GIT_CONFIG &&
 		mkdir plain-bare-2 &&
 		cd plain-bare-2 &&
 		GIT_WORK_TREE=$(pwd) git --bare init
@@ -128,7 +120,6 @@ test_expect_success 'plain bare with GIT_WORK_TREE' '
 test_expect_success 'GIT_DIR bare' '
 
 	(
-		sane_unset GIT_CONFIG &&
 		mkdir git-dir-bare.git &&
 		GIT_DIR=git-dir-bare.git git init
 	) &&
@@ -138,7 +129,6 @@ test_expect_success 'GIT_DIR bare' '
 test_expect_success 'init --bare' '
 
 	(
-		sane_unset GIT_DIR GIT_WORK_TREE GIT_CONFIG &&
 		mkdir init-bare.git &&
 		cd init-bare.git &&
 		git init --bare
@@ -149,7 +139,6 @@ test_expect_success 'init --bare' '
 test_expect_success 'GIT_DIR non-bare' '
 
 	(
-		sane_unset GIT_CONFIG &&
 		mkdir non-bare &&
 		cd non-bare &&
 		GIT_DIR=.git git init
@@ -160,7 +149,6 @@ test_expect_success 'GIT_DIR non-bare' '
 test_expect_success 'GIT_DIR & GIT_WORK_TREE (1)' '
 
 	(
-		sane_unset GIT_CONFIG &&
 		mkdir git-dir-wt-1.git &&
 		GIT_WORK_TREE=$(pwd) GIT_DIR=git-dir-wt-1.git git init
 	) &&
@@ -170,7 +158,6 @@ test_expect_success 'GIT_DIR & GIT_WORK_TREE (1)' '
 test_expect_success 'GIT_DIR & GIT_WORK_TREE (2)' '
 
 	if (
-		sane_unset GIT_CONFIG &&
 		mkdir git-dir-wt-2.git &&
 		GIT_WORK_TREE=$(pwd) GIT_DIR=git-dir-wt-2.git git --bare init
 	)
@@ -183,8 +170,6 @@ test_expect_success 'GIT_DIR & GIT_WORK_TREE (2)' '
 test_expect_success 'reinit' '
 
 	(
-		sane_unset GIT_CONFIG GIT_WORK_TREE GIT_CONFIG &&
-
 		mkdir again &&
 		cd again &&
 		git init >out1 2>err1 &&
diff --git a/t/t9130-git-svn-authors-file.sh b/t/t9130-git-svn-authors-file.sh
index c3443ce..a812783 100755
--- a/t/t9130-git-svn-authors-file.sh
+++ b/t/t9130-git-svn-authors-file.sh
@@ -97,7 +97,6 @@ test_expect_success 'fresh clone with svn.authors-file in config' '
 		test x = x"$(git config svn.authorsfile)" &&
 		test_config="$HOME"/.gitconfig &&
 		sane_unset GIT_DIR &&
-		sane_unset GIT_CONFIG &&
 		git config --global \
 		  svn.authorsfile "$HOME"/svn-authors &&
 		test x"$HOME"/svn-authors = x"$(git config svn.authorsfile)" &&
diff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh
index 3edc408..ed98e64 100755
--- a/t/t9400-git-cvsserver-server.sh
+++ b/t/t9400-git-cvsserver-server.sh
@@ -25,7 +25,6 @@ perl -e 'use DBI; use DBD::SQLite' >/dev/null 2>&1 || {
     test_done
 }
 
-unset GIT_DIR GIT_CONFIG
 WORKDIR=$(pwd)
 SERVERDIR=$(pwd)/gitcvs.git
 git_config="$SERVERDIR/config"
-- 
1.9.0.560.g01ceb46

[PATCH 04/12] t: stop using GIT_CONFIG to cross repo boundaries

[Jeff King]

Some tests want to check or set config in another
repository. E.g., t1000 creates repositories and makes sure
that their core.bare and core.worktree settings are what we
expect. We can do this with:

  GIT_CONFIG=$repo/.git/config git config ...

but it better shows the intent to just enter the repository
and let "git config" do the normal lookups:

  (cd $repo && git config ...)

In theory, this would cause us to use an extra subshell, but
in all such cases, we are actually already in a subshell.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t0001-init.sh        | 4 ++--
 t/t5701-clone-local.sh | 6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index ddc8160..9b05fdf 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -12,8 +12,8 @@ check_config () {
 		echo "expected a directory $1, a file $1/config and $1/refs"
 		return 1
 	fi
-	bare=$(GIT_CONFIG="$1/config" git config --bool core.bare)
-	worktree=$(GIT_CONFIG="$1/config" git config core.worktree) ||
+	bare=$(cd "$1" && git config --bool core.bare)
+	worktree=$(cd "$1" && git config core.worktree) ||
 	worktree=unset
 
 	test "$bare" = "$2" && test "$worktree" = "$3" || {
diff --git a/t/t5701-clone-local.sh b/t/t5701-clone-local.sh
index c490368..3c087e9 100755
--- a/t/t5701-clone-local.sh
+++ b/t/t5701-clone-local.sh
@@ -12,8 +12,8 @@ test_expect_success 'preparing origin repository' '
 	: >file && git add . && git commit -m1 &&
 	git clone --bare . a.git &&
 	git clone --bare . x &&
-	test "$(GIT_CONFIG=a.git/config git config --bool core.bare)" = true &&
-	test "$(GIT_CONFIG=x/config git config --bool core.bare)" = true &&
+	test "$(cd a.git && git config --bool core.bare)" = true &&
+	test "$(cd x && git config --bool core.bare)" = true &&
 	git bundle create b1.bundle --all &&
 	git bundle create b2.bundle master &&
 	mkdir dir &&
@@ -24,7 +24,7 @@ test_expect_success 'preparing origin repository' '
 test_expect_success 'local clone without .git suffix' '
 	git clone -l -s a b &&
 	(cd b &&
-	test "$(GIT_CONFIG=.git/config git config --bool core.bare)" = false &&
+	test "$(git config --bool core.bare)" = false &&
 	git fetch)
 '
 
-- 
1.9.0.560.g01ceb46

[PATCH 05/12] t: prefer "git config --file" to GIT_CONFIG with test_must_fail

[Jeff King]

This lets us get rid of an extra "env" invocation in the
middle, and is slightly more readable.

Signed-off-by: Jeff King <peff@peff.net>
---
The case that started this all...

This is also the only reason this series needs to go on top of David's
patch.

 t/t1300-repo-config.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index cd23d07..e355aa1 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -961,15 +961,15 @@ test_expect_success SYMLINKS 'symlinked configuration' '
 '
 
 test_expect_success 'nonexistent configuration' '
-	test_must_fail env GIT_CONFIG=doesnotexist git config --list &&
-	test_must_fail env GIT_CONFIG=doesnotexist git config test.xyzzy
+	test_must_fail git config --file=doesnotexist --list &&
+	test_must_fail git config --file=doesnotexist test.xyzzy
 '
 
 test_expect_success SYMLINKS 'symlink to nonexistent configuration' '
 	ln -s doesnotexist linktonada &&
 	ln -s linktonada linktolinktonada &&
-	test_must_fail env GIT_CONFIG=linktonada git config --list &&
-	test_must_fail env GIT_CONFIG=linktolinktonada git config --list
+	test_must_fail git config --file=linktonada --list &&
+	test_must_fail git config --file=linktolinktonada --list
 '
 
 test_expect_success 'check split_cmdline return' "
-- 
1.9.0.560.g01ceb46

[PATCH 06/12] t: prefer "git config --file" to GIT_CONFIG

[Jeff King]

Doing:

  GIT_CONFIG=foo git config ...

is equivalent to:

  git config --file=foo ...

The latter is easier to read and slightly less error-prone,
because of issues with one-shot variables and shell
functions (e.g., you cannot use the former with
test_must_fail).

Note that we explicitly leave one case in t1300 which checks
the same operation on both GIT_CONFIG and "git config
--file". They are equivalent in the code these days, but
this will make sure it remains so.

Signed-off-by: Jeff King <peff@peff.net>
---
Unlike the last patch, this one has no tangible benefits besides "Peff
thinks it looks better". I also tend to think that GIT_CONFIG is
something that it would be nice to get rid of in the long run, but I
don't have any immediate plans to do so.

 t/t1300-repo-config.sh          | 20 ++++++++++----------
 t/t1302-repo-version.sh         |  2 +-
 t/t7400-submodule-basic.sh      |  5 ++---
 t/t9130-git-svn-authors-file.sh |  2 +-
 t/t9154-git-svn-fancy-glob.sh   |  6 +++---
 5 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index e355aa1..85c6637 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -461,7 +461,7 @@ test_expect_success 'new variable inserts into proper section' '
 	test_cmp expect .git/config
 '
 
-test_expect_success 'alternative GIT_CONFIG (non-existing file should fail)' '
+test_expect_success 'alternative --file (non-existing file should fail)' '
 	test_must_fail git config --file non-existing-config -l
 '
 
@@ -495,10 +495,10 @@ test_expect_success 'refer config from subdirectory' '
 
 '
 
-test_expect_success 'refer config from subdirectory via GIT_CONFIG' '
+test_expect_success 'refer config from subdirectory via --file' '
 	(
 		cd x &&
-		GIT_CONFIG=../other-config git config --get ein.bahn >actual &&
+		git config --file=../other-config --get ein.bahn >actual &&
 		test_cmp expect actual
 	)
 '
@@ -510,8 +510,8 @@ cat > expect << EOF
 	park = ausweis
 EOF
 
-test_expect_success '--set in alternative GIT_CONFIG' '
-	GIT_CONFIG=other-config git config anwohner.park ausweis &&
+test_expect_success '--set in alternative file' '
+	git config --file=other-config anwohner.park ausweis &&
 	test_cmp expect other-config
 '
 
@@ -942,11 +942,11 @@ test_expect_success 'inner whitespace kept verbatim' '
 
 test_expect_success SYMLINKS 'symlinked configuration' '
 	ln -s notyet myconfig &&
-	GIT_CONFIG=myconfig git config test.frotz nitfol &&
+	git config --file=myconfig test.frotz nitfol &&
 	test -h myconfig &&
 	test -f notyet &&
-	test "z$(GIT_CONFIG=notyet git config test.frotz)" = znitfol &&
-	GIT_CONFIG=myconfig git config test.xyzzy rezrov &&
+	test "z$(git config --file=notyet test.frotz)" = znitfol &&
+	git config --file=myconfig test.xyzzy rezrov &&
 	test -h myconfig &&
 	test -f notyet &&
 	cat >expect <<-\EOF &&
@@ -954,8 +954,8 @@ test_expect_success SYMLINKS 'symlinked configuration' '
 	rezrov
 	EOF
 	{
-		GIT_CONFIG=notyet git config test.frotz &&
-		GIT_CONFIG=notyet git config test.xyzzy
+		git config --file=notyet test.frotz &&
+		git config --file=notyet test.xyzzy
 	} >actual &&
 	test_cmp expect actual
 '
diff --git a/t/t1302-repo-version.sh b/t/t1302-repo-version.sh
index 0e47662..0d9388a 100755
--- a/t/t1302-repo-version.sh
+++ b/t/t1302-repo-version.sh
@@ -19,7 +19,7 @@ test_expect_success 'setup' '
 
 	test_create_repo "test" &&
 	test_create_repo "test2" &&
-	GIT_CONFIG=test2/.git/config git config core.repositoryformatversion 99
+	git config --file=test2/.git/config core.repositoryformatversion 99
 '
 
 test_expect_success 'gitdir selection on normal repos' '
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index c28e8d8..7c88245 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -249,8 +249,7 @@ test_expect_success 'submodule add in subdirectory with relative path should fai
 '
 
 test_expect_success 'setup - add an example entry to .gitmodules' '
-	GIT_CONFIG=.gitmodules \
-	git config submodule.example.url git://example.com/init.git
+	git config --file=.gitmodules submodule.example.url git://example.com/init.git
 '
 
 test_expect_success 'status should fail for unmapped paths' '
@@ -264,7 +263,7 @@ test_expect_success 'setup - map path in .gitmodules' '
 	path = init
 EOF
 
-	GIT_CONFIG=.gitmodules git config submodule.example.path init &&
+	git config --file=.gitmodules submodule.example.path init &&
 
 	test_cmp expect .gitmodules
 '
diff --git a/t/t9130-git-svn-authors-file.sh b/t/t9130-git-svn-authors-file.sh
index a812783..c44de26 100755
--- a/t/t9130-git-svn-authors-file.sh
+++ b/t/t9130-git-svn-authors-file.sh
@@ -67,7 +67,7 @@ test_expect_success 'fetch fails on ee' '
 	'
 
 tmp_config_get () {
-	GIT_CONFIG=.git/svn/.metadata git config --get "$1"
+	git config --file=.git/svn/.metadata --get "$1"
 }
 
 test_expect_success 'failure happened without negative side effects' '
diff --git a/t/t9154-git-svn-fancy-glob.sh b/t/t9154-git-svn-fancy-glob.sh
index b780e0e..a0150f0 100755
--- a/t/t9154-git-svn-fancy-glob.sh
+++ b/t/t9154-git-svn-fancy-glob.sh
@@ -22,7 +22,7 @@ test_expect_success 'add red branch' "
 	"
 
 test_expect_success 'add gre branch' "
-	GIT_CONFIG=.git/svn/.metadata git config --unset svn-remote.svn.branches-maxRev &&
+	git config --file=.git/svn/.metadata --unset svn-remote.svn.branches-maxRev &&
 	git config svn-remote.svn.branches 'branches/{red,gre}:refs/remotes/*' &&
 	git svn fetch &&
 	git rev-parse refs/remotes/red &&
@@ -31,7 +31,7 @@ test_expect_success 'add gre branch' "
 	"
 
 test_expect_success 'add green branch' "
-	GIT_CONFIG=.git/svn/.metadata git config --unset svn-remote.svn.branches-maxRev &&
+	git config --file=.git/svn/.metadata --unset svn-remote.svn.branches-maxRev &&
 	git config svn-remote.svn.branches 'branches/{red,green}:refs/remotes/*' &&
 	git svn fetch &&
 	git rev-parse refs/remotes/red &&
@@ -40,7 +40,7 @@ test_expect_success 'add green branch' "
 	"
 
 test_expect_success 'add all branches' "
-	GIT_CONFIG=.git/svn/.metadata git config --unset svn-remote.svn.branches-maxRev &&
+	git config --file=.git/svn/.metadata --unset svn-remote.svn.branches-maxRev &&
 	git config svn-remote.svn.branches 'branches/*:refs/remotes/*' &&
 	git svn fetch &&
 	git rev-parse refs/remotes/red &&
-- 
1.9.0.560.g01ceb46

[PATCH 07/12] t0001: make symlink reinit test more careful

[Jeff King]

In the final test of t0001, we have a repo whose .git is a
symlink to a directory "here", and we use
"--separate-git-dir" to migrate that to a .git file pointing
to a different directory. We check that the data is migrated
to the new directory and that .git looks like a git-file.

We also check that "here" is not a directory, which is
slightly misleading. It should not be a directory, but
neither should it be gone. It is the actual resting place of
the git-file, and .git remains a symlink to it.

Let's check that more explicitly, both to make our test more
robust, and to make further cleanups in this area more
obvious.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t0001-init.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 9b05fdf..5245711 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -402,8 +402,8 @@ test_expect_success SYMLINKS 're-init to move gitdir symlink' '
 	) &&
 	echo "gitdir: `pwd`/realgitdir" >expected &&
 	test_cmp expected newdir/.git &&
-	test -d realgitdir/refs &&
-	! test -d newdir/here
+	test_cmp expected newdir/here &&
+	test -d realgitdir/refs
 '
 
 test_done
-- 
1.9.0.560.g01ceb46

[PATCH 08/12] t0001: use test_path_is_*

[Jeff King]

t0001 predates the test_path_is_* helpers, and uses "test
-f" and "test -d" directly. Using the helpers provides
better debugging output, and are a little more robust.
As opposed to "! test -d", test_path_is_missing will
actually makes sure the path does not exist at all.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t0001-init.sh | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 5245711..fdcf4b3 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -199,13 +199,13 @@ test_expect_success 'init with --template (blank)' '
 		cd template-plain &&
 		git init
 	) &&
-	test -f template-plain/.git/info/exclude &&
+	test_path_is_file template-plain/.git/info/exclude &&
 	(
 		mkdir template-blank &&
 		cd template-blank &&
 		git init --template=
 	) &&
-	! test -f template-blank/.git/info/exclude
+	test_path_is_missing template-blank/.git/info/exclude
 '
 
 test_expect_success 'init with init.templatedir set' '
@@ -263,7 +263,7 @@ test_expect_success 'init creates a new directory' '
 	rm -fr newdir &&
 	(
 		git init newdir &&
-		test -d newdir/.git/refs
+		test_path_is_dir newdir/.git/refs
 	)
 '
 
@@ -271,7 +271,7 @@ test_expect_success 'init creates a new bare directory' '
 	rm -fr newdir &&
 	(
 		git init --bare newdir &&
-		test -d newdir/refs
+		test_path_is_dir newdir/refs
 	)
 '
 
@@ -280,7 +280,7 @@ test_expect_success 'init recreates a directory' '
 	(
 		mkdir newdir &&
 		git init newdir &&
-		test -d newdir/.git/refs
+		test_path_is_dir newdir/.git/refs
 	)
 '
 
@@ -289,14 +289,14 @@ test_expect_success 'init recreates a new bare directory' '
 	(
 		mkdir newdir &&
 		git init --bare newdir &&
-		test -d newdir/refs
+		test_path_is_dir newdir/refs
 	)
 '
 
 test_expect_success 'init creates a new deep directory' '
 	rm -fr newdir &&
 	git init newdir/a/b/c &&
-	test -d newdir/a/b/c/.git/refs
+	test_path_is_dir newdir/a/b/c/.git/refs
 '
 
 test_expect_success POSIXPERM 'init creates a new deep directory (umask vs. shared)' '
@@ -306,7 +306,7 @@ test_expect_success POSIXPERM 'init creates a new deep directory (umask vs. shar
 		# the repository itself should follow "shared"
 		umask 002 &&
 		git init --bare --shared=0660 newdir/a/b/c &&
-		test -d newdir/a/b/c/refs &&
+		test_path_is_dir newdir/a/b/c/refs &&
 		ls -ld newdir/a newdir/a/b > lsab.out &&
 		! grep -v "^drwxrw[sx]r-x" lsab.out &&
 		ls -ld newdir/a/b/c > lsc.out &&
@@ -319,7 +319,7 @@ test_expect_success 'init notices EEXIST (1)' '
 	(
 		>newdir &&
 		test_must_fail git init newdir &&
-		test -f newdir
+		test_path_is_file newdir
 	)
 '
 
@@ -329,7 +329,7 @@ test_expect_success 'init notices EEXIST (2)' '
 		mkdir newdir &&
 		>newdir/a
 		test_must_fail git init newdir/a/b &&
-		test -f newdir/a
+		test_path_is_file newdir/a
 	)
 '
 
@@ -345,15 +345,15 @@ test_expect_success POSIXPERM,SANITY 'init notices EPERM' '
 test_expect_success 'init creates a new bare directory with global --bare' '
 	rm -rf newdir &&
 	git --bare init newdir &&
-	test -d newdir/refs
+	test_path_is_dir newdir/refs
 '
 
 test_expect_success 'init prefers command line to GIT_DIR' '
 	rm -rf newdir &&
 	mkdir otherdir &&
 	GIT_DIR=otherdir git --bare init newdir &&
-	test -d newdir/refs &&
-	! test -d otherdir/refs
+	test_path_is_dir newdir/refs &&
+	test_path_is_missing otherdir/refs
 '
 
 test_expect_success 'init with separate gitdir' '
@@ -361,7 +361,7 @@ test_expect_success 'init with separate gitdir' '
 	git init --separate-git-dir realgitdir newdir &&
 	echo "gitdir: `pwd`/realgitdir" >expected &&
 	test_cmp expected newdir/.git &&
-	test -d realgitdir/refs
+	test_path_is_dir realgitdir/refs
 '
 
 test_expect_success 're-init on .git file' '
@@ -375,8 +375,8 @@ test_expect_success 're-init to update git link' '
 	) &&
 	echo "gitdir: `pwd`/surrealgitdir" >expected &&
 	test_cmp expected newdir/.git &&
-	test -d surrealgitdir/refs &&
-	! test -d realgitdir/refs
+	test_path_is_dir surrealgitdir/refs &&
+	test_path_is_missing realgitdir/refs
 '
 
 test_expect_success 're-init to move gitdir' '
@@ -388,7 +388,7 @@ test_expect_success 're-init to move gitdir' '
 	) &&
 	echo "gitdir: `pwd`/realgitdir" >expected &&
 	test_cmp expected newdir/.git &&
-	test -d realgitdir/refs
+	test_path_is_dir realgitdir/refs
 '
 
 test_expect_success SYMLINKS 're-init to move gitdir symlink' '
@@ -403,7 +403,7 @@ test_expect_success SYMLINKS 're-init to move gitdir symlink' '
 	echo "gitdir: `pwd`/realgitdir" >expected &&
 	test_cmp expected newdir/.git &&
 	test_cmp expected newdir/here &&
-	test -d realgitdir/refs
+	test_path_is_dir realgitdir/refs
 '
 
 test_done
-- 
1.9.0.560.g01ceb46

[PATCH 09/12] t0001: use test_config_global

[Jeff King]

We hand-set several config options using :

  git config -f $HOME/.gitconfig ...

Instead, we can use "test_config_global". Not only is this
more readable, but it cleans up for us so that subsequent
tests aren't polluted by our settings.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t0001-init.sh | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index fdcf4b3..9515da3 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -211,9 +211,8 @@ test_expect_success 'init with --template (blank)' '
 test_expect_success 'init with init.templatedir set' '
 	mkdir templatedir-source &&
 	echo Content >templatedir-source/file &&
+	test_config_global init.templatedir "${HOME}/templatedir-source" &&
 	(
-		test_config="${HOME}/.gitconfig" &&
-		git config -f "$test_config"  init.templatedir "${HOME}/templatedir-source" &&
 		mkdir templatedir-set &&
 		cd templatedir-set &&
 		sane_unset GIT_TEMPLATE_DIR &&
@@ -225,10 +224,9 @@ test_expect_success 'init with init.templatedir set' '
 '
 
 test_expect_success 'init --bare/--shared overrides system/global config' '
+	test_config_global core.bare false &&
+	test_config_global core.sharedRepository 0640 &&
 	(
-		test_config="$HOME"/.gitconfig &&
-		git config -f "$test_config" core.bare false &&
-		git config -f "$test_config" core.sharedRepository 0640 &&
 		mkdir init-bare-shared-override &&
 		cd init-bare-shared-override &&
 		git init --bare --shared=0666
@@ -239,9 +237,8 @@ test_expect_success 'init --bare/--shared overrides system/global config' '
 '
 
 test_expect_success 'init honors global core.sharedRepository' '
+	test_config_global core.sharedRepository 0666 &&
 	(
-		test_config="$HOME"/.gitconfig &&
-		git config -f "$test_config" core.sharedRepository 0666 &&
 		mkdir shared-honor-global &&
 		cd shared-honor-global &&
 		git init
-- 
1.9.0.560.g01ceb46

[PATCH 10/12] t0001: use test_must_fail

[Jeff King]

We've hand-rolled several "if" statements looking for
failures. We can use test_must_fail here, which is shorter
and more robust.

Note that we modify the commands slightly (to use "git init
foo" rather than "cd foo && git init") to avoid dealing with
a subshell, but this should not affect the outcome.

Signed-off-by: Jeff King <peff@peff.net>
---
I'm pretty sure we can actually drop the "mkdir" in each of
these cases, too, but I was trying to leave things as close
to the original as possible.

 t/t0001-init.sh | 38 +++++++++++---------------------------
 1 file changed, 11 insertions(+), 27 deletions(-)

diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 9515da3..4560bba 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -85,15 +85,8 @@ test_expect_failure 'plain nested in bare through aliased command' '
 '
 
 test_expect_success 'plain with GIT_WORK_TREE' '
-	if (
-		mkdir plain-wt &&
-		cd plain-wt &&
-		GIT_WORK_TREE=$(pwd) git init
-	)
-	then
-		echo Should have failed -- GIT_WORK_TREE should not be used
-		false
-	fi
+	mkdir plain-wt &&
+	test_must_fail env GIT_WORK_TREE="$(pwd)/plain-wt" git init plain-wt
 '
 
 test_expect_success 'plain bare' '
@@ -106,15 +99,10 @@ test_expect_success 'plain bare' '
 '
 
 test_expect_success 'plain bare with GIT_WORK_TREE' '
-	if (
-		mkdir plain-bare-2 &&
-		cd plain-bare-2 &&
-		GIT_WORK_TREE=$(pwd) git --bare init
-	)
-	then
-		echo Should have failed -- GIT_WORK_TREE should not be used
-		false
-	fi
+	mkdir plain-bare-2 &&
+	test_must_fail \
+		env GIT_WORK_TREE="$(pwd)/plain-bare-2" \
+		git --bare init plain-bare-2
 '
 
 test_expect_success 'GIT_DIR bare' '
@@ -156,15 +144,11 @@ test_expect_success 'GIT_DIR & GIT_WORK_TREE (1)' '
 '
 
 test_expect_success 'GIT_DIR & GIT_WORK_TREE (2)' '
-
-	if (
-		mkdir git-dir-wt-2.git &&
-		GIT_WORK_TREE=$(pwd) GIT_DIR=git-dir-wt-2.git git --bare init
-	)
-	then
-		echo Should have failed -- --bare should not be used
-		false
-	fi
+	mkdir git-dir-wt-2.git &&
+	test_must_fail env \
+		GIT_WORK_TREE="$(pwd)" \
+		GIT_DIR=git-dir-wt-2.git \
+		git --bare init
 '
 
 test_expect_success 'reinit' '
-- 
1.9.0.560.g01ceb46

[PATCH 11/12] t0001: drop useless subshells

[Jeff King]

Many tests use subshells, but don't actually change the
shell environment. They were probably cargo-culted from
earlier tests which did need subshells. Drop the useless
ones.

Signed-off-by: Jeff King <peff@peff.net>
---
These ones should produce no behavior change at all; they're purely
mechanical "(foo && bar)" to "foo && bar" (though of course I did them
by hand, because you need to know that "foo" and "bar" do not affect the
environment).

 t/t0001-init.sh | 61 +++++++++++++++++++++------------------------------------
 1 file changed, 22 insertions(+), 39 deletions(-)

diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 4560bba..55a68bc 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -106,11 +106,8 @@ test_expect_success 'plain bare with GIT_WORK_TREE' '
 '
 
 test_expect_success 'GIT_DIR bare' '
-
-	(
-		mkdir git-dir-bare.git &&
-		GIT_DIR=git-dir-bare.git git init
-	) &&
+	mkdir git-dir-bare.git &&
+	GIT_DIR=git-dir-bare.git git init &&
 	check_config git-dir-bare.git true unset
 '
 
@@ -242,36 +239,28 @@ test_expect_success 'init rejects insanely long --template' '
 
 test_expect_success 'init creates a new directory' '
 	rm -fr newdir &&
-	(
-		git init newdir &&
-		test_path_is_dir newdir/.git/refs
-	)
+	git init newdir &&
+	test_path_is_dir newdir/.git/refs
 '
 
 test_expect_success 'init creates a new bare directory' '
 	rm -fr newdir &&
-	(
-		git init --bare newdir &&
-		test_path_is_dir newdir/refs
-	)
+	git init --bare newdir &&
+	test_path_is_dir newdir/refs
 '
 
 test_expect_success 'init recreates a directory' '
 	rm -fr newdir &&
-	(
-		mkdir newdir &&
-		git init newdir &&
-		test_path_is_dir newdir/.git/refs
-	)
+	mkdir newdir &&
+	git init newdir &&
+	test_path_is_dir newdir/.git/refs
 '
 
 test_expect_success 'init recreates a new bare directory' '
 	rm -fr newdir &&
-	(
-		mkdir newdir &&
-		git init --bare newdir &&
-		test_path_is_dir newdir/refs
-	)
+	mkdir newdir &&
+	git init --bare newdir &&
+	test_path_is_dir newdir/refs
 '
 
 test_expect_success 'init creates a new deep directory' '
@@ -297,30 +286,24 @@ test_expect_success POSIXPERM 'init creates a new deep directory (umask vs. shar
 
 test_expect_success 'init notices EEXIST (1)' '
 	rm -fr newdir &&
-	(
-		>newdir &&
-		test_must_fail git init newdir &&
-		test_path_is_file newdir
-	)
+	>newdir &&
+	test_must_fail git init newdir &&
+	test_path_is_file newdir
 '
 
 test_expect_success 'init notices EEXIST (2)' '
 	rm -fr newdir &&
-	(
-		mkdir newdir &&
-		>newdir/a
-		test_must_fail git init newdir/a/b &&
-		test_path_is_file newdir/a
-	)
+	mkdir newdir &&
+	>newdir/a
+	test_must_fail git init newdir/a/b &&
+	test_path_is_file newdir/a
 '
 
 test_expect_success POSIXPERM,SANITY 'init notices EPERM' '
 	rm -fr newdir &&
-	(
-		mkdir newdir &&
-		chmod -w newdir &&
-		test_must_fail git init newdir/a/b
-	)
+	mkdir newdir &&
+	chmod -w newdir &&
+	test_must_fail git init newdir/a/b
 '
 
 test_expect_success 'init creates a new bare directory with global --bare' '
-- 
1.9.0.560.g01ceb46

[PATCH 12/12] t0001: drop subshells just for "cd"

[Jeff King]

Many tests do something like:

  (
	mkdir foo &&
	cd foo &&
	git init
  )

You can do the same these days with "git init foo", which
makes the tests shorter and simpler to read.

Signed-off-by: Jeff King <peff@peff.net>
---
Unlike the last patch, this one _could_ have an affect. I made the
assumption that "git init foo" would behave sanely, but that other
complex things should be left alone. E.g., ones that set GIT_DIR in the
environment to a relative path might be affected based on when git does
the "chdir".

 t/t0001-init.sh | 56 +++++++++-----------------------------------------------
 1 file changed, 9 insertions(+), 47 deletions(-)

diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 55a68bc..68549d1 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -24,11 +24,7 @@ check_config () {
 }
 
 test_expect_success 'plain' '
-	(
-		mkdir plain &&
-		cd plain &&
-		git init
-	) &&
+	git init plain &&
 	check_config plain/.git false unset
 '
 
@@ -90,11 +86,7 @@ test_expect_success 'plain with GIT_WORK_TREE' '
 '
 
 test_expect_success 'plain bare' '
-	(
-		mkdir plain-bare-1 &&
-		cd plain-bare-1 &&
-		git --bare init
-	) &&
+	git --bare init plain-bare-1 &&
 	check_config plain-bare-1 true unset
 '
 
@@ -112,12 +104,7 @@ test_expect_success 'GIT_DIR bare' '
 '
 
 test_expect_success 'init --bare' '
-
-	(
-		mkdir init-bare.git &&
-		cd init-bare.git &&
-		git init --bare
-	) &&
+	git init --bare init-bare.git &&
 	check_config init-bare.git true unset
 '
 
@@ -166,26 +153,14 @@ test_expect_success 'reinit' '
 test_expect_success 'init with --template' '
 	mkdir template-source &&
 	echo content >template-source/file &&
-	(
-		mkdir template-custom &&
-		cd template-custom &&
-		git init --template=../template-source
-	) &&
+	git init --template=../template-source template-custom &&
 	test_cmp template-source/file template-custom/.git/file
 '
 
 test_expect_success 'init with --template (blank)' '
-	(
-		mkdir template-plain &&
-		cd template-plain &&
-		git init
-	) &&
+	git init template-plain &&
 	test_path_is_file template-plain/.git/info/exclude &&
-	(
-		mkdir template-blank &&
-		cd template-blank &&
-		git init --template=
-	) &&
+	git init --template= template-blank &&
 	test_path_is_missing template-blank/.git/info/exclude
 '
 
@@ -207,11 +182,7 @@ test_expect_success 'init with init.templatedir set' '
 test_expect_success 'init --bare/--shared overrides system/global config' '
 	test_config_global core.bare false &&
 	test_config_global core.sharedRepository 0640 &&
-	(
-		mkdir init-bare-shared-override &&
-		cd init-bare-shared-override &&
-		git init --bare --shared=0666
-	) &&
+	git init --bare --shared=0666 init-bare-shared-override &&
 	check_config init-bare-shared-override true unset &&
 	test x0666 = \
 	x`git config -f init-bare-shared-override/config core.sharedRepository`
@@ -219,22 +190,13 @@ test_expect_success 'init --bare/--shared overrides system/global config' '
 
 test_expect_success 'init honors global core.sharedRepository' '
 	test_config_global core.sharedRepository 0666 &&
-	(
-		mkdir shared-honor-global &&
-		cd shared-honor-global &&
-		git init
-	) &&
+	git init shared-honor-global &&
 	test x0666 = \
 	x`git config -f shared-honor-global/.git/config core.sharedRepository`
 '
 
 test_expect_success 'init rejects insanely long --template' '
-	(
-		insane=$(printf "x%09999dx" 1) &&
-		mkdir test &&
-		cd test &&
-		test_must_fail git init --template=$insane
-	)
+	test_must_fail git init --template=$(printf "x%09999dx" 1) test
 '
 
 test_expect_success 'init creates a new directory' '
-- 
1.9.0.560.g01ceb46

Re: [PATCH 11/12] t0001: drop useless subshells

[Eric Sunshine]

On Thu, Mar 20, 2014 at 7:21 PM, Jeff King <peff@peff.net> wrote:
> Many tests use subshells, but don't actually change the
> shell environment. They were probably cargo-culted from
> earlier tests which did need subshells. Drop the useless
> ones.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> These ones should produce no behavior change at all; they're purely
> mechanical "(foo && bar)" to "foo && bar" (though of course I did them
> by hand, because you need to know that "foo" and "bar" do not affect the
> environment).
>
>  t/t0001-init.sh | 61 +++++++++++++++++++++------------------------------------
>  1 file changed, 22 insertions(+), 39 deletions(-)
>
> diff --git a/t/t0001-init.sh b/t/t0001-init.sh
> index 4560bba..55a68bc 100755
> --- a/t/t0001-init.sh
> +++ b/t/t0001-init.sh
> @@ -297,30 +286,24 @@ test_expect_success POSIXPERM 'init creates a new deep directory (umask vs. shar
>
>  test_expect_success 'init notices EEXIST (2)' '
>         rm -fr newdir &&
> -       (
> -               mkdir newdir &&
> -               >newdir/a
> -               test_must_fail git init newdir/a/b &&
> -               test_path_is_file newdir/a
> -       )
> +       mkdir newdir &&
> +       >newdir/a

Broken &&-chain (though, not introduced by this patch).

> +       test_must_fail git init newdir/a/b &&
> +       test_path_is_file newdir/a
>  '
>
>  test_expect_success POSIXPERM,SANITY 'init notices EPERM' '
>         rm -fr newdir &&
> -       (
> -               mkdir newdir &&
> -               chmod -w newdir &&
> -               test_must_fail git init newdir/a/b
> -       )
> +       mkdir newdir &&
> +       chmod -w newdir &&
> +       test_must_fail git init newdir/a/b
>  '
>
>  test_expect_success 'init creates a new bare directory with global --bare' '
> --
> 1.9.0.560.g01ceb46

Re: [PATCH 03/12] t: drop useless sane_unset GIT_* calls

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Several test scripts manually unset GIT_CONFIG and other
> GIT_* variables. These are generally taken care of for us by
> test-lib.sh already.
>
> Unsetting these is not only useless, but can be confusing to
> a reader, who may wonder why some tests in a script unset
> them and others do not (t0001 is particularly guilty of this
> inconsistency, probably because many of its tests predate
> the test-lib.sh environment-cleansing).

> Note that we cannot always get rid of such unsetting. For
> example, t9130 can drop the GIT_CONFIG unset, but not the
> GIT_DIR one, because lib-git-svn.sh sets the latter. And in
> t1000, we unset GIT_TEMPLATE_DIR, which is explicitly
> initialized by test-lib.sh.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I suppose one could make an argument that test-lib.sh may later change
> the set of variables it clears, and these unsets are documenting an
> explicit need of each test. I'd find that more compelling if it were
> actually applied consistently.

Hmph.  I am looking at "git show HEAD^:t/t0001-init.sh" after
applying this patch, and it does look consistently done with
GIT_CONFIG and GIT_DIR (I am not sure about GIT_WORK_TREE but from a
cursory read it is done consistently for tests on non-bare
repositories).

So I would actually agree with your alternative interpretation
"Unsetting these is useless, but it does serve documentation
purpose---without having to see what the state of the environment
when the subprocess is started, the reader can understand what is
being tested", rather than the one in the log message.

Having said that, I am perfectly OK with the change to t0001 in this
patch, if we added at the very beginning of the test sequence a
comment that says:

    Below, creation and use of repositories are tested with various
    combinations of environment settings and command line flags.
    They are done inside subshells to avoid leaking temporary
    environment settings to later tests *and* assumes that the
    initial environment does not have have GIT_DIR, GIT_CONFIG, and
    GIT_WORK_TREE defined.

or something.

>  t/t0001-init.sh                 | 15 ---------------
>  t/t9130-git-svn-authors-file.sh |  1 -
>  t/t9400-git-cvsserver-server.sh |  1 -
>  3 files changed, 17 deletions(-)
>
> diff --git a/t/t0001-init.sh b/t/t0001-init.sh
> index 9fb582b..ddc8160 100755
> --- a/t/t0001-init.sh
> +++ b/t/t0001-init.sh
> @@ -25,7 +25,6 @@ check_config () {
>  
>  test_expect_success 'plain' '
>  	(
> -		sane_unset GIT_DIR GIT_WORK_TREE &&
>  		mkdir plain &&
>  		cd plain &&
>  		git init
> @@ -35,7 +34,6 @@ test_expect_success 'plain' '
>  
>  test_expect_success 'plain nested in bare' '
>  	(
> -		sane_unset GIT_DIR GIT_WORK_TREE &&
>  		git init --bare bare-ancestor.git &&
>  		cd bare-ancestor.git &&
>  		mkdir plain-nested &&
> @@ -47,7 +45,6 @@ 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 &&
>  		HOME=$(pwd)/alias-config &&
>  		export HOME &&
>  		mkdir alias-config &&
> @@ -65,7 +62,6 @@ test_expect_success 'plain through aliased command, outside any git repo' '
>  
>  test_expect_failure 'plain nested through aliased command' '
>  	(
> -		sane_unset GIT_DIR GIT_WORK_TREE &&
>  		git init plain-ancestor-aliased &&
>  		cd plain-ancestor-aliased &&
>  		echo "[alias] aliasedinit = init" >>.git/config &&
> @@ -78,7 +74,6 @@ test_expect_failure 'plain nested through aliased command' '
>  
>  test_expect_failure 'plain nested in bare through aliased command' '
>  	(
> -		sane_unset GIT_DIR GIT_WORK_TREE &&
>  		git init --bare bare-ancestor-aliased.git &&
>  		cd bare-ancestor-aliased.git &&
>  		echo "[alias] aliasedinit = init" >>config &&
> @@ -91,7 +86,6 @@ test_expect_failure 'plain nested in bare through aliased command' '
>  
>  test_expect_success 'plain with GIT_WORK_TREE' '
>  	if (
> -		sane_unset GIT_DIR &&
>  		mkdir plain-wt &&
>  		cd plain-wt &&
>  		GIT_WORK_TREE=$(pwd) git init
> @@ -104,7 +98,6 @@ test_expect_success 'plain with GIT_WORK_TREE' '
>  
>  test_expect_success 'plain bare' '
>  	(
> -		sane_unset GIT_DIR GIT_WORK_TREE GIT_CONFIG &&
>  		mkdir plain-bare-1 &&
>  		cd plain-bare-1 &&
>  		git --bare init
> @@ -114,7 +107,6 @@ test_expect_success 'plain bare' '
>  
>  test_expect_success 'plain bare with GIT_WORK_TREE' '
>  	if (
> -		sane_unset GIT_DIR GIT_CONFIG &&
>  		mkdir plain-bare-2 &&
>  		cd plain-bare-2 &&
>  		GIT_WORK_TREE=$(pwd) git --bare init
> @@ -128,7 +120,6 @@ test_expect_success 'plain bare with GIT_WORK_TREE' '
>  test_expect_success 'GIT_DIR bare' '
>  
>  	(
> -		sane_unset GIT_CONFIG &&
>  		mkdir git-dir-bare.git &&
>  		GIT_DIR=git-dir-bare.git git init
>  	) &&
> @@ -138,7 +129,6 @@ test_expect_success 'GIT_DIR bare' '
>  test_expect_success 'init --bare' '
>  
>  	(
> -		sane_unset GIT_DIR GIT_WORK_TREE GIT_CONFIG &&
>  		mkdir init-bare.git &&
>  		cd init-bare.git &&
>  		git init --bare
> @@ -149,7 +139,6 @@ test_expect_success 'init --bare' '
>  test_expect_success 'GIT_DIR non-bare' '
>  
>  	(
> -		sane_unset GIT_CONFIG &&
>  		mkdir non-bare &&
>  		cd non-bare &&
>  		GIT_DIR=.git git init
> @@ -160,7 +149,6 @@ test_expect_success 'GIT_DIR non-bare' '
>  test_expect_success 'GIT_DIR & GIT_WORK_TREE (1)' '
>  
>  	(
> -		sane_unset GIT_CONFIG &&
>  		mkdir git-dir-wt-1.git &&
>  		GIT_WORK_TREE=$(pwd) GIT_DIR=git-dir-wt-1.git git init
>  	) &&
> @@ -170,7 +158,6 @@ test_expect_success 'GIT_DIR & GIT_WORK_TREE (1)' '
>  test_expect_success 'GIT_DIR & GIT_WORK_TREE (2)' '
>  
>  	if (
> -		sane_unset GIT_CONFIG &&
>  		mkdir git-dir-wt-2.git &&
>  		GIT_WORK_TREE=$(pwd) GIT_DIR=git-dir-wt-2.git git --bare init
>  	)
> @@ -183,8 +170,6 @@ test_expect_success 'GIT_DIR & GIT_WORK_TREE (2)' '
>  test_expect_success 'reinit' '
>  
>  	(
> -		sane_unset GIT_CONFIG GIT_WORK_TREE GIT_CONFIG &&
> -
>  		mkdir again &&
>  		cd again &&
>  		git init >out1 2>err1 &&
> diff --git a/t/t9130-git-svn-authors-file.sh b/t/t9130-git-svn-authors-file.sh
> index c3443ce..a812783 100755
> --- a/t/t9130-git-svn-authors-file.sh
> +++ b/t/t9130-git-svn-authors-file.sh
> @@ -97,7 +97,6 @@ test_expect_success 'fresh clone with svn.authors-file in config' '
>  		test x = x"$(git config svn.authorsfile)" &&
>  		test_config="$HOME"/.gitconfig &&
>  		sane_unset GIT_DIR &&
> -		sane_unset GIT_CONFIG &&
>  		git config --global \
>  		  svn.authorsfile "$HOME"/svn-authors &&
>  		test x"$HOME"/svn-authors = x"$(git config svn.authorsfile)" &&
> diff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh
> index 3edc408..ed98e64 100755
> --- a/t/t9400-git-cvsserver-server.sh
> +++ b/t/t9400-git-cvsserver-server.sh
> @@ -25,7 +25,6 @@ perl -e 'use DBI; use DBD::SQLite' >/dev/null 2>&1 || {
>      test_done
>  }
>  
> -unset GIT_DIR GIT_CONFIG
>  WORKDIR=$(pwd)
>  SERVERDIR=$(pwd)/gitcvs.git
>  git_config="$SERVERDIR/config"

Re: [PATCH 04/12] t: stop using GIT_CONFIG to cross repo boundaries

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Some tests want to check or set config in another
> repository. E.g., t1000 creates repositories and makes sure
> that their core.bare and core.worktree settings are what we
> expect. We can do this with:
>
>   GIT_CONFIG=$repo/.git/config git config ...
>
> but it better shows the intent to just enter the repository
> and let "git config" do the normal lookups:
>
>   (cd $repo && git config ...)
>
> In theory, this would cause us to use an extra subshell, but
> in all such cases, we are actually already in a subshell.

Sure; alternatively we could use "git -C $there", but this rewrite
is fine by me.

Thanks.

> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  t/t0001-init.sh        | 4 ++--
>  t/t5701-clone-local.sh | 6 +++---
>  2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/t/t0001-init.sh b/t/t0001-init.sh
> index ddc8160..9b05fdf 100755
> --- a/t/t0001-init.sh
> +++ b/t/t0001-init.sh
> @@ -12,8 +12,8 @@ check_config () {
>  		echo "expected a directory $1, a file $1/config and $1/refs"
>  		return 1
>  	fi
> -	bare=$(GIT_CONFIG="$1/config" git config --bool core.bare)
> -	worktree=$(GIT_CONFIG="$1/config" git config core.worktree) ||
> +	bare=$(cd "$1" && git config --bool core.bare)
> +	worktree=$(cd "$1" && git config core.worktree) ||
>  	worktree=unset
>  
>  	test "$bare" = "$2" && test "$worktree" = "$3" || {
> diff --git a/t/t5701-clone-local.sh b/t/t5701-clone-local.sh
> index c490368..3c087e9 100755
> --- a/t/t5701-clone-local.sh
> +++ b/t/t5701-clone-local.sh
> @@ -12,8 +12,8 @@ test_expect_success 'preparing origin repository' '
>  	: >file && git add . && git commit -m1 &&
>  	git clone --bare . a.git &&
>  	git clone --bare . x &&
> -	test "$(GIT_CONFIG=a.git/config git config --bool core.bare)" = true &&
> -	test "$(GIT_CONFIG=x/config git config --bool core.bare)" = true &&
> +	test "$(cd a.git && git config --bool core.bare)" = true &&
> +	test "$(cd x && git config --bool core.bare)" = true &&
>  	git bundle create b1.bundle --all &&
>  	git bundle create b2.bundle master &&
>  	mkdir dir &&
> @@ -24,7 +24,7 @@ test_expect_success 'preparing origin repository' '
>  test_expect_success 'local clone without .git suffix' '
>  	git clone -l -s a b &&
>  	(cd b &&
> -	test "$(GIT_CONFIG=.git/config git config --bool core.bare)" = false &&
> +	test "$(git config --bool core.bare)" = false &&
>  	git fetch)
>  '

Re: [PATCH 03/12] t: drop useless sane_unset GIT_* calls

[Jeff King]

On Fri, Mar 21, 2014 at 02:24:31PM -0700, Junio C Hamano wrote:

> > Unsetting these is not only useless, but can be confusing to
> > a reader, who may wonder why some tests in a script unset
> > them and others do not (t0001 is particularly guilty of this
> > inconsistency, probably because many of its tests predate
> > the test-lib.sh environment-cleansing).
> [...]
> > I suppose one could make an argument that test-lib.sh may later change
> > the set of variables it clears, and these unsets are documenting an
> > explicit need of each test. I'd find that more compelling if it were
> > actually applied consistently.
> 
> Hmph.  I am looking at "git show HEAD^:t/t0001-init.sh" after
> applying this patch, and it does look consistently done with
> GIT_CONFIG and GIT_DIR (I am not sure about GIT_WORK_TREE but from a
> cursory read it is done consistently for tests on non-bare
> repositories).

I don't understand why we stop bothering with the unsets starting with
"init with --template". Are those variables not important to the outcome
of that and later tests, or did the author simply not bother because
they are noops?

> So I would actually agree with your alternative interpretation
> "Unsetting these is useless, but it does serve documentation
> purpose---without having to see what the state of the environment
> when the subprocess is started, the reader can understand what is
> being tested", rather than the one in the log message.

I'd agree with that if I were convinced that the presence of them there
versus the absence of them later was meaningful.

> Having said that, I am perfectly OK with the change to t0001 in this
> patch, if we added at the very beginning of the test sequence a
> comment that says:
> 
>     Below, creation and use of repositories are tested with various
>     combinations of environment settings and command line flags.
>     They are done inside subshells to avoid leaking temporary
>     environment settings to later tests *and* assumes that the
>     initial environment does not have have GIT_DIR, GIT_CONFIG, and
>     GIT_WORK_TREE defined.
> 
> or something.

I do not have a problem with that, as it implicitly covers all of the
tests following it. I do not think it is particularly necessary, though.
Assuming we start with a known test environment and avoiding polluting
it for further tests are basic principles of _all_ test scripts.

-Peff

Re: [PATCH 04/12] t: stop using GIT_CONFIG to cross repo boundaries

[Jeff King]

On Fri, Mar 21, 2014 at 02:26:02PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Some tests want to check or set config in another
> > repository. E.g., t1000 creates repositories and makes sure
> > that their core.bare and core.worktree settings are what we
> > expect. We can do this with:
> >
> >   GIT_CONFIG=$repo/.git/config git config ...
> >
> > but it better shows the intent to just enter the repository
> > and let "git config" do the normal lookups:
> >
> >   (cd $repo && git config ...)
> >
> > In theory, this would cause us to use an extra subshell, but
> > in all such cases, we are actually already in a subshell.
> 
> Sure; alternatively we could use "git -C $there", but this rewrite
> is fine by me.

The existing callers all pass actual $GIT_DIRs, so I initially wrote it
as "git --git-dir=$repo config ...". Doing it as "-C" is perhaps nicer,
as callers could potentially pass a shorter string to the repo root,
and not bother with adding "/.git". However, t0001 needs the actual
$GIT_DIR (because it looks for things like the refs/ directory in the
same function), and the other callers are just passing bare repos.

So I'm fine with any of them. Feel free to mark it up if you have a
preference.

-Peff

Re: [PATCH 03/12] t: drop useless sane_unset GIT_* calls

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I do not have a problem with that, as it implicitly covers all of the
> tests following it. I do not think it is particularly necessary, though.
> Assuming we start with a known test environment and avoiding polluting
> it for further tests are basic principles of _all_ test scripts.

They should be, but I suspect majority of tests, especially the
older ones, do have dependencies on earlier test pieces X-<.

Re: [PATCH 03/12] t: drop useless sane_unset GIT_* calls

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> Hmph.  I am looking at "git show HEAD^:t/t0001-init.sh" after
>> applying this patch, and it does look consistently done with
>> GIT_CONFIG and GIT_DIR (I am not sure about GIT_WORK_TREE but from a
>> cursory read it is done consistently for tests on non-bare
>> repositories).
>
> I don't understand why we stop bothering with the unsets starting with
> "init with --template". Are those variables not important to the outcome
> of that and later tests, or did the author simply not bother because
> they are noops?

If I had to guess (without running "blame", so it may well turn out
that "the author" may turn out to be me ;-), it is simply the author
not being careful.