[PATCH 00/10] 'test_i18ngrep'-related fixes and improvements

[PATCH 00/10] 'test_i18ngrep'-related fixes and improvements

[SZEDER Gábor]

When 'test_i18ngrep' can't find the expected pattern, it exits
completely silently; when its negated form does find the pattern that
shouldn't be there, it prints the matching line(s) but otherwise exits
without any error message.  This leaves the developer puzzled about
what could have gone wrong.  Well, at least it left me puzzled...

Initially all I wanted to do was to make 'test_i18ngrep' more
informative on failure, but then skeletons started to fall out of the
closet^Wour test suite, and BAM! before I knew it I had 10 patches:


  t5541: add 'test_i18ngrep's missing filename parameter
  t5812: add 'test_i18ngrep's missing filename parameter
  t6022: don't run 'git merge' upstream of a pipe
  t4001: don't run 'git status' upstream of a pipe

Bugfixes for a few tests.  The first two are fun.

  t5510: consolidate 'grep' and 'test_i18ngrep' patterns
  t5536: let 'test_i18ngrep' read the file without redirection

Cleanups.

  t: move 'test_i18ncmp' and 'test_i18ngrep' to 'test-lib-functions.sh'

Pure code movement.

  t: forbid piping into 'test_i18ngrep'
  t: make sure that 'test_i18ngrep' got enough parameters

These try to prevent similar bugs in our tests in the future.  Both
are imperfect, see the commit messages about their limitations and why
I think they are good enough.

  t: make 'test_i18ngrep' more informative on failure

Again, see the commit message about its limitations and why it's good
enough.


 t/t4001-diff-rename.sh        | 11 ++++++---
 t/t5510-fetch.sh              |  9 +++-----
 t/t5536-fetch-conflicts.sh    |  2 +-
 t/t5541-http-push-smart.sh    |  2 +-
 t/t5812-proto-disable-http.sh |  3 +--
 t/t6022-merge-rename.sh       |  6 +++--
 t/test-lib-functions.sh       | 53 +++++++++++++++++++++++++++++++++++++++++++
 t/test-lib.sh                 | 26 ---------------------
 8 files changed, 71 insertions(+), 41 deletions(-)

-- 
2.16.1.155.g5159265b1

[PATCH 01/10] t5541: add 'test_i18ngrep's missing filename parameter

[SZEDER Gábor]

The test 'push --no-progress silences progress but not status' runs
'test_i18ngrep' without specifying a filename parameter.  This has
remained unnoticed since its introduction in e304aeba2 (t5541: test
more combinations of --progress, 2012-05-01), because that
'test_i18ngrep' is supposed to check that the given pattern is not
present in its input, and of course it won't find that pattern if its
input is empty, (as it comes from /dev/null).  This also means that
this test could miss a potential breakage of 'git push --no-progress'.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t5541-http-push-smart.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
index d38bf3247..21340e89c 100755
--- a/t/t5541-http-push-smart.sh
+++ b/t/t5541-http-push-smart.sh
@@ -234,7 +234,7 @@ test_expect_success TTY 'push --no-progress silences progress but not status' '
 	test_commit no-progress &&
 	test_terminal git push --no-progress >output 2>&1 &&
 	test_i18ngrep "^To http" output &&
-	test_i18ngrep ! "^Writing objects"
+	test_i18ngrep ! "^Writing objects" output
 '
 
 test_expect_success 'push --progress shows progress to non-tty' '
-- 
2.16.1.155.g5159265b1

[PATCH 02/10] t5812: add 'test_i18ngrep's missing filename parameter

[SZEDER Gábor]

The second 'test_i18ngrep' invocation in the test 'curl redirects
respect whitelist' is missing its filename parameter.  This has
remained unnoticed since its introduction in f4113cac0 (http: limit
redirection to protocol-whitelist, 2015-09-22), because it would only
cause the test to fail if Git was built with a sufficiently old
libcurl version.  The test's two ||-chained 'test_i18ngrep'
invocations are supposed to check that either one of the two patterns
is present in 'git clone's error message.  As it happens, the first
invocation covers the error message from any reasonably up-to-date
libcurl, thus the second invocation, the one without the filename
parameter, isn't executed at all.  Apparently no one has run the test
suite's httpd tests with such an old libcurl in the last 2+ years, or
at least they haven't bothered to notify us about the failed test.

Fix this by consolidating the two patterns into a single extended
regexp, eliminating the need for an ||-chained second 'test_i18ngrep'
invocation.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t5812-proto-disable-http.sh | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/t/t5812-proto-disable-http.sh b/t/t5812-proto-disable-http.sh
index d911afd24..226a4920c 100755
--- a/t/t5812-proto-disable-http.sh
+++ b/t/t5812-proto-disable-http.sh
@@ -21,8 +21,7 @@ test_expect_success 'curl redirects respect whitelist' '
 			   GIT_SMART_HTTP=0 \
 		git clone "$HTTPD_URL/ftp-redir/repo.git" 2>stderr &&
 	{
-		test_i18ngrep "ftp.*disabled" stderr ||
-		test_i18ngrep "your curl version is too old"
+		test_i18ngrep -E "(ftp.*disabled|your curl version is too old)" stderr
 	}
 '
 
-- 
2.16.1.155.g5159265b1

[PATCH 03/10] t6022: don't run 'git merge' upstream of a pipe

[SZEDER Gábor]

The primary purpose of 't6022-merge-rename.sh' is to test 'git merge',
but one of the tests runs it upstream of a pipe, hiding its exit code.
Consequently, the test could continue even if 'git merge' exited with
error.

Use an intermediate file between 'git merge' and 'test_i18ngrep' to
catch a potential failure of the former.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t6022-merge-rename.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/t/t6022-merge-rename.sh b/t/t6022-merge-rename.sh
index 05ebba7af..c01f721f1 100755
--- a/t/t6022-merge-rename.sh
+++ b/t/t6022-merge-rename.sh
@@ -242,10 +242,12 @@ test_expect_success 'merge of identical changes in a renamed file' '
 	rm -f A M N &&
 	git reset --hard &&
 	git checkout change+rename &&
-	GIT_MERGE_VERBOSITY=3 git merge change | test_i18ngrep "^Skipped B" &&
+	GIT_MERGE_VERBOSITY=3 git merge change >out &&
+	test_i18ngrep "^Skipped B" out &&
 	git reset --hard HEAD^ &&
 	git checkout change &&
-	GIT_MERGE_VERBOSITY=3 git merge change+rename | test_i18ngrep "^Skipped B"
+	GIT_MERGE_VERBOSITY=3 git merge change+rename >out &&
+	test_i18ngrep "^Skipped B" out
 '
 
 test_expect_success 'setup for rename + d/f conflicts' '
-- 
2.16.1.155.g5159265b1

[PATCH 04/10] t4001: don't run 'git status' upstream of a pipe

[SZEDER Gábor]

The primary purpose of three tests in 't4001-diff-rename.sh' is to
check rename detection in 'git status', but all three do so by running
'git status' upstream of a pipe, hiding its exit code.  Consequently,
the test could continue even if 'git status' exited with error.

Use an intermediate file between 'git status' and 'test_i18ngrep' to
catch a potential failure of the former.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t4001-diff-rename.sh | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/t/t4001-diff-rename.sh b/t/t4001-diff-rename.sh
index eadf4f624..a07816d56 100755
--- a/t/t4001-diff-rename.sh
+++ b/t/t4001-diff-rename.sh
@@ -134,11 +134,15 @@ test_expect_success 'favour same basenames over different ones' '
 	git rm path1 &&
 	mkdir subdir &&
 	git mv another-path subdir/path1 &&
-	git status | test_i18ngrep "renamed: .*path1 -> subdir/path1"'
+	git status >out &&
+	test_i18ngrep "renamed: .*path1 -> subdir/path1" out
+'
 
 test_expect_success 'favour same basenames even with minor differences' '
 	git show HEAD:path1 | sed "s/15/16/" > subdir/path1 &&
-	git status | test_i18ngrep "renamed: .*path1 -> subdir/path1"'
+	git status >out &&
+	test_i18ngrep "renamed: .*path1 -> subdir/path1" out
+'
 
 test_expect_success 'two files with same basename and same content' '
 	git reset --hard &&
@@ -148,7 +152,8 @@ test_expect_success 'two files with same basename and same content' '
 	git add dir &&
 	git commit -m 2 &&
 	git mv dir other-dir &&
-	git status | test_i18ngrep "renamed: .*dir/A/file -> other-dir/A/file"
+	git status >out &&
+	test_i18ngrep "renamed: .*dir/A/file -> other-dir/A/file" out
 '
 
 test_expect_success 'setup for many rename source candidates' '
-- 
2.16.1.155.g5159265b1

[PATCH 05/10] t5510: consolidate 'grep' and 'test_i18ngrep' patterns

[SZEDER Gábor]

One of the tests in 't5510-fetch.sh' checks the output of 'git fetch'
using 'test_i18ngrep', and while doing so it prefilters the output
with 'grep' before piping the result into 'test_i18ngrep'.

This prefiltering is unnecessary, with the appropriate pattern
'test_i18ngrep' can do it all by itself.  Furthermore, piping data
into 'test_i18ngrep' will interfere with the linting that will be
added in a later patch.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t5510-fetch.sh | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 668c54be4..3debc87d4 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -222,12 +222,9 @@ test_expect_success 'fetch uses remote ref names to describe new refs' '
 	(
 		cd descriptive &&
 		git fetch o 2>actual &&
-		grep " -> refs/crazyheads/descriptive-branch$" actual |
-		test_i18ngrep "new branch" &&
-		grep " -> descriptive-tag$" actual |
-		test_i18ngrep "new tag" &&
-		grep " -> crazy$" actual |
-		test_i18ngrep "new ref"
+		test_i18ngrep "new branch.* -> refs/crazyheads/descriptive-branch$" actual &&
+		test_i18ngrep "new tag.* -> descriptive-tag$" actual &&
+		test_i18ngrep "new ref.* -> crazy$" actual
 	) &&
 	git checkout master
 '
-- 
2.16.1.155.g5159265b1

[PATCH 06/10] t5536: let 'test_i18ngrep' read the file without redirection

