On Wed, Apr 13, 2016 at 12:54 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote: > On Sat, Apr 9, 2016 at 7:19 AM, Michael Rappazzo <rappazzo@xxxxxxxxx> wrote: >> t1500-rev-parse has many tests which change directories and leak >> environment variables. This makes it difficult to add new tests without >> minding the environment variables and current directory. >> >> Each test is now setup, executed, and cleaned up without leaving anything >> behind. Tests which have textual expectations have been converted to use >> test_cmp (which will show a diff when the test is run with --verbose). > > It might be easier to review this if broken into several cleanup and > modernization patches, however, some comments below... I felt that the changes are repetitive enough that it did not necessitate separate patches. Perhaps after simplifying based on your suggestions, it will be even smaller. > >> Signed-off-by: Michael Rappazzo <rappazzo@xxxxxxxxx> >> --- >> diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh >> @@ -3,88 +3,571 @@ >> +test_expect_success '.git/: is-bare-repository' ' >> + (cd .git && test false = "$(git rev-parse --is-bare-repository)") >> +' >> >> -# label is-bare is-inside-git is-inside-work prefix git-dir absolute-git-dir >> +test_expect_success '.git/: is-inside-git-dir' ' >> + (cd .git && test true = "$(git rev-parse --is-inside-git-dir)") > > Simpler: > > test true = "$(git -C .git rev-parse --is-inside-git-dir)" > >> +' >> >> -ROOT=$(pwd) >> +test_expect_success '.git/: is-inside-work-tree' ' >> + (cd .git && test false = "$(git rev-parse --is-inside-work-tree)") > > Ditto. > >> +' >> >> -test_rev_parse toplevel false false true '' .git "$ROOT/.git" >> +test_expect_success '.git/: prefix' ' >> + ( >> + cd .git && >> + echo >expected && >> + git rev-parse --show-prefix >actual && >> + test_cmp expected actual >> + ) > > Likewise, you could drop the entire subshell: > > echo >expected && > git -C .git rev-parse --show-prefix >actual && > test_cmp expected actual > >> +' >> >> +test_expect_success '.git/: git-dir' ' >> + ( >> + cd .git && >> + echo . >expected && >> + git rev-parse --git-dir >actual && >> + test_cmp expected actual >> + ) > > Same here and for many subsequent tests (which I won't quote). > >> +' >> +test_expect_success 'core.bare = true: is-bare-repository' ' >> + git config core.bare true && >> + test_when_finished "git config core.bare false" && >> + test true = "$(git rev-parse --is-bare-repository)" > > Simpler: > > test_config core.bare true > > and then you can drop 'test_when_finished' altogether. However, even simpler: > > test true = "$(git -c core.bare=false rev-parse --is-bare-repository)" > > which allows you to drop 'test_config', as well. > > Ditto for subsequent tests (which I won't quote). > >> +' >> +test_expect_success 'core.bare undefined: is-bare-repository' ' >> + git config --unset core.bare && > > test_unconfig core.bare > >> + test_when_finished "git config core.bare false" && > > Why does this need to re-instate core.bare? > > Same comments for subsequent tests. > >> + test false = "$(git rev-parse --is-bare-repository)" >> +' >> +test_expect_success 'GIT_DIR=../.git, core.bare = false: is-bare-repository' ' >> + mkdir work && >> + test_when_finished "rm -rf work && git config core.bare false" && >> + ( >> + cd work && >> + export GIT_DIR=../.git && >> + export GIT_CONFIG="$(pwd)"/../.git/config >> + git config core.bare false && >> + test false = "$(git rev-parse --is-bare-repository)" >> + ) >> +' > > Same comments about -C to avoid the subshell and -c for configuration. > > Also, you can do one-shot environment variable setting for the command > invocation, so the subshell goes away, and everything inside the > subshell collapses to: > > test false = "$(GIT_DIR=... GIT_CONFIG=... > git -C work -c core.bare=false rev-parse ...)" > > Additionally, I'm confused about why this test "reverts" the > core.bare=false by setting core.bare=false in 'test_when_finished'. > > Ditto for subsequent tests. > >> +test_expect_success 'GIT_DIR=../repo.git, core.bare = false: is-bare-repository' ' >> + mkdir work && >> + cp -r .git repo.git && >> + test_when_finished "rm -r repo.git && rm -rf work && git config core.bare false" && > > If 'cp' fails, then 'test_when_finished' will never be invoked, which > means that the cleanup will never happen; thus 'test_when_finished' > needs to be called earlier. Ditto for subsequent tests. > >> + ( >> + cd work && >> + export GIT_DIR=../repo.git && >> + export GIT_CONFIG="$(pwd)"/../repo.git/config >> + git config core.bare false && >> + test false = "$(git rev-parse --is-bare-repository)" >> + ) >> +' > > Closing comments: > > By using -C, -c, and one-shot environment variables, you can ditch the > subshells, and most of these tests should collapse to one or two > lines. I follow, and I can make that adjustment. Thanks for the review. > > There seems to be a lot of repetition here. To reduce the repetition, > have you considered encoding the state which varies between tests into > a table and making the tests table-driven. Or, by encoding the varying > state in some nested for-loops? The nice thing about driving the tests > from a table or for-loops is that it is easier to see at a glance if > all cases are covered. I was aiming for readability and the ability to test an individual case. The previous version was driven by a function, which did reduce the repetition, but it made it not simple to run an individual test in the case of a failure. Would your table implementation be much different from the function, in terms of readability? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html