Re: [PATCH v2 2/2] t1500-rev-parse: rewrite each test to run in isolation

[Eric Sunshine <sunshine@sunshineco.com>]

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@gmail.com>
> ---
> 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