[SZEDER Gábor]

Redirecting 'test_i18ngrep's standard input from a file will interfere
with the linting that will be added in a later patch.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t5536-fetch-conflicts.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5536-fetch-conflicts.sh b/t/t5536-fetch-conflicts.sh
index 2e42cf331..644736b8a 100755
--- a/t/t5536-fetch-conflicts.sh
+++ b/t/t5536-fetch-conflicts.sh
@@ -22,7 +22,7 @@ verify_stderr () {
 	cat >expected &&
 	# We're not interested in the error
 	# "fatal: The remote end hung up unexpectedly":
-	test_i18ngrep -E '^(fatal|warning):' <error | grep -v 'hung up' >actual | sort &&
+	test_i18ngrep -E '^(fatal|warning):' error | grep -v 'hung up' >actual | sort &&
 	test_i18ncmp expected actual
 }
 
-- 
2.16.1.155.g5159265b1

[PATCH 07/10] t: move 'test_i18ncmp' and 'test_i18ngrep' to 'test-lib-functions.sh'

[SZEDER Gábor]

Both 'test_i18ncmp' and 'test_i18ngrep' helper functions are supposed
to be called from our test scripts, so they should be in
'test-lib-functions.sh'.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/test-lib-functions.sh | 26 ++++++++++++++++++++++++++
 t/test-lib.sh           | 26 --------------------------
 2 files changed, 26 insertions(+), 26 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 1701fe2a0..92ed02937 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -705,6 +705,32 @@ test_cmp_bin() {
 	cmp "$@"
 }
 
+# Use this instead of test_cmp to compare files that contain expected and
+# actual output from git commands that can be translated.  When running
+# under GETTEXT_POISON this pretends that the command produced expected
+# results.
+test_i18ncmp () {
+	test -n "$GETTEXT_POISON" || test_cmp "$@"
+}
+
+# Use this instead of "grep expected-string actual" to see if the
+# output from a git command that can be translated either contains an
+# expected string, or does not contain an unwanted one.  When running
+# under GETTEXT_POISON this pretends that the command produced expected
+# results.
+test_i18ngrep () {
+	if test -n "$GETTEXT_POISON"
+	then
+	    : # pretend success
+	elif test "x!" = "x$1"
+	then
+		shift
+		! grep "$@"
+	else
+		grep "$@"
+	fi
+}
+
 # Call any command "$@" but be more verbose about its
 # failure. This is handy for commands like "test" which do
 # not output anything when they fail.
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 9a0a21f49..852b22c80 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1062,32 +1062,6 @@ else
 	test_set_prereq C_LOCALE_OUTPUT
 fi
 
-# Use this instead of test_cmp to compare files that contain expected and
-# actual output from git commands that can be translated.  When running
-# under GETTEXT_POISON this pretends that the command produced expected
-# results.
-test_i18ncmp () {
-	test -n "$GETTEXT_POISON" || test_cmp "$@"
-}
-
-# Use this instead of "grep expected-string actual" to see if the
-# output from a git command that can be translated either contains an
-# expected string, or does not contain an unwanted one.  When running
-# under GETTEXT_POISON this pretends that the command produced expected
-# results.
-test_i18ngrep () {
-	if test -n "$GETTEXT_POISON"
-	then
-	    : # pretend success
-	elif test "x!" = "x$1"
-	then
-		shift
-		! grep "$@"
-	else
-		grep "$@"
-	fi
-}
-
 test_lazy_prereq PIPE '
 	# test whether the filesystem supports FIFOs
 	test_have_prereq !MINGW,!CYGWIN &&
-- 
2.16.1.155.g5159265b1

[PATCH 08/10] t: forbid piping into 'test_i18ngrep'

[SZEDER Gábor]

When checking a git command's output with 'test_i18ngrep', it's
tempting to conveniently pipe the git command's standard output into
'test_i18ngrep'.  Unfortunately, this is an anti-pattern, because it
hides the git command's exit code, and the test could continue even if
the command exited with error.

Add a bit of linting to 'test_i18ngrep' to detect when data is fed to
its standard input and to error out with a "bug in the test script"
message.

Note that this change will also forbid cases where 'test_i18ngrep'
would legitimately read its standard input, e.g.

  - when its standard input is redirected from a file, or

  - when a git command's standard output is first written to an
    intermediate file, which is then preprocessed by a non-git command
    before the results are piped into 'test_i18ngrep'.

