{"thread":{"id":"42270","subject":"[PATCH 4/6] t1500: avoid setting configuration options outside of tests","startedAt":"2016-05-10T05:20:49Z","lastAt":"2016-05-16T18:52:32Z","messageCount":24,"participants":["Eric Sunshine","Junio C Hamano","Jeff King","SZEDER Gábor"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"286010","messageId":"20160510052055.32924-1-sunshine@sunshineco.com","threadId":"42270","inReplyTo":null,"subject":"[PATCH 0/6] modernize t1500","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-05-10T05:20:49Z","receivedAt":"2016-05-10T05:20:49Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"This series modernizes t1500; it takes an entirely different approach\nthan [1][2] and is intended to replace that series. Whereas [1][2]\ndropped the systematic function-driven testing of git-rev-parse in favor\nof dozens of nearly identical copy/paste tests, this series retains the\nstructure of the existing script and instead updates the tests to set up\ntheir own needed state rather than relying upon transient and fragile\nmanipulation of global state.\n\nDue to its systematic function-driven approach, the original script is\nsmall at 87 lines, and easily understood. When [1] dropped the\nsystematic approach and replaced it with individual copy/paste tests,\nthe script ballooned to a whopping 573 lines; its v2 successor, while\nsmaller, still inflated it to 322 lines. Writing that amount of code\ncorrectly (even when primarily copy/paste) is difficult; reviewing it\nfor correctness is tedious, mind-numbing, and error-prone, as evidenced\nby [3].\n\nThis series, on the other hand, values the concision of the original and\nonly makes changes necessary to modernize it; the script size remains\nabout the same; it drops from 87 to 83 lines.\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/291087/focus=291088\n[2]: http://thread.gmane.org/gmane.comp.version-control.git/291729/focus=291731\n[3]: http://thread.gmane.org/gmane.comp.version-control.git/291729/focus=291745\n\nEric Sunshine (6):\n t1500: test_rev_parse: facilitate future test enhancements\n t1500: reduce dependence upon global state\n t1500: avoid changing working directory outside of tests\n t1500: avoid setting configuration options outside of tests\n t1500: avoid setting environment variables outside of tests\n t1500: be considerate to future potential tests\n\n t/t1500-rev-parse.sh | 116 +++++++++++++++++++++++++--------------------------\n 1 file changed, 56 insertions(+), 60 deletions(-)\n\n-- \n2.8.2.530.g51d527d\n"},{"id":"286014","messageId":"20160510052055.32924-2-sunshine@sunshineco.com","threadId":"42270","inReplyTo":"20160510052055.32924-1-sunshine@sunshineco.com","subject":"[PATCH 1/6] t1500: test_rev_parse: facilitate future test enhancements","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-05-10T05:20:50Z","receivedAt":"2016-05-10T05:20:50Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Tests run by test_rev_parse() are nearly identical; each invokes\ngit-rev-parse with a single option and compares the result against an\nexpected value. Such duplication makes it onerous to extend the tests\nsince any change needs to be repeated in each test. Reduce the\nduplication by parameterizing the test and driving it via a for-loop.\n\nSigned-off-by: Eric Sunshine \n---\n t/t1500-rev-parse.sh | 44 +++++++++++++++++---------------------------\n 1 file changed, 17 insertions(+), 27 deletions(-)\n\ndiff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh\nindex 48ee077..03f22fe 100755\n--- a/t/t1500-rev-parse.sh\n+++ b/t/t1500-rev-parse.sh\n@@ -3,38 +3,28 @@\n test_description='test git rev-parse'\n . ./test-lib.sh\n \n-test_rev_parse() {\n+# usage: label is-bare is-inside-git is-inside-work prefix git-dir\n+test_rev_parse () {\n \tname=$1\n \tshift\n \n-\ttest_expect_success \"$name: is-bare-repository\" \\\n-\t\"test '$1' = \\\"\\$(git rev-parse --is-bare-repository)\\\"\"\n-\tshift\n-\t[ $# -eq 0 ] && return\n-\n-\ttest_expect_success \"$name: is-inside-git-dir\" \\\n-\t\"test '$1' = \\\"\\$(git rev-parse --is-inside-git-dir)\\\"\"\n-\tshift\n-\t[ $# -eq 0 ] && return\n-\n-\ttest_expect_success \"$name: is-inside-work-tree\" \\\n-\t\"test '$1' = \\\"\\$(git rev-parse --is-inside-work-tree)\\\"\"\n-\tshift\n-\t[ $# -eq 0 ] && return\n-\n-\ttest_expect_success \"$name: prefix\" \\\n-\t\"test '$1' = \\\"\\$(git rev-parse --show-prefix)\\\"\"\n-\tshift\n-\t[ $# -eq 0 ] && return\n-\n-\ttest_expect_success \"$name: git-dir\" \\\n-\t\"test '$1' = \\\"\\$(git rev-parse --git-dir)\\\"\"\n-\tshift\n-\t[ $# -eq 0 ] && return\n+\tfor o in is-bare-repository \\\n+\t\t is-inside-git-dir \\\n+\t\t is-inside-work-tree \\\n+\t\t show-prefix \\\n+\t\t git-dir\n+\tdo\n+\t\texpect=\"$1\"\n+\t\ttest_expect_success \"$name: $o\" '\n+\t\t\techo \"$expect\" >expect &&\n+\t\t\tgit rev-parse --$o >actual &&\n+\t\t\ttest_cmp expect actual\n+\t\t'\n+\t\tshift\n+\t\ttest $# -eq 0 && break\n+\tdone\n }\n \n-# label is-bare is-inside-git is-inside-work prefix git-dir\n-\n ROOT=$(pwd)\n \n test_rev_parse toplevel false false true '' .git\n-- \n2.8.2.530.g51d527d\n"},{"id":"286015","messageId":"20160510052055.32924-3-sunshine@sunshineco.com","threadId":"42270","inReplyTo":"20160510052055.32924-1-sunshine@sunshineco.com","subject":"[PATCH 2/6] t1500: reduce dependence upon global state","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-05-10T05:20:51Z","receivedAt":"2016-05-10T05:20:51Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"This script pollutes its global state by changing its working directory\nand capturing that state via $(pwd) in the GIT_CONFIG environment\nvariable. This makes it difficult to modernize the script since tests\nideally should be self-contained and not rely upon such transient global\nstate.\n\nWith the ultimate goal of eliminating working directory changes, as a\nfirst step, avoid capturing global state by instead taking advantage of\n$ROOT which captured $(pwd) prior to any working directory changes, and\ncompose GIT_CONFIG from it and local knowledge of the working directory.\n\nSubsequent patches will eliminate changes of working directory and drop\nGIT_CONFIG altogether.\n\nSigned-off-by: Eric Sunshine \n---\n t/t1500-rev-parse.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh\nindex 03f22fe..3d79568 100755\n--- a/t/t1500-rev-parse.sh\n+++ b/t/t1500-rev-parse.sh\n@@ -49,7 +49,7 @@ test_rev_parse 'core.bare undefined' false false true\n mkdir work || exit 1\n cd work || exit 1\n GIT_DIR=../.git\n-GIT_CONFIG=\"$(pwd)\"/../.git/config\n+GIT_CONFIG=\"$ROOT/work/../.git/config\"\n export GIT_DIR GIT_CONFIG\n \n git config core.bare false\n@@ -63,7 +63,7 @@ test_rev_parse 'GIT_DIR=../.git, core.bare undefined' false false true ''\n \n mv ../.git ../repo.git || exit 1\n GIT_DIR=../repo.git\n-GIT_CONFIG=\"$(pwd)\"/../repo.git/config\n+GIT_CONFIG=\"$ROOT/work/../repo.git/config\"\n \n git config core.bare false\n test_rev_parse 'GIT_DIR=../repo.git, core.bare = false' false false true ''\n-- \n2.8.2.530.g51d527d\n"},{"id":"286012","messageId":"20160510052055.32924-4-sunshine@sunshineco.com","threadId":"42270","inReplyTo":"20160510052055.32924-1-sunshine@sunshineco.com","subject":"[PATCH 3/6] t1500: avoid changing working directory outside of tests","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-05-10T05:20:52Z","receivedAt":"2016-05-10T05:20:52Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Ideally, each test should be responsible for setting up state it needs\nrather than relying upon transient global state. Toward this end, teach\ntest_rev_parse() to accept a \"-C \" option to allow callers to\ninstruct it explicitly in which directory its tests should be run, and\ntake advantage of this new option to avoid changing the working\ndirectory outside of tests.\n\nSigned-off-by: Eric Sunshine \n---\n t/t1500-rev-parse.sh | 44 ++++++++++++++++++++++++--------------------\n 1 file changed, 24 insertions(+), 20 deletions(-)\n\ndiff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh\nindex 3d79568..f294ecc 100755\n--- a/t/t1500-rev-parse.sh\n+++ b/t/t1500-rev-parse.sh\n@@ -3,8 +3,18 @@\n test_description='test git rev-parse'\n . ./test-lib.sh\n \n-# usage: label is-bare is-inside-git is-inside-work prefix git-dir\n+# usage: [options] label is-bare is-inside-git is-inside-work prefix git-dir\n test_rev_parse () {\n+\tdir=\n+\twhile :\n+\tdo\n+\t\tcase \"$1\" in\n+\t\t-C) dir=\"-C $2\"; shift; shift ;;\n+\t\t-*) error \"test_rev_parse: unrecognized option '$1'\" ;;\n+\t\t*) break ;;\n+\t\tesac\n+\tdone\n+\n \tname=$1\n \tshift\n \n@@ -17,7 +27,7 @@ test_rev_parse () {\n \t\texpect=\"$1\"\n \t\ttest_expect_success \"$name: $o\" '\n \t\t\techo \"$expect\" >expect &&\n-\t\t\tgit rev-parse --$o >actual &&\n+\t\t\tgit $dir rev-parse --$o >actual &&\n \t\t\ttest_cmp expect actual\n \t\t'\n \t\tshift\n@@ -29,16 +39,11 @@ ROOT=$(pwd)\n \n test_rev_parse toplevel false false true '' .git\n \n-cd .git || exit 1\n-test_rev_parse .git/ false true false '' .\n-cd objects || exit 1\n-test_rev_parse .git/objects/ false true false '' \"$ROOT/.git\"\n-cd ../.. || exit 1\n+test_rev_parse -C .git .git/ false true false '' .\n+test_rev_parse -C .git/objects .git/objects/ false true false '' \"$ROOT/.git\"\n \n-mkdir -p sub/dir || exit 1\n-cd sub/dir || exit 1\n-test_rev_parse subdirectory false false true sub/dir/ \"$ROOT/.git\"\n-cd ../.. || exit 1\n+test_expect_success 'setup untracked sub/dir' 'mkdir -p sub/dir'\n+test_rev_parse -C sub/dir subdirectory false false true sub/dir/ \"$ROOT/.git\"\n \n git config core.bare true\n test_rev_parse 'core.bare = true' true false false\n@@ -46,32 +51,31 @@ test_rev_parse 'core.bare = true' true false false\n git config --unset core.bare\n test_rev_parse 'core.bare undefined' false false true\n \n-mkdir work || exit 1\n-cd work || exit 1\n+test_expect_success 'setup non-local database ../.git' 'mkdir work'\n GIT_DIR=../.git\n GIT_CONFIG=\"$ROOT/work/../.git/config\"\n export GIT_DIR GIT_CONFIG\n \n git config core.bare false\n-test_rev_parse 'GIT_DIR=../.git, core.bare = false' false false true ''\n+test_rev_parse -C work 'GIT_DIR=../.git, core.bare = false' false false true ''\n \n git config core.bare true\n-test_rev_parse 'GIT_DIR=../.git, core.bare = true' true false false ''\n+test_rev_parse -C work 'GIT_DIR=../.git, core.bare = true' true false false ''\n \n git config --unset core.bare\n-test_rev_parse 'GIT_DIR=../.git, core.bare undefined' false false true ''\n+test_rev_parse -C work 'GIT_DIR=../.git, core.bare undefined' false false true ''\n \n-mv ../.git ../repo.git || exit 1\n+test_expect_success 'setup non-local database ../repo.git' 'mv .git repo.git'\n GIT_DIR=../repo.git\n GIT_CONFIG=\"$ROOT/work/../repo.git/config\"\n \n git config core.bare false\n-test_rev_parse 'GIT_DIR=../repo.git, core.bare = false' false false true ''\n+test_rev_parse -C work 'GIT_DIR=../repo.git, core.bare = false' false false true ''\n \n git config core.bare true\n-test_rev_parse 'GIT_DIR=../repo.git, core.bare = true' true false false ''\n+test_rev_parse -C work 'GIT_DIR=../repo.git, core.bare = true' true false false ''\n \n git config --unset core.bare\n-test_rev_parse 'GIT_DIR=../repo.git, core.bare undefined' false false true ''\n+test_rev_parse -C work 'GIT_DIR=../repo.git, core.bare undefined' false false true ''\n \n test_done\n-- \n2.8.2.530.g51d527d\n"},{"id":"286009","messageId":"20160510052055.32924-5-sunshine@sunshineco.com","threadId":"42270","inReplyTo":"20160510052055.32924-1-sunshine@sunshineco.com","subject":"[PATCH 4/6] t1500: avoid setting configuration options outside of tests","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-05-10T05:20:53Z","receivedAt":"2016-05-10T05:20:53Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Ideally, each test should be responsible for setting up state it needs\nrather than relying upon transient global state. Toward this end, teach\ntest_rev_parse() to accept a \"-b \" option to allow callers to set\n\"core.bare\" explicitly or undefine it, and take advantage of this new\noption to avoid setting \"core.bare\" outside of tests.\n\nUnder the hood, \"-b \" invokes \"test_config -C \" (or\n\"test_unconfig -C \"), which means that git-config knows explicitly\nwhere to find its configuration file. Consequently, the global\nGIT_CONFIG environment variable, which was needed by the manual\ngit-config invocations outside of tests, is no longer needed, and is\nthus dropped.\n\nSigned-off-by: Eric Sunshine \n---\n t/t1500-rev-parse.sh | 39 ++++++++++++++++++++-------------------\n 1 file changed, 20 insertions(+), 19 deletions(-)\n\ndiff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh\nindex f294ecc..c058aa4 100755\n--- a/t/t1500-rev-parse.sh\n+++ b/t/t1500-rev-parse.sh\n@@ -6,15 +6,25 @@ test_description='test git rev-parse'\n # usage: [options] label is-bare is-inside-git is-inside-work prefix git-dir\n test_rev_parse () {\n \tdir=\n+\tbare=\n \twhile :\n \tdo\n \t\tcase \"$1\" in\n \t\t-C) dir=\"-C $2\"; shift; shift ;;\n+\t\t-b) bare=\"$2\"; shift; shift ;;\n \t\t-*) error \"test_rev_parse: unrecognized option '$1'\" ;;\n \t\t*) break ;;\n \t\tesac\n \tdone\n \n+\tcase \"$bare\" in\n+\t'') ;;\n+\tt*) bare=\"test_config $dir core.bare true\" ;;\n+\tf*) bare=\"test_config $dir core.bare false\" ;;\n+\tu*) bare=\"test_unconfig $dir core.bare\" ;;\n+\t*) error \"test_rev_parse: unrecognized core.bare value '$bare'\"\n+\tesac\n+\n \tname=$1\n \tshift\n \n@@ -26,6 +36,7 @@ test_rev_parse () {\n \tdo\n \t\texpect=\"$1\"\n \t\ttest_expect_success \"$name: $o\" '\n+\t\t\t$bare &&\n \t\t\techo \"$expect\" >expect &&\n \t\t\tgit $dir rev-parse --$o >actual &&\n \t\t\ttest_cmp expect actual\n@@ -45,37 +56,27 @@ test_rev_parse -C .git/objects .git/objects/ false true false '' \"$ROOT/.git\"\n test_expect_success 'setup untracked sub/dir' 'mkdir -p sub/dir'\n test_rev_parse -C sub/dir subdirectory false false true sub/dir/ \"$ROOT/.git\"\n \n-git config core.bare true\n-test_rev_parse 'core.bare = true' true false false\n+test_rev_parse -b t 'core.bare = true' true false false\n \n-git config --unset core.bare\n-test_rev_parse 'core.bare undefined' false false true\n+test_rev_parse -b u 'core.bare undefined' false false true\n \n test_expect_success 'setup non-local database ../.git' 'mkdir work'\n GIT_DIR=../.git\n-GIT_CONFIG=\"$ROOT/work/../.git/config\"\n-export GIT_DIR GIT_CONFIG\n+export GIT_DIR\n \n-git config core.bare false\n-test_rev_parse -C work 'GIT_DIR=../.git, core.bare = false' false false true ''\n+test_rev_parse -C work -b f 'GIT_DIR=../.git, core.bare = false' false false true ''\n \n-git config core.bare true\n-test_rev_parse -C work 'GIT_DIR=../.git, core.bare = true' true false false ''\n+test_rev_parse -C work -b t 'GIT_DIR=../.git, core.bare = true' true false false ''\n \n-git config --unset core.bare\n-test_rev_parse -C work 'GIT_DIR=../.git, core.bare undefined' false false true ''\n+test_rev_parse -C work -b u 'GIT_DIR=../.git, core.bare undefined' false false true ''\n \n test_expect_success 'setup non-local database ../repo.git' 'mv .git repo.git'\n GIT_DIR=../repo.git\n-GIT_CONFIG=\"$ROOT/work/../repo.git/config\"\n \n-git config core.bare false\n-test_rev_parse -C work 'GIT_DIR=../repo.git, core.bare = false' false false true ''\n+test_rev_parse -C work -b f 'GIT_DIR=../repo.git, core.bare = false' false false true ''\n \n-git config core.bare true\n-test_rev_parse -C work 'GIT_DIR=../repo.git, core.bare = true' true false false ''\n+test_rev_parse -C work -b t 'GIT_DIR=../repo.git, core.bare = true' true false false ''\n \n-git config --unset core.bare\n-test_rev_parse -C work 'GIT_DIR=../repo.git, core.bare undefined' false false true ''\n+test_rev_parse -C work -b u 'GIT_DIR=../repo.git, core.bare undefined' false false true ''\n \n test_done\n-- \n2.8.2.530.g51d527d\n"},{"id":"286011","messageId":"20160510052055.32924-6-sunshine@sunshineco.com","threadId":"42270","inReplyTo":"20160510052055.32924-1-sunshine@sunshineco.com","subject":"[PATCH 5/6] t1500: avoid setting environment variables outside of tests","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-05-10T05:20:54Z","receivedAt":"2016-05-10T05:20:54Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Ideally, each test should be responsible for setting up state it needs\nrather than relying upon transient global state. Toward this end, teach\ntest_rev_parse() to accept a \"-g \" option to allow callers to\nspecify the value of the GIT_DIR environment variable explicitly, and\ntake advantage of this new option to avoid polluting the global scope\nwith GIT_DIR assignments.\n\nRegarding the implementation: Typically, tests avoid polluting the\nglobal state by wrapping transient environment variable assignments\nwithin a subshell, however, this technique can not be used here since\ntest_config() and test_unconfig() need to know GIT_DIR, as well, but\nneither function can be used within a subshell. Consequently, GIT_DIR is\ncleared manually via sane_unset().\n\nSigned-off-by: Eric Sunshine \n---\n t/t1500-rev-parse.sh | 19 ++++++++++---------\n 1 file changed, 10 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh\nindex c058aa4..525e6d3 100755\n--- a/t/t1500-rev-parse.sh\n+++ b/t/t1500-rev-parse.sh\n@@ -7,11 +7,13 @@ test_description='test git rev-parse'\n test_rev_parse () {\n \tdir=\n \tbare=\n+\tenv=\n \twhile :\n \tdo\n \t\tcase \"$1\" in\n \t\t-C) dir=\"-C $2\"; shift; shift ;;\n \t\t-b) bare=\"$2\"; shift; shift ;;\n+\t\t-g) env=\"GIT_DIR=$2; export GIT_DIR\"; shift; shift ;;\n \t\t-*) error \"test_rev_parse: unrecognized option '$1'\" ;;\n \t\t*) break ;;\n \t\tesac\n@@ -36,6 +38,8 @@ test_rev_parse () {\n \tdo\n \t\texpect=\"$1\"\n \t\ttest_expect_success \"$name: $o\" '\n+\t\t\ttest_when_finished \"sane_unset GIT_DIR\" &&\n+\t\t\teval $env &&\n \t\t\t$bare &&\n \t\t\techo \"$expect\" >expect &&\n \t\t\tgit $dir rev-parse --$o >actual &&\n@@ -61,22 +65,19 @@ test_rev_parse -b t 'core.bare = true' true false false\n test_rev_parse -b u 'core.bare undefined' false false true\n \n test_expect_success 'setup non-local database ../.git' 'mkdir work'\n-GIT_DIR=../.git\n-export GIT_DIR\n \n-test_rev_parse -C work -b f 'GIT_DIR=../.git, core.bare = false' false false true ''\n+test_rev_parse -C work -g ../.git -b f 'GIT_DIR=../.git, core.bare = false' false false true ''\n \n-test_rev_parse -C work -b t 'GIT_DIR=../.git, core.bare = true' true false false ''\n+test_rev_parse -C work -g ../.git -b t 'GIT_DIR=../.git, core.bare = true' true false false ''\n \n-test_rev_parse -C work -b u 'GIT_DIR=../.git, core.bare undefined' false false true ''\n+test_rev_parse -C work -g ../.git -b u 'GIT_DIR=../.git, core.bare undefined' false false true ''\n \n test_expect_success 'setup non-local database ../repo.git' 'mv .git repo.git'\n-GIT_DIR=../repo.git\n \n-test_rev_parse -C work -b f 'GIT_DIR=../repo.git, core.bare = false' false false true ''\n+test_rev_parse -C work -g ../repo.git -b f 'GIT_DIR=../repo.git, core.bare = false' false false true ''\n \n-test_rev_parse -C work -b t 'GIT_DIR=../repo.git, core.bare = true' true false false ''\n+test_rev_parse -C work -g ../repo.git -b t 'GIT_DIR=../repo.git, core.bare = true' true false false ''\n \n-test_rev_parse -C work -b u 'GIT_DIR=../repo.git, core.bare undefined' false false true ''\n+test_rev_parse -C work -g ../repo.git -b u 'GIT_DIR=../repo.git, core.bare undefined' false false true ''\n \n test_done\n-- \n2.8.2.530.g51d527d\n"},{"id":"286013","messageId":"20160510052055.32924-7-sunshine@sunshineco.com","threadId":"42270","inReplyTo":"20160510052055.32924-1-sunshine@sunshineco.com","subject":"[PATCH 6/6] t1500: be considerate to future potential tests","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-05-10T05:20:55Z","receivedAt":"2016-05-10T05:20:55Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"The final batch of git-rev-parse tests work against a non-local object\ndatabase named ../repo.git rather than the typically-named ../.git. It\nprepares by renaming .git/ to repo.git/ and pointing GIT_DIR at\n../repo.git, but never restores the name to .git/, which can be\nproblematic for tests added in the future. Be more friendly by instead\nmaking repo.git/ a copy of .git/.\n\nSigned-off-by: Eric Sunshine \n---\n t/t1500-rev-parse.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh\nindex 525e6d3..af223ed 100755\n--- a/t/t1500-rev-parse.sh\n+++ b/t/t1500-rev-parse.sh\n@@ -72,7 +72,7 @@ test_rev_parse -C work -g ../.git -b t 'GIT_DIR=../.git, core.bare = true' true\n \n test_rev_parse -C work -g ../.git -b u 'GIT_DIR=../.git, core.bare undefined' false false true ''\n \n-test_expect_success 'setup non-local database ../repo.git' 'mv .git repo.git'\n+test_expect_success 'setup non-local database ../repo.git' 'cp -R .git repo.git'\n \n test_rev_parse -C work -g ../repo.git -b f 'GIT_DIR=../repo.git, core.bare = false' false false true ''\n \n-- \n2.8.2.530.g51d527d\n"},{"id":"286020","messageId":"xmqqfutqsaxn.fsf@gitster.mtv.corp.google.com","threadId":"42270","inReplyTo":"20160510052055.32924-1-sunshine@sunshineco.com","subject":"Re: [PATCH 0/6] modernize t1500","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-10T06:07:32Z","receivedAt":"2016-05-10T06:07:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> This series modernizes t1500; it takes an entirely different approach\n> than [1][2] and is intended to replace that series.\n\nTurns out that it wasn't so painful after all.\n\nThe only small niggle I have is on 6/6; my preference would be,\nbecause once we set up .git we do not add objects and refs to it, to\nmake a copy a lot earlier before the tests start.\n\nI'll let it simmer on the list overnight and take a fresh look in\nthe morning, but I think I like these better than what I had to\ntweak yesterday.\n"},{"id":"286022","messageId":"CAPig+cTXOcUfO=EE5xhOjEgzSNtmhrD84PDMvVHX9hEmwuV==g@mail.gmail.com","threadId":"42270","inReplyTo":"20160510052055.32924-5-sunshine@sunshineco.com","subject":"Re: [PATCH 4/6] t1500: avoid setting configuration options outside of tests","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-05-10T06:34:30Z","receivedAt":"2016-05-10T06:34:30Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, May 10, 2016 at 1:20 AM, Eric Sunshine wrote:\n> Ideally, each test should be responsible for setting up state it needs\n> rather than relying upon transient global state. Toward this end, teach\n> test_rev_parse() to accept a \"-b \" option to allow callers to set\n> \"core.bare\" explicitly or undefine it, and take advantage of this new\n> option to avoid setting \"core.bare\" outside of tests.\n> [...snip...]\n>\n> Signed-off-by: Eric Sunshine \n> ---\n> diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh\n> @@ -6,15 +6,25 @@ test_description='test git rev-parse'\n> + case \"$bare\" in\n> + '') ;;\n> + t*) bare=\"test_config $dir core.bare true\" ;;\n> + f*) bare=\"test_config $dir core.bare false\" ;;\n> + u*) bare=\"test_unconfig $dir core.bare\" ;;\n> + *) error \"test_rev_parse: unrecognized core.bare value '$bare'\"\n\nOops, this line lost its ;; at some point while refining the code.\n\n> + esac\n> +\n"},{"id":"286089","messageId":"xmqqr3d9pz9k.fsf@gitster.mtv.corp.google.com","threadId":"42270","inReplyTo":"CAPig+cTXOcUfO=EE5xhOjEgzSNtmhrD84PDMvVHX9hEmwuV==g@mail.gmail.com","subject":"Re: [PATCH 4/6] t1500: avoid setting configuration options outside of tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-10T18:02:31Z","receivedAt":"2016-05-10T18:02:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n>> diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh\n>> @@ -6,15 +6,25 @@ test_description='test git rev-parse'\n>> + case \"$bare\" in\n>> ...\n>> + u*) bare=\"test_unconfig $dir core.bare\" ;;\n>> + *) error \"test_rev_parse: unrecognized core.bare value '$bare'\"\n>\n> Oops, this line lost its ;; at some point while refining the code.\n>\n>> + esac\n>> +\n\nStrictly speaking, you do not have to have one. I'll squeeze it in\nanyways.\n"},{"id":"286090","messageId":"xmqqmvnxpyw6.fsf@gitster.mtv.corp.google.com","threadId":"42270","inReplyTo":"xmqqfutqsaxn.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/6] modernize t1500","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-10T18:10:33Z","receivedAt":"2016-05-10T18:10:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Eric Sunshine writes:\n>\n>> This series modernizes t1500; it takes an entirely different approach\n>> than [1][2] and is intended to replace that series.\n>\n> Turns out that it wasn't so painful after all.\n>\n> The only small niggle I have is on 6/6; my preference would be,\n> because once we set up .git we do not add objects and refs to it, to\n> make a copy a lot earlier before the tests start.\n\n-- >8 --\nSubject: [PATCH 7/6] t1500: finish preparation upfront\n\nThe earlier tests do not attempt to modify the contents of .git (by\ncreating objects or refs, for example), which means that even if\nsome earlier tests before \"cp -R\" fail, the tests in repo.git will\nrun in an environment that we can expect them to succeed in.\n\nMake it clear that tests in repo.git will be independent from the\nresults of earlier tests done in .git by moving its initialization\n\"cp -R .git repo.git\" much higher in the test sequence.\n\nSigned-off-by: Junio C Hamano \n---\n t/t1500-rev-parse.sh | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh\nindex 811c70f2..cb08677 100755\n--- a/t/t1500-rev-parse.sh\n+++ b/t/t1500-rev-parse.sh\n@@ -51,6 +51,7 @@ test_rev_parse () {\n }\n \n ROOT=$(pwd)\n+test_expect_success 'setup non-local database ../repo.git' 'cp -R .git repo.git'\n \n test_rev_parse toplevel false false true '' .git\n \n@@ -72,8 +73,6 @@ test_rev_parse -C work -g ../.git -b t 'GIT_DIR=../.git, core.bare = true' true\n \n test_rev_parse -C work -g ../.git -b u 'GIT_DIR=../.git, core.bare undefined' false false true ''\n \n-test_expect_success 'setup non-local database ../repo.git' 'cp -R .git repo.git'\n-\n test_rev_parse -C work -g ../repo.git -b f 'GIT_DIR=../repo.git, core.bare = false' false false true ''\n \n test_rev_parse -C work -g ../repo.git -b t 'GIT_DIR=../repo.git, core.bare = true' true false false ''\n-- \n2.8.2-623-gacdd3f1\n"},{"id":"286092","messageId":"xmqqeg99py5j.fsf@gitster.mtv.corp.google.com","threadId":"42270","inReplyTo":"xmqqmvnxpyw6.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/6] modernize t1500","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-10T18:26:32Z","receivedAt":"2016-05-10T18:26:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Junio C Hamano writes:\n>\n>> Eric Sunshine writes:\n>>\n>>> This series modernizes t1500; it takes an entirely different approach\n>>> than [1][2] and is intended to replace that series.\n>>\n>> Turns out that it wasn't so painful after all.\n>>\n>> The only small niggle I have is on 6/6; my preference would be,\n>> because once we set up .git we do not add objects and refs to it, to\n>> make a copy a lot earlier before the tests start.\n>\n> -- >8 --\n> Subject: [PATCH 7/6] t1500: finish preparation upfront\n>\n> The earlier tests do not attempt to modify the contents of .git (by\n> creating objects or refs, for example), which means that even if\n> some earlier tests before \"cp -R\" fail, the tests in repo.git will\n> run in an environment that we can expect them to succeed in.\n>\n> Make it clear that tests in repo.git will be independent from the\n> results of earlier tests done in .git by moving its initialization\n> \"cp -R .git repo.git\" much higher in the test sequence.\n>\n> Signed-off-by: Junio C Hamano \n> ---\n\nI think the same logic applies to other preparatory things like\ncreation of sub/dir in the working tree etc.\n"},{"id":"286094","messageId":"20160510183955.GA16211@sigill.intra.peff.net","threadId":"42270","inReplyTo":"20160510052055.32924-6-sunshine@sunshineco.com","subject":"Re: [PATCH 5/6] t1500: avoid setting environment variables outside of tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-10T18:39:55Z","receivedAt":"2016-05-10T18:39:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 10, 2016 at 01:20:54AM -0400, Eric Sunshine wrote:\n\n> diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh\n> index c058aa4..525e6d3 100755\n> --- a/t/t1500-rev-parse.sh\n> +++ b/t/t1500-rev-parse.sh\n> @@ -7,11 +7,13 @@ test_description='test git rev-parse'\n> test_rev_parse () {\n> \tdir=\n> \tbare=\n> +\tenv=\n> \twhile :\n> \tdo\n> \t\tcase \"$1\" in\n> \t\t-C) dir=\"-C $2\"; shift; shift ;;\n> \t\t-b) bare=\"$2\"; shift; shift ;;\n> +\t\t-g) env=\"GIT_DIR=$2; export GIT_DIR\"; shift; shift ;;\n\nThis will expand $2 inside $env, which is later eval'd. So funny things\nhappen if there are spaces or metacharacters. It looks like you only use\nit with short relative paths (\"../repo.git\", etc), which is OK, but this\nwould probably break badly if we ever used absolute paths.\n\nI don't know if it's worth worrying about or not. The usual solution is\nsomething like:\n\n env_git_dir=$2\n env='GIT_DIR=$env_git_dir; export GIT_DIR'\n ...\n eval \"$env\"\n\n> @@ -36,6 +38,8 @@ test_rev_parse () {\n> \tdo\n> \t\texpect=\"$1\"\n> \t\ttest_expect_success \"$name: $o\" '\n> +\t\t\ttest_when_finished \"sane_unset GIT_DIR\" &&\n> +\t\t\teval $env &&\n\nI was surprised not to see quoting around $env here, but it probably\ndoesn't matter (I think it may affect how some whitespace is treated,\nbut the contents of $env are pretty tame).\n\nThis will set up the sane_unset regardless of whether $env does\nanything. Would it make more sense to stick the test_when_finished\ninside $env? You could use regular unset then, too, since you know the\nvariable would be set.\n\n-Peff\n"},{"id":"286097","messageId":"CAPig+cRR49iJOkaLfynkvch4zUHVkpcJwVah0rvaEOeR7aY2Nw@mail.gmail.com","threadId":"42270","inReplyTo":"20160510183955.GA16211@sigill.intra.peff.net","subject":"Re: [PATCH 5/6] t1500: avoid setting environment variables outside of tests","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-05-10T19:12:23Z","receivedAt":"2016-05-10T19:12:23Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, May 10, 2016 at 2:39 PM, Jeff King wrote:\n> On Tue, May 10, 2016 at 01:20:54AM -0400, Eric Sunshine wrote:\n>> diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh\n>> @@ -7,11 +7,13 @@ test_description='test git rev-parse'\n>> while :\n>> do\n>> case \"$1\" in\n>> -C) dir=\"-C $2\"; shift; shift ;;\n>> -b) bare=\"$2\"; shift; shift ;;\n>> + -g) env=\"GIT_DIR=$2; export GIT_DIR\"; shift; shift ;;\n>\n> This will expand $2 inside $env, which is later eval'd. So funny things\n> happen if there are spaces or metacharacters. It looks like you only use\n> it with short relative paths (\"../repo.git\", etc), which is OK, but this\n> would probably break badly if we ever used absolute paths.\n>\n> I don't know if it's worth worrying about or not. The usual solution is\n> something like:\n>\n> env_git_dir=$2\n> env='GIT_DIR=$env_git_dir; export GIT_DIR'\n> ...\n> eval \"$env\"\n\nMakes sense; I wasn't quite happy with having $2 interpolated\nunquoted. Like you, though, I don't know if it's worth worrying\nabout...\n\n>> @@ -36,6 +38,8 @@ test_rev_parse () {\n>> do\n>> expect=\"$1\"\n>> test_expect_success \"$name: $o\" '\n>> + test_when_finished \"sane_unset GIT_DIR\" &&\n>> + eval $env &&\n>\n> I was surprised not to see quoting around $env here, but it probably\n> doesn't matter (I think it may affect how some whitespace is treated,\n> but the contents of $env are pretty tame).\n\nI flip-flopped on this one several times, quoting, and not quoting.\nDocumentation for 'eval' says:\n\n The args are read and concatenated together into a single\n command.\n\nso, I ultimately left it unquoted, but don't feel strongly about it.\n\n> This will set up the sane_unset regardless of whether $env does\n> anything. Would it make more sense to stick the test_when_finished\n> inside $env? You could use regular unset then, too, since you know the\n> variable would be set.\n\nI didn't worry about it too much because the end result is effectively\nthe same and, with all the 'case' arms being short one-liners, I think\nthe code is a bit easier to read as-is; bundling 'test_when_finished'\ninto the 'env' assignment line would probably require wrapping the\nline. I do like the improved encapsulation of your suggestion but\ndon't otherwise feel strongly about it.\n\nNevertheless, I can re-roll with these changes if you feel more\nstrongly than I about it.\n"},{"id":"286099","messageId":"xmqqwpn1oh45.fsf@gitster.mtv.corp.google.com","threadId":"42270","inReplyTo":"CAPig+cRR49iJOkaLfynkvch4zUHVkpcJwVah0rvaEOeR7aY2Nw@mail.gmail.com","subject":"Re: [PATCH 5/6] t1500: avoid setting environment variables outside of tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-10T19:19:54Z","receivedAt":"2016-05-10T19:19:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n>> I don't know if it's worth worrying about or not. The usual solution is\n>> something like:\n>>\n>> env_git_dir=$2\n>> env='GIT_DIR=$env_git_dir; export GIT_DIR'\n>> ...\n>> eval \"$env\"\n>\n> Makes sense; I wasn't quite happy with having $2 interpolated\n> unquoted. Like you, though, I don't know if it's worth worrying\n> about...\n\nThis is a good change, including the quoting of $env.\n\n> I flip-flopped on this one several times, quoting, and not quoting.\n> Documentation for 'eval' says:\n>\n> The args are read and concatenated together into a single\n> command.\n\nWhich means the two eval's give different results:\n\n $ e='echo \"a b\"'\n $ eval $e\n $ eval \"$e\"\n\n>> This will set up the sane_unset regardless of whether $env does\n>> anything. Would it make more sense to stick the test_when_finished\n>> inside $env? You could use regular unset then, too, since you know the\n>> variable would be set.\n>\n> I didn't worry about it too much because the end result is effectively\n> the same and, with all the 'case' arms being short one-liners, I think\n> the code is a bit easier to read as-is.\n\nYeah, this is OK.\n"},{"id":"286100","messageId":"CAPig+cTtU9_3=2eu0boaPbXKXh2gngEe7byDpJuSFAR4rcbrMA@mail.gmail.com","threadId":"42270","inReplyTo":"CAPig+cRR49iJOkaLfynkvch4zUHVkpcJwVah0rvaEOeR7aY2Nw@mail.gmail.com","subject":"Re: [PATCH 5/6] t1500: avoid setting environment variables outside of tests","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-05-10T19:48:56Z","receivedAt":"2016-05-10T19:48:56Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, May 10, 2016 at 3:12 PM, Eric Sunshine wrote:\n> On Tue, May 10, 2016 at 2:39 PM, Jeff King wrote:\n>> On Tue, May 10, 2016 at 01:20:54AM -0400, Eric Sunshine wrote:\n>>> while :\n>>> do\n>>> case \"$1\" in\n>>> -C) dir=\"-C $2\"; shift; shift ;;\n>>> -b) bare=\"$2\"; shift; shift ;;\n>>> + -g) env=\"GIT_DIR=$2; export GIT_DIR\"; shift; shift ;;\n>>> @@ -36,6 +38,8 @@ test_rev_parse () {\n>>> do\n>>> expect=\"$1\"\n>>> test_expect_success \"$name: $o\" '\n>>> + test_when_finished \"sane_unset GIT_DIR\" &&\n>>> + eval $env &&\n>>\n>> This will set up the sane_unset regardless of whether $env does\n>> anything. Would it make more sense to stick the test_when_finished\n>> inside $env? You could use regular unset then, too, since you know the\n>> variable would be set.\n>\n> I didn't worry about it too much because the end result is effectively\n> the same and, with all the 'case' arms being short one-liners, I think\n> the code is a bit easier to read as-is; bundling 'test_when_finished'\n> into the 'env' assignment line would probably require wrapping the\n> line. I do like the improved encapsulation of your suggestion but\n> don't otherwise feel strongly about it.\n\nActually, I think we can have improved encapsulation and maintain\nreadability like this:\n\n case \"$1\" in\n ...\n -g) env=\"$2\"; shift; shift ;;\n ...\n esac\n\n ...\n test_expect_success \"...\" '\n if test -n \"$env\"\n do\n test_when_finished \"unset GIT_DIR\"\n GIT_DIR=\"$env\"\n export GIT_DIR\n fi\n ...\n '\n"},{"id":"286103","messageId":"CAPig+cQ0jEt10p--K8jNrFBi21PkmFXuHpUPewuOWrpx+3Ma7g@mail.gmail.com","threadId":"42270","inReplyTo":"CAPig+cTtU9_3=2eu0boaPbXKXh2gngEe7byDpJuSFAR4rcbrMA@mail.gmail.com","subject":"Re: [PATCH 5/6] t1500: avoid setting environment variables outside of tests","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-05-10T19:59:42Z","receivedAt":"2016-05-10T19:59:42Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, May 10, 2016 at 3:48 PM, Eric Sunshine wrote:\n> Actually, I think we can have improved encapsulation and maintain\n> readability like this:\n>\n> case \"$1\" in\n> ...\n> -g) env=\"$2\"; shift; shift ;;\n> ...\n> esac\n>\n> ...\n> test_expect_success \"...\" '\n> if test -n \"$env\"\n> do\n> test_when_finished \"unset GIT_DIR\"\n> GIT_DIR=\"$env\"\n> export GIT_DIR\n> fi\n> ...\n> '\n\nAt this point, I'd also rename 'env' to 'gitdir' to be more meaningful.\n"},{"id":"286104","messageId":"xmqqk2j1oeww.fsf@gitster.mtv.corp.google.com","threadId":"42270","inReplyTo":"CAPig+cTtU9_3=2eu0boaPbXKXh2gngEe7byDpJuSFAR4rcbrMA@mail.gmail.com","subject":"Re: [PATCH 5/6] t1500: avoid setting environment variables outside of tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-10T20:07:27Z","receivedAt":"2016-05-10T20:07:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Tue, May 10, 2016 at 3:12 PM, Eric Sunshine wrote:\n>> On Tue, May 10, 2016 at 2:39 PM, Jeff King wrote:\n>>> On Tue, May 10, 2016 at 01:20:54AM -0400, Eric Sunshine wrote:\n>>>> while :\n>>>> do\n>>>> case \"$1\" in\n>>>> -C) dir=\"-C $2\"; shift; shift ;;\n>>>> -b) bare=\"$2\"; shift; shift ;;\n>>>> + -g) env=\"GIT_DIR=$2; export GIT_DIR\"; shift; shift ;;\n>>>> @@ -36,6 +38,8 @@ test_rev_parse () {\n>>>> do\n>>>> expect=\"$1\"\n>>>> test_expect_success \"$name: $o\" '\n>>>> + test_when_finished \"sane_unset GIT_DIR\" &&\n>>>> + eval $env &&\n>>>\n>>> This will set up the sane_unset regardless of whether $env does\n>>> anything. Would it make more sense to stick the test_when_finished\n>>> inside $env? You could use regular unset then, too, since you know the\n>>> variable would be set.\n>>\n>> I didn't worry about it too much because the end result is effectively\n>> the same and, with all the 'case' arms being short one-liners, I think\n>> the code is a bit easier to read as-is; bundling 'test_when_finished'\n>> into the 'env' assignment line would probably require wrapping the\n>> line. I do like the improved encapsulation of your suggestion but\n>> don't otherwise feel strongly about it.\n>\n> Actually, I think we can have improved encapsulation and maintain\n> readability like this:\n>\n> case \"$1\" in\n> ...\n> -g) env=\"$2\"; shift; shift ;;\n> ...\n> esac\n>\n> ...\n> test_expect_success \"...\" '\n> if test -n \"$env\"\n> do\n> test_when_finished \"unset GIT_DIR\"\n> GIT_DIR=\"$env\"\n> export GIT_DIR\n> fi\n> ...\n> '\n\nThat's even better. Thanks.\n"},{"id":"286110","messageId":"20160510204159.GA19958@sigill.intra.peff.net","threadId":"42270","inReplyTo":"CAPig+cQ0jEt10p--K8jNrFBi21PkmFXuHpUPewuOWrpx+3Ma7g@mail.gmail.com","subject":"Re: [PATCH 5/6] t1500: avoid setting environment variables outside of tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-10T20:41:59Z","receivedAt":"2016-05-10T20:41:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 10, 2016 at 03:59:42PM -0400, Eric Sunshine wrote:\n\n> On Tue, May 10, 2016 at 3:48 PM, Eric Sunshine wrote:\n> > Actually, I think we can have improved encapsulation and maintain\n> > readability like this:\n> >\n> > case \"$1\" in\n> > ...\n> > -g) env=\"$2\"; shift; shift ;;\n> > ...\n> > esac\n> >\n> > ...\n> > test_expect_success \"...\" '\n> > if test -n \"$env\"\n> > do\n> > test_when_finished \"unset GIT_DIR\"\n> > GIT_DIR=\"$env\"\n> > export GIT_DIR\n> > fi\n> > ...\n> > '\n> \n> At this point, I'd also rename 'env' to 'gitdir' to be more meaningful.\n\nYeah, I like this even better. As much as I enjoy eval tricks, I think\nthis case is more readable with the condition written out.\n\n-Peff\n"},{"id":"286114","messageId":"20160510230111.Horde.JpLRouwWbSCYp7yuvqhy5SS@webmail.informatik.kit.edu","threadId":"42270","inReplyTo":"CAPig+cTtU9_3=2eu0boaPbXKXh2gngEe7byDpJuSFAR4rcbrMA@mail.gmail.com","subject":"Re: [PATCH 5/6] t1500: avoid setting environment variables outside of tests","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2016-05-10T21:01:11Z","receivedAt":"2016-05-10T21:01:11Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\nQuoting Eric Sunshine :\n\n> On Tue, May 10, 2016 at 3:12 PM, Eric Sunshine \n> wrote:\n>> On Tue, May 10, 2016 at 2:39 PM, Jeff King wrote:\n>>> On Tue, May 10, 2016 at 01:20:54AM -0400, Eric Sunshine wrote:\n>>>> while :\n>>>> do\n>>>> case \"$1\" in\n>>>> -C) dir=\"-C $2\"; shift; shift ;;\n>>>> -b) bare=\"$2\"; shift; shift ;;\n>>>> + -g) env=\"GIT_DIR=$2; export GIT_DIR\"; shift; shift ;;\n>>>> @@ -36,6 +38,8 @@ test_rev_parse () {\n>>>> do\n>>>> expect=\"$1\"\n>>>> test_expect_success \"$name: $o\" '\n>>>> + test_when_finished \"sane_unset GIT_DIR\" &&\n>>>> + eval $env &&\n>>>\n>>> This will set up the sane_unset regardless of whether $env does\n>>> anything. Would it make more sense to stick the test_when_finished\n>>> inside $env? You could use regular unset then, too, since you know the\n>>> variable would be set.\n>>\n>> I didn't worry about it too much because the end result is effectively\n>> the same and, with all the 'case' arms being short one-liners, I think\n>> the code is a bit easier to read as-is; bundling 'test_when_finished'\n>> into the 'env' assignment line would probably require wrapping the\n>> line. I do like the improved encapsulation of your suggestion but\n>> don't otherwise feel strongly about it.\n>\n> Actually, I think we can have improved encapsulation and maintain\n> readability like this:\n>\n> case \"$1\" in\n> ...\n> -g) env=\"$2\"; shift; shift ;;\n> ...\n> esac\n>\n> ...\n> test_expect_success \"...\" '\n> if test -n \"$env\"\n> do\n> test_when_finished \"unset GIT_DIR\"\n> GIT_DIR=\"$env\"\n> export GIT_DIR\n> fi\n> ...\n> '\n\nI wonder if is it really necessary to specify the path to the .git\ndirectory via $GIT_DIR. Would 'git --git-dir=/over/there' be just as\ngood? If yes, then how about\n\n git ${gitdir:+--git-dir=\"$gitdir\"} rev-parse ...\n"},{"id":"286116","messageId":"xmqqtwi5mxeh.fsf@gitster.mtv.corp.google.com","threadId":"42270","inReplyTo":"20160510230111.Horde.JpLRouwWbSCYp7yuvqhy5SS@webmail.informatik.kit.edu","subject":"Re: [PATCH 5/6] t1500: avoid setting environment variables outside of tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-10T21:11:02Z","receivedAt":"2016-05-10T21:11:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> I wonder if is it really necessary to specify the path to the .git\n> directory via $GIT_DIR. Would 'git --git-dir=/over/there' be just as\n> good?\n\nThen you are testing two different things that may go through\ndifferent codepaths.\n\nAdding yet another test to check \"git --git-dir=\" in addition is\nfine, but that is not a replacement. We do want to make sure that\n\"GIT_DIR=there git\" form keeps giving us the expected outcome.\n"},{"id":"286118","messageId":"CAPig+cRhaum3V0YPUFKBwySANr060GwtRUzfRV9fuQaN60pMgw@mail.gmail.com","threadId":"42270","inReplyTo":"xmqqtwi5mxeh.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 5/6] t1500: avoid setting environment variables outside of tests","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-05-10T21:19:19Z","receivedAt":"2016-05-10T21:19:19Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, May 10, 2016 at 5:11 PM, Junio C Hamano wrote:\n> SZEDER Gábor writes:\n>> I wonder if is it really necessary to specify the path to the .git\n>> directory via $GIT_DIR. Would 'git --git-dir=/over/there' be just as\n>> good?\n>\n> Then you are testing two different things that may go through\n> different codepaths.\n>\n> Adding yet another test to check \"git --git-dir=\" in addition is\n> fine, but that is not a replacement. We do want to make sure that\n> \"GIT_DIR=there git\" form keeps giving us the expected outcome.\n\nWhen working on this, I did test with --git-dir= in place of GIT_DIR\nand some tests failed, but I didn't follow through to see what the\nactual problem was, partly because the code was in flux and I may have\nmessed up something else, but primarily for the reason Junio gives\nabove: I wanted this modernization series to be faithful to the\noriginal; additional testing can be added later.\n"},{"id":"286682","messageId":"CAPig+cTPsya6_3D2wx=k3pVaDJ2PokUvK4muM7nz-eV+Ss5+tw@mail.gmail.com","threadId":"42270","inReplyTo":"xmqqeg99py5j.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/6] modernize t1500","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-05-16T17:39:52Z","receivedAt":"2016-05-16T17:39:52Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, May 10, 2016 at 2:26 PM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>> Junio C Hamano writes:\n>> Subject: [PATCH 7/6] t1500: finish preparation upfront\n>>\n>> The earlier tests do not attempt to modify the contents of .git (by\n>> creating objects or refs, for example), which means that even if\n>> some earlier tests before \"cp -R\" fail, the tests in repo.git will\n>> run in an environment that we can expect them to succeed in.\n>>\n>> Make it clear that tests in repo.git will be independent from the\n>> results of earlier tests done in .git by moving its initialization\n>> \"cp -R .git repo.git\" much higher in the test sequence.\n>>\n>> Signed-off-by: Junio C Hamano \n>> ---\n>\n> I think the same logic applies to other preparatory things like\n> creation of sub/dir in the working tree etc.\n\nHmm, so are you suggesting a single 'setup' test at the start of\nscript which does the 'cp -R' and creates those other directories\nneeded by later tests?\n"},{"id":"286699","messageId":"xmqq4m9x3ken.fsf@gitster.mtv.corp.google.com","threadId":"42270","inReplyTo":"CAPig+cTPsya6_3D2wx=k3pVaDJ2PokUvK4muM7nz-eV+Ss5+tw@mail.gmail.com","subject":"Re: [PATCH 0/6] modernize t1500","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-16T18:52:32Z","receivedAt":"2016-05-16T18:52:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Tue, May 10, 2016 at 2:26 PM, Junio C Hamano wrote:\n>> Junio C Hamano writes:\n>>> Junio C Hamano writes:\n>>> Subject: [PATCH 7/6] t1500: finish preparation upfront\n>>>\n>>> The earlier tests do not attempt to modify the contents of .git (by\n>>> creating objects or refs, for example), which means that even if\n>>> some earlier tests before \"cp -R\" fail, the tests in repo.git will\n>>> run in an environment that we can expect them to succeed in.\n>>>\n>>> Make it clear that tests in repo.git will be independent from the\n>>> results of earlier tests done in .git by moving its initialization\n>>> \"cp -R .git repo.git\" much higher in the test sequence.\n>>>\n>>> Signed-off-by: Junio C Hamano \n>>> ---\n>>\n>> I think the same logic applies to other preparatory things like\n>> creation of sub/dir in the working tree etc.\n>\n> Hmm, so are you suggesting a single 'setup' test at the start of\n> script which does the 'cp -R' and creates those other directories\n> needed by later tests?\n\nExactly.\n"}]}