threads / patch / 42047

v2, 2 partst1500-rev-parse: re-write t1500

Subject: [PATCH v2 0/2] t1500-rev-parse: re-write t1500

## tl;dr

13 messages between Apr 16, 2016 and Apr 17, 2016. Diffs are folded; open one to read it.

replies: 12people: 5as markdown or json

Michael Rappazzo· Apr 16, 2016, 16:13 UTC · lore
Differences between v1[1]:
	- Rebased the change on master.
	- Added a test-lib function `test_stdout` which is similar to `test_cmp`.
	  This addition is based on a patch from Jeff King[2] found the same 
	  discussion.
	- Cleaned up the use of subshells as recommended in the discussion.

[1] http://thread.gmane.org/gmane.comp.version-control.git/291087 [2] http://thread.gmane.org/gmane.comp.version-control.git/291087/focus=291475

Michael Rappazzo (2):
  test-lib: add a function to compare an expection with stdout from a
    command
  t1500-rev-parse: rewrite each test to run in isolation
 t/t1500-rev-parse.sh    | 355 ++++++++++++++++++++++++++++++++++++++++--------
 t/test-lib-functions.sh |  34 +++++
 2 files changed, 329 insertions(+), 60 deletions(-)
-- 
2.8.0
Michael Rappazzo· Apr 16, 2016, 16:13 UTC · re: Michael Rappazzo · lore

[PATCH v2 1/2] test-lib: add a function to compare an expection with stdout from a command

test_stdout accepts an expection and a command to execute. It will execute the command and then compare the stdout from that command to an expectation. If the expectation is not met, a mock diff output is written to stderr.

Based-on-a-patch-by: Jeff King <peff@peff.net>
Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
---
 t/test-lib-functions.sh | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)
Show changes to t/test-lib-functions.sh +34 −0
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 8d99eb3..95e54b2 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -941,3 +941,37 @@ mingw_read_file_strip_cr_ () {
 		eval "$1=\$$1\$line"
 	done
 }
+
+#	test_stdout is a helper function to compare expected output with
+#	the standard output of a command execution
+#
+#	Args:
+#		1: The expected output
+#		2: The command to run
+#
+#	You can use it like:
+#
+#	test_expect_success 'foo works' '
+#		test_cmp "This is expected" cmd_to_run arg1 arg2 ... argN
+#	'
+#
+#	The output when there is a mismatch mimics diff output, but this
+#	can break down for a multi-line result
+test_stdout () {
+	expect=$1
+	shift
+	if ! actual=$("$@")
+	then
+		echo "test_stdout: command failed: '$*'" >&2
+		return 1
+	fi
+	if test "$expect" != "$actual"
+	then
+		echo "test_stdout: unexpected output for '$*'" >&2
+		echo "@@ -N +N @@" >&2
+		echo "-$expect" >&2
+		echo "+$actual" >&2
+		return 1
+	fi
+	return 0
+}

-- 
2.8.0
Eric Sunshine· Apr 17, 2016, 03:07 UTC · re: Michael Rappazzo · lore

Re: [PATCH v2 1/2] test-lib: add a function to compare an expection with stdout from a command

On Sat, Apr 16, 2016 at 12:13 PM, Michael Rappazzo <rappazzo@gmail.com> wrote:
> test-lib: add a function to compare an expection with stdout from a command
Rather long subject. Perhaps:
    test-lib: add convenience function to check command output
> test_stdout accepts an expection and a command to execute.  It will execute
> the command and then compare the stdout from that command to an expectation.
> If the expectation is not met, a mock diff output is written to stderr.

I wonder if this deserves more flexibility by accepting a comparison operator, such as = and !=, similar to test_line_count()? Although, I suppose such functionality could be added later if deemed useful.

> Based-on-a-patch-by: Jeff King <peff@peff.net>

Since Peff wrote the actual code[1], it might be worthwhile to give him authorship by prepending the commit message with a "From: Jeff King <peff@peff.net>" header.

