From 8cdef01c4282e6006b785a0222589ccd2b65a187 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?SZEDER=20G=C3=A1bor?= Date: Thu, 8 Feb 2018 16:56:48 +0100 Subject: t5541: add 'test_i18ngrep's missing filename parameter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Jeff King Signed-off-by: Junio C Hamano --- 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' ' -- cgit v1.2.1 From a4ca4553e08ab5626f744171c1d2d7fbfed11e8b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?SZEDER=20G=C3=A1bor?= Date: Thu, 8 Feb 2018 16:56:49 +0100 Subject: t5812: add 'test_i18ngrep's missing filename parameter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Jeff King Signed-off-by: Junio C Hamano --- 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' ' -- cgit v1.2.1 From cc04adc2d0514f0f666b7e123ba2022b1a20dcde Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?SZEDER=20G=C3=A1bor?= Date: Thu, 8 Feb 2018 16:56:50 +0100 Subject: t6022: don't run 'git merge' upstream of a pipe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Jeff King Signed-off-by: Junio C Hamano --- 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' ' -- cgit v1.2.1 From 3b85ec34b8448d0311ef20e225c76df11c006008 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?SZEDER=20G=C3=A1bor?= Date: Thu, 8 Feb 2018 16:56:51 +0100 Subject: t4001: don't run 'git status' upstream of a pipe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Jeff King Signed-off-by: Junio C Hamano --- 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' ' -- cgit v1.2.1 From 927c1a643a9320368a95046f855371b0ce473263 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?SZEDER=20G=C3=A1bor?= Date: Thu, 8 Feb 2018 16:56:52 +0100 Subject: t5510: consolidate 'grep' and 'test_i18ngrep' patterns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Jeff King Signed-off-by: Junio C Hamano --- 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 ' -- cgit v1.2.1 From 93b4b0313c8d9d3600fc27b7023dab0d9d9be739 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?SZEDER=20G=C3=A1bor?= Date: Thu, 8 Feb 2018 16:56:53 +0100 Subject: t5536: let 'test_i18ngrep' read the file without redirection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Jeff King Signed-off-by: Junio C Hamano --- 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):' actual | sort && + test_i18ngrep -E '^(fatal|warning):' error | grep -v 'hung up' >actual | sort && test_i18ncmp expected actual } -- cgit v1.2.1 From 0f59128f7bb6ce20889156efd101d1e059377eaa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?SZEDER=20G=C3=A1bor?= Date: Thu, 8 Feb 2018 16:56:54 +0100 Subject: t: move 'test_i18ncmp' and 'test_i18ngrep' to 'test-lib-functions.sh' MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Jeff King Signed-off-by: Junio C Hamano --- 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 && -- cgit v1.2.1 From fd29d7b9d767296e0f8fbd3f7def735424fdbb30 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?SZEDER=20G=C3=A1bor?= Date: Thu, 8 Feb 2018 16:56:55 +0100 Subject: t: validate 'test_i18ngrep's parameters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 forbidding 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 Reviewed-by: Jeff King Signed-off-by: Junio C Hamano --- 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..64f793e3d7 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 -- cgit v1.2.1 From 63b1a175ee284f37c009e0afd7ee88a7c04ca515 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?SZEDER=20G=C3=A1bor?= Date: Thu, 8 Feb 2018 16:56:56 +0100 Subject: t: make 'test_i18ngrep' more informative on failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Jeff King Signed-off-by: Junio C Hamano --- 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 64f793e3d7..1de31fae7f 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 "" + fi + + return 1 } # Call any command "$@" but be more verbose about its -- cgit v1.2.1