Re: [PATCH v2] tests: set temp variables using 'env' in test function instead of subshell
Jeff King p...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] tests: set temp variables using 'env' in test function instead of subshell
David Tran unsignedz...@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 unsignedz...@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 2result + test_must_fail env GIT_DIR=.git git symbolic-ref HEAD 2result ) 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
Re: [PATCH v2] tests: set temp variables using 'env' in test function instead of subshell
On Tue, Mar 18, 2014 at 8:08 AM, David Tran unsignedz...@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 unsignedz...@gmail.com --- Oh, really ;-)? Missed that. Thanks. Getting closer, I think. Slowly but surely. Getting better. See below. Signed-off-by: David Tran unsignedz...@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 21 + test_must_fail env FAKE_LINES=exec_this-command-does-not-exist 1 \ + git rebase -i HEAD^ actual 21 ) 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 ' -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] tests: set temp variables using 'env' in test function instead of subshell
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] tests: set temp variables using 'env' in test function instead of subshell
Jeff King p...@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? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] tests: set temp variables using 'env' in test function instead of subshell
On Tue, Mar 18, 2014 at 5:45 PM, Jeff King p...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] tests: set temp variables using 'env' in test function instead of subshell
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