Show 16 quoted lines
> Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
> ---
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> @@ -941,3 +941,37 @@ mingw_read_file_strip_cr_ () {
> +#      test_stdout is a helper function to compare expected output with
> +#      the standard output of a command execution
> +#
> +#      Args:
> +#              1: The expected output
> +#              2: The command to run
> +#
> +#      You can use it like:
> +#
> +#      test_expect_success 'foo works' '
> +#              test_cmp "This is expected" cmd_to_run arg1 arg2 ... argN
> +#      '
test_cmp?
> +#      The output when there is a mismatch mimics diff output, but this
> +#      can break down for a multi-line result

Hmm, considering that $(...) collapses each whitespace run (including newlines) down to a single space, I don't see how you could get a multi-line result.

By the way, either the documentation should mention this limitation ("not possible to check multi-line output") or the implementation should be upgraded to support it.

Show 14 quoted lines
> +test_stdout () {
> +       expect=$1
> +       shift
> +       if ! actual=$("$@")
> +       then
> +               echo "test_stdout: command failed: '$*'" >&2
> +               return 1
> +       fi
> +       if test "$expect" != "$actual"
> +       then
> +               echo "test_stdout: unexpected output for '$*'" >&2
> +               echo "@@ -N +N @@" >&2
> +               echo "-$expect" >&2
> +               echo "+$actual" >&2

This faux diff output is quite a bit more noisy than the simple error message emitted by Peff's original[1] and it doesn't provide any additional useful information, so it doesn't feel like an improvement. Adding quotes around $expect and $actual in Peff's error message would probably be an improvement, though.

> +               return 1
> +       fi
> +       return 0
> +}
[1]: http://article.gmane.org/gmane.comp.version-control.git/291475
Jeff King· Apr 17, 2016, 03:54 UTC · re: Eric Sunshine · lore

Re: [PATCH v2 1/2] test-lib: add a function to compare an expection with stdout from a command

On Sat, Apr 16, 2016 at 11:07:02PM -0400, Eric Sunshine wrote:
Show 7 quoted lines
> > test_stdout accepts an expection and a command to execute.  It will execute
> > the command and then compare the stdout from that command to an expectation.
> > If the expectation is not met, a mock diff output is written to stderr.
> 
> I wonder if this deserves more flexibility by accepting a comparison
> operator, such as = and !=, similar to test_line_count()? Although, I
> suppose such functionality could be added later if deemed useful.

IMHO the funny syntax would outweigh the readability benefits. Unlike test_line_count(), which is abstracting a portability solution, this is mostly just about trying to save a few lines.

Though I do actually find that:
  test_stdout false git rev-parse --whatever

isn't great, because there's no syntactic separator between the expected output and the actual command to run. So I dunno, maybe it would be better as:

  test_stdout false = git rev-parse --whatever
and then you get "!=" for free later on if you want it.
We could also do:
  test_stdout git rev-parse --whatever <<-\EOF
  false
  EOF

which is more robust for multi-line output, but I think part of the point is to keep these as simple one-liners. You're not buying all that much over:

  cat >expect <<-\EOF &&
  false
  EOF
  git rev-parse --whatever >actual &&
  test_cmp expect actual

Though I do admit I've considered such a helper for some tests where that pattern is repeated ad nauseam.

Show 5 quoted lines
> > Based-on-a-patch-by: Jeff King <peff@peff.net>
> 
> Since Peff wrote the actual code[1], it might be worthwhile to give
> him authorship by prepending the commit message with a "From: Jeff
> King <peff@peff.net>" header.

Michael contacted me offline asking how to credit, and I actually suggested the "Based-on" route. I'm OK with it either way.

And for the record, my contribution is:
  Signed-off-by: Jeff King <peff@peff.net>
in case there are any DCO questions.
-Peff
Eric Sunshine· Apr 17, 2016, 06:36 UTC · re: Jeff King · lore

Re: [PATCH v2 1/2] test-lib: add a function to compare an expection with stdout from a command

On Sat, Apr 16, 2016 at 11:54 PM, Jeff King <peff@peff.net> wrote:
Show 37 quoted lines
> On Sat, Apr 16, 2016 at 11:07:02PM -0400, Eric Sunshine wrote:
>> > test_stdout accepts an expection and a command to execute.  It will execute
>> > the command and then compare the stdout from that command to an expectation.
>> > If the expectation is not met, a mock diff output is written to stderr.
>>
>> I wonder if this deserves more flexibility by accepting a comparison
>> operator, such as = and !=, similar to test_line_count()? Although, I
>> suppose such functionality could be added later if deemed useful.
>
> [...] Though I do actually find that:
>
>   test_stdout false git rev-parse --whatever
>
> isn't great, because there's no syntactic separator between the expected
> output and the actual command to run. So I dunno, maybe it would be
> better as:
>
>   test_stdout false = git rev-parse --whatever
>
> [...] We could also do:
>
>   test_stdout git rev-parse --whatever <<-\EOF
>   false
>   EOF
>
> which is more robust for multi-line output, but I think part of the
> point is to keep these as simple one-liners. You're not buying all that
> much over:
>
>   cat >expect <<-\EOF &&
>   false
>   EOF
>   git rev-parse --whatever >actual &&
>   test_cmp expect actual
>
> Though I do admit I've considered such a helper for some tests where
> that pattern is repeated ad nauseam.

Agreed. I wouldn't mind the version where test_stdout grabs "expected" from <<EOF, but, as you say, it doesn't buy much over the manually prepared test_cmp version.

I suppose that the one-liner form of test_stdout could have its uses, however, it bothers me for a couple reasons: (1) it's not generally useful like the version which grabs "expected" from <<EOF, (2) it squats on a nice concise name which would better suit the <<EOF version.

Anyhow, this may all be moot (for now) since I think this patch series is going in the wrong direction entirely by abandoning the systematic approach taken by the original t1500 code, as explained in my review[1]. If modernization of t1500 retains a systematic approach, then the repetitive code which prompted the suggestion of test_stdout won't exist in the first place.

[1]: http://article.gmane.org/gmane.comp.version-control.git/291745
Jeff King· Apr 17, 2016, 06:41 UTC · re: Eric Sunshine · lore

Re: [PATCH v2 1/2] test-lib: add a function to compare an expection with stdout from a command

On Sun, Apr 17, 2016 at 02:36:24AM -0400, Eric Sunshine wrote:
Show 9 quoted lines
> Agreed. I wouldn't mind the version where test_stdout grabs "expected"
> from <<EOF, but, as you say, it doesn't buy much over the manually
> prepared test_cmp version.
> 
> I suppose that the one-liner form of test_stdout could have its uses,
> however, it bothers me for a couple reasons: (1) it's not generally
> useful like the version which grabs "expected" from <<EOF, (2) it
> squats on a nice concise name which would better suit the <<EOF
> version.

I think you could get around your second objection by making "-" a magic token, like:

  test_stdout - = git rev-parse ... <<-\EOF
  false
  EOF
Though I admit the combination of "-" and "=" is pretty ugly to read.

I'm OK with abandoning this line of inquiry, too. This may be a case where a little repetition makes things a lot less magical to a reader, and it's not worth trying to devise the perfect helper.

Show 6 quoted lines
> Anyhow, this may all be moot (for now) since I think this patch series
> is going in the wrong direction entirely by abandoning the systematic
> approach taken by the original t1500 code, as explained in my
> review[1]. If modernization of t1500 retains a systematic approach,
> then the repetitive code which prompted the suggestion of test_stdout
> won't exist in the first place.

Fair enough. I haven't really followed the other part of the series very closely.

-Peff
Johannes Sixt· Apr 17, 2016, 15:19 UTC · re: Eric Sunshine · lore

Re: [PATCH v2 1/2] test-lib: add a function to compare an expection with stdout from a command

Am 17.04.2016 um 05:07 schrieb Eric Sunshine:
> Hmm, considering that $(...) collapses each whitespace run (including
> newlines) down to a single space, I don't see how you could get a
> multi-line result.
No, it doesn't. It only removes trailing newlines:

~:1004> frotz=$(echo 1; echo; echo 2; echo; echo; echo); echo "$frotz" 1

2 ~:1005>

-- Hannes
Eric Sunshine· Apr 17, 2016, 16:22 UTC · re: Johannes Sixt · lore

Re: [PATCH v2 1/2] test-lib: add a function to compare an expection with stdout from a command

On Sun, Apr 17, 2016 at 11:19 AM, Johannes Sixt <j6t@kdbg.org> wrote:
Show 12 quoted lines
> Am 17.04.2016 um 05:07 schrieb Eric Sunshine:
>> Hmm, considering that $(...) collapses each whitespace run (including
>> newlines) down to a single space, I don't see how you could get a
>> multi-line result.
>
> No, it doesn't. It only removes trailing newlines:
>
> ~:1004> frotz=$(echo 1; echo; echo 2; echo; echo; echo); echo "$frotz"
> 1
>
> 2
> ~:1005>
Thanks for the correction.
Michael Rappazzo· Apr 16, 2016, 16:13 UTC · re: Michael Rappazzo · lore

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

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.

Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
---
 t/t1500-rev-parse.sh | 355 ++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 295 insertions(+), 60 deletions(-)
Show changes to t/t1500-rev-parse.sh +295 −60
diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 48ee077..e2c2a06 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -3,85 +3,320 @@
 test_description='test git rev-parse'
 . ./test-lib.sh
 
-test_rev_parse() {
-	name=$1
-	shift
+test_expect_success 'toplevel: is-bare-repository' '
+	test_stdout false git rev-parse --is-bare-repository
+'
 
-	test_expect_success "$name: is-bare-repository" \
-	"test '$1' = \"\$(git rev-parse --is-bare-repository)\""
-	shift
-	[ $# -eq 0 ] && return
+test_expect_success 'toplevel: is-inside-git-dir' '
+	test_stdout false git rev-parse --is-inside-git-dir
+'
 
-	test_expect_success "$name: is-inside-git-dir" \
-	"test '$1' = \"\$(git rev-parse --is-inside-git-dir)\""
-	shift
-	[ $# -eq 0 ] && return
+test_expect_success 'toplevel: is-inside-work-tree' '
+	test_stdout true git rev-parse --is-inside-work-tree
+'
 
-	test_expect_success "$name: is-inside-work-tree" \
-	"test '$1' = \"\$(git rev-parse --is-inside-work-tree)\""
-	shift
-	[ $# -eq 0 ] && return
+test_expect_success 'toplevel: prefix' '
+	test_stdout "" git rev-parse --show-prefix
+'
 
-	test_expect_success "$name: prefix" \
-	"test '$1' = \"\$(git rev-parse --show-prefix)\""
-	shift
-	[ $# -eq 0 ] && return
+test_expect_success 'toplevel: git-dir' '
+	test_stdout .git git rev-parse --git-dir
+'
 
-	test_expect_success "$name: git-dir" \
-	"test '$1' = \"\$(git rev-parse --git-dir)\""
-	shift
-	[ $# -eq 0 ] && return
-}
+test_expect_success '.git/: is-bare-repository' '
+	test_stdout false git -C .git rev-parse --is-bare-repository
+'
 
-# label is-bare is-inside-git is-inside-work prefix git-dir
+test_expect_success '.git/: is-inside-git-dir' '
+	test_stdout true git -C .git rev-parse --is-inside-git-dir
+'
 
-ROOT=$(pwd)
+test_expect_success '.git/: is-inside-work-tree' '
+	test_stdout false git -C .git rev-parse --is-inside-work-tree
+'
 
-test_rev_parse toplevel false false true '' .git
+test_expect_success '.git/: prefix' '
+	test_stdout "" git -C .git rev-parse --show-prefix
+'
 
-cd .git || exit 1
-test_rev_parse .git/ false true false '' .
-cd objects || exit 1
-test_rev_parse .git/objects/ false true false '' "$ROOT/.git"
-cd ../.. || exit 1
+test_expect_success '.git/: git-dir' '
+	test_stdout . git -C .git rev-parse --git-dir
+'
 
-mkdir -p sub/dir || exit 1
-cd sub/dir || exit 1
-test_rev_parse subdirectory false false true sub/dir/ "$ROOT/.git"
-cd ../.. || exit 1
+test_expect_success '.git/objects/: is-bare-repository' '
+	test_stdout false git -C .git/objects rev-parse --is-bare-repository
+'
 
-git config core.bare true
-test_rev_parse 'core.bare = true' true false false
+test_expect_success '.git/objects/: is-inside-git-dir' '
+	test_stdout true git -C .git/objects rev-parse --is-inside-git-dir
+'
 
-git config --unset core.bare
-test_rev_parse 'core.bare undefined' false false true
+test_expect_success '.git/objects/: is-inside-work-tree' '
+	test_stdout false git -C .git/objects rev-parse --is-inside-work-tree
+'
 
-mkdir work || exit 1
-cd work || exit 1
-GIT_DIR=../.git
-GIT_CONFIG="$(pwd)"/../.git/config
-export GIT_DIR GIT_CONFIG
+test_expect_success '.git/objects/: prefix' '
+	test_stdout "" git -C .git/objects rev-parse --show-prefix
+'
 
-git config core.bare false
-test_rev_parse 'GIT_DIR=../.git, core.bare = false' false false true ''
+test_expect_success '.git/objects/: git-dir' '
+	echo $(pwd)/.git >expect &&
+	git -C .git/objects rev-parse --git-dir >actual &&
+	test_cmp expect actual
+'
 
-git config core.bare true
-test_rev_parse 'GIT_DIR=../.git, core.bare = true' true false false ''
+test_expect_success 'subdirectory: is-bare-repository' '
+	mkdir -p sub/dir &&
+	test_when_finished "rm -rf sub" &&
+	test_stdout false git -C sub/dir rev-parse --is-bare-repository
+'
 
-git config --unset core.bare
-test_rev_parse 'GIT_DIR=../.git, core.bare undefined' false false true ''
+test_expect_success 'subdirectory: is-inside-git-dir' '
+	mkdir -p sub/dir &&
+	test_when_finished "rm -rf sub" &&
+	test_stdout false git -C sub/dir rev-parse --is-inside-git-dir
+'
 
-mv ../.git ../repo.git || exit 1
-GIT_DIR=../repo.git
-GIT_CONFIG="$(pwd)"/../repo.git/config
+test_expect_success 'subdirectory: is-inside-work-tree' '
+	mkdir -p sub/dir &&
+	test_when_finished "rm -rf sub" &&
+	test_stdout true git -C sub/dir rev-parse --is-inside-work-tree
+'
 
-git config core.bare false
-test_rev_parse 'GIT_DIR=../repo.git, core.bare = false' false false true ''
+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)"
+'
 
-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 &&
+	git -C sub/dir rev-parse --git-dir >actual &&
+	test_cmp expect actual
+'
 
-git config --unset core.bare
-test_rev_parse 'GIT_DIR=../repo.git, core.bare undefined' false false true ''
+test_expect_success 'core.bare = true: is-bare-repository' '
+	test_config core.bare true &&
+	test_stdout true git 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_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_expect_success 'core.bare undefined: is-bare-repository' '
+	test_config core.bare "" &&
+	test_stdout false git 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_expect_success 'core.bare undefined: is-inside-work-tree' '
+	test_config core.bare "" &&
+	test_stdout true git 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
+'
+
+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
+'
+
+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
+'
+
+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
+'
+
+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
+'
+
+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
+'
+
+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
+'
+
+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
+'
+
+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
+'
+
+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
+'
+
+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
+'
+
+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
+'
+
+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" &&
+	test_config -C "$(pwd)"/repo.git core.bare false &&
+	GIT_DIR=../repo.git test_stdout false git -C work rev-parse --is-bare-repository
+'
+
+test_expect_success 'GIT_DIR=../repo.git, core.bare = false: is-inside-git-dir' '
+	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
+'
+
+test_expect_success 'GIT_DIR=../repo.git, core.bare = false: is-inside-work-tree' '
+	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
+'
+
+test_expect_success 'GIT_DIR=../repo.git, core.bare = false: prefix' '
+	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
+'
+
+test_expect_success 'GIT_DIR=../repo.git, core.bare = true: is-bare-repository' '
+	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
+'
+
+test_expect_success 'GIT_DIR=../repo.git, core.bare = true: is-inside-git-dir' '
+	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
+'
+
+test_expect_success 'GIT_DIR=../repo.git, core.bare = true: is-inside-work-tree' '
+	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
+'
+
+test_expect_success 'GIT_DIR=../repo.git, core.bare = true: prefix' '
+	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
+'
+
+test_expect_success 'GIT_DIR=../repo.git, core.bare undefined: is-bare-repository' '
+	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
+'
+
+test_expect_success 'GIT_DIR=../repo.git, core.bare undefined: is-inside-git-dir' '
+	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
+'
+
+test_expect_success 'GIT_DIR=../repo.git, core.bare undefined: is-inside-work-tree' '
+	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
+'
+
+test_expect_success 'GIT_DIR=../repo.git, core.bare undefined: prefix' '
+	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
+'
 
 test_done

-- 
2.8.0
Eric Sunshine· Apr 17, 2016, 05:59 UTC · re: Michael Rappazzo · lore

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

On Sat, Apr 16, 2016 at 12:13:50PM -0400, Michael Rappazzo wrote:
Show 6 quoted lines
> 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.

Show 9 quoted lines
> 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.
Show 5 quoted lines
> +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.
Show 6 quoted lines
> -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.)

Show 6 quoted lines
> +	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.
Show 8 quoted lines
> +	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.
Show 6 quoted lines
> +'
> +
> +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)?
Show 8 quoted lines
> +	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< ---
Show changes to t/t1500-rev-parse.sh +57 −103
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
Johannes Sixt· Apr 17, 2016, 15:05 UTC · re: Eric Sunshine · lore

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

Am 17.04.2016 um 07:59 schrieb Eric Sunshine:
Show 8 quoted lines
> On Sat, Apr 16, 2016 at 12:13:50PM -0400, Michael Rappazzo wrote:
>> +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.

Not only that: setting an environment variable in front of a shell function invocation keeps the variable's value in some (most?) shells. This occurs frequently in the new code. I don't know whether we have a shorter pattern than

	(
		GIT_DIR=../.git &&
		export GIT_DIR &&
		test_stdout "" git -C work rev-parse --show-prefix
	)
-- Hannes
SZEDER Gábor· Apr 17, 2016, 09:42 UTC · re: Michael Rappazzo · lore

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

Quoting Michael Rappazzo <rappazzo@gmail.com>:
Show 8 quoted lines
> +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
> +'

Here and in the following tests as well: some shells don't cope that well with a one-shot environmental variable set in front of a shell function. See commit 512477b17528:

     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, like so:
             (
                     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.
Eric Sunshine· Apr 17, 2016, 16:15 UTC · re: SZEDER Gábor · lore

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

On Sun, Apr 17, 2016 at 5:42 AM, SZEDER Gábor <szeder@ira.uka.de> wrote:
Show 15 quoted lines
> Quoting Michael Rappazzo <rappazzo@gmail.com>:
>> +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
>> +'
>
> Here and in the following tests as well: some shells don't cope that well
> with a one-shot environmental variable set in front of a shell function.
> See commit 512477b17528:
>
>     tests: use "env" to run commands with temporary env-var settings

While reviewing the patch, I stared at that code for a good while thinking that there was something about it I ought to remember but couldn't, so thanks for the reminder (and j6t's too).

Considering that this patch is probably going in the wrong direction and that if, when re-rolled, it takes a systematic approach testing that the original code uses, then the "need" for test_stdout effectively disappears, so this issue should go away too (but it's good to remember, nevertheless).

← back to recent threads