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

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

[Michael Rappazzo]

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

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

[Michael Rappazzo]

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(+)

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

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

[Michael Rappazzo]

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(-)

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

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

[Eric Sunshine]

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.

> 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.

> +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

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

[Jeff King]

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.

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.

> > 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

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

[Eric Sunshine]

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

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

[Eric Sunshine]

On Sat, Apr 16, 2016 at 11:54 PM, Jeff King <peff@peff.net> wrote:
> 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

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

[Jeff King]

On Sun, Apr 17, 2016 at 02:36:24AM -0400, Eric Sunshine wrote:

> 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.

> 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

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

[SZEDER Gábor]

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

     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.

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

[Johannes Sixt]

Am 17.04.2016 um 07:59 schrieb Eric Sunshine:
> 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

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

[Johannes Sixt]

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

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

[Eric Sunshine]

On Sun, Apr 17, 2016 at 5:42 AM, SZEDER Gábor <szeder@ira.uka.de> wrote:
> 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).

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

[Eric Sunshine]

On Sun, Apr 17, 2016 at 11:19 AM, Johannes Sixt <j6t@kdbg.org> wrote:
> 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.