tests: set temp variables using 'env' in test function instead of subshell
PATCH v2 1/1 tests: set temp variables using 'env' in test function instead of subshell
In reply to a message not in the archiveOriginally, 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.
---
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
Quoted 1 line
Oh, really ;-)?
Missed that.
Quoted 1 line
Thanks. Getting closer, I think.
Slowly but surely.
--- 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(-)
t/t1300-repo-config.sh +4 -13
@@ -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' "
t/t1510-repo-setup.sh +1 -3
@@ -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 '
t/t3200-branch.sh +2 -10
@@ -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' '
t/t3301-notes.sh +6 -16
@@ -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 '
t/t3404-rebase-interactive.sh +13 -52
131 lines not on this page · raw message
t/t3413-rebase-hook.sh +1 -5
@@ -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) '
t/t4014-format-patch.sh +3 -11
@@ -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 '
t/t5305-include-tag.sh +1 -3
@@ -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
t/t5602-clone-remote-exec.sh +3 -10
@@ -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 '
t/t5801-remote-helpers.sh +2 -4
@@ -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 ) '
t/t6006-rev-list-format.sh +3 -6
@@ -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)' '
t/t7006-pager.sh +3 -15
@@ -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
Quoted 6 lines · 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.
Quoted 7 lines
Signed-off-by David Tran <[email protected]>---
Quoted 1 line
Oh, really ;-)?
Missed that.
Quoted 1 line
Thanks. Getting closer, I think.
Slowly but surely.
Getting better. See below.
Quoted 7 lines
Signed-off-by David Tran <[email protected]>---
t/t3404-rebase-interactive.sh +0
-0
t/t3404-rebase-interactive.sh test_expect_success 'rebase -i with the exec command checks tree cleanness' ' · line 108
Quoted 8 lines
t/t3404-rebase-interactive.sh +1
-3@@ -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.
t/t3404-rebase-interactive.sh test_expect_success 'rebase -i with exec of inexistent command' ' · line 121
Quoted 12 lines
t/t3404-rebase-interactive.sh +2
-3test_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.
t/t3404-rebase-interactive.sh test_expect_success 'aborted --continue does not squash commits after "edit"' ' · line 512
Quoted 12 lines
t/t3404-rebase-interactive.sh +1
-5! 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.
Quoted 3 lines
t/t3404-rebase-interactive.sh +0
-0test $old = $(git rev-parse HEAD) && git rebase --abort '
Quoted 6 lines · 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.
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.
Quoted 2 lines
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".
Quoted 15 lines
Signed-off-by David Tran <[email protected]>--- 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.
Quoted 450 lines
t/t1300-repo-config.sh +4
-13@@ -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' "t/t1510-repo-setup.sh +1
-3@@ -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 't/t3200-branch.sh +2
-10@@ -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' 't/t3301-notes.sh +6
-16@@ -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 't/t3404-rebase-interactive.sh +13
-52@@ -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 't/t3413-rebase-hook.sh +1
-5@@ -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) 't/t4014-format-patch.sh +3
-11@@ -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 't/t5305-include-tag.sh +1
-3@@ -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.actualt/t5602-clone-remote-exec.sh +3
-10@@ -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 't/t5801-remote-helpers.sh +2
-4@@ -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 ) 't/t6006-rev-list-format.sh +3
-6@@ -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)' 't/t7006-pager.sh +3
-16@@ -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
Quoted 7 lines · Junio C Hamano
Quoted 7 lines
t/t1300-repo-config.sh +0
-0@@ -974,24 +974,15 @@ test_expect_success SYMLINKS 'symlinked configuration' ' '
Quoted 10 lines
t/t1300-repo-config.sh test_expect_success SYMLINKS 'symlinked configuration' ' · line 979
Quoted 10 lines
t/t1300-repo-config.sh +2
-6test_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
Quoted 22 lines · Jeff King
Quoted 7 lines · Junio C Hamano
Quoted 7 lines
t/t1300-repo-config.sh +0
-0@@ -974,24 +974,15 @@ test_expect_success SYMLINKS 'symlinked configuration' ' 'Quoted 10 lines
t/t1300-repo-config.sh test_expect_success SYMLINKS 'symlinked configuration' ' · line 979
Quoted 10 lines
t/t1300-repo-config.sh +2
-6test_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_CONFIGhere another way of saying:
Quoted 4 lines
test_must_fail git config -f doesnotexist --listPerhaps 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.
Quoted 22 lines · Jeff King
Quoted 7 lines · Junio C Hamano
Quoted 7 lines
t/t1300-repo-config.sh +0
-0@@ -974,24 +974,15 @@ test_expect_success SYMLINKS 'symlinked configuration' ' 'Quoted 10 lines
t/t1300-repo-config.sh test_expect_success SYMLINKS 'symlinked configuration' ' · line 979
Quoted 10 lines
t/t1300-repo-config.sh +2
-6test_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_CONFIGhere another way of saying:
Quoted 4 lines
test_must_fail git config -f doesnotexist --listPerhaps 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?
Quoted 10 lines · Junio C Hamano
Quoted 6 lines
Isn't
GIT_CONFIGhere another way of saying:test_must_fail git config -f doesnotexist --listPerhaps 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
Quoted 17 lines · Jeff King
Quoted 10 lines · Junio C Hamano
Quoted 6 lines
Isn't
GIT_CONFIGhere another way of saying:test_must_fail git config -f doesnotexist --listPerhaps 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).
Quoted 5 lines
My recollection is that
GIT_CONFIGmostly 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 useGIT_CONFIGin "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.
cover GIT_CONFIG in the test suite
Quoted 5 lines · Junio C Hamano
[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 1/12 t/Makefile: stop setting GIT_CONFIG
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.
--- t/Makefile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
t/Makefile +2 -2
@@ -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)'
Signature · 1 line
1.9.0.560.g01ceb46
PATCH 2/12 t/test-lib: drop redundant unset of GIT_CONFIG
In reply to Jeff KingThis is already handled by the mass GIT_* unsetting added by 95a1d12 (tests: scrub environment of GIT_* variables, 2011-03-15).
--- t/test-lib.sh | 1 - 1 file changed, 1 deletion(-)
t/test-lib.sh +0 -1
@@ -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
Signature · 1 line
1.9.0.560.g01ceb46
PATCH 3/12 t: drop useless sane_unset GIT_* calls
In reply to Jeff KingSeveral 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.
---
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(-)
t/t0001-init.sh +0 -15
@@ -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 &&
t/t9130-git-svn-authors-file.sh +0 -1
@@ -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)" &&
t/t9400-git-cvsserver-server.sh +0 -1
@@ -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"
Signature · 1 line
1.9.0.560.g01ceb46
PATCH 4/12 t: stop using GIT_CONFIG to cross repo boundaries
In reply to Jeff KingSome 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.
--- t/t0001-init.sh | 4 ++-- t/t5701-clone-local.sh | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-)
t/t0001-init.sh +2 -2
@@ -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" || {
t/t5701-clone-local.sh +3 -3
@@ -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) '
Signature · 1 line
1.9.0.560.g01ceb46
PATCH 5/12 t: prefer "git config --file" to GIT_CONFIG with test_must_fail
In reply to Jeff KingThis lets us get rid of an extra "env" invocation in the middle, and is slightly more readable.
---
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(-)
t/t1300-repo-config.sh +4 -4
@@ -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' "
Signature · 1 line
1.9.0.560.g01ceb46
PATCH 6/12 t: prefer "git config --file" to GIT_CONFIG
In reply to Jeff KingDoing:
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.
---
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(-)
t/t1300-repo-config.sh +10 -10
@@ -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 '
t/t1302-repo-version.sh +1 -1
@@ -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' '
t/t7400-submodule-basic.sh +2 -3
@@ -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 '
t/t9130-git-svn-authors-file.sh +1 -1
@@ -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' '
t/t9154-git-svn-fancy-glob.sh +3 -3
@@ -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 &&
Signature · 1 line
1.9.0.560.g01ceb46
PATCH 7/12 t0001: make symlink reinit test more careful
In reply to Jeff KingIn 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.
--- t/t0001-init.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
t/t0001-init.sh +2 -2
@@ -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
Signature · 1 line
1.9.0.560.g01ceb46
PATCH 8/12 t0001: use test_path_is_*
In reply to Jeff Kingt0001 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.
--- t/t0001-init.sh | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-)
t/t0001-init.sh +18 -18
@@ -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
Signature · 1 line
1.9.0.560.g01ceb46
PATCH 9/12 t0001: use test_config_global
In reply to Jeff KingWe 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.
--- t/t0001-init.sh | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)
t/t0001-init.sh +4 -7
@@ -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
Signature · 1 line
1.9.0.560.g01ceb46
PATCH 10/12 t0001: use test_must_fail
In reply to Jeff KingWe'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.
---
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(-)
t/t0001-init.sh +11 -27
@@ -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' '
Signature · 1 line
1.9.0.560.g01ceb46
PATCH 11/12 t0001: drop useless subshells
In reply to Jeff KingMany 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.
---
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(-)
t/t0001-init.sh +22 -39
@@ -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' '
Signature · 1 line
1.9.0.560.g01ceb46
PATCH 12/12 t0001: drop subshells just for "cd"
In reply to Jeff KingMany 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.
---
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(-)
t/t0001-init.sh +9 -47
@@ -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' '
Signature · 1 line
1.9.0.560.g01ceb46
Re: [PATCH 03/12] t: drop useless sane_unset GIT_* calls
In reply to Jeff KingQuoted 16 lines · Jeff King
Several test scripts manually unset
GIT_CONFIGand other GIT_* variables. These are generally taken care of for us bytest-lib.shalready.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.shenvironment-cleansing).Note that we cannot always get rid of such unsetting. For example, t9130 can drop the
GIT_CONFIGunset, but not theGIT_DIRone, becauselib-git-svn.shsets the latter. And in t1000, we unsetGIT_TEMPLATE_DIR, which is explicitly initialized bytest-lib.sh.
Quoted 6 lines
Signed-off-by Jeff King <[email protected]>---
I suppose one could make an argument thattest-lib.shmay 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.
Quoted 146 lines
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(-)t/t0001-init.sh +0
-15@@ -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 &&t/t9130-git-svn-authors-file.sh +0
-1@@ -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)" &&t/t9400-git-cvsserver-server.sh +0
-1@@ -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
In reply to Jeff KingQuoted 7 lines · 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 ...
Quoted 7 lines
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.
Quoted 44 lines
Signed-off-by Jeff King <[email protected]>--- t/t0001-init.sh | 4 ++-- t/t5701-clone-local.sh | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-)t/t0001-init.sh +2
-2@@ -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" || {t/t5701-clone-local.sh +3
-3@@ -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 11/12] t0001: drop useless subshells
In reply to Jeff KingQuoted 21 lines · 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 <[email protected]>---
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(-)t/t0001-init.sh +0
-0@@ -297,30 +286,24 @@ test_expect_success POSIXPERM 'init creates a new deep directory (umask vs. shar
t/t0001-init.sh test_expect_success POSIXPERM 'init creates a new deep directory (umask vs. shar · line 290
Quoted 10 lines
t/t0001-init.sh +2
-6test_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).
Quoted 19 lines
t/t0001-init.sh +5
-6+ 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
In reply to Junio C HamanoQuoted 11 lines · Junio C Hamano
Quoted 5 lines
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.shenvironment-cleansing).[...]
Quoted 4 lines
I suppose one could make an argument that
test-lib.shmay 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.
Quoted 5 lines
Hmph. I am looking at "git show HEAD^:t/t0001-init.sh" after applying this patch, and it does look consistently done with
GIT_CONFIGandGIT_DIR(I am not sure aboutGIT_WORK_TREEbut 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?
Quoted 5 lines
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.
Quoted 4 lines
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:
Quoted 8 lines
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, andGIT_WORK_TREEdefined.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 03/12] t: drop useless sane_unset GIT_* calls
Quoted 10 lines · Jeff King
Quoted 5 lines
Hmph. I am looking at "git show HEAD^:t/t0001-init.sh" after applying this patch, and it does look consistently done with
GIT_CONFIGandGIT_DIR(I am not sure aboutGIT_WORK_TREEbut 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.
Re: [PATCH 03/12] t: drop useless sane_unset GIT_* calls
In reply to Jeff KingQuoted 4 lines · Jeff King
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 04/12] t: stop using GIT_CONFIG to cross repo boundaries
In reply to Junio C HamanoQuoted 12 lines · Junio C Hamano
Quoted 10 lines · 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:
Quoted 7 lines
Quoted 4 lines
(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