See two of the previous patches for the only such cases we had in our
test suite.  However, reliably preventing this antipattern is arguably
more important than supporting these cases, which can be worked around
by only minor inconveniences.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/test-lib-functions.sh | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 92ed02937..e381d50d0 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -719,6 +719,10 @@ test_i18ncmp () {
 # under GETTEXT_POISON this pretends that the command produced expected
 # results.
 test_i18ngrep () {
+	( read line ) &&
+	error "bug in the test script: data on test_i18ngrep's stdin;" \
+	      "perhaps a git command's output is piped into it?"
+
 	if test -n "$GETTEXT_POISON"
 	then
 	    : # pretend success
-- 
2.16.1.155.g5159265b1

[PATCH 09/10] t: make sure that 'test_i18ngrep' got enough parameters

[SZEDER Gábor]

Two of the previous patches in this series fixed two bogus
'test_i18ngrep' invocations that had neither a filename parameter not
anything piped into their standard input, yet both managed to remain
unnoticed for years.  A third similarly bogus invocation is currently
lurking in 'pu' for a couple of weeks now.

Try to catch similar mistakes in the future by ensuring that
'test_i18ngrep' has at least two parameters, not including an optional
'!' to negate the pattern.  Perform these checks after we made sure
that there is no data on the 'test_i18ngrep's standard input, so if
the filename parameter is missing because someone is piping a git
command's output into this function, then they would get the more
relevant error message.

Note that this is not quite perfect, as it doesn't account for any
'grep --options' given as parameters.  However, doing so would be far
too complicated, considering that patters can start with dashes as
well, and in the majority of the cases we don't use any such options
anyway.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---

About that third one in 'pu': it's test '3b-check: Avoid implicit
rename if involved as source on current side' introduced in commit
fcd649216 (directory rename detection: testcases to avoid taking
detection too far, 2018-01-05) in branch
'en/rename-directory-detection'.

  https://public-inbox.org/git/CAM0VKj=qhJQJ7uJWbBouSTYD0frA1zp1gwXzMVXuTiF+C6GH+g@mail.gmail.com/T/#u

 t/test-lib-functions.sh | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index e381d50d0..b543fd0e0 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -723,6 +723,12 @@ test_i18ngrep () {
 	error "bug in the test script: data on test_i18ngrep's stdin;" \
 	      "perhaps a git command's output is piped into it?"
 
+	if test $# -lt 2 ||
+	   { test "x!" = "x$1" && test $# -lt 3 ; }
+	then
+		error "bug in the test script: too few parameters to test_i18ngrep"
+	fi
+
 	if test -n "$GETTEXT_POISON"
 	then
 	    : # pretend success
-- 
2.16.1.155.g5159265b1

[PATCH 10/10] t: make 'test_i18ngrep' more informative on failure

[SZEDER Gábor]

When 'test_i18ngrep' can't find the expected pattern, it exits
completely silently; when its negated form does find the pattern that
shouldn't be there, it prints the matching line(s) but otherwise exits
without any error message.  This leaves the developer puzzled about
what could have gone wrong.

Make 'test_i18ngrep' more informative on failure by printing an error
message including the invoked 'grep' command and the contents of the
file it had to scan through.

Note that this "dump the scanned file" part is not quite perfect, as
it dumps only the file specified as the function's last positional
parameter, thus assuming that there is only a single file parameter.
I think that's a reasonable assumption to make, one that holds true in
the current code base.  And even if someone were to scan multiple
files at once in the future, the worst thing that could happen is that
the verbose error message won't include the contents of all those
files, only the last one.  Alas, we can't really do any better than
this, because checking whether the other positional parameters match a
filename can result in false positives: 't3400-rebase.sh' and
't3404-rebase-interactive.sh' contain one test each, where the
'test_i18ngrep's pattern verbatimely matches a file in the trash
directory.  Note that the absence of a file parameter is not an issue,
because the lint check added in the previous commit ensures that
'test_i18ngrep' never reads from its standard input, consequently
there must be a file parameter.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/test-lib-functions.sh | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index b543fd0e0..1f1d89d7a 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -731,14 +731,31 @@ test_i18ngrep () {
 
 	if test -n "$GETTEXT_POISON"
 	then
-	    : # pretend success
-	elif test "x!" = "x$1"
+		# pretend success
+		return 0
+	fi
+
+	if test "x!" = "x$1"
 	then
 		shift
-		! grep "$@"
+		! grep "$@" && return 0
+
+		echo >&2 "error: grep '! $@' did find a match in:"
 	else
-		grep "$@"
+		grep "$@" && return 0
+
+		echo >&2 "error: grep '$@' didn't find a match in:"
 	fi
+	(
+		eval "f=\"\${$#}\""
+		if test -s "$f"
+		then
+			cat >&2 "$f"
+		else
+			echo "<File '$f' is empty>"
+		fi
+	)
+	return 1
 }
 
 # Call any command "$@" but be more verbose about its
-- 
2.16.1.155.g5159265b1

Re: [PATCH 05/10] t5510: consolidate 'grep' and 'test_i18ngrep' patterns

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> One of the tests in 't5510-fetch.sh' checks the output of 'git fetch'
> using 'test_i18ngrep', and while doing so it prefilters the output
> with 'grep' before piping the result into 'test_i18ngrep'.
>
> This prefiltering is unnecessary, with the appropriate pattern
> 'test_i18ngrep' can do it all by itself.  Furthermore, piping data
> into 'test_i18ngrep' will interfere with the linting that will be
> added in a later patch.

It is very likely that the prefiltering "grep" will not even see
what it is looking for under GETTEXT_POISON build in the first
place, so this conversion is the right thing to do from that point
of view as well.



>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>  t/t5510-fetch.sh | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> index 668c54be4..3debc87d4 100755
> --- a/t/t5510-fetch.sh
> +++ b/t/t5510-fetch.sh
> @@ -222,12 +222,9 @@ test_expect_success 'fetch uses remote ref names to describe new refs' '
>  	(
>  		cd descriptive &&
>  		git fetch o 2>actual &&
> -		grep " -> refs/crazyheads/descriptive-branch$" actual |
> -		test_i18ngrep "new branch" &&
> -		grep " -> descriptive-tag$" actual |
> -		test_i18ngrep "new tag" &&
> -		grep " -> crazy$" actual |
> -		test_i18ngrep "new ref"
> +		test_i18ngrep "new branch.* -> refs/crazyheads/descriptive-branch$" actual &&
> +		test_i18ngrep "new tag.* -> descriptive-tag$" actual &&
> +		test_i18ngrep "new ref.* -> crazy$" actual
>  	) &&
>  	git checkout master
>  '

Re: [PATCH 07/10] t: move 'test_i18ncmp' and 'test_i18ngrep' to 'test-lib-functions.sh'

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> Both 'test_i18ncmp' and 'test_i18ngrep' helper functions are supposed
> to be called from our test scripts, so they should be in
> 'test-lib-functions.sh'.
>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>  t/test-lib-functions.sh | 26 ++++++++++++++++++++++++++
>  t/test-lib.sh           | 26 --------------------------
>  2 files changed, 26 insertions(+), 26 deletions(-)

Hmph.  I do not care too much either way, but I had an impression
that test-lib-functions.sh is meant to be more generic (i.e. those
who want can steal it from us and use it in their project without
dragging too much of the local convention we employ in this project)
than what is in test-lib.sh, which can heavily be specific to Git,
and I also had an impression that gettext-poison build is quite a
local convention we use in this project, not applicable to other
people.

>
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 1701fe2a0..92ed02937 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -705,6 +705,32 @@ test_cmp_bin() {
>  	cmp "$@"
>  }
>  
> +# Use this instead of test_cmp to compare files that contain expected and
> +# actual output from git commands that can be translated.  When running
> +# under GETTEXT_POISON this pretends that the command produced expected
> +# results.
> +test_i18ncmp () {
> +	test -n "$GETTEXT_POISON" || test_cmp "$@"
> +}
> +
> +# Use this instead of "grep expected-string actual" to see if the
> +# output from a git command that can be translated either contains an
> +# expected string, or does not contain an unwanted one.  When running
> +# under GETTEXT_POISON this pretends that the command produced expected
> +# results.
> +test_i18ngrep () {
> +	if test -n "$GETTEXT_POISON"
> +	then
> +	    : # pretend success
> +	elif test "x!" = "x$1"
> +	then
> +		shift
> +		! grep "$@"
> +	else
> +		grep "$@"
> +	fi
> +}
> +
>  # Call any command "$@" but be more verbose about its
>  # failure. This is handy for commands like "test" which do
>  # not output anything when they fail.
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 9a0a21f49..852b22c80 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1062,32 +1062,6 @@ else
>  	test_set_prereq C_LOCALE_OUTPUT
>  fi
>  
> -# Use this instead of test_cmp to compare files that contain expected and
> -# actual output from git commands that can be translated.  When running
> -# under GETTEXT_POISON this pretends that the command produced expected
> -# results.
> -test_i18ncmp () {
> -	test -n "$GETTEXT_POISON" || test_cmp "$@"
> -}
> -
> -# Use this instead of "grep expected-string actual" to see if the
> -# output from a git command that can be translated either contains an
> -# expected string, or does not contain an unwanted one.  When running
> -# under GETTEXT_POISON this pretends that the command produced expected
> -# results.
> -test_i18ngrep () {
> -	if test -n "$GETTEXT_POISON"
> -	then
> -	    : # pretend success
> -	elif test "x!" = "x$1"
> -	then
> -		shift
> -		! grep "$@"
> -	else
> -		grep "$@"
> -	fi
> -}
> -
>  test_lazy_prereq PIPE '
>  	# test whether the filesystem supports FIFOs
>  	test_have_prereq !MINGW,!CYGWIN &&

Re: [PATCH 01/10] t5541: add 'test_i18ngrep's missing filename parameter

[Jeff King]

On Fri, Jan 26, 2018 at 01:36:59PM +0100, SZEDER Gábor wrote:

> The test 'push --no-progress silences progress but not status' runs
> 'test_i18ngrep' without specifying a filename parameter.  This has
> remained unnoticed since its introduction in e304aeba2 (t5541: test
> more combinations of --progress, 2012-05-01), because that
> 'test_i18ngrep' is supposed to check that the given pattern is not
> present in its input, and of course it won't find that pattern if its
> input is empty, (as it comes from /dev/null).  This also means that
> this test could miss a potential breakage of 'git push --no-progress'.

Oof, embarrassing. Thanks for catching.

This and other errors make me wonder if test_i18ngrep ought to take an
explicit "-" for stdin, and error out if no file argument is given. That
may be overkill, though (and it's not like we wouldn't have the same
problem with regular "grep").

-Peff

Re: [PATCH 01/10] t5541: add 'test_i18ngrep's missing filename parameter

[Jeff King]

On Fri, Jan 26, 2018 at 01:23:03PM -0500, Jeff King wrote:

> On Fri, Jan 26, 2018 at 01:36:59PM +0100, SZEDER Gábor wrote:
> 
> > The test 'push --no-progress silences progress but not status' runs
> > 'test_i18ngrep' without specifying a filename parameter.  This has
> > remained unnoticed since its introduction in e304aeba2 (t5541: test
> > more combinations of --progress, 2012-05-01), because that
> > 'test_i18ngrep' is supposed to check that the given pattern is not
> > present in its input, and of course it won't find that pattern if its
> > input is empty, (as it comes from /dev/null).  This also means that
> > this test could miss a potential breakage of 'git push --no-progress'.
> 
> Oof, embarrassing. Thanks for catching.
> 
> This and other errors make me wonder if test_i18ngrep ought to take an
> explicit "-" for stdin, and error out if no file argument is given. That
> may be overkill, though (and it's not like we wouldn't have the same
> problem with regular "grep").

....and I really ought to start reading your entire sets of patches
before commenting on the early ones. :-/

-Peff

Re: [PATCH 08/10] t: forbid piping into 'test_i18ngrep'

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> See two of the previous patches for the only such cases we had in our
> test suite.  However, reliably preventing this antipattern is arguably
> more important than supporting these cases, which can be worked around
> by only minor inconveniences.

I am not sure if that inconveniences will be minor.  Is this too
contrived an example, for example?

  check () {
        pattern=$1 file=$2 script=./runme

        test_i18ngrep "$pattern" "$file" &&
        write_script "$script" &&
        test_expect_success "check $pattern" '
                "$script"
        '
  }

  check foo file <<-EOF
  ... test script comes here ...
  EOF


>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>  t/test-lib-functions.sh | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 92ed02937..e381d50d0 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -719,6 +719,10 @@ test_i18ncmp () {
>  # under GETTEXT_POISON this pretends that the command produced expected
>  # results.
>  test_i18ngrep () {
> +	( read line ) &&
> +	error "bug in the test script: data on test_i18ngrep's stdin;" \
> +	      "perhaps a git command's output is piped into it?"
> +
>  	if test -n "$GETTEXT_POISON"
>  	then
>  	    : # pretend success

Re: [PATCH 02/10] t5812: add 'test_i18ngrep's missing filename parameter

[Jeff King]

On Fri, Jan 26, 2018 at 01:37:00PM +0100, SZEDER Gábor wrote:

> The second 'test_i18ngrep' invocation in the test 'curl redirects
> respect whitelist' is missing its filename parameter.  This has
> remained unnoticed since its introduction in f4113cac0 (http: limit
> redirection to protocol-whitelist, 2015-09-22), because it would only
> cause the test to fail if Git was built with a sufficiently old
> libcurl version.  The test's two ||-chained 'test_i18ngrep'
> invocations are supposed to check that either one of the two patterns
> is present in 'git clone's error message.  As it happens, the first
> invocation covers the error message from any reasonably up-to-date
> libcurl, thus the second invocation, the one without the filename
> parameter, isn't executed at all.  Apparently no one has run the test
> suite's httpd tests with such an old libcurl in the last 2+ years, or
> at least they haven't bothered to notify us about the failed test.

Interesting find.

The "too old" curl is older than 7.19.4, which we actually fail to build
with since v2.12.0. So they probably did not even get as far as the
tests. ;)

> Fix this by consolidating the two patterns into a single extended
> regexp, eliminating the need for an ||-chained second 'test_i18ngrep'
> invocation.

OK. Once upon a time I think we had trouble with "grep -E", since some
older systems had only "egrep". But I see we've introduced some "grep
-E" invocations as far back as 2013 and nobody has complained, so it's
probably fine.

-Peff

Re: [PATCH 07/10] t: move 'test_i18ncmp' and 'test_i18ngrep' to 'test-lib-functions.sh'

[Jeff King]

On Fri, Jan 26, 2018 at 10:19:17AM -0800, Junio C Hamano wrote:

> SZEDER Gábor <szeder.dev@gmail.com> writes:
> 
> > Both 'test_i18ncmp' and 'test_i18ngrep' helper functions are supposed
> > to be called from our test scripts, so they should be in
> > 'test-lib-functions.sh'.
> >
> > Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> > ---
> >  t/test-lib-functions.sh | 26 ++++++++++++++++++++++++++
> >  t/test-lib.sh           | 26 --------------------------
> >  2 files changed, 26 insertions(+), 26 deletions(-)
> 
> Hmph.  I do not care too much either way, but I had an impression
> that test-lib-functions.sh is meant to be more generic (i.e. those
> who want can steal it from us and use it in their project without
> dragging too much of the local convention we employ in this project)
> than what is in test-lib.sh, which can heavily be specific to Git,
> and I also had an impression that gettext-poison build is quite a
> local convention we use in this project, not applicable to other
> people.

I had a similar notion, but I thought it was the other way around:
test-lib.sh was supposed to be the harness, and test-lib-functions.sh
was our own stuff. But I do not think we have really kept to that over
the years. TBH I have generally been confused by the distinction and
just use ctags to find the right source file. ;)

-Peff

Re: [PATCH 08/10] t: forbid piping into 'test_i18ngrep'

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> SZEDER Gábor <szeder.dev@gmail.com> writes:
>
>> See two of the previous patches for the only such cases we had in our
>> test suite.  However, reliably preventing this antipattern is arguably
>> more important than supporting these cases, which can be worked around
>> by only minor inconveniences.
>
> I am not sure if that inconveniences will be minor.  Is this too
> contrived an example, for example?
>
>   check () {
>         pattern=$1 file=$2 script=./runme
>
>         test_i18ngrep "$pattern" "$file" &&
>         write_script "$script" &&
>         test_expect_success "check $pattern" '
>                 "$script"
>         '
>   }
>
>   check foo file <<-EOF
>   ... test script comes here ...
>   EOF

Is there a case where test_i18ngrep (after your clean-ups in this
series up to 06/10) needs to read from more than one file?

I actually think that the kind of inconveniences we *can* work with,
without risking breakage to legitimate test, would be to allow and
require test_i18ngrep to name and read only from one file that
appears at the end of its command line.  IOW, instead of doing a
probing "read" that you cannot undo and break legitimate test, I
think it is OK to see if the last token names a file that is on the
filesystem, e.g.

	test_i18ngrep () {
		eval test -f \"\${$#}\" ||
		error "bug in the test sript: test_i18ngrep must" \
		      "name a file to read as the last token on the command line"
		...

Re: [PATCH 08/10] t: forbid piping into 'test_i18ngrep'

[Jeff King]

On Fri, Jan 26, 2018 at 01:37:06PM +0100, SZEDER Gábor wrote:

> When checking a git command's output with 'test_i18ngrep', it's
> tempting to conveniently pipe the git command's standard output into
> 'test_i18ngrep'.  Unfortunately, this is an anti-pattern, because it
> hides the git command's exit code, and the test could continue even if
> the command exited with error.
> 
> Add a bit of linting to 'test_i18ngrep' to detect when data is fed to
> its standard input and to error out with a "bug in the test script"
> message.
> 
> Note that this change will also forbid cases where 'test_i18ngrep'
> would legitimately read its standard input, e.g.
> 
>   - when its standard input is redirected from a file, or
> 
>   - when a git command's standard output is first written to an
>     intermediate file, which is then preprocessed by a non-git command
>     before the results are piped into 'test_i18ngrep'.
> 
> See two of the previous patches for the only such cases we had in our
> test suite.  However, reliably preventing this antipattern is arguably
> more important than supporting these cases, which can be worked around
> by only minor inconveniences.

The idea seems reasonable to me. Let's think about what the escape hatch
looks like to work around it if you need to.

I guess you've got:

  cat >file &&
  test_i18ngrep ... file

which is not too bad.

You've also got:

  test_i18ngrep ... -

though that relies on the underlying grep understanding "-" (which is in
POSIX, though with a rather vague "if the implementations supports it").
And it wouldn't work with the "read" test in this patch.

> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 92ed02937..e381d50d0 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -719,6 +719,10 @@ test_i18ncmp () {
>  # under GETTEXT_POISON this pretends that the command produced expected
>  # results.
>  test_i18ngrep () {
> +	( read line ) &&
> +	error "bug in the test script: data on test_i18ngrep's stdin;" \
> +	      "perhaps a git command's output is piped into it?"
> +

This seems kind of hacky compared to just seeing if there is a file
argument. But I suppose that is hard to do, since we just pass through
the arguments to grep.

Though looking at our test_18ngrep invocations, they are simple enough
that would just ask "are there two non-option arguments at the end of
the command line". The exception is "-e", but IMHO we could just drop
that. It serves no purpose unless you're trying to hide a "-" at the
start of your pattern, and in fact we used to ban it since sysv grep
didn't understand it (e.g., aadbe44f883).

-Peff

Re: [PATCH 08/10] t: forbid piping into 'test_i18ngrep'

[Jeff King]

On Fri, Jan 26, 2018 at 10:39:05AM -0800, Junio C Hamano wrote:

> Is there a case where test_i18ngrep (after your clean-ups in this
> series up to 06/10) needs to read from more than one file?
> 
> I actually think that the kind of inconveniences we *can* work with,
> without risking breakage to legitimate test, would be to allow and
> require test_i18ngrep to name and read only from one file that
> appears at the end of its command line.  IOW, instead of doing a
> probing "read" that you cannot undo and break legitimate test, I
> think it is OK to see if the last token names a file that is on the
> filesystem, e.g.
> 
> 	test_i18ngrep () {
> 		eval test -f \"\${$#}\" ||
> 		error "bug in the test sript: test_i18ngrep must" \
> 		      "name a file to read as the last token on the command line"

FWIW I like that much more than the weird "read" thing. It would also
disallow "-", but we could make an exception for that if we choose to.

-Peff

Re: [PATCH 09/10] t: make sure that 'test_i18ngrep' got enough parameters

[Jeff King]

On Fri, Jan 26, 2018 at 01:37:07PM +0100, SZEDER Gábor wrote:

> Two of the previous patches in this series fixed two bogus
> 'test_i18ngrep' invocations that had neither a filename parameter not
> anything piped into their standard input, yet both managed to remain
> unnoticed for years.  A third similarly bogus invocation is currently
> lurking in 'pu' for a couple of weeks now.

Hrm. At first I thought this was redundant with the stdin thing in the
previous one. But that is only checking "did you _try_ to use stdin".
This is checking "did you accidentally use stdin, which was empty".

But I think maybe it's the opposite; the other one is redundant with
this one, since it would be hard to convince grep to read from stdin
anyway with this.

> Note that this is not quite perfect, as it doesn't account for any
> 'grep --options' given as parameters.  However, doing so would be far
> too complicated, considering that patters can start with dashes as
> well, and in the majority of the cases we don't use any such options
> anyway.

Yeah, I agree this would help most cases, but not hurt in others. I do
think Junio's "see if the final argument is a file" approach seems like
it would cover us pretty accurately, though, without having to get too
intimate with grep options.

-Peff

Re: [PATCH 10/10] t: make 'test_i18ngrep' more informative on failure

[Jeff King]

On Fri, Jan 26, 2018 at 01:37:08PM +0100, SZEDER Gábor wrote:

> When 'test_i18ngrep' can't find the expected pattern, it exits
> completely silently; when its negated form does find the pattern that
> shouldn't be there, it prints the matching line(s) but otherwise exits
> without any error message.  This leaves the developer puzzled about
> what could have gone wrong.
> 
> Make 'test_i18ngrep' more informative on failure by printing an error
> message including the invoked 'grep' command and the contents of the
> file it had to scan through.

I think this is an improvement. You can also use "-x" to get a better
sense of exactly which command failed, but I have never been sad to
see more verbose output from failing tests by default. :)

> Note that this "dump the scanned file" part is not quite perfect, as
> it dumps only the file specified as the function's last positional
> parameter, thus assuming that there is only a single file parameter.
> I think that's a reasonable assumption to make, one that holds true in
> the current code base.  And even if someone were to scan multiple
> files at once in the future, the worst thing that could happen is that
> the verbose error message won't include the contents of all those
> files, only the last one.  Alas, we can't really do any better than
> this, because checking whether the other positional parameters match a
> filename can result in false positives: 't3400-rebase.sh' and
> 't3404-rebase-interactive.sh' contain one test each, where the
> 'test_i18ngrep's pattern verbatimely matches a file in the trash
> directory.  Note that the absence of a file parameter is not an issue,
> because the lint check added in the previous commit ensures that
> 'test_i18ngrep' never reads from its standard input, consequently
> there must be a file parameter.

Heh, this makes me support even more the "last one must be a file" rule
that Junio suggested for the linting check.

-Peff

Re: [PATCH 00/10] 'test_i18ngrep'-related fixes and improvements

[Jeff King]

On Fri, Jan 26, 2018 at 01:36:58PM +0100, SZEDER Gábor wrote:

> When 'test_i18ngrep' can't find the expected pattern, it exits
> completely silently; when its negated form does find the pattern that
> shouldn't be there, it prints the matching line(s) but otherwise exits
> without any error message.  This leaves the developer puzzled about
> what could have gone wrong.  Well, at least it left me puzzled...
> 
> Initially all I wanted to do was to make 'test_i18ngrep' more
> informative on failure, but then skeletons started to fall out of the
> closet^Wour test suite, and BAM! before I knew it I had 10 patches:

I know the feeling. :)

The series overall looks good to me. I left some comments on the
approach in the final few patches, but I could live with it as-is, or
with the approach Junio suggested.

-Peff

Re: [PATCH 08/10] t: forbid piping into 'test_i18ngrep'

[SZEDER Gábor]

On Fri, Jan 26, 2018 at 7:24 PM, Junio C Hamano <gitster@pobox.com> wrote:
> SZEDER Gábor <szeder.dev@gmail.com> writes:
>
>> See two of the previous patches for the only such cases we had in our
>> test suite.  However, reliably preventing this antipattern is arguably
>> more important than supporting these cases, which can be worked around
>> by only minor inconveniences.
>
> I am not sure if that inconveniences will be minor.  Is this too
> contrived an example, for example?
>
>   check () {
>         pattern=$1 file=$2 script=./runme
>
>         test_i18ngrep "$pattern" "$file" &&
>         write_script "$script" &&
>         test_expect_success "check $pattern" '
>                 "$script"
>         '
>   }
>
>   check foo file <<-EOF
>   ... test script comes here ...
>   EOF

With 'test_i18ngrep' outside the 'test_expect_success' block!?
Definitely too contrived. :)

OTOH, what about flipping the order of 'test_i18ngrep' and
'write_script'?  If we can't do that, then I wonder what the reason
might be that is not too contrived.


>>
>> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
>> ---
>>  t/test-lib-functions.sh | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
>> index 92ed02937..e381d50d0 100644
>> --- a/t/test-lib-functions.sh
>> +++ b/t/test-lib-functions.sh
>> @@ -719,6 +719,10 @@ test_i18ncmp () {
>>  # under GETTEXT_POISON this pretends that the command produced expected
>>  # results.
>>  test_i18ngrep () {
>> +     ( read line ) &&
>> +     error "bug in the test script: data on test_i18ngrep's stdin;" \
>> +           "perhaps a git command's output is piped into it?"
>> +
>>       if test -n "$GETTEXT_POISON"
>>       then
>>           : # pretend success

Re: [PATCH 07/10] t: move 'test_i18ncmp' and 'test_i18ngrep' to 'test-lib-functions.sh'

[SZEDER Gábor]

On Fri, Jan 26, 2018 at 7:19 PM, Junio C Hamano <gitster@pobox.com> wrote:
> SZEDER Gábor <szeder.dev@gmail.com> writes:
>
>> Both 'test_i18ncmp' and 'test_i18ngrep' helper functions are supposed
>> to be called from our test scripts, so they should be in
>> 'test-lib-functions.sh'.
>>
>> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
>> ---
>>  t/test-lib-functions.sh | 26 ++++++++++++++++++++++++++
>>  t/test-lib.sh           | 26 --------------------------
>>  2 files changed, 26 insertions(+), 26 deletions(-)
>
> Hmph.  I do not care too much either way, but I had an impression
> that test-lib-functions.sh is meant to be more generic (i.e. those
> who want can steal it from us and use it in their project without
> dragging too much of the local convention we employ in this project)
> than what is in test-lib.sh, which can heavily be specific to Git,
> and I also had an impression that gettext-poison build is quite a
> local convention we use in this project, not applicable to other
> people.

Well, there are a lot of Git-specific functions in
'test-lib-functions.sh' already:

test_set_index_version
test_tick
debug
test_commit
test_merge
test_chmod
test_unconfig
test_config{,_global}
test_cmp_rev
test_create_repo
test_ln_s_add
test_normalize_bool
nongit

Re: [PATCH 08/10] t: forbid piping into 'test_i18ngrep'

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> With 'test_i18ngrep' outside the 'test_expect_success' block!?
> Definitely too contrived. :)

Well, I think you got the idea.  The point is that test_i18ngrep may
not be the only thing that is redirected into, but can just be a
part of a block of commands, and the "probing" read will hurt.

Re: [PATCH 05/10] t5510: consolidate 'grep' and 'test_i18ngrep' patterns

[SZEDER Gábor]

On Fri, Jan 26, 2018 at 7:16 PM, Junio C Hamano <gitster@pobox.com> wrote:
> SZEDER Gábor <szeder.dev@gmail.com> writes:
>
>> One of the tests in 't5510-fetch.sh' checks the output of 'git fetch'
>> using 'test_i18ngrep', and while doing so it prefilters the output
>> with 'grep' before piping the result into 'test_i18ngrep'.
>>
>> This prefiltering is unnecessary, with the appropriate pattern
>> 'test_i18ngrep' can do it all by itself.  Furthermore, piping data
>> into 'test_i18ngrep' will interfere with the linting that will be
>> added in a later patch.
>
> It is very likely that the prefiltering "grep" will not even see
> what it is looking for under GETTEXT_POISON build in the first
> place, so this conversion is the right thing to do from that point
> of view as well.

No, GETTEXT_POISON only affects the translated messages, but those
'grep' invocations looked only at refnames and formatting.

This is the GETTEXT_POISON-ed output of 'git fetch' in that test
(probably will get line-wrapped):

# GETTEXT POISON # * # GETTEXT POISON # descriptive-branch ->
refs/crazyheads/descriptive-branch
 * # GETTEXT POISON # refs/others/crazy  -> crazy
 * # GETTEXT POISON # descriptive-tag    -> descriptive-tag


>> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
>> ---
>>  t/t5510-fetch.sh | 9 +++------
>>  1 file changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
>> index 668c54be4..3debc87d4 100755
>> --- a/t/t5510-fetch.sh
>> +++ b/t/t5510-fetch.sh
>> @@ -222,12 +222,9 @@ test_expect_success 'fetch uses remote ref names to describe new refs' '
>>       (
>>               cd descriptive &&
>>               git fetch o 2>actual &&
>> -             grep " -> refs/crazyheads/descriptive-branch$" actual |
>> -             test_i18ngrep "new branch" &&
>> -             grep " -> descriptive-tag$" actual |
>> -             test_i18ngrep "new tag" &&
>> -             grep " -> crazy$" actual |
>> -             test_i18ngrep "new ref"
>> +             test_i18ngrep "new branch.* -> refs/crazyheads/descriptive-branch$" actual &&
>> +             test_i18ngrep "new tag.* -> descriptive-tag$" actual &&
>> +             test_i18ngrep "new ref.* -> crazy$" actual
>>       ) &&
>>       git checkout master
>>  '

Re: [PATCH 05/10] t5510: consolidate 'grep' and 'test_i18ngrep' patterns

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> No, GETTEXT_POISON only affects the translated messages, but those
> 'grep' invocations looked only at refnames and formatting.

You are right for this specific case, but I was talking more from
general principle---running test_i18ngrep on an output from grep
should be flagged as anti-pattern (I recall vaguly finding an
instance not in too distant past).

Re: [PATCH 10/10] t: make 'test_i18ngrep' more informative on failure

[SZEDER Gábor]

On Fri, Jan 26, 2018 at 7:50 PM, Jeff King <peff@peff.net> wrote:
> On Fri, Jan 26, 2018 at 01:37:08PM +0100, SZEDER Gábor wrote:
>
>> When 'test_i18ngrep' can't find the expected pattern, it exits
>> completely silently; when its negated form does find the pattern that
>> shouldn't be there, it prints the matching line(s) but otherwise exits
>> without any error message.  This leaves the developer puzzled about
>> what could have gone wrong.
>>
>> Make 'test_i18ngrep' more informative on failure by printing an error
>> message including the invoked 'grep' command and the contents of the
>> file it had to scan through.
>
> I think this is an improvement. You can also use "-x" to get a better
> sense of exactly which command failed,

Yeah, I know...  but I have some issues with running tests with '-x'; I
suspect PEBKAC, but haven't yet got around to investigate.

Re: [PATCH 10/10] t: make 'test_i18ngrep' more informative on failure

[Jeff King]

On Fri, Jan 26, 2018 at 08:23:24PM +0100, SZEDER Gábor wrote:

> On Fri, Jan 26, 2018 at 7:50 PM, Jeff King <peff@peff.net> wrote:
> > On Fri, Jan 26, 2018 at 01:37:08PM +0100, SZEDER Gábor wrote:
> >
> >> When 'test_i18ngrep' can't find the expected pattern, it exits
> >> completely silently; when its negated form does find the pattern that
> >> shouldn't be there, it prints the matching line(s) but otherwise exits
> >> without any error message.  This leaves the developer puzzled about
> >> what could have gone wrong.
> >>
> >> Make 'test_i18ngrep' more informative on failure by printing an error
> >> message including the invoked 'grep' command and the contents of the
> >> file it had to scan through.
> >
> > I think this is an improvement. You can also use "-x" to get a better
> > sense of exactly which command failed,
> 
> Yeah, I know...  but I have some issues with running tests with '-x'; I
> suspect PEBKAC, but haven't yet got around to investigate.

Some tests absolutely fail with "-x", due to them caring about the
stderr output of shell functions. But with the BASH_XTRACEFD stuff, if
you run suite under bash it should al Just Work (and I recently added
TEST_SHELL_PATH to use bash just for the test suite without building all
of the scripts with it).

-Peff

Re: [PATCH 10/10] t: make 'test_i18ngrep' more informative on failure

[SZEDER Gábor]

On Fri, Jan 26, 2018 at 8:25 PM, Jeff King <peff@peff.net> wrote:
> On Fri, Jan 26, 2018 at 08:23:24PM +0100, SZEDER Gábor wrote:
>
>> On Fri, Jan 26, 2018 at 7:50 PM, Jeff King <peff@peff.net> wrote:

>> > You can also use "-x" to get a better
>> > sense of exactly which command failed,
>>
>> Yeah, I know...  but I have some issues with running tests with '-x'; I
>> suspect PEBKAC, but haven't yet got around to investigate.
>
> Some tests absolutely fail with "-x", due to them caring about the
> stderr output of shell functions. But with the BASH_XTRACEFD stuff, if
> you run suite under bash it should al Just Work (and I recently added
> TEST_SHELL_PATH to use bash just for the test suite without building all
> of the scripts with it).

Yeah, I knew about TEST_SHELL_PATH, but still:

  $ make -j4 TEST_SHELL_PATH=/bin/bash
  <...>
  $ cd t/
  $ for t in t[0-9][0-9][0-9][0-9]-*.sh ; do "./$t" -x ; done >/dev/null 2>&1
  $ grep '^failed [^0]$' test-results/*.counts |wc -l
  44

The worst offender is t0008-ignores with 208 tests failing with '-x'...
I suspect a setup test fails for some reason, and (most of) the other
failed tests are just fallout; haven't dared to look yet :)

Re: [PATCH 10/10] t: make 'test_i18ngrep' more informative on failure

[Jeff King]

On Fri, Jan 26, 2018 at 09:26:38PM +0100, SZEDER Gábor wrote:

> Yeah, I knew about TEST_SHELL_PATH, but still:
> 
>   $ make -j4 TEST_SHELL_PATH=/bin/bash
>   <...>
>   $ cd t/
>   $ for t in t[0-9][0-9][0-9][0-9]-*.sh ; do "./$t" -x ; done >/dev/null 2>&1
>   $ grep '^failed [^0]$' test-results/*.counts |wc -l
>   44
> 
> The worst offender is t0008-ignores with 208 tests failing with '-x'...
> I suspect a setup test fails for some reason, and (most of) the other
> failed tests are just fallout; haven't dared to look yet :)

You cannot run "./$t" and expect TEST_SHELL_PATH to kick in; that starts
the test with the #! header, which is always /bin/sh (we don't "build"
the test scripts like we do regular scripts).

You need to run either:

  - make TEST_SHELL_PATH=/bin/bash test

or

  - bash $t -x

There _is_ one exception where it sometimes works, which is if you use
--tee or --verbose-log, in which case the shell script has to re-exec
itself, in which case it always pulls the value from GIT-BUILD-OPTIONS
to re-exec.

-Peff

Re: [PATCH 09/10] t: make sure that 'test_i18ngrep' got enough parameters

[Eric Sunshine]

On Fri, Jan 26, 2018 at 7:37 AM, SZEDER Gábor <szeder.dev@gmail.com> wrote:
> Two of the previous patches in this series fixed two bogus
> 'test_i18ngrep' invocations that had neither a filename parameter not

s/not/nor/

> anything piped into their standard input, yet both managed to remain
> unnoticed for years.  A third similarly bogus invocation is currently
> lurking in 'pu' for a couple of weeks now.
>
> Try to catch similar mistakes in the future by ensuring that
> 'test_i18ngrep' has at least two parameters, not including an optional
> '!' to negate the pattern.  Perform these checks after we made sure
> that there is no data on the 'test_i18ngrep's standard input, so if
> the filename parameter is missing because someone is piping a git
> command's output into this function, then they would get the more
> relevant error message.
>
> Note that this is not quite perfect, as it doesn't account for any
> 'grep --options' given as parameters.  However, doing so would be far
> too complicated, considering that patters can start with dashes as
> well, and in the majority of the cases we don't use any such options
> anyway.
>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>

Re: [PATCH 02/10] t5812: add 'test_i18ngrep's missing filename parameter

[Simon Ruderich]

On Fri, Jan 26, 2018 at 01:37:00PM +0100, SZEDER Gábor wrote:
> [snip]
>
> diff --git a/t/t5812-proto-disable-http.sh b/t/t5812-proto-disable-http.sh
> index d911afd24..226a4920c 100755
> --- a/t/t5812-proto-disable-http.sh
> +++ b/t/t5812-proto-disable-http.sh
> @@ -21,8 +21,7 @@ test_expect_success 'curl redirects respect whitelist' '
>  			   GIT_SMART_HTTP=0 \
>  		git clone "$HTTPD_URL/ftp-redir/repo.git" 2>stderr &&
>  	{
> -		test_i18ngrep "ftp.*disabled" stderr ||
> -		test_i18ngrep "your curl version is too old"
> +		test_i18ngrep -E "(ftp.*disabled|your curl version is too old)" stderr
>  	}

I think we can drop the curly braces as well, as they were only
used to group the ||; leaving only:

> +	test_i18ngrep -E "(ftp.*disabled|your curl version is too old)" stderr

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

Re: [PATCH 02/10] t5812: add 'test_i18ngrep's missing filename parameter

[SZEDER Gábor]

On Fri, Jan 26, 2018 at 7:27 PM, Jeff King <peff@peff.net> wrote:
> On Fri, Jan 26, 2018 at 01:37:00PM +0100, SZEDER Gábor wrote:
>
>> The second 'test_i18ngrep' invocation in the test 'curl redirects
>> respect whitelist' is missing its filename parameter.  This has
>> remained unnoticed since its introduction in f4113cac0 (http: limit
>> redirection to protocol-whitelist, 2015-09-22), because it would only
>> cause the test to fail if Git was built with a sufficiently old
>> libcurl version.  The test's two ||-chained 'test_i18ngrep'
>> invocations are supposed to check that either one of the two patterns
>> is present in 'git clone's error message.  As it happens, the first
>> invocation covers the error message from any reasonably up-to-date
>> libcurl, thus the second invocation, the one without the filename
>> parameter, isn't executed at all.  Apparently no one has run the test
>> suite's httpd tests with such an old libcurl in the last 2+ years, or
>> at least they haven't bothered to notify us about the failed test.
>
> Interesting find.
>
> The "too old" curl is older than 7.19.4, which we actually fail to build
> with since v2.12.0. So they probably did not even get as far as the
> tests. ;)

Oh, OK, I was not aware of that.  The oldest non-maintenance release
with the missing filename parameter is v2.7.0, so that's still a 5
releases time frame to notice it.

Anyway, I'm preparing v2 of this series, and I'm not sure what to do
about this.

  - Should I simply drop the "your curl version is too old" pattern?  It
    would make sense, but it just doesn't feel quite right to remove it
    while the corresponding printf() is still there, even if it can't be
    triggered anymore.  However, cleaning up the curl version checks in
    http.c to remove this message is beyond the scope of this patch
    series.

  - Or leave it almost-as-is, only dropping the now unnecessary curly
    braces as Simon pointed out.  And perhaps a bit of update to the
    commit message.

I'd prefer the second option.

>> Fix this by consolidating the two patterns into a single extended
>> regexp, eliminating the need for an ||-chained second 'test_i18ngrep'
>> invocation.
>
> OK. Once upon a time I think we had trouble with "grep -E", since some
> older systems had only "egrep". But I see we've introduced some "grep
> -E" invocations as far back as 2013 and nobody has complained, so it's
> probably fine.

Yeah, first I went with the more traditional "\(this\|that\)" pattern,
but then noticed that 'grep -E' is already used in a couple of places,
and picked the format that uses less escape characters.

Re: [PATCH 02/10] t5812: add 'test_i18ngrep's missing filename parameter

[Jeff King]

On Wed, Feb 07, 2018 at 02:53:17PM +0100, SZEDER Gábor wrote:

> > The "too old" curl is older than 7.19.4, which we actually fail to build
> > with since v2.12.0. So they probably did not even get as far as the
> > tests. ;)
> 
> Oh, OK, I was not aware of that.  The oldest non-maintenance release
> with the missing filename parameter is v2.7.0, so that's still a 5
> releases time frame to notice it.

Actually, I'm wrong. It looks like we did finally fix it in f18777ba6e
(http: fix handling of missing CURLPROTO_*, 2017-08-11), which is in
v2.15. So:

> Anyway, I'm preparing v2 of this series, and I'm not sure what to do
> about this.
> 
>   - Should I simply drop the "your curl version is too old" pattern?  It
>     would make sense, but it just doesn't feel quite right to remove it
>     while the corresponding printf() is still there, even if it can't be
>     triggered anymore.  However, cleaning up the curl version checks in
>     http.c to remove this message is beyond the scope of this patch
>     series.
> 
>   - Or leave it almost-as-is, only dropping the now unnecessary curly
>     braces as Simon pointed out.  And perhaps a bit of update to the
>     commit message.
> 
> I'd prefer the second option.

Yeah, I think just leave it as-is. Thanks.

-Peff

[PATCH v2 0/9] 'test_i18ngrep'-related fixes and improvements

[SZEDER Gábor]

This is the second version of 'sg/test-i18ngrep'.

To recap, this patch series fixes a couple of bogus 'test_i18ngrep'
invocations (patches 1-4), tries to prevent similar bugs in the future
(patch 8), teaches 'test_i18ngrep' to be more informative on failure
(patch 9), and a bit of cleanups in between (patches 5-7).

Changes since the previous version [1]:

  - Use Junio's "last parameter must be file" suggestion instead of
    trying to read stdin in patch 8.
  - Squashed together the patches validating 'test_i18ngrep's
    parameters (patches 8 and 9), in the hope that this way I can
    better explain that the two checks are not redundant but
    complement each other.
  - Followed Simon's suggestion and dropped the now unnecessary curly
    brackets in patch 2.
  - Dropped a subshell in the last patch.  I initially used it to
    prevent the variable $f from leaking into the tests, since we
    can't use the 'local' keyword (yet), but other test helper
    function don't seem to care.
  - Fixed the placements of single quotes and '!' in error messages
    and redirected one more error message to stderr in the last patch.
  - Fixed a couple of typos in commit messages (the one Eric pointed
    out, but later noticed maybe 2-3 more).


[1] - https://public-inbox.org/git/20180126123708.21722-1-szeder.dev@gmail.com/T/


SZEDER Gábor (9):
  t5541: add 'test_i18ngrep's missing filename parameter
  t5812: add 'test_i18ngrep's missing filename parameter
  t6022: don't run 'git merge' upstream of a pipe
  t4001: don't run 'git status' upstream of a pipe
  t5510: consolidate 'grep' and 'test_i18ngrep' patterns
  t5536: let 'test_i18ngrep' read the file without redirection
  t: move 'test_i18ncmp' and 'test_i18ngrep' to 'test-lib-functions.sh'
  t: validate 'test_i18ngrep's parameters
  t: make 'test_i18ngrep' more informative on failure

 t/t4001-diff-rename.sh        | 11 ++++++---
 t/t5510-fetch.sh              |  9 +++-----
 t/t5536-fetch-conflicts.sh    |  2 +-
 t/t5541-http-push-smart.sh    |  2 +-
 t/t5812-proto-disable-http.sh |  5 +---
 t/t6022-merge-rename.sh       |  6 +++--
 t/test-lib-functions.sh       | 54 +++++++++++++++++++++++++++++++++++++++++++
 t/test-lib.sh                 | 26 ---------------------
 8 files changed, 72 insertions(+), 43 deletions(-)

-- 
2.16.1.158.ge6451079d


diff --git a/t/t5812-proto-disable-http.sh b/t/t5812-proto-disable-http.sh
index 226a4920cd..872788ac8c 100755
--- a/t/t5812-proto-disable-http.sh
+++ b/t/t5812-proto-disable-http.sh
@@ -20,9 +20,7 @@ test_expect_success 'curl redirects respect whitelist' '
 	test_must_fail env GIT_ALLOW_PROTOCOL=http:https \
 			   GIT_SMART_HTTP=0 \
 		git clone "$HTTPD_URL/ftp-redir/repo.git" 2>stderr &&
-	{
-		test_i18ngrep -E "(ftp.*disabled|your curl version is too old)" stderr
-	}
+	test_i18ngrep -E "(ftp.*disabled|your curl version is too old)" stderr
 '
 
 test_expect_success 'curl limits redirects' '
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 1f1d89d7ad..d936ecc0a5 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -719,9 +719,11 @@ test_i18ncmp () {
 # under GETTEXT_POISON this pretends that the command produced expected
 # results.
 test_i18ngrep () {
-	( read line ) &&
-	error "bug in the test script: data on test_i18ngrep's stdin;" \
-	      "perhaps a git command's output is piped into it?"
+	eval "last_arg=\"\${$#}\""
+
+	test -f "$last_arg" ||
+	error "bug in the test script: test_i18ngrep requires a file" \
+	      "to read as the last parameter"
 
 	if test $# -lt 2 ||
 	   { test "x!" = "x$1" && test $# -lt 3 ; }
@@ -740,21 +742,20 @@ test_i18ngrep () {
 		shift
 		! grep "$@" && return 0
 
-		echo >&2 "error: grep '! $@' did find a match in:"
+		echo >&2 "error: '! grep $@' did find a match in:"
 	else
 		grep "$@" && return 0
 
-		echo >&2 "error: grep '$@' didn't find a match in:"
+		echo >&2 "error: 'grep $@' didn't find a match in:"
 	fi
-	(
-		eval "f=\"\${$#}\""
-		if test -s "$f"
-		then
-			cat >&2 "$f"
-		else
-			echo "<File '$f' is empty>"
-		fi
-	)
+
+	if test -s "$last_arg"
+	then
+		cat >&2 "$last_arg"
+	else
+		echo >&2 "<File '$last_arg' is empty>"
+	fi
+
 	return 1
 }
 

[PATCH v2 1/9] t5541: add 'test_i18ngrep's missing filename parameter

[SZEDER Gábor]

The test 'push --no-progress silences progress but not status' runs
'test_i18ngrep' without specifying a filename parameter.  This has
remained unnoticed since its introduction in e304aeba2 (t5541: test
more combinations of --progress, 2012-05-01), because that
'test_i18ngrep' is supposed to check that the given pattern is not
present in its input, and of course it won't find that pattern if its
input is empty (as it comes from /dev/null).  This also means that
this test could miss a potential breakage of 'git push --no-progress'.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t5541-http-push-smart.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
index d38bf32470..21340e89c9 100755
--- a/t/t5541-http-push-smart.sh
+++ b/t/t5541-http-push-smart.sh
@@ -234,7 +234,7 @@ test_expect_success TTY 'push --no-progress silences progress but not status' '
 	test_commit no-progress &&
 	test_terminal git push --no-progress >output 2>&1 &&
 	test_i18ngrep "^To http" output &&
-	test_i18ngrep ! "^Writing objects"
+	test_i18ngrep ! "^Writing objects" output
 '
 
 test_expect_success 'push --progress shows progress to non-tty' '
-- 
2.16.1.158.ge6451079d

[PATCH v2 2/9] t5812: add 'test_i18ngrep's missing filename parameter

[SZEDER Gábor]

The second 'test_i18ngrep' invocation in the test 'curl redirects
respect whitelist' is missing its filename parameter.  This has
remained unnoticed since its introduction in f4113cac0 (http: limit
redirection to protocol-whitelist, 2015-09-22), because it would only
cause the test to fail if Git was built with a sufficiently old
libcurl version.  The test's two ||-chained 'test_i18ngrep'
invocations are supposed to check that either one of the two patterns
is present in 'git clone's error message.  As it happens, the first
invocation covers the error message from any reasonably up-to-date
libcurl, thus the second invocation, the one without the filename
parameter, isn't executed at all.  Apparently no one has run the test
suite's httpd tests with such an old libcurl in the last 2+ years, or
at least they haven't bothered to notify us about the failed test.

Fix this by consolidating the two patterns into a single extended
regexp, eliminating the need for an ||-chained second 'test_i18ngrep'
invocation.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t5812-proto-disable-http.sh | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/t/t5812-proto-disable-http.sh b/t/t5812-proto-disable-http.sh
index d911afd24c..872788ac8c 100755
--- a/t/t5812-proto-disable-http.sh
+++ b/t/t5812-proto-disable-http.sh
@@ -20,10 +20,7 @@ test_expect_success 'curl redirects respect whitelist' '
 	test_must_fail env GIT_ALLOW_PROTOCOL=http:https \
 			   GIT_SMART_HTTP=0 \
 		git clone "$HTTPD_URL/ftp-redir/repo.git" 2>stderr &&
-	{
-		test_i18ngrep "ftp.*disabled" stderr ||
-		test_i18ngrep "your curl version is too old"
-	}
+	test_i18ngrep -E "(ftp.*disabled|your curl version is too old)" stderr
 '
 
 test_expect_success 'curl limits redirects' '
-- 
2.16.1.158.ge6451079d

[PATCH v2 3/9] t6022: don't run 'git merge' upstream of a pipe

[SZEDER Gábor]

The primary purpose of 't6022-merge-rename.sh' is to test 'git merge',
but one of the tests runs it upstream of a pipe, hiding its exit code.
Consequently, the test could continue even if 'git merge' exited with
error.

Use an intermediate file between 'git merge' and 'test_i18ngrep' to
catch a potential failure of the former.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t6022-merge-rename.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/t/t6022-merge-rename.sh b/t/t6022-merge-rename.sh
index 05ebba7afa..c01f721f13 100755
--- a/t/t6022-merge-rename.sh
+++ b/t/t6022-merge-rename.sh
@@ -242,10 +242,12 @@ test_expect_success 'merge of identical changes in a renamed file' '
 	rm -f A M N &&
 	git reset --hard &&
 	git checkout change+rename &&
-	GIT_MERGE_VERBOSITY=3 git merge change | test_i18ngrep "^Skipped B" &&
+	GIT_MERGE_VERBOSITY=3 git merge change >out &&
+	test_i18ngrep "^Skipped B" out &&
 	git reset --hard HEAD^ &&
 	git checkout change &&
-	GIT_MERGE_VERBOSITY=3 git merge change+rename | test_i18ngrep "^Skipped B"
+	GIT_MERGE_VERBOSITY=3 git merge change+rename >out &&
+	test_i18ngrep "^Skipped B" out
 '
 
 test_expect_success 'setup for rename + d/f conflicts' '
-- 
2.16.1.158.ge6451079d

[PATCH v2 4/9] t4001: don't run 'git status' upstream of a pipe

[SZEDER Gábor]

The primary purpose of three tests in 't4001-diff-rename.sh' is to
check rename detection in 'git status', but all three do so by running
'git status' upstream of a pipe, hiding its exit code.  Consequently,
the test could continue even if 'git status' exited with error.

Use an intermediate file between 'git status' and 'test_i18ngrep' to
catch a potential failure of the former.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t4001-diff-rename.sh | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/t/t4001-diff-rename.sh b/t/t4001-diff-rename.sh
index eadf4f6244..a07816d560 100755
--- a/t/t4001-diff-rename.sh
+++ b/t/t4001-diff-rename.sh
@@ -134,11 +134,15 @@ test_expect_success 'favour same basenames over different ones' '
 	git rm path1 &&
 	mkdir subdir &&
 	git mv another-path subdir/path1 &&
-	git status | test_i18ngrep "renamed: .*path1 -> subdir/path1"'
+	git status >out &&
+	test_i18ngrep "renamed: .*path1 -> subdir/path1" out
+'
 
 test_expect_success 'favour same basenames even with minor differences' '
 	git show HEAD:path1 | sed "s/15/16/" > subdir/path1 &&
-	git status | test_i18ngrep "renamed: .*path1 -> subdir/path1"'
+	git status >out &&
+	test_i18ngrep "renamed: .*path1 -> subdir/path1" out
+'
 
 test_expect_success 'two files with same basename and same content' '
 	git reset --hard &&
@@ -148,7 +152,8 @@ test_expect_success 'two files with same basename and same content' '
 	git add dir &&
 	git commit -m 2 &&
 	git mv dir other-dir &&
-	git status | test_i18ngrep "renamed: .*dir/A/file -> other-dir/A/file"
+	git status >out &&
+	test_i18ngrep "renamed: .*dir/A/file -> other-dir/A/file" out
 '
 
 test_expect_success 'setup for many rename source candidates' '
-- 
2.16.1.158.ge6451079d

[PATCH v2 5/9] t5510: consolidate 'grep' and 'test_i18ngrep' patterns

[SZEDER Gábor]

One of the tests in 't5510-fetch.sh' checks the output of 'git fetch'
using 'test_i18ngrep', and while doing so it prefilters the output
with 'grep' before piping the result into 'test_i18ngrep'.

This prefiltering is unnecessary, with the appropriate pattern
'test_i18ngrep' can do it all by itself.  Furthermore, piping data
into 'test_i18ngrep' will interfere with the linting that will be
added in a later patch.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t5510-fetch.sh | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 668c54be41..3debc87d4a 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -222,12 +222,9 @@ test_expect_success 'fetch uses remote ref names to describe new refs' '
 	(
 		cd descriptive &&
 		git fetch o 2>actual &&
-		grep " -> refs/crazyheads/descriptive-branch$" actual |
-		test_i18ngrep "new branch" &&
-		grep " -> descriptive-tag$" actual |
-		test_i18ngrep "new tag" &&
-		grep " -> crazy$" actual |
-		test_i18ngrep "new ref"
+		test_i18ngrep "new branch.* -> refs/crazyheads/descriptive-branch$" actual &&
+		test_i18ngrep "new tag.* -> descriptive-tag$" actual &&
+		test_i18ngrep "new ref.* -> crazy$" actual
 	) &&
 	git checkout master
 '
-- 
2.16.1.158.ge6451079d

[PATCH v2 6/9] t5536: let 'test_i18ngrep' read the file without redirection

[SZEDER Gábor]

Redirecting 'test_i18ngrep's standard input from a file will interfere
with the linting that will be added in a later patch.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t5536-fetch-conflicts.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5536-fetch-conflicts.sh b/t/t5536-fetch-conflicts.sh
index 2e42cf3316..644736b8a3 100755
--- a/t/t5536-fetch-conflicts.sh
+++ b/t/t5536-fetch-conflicts.sh
@@ -22,7 +22,7 @@ verify_stderr () {
 	cat >expected &&
 	# We're not interested in the error
 	# "fatal: The remote end hung up unexpectedly":
-	test_i18ngrep -E '^(fatal|warning):' <error | grep -v 'hung up' >actual | sort &&
+	test_i18ngrep -E '^(fatal|warning):' error | grep -v 'hung up' >actual | sort &&
 	test_i18ncmp expected actual
 }
 
-- 
2.16.1.158.ge6451079d

[PATCH v2 7/9] t: move 'test_i18ncmp' and 'test_i18ngrep' to 'test-lib-functions.sh'

[SZEDER Gábor]

Both 'test_i18ncmp' and 'test_i18ngrep' helper functions are supposed
to be called from our test scripts, so they should be in
'test-lib-functions.sh'.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/test-lib-functions.sh | 26 ++++++++++++++++++++++++++
 t/test-lib.sh           | 26 --------------------------
 2 files changed, 26 insertions(+), 26 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 1701fe2a06..92ed029371 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -705,6 +705,32 @@ test_cmp_bin() {
 	cmp "$@"
 }
 
+# Use this instead of test_cmp to compare files that contain expected and
+# actual output from git commands that can be translated.  When running
+# under GETTEXT_POISON this pretends that the command produced expected
+# results.
+test_i18ncmp () {
+	test -n "$GETTEXT_POISON" || test_cmp "$@"
+}
+
+# Use this instead of "grep expected-string actual" to see if the
+# output from a git command that can be translated either contains an
+# expected string, or does not contain an unwanted one.  When running
+# under GETTEXT_POISON this pretends that the command produced expected
+# results.
+test_i18ngrep () {
+	if test -n "$GETTEXT_POISON"
+	then
+	    : # pretend success
+	elif test "x!" = "x$1"
+	then
+		shift
+		! grep "$@"
+	else
+		grep "$@"
+	fi
+}
+
 # Call any command "$@" but be more verbose about its
 # failure. This is handy for commands like "test" which do
 # not output anything when they fail.
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 9a0a21f49a..852b22c80a 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1062,32 +1062,6 @@ else
 	test_set_prereq C_LOCALE_OUTPUT
 fi
 
-# Use this instead of test_cmp to compare files that contain expected and
-# actual output from git commands that can be translated.  When running
-# under GETTEXT_POISON this pretends that the command produced expected
-# results.
-test_i18ncmp () {
-	test -n "$GETTEXT_POISON" || test_cmp "$@"
-}
-
-# Use this instead of "grep expected-string actual" to see if the
-# output from a git command that can be translated either contains an
-# expected string, or does not contain an unwanted one.  When running
-# under GETTEXT_POISON this pretends that the command produced expected
-# results.
-test_i18ngrep () {
-	if test -n "$GETTEXT_POISON"
-	then
-	    : # pretend success
-	elif test "x!" = "x$1"
-	then
-		shift
-		! grep "$@"
-	else
-		grep "$@"
-	fi
-}
-
 test_lazy_prereq PIPE '
 	# test whether the filesystem supports FIFOs
 	test_have_prereq !MINGW,!CYGWIN &&
-- 
2.16.1.158.ge6451079d

[PATCH v2 8/9] t: validate 'test_i18ngrep's parameters

[SZEDER Gábor]

Some of the previous patches in this series fixed bogus
'test_i18ngrep' invocations:

  - Two invocations where the tested git command's standard output is
    directly piped into 'test_i18ngrep'.  While convenient, this is an
    antipattern, because the pipe hides the git command's exit code,
    and the test could continue even if the command exited with error.

  - Two invocations that had neither a filename parameter nor anything
    piped into their standard input, yet both managed to remain
    unnoticed for years.  A third similarly bogus invocation is
    currently lurking in 'pu' for a couple of weeks now.

Prevent similar mistakes in the future by validating 'test_i18ngrep's
parameters requiring that

  - The last parameter names an existing file to be read, effectively
    forbiding piping into 'test_i18ngrep'.

    Note that this change will also forbid cases where 'test_i18ngrep'
    would legitimately read its standard input, e.g. when its standard
    input is redirected from a file, or when a git command's standard
    output is first written to an intermediate file, which is then
    preprocessed by a non-git command before the results are piped
    into 'test_i18ngrep'.  See two of the previous patches for the
    only such cases we had in our test suite.  However, reliably
    preventing the piping antipattern is arguably more important than
    supporting these cases, which can be easily worked around by
    opening the file directly or using an intermediate file anyway.

  - There are at least two parameters, not including the optional '!'
    to negate the pattern.  This ought to catch corner cases when
    'test_i18ngrep' looks for the name of an existing file on its
    standard input; the above check would miss this case becase the
    filename as pattern would be the last parameter.

    Note that this is not quite perfect, as it doesn't account for any
    'grep --options' given as parameters.  However, doing so would be
    far too complicated, considering that patterns can start with
    dashes as well, and in the majority of the cases we don't use any
    such options anyway.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/test-lib-functions.sh | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 92ed029371..a1676e0386 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -719,6 +719,18 @@ test_i18ncmp () {
 # under GETTEXT_POISON this pretends that the command produced expected
 # results.
 test_i18ngrep () {
+	eval "last_arg=\"\${$#}\""
+
+	test -f "$last_arg" ||
+	error "bug in the test script: test_i18ngrep requires a file" \
+	      "to read as the last parameter"
+
+	if test $# -lt 2 ||
+	   { test "x!" = "x$1" && test $# -lt 3 ; }
+	then
+		error "bug in the test script: too few parameters to test_i18ngrep"
+	fi
+
 	if test -n "$GETTEXT_POISON"
 	then
 	    : # pretend success
-- 
2.16.1.158.ge6451079d

[PATCH v2 9/9] t: make 'test_i18ngrep' more informative on failure

[SZEDER Gábor]

When 'test_i18ngrep' can't find the expected pattern, it exits
completely silently; when its negated form does find the pattern that
shouldn't be there, it prints the matching line(s) but otherwise exits
without any error message.  This leaves the developer puzzled about
what could have gone wrong.

Make 'test_i18ngrep' more informative on failure by printing an error
message including the invoked 'grep' command and the contents of the
file it had to scan through.

Note that this "dump the scanned file" part is not quite perfect, as
it dumps only the file specified as the function's last positional
parameter, thus assuming that there is only a single file parameter.
I think that's a reasonable assumption to make, one that holds true in
the current code base.  And even if someone were to scan multiple
files at once in the future, the worst thing that could happen is that
the verbose error message won't include the contents of all those
files, only the last one.  Alas, we can't really do any better than
this, because checking whether the other positional parameters match a
filename can result in false positives: 't3400-rebase.sh' and
't3404-rebase-interactive.sh' contain one test each, where the
'test_i18ngrep's pattern verbatimly matches a file in the trash
directory.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/test-lib-functions.sh | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index a1676e0386..d936ecc0a5 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -733,14 +733,30 @@ test_i18ngrep () {
 
 	if test -n "$GETTEXT_POISON"
 	then
-	    : # pretend success
-	elif test "x!" = "x$1"
+		# pretend success
+		return 0
+	fi
+
+	if test "x!" = "x$1"
 	then
 		shift
-		! grep "$@"
+		! grep "$@" && return 0
+
+		echo >&2 "error: '! grep $@' did find a match in:"
 	else
-		grep "$@"
+		grep "$@" && return 0
+
+		echo >&2 "error: 'grep $@' didn't find a match in:"
 	fi
+
+	if test -s "$last_arg"
+	then
+		cat >&2 "$last_arg"
+	else
+		echo >&2 "<File '$last_arg' is empty>"
+	fi
+
+	return 1
 }
 
 # Call any command "$@" but be more verbose about its
-- 
2.16.1.158.ge6451079d

Re: [PATCH v2 8/9] t: validate 'test_i18ngrep's parameters

[Jeff King]

On Thu, Feb 08, 2018 at 04:56:55PM +0100, SZEDER Gábor wrote:

> Prevent similar mistakes in the future by validating 'test_i18ngrep's
> parameters requiring that
> 
>   - The last parameter names an existing file to be read, effectively
>     forbiding piping into 'test_i18ngrep'.

s/forbiding/forbidding/

>     Note that this change will also forbid cases where 'test_i18ngrep'
>     would legitimately read its standard input, e.g. when its standard
>     input is redirected from a file, or when a git command's standard
>     output is first written to an intermediate file, which is then
>     preprocessed by a non-git command before the results are piped
>     into 'test_i18ngrep'.  See two of the previous patches for the
>     only such cases we had in our test suite.  However, reliably
>     preventing the piping antipattern is arguably more important than
>     supporting these cases, which can be easily worked around by
>     opening the file directly or using an intermediate file anyway.
> 
>   - There are at least two parameters, not including the optional '!'
>     to negate the pattern.  This ought to catch corner cases when
>     'test_i18ngrep' looks for the name of an existing file on its
>     standard input; the above check would miss this case becase the
>     filename as pattern would be the last parameter.
> 
>     Note that this is not quite perfect, as it doesn't account for any
>     'grep --options' given as parameters.  However, doing so would be
>     far too complicated, considering that patterns can start with
>     dashes as well, and in the majority of the cases we don't use any
>     such options anyway.

And most importantly, we never err on the side of complaining
unnecessarily. So our safety might not kick in, but as long as it kicks
in most of the time, we're fine.

I like this approach much better than the previous round.

> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 92ed029371..a1676e0386 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -719,6 +719,18 @@ test_i18ncmp () {
>  # under GETTEXT_POISON this pretends that the command produced expected
>  # results.
>  test_i18ngrep () {
> +	eval "last_arg=\"\${$#}\""

These embedded double-quotes are unnecessary, I think, because it's a
variable assignment: E.g.:

  set -- one two 'foo bar'
  eval "last_arg=\${$#}"
  echo $last_arg

should produce "foo bar".

Usually not a big deal, but because of the extra quoting it may make the
whole thing a bit more readable to drop them.

-Peff

Re: [PATCH v2 0/9] 'test_i18ngrep'-related fixes and improvements

[Jeff King]

On Thu, Feb 08, 2018 at 04:56:47PM +0100, SZEDER Gábor wrote:

> This is the second version of 'sg/test-i18ngrep'.
> 
> To recap, this patch series fixes a couple of bogus 'test_i18ngrep'
> invocations (patches 1-4), tries to prevent similar bugs in the future
> (patch 8), teaches 'test_i18ngrep' to be more informative on failure
> (patch 9), and a bit of cleanups in between (patches 5-7).
> 
> Changes since the previous version [1]:
> [...]

This round looks good to me. I left a very minor comment on patch 8, but
otherwise didn't find anything wrong. Thanks.

-Peff