On Sat, Apr 16, 2016 at 12:13:50PM -0400, Michael Rappazzo 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. Test comparisons been converted to use test_cmp or test_stdout. Overall, I'm not enthused about how this patch unrolls the systematic function-driven approach taken by the original code and turns it into a series of highly repetitive individual tests. Not only does it make the patch mind-numbing to review, but it is far too easy for errors to creep into the conversion which simply wouldn't exist if a systematic approach was used (whether via function, table, or for-loops). The very fact that you missed several test_stdout conversion opportunities and didn't notice a bit of gunk (an unnecessary ">actual") or several bogus and misspelled "test_config care.bar =" invocations, makes a good argument that this patch's approach is undesirable. The fact that I also missed these problems when reading through the patch hammers the point home. It wasn't until I started actually changing the patch in order to present you with a "here's a diff atop your patch which fixes the issues" as a convenience, that I noticed the more serious problems. > Signed-off-by: Michael Rappazzo <rappazzo@xxxxxxxxx> > --- > diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh > @@ -3,85 +3,320 @@ > +test_expect_success '.git/objects/: git-dir' ' > + echo $(pwd)/.git >expect && > + git -C .git/objects rev-parse --git-dir >actual && > + test_cmp expect actual > +' You forgot to convert this test_cmp to test_stdout. > +test_expect_success 'subdirectory: prefix' ' > + mkdir -p sub/dir && > + test_when_finished "rm -rf sub" && > + test sub/dir/ = "$(git -C sub/dir rev-parse --show-prefix)" > +' You forgot to convert this 'test' to test_stdout. > -git config core.bare true > -test_rev_parse 'GIT_DIR=../repo.git, core.bare = true' true false false '' > +test_expect_success 'subdirectory: git-dir' ' > + mkdir -p sub/dir && > + test_when_finished "rm -rf sub" && > + echo $(pwd)/.git >expect && Nit: Here and one other place, you could quote this: "$(pwd)/.git" > + git -C sub/dir rev-parse --git-dir >actual && > + test_cmp expect actual > +' Ditto: test_cmp => test_stdout > +test_expect_success 'core.bare = true: is-bare-repository' ' > + test_config core.bare true && > + test_stdout true git rev-parse --is-bare-repository > +' Is there a reason you chose to use test_config rather than the more concise '-c foo=bar' as suggested by the review[1]? [1]: http://article.gmane.org/gmane.comp.version-control.git/291368 > +test_expect_success 'core.bare undefined: is-bare-repository' ' > + test_config core.bare "" && Is setting core.bare to "" really the same as undefining it, or is the effect the same only as an accident of implementation? (Genuine question; I haven't checked.) > + test_stdout 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" && > + test_config -C "$(pwd)"/.git core.bare false && Drop the unnecessary "$(pwd)"/ here and elsewhere. > + GIT_DIR=../.git test_stdout false git -C work rev-parse --is-bare-repository > +' > + > +test_expect_success 'GIT_DIR=../.git, core.bare = false: prefix' ' > + mkdir work && > + test_when_finished "rm -rf work" && > + test_config -C "$(pwd)"/.git core.bare false && > + GIT_DIR=../.git test_stdout "" git -C work rev-parse --show-prefix >actual Drop the unnecessary '>actual' redirection. > +' > + > +test_expect_success 'GIT_DIR=../.git, core.bare undefined: is-bare-repository' ' > + mkdir work && > + test_when_finished "rm -rf work" && > + test_config -C "$(pwd)"/.git core.bar = && What is "core.bar =" (here and elsewhere)? > + GIT_DIR=../.git test_stdout false git -C work rev-parse --is-bare-repository > +' > + > +test_expect_success 'GIT_DIR=../repo.git, core.bare = false: is-bare-repository' ' > + mkdir work && > + test_when_finished "rm -rf work" && > + cp -r .git repo.git && > + test_when_finished "rm -r repo.git" && You could coalesce these two test_when_finished invocations: test_when_finished "rm -rf work repo.git" && Windows folk might appreciate it since process spawning is expensive and slow there. > + test_config -C "$(pwd)"/repo.git core.bare false && > + GIT_DIR=../repo.git test_stdout false git -C work rev-parse --is-bare-repository > +' For convenience, here's a diff atop your patch which addresses the above issues (except the question about core.bare set to "" versus being undefined). However, as noted above, I think this patch's approach is going in the wrong direction. --- 8< --- diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh index e2c2a06..beaf6e3 100755 --- a/t/t1500-rev-parse.sh +++ b/t/t1500-rev-parse.sh @@ -60,9 +60,7 @@ test_expect_success '.git/objects/: prefix' ' ' test_expect_success '.git/objects/: git-dir' ' - echo $(pwd)/.git >expect && - git -C .git/objects rev-parse --git-dir >actual && - test_cmp expect actual + test_stdout "$(pwd)/.git" git -C .git/objects rev-parse --git-dir ' test_expect_success 'subdirectory: is-bare-repository' ' @@ -86,237 +84,193 @@ test_expect_success 'subdirectory: is-inside-work-tree' ' test_expect_success 'subdirectory: prefix' ' mkdir -p sub/dir && test_when_finished "rm -rf sub" && - test sub/dir/ = "$(git -C sub/dir rev-parse --show-prefix)" + test_stdout sub/dir/ git -C sub/dir rev-parse --show-prefix ' test_expect_success 'subdirectory: git-dir' ' mkdir -p sub/dir && test_when_finished "rm -rf sub" && - echo $(pwd)/.git >expect && - git -C sub/dir rev-parse --git-dir >actual && - test_cmp expect actual + test_stdout "$(pwd)/.git" git -C sub/dir rev-parse --git-dir ' test_expect_success 'core.bare = true: is-bare-repository' ' - test_config core.bare true && - test_stdout true git rev-parse --is-bare-repository + test_stdout true git -c core.bare=true rev-parse --is-bare-repository ' test_expect_success 'core.bare = true: is-inside-git-dir' ' - test_config core.bare true && - test_stdout false git rev-parse --is-inside-git-dir + test_stdout false git -c core.bare=true rev-parse --is-inside-git-dir ' test_expect_success 'core.bare = true: is-inside-work-tree' ' - test_config core.bare true && - test_stdout false git rev-parse --is-inside-work-tree + test_stdout false git -c core.bare=true rev-parse --is-inside-work-tree ' test_expect_success 'core.bare undefined: is-bare-repository' ' - test_config core.bare "" && - test_stdout false git rev-parse --is-bare-repository + test_stdout false git -c core.bare= rev-parse --is-bare-repository ' test_expect_success 'core.bare undefined: is-inside-git-dir' ' - test_config core.bare "" && - test_stdout false git rev-parse --is-inside-git-dir + test_stdout false git -c core.bare= rev-parse --is-inside-git-dir ' test_expect_success 'core.bare undefined: is-inside-work-tree' ' - test_config core.bare "" && - test_stdout true git rev-parse --is-inside-work-tree + test_stdout true git -c core.bare= rev-parse --is-inside-work-tree ' test_expect_success 'GIT_DIR=../.git, core.bare = false: is-bare-repository' ' - mkdir work && test_when_finished "rm -rf work" && - test_config -C "$(pwd)"/.git core.bare false && - GIT_DIR=../.git test_stdout false git -C work rev-parse --is-bare-repository + mkdir work && + GIT_DIR=../.git test_stdout false git -C work -c core.bare=false rev-parse --is-bare-repository ' test_expect_success 'GIT_DIR=../.git, core.bare = false: is-inside-git-dir' ' - mkdir work && test_when_finished "rm -rf work" && - test_config -C "$(pwd)"/.git core.bare false && - GIT_DIR=../.git test_stdout false git -C work rev-parse --is-inside-git-dir + mkdir work && + GIT_DIR=../.git test_stdout false git -C work -c core.bare=false rev-parse --is-inside-git-dir ' test_expect_success 'GIT_DIR=../.git, core.bare = false: is-inside-work-tree' ' - mkdir work && test_when_finished "rm -rf work" && - test_config -C "$(pwd)"/.git core.bare false && - GIT_DIR=../.git test_stdout true git -C work rev-parse --is-inside-work-tree + mkdir work && + GIT_DIR=../.git test_stdout true git -C work -c core.bare=false rev-parse --is-inside-work-tree ' test_expect_success 'GIT_DIR=../.git, core.bare = false: prefix' ' - mkdir work && test_when_finished "rm -rf work" && - test_config -C "$(pwd)"/.git core.bare false && - GIT_DIR=../.git test_stdout "" git -C work rev-parse --show-prefix >actual + mkdir work && + GIT_DIR=../.git test_stdout "" git -C work -c core.bare=false rev-parse --show-prefix ' test_expect_success 'GIT_DIR=../.git, core.bare = true: is-bare-repository' ' - mkdir work && test_when_finished "rm -rf work" && - test_config -C "$(pwd)"/.git core.bare true && - GIT_DIR=../.git test_stdout true git -C work rev-parse --is-bare-repository + mkdir work && + GIT_DIR=../.git test_stdout true git -C work -c core.bare=true rev-parse --is-bare-repository ' test_expect_success 'GIT_DIR=../.git, core.bare = true: is-inside-git-dir' ' - mkdir work && test_when_finished "rm -rf work" && - test_config -C "$(pwd)"/.git core.bare true && - GIT_DIR=../.git test_stdout false git -C work rev-parse --is-inside-git-dir + mkdir work && + GIT_DIR=../.git test_stdout false git -C work -c core.bare=true rev-parse --is-inside-git-dir ' test_expect_success 'GIT_DIR=../.git, core.bare = true: is-inside-work-tree' ' - mkdir work && test_when_finished "rm -rf work" && - test_config -C "$(pwd)"/.git core.bare true && - GIT_DIR=../.git test_stdout false git -C work rev-parse --is-inside-work-tree + mkdir work && + GIT_DIR=../.git test_stdout false git -C work -c core.bare=true rev-parse --is-inside-work-tree ' test_expect_success 'GIT_DIR=../.git, core.bare = true: prefix' ' - mkdir work && test_when_finished "rm -rf work" && - test_config -C "$(pwd)"/.git core.bare true && - GIT_DIR=../.git test_stdout "" git -C work rev-parse --show-prefix + mkdir work && + GIT_DIR=../.git test_stdout "" git -C work -c core.bare=true rev-parse --show-prefix ' test_expect_success 'GIT_DIR=../.git, core.bare undefined: is-bare-repository' ' - mkdir work && test_when_finished "rm -rf work" && - test_config -C "$(pwd)"/.git core.bar = && - GIT_DIR=../.git test_stdout false git -C work rev-parse --is-bare-repository + mkdir work && + GIT_DIR=../.git test_stdout false git -C work -c core.bare= rev-parse --is-bare-repository ' test_expect_success 'GIT_DIR=../.git, core.bare undefined: is-inside-git-dir' ' - mkdir work && test_when_finished "rm -rf work" && - test_config -C "$(pwd)"/.git core.bar = && - GIT_DIR=../.git test_stdout false git -C work rev-parse --is-inside-git-dir + mkdir work && + GIT_DIR=../.git test_stdout false git -C work -c core.bare= rev-parse --is-inside-git-dir ' test_expect_success 'GIT_DIR=../.git, core.bare undefined: is-inside-work-tree' ' - mkdir work && test_when_finished "rm -rf work" && - test_config -C "$(pwd)"/.git core.bar = && - GIT_DIR=../.git test_stdout true git -C work rev-parse --is-inside-work-tree + mkdir work && + GIT_DIR=../.git test_stdout true git -C work -c core.bare= rev-parse --is-inside-work-tree ' test_expect_success 'GIT_DIR=../.git, core.bare undefined: prefix' ' - mkdir work && test_when_finished "rm -rf work" && - test_config -C "$(pwd)"/.git core.bar = && - GIT_DIR=../.git test_stdout "" git -C work rev-parse --show-prefix + mkdir work && + GIT_DIR=../.git test_stdout "" git -C work -c core.bare= rev-parse --show-prefix ' test_expect_success 'GIT_DIR=../repo.git, core.bare = false: is-bare-repository' ' + test_when_finished "rm -rf work repo.git" && mkdir work && - test_when_finished "rm -rf work" && cp -r .git repo.git && - test_when_finished "rm -r repo.git" && - test_config -C "$(pwd)"/repo.git core.bare false && - GIT_DIR=../repo.git test_stdout false git -C work rev-parse --is-bare-repository + GIT_DIR=../repo.git test_stdout false git -C work -c core.bare=false rev-parse --is-bare-repository ' test_expect_success 'GIT_DIR=../repo.git, core.bare = false: is-inside-git-dir' ' + test_when_finished "rm -rf work repo.git" && mkdir work && - test_when_finished "rm -rf work" && cp -r .git repo.git && - test_when_finished "rm -r repo.git" && - test_config -C "$(pwd)"/repo.git core.bare false && - GIT_DIR=../repo.git test_stdout false git -C work rev-parse --is-inside-git-dir + GIT_DIR=../repo.git test_stdout false git -C work -c core.bare=false rev-parse --is-inside-git-dir ' test_expect_success 'GIT_DIR=../repo.git, core.bare = false: is-inside-work-tree' ' + test_when_finished "rm -rf work repo.git" && mkdir work && - test_when_finished "rm -rf work" && cp -r .git repo.git && - test_when_finished "rm -r repo.git" && - test_config -C "$(pwd)"/repo.git core.bare false && - GIT_DIR=../repo.git test_stdout true git -C work rev-parse --is-inside-work-tree + GIT_DIR=../repo.git test_stdout true git -C work -c core.bare=false rev-parse --is-inside-work-tree ' test_expect_success 'GIT_DIR=../repo.git, core.bare = false: prefix' ' + test_when_finished "rm -rf work repo.git" && mkdir work && - test_when_finished "rm -rf work" && cp -r .git repo.git && - test_when_finished "rm -r repo.git" && - test_config -C "$(pwd)"/repo.git core.bare false && - GIT_DIR=../repo.git test_stdout "" git -C work rev-parse --show-prefix + GIT_DIR=../repo.git test_stdout "" git -C work -c core.bare=false rev-parse --show-prefix ' test_expect_success 'GIT_DIR=../repo.git, core.bare = true: is-bare-repository' ' + test_when_finished "rm -rf work repo.git" && mkdir work && - test_when_finished "rm -rf work" && cp -r .git repo.git && - test_when_finished "rm -r repo.git" && - test_config -C "$(pwd)"/repo.git core.bare true && - GIT_DIR=../repo.git test_stdout true git -C work rev-parse --is-bare-repository + GIT_DIR=../repo.git test_stdout true git -C work -c core.bare=true rev-parse --is-bare-repository ' test_expect_success 'GIT_DIR=../repo.git, core.bare = true: is-inside-git-dir' ' + test_when_finished "rm -rf work repo.git" && mkdir work && - test_when_finished "rm -rf work" && cp -r .git repo.git && - test_when_finished "rm -r repo.git" && - test_config -C "$(pwd)"/repo.git core.bare true && - GIT_DIR=../repo.git test_stdout false git -C work rev-parse --is-inside-git-dir + GIT_DIR=../repo.git test_stdout false git -C work -c core.bare=true rev-parse --is-inside-git-dir ' test_expect_success 'GIT_DIR=../repo.git, core.bare = true: is-inside-work-tree' ' + test_when_finished "rm -rf work repo.git" && mkdir work && - test_when_finished "rm -rf work" && cp -r .git repo.git && - test_when_finished "rm -r repo.git" && - test_config -C "$(pwd)"/repo.git core.bare true && - GIT_DIR=../repo.git test_stdout false git -C work rev-parse --is-inside-work-tree + GIT_DIR=../repo.git test_stdout false git -C work -c core.bare=true rev-parse --is-inside-work-tree ' test_expect_success 'GIT_DIR=../repo.git, core.bare = true: prefix' ' + test_when_finished "rm -rf work repo.git" && mkdir work && - test_when_finished "rm -rf work" && cp -r .git repo.git && - test_when_finished "rm -r repo.git" && - test_config -C "$(pwd)"/repo.git core.bare true && - GIT_DIR=../repo.git test_stdout "" git -C work rev-parse --show-prefix + GIT_DIR=../repo.git test_stdout "" git -C work -c core.bare=true rev-parse --show-prefix ' test_expect_success 'GIT_DIR=../repo.git, core.bare undefined: is-bare-repository' ' + test_when_finished "rm -rf work repo.git" && mkdir work && - test_when_finished "rm -rf work" && cp -r .git repo.git && - test_when_finished "rm -r repo.git" && - test_config -C "$(pwd)"/repo.git core.bare "" && - GIT_DIR=../repo.git test_stdout false git -C work rev-parse --is-bare-repository + GIT_DIR=../repo.git test_stdout false git -C work -c core.bare= rev-parse --is-bare-repository ' test_expect_success 'GIT_DIR=../repo.git, core.bare undefined: is-inside-git-dir' ' + test_when_finished "rm -rf work repo.git" && mkdir work && - test_when_finished "rm -rf work" && cp -r .git repo.git && - test_when_finished "rm -r repo.git" && - test_config -C "$(pwd)"/repo.git core.bare "" && - GIT_DIR=../repo.git test_stdout false git -C work rev-parse --is-inside-git-dir + GIT_DIR=../repo.git test_stdout false git -C work -c core.bare= rev-parse --is-inside-git-dir ' test_expect_success 'GIT_DIR=../repo.git, core.bare undefined: is-inside-work-tree' ' + test_when_finished "rm -rf work repo.git" && mkdir work && - test_when_finished "rm -rf work" && cp -r .git repo.git && - test_when_finished "rm -r repo.git" && - test_config -C "$(pwd)"/repo.git core.bare "" && - GIT_DIR=../repo.git test_stdout true git -C work rev-parse --is-inside-work-tree + GIT_DIR=../repo.git test_stdout true git -C work -c core.bare= rev-parse --is-inside-work-tree ' test_expect_success 'GIT_DIR=../repo.git, core.bare undefined: prefix' ' + test_when_finished "rm -rf work repo.git" && mkdir work && - test_when_finished "rm -rf work" && cp -r .git repo.git && - test_when_finished "rm -r repo.git" && - test_config -C "$(pwd)"/repo.git core.bare "" && - GIT_DIR=../repo.git test_stdout "" git -C work rev-parse --show-prefix + GIT_DIR=../repo.git test_stdout "" git -C work -c core.bare= rev-parse --show-prefix ' test_done -- 2.8.1.217.gcab2cda -- 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