{"thread":{"id":"47699","subject":"[PATCH 00/10] 'test_i18ngrep'-related fixes and improvements","startedAt":"2018-01-26T12:37:29Z","lastAt":"2018-02-08T16:36:53Z","messageCount":49,"participants":["SZEDER Gábor","Junio C Hamano","Jeff King","Eric Sunshine","Simon Ruderich"],"isPatch":true,"patchVersion":1,"patchTotal":10},"messages":[{"id":"337465","messageId":"20180126123708.21722-1-szeder.dev@gmail.com","threadId":"47699","inReplyTo":null,"subject":"[PATCH 00/10] 'test_i18ngrep'-related fixes and improvements","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-01-26T12:36:58Z","receivedAt":"2018-01-26T12:37:29Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"When 'test_i18ngrep' can't find the expected pattern, it exits\ncompletely silently; when its negated form does find the pattern that\nshouldn't be there, it prints the matching line(s) but otherwise exits\nwithout any error message. This leaves the developer puzzled about\nwhat could have gone wrong. Well, at least it left me puzzled...\n\nInitially all I wanted to do was to make 'test_i18ngrep' more\ninformative on failure, but then skeletons started to fall out of the\ncloset^Wour test suite, and BAM! before I knew it I had 10 patches:\n\n\n t5541: add 'test_i18ngrep's missing filename parameter\n t5812: add 'test_i18ngrep's missing filename parameter\n t6022: don't run 'git merge' upstream of a pipe\n t4001: don't run 'git status' upstream of a pipe\n\nBugfixes for a few tests. The first two are fun.\n\n t5510: consolidate 'grep' and 'test_i18ngrep' patterns\n t5536: let 'test_i18ngrep' read the file without redirection\n\nCleanups.\n\n t: move 'test_i18ncmp' and 'test_i18ngrep' to 'test-lib-functions.sh'\n\nPure code movement.\n\n t: forbid piping into 'test_i18ngrep'\n t: make sure that 'test_i18ngrep' got enough parameters\n\nThese try to prevent similar bugs in our tests in the future. Both\nare imperfect, see the commit messages about their limitations and why\nI think they are good enough.\n\n t: make 'test_i18ngrep' more informative on failure\n\nAgain, see the commit message about its limitations and why it's good\nenough.\n\n\n t/t4001-diff-rename.sh | 11 ++++++---\n t/t5510-fetch.sh | 9 +++-----\n t/t5536-fetch-conflicts.sh | 2 +-\n t/t5541-http-push-smart.sh | 2 +-\n t/t5812-proto-disable-http.sh | 3 +--\n t/t6022-merge-rename.sh | 6 +++--\n t/test-lib-functions.sh | 53 +++++++++++++++++++++++++++++++++++++++++++\n t/test-lib.sh | 26 ---------------------\n 8 files changed, 71 insertions(+), 41 deletions(-)\n\n-- \n2.16.1.155.g5159265b1\n\n"},{"id":"337466","messageId":"20180126123708.21722-2-szeder.dev@gmail.com","threadId":"47699","inReplyTo":"20180126123708.21722-1-szeder.dev@gmail.com","subject":"[PATCH 01/10] t5541: add 'test_i18ngrep's missing filename parameter","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-01-26T12:36:59Z","receivedAt":"2018-01-26T12:37:31Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The test 'push --no-progress silences progress but not status' runs\n'test_i18ngrep' without specifying a filename parameter. This has\nremained unnoticed since its introduction in e304aeba2 (t5541: test\nmore combinations of --progress, 2012-05-01), because that\n'test_i18ngrep' is supposed to check that the given pattern is not\npresent in its input, and of course it won't find that pattern if its\ninput is empty, (as it comes from /dev/null). This also means that\nthis test could miss a potential breakage of 'git push --no-progress'.\n\nSigned-off-by: SZEDER Gábor \n---\n t/t5541-http-push-smart.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh\nindex d38bf3247..21340e89c 100755\n--- a/t/t5541-http-push-smart.sh\n+++ b/t/t5541-http-push-smart.sh\n@@ -234,7 +234,7 @@ test_expect_success TTY 'push --no-progress silences progress but not status' '\n \ttest_commit no-progress &&\n \ttest_terminal git push --no-progress >output 2>&1 &&\n \ttest_i18ngrep \"^To http\" output &&\n-\ttest_i18ngrep ! \"^Writing objects\"\n+\ttest_i18ngrep ! \"^Writing objects\" output\n '\n \n test_expect_success 'push --progress shows progress to non-tty' '\n-- \n2.16.1.155.g5159265b1\n\n"},{"id":"337467","messageId":"20180126123708.21722-5-szeder.dev@gmail.com","threadId":"47699","inReplyTo":"20180126123708.21722-1-szeder.dev@gmail.com","subject":"[PATCH 04/10] t4001: don't run 'git status' upstream of a pipe","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-01-26T12:37:02Z","receivedAt":"2018-01-26T12:37:34Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The primary purpose of three tests in 't4001-diff-rename.sh' is to\ncheck rename detection in 'git status', but all three do so by running\n'git status' upstream of a pipe, hiding its exit code. Consequently,\nthe test could continue even if 'git status' exited with error.\n\nUse an intermediate file between 'git status' and 'test_i18ngrep' to\ncatch a potential failure of the former.\n\nSigned-off-by: SZEDER Gábor \n---\n t/t4001-diff-rename.sh | 11 ++++++++---\n 1 file changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t4001-diff-rename.sh b/t/t4001-diff-rename.sh\nindex eadf4f624..a07816d56 100755\n--- a/t/t4001-diff-rename.sh\n+++ b/t/t4001-diff-rename.sh\n@@ -134,11 +134,15 @@ test_expect_success 'favour same basenames over different ones' '\n \tgit rm path1 &&\n \tmkdir subdir &&\n \tgit mv another-path subdir/path1 &&\n-\tgit status | test_i18ngrep \"renamed: .*path1 -> subdir/path1\"'\n+\tgit status >out &&\n+\ttest_i18ngrep \"renamed: .*path1 -> subdir/path1\" out\n+'\n \n test_expect_success 'favour same basenames even with minor differences' '\n \tgit show HEAD:path1 | sed \"s/15/16/\" > subdir/path1 &&\n-\tgit status | test_i18ngrep \"renamed: .*path1 -> subdir/path1\"'\n+\tgit status >out &&\n+\ttest_i18ngrep \"renamed: .*path1 -> subdir/path1\" out\n+'\n \n test_expect_success 'two files with same basename and same content' '\n \tgit reset --hard &&\n@@ -148,7 +152,8 @@ test_expect_success 'two files with same basename and same content' '\n \tgit add dir &&\n \tgit commit -m 2 &&\n \tgit mv dir other-dir &&\n-\tgit status | test_i18ngrep \"renamed: .*dir/A/file -> other-dir/A/file\"\n+\tgit status >out &&\n+\ttest_i18ngrep \"renamed: .*dir/A/file -> other-dir/A/file\" out\n '\n \n test_expect_success 'setup for many rename source candidates' '\n-- \n2.16.1.155.g5159265b1\n\n"},{"id":"337468","messageId":"20180126123708.21722-8-szeder.dev@gmail.com","threadId":"47699","inReplyTo":"20180126123708.21722-1-szeder.dev@gmail.com","subject":"[PATCH 07/10] t: move 'test_i18ncmp' and 'test_i18ngrep' to 'test-lib-functions.sh'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-01-26T12:37:05Z","receivedAt":"2018-01-26T12:37:37Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Both 'test_i18ncmp' and 'test_i18ngrep' helper functions are supposed\nto be called from our test scripts, so they should be in\n'test-lib-functions.sh'.\n\nSigned-off-by: SZEDER Gábor \n---\n t/test-lib-functions.sh | 26 ++++++++++++++++++++++++++\n t/test-lib.sh | 26 --------------------------\n 2 files changed, 26 insertions(+), 26 deletions(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 1701fe2a0..92ed02937 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -705,6 +705,32 @@ test_cmp_bin() {\n \tcmp \"$@\"\n }\n \n+# Use this instead of test_cmp to compare files that contain expected and\n+# actual output from git commands that can be translated. When running\n+# under GETTEXT_POISON this pretends that the command produced expected\n+# results.\n+test_i18ncmp () {\n+\ttest -n \"$GETTEXT_POISON\" || test_cmp \"$@\"\n+}\n+\n+# Use this instead of \"grep expected-string actual\" to see if the\n+# output from a git command that can be translated either contains an\n+# expected string, or does not contain an unwanted one. When running\n+# under GETTEXT_POISON this pretends that the command produced expected\n+# results.\n+test_i18ngrep () {\n+\tif test -n \"$GETTEXT_POISON\"\n+\tthen\n+\t : # pretend success\n+\telif test \"x!\" = \"x$1\"\n+\tthen\n+\t\tshift\n+\t\t! grep \"$@\"\n+\telse\n+\t\tgrep \"$@\"\n+\tfi\n+}\n+\n # Call any command \"$@\" but be more verbose about its\n # failure. This is handy for commands like \"test\" which do\n # not output anything when they fail.\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 9a0a21f49..852b22c80 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1062,32 +1062,6 @@ else\n \ttest_set_prereq C_LOCALE_OUTPUT\n fi\n \n-# Use this instead of test_cmp to compare files that contain expected and\n-# actual output from git commands that can be translated. When running\n-# under GETTEXT_POISON this pretends that the command produced expected\n-# results.\n-test_i18ncmp () {\n-\ttest -n \"$GETTEXT_POISON\" || test_cmp \"$@\"\n-}\n-\n-# Use this instead of \"grep expected-string actual\" to see if the\n-# output from a git command that can be translated either contains an\n-# expected string, or does not contain an unwanted one. When running\n-# under GETTEXT_POISON this pretends that the command produced expected\n-# results.\n-test_i18ngrep () {\n-\tif test -n \"$GETTEXT_POISON\"\n-\tthen\n-\t : # pretend success\n-\telif test \"x!\" = \"x$1\"\n-\tthen\n-\t\tshift\n-\t\t! grep \"$@\"\n-\telse\n-\t\tgrep \"$@\"\n-\tfi\n-}\n-\n test_lazy_prereq PIPE '\n \t# test whether the filesystem supports FIFOs\n \ttest_have_prereq !MINGW,!CYGWIN &&\n-- \n2.16.1.155.g5159265b1\n\n"},{"id":"337469","messageId":"20180126123708.21722-11-szeder.dev@gmail.com","threadId":"47699","inReplyTo":"20180126123708.21722-1-szeder.dev@gmail.com","subject":"[PATCH 10/10] t: make 'test_i18ngrep' more informative on failure","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-01-26T12:37:08Z","receivedAt":"2018-01-26T12:37:40Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"When 'test_i18ngrep' can't find the expected pattern, it exits\ncompletely silently; when its negated form does find the pattern that\nshouldn't be there, it prints the matching line(s) but otherwise exits\nwithout any error message. This leaves the developer puzzled about\nwhat could have gone wrong.\n\nMake 'test_i18ngrep' more informative on failure by printing an error\nmessage including the invoked 'grep' command and the contents of the\nfile it had to scan through.\n\nNote that this \"dump the scanned file\" part is not quite perfect, as\nit dumps only the file specified as the function's last positional\nparameter, thus assuming that there is only a single file parameter.\nI think that's a reasonable assumption to make, one that holds true in\nthe current code base. And even if someone were to scan multiple\nfiles at once in the future, the worst thing that could happen is that\nthe verbose error message won't include the contents of all those\nfiles, only the last one. Alas, we can't really do any better than\nthis, because checking whether the other positional parameters match a\nfilename can result in false positives: 't3400-rebase.sh' and\n't3404-rebase-interactive.sh' contain one test each, where the\n'test_i18ngrep's pattern verbatimely matches a file in the trash\ndirectory. Note that the absence of a file parameter is not an issue,\nbecause the lint check added in the previous commit ensures that\n'test_i18ngrep' never reads from its standard input, consequently\nthere must be a file parameter.\n\nSigned-off-by: SZEDER Gábor \n---\n t/test-lib-functions.sh | 25 +++++++++++++++++++++----\n 1 file changed, 21 insertions(+), 4 deletions(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex b543fd0e0..1f1d89d7a 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -731,14 +731,31 @@ test_i18ngrep () {\n \n \tif test -n \"$GETTEXT_POISON\"\n \tthen\n-\t : # pretend success\n-\telif test \"x!\" = \"x$1\"\n+\t\t# pretend success\n+\t\treturn 0\n+\tfi\n+\n+\tif test \"x!\" = \"x$1\"\n \tthen\n \t\tshift\n-\t\t! grep \"$@\"\n+\t\t! grep \"$@\" && return 0\n+\n+\t\techo >&2 \"error: grep '! $@' did find a match in:\"\n \telse\n-\t\tgrep \"$@\"\n+\t\tgrep \"$@\" && return 0\n+\n+\t\techo >&2 \"error: grep '$@' didn't find a match in:\"\n \tfi\n+\t(\n+\t\teval \"f=\\\"\\${$#}\\\"\"\n+\t\tif test -s \"$f\"\n+\t\tthen\n+\t\t\tcat >&2 \"$f\"\n+\t\telse\n+\t\t\techo \"\"\n+\t\tfi\n+\t)\n+\treturn 1\n }\n \n # Call any command \"$@\" but be more verbose about its\n-- \n2.16.1.155.g5159265b1\n\n"},{"id":"337470","messageId":"20180126123708.21722-7-szeder.dev@gmail.com","threadId":"47699","inReplyTo":"20180126123708.21722-1-szeder.dev@gmail.com","subject":"[PATCH 06/10] t5536: let 'test_i18ngrep' read the file without redirection","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-01-26T12:37:04Z","receivedAt":"2018-01-26T12:37:44Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Redirecting 'test_i18ngrep's standard input from a file will interfere\nwith the linting that will be added in a later patch.\n\nSigned-off-by: SZEDER Gábor \n---\n t/t5536-fetch-conflicts.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t5536-fetch-conflicts.sh b/t/t5536-fetch-conflicts.sh\nindex 2e42cf331..644736b8a 100755\n--- a/t/t5536-fetch-conflicts.sh\n+++ b/t/t5536-fetch-conflicts.sh\n@@ -22,7 +22,7 @@ verify_stderr () {\n \tcat >expected &&\n \t# We're not interested in the error\n \t# \"fatal: The remote end hung up unexpectedly\":\n-\ttest_i18ngrep -E '^(fatal|warning):' actual | sort &&\n+\ttest_i18ngrep -E '^(fatal|warning):' error | grep -v 'hung up' >actual | sort &&\n \ttest_i18ncmp expected actual\n }\n \n-- \n2.16.1.155.g5159265b1\n\n"},{"id":"337471","messageId":"20180126123708.21722-10-szeder.dev@gmail.com","threadId":"47699","inReplyTo":"20180126123708.21722-1-szeder.dev@gmail.com","subject":"[PATCH 09/10] t: make sure that 'test_i18ngrep' got enough parameters","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-01-26T12:37:07Z","receivedAt":"2018-01-26T12:37:46Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Two of the previous patches in this series fixed two bogus\n'test_i18ngrep' invocations that had neither a filename parameter not\nanything piped into their standard input, yet both managed to remain\nunnoticed for years. A third similarly bogus invocation is currently\nlurking in 'pu' for a couple of weeks now.\n\nTry to catch similar mistakes in the future by ensuring that\n'test_i18ngrep' has at least two parameters, not including an optional\n'!' to negate the pattern. Perform these checks after we made sure\nthat there is no data on the 'test_i18ngrep's standard input, so if\nthe filename parameter is missing because someone is piping a git\ncommand's output into this function, then they would get the more\nrelevant error message.\n\nNote that this is not quite perfect, as it doesn't account for any\n'grep --options' given as parameters. However, doing so would be far\ntoo complicated, considering that patters can start with dashes as\nwell, and in the majority of the cases we don't use any such options\nanyway.\n\nSigned-off-by: SZEDER Gábor \n---\n\nAbout that third one in 'pu': it's test '3b-check: Avoid implicit\nrename if involved as source on current side' introduced in commit\nfcd649216 (directory rename detection: testcases to avoid taking\ndetection too far, 2018-01-05) in branch\n'en/rename-directory-detection'.\n\n https://public-inbox.org/git/CAM0VKj=qhJQJ7uJWbBouSTYD0frA1zp1gwXzMVXuTiF+C6GH+g@mail.gmail.com/T/#u\n\n t/test-lib-functions.sh | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex e381d50d0..b543fd0e0 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -723,6 +723,12 @@ test_i18ngrep () {\n \terror \"bug in the test script: data on test_i18ngrep's stdin;\" \\\n \t \"perhaps a git command's output is piped into it?\"\n \n+\tif test $# -lt 2 ||\n+\t { test \"x!\" = \"x$1\" && test $# -lt 3 ; }\n+\tthen\n+\t\terror \"bug in the test script: too few parameters to test_i18ngrep\"\n+\tfi\n+\n \tif test -n \"$GETTEXT_POISON\"\n \tthen\n \t : # pretend success\n-- \n2.16.1.155.g5159265b1\n\n"},{"id":"337472","messageId":"20180126123708.21722-6-szeder.dev@gmail.com","threadId":"47699","inReplyTo":"20180126123708.21722-1-szeder.dev@gmail.com","subject":"[PATCH 05/10] t5510: consolidate 'grep' and 'test_i18ngrep' patterns","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-01-26T12:37:03Z","receivedAt":"2018-01-26T12:37:48Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"One of the tests in 't5510-fetch.sh' checks the output of 'git fetch'\nusing 'test_i18ngrep', and while doing so it prefilters the output\nwith 'grep' before piping the result into 'test_i18ngrep'.\n\nThis prefiltering is unnecessary, with the appropriate pattern\n'test_i18ngrep' can do it all by itself. Furthermore, piping data\ninto 'test_i18ngrep' will interfere with the linting that will be\nadded in a later patch.\n\nSigned-off-by: SZEDER Gábor \n---\n t/t5510-fetch.sh | 9 +++------\n 1 file changed, 3 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 668c54be4..3debc87d4 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -222,12 +222,9 @@ test_expect_success 'fetch uses remote ref names to describe new refs' '\n \t(\n \t\tcd descriptive &&\n \t\tgit fetch o 2>actual &&\n-\t\tgrep \" -> refs/crazyheads/descriptive-branch$\" actual |\n-\t\ttest_i18ngrep \"new branch\" &&\n-\t\tgrep \" -> descriptive-tag$\" actual |\n-\t\ttest_i18ngrep \"new tag\" &&\n-\t\tgrep \" -> crazy$\" actual |\n-\t\ttest_i18ngrep \"new ref\"\n+\t\ttest_i18ngrep \"new branch.* -> refs/crazyheads/descriptive-branch$\" actual &&\n+\t\ttest_i18ngrep \"new tag.* -> descriptive-tag$\" actual &&\n+\t\ttest_i18ngrep \"new ref.* -> crazy$\" actual\n \t) &&\n \tgit checkout master\n '\n-- \n2.16.1.155.g5159265b1\n\n"},{"id":"337473","messageId":"20180126123708.21722-9-szeder.dev@gmail.com","threadId":"47699","inReplyTo":"20180126123708.21722-1-szeder.dev@gmail.com","subject":"[PATCH 08/10] t: forbid piping into 'test_i18ngrep'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-01-26T12:37:06Z","receivedAt":"2018-01-26T12:37:51Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"When checking a git command's output with 'test_i18ngrep', it's\ntempting to conveniently pipe the git command's standard output into\n'test_i18ngrep'. Unfortunately, this is an anti-pattern, because it\nhides the git command's exit code, and the test could continue even if\nthe command exited with error.\n\nAdd a bit of linting to 'test_i18ngrep' to detect when data is fed to\nits standard input and to error out with a \"bug in the test script\"\nmessage.\n\nNote that this change will also forbid cases where 'test_i18ngrep'\nwould legitimately read its standard input, e.g.\n\n - when its standard input is redirected from a file, or\n\n - when a git command's standard output is first written to an\n intermediate file, which is then preprocessed by a non-git command\n before the results are piped into 'test_i18ngrep'.\n\nSee two of the previous patches for the only such cases we had in our\ntest suite. However, reliably preventing this antipattern is arguably\nmore important than supporting these cases, which can be worked around\nby only minor inconveniences.\n\nSigned-off-by: SZEDER Gábor \n---\n t/test-lib-functions.sh | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 92ed02937..e381d50d0 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -719,6 +719,10 @@ test_i18ncmp () {\n # under GETTEXT_POISON this pretends that the command produced expected\n # results.\n test_i18ngrep () {\n+\t( read line ) &&\n+\terror \"bug in the test script: data on test_i18ngrep's stdin;\" \\\n+\t \"perhaps a git command's output is piped into it?\"\n+\n \tif test -n \"$GETTEXT_POISON\"\n \tthen\n \t : # pretend success\n-- \n2.16.1.155.g5159265b1\n\n"},{"id":"337474","messageId":"20180126123708.21722-3-szeder.dev@gmail.com","threadId":"47699","inReplyTo":"20180126123708.21722-1-szeder.dev@gmail.com","subject":"[PATCH 02/10] t5812: add 'test_i18ngrep's missing filename parameter","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-01-26T12:37:00Z","receivedAt":"2018-01-26T12:37:53Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The second 'test_i18ngrep' invocation in the test 'curl redirects\nrespect whitelist' is missing its filename parameter. This has\nremained unnoticed since its introduction in f4113cac0 (http: limit\nredirection to protocol-whitelist, 2015-09-22), because it would only\ncause the test to fail if Git was built with a sufficiently old\nlibcurl version. The test's two ||-chained 'test_i18ngrep'\ninvocations are supposed to check that either one of the two patterns\nis present in 'git clone's error message. As it happens, the first\ninvocation covers the error message from any reasonably up-to-date\nlibcurl, thus the second invocation, the one without the filename\nparameter, isn't executed at all. Apparently no one has run the test\nsuite's httpd tests with such an old libcurl in the last 2+ years, or\nat least they haven't bothered to notify us about the failed test.\n\nFix this by consolidating the two patterns into a single extended\nregexp, eliminating the need for an ||-chained second 'test_i18ngrep'\ninvocation.\n\nSigned-off-by: SZEDER Gábor \n---\n t/t5812-proto-disable-http.sh | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/t/t5812-proto-disable-http.sh b/t/t5812-proto-disable-http.sh\nindex d911afd24..226a4920c 100755\n--- a/t/t5812-proto-disable-http.sh\n+++ b/t/t5812-proto-disable-http.sh\n@@ -21,8 +21,7 @@ test_expect_success 'curl redirects respect whitelist' '\n \t\t\t GIT_SMART_HTTP=0 \\\n \t\tgit clone \"$HTTPD_URL/ftp-redir/repo.git\" 2>stderr &&\n \t{\n-\t\ttest_i18ngrep \"ftp.*disabled\" stderr ||\n-\t\ttest_i18ngrep \"your curl version is too old\"\n+\t\ttest_i18ngrep -E \"(ftp.*disabled|your curl version is too old)\" stderr\n \t}\n '\n \n-- \n2.16.1.155.g5159265b1\n\n"},{"id":"337475","messageId":"20180126123708.21722-4-szeder.dev@gmail.com","threadId":"47699","inReplyTo":"20180126123708.21722-1-szeder.dev@gmail.com","subject":"[PATCH 03/10] t6022: don't run 'git merge' upstream of a pipe","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-01-26T12:37:01Z","receivedAt":"2018-01-26T12:37:58Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The primary purpose of 't6022-merge-rename.sh' is to test 'git merge',\nbut one of the tests runs it upstream of a pipe, hiding its exit code.\nConsequently, the test could continue even if 'git merge' exited with\nerror.\n\nUse an intermediate file between 'git merge' and 'test_i18ngrep' to\ncatch a potential failure of the former.\n\nSigned-off-by: SZEDER Gábor \n---\n t/t6022-merge-rename.sh | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t6022-merge-rename.sh b/t/t6022-merge-rename.sh\nindex 05ebba7af..c01f721f1 100755\n--- a/t/t6022-merge-rename.sh\n+++ b/t/t6022-merge-rename.sh\n@@ -242,10 +242,12 @@ test_expect_success 'merge of identical changes in a renamed file' '\n \trm -f A M N &&\n \tgit reset --hard &&\n \tgit checkout change+rename &&\n-\tGIT_MERGE_VERBOSITY=3 git merge change | test_i18ngrep \"^Skipped B\" &&\n+\tGIT_MERGE_VERBOSITY=3 git merge change >out &&\n+\ttest_i18ngrep \"^Skipped B\" out &&\n \tgit reset --hard HEAD^ &&\n \tgit checkout change &&\n-\tGIT_MERGE_VERBOSITY=3 git merge change+rename | test_i18ngrep \"^Skipped B\"\n+\tGIT_MERGE_VERBOSITY=3 git merge change+rename >out &&\n+\ttest_i18ngrep \"^Skipped B\" out\n '\n \n test_expect_success 'setup for rename + d/f conflicts' '\n-- \n2.16.1.155.g5159265b1\n\n"},{"id":"337493","messageId":"xmqqa7x08p0e.fsf@gitster.mtv.corp.google.com","threadId":"47699","inReplyTo":"20180126123708.21722-6-szeder.dev@gmail.com","subject":"Re: [PATCH 05/10] t5510: consolidate 'grep' and 'test_i18ngrep' patterns","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-01-26T18:16:01Z","receivedAt":"2018-01-26T18:16:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> One of the tests in 't5510-fetch.sh' checks the output of 'git fetch'\n> using 'test_i18ngrep', and while doing so it prefilters the output\n> with 'grep' before piping the result into 'test_i18ngrep'.\n>\n> This prefiltering is unnecessary, with the appropriate pattern\n> 'test_i18ngrep' can do it all by itself. Furthermore, piping data\n> into 'test_i18ngrep' will interfere with the linting that will be\n> added in a later patch.\n\nIt is very likely that the prefiltering \"grep\" will not even see\nwhat it is looking for under GETTEXT_POISON build in the first\nplace, so this conversion is the right thing to do from that point\nof view as well.\n\n\n\n>\n> Signed-off-by: SZEDER Gábor \n> ---\n> t/t5510-fetch.sh | 9 +++------\n> 1 file changed, 3 insertions(+), 6 deletions(-)\n>\n> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n> index 668c54be4..3debc87d4 100755\n> --- a/t/t5510-fetch.sh\n> +++ b/t/t5510-fetch.sh\n> @@ -222,12 +222,9 @@ test_expect_success 'fetch uses remote ref names to describe new refs' '\n> \t(\n> \t\tcd descriptive &&\n> \t\tgit fetch o 2>actual &&\n> -\t\tgrep \" -> refs/crazyheads/descriptive-branch$\" actual |\n> -\t\ttest_i18ngrep \"new branch\" &&\n> -\t\tgrep \" -> descriptive-tag$\" actual |\n> -\t\ttest_i18ngrep \"new tag\" &&\n> -\t\tgrep \" -> crazy$\" actual |\n> -\t\ttest_i18ngrep \"new ref\"\n> +\t\ttest_i18ngrep \"new branch.* -> refs/crazyheads/descriptive-branch$\" actual &&\n> +\t\ttest_i18ngrep \"new tag.* -> descriptive-tag$\" actual &&\n> +\t\ttest_i18ngrep \"new ref.* -> crazy$\" actual\n> \t) &&\n> \tgit checkout master\n> '\n"},{"id":"337495","messageId":"xmqq607o8ouy.fsf@gitster.mtv.corp.google.com","threadId":"47699","inReplyTo":"20180126123708.21722-8-szeder.dev@gmail.com","subject":"Re: [PATCH 07/10] t: move 'test_i18ncmp' and 'test_i18ngrep' to 'test-lib-functions.sh'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-01-26T18:19:17Z","receivedAt":"2018-01-26T18:19:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> Both 'test_i18ncmp' and 'test_i18ngrep' helper functions are supposed\n> to be called from our test scripts, so they should be in\n> 'test-lib-functions.sh'.\n>\n> Signed-off-by: SZEDER Gábor \n> ---\n> t/test-lib-functions.sh | 26 ++++++++++++++++++++++++++\n> t/test-lib.sh | 26 --------------------------\n> 2 files changed, 26 insertions(+), 26 deletions(-)\n\nHmph. I do not care too much either way, but I had an impression\nthat test-lib-functions.sh is meant to be more generic (i.e. those\nwho want can steal it from us and use it in their project without\ndragging too much of the local convention we employ in this project)\nthan what is in test-lib.sh, which can heavily be specific to Git,\nand I also had an impression that gettext-poison build is quite a\nlocal convention we use in this project, not applicable to other\npeople.\n\n>\n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index 1701fe2a0..92ed02937 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -705,6 +705,32 @@ test_cmp_bin() {\n> \tcmp \"$@\"\n> }\n> \n> +# Use this instead of test_cmp to compare files that contain expected and\n> +# actual output from git commands that can be translated. When running\n> +# under GETTEXT_POISON this pretends that the command produced expected\n> +# results.\n> +test_i18ncmp () {\n> +\ttest -n \"$GETTEXT_POISON\" || test_cmp \"$@\"\n> +}\n> +\n> +# Use this instead of \"grep expected-string actual\" to see if the\n> +# output from a git command that can be translated either contains an\n> +# expected string, or does not contain an unwanted one. When running\n> +# under GETTEXT_POISON this pretends that the command produced expected\n> +# results.\n> +test_i18ngrep () {\n> +\tif test -n \"$GETTEXT_POISON\"\n> +\tthen\n> +\t : # pretend success\n> +\telif test \"x!\" = \"x$1\"\n> +\tthen\n> +\t\tshift\n> +\t\t! grep \"$@\"\n> +\telse\n> +\t\tgrep \"$@\"\n> +\tfi\n> +}\n> +\n> # Call any command \"$@\" but be more verbose about its\n> # failure. This is handy for commands like \"test\" which do\n> # not output anything when they fail.\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 9a0a21f49..852b22c80 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -1062,32 +1062,6 @@ else\n> \ttest_set_prereq C_LOCALE_OUTPUT\n> fi\n> \n> -# Use this instead of test_cmp to compare files that contain expected and\n> -# actual output from git commands that can be translated. When running\n> -# under GETTEXT_POISON this pretends that the command produced expected\n> -# results.\n> -test_i18ncmp () {\n> -\ttest -n \"$GETTEXT_POISON\" || test_cmp \"$@\"\n> -}\n> -\n> -# Use this instead of \"grep expected-string actual\" to see if the\n> -# output from a git command that can be translated either contains an\n> -# expected string, or does not contain an unwanted one. When running\n> -# under GETTEXT_POISON this pretends that the command produced expected\n> -# results.\n> -test_i18ngrep () {\n> -\tif test -n \"$GETTEXT_POISON\"\n> -\tthen\n> -\t : # pretend success\n> -\telif test \"x!\" = \"x$1\"\n> -\tthen\n> -\t\tshift\n> -\t\t! grep \"$@\"\n> -\telse\n> -\t\tgrep \"$@\"\n> -\tfi\n> -}\n> -\n> test_lazy_prereq PIPE '\n> \t# test whether the filesystem supports FIFOs\n> \ttest_have_prereq !MINGW,!CYGWIN &&\n"},{"id":"337496","messageId":"20180126182303.GA27618@sigill.intra.peff.net","threadId":"47699","inReplyTo":"20180126123708.21722-2-szeder.dev@gmail.com","subject":"Re: [PATCH 01/10] t5541: add 'test_i18ngrep's missing filename parameter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-26T18:23:03Z","receivedAt":"2018-01-26T18:23:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 26, 2018 at 01:36:59PM +0100, SZEDER Gábor wrote:\n\n> The test 'push --no-progress silences progress but not status' runs\n> 'test_i18ngrep' without specifying a filename parameter. This has\n> remained unnoticed since its introduction in e304aeba2 (t5541: test\n> more combinations of --progress, 2012-05-01), because that\n> 'test_i18ngrep' is supposed to check that the given pattern is not\n> present in its input, and of course it won't find that pattern if its\n> input is empty, (as it comes from /dev/null). This also means that\n> this test could miss a potential breakage of 'git push --no-progress'.\n\nOof, embarrassing. Thanks for catching.\n\nThis and other errors make me wonder if test_i18ngrep ought to take an\nexplicit \"-\" for stdin, and error out if no file argument is given. That\nmay be overkill, though (and it's not like we wouldn't have the same\nproblem with regular \"grep\").\n\n-Peff\n"},{"id":"337497","messageId":"20180126182346.GA27712@sigill.intra.peff.net","threadId":"47699","inReplyTo":"20180126182303.GA27618@sigill.intra.peff.net","subject":"Re: [PATCH 01/10] t5541: add 'test_i18ngrep's missing filename parameter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-26T18:23:46Z","receivedAt":"2018-01-26T18:23:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 26, 2018 at 01:23:03PM -0500, Jeff King wrote:\n\n> On Fri, Jan 26, 2018 at 01:36:59PM +0100, SZEDER Gábor wrote:\n> \n> > The test 'push --no-progress silences progress but not status' runs\n> > 'test_i18ngrep' without specifying a filename parameter. This has\n> > remained unnoticed since its introduction in e304aeba2 (t5541: test\n> > more combinations of --progress, 2012-05-01), because that\n> > 'test_i18ngrep' is supposed to check that the given pattern is not\n> > present in its input, and of course it won't find that pattern if its\n> > input is empty, (as it comes from /dev/null). This also means that\n> > this test could miss a potential breakage of 'git push --no-progress'.\n> \n> Oof, embarrassing. Thanks for catching.\n> \n> This and other errors make me wonder if test_i18ngrep ought to take an\n> explicit \"-\" for stdin, and error out if no file argument is given. That\n> may be overkill, though (and it's not like we wouldn't have the same\n> problem with regular \"grep\").\n\n....and I really ought to start reading your entire sets of patches\nbefore commenting on the early ones. :-/\n\n-Peff\n"},{"id":"337498","messageId":"xmqq1sic8omp.fsf@gitster.mtv.corp.google.com","threadId":"47699","inReplyTo":"20180126123708.21722-9-szeder.dev@gmail.com","subject":"Re: [PATCH 08/10] t: forbid piping into 'test_i18ngrep'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-01-26T18:24:14Z","receivedAt":"2018-01-26T18:24:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> See two of the previous patches for the only such cases we had in our\n> test suite. However, reliably preventing this antipattern is arguably\n> more important than supporting these cases, which can be worked around\n> by only minor inconveniences.\n\nI am not sure if that inconveniences will be minor. Is this too\ncontrived an example, for example?\n\n check () {\n pattern=$1 file=$2 script=./runme\n\n test_i18ngrep \"$pattern\" \"$file\" &&\n write_script \"$script\" &&\n test_expect_success \"check $pattern\" '\n \"$script\"\n '\n }\n\n check foo file <<-EOF\n ... test script comes here ...\n EOF\n\n\n>\n> Signed-off-by: SZEDER Gábor \n> ---\n> t/test-lib-functions.sh | 4 ++++\n> 1 file changed, 4 insertions(+)\n>\n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index 92ed02937..e381d50d0 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -719,6 +719,10 @@ test_i18ncmp () {\n> # under GETTEXT_POISON this pretends that the command produced expected\n> # results.\n> test_i18ngrep () {\n> +\t( read line ) &&\n> +\terror \"bug in the test script: data on test_i18ngrep's stdin;\" \\\n> +\t \"perhaps a git command's output is piped into it?\"\n> +\n> \tif test -n \"$GETTEXT_POISON\"\n> \tthen\n> \t : # pretend success\n"},{"id":"337499","messageId":"20180126182734.GB27618@sigill.intra.peff.net","threadId":"47699","inReplyTo":"20180126123708.21722-3-szeder.dev@gmail.com","subject":"Re: [PATCH 02/10] t5812: add 'test_i18ngrep's missing filename parameter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-26T18:27:35Z","receivedAt":"2018-01-26T18:27:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 26, 2018 at 01:37:00PM +0100, SZEDER Gábor wrote:\n\n> The second 'test_i18ngrep' invocation in the test 'curl redirects\n> respect whitelist' is missing its filename parameter. This has\n> remained unnoticed since its introduction in f4113cac0 (http: limit\n> redirection to protocol-whitelist, 2015-09-22), because it would only\n> cause the test to fail if Git was built with a sufficiently old\n> libcurl version. The test's two ||-chained 'test_i18ngrep'\n> invocations are supposed to check that either one of the two patterns\n> is present in 'git clone's error message. As it happens, the first\n> invocation covers the error message from any reasonably up-to-date\n> libcurl, thus the second invocation, the one without the filename\n> parameter, isn't executed at all. Apparently no one has run the test\n> suite's httpd tests with such an old libcurl in the last 2+ years, or\n> at least they haven't bothered to notify us about the failed test.\n\nInteresting find.\n\nThe \"too old\" curl is older than 7.19.4, which we actually fail to build\nwith since v2.12.0. So they probably did not even get as far as the\ntests. ;)\n\n> Fix this by consolidating the two patterns into a single extended\n> regexp, eliminating the need for an ||-chained second 'test_i18ngrep'\n> invocation.\n\nOK. Once upon a time I think we had trouble with \"grep -E\", since some\nolder systems had only \"egrep\". But I see we've introduced some \"grep\n-E\" invocations as far back as 2013 and nobody has complained, so it's\nprobably fine.\n\n-Peff\n"},{"id":"337501","messageId":"20180126183229.GC27618@sigill.intra.peff.net","threadId":"47699","inReplyTo":"xmqq607o8ouy.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 07/10] t: move 'test_i18ncmp' and 'test_i18ngrep' to 'test-lib-functions.sh'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-26T18:32:30Z","receivedAt":"2018-01-26T18:32:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 26, 2018 at 10:19:17AM -0800, Junio C Hamano wrote:\n\n> SZEDER Gábor writes:\n> \n> > Both 'test_i18ncmp' and 'test_i18ngrep' helper functions are supposed\n> > to be called from our test scripts, so they should be in\n> > 'test-lib-functions.sh'.\n> >\n> > Signed-off-by: SZEDER Gábor \n> > ---\n> > t/test-lib-functions.sh | 26 ++++++++++++++++++++++++++\n> > t/test-lib.sh | 26 --------------------------\n> > 2 files changed, 26 insertions(+), 26 deletions(-)\n> \n> Hmph. I do not care too much either way, but I had an impression\n> that test-lib-functions.sh is meant to be more generic (i.e. those\n> who want can steal it from us and use it in their project without\n> dragging too much of the local convention we employ in this project)\n> than what is in test-lib.sh, which can heavily be specific to Git,\n> and I also had an impression that gettext-poison build is quite a\n> local convention we use in this project, not applicable to other\n> people.\n\nI had a similar notion, but I thought it was the other way around:\ntest-lib.sh was supposed to be the harness, and test-lib-functions.sh\nwas our own stuff. But I do not think we have really kept to that over\nthe years. TBH I have generally been confused by the distinction and\njust use ctags to find the right source file. ;)\n\n-Peff\n"},{"id":"337502","messageId":"xmqqshas79di.fsf@gitster.mtv.corp.google.com","threadId":"47699","inReplyTo":"xmqq1sic8omp.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 08/10] t: forbid piping into 'test_i18ngrep'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-01-26T18:39:05Z","receivedAt":"2018-01-26T18:39:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> SZEDER Gábor writes:\n>\n>> See two of the previous patches for the only such cases we had in our\n>> test suite. However, reliably preventing this antipattern is arguably\n>> more important than supporting these cases, which can be worked around\n>> by only minor inconveniences.\n>\n> I am not sure if that inconveniences will be minor. Is this too\n> contrived an example, for example?\n>\n> check () {\n> pattern=$1 file=$2 script=./runme\n>\n> test_i18ngrep \"$pattern\" \"$file\" &&\n> write_script \"$script\" &&\n> test_expect_success \"check $pattern\" '\n> \"$script\"\n> '\n> }\n>\n> check foo file <<-EOF\n> ... test script comes here ...\n> EOF\n\nIs there a case where test_i18ngrep (after your clean-ups in this\nseries up to 06/10) needs to read from more than one file?\n\nI actually think that the kind of inconveniences we *can* work with,\nwithout risking breakage to legitimate test, would be to allow and\nrequire test_i18ngrep to name and read only from one file that\nappears at the end of its command line. IOW, instead of doing a\nprobing \"read\" that you cannot undo and break legitimate test, I\nthink it is OK to see if the last token names a file that is on the\nfilesystem, e.g.\n\n\ttest_i18ngrep () {\n\t\teval test -f \\\"\\${$#}\\\" ||\n\t\terror \"bug in the test sript: test_i18ngrep must\" \\\n\t\t \"name a file to read as the last token on the command line\"\n\t\t...\n\n"},{"id":"337503","messageId":"20180126184151.GD27618@sigill.intra.peff.net","threadId":"47699","inReplyTo":"20180126123708.21722-9-szeder.dev@gmail.com","subject":"Re: [PATCH 08/10] t: forbid piping into 'test_i18ngrep'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-26T18:41:51Z","receivedAt":"2018-01-26T18:42:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 26, 2018 at 01:37:06PM +0100, SZEDER Gábor wrote:\n\n> When checking a git command's output with 'test_i18ngrep', it's\n> tempting to conveniently pipe the git command's standard output into\n> 'test_i18ngrep'. Unfortunately, this is an anti-pattern, because it\n> hides the git command's exit code, and the test could continue even if\n> the command exited with error.\n> \n> Add a bit of linting to 'test_i18ngrep' to detect when data is fed to\n> its standard input and to error out with a \"bug in the test script\"\n> message.\n> \n> Note that this change will also forbid cases where 'test_i18ngrep'\n> would legitimately read its standard input, e.g.\n> \n> - when its standard input is redirected from a file, or\n> \n> - when a git command's standard output is first written to an\n> intermediate file, which is then preprocessed by a non-git command\n> before the results are piped into 'test_i18ngrep'.\n> \n> See two of the previous patches for the only such cases we had in our\n> test suite. However, reliably preventing this antipattern is arguably\n> more important than supporting these cases, which can be worked around\n> by only minor inconveniences.\n\nThe idea seems reasonable to me. Let's think about what the escape hatch\nlooks like to work around it if you need to.\n\nI guess you've got:\n\n cat >file &&\n test_i18ngrep ... file\n\nwhich is not too bad.\n\nYou've also got:\n\n test_i18ngrep ... -\n\nthough that relies on the underlying grep understanding \"-\" (which is in\nPOSIX, though with a rather vague \"if the implementations supports it\").\nAnd it wouldn't work with the \"read\" test in this patch.\n\n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index 92ed02937..e381d50d0 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -719,6 +719,10 @@ test_i18ncmp () {\n> # under GETTEXT_POISON this pretends that the command produced expected\n> # results.\n> test_i18ngrep () {\n> +\t( read line ) &&\n> +\terror \"bug in the test script: data on test_i18ngrep's stdin;\" \\\n> +\t \"perhaps a git command's output is piped into it?\"\n> +\n\nThis seems kind of hacky compared to just seeing if there is a file\nargument. But I suppose that is hard to do, since we just pass through\nthe arguments to grep.\n\nThough looking at our test_18ngrep invocations, they are simple enough\nthat would just ask \"are there two non-option arguments at the end of\nthe command line\". The exception is \"-e\", but IMHO we could just drop\nthat. It serves no purpose unless you're trying to hide a \"-\" at the\nstart of your pattern, and in fact we used to ban it since sysv grep\ndidn't understand it (e.g., aadbe44f883).\n\n-Peff\n"},{"id":"337504","messageId":"20180126184319.GE27618@sigill.intra.peff.net","threadId":"47699","inReplyTo":"xmqqshas79di.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 08/10] t: forbid piping into 'test_i18ngrep'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-26T18:43:20Z","receivedAt":"2018-01-26T18:43:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 26, 2018 at 10:39:05AM -0800, Junio C Hamano wrote:\n\n> Is there a case where test_i18ngrep (after your clean-ups in this\n> series up to 06/10) needs to read from more than one file?\n> \n> I actually think that the kind of inconveniences we *can* work with,\n> without risking breakage to legitimate test, would be to allow and\n> require test_i18ngrep to name and read only from one file that\n> appears at the end of its command line. IOW, instead of doing a\n> probing \"read\" that you cannot undo and break legitimate test, I\n> think it is OK to see if the last token names a file that is on the\n> filesystem, e.g.\n> \n> \ttest_i18ngrep () {\n> \t\teval test -f \\\"\\${$#}\\\" ||\n> \t\terror \"bug in the test sript: test_i18ngrep must\" \\\n> \t\t \"name a file to read as the last token on the command line\"\n\nFWIW I like that much more than the weird \"read\" thing. It would also\ndisallow \"-\", but we could make an exception for that if we choose to.\n\n-Peff\n"},{"id":"337505","messageId":"20180126184736.GF27618@sigill.intra.peff.net","threadId":"47699","inReplyTo":"20180126123708.21722-10-szeder.dev@gmail.com","subject":"Re: [PATCH 09/10] t: make sure that 'test_i18ngrep' got enough parameters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-26T18:47:36Z","receivedAt":"2018-01-26T18:47:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 26, 2018 at 01:37:07PM +0100, SZEDER Gábor wrote:\n\n> Two of the previous patches in this series fixed two bogus\n> 'test_i18ngrep' invocations that had neither a filename parameter not\n> anything piped into their standard input, yet both managed to remain\n> unnoticed for years. A third similarly bogus invocation is currently\n> lurking in 'pu' for a couple of weeks now.\n\nHrm. At first I thought this was redundant with the stdin thing in the\nprevious one. But that is only checking \"did you _try_ to use stdin\".\nThis is checking \"did you accidentally use stdin, which was empty\".\n\nBut I think maybe it's the opposite; the other one is redundant with\nthis one, since it would be hard to convince grep to read from stdin\nanyway with this.\n\n> Note that this is not quite perfect, as it doesn't account for any\n> 'grep --options' given as parameters. However, doing so would be far\n> too complicated, considering that patters can start with dashes as\n> well, and in the majority of the cases we don't use any such options\n> anyway.\n\nYeah, I agree this would help most cases, but not hurt in others. I do\nthink Junio's \"see if the final argument is a file\" approach seems like\nit would cover us pretty accurately, though, without having to get too\nintimate with grep options.\n\n-Peff\n"},{"id":"337507","messageId":"20180126185007.GG27618@sigill.intra.peff.net","threadId":"47699","inReplyTo":"20180126123708.21722-11-szeder.dev@gmail.com","subject":"Re: [PATCH 10/10] t: make 'test_i18ngrep' more informative on failure","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-26T18:50:07Z","receivedAt":"2018-01-26T18:50:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 26, 2018 at 01:37:08PM +0100, SZEDER Gábor wrote:\n\n> When 'test_i18ngrep' can't find the expected pattern, it exits\n> completely silently; when its negated form does find the pattern that\n> shouldn't be there, it prints the matching line(s) but otherwise exits\n> without any error message. This leaves the developer puzzled about\n> what could have gone wrong.\n> \n> Make 'test_i18ngrep' more informative on failure by printing an error\n> message including the invoked 'grep' command and the contents of the\n> file it had to scan through.\n\nI think this is an improvement. You can also use \"-x\" to get a better\nsense of exactly which command failed, but I have never been sad to\nsee more verbose output from failing tests by default. :)\n\n> Note that this \"dump the scanned file\" part is not quite perfect, as\n> it dumps only the file specified as the function's last positional\n> parameter, thus assuming that there is only a single file parameter.\n> I think that's a reasonable assumption to make, one that holds true in\n> the current code base. And even if someone were to scan multiple\n> files at once in the future, the worst thing that could happen is that\n> the verbose error message won't include the contents of all those\n> files, only the last one. Alas, we can't really do any better than\n> this, because checking whether the other positional parameters match a\n> filename can result in false positives: 't3400-rebase.sh' and\n> 't3404-rebase-interactive.sh' contain one test each, where the\n> 'test_i18ngrep's pattern verbatimely matches a file in the trash\n> directory. Note that the absence of a file parameter is not an issue,\n> because the lint check added in the previous commit ensures that\n> 'test_i18ngrep' never reads from its standard input, consequently\n> there must be a file parameter.\n\nHeh, this makes me support even more the \"last one must be a file\" rule\nthat Junio suggested for the linting check.\n\n-Peff\n"},{"id":"337508","messageId":"20180126185136.GH27618@sigill.intra.peff.net","threadId":"47699","inReplyTo":"20180126123708.21722-1-szeder.dev@gmail.com","subject":"Re: [PATCH 00/10] 'test_i18ngrep'-related fixes and improvements","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-26T18:51:36Z","receivedAt":"2018-01-26T18:51:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 26, 2018 at 01:36:58PM +0100, SZEDER Gábor wrote:\n\n> When 'test_i18ngrep' can't find the expected pattern, it exits\n> completely silently; when its negated form does find the pattern that\n> shouldn't be there, it prints the matching line(s) but otherwise exits\n> without any error message. This leaves the developer puzzled about\n> what could have gone wrong. Well, at least it left me puzzled...\n> \n> Initially all I wanted to do was to make 'test_i18ngrep' more\n> informative on failure, but then skeletons started to fall out of the\n> closet^Wour test suite, and BAM! before I knew it I had 10 patches:\n\nI know the feeling. :)\n\nThe series overall looks good to me. I left some comments on the\napproach in the final few patches, but I could live with it as-is, or\nwith the approach Junio suggested.\n\n-Peff\n"},{"id":"337509","messageId":"CAM0VKjn401p4fbF-mJrpaQrgOHGHZ1HtRNx9n+CV+jn4n2a1Uw@mail.gmail.com","threadId":"47699","inReplyTo":"xmqq1sic8omp.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 08/10] t: forbid piping into 'test_i18ngrep'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-01-26T18:51:56Z","receivedAt":"2018-01-26T18:52:24Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Jan 26, 2018 at 7:24 PM, Junio C Hamano wrote:\n> SZEDER Gábor writes:\n>\n>> See two of the previous patches for the only such cases we had in our\n>> test suite. However, reliably preventing this antipattern is arguably\n>> more important than supporting these cases, which can be worked around\n>> by only minor inconveniences.\n>\n> I am not sure if that inconveniences will be minor. Is this too\n> contrived an example, for example?\n>\n> check () {\n> pattern=$1 file=$2 script=./runme\n>\n> test_i18ngrep \"$pattern\" \"$file\" &&\n> write_script \"$script\" &&\n> test_expect_success \"check $pattern\" '\n> \"$script\"\n> '\n> }\n>\n> check foo file <<-EOF\n> ... test script comes here ...\n> EOF\n\nWith 'test_i18ngrep' outside the 'test_expect_success' block!?\nDefinitely too contrived. :)\n\nOTOH, what about flipping the order of 'test_i18ngrep' and\n'write_script'? If we can't do that, then I wonder what the reason\nmight be that is not too contrived.\n\n\n>>\n>> Signed-off-by: SZEDER Gábor \n>> ---\n>> t/test-lib-functions.sh | 4 ++++\n>> 1 file changed, 4 insertions(+)\n>>\n>> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n>> index 92ed02937..e381d50d0 100644\n>> --- a/t/test-lib-functions.sh\n>> +++ b/t/test-lib-functions.sh\n>> @@ -719,6 +719,10 @@ test_i18ncmp () {\n>> # under GETTEXT_POISON this pretends that the command produced expected\n>> # results.\n>> test_i18ngrep () {\n>> + ( read line ) &&\n>> + error \"bug in the test script: data on test_i18ngrep's stdin;\" \\\n>> + \"perhaps a git command's output is piped into it?\"\n>> +\n>> if test -n \"$GETTEXT_POISON\"\n>> then\n>> : # pretend success\n"},{"id":"337511","messageId":"CAM0VKjn1uzO8JB_0eGV_LHRdrBPgd8rmjwoW1BTJgSV49AOdCA@mail.gmail.com","threadId":"47699","inReplyTo":"xmqq607o8ouy.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 07/10] t: move 'test_i18ncmp' and 'test_i18ngrep' to 'test-lib-functions.sh'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-01-26T19:08:30Z","receivedAt":"2018-01-26T19:08:50Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Jan 26, 2018 at 7:19 PM, Junio C Hamano wrote:\n> SZEDER Gábor writes:\n>\n>> Both 'test_i18ncmp' and 'test_i18ngrep' helper functions are supposed\n>> to be called from our test scripts, so they should be in\n>> 'test-lib-functions.sh'.\n>>\n>> Signed-off-by: SZEDER Gábor \n>> ---\n>> t/test-lib-functions.sh | 26 ++++++++++++++++++++++++++\n>> t/test-lib.sh | 26 --------------------------\n>> 2 files changed, 26 insertions(+), 26 deletions(-)\n>\n> Hmph. I do not care too much either way, but I had an impression\n> that test-lib-functions.sh is meant to be more generic (i.e. those\n> who want can steal it from us and use it in their project without\n> dragging too much of the local convention we employ in this project)\n> than what is in test-lib.sh, which can heavily be specific to Git,\n> and I also had an impression that gettext-poison build is quite a\n> local convention we use in this project, not applicable to other\n> people.\n\nWell, there are a lot of Git-specific functions in\n'test-lib-functions.sh' already:\n\ntest_set_index_version\ntest_tick\ndebug\ntest_commit\ntest_merge\ntest_chmod\ntest_unconfig\ntest_config{,_global}\ntest_cmp_rev\ntest_create_repo\ntest_ln_s_add\ntest_normalize_bool\nnongit\n"},{"id":"337513","messageId":"xmqqo9lg77hc.fsf@gitster.mtv.corp.google.com","threadId":"47699","inReplyTo":"CAM0VKjn401p4fbF-mJrpaQrgOHGHZ1HtRNx9n+CV+jn4n2a1Uw@mail.gmail.com","subject":"Re: [PATCH 08/10] t: forbid piping into 'test_i18ngrep'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-01-26T19:19:59Z","receivedAt":"2018-01-26T19:20:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> With 'test_i18ngrep' outside the 'test_expect_success' block!?\n> Definitely too contrived. :)\n\nWell, I think you got the idea. The point is that test_i18ngrep may\nnot be the only thing that is redirected into, but can just be a\npart of a block of commands, and the \"probing\" read will hurt.\n"},{"id":"337514","messageId":"CAM0VKjm0uveCRpNy8H+inwTKa6fzHAjGn=f9tmQ4p1MCWGuirQ@mail.gmail.com","threadId":"47699","inReplyTo":"xmqqa7x08p0e.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 05/10] t5510: consolidate 'grep' and 'test_i18ngrep' patterns","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-01-26T19:20:02Z","receivedAt":"2018-01-26T19:20:09Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Jan 26, 2018 at 7:16 PM, Junio C Hamano wrote:\n> SZEDER Gábor writes:\n>\n>> One of the tests in 't5510-fetch.sh' checks the output of 'git fetch'\n>> using 'test_i18ngrep', and while doing so it prefilters the output\n>> with 'grep' before piping the result into 'test_i18ngrep'.\n>>\n>> This prefiltering is unnecessary, with the appropriate pattern\n>> 'test_i18ngrep' can do it all by itself. Furthermore, piping data\n>> into 'test_i18ngrep' will interfere with the linting that will be\n>> added in a later patch.\n>\n> It is very likely that the prefiltering \"grep\" will not even see\n> what it is looking for under GETTEXT_POISON build in the first\n> place, so this conversion is the right thing to do from that point\n> of view as well.\n\nNo, GETTEXT_POISON only affects the translated messages, but those\n'grep' invocations looked only at refnames and formatting.\n\nThis is the GETTEXT_POISON-ed output of 'git fetch' in that test\n(probably will get line-wrapped):\n\n# GETTEXT POISON # * # GETTEXT POISON # descriptive-branch ->\nrefs/crazyheads/descriptive-branch\n * # GETTEXT POISON # refs/others/crazy -> crazy\n * # GETTEXT POISON # descriptive-tag -> descriptive-tag\n\n\n>> Signed-off-by: SZEDER Gábor \n>> ---\n>> t/t5510-fetch.sh | 9 +++------\n>> 1 file changed, 3 insertions(+), 6 deletions(-)\n>>\n>> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n>> index 668c54be4..3debc87d4 100755\n>> --- a/t/t5510-fetch.sh\n>> +++ b/t/t5510-fetch.sh\n>> @@ -222,12 +222,9 @@ test_expect_success 'fetch uses remote ref names to describe new refs' '\n>> (\n>> cd descriptive &&\n>> git fetch o 2>actual &&\n>> - grep \" -> refs/crazyheads/descriptive-branch$\" actual |\n>> - test_i18ngrep \"new branch\" &&\n>> - grep \" -> descriptive-tag$\" actual |\n>> - test_i18ngrep \"new tag\" &&\n>> - grep \" -> crazy$\" actual |\n>> - test_i18ngrep \"new ref\"\n>> + test_i18ngrep \"new branch.* -> refs/crazyheads/descriptive-branch$\" actual &&\n>> + test_i18ngrep \"new tag.* -> descriptive-tag$\" actual &&\n>> + test_i18ngrep \"new ref.* -> crazy$\" actual\n>> ) &&\n>> git checkout master\n>> '\n"},{"id":"337515","messageId":"xmqqk1w477c3.fsf@gitster.mtv.corp.google.com","threadId":"47699","inReplyTo":"CAM0VKjm0uveCRpNy8H+inwTKa6fzHAjGn=f9tmQ4p1MCWGuirQ@mail.gmail.com","subject":"Re: [PATCH 05/10] t5510: consolidate 'grep' and 'test_i18ngrep' patterns","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-01-26T19:23:08Z","receivedAt":"2018-01-26T19:23:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> No, GETTEXT_POISON only affects the translated messages, but those\n> 'grep' invocations looked only at refnames and formatting.\n\nYou are right for this specific case, but I was talking more from\ngeneral principle---running test_i18ngrep on an output from grep\nshould be flagged as anti-pattern (I recall vaguly finding an\ninstance not in too distant past).\n"},{"id":"337516","messageId":"CAM0VKj=Qsbkog+rj94bZOk=G-XBsXAqQnRUo4eCXQq2LKjre-w@mail.gmail.com","threadId":"47699","inReplyTo":"20180126185007.GG27618@sigill.intra.peff.net","subject":"Re: [PATCH 10/10] t: make 'test_i18ngrep' more informative on failure","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-01-26T19:23:24Z","receivedAt":"2018-01-26T19:23:36Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Jan 26, 2018 at 7:50 PM, Jeff King wrote:\n> On Fri, Jan 26, 2018 at 01:37:08PM +0100, SZEDER Gábor wrote:\n>\n>> When 'test_i18ngrep' can't find the expected pattern, it exits\n>> completely silently; when its negated form does find the pattern that\n>> shouldn't be there, it prints the matching line(s) but otherwise exits\n>> without any error message. This leaves the developer puzzled about\n>> what could have gone wrong.\n>>\n>> Make 'test_i18ngrep' more informative on failure by printing an error\n>> message including the invoked 'grep' command and the contents of the\n>> file it had to scan through.\n>\n> I think this is an improvement. You can also use \"-x\" to get a better\n> sense of exactly which command failed,\n\nYeah, I know... but I have some issues with running tests with '-x'; I\nsuspect PEBKAC, but haven't yet got around to investigate.\n"},{"id":"337517","messageId":"20180126192532.GA29438@sigill.intra.peff.net","threadId":"47699","inReplyTo":"CAM0VKj=Qsbkog+rj94bZOk=G-XBsXAqQnRUo4eCXQq2LKjre-w@mail.gmail.com","subject":"Re: [PATCH 10/10] t: make 'test_i18ngrep' more informative on failure","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-26T19:25:33Z","receivedAt":"2018-01-26T19:26:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 26, 2018 at 08:23:24PM +0100, SZEDER Gábor wrote:\n\n> On Fri, Jan 26, 2018 at 7:50 PM, Jeff King wrote:\n> > On Fri, Jan 26, 2018 at 01:37:08PM +0100, SZEDER Gábor wrote:\n> >\n> >> When 'test_i18ngrep' can't find the expected pattern, it exits\n> >> completely silently; when its negated form does find the pattern that\n> >> shouldn't be there, it prints the matching line(s) but otherwise exits\n> >> without any error message. This leaves the developer puzzled about\n> >> what could have gone wrong.\n> >>\n> >> Make 'test_i18ngrep' more informative on failure by printing an error\n> >> message including the invoked 'grep' command and the contents of the\n> >> file it had to scan through.\n> >\n> > I think this is an improvement. You can also use \"-x\" to get a better\n> > sense of exactly which command failed,\n> \n> Yeah, I know... but I have some issues with running tests with '-x'; I\n> suspect PEBKAC, but haven't yet got around to investigate.\n\nSome tests absolutely fail with \"-x\", due to them caring about the\nstderr output of shell functions. But with the BASH_XTRACEFD stuff, if\nyou run suite under bash it should al Just Work (and I recently added\nTEST_SHELL_PATH to use bash just for the test suite without building all\nof the scripts with it).\n\n-Peff\n"},{"id":"337546","messageId":"CAM0VKjkXKcwjt1J+KwHYwcaTpb5COXX9ojBWJ4b4b+PRS=AsZQ@mail.gmail.com","threadId":"47699","inReplyTo":"20180126192532.GA29438@sigill.intra.peff.net","subject":"Re: [PATCH 10/10] t: make 'test_i18ngrep' more informative on failure","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-01-26T20:26:38Z","receivedAt":"2018-01-26T20:26:44Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Jan 26, 2018 at 8:25 PM, Jeff King wrote:\n> On Fri, Jan 26, 2018 at 08:23:24PM +0100, SZEDER Gábor wrote:\n>\n>> On Fri, Jan 26, 2018 at 7:50 PM, Jeff King wrote:\n\n>> > You can also use \"-x\" to get a better\n>> > sense of exactly which command failed,\n>>\n>> Yeah, I know... but I have some issues with running tests with '-x'; I\n>> suspect PEBKAC, but haven't yet got around to investigate.\n>\n> Some tests absolutely fail with \"-x\", due to them caring about the\n> stderr output of shell functions. But with the BASH_XTRACEFD stuff, if\n> you run suite under bash it should al Just Work (and I recently added\n> TEST_SHELL_PATH to use bash just for the test suite without building all\n> of the scripts with it).\n\nYeah, I knew about TEST_SHELL_PATH, but still:\n\n $ make -j4 TEST_SHELL_PATH=/bin/bash\n <...>\n $ cd t/\n $ for t in t[0-9][0-9][0-9][0-9]-*.sh ; do \"./$t\" -x ; done >/dev/null 2>&1\n $ grep '^failed [^0]$' test-results/*.counts |wc -l\n 44\n\nThe worst offender is t0008-ignores with 208 tests failing with '-x'...\nI suspect a setup test fails for some reason, and (most of) the other\nfailed tests are just fallout; haven't dared to look yet :)\n"},{"id":"337547","messageId":"20180126203254.GA1767@sigill.intra.peff.net","threadId":"47699","inReplyTo":"CAM0VKjkXKcwjt1J+KwHYwcaTpb5COXX9ojBWJ4b4b+PRS=AsZQ@mail.gmail.com","subject":"Re: [PATCH 10/10] t: make 'test_i18ngrep' more informative on failure","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-26T20:32:54Z","receivedAt":"2018-01-26T20:33:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 26, 2018 at 09:26:38PM +0100, SZEDER Gábor wrote:\n\n> Yeah, I knew about TEST_SHELL_PATH, but still:\n> \n> $ make -j4 TEST_SHELL_PATH=/bin/bash\n> <...>\n> $ cd t/\n> $ for t in t[0-9][0-9][0-9][0-9]-*.sh ; do \"./$t\" -x ; done >/dev/null 2>&1\n> $ grep '^failed [^0]$' test-results/*.counts |wc -l\n> 44\n> \n> The worst offender is t0008-ignores with 208 tests failing with '-x'...\n> I suspect a setup test fails for some reason, and (most of) the other\n> failed tests are just fallout; haven't dared to look yet :)\n\nYou cannot run \"./$t\" and expect TEST_SHELL_PATH to kick in; that starts\nthe test with the #! header, which is always /bin/sh (we don't \"build\"\nthe test scripts like we do regular scripts).\n\nYou need to run either:\n\n - make TEST_SHELL_PATH=/bin/bash test\n\nor\n\n - bash $t -x\n\nThere _is_ one exception where it sometimes works, which is if you use\n--tee or --verbose-log, in which case the shell script has to re-exec\nitself, in which case it always pulls the value from GIT-BUILD-OPTIONS\nto re-exec.\n\n-Peff\n"},{"id":"337555","messageId":"CAPig+cSHQn3va8a47FzPRAzRX_uYKmJLpAmvp9vnv2HYi3G9pg@mail.gmail.com","threadId":"47699","inReplyTo":"20180126123708.21722-10-szeder.dev@gmail.com","subject":"Re: [PATCH 09/10] t: make sure that 'test_i18ngrep' got enough parameters","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-01-26T22:07:07Z","receivedAt":"2018-01-26T22:07:12Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Jan 26, 2018 at 7:37 AM, SZEDER Gábor wrote:\n> Two of the previous patches in this series fixed two bogus\n> 'test_i18ngrep' invocations that had neither a filename parameter not\n\ns/not/nor/\n\n> anything piped into their standard input, yet both managed to remain\n> unnoticed for years. A third similarly bogus invocation is currently\n> lurking in 'pu' for a couple of weeks now.\n>\n> Try to catch similar mistakes in the future by ensuring that\n> 'test_i18ngrep' has at least two parameters, not including an optional\n> '!' to negate the pattern. Perform these checks after we made sure\n> that there is no data on the 'test_i18ngrep's standard input, so if\n> the filename parameter is missing because someone is piping a git\n> command's output into this function, then they would get the more\n> relevant error message.\n>\n> Note that this is not quite perfect, as it doesn't account for any\n> 'grep --options' given as parameters. However, doing so would be far\n> too complicated, considering that patters can start with dashes as\n> well, and in the majority of the cases we don't use any such options\n> anyway.\n>\n> Signed-off-by: SZEDER Gábor \n"},{"id":"337777","messageId":"20180130095017.GA7722@ruderich.org","threadId":"47699","inReplyTo":"20180126123708.21722-3-szeder.dev@gmail.com","subject":"Re: [PATCH 02/10] t5812: add 'test_i18ngrep's missing filename parameter","fromName":"Simon Ruderich","fromEmail":"simon@ruderich.org","sentAt":"2018-01-30T09:50:17Z","receivedAt":"2018-01-30T09:50:26Z","isPatch":true,"sender":{"key":"simon@ruderich.org","avatar":"https://avatars.githubusercontent.com/u/390994?v=4"},"body":"On Fri, Jan 26, 2018 at 01:37:00PM +0100, SZEDER Gábor wrote:\n> [snip]\n>\n> diff --git a/t/t5812-proto-disable-http.sh b/t/t5812-proto-disable-http.sh\n> index d911afd24..226a4920c 100755\n> --- a/t/t5812-proto-disable-http.sh\n> +++ b/t/t5812-proto-disable-http.sh\n> @@ -21,8 +21,7 @@ test_expect_success 'curl redirects respect whitelist' '\n> \t\t\t GIT_SMART_HTTP=0 \\\n> \t\tgit clone \"$HTTPD_URL/ftp-redir/repo.git\" 2>stderr &&\n> \t{\n> -\t\ttest_i18ngrep \"ftp.*disabled\" stderr ||\n> -\t\ttest_i18ngrep \"your curl version is too old\"\n> +\t\ttest_i18ngrep -E \"(ftp.*disabled|your curl version is too old)\" stderr\n> \t}\n\nI think we can drop the curly braces as well, as they were only\nused to group the ||; leaving only:\n\n> +\ttest_i18ngrep -E \"(ftp.*disabled|your curl version is too old)\" stderr\n\nRegards\nSimon\n-- \n+ privacy is necessary\n+ using gnupg http://gnupg.org\n+ public key id: 0x92FEFDB7E44C32F9\n"},{"id":"338623","messageId":"CAM0VKjkf=51i1YPqdNm=pyPHaNNguXLu0T1iHDYv28jW92QTow@mail.gmail.com","threadId":"47699","inReplyTo":"20180126182734.GB27618@sigill.intra.peff.net","subject":"Re: [PATCH 02/10] t5812: add 'test_i18ngrep's missing filename parameter","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-02-07T13:53:17Z","receivedAt":"2018-02-07T13:53:23Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Jan 26, 2018 at 7:27 PM, Jeff King wrote:\n> On Fri, Jan 26, 2018 at 01:37:00PM +0100, SZEDER Gábor wrote:\n>\n>> The second 'test_i18ngrep' invocation in the test 'curl redirects\n>> respect whitelist' is missing its filename parameter. This has\n>> remained unnoticed since its introduction in f4113cac0 (http: limit\n>> redirection to protocol-whitelist, 2015-09-22), because it would only\n>> cause the test to fail if Git was built with a sufficiently old\n>> libcurl version. The test's two ||-chained 'test_i18ngrep'\n>> invocations are supposed to check that either one of the two patterns\n>> is present in 'git clone's error message. As it happens, the first\n>> invocation covers the error message from any reasonably up-to-date\n>> libcurl, thus the second invocation, the one without the filename\n>> parameter, isn't executed at all. Apparently no one has run the test\n>> suite's httpd tests with such an old libcurl in the last 2+ years, or\n>> at least they haven't bothered to notify us about the failed test.\n>\n> Interesting find.\n>\n> The \"too old\" curl is older than 7.19.4, which we actually fail to build\n> with since v2.12.0. So they probably did not even get as far as the\n> tests. ;)\n\nOh, OK, I was not aware of that. The oldest non-maintenance release\nwith the missing filename parameter is v2.7.0, so that's still a 5\nreleases time frame to notice it.\n\nAnyway, I'm preparing v2 of this series, and I'm not sure what to do\nabout this.\n\n - Should I simply drop the \"your curl version is too old\" pattern? It\n would make sense, but it just doesn't feel quite right to remove it\n while the corresponding printf() is still there, even if it can't be\n triggered anymore. However, cleaning up the curl version checks in\n http.c to remove this message is beyond the scope of this patch\n series.\n\n - Or leave it almost-as-is, only dropping the now unnecessary curly\n braces as Simon pointed out. And perhaps a bit of update to the\n commit message.\n\nI'd prefer the second option.\n\n>> Fix this by consolidating the two patterns into a single extended\n>> regexp, eliminating the need for an ||-chained second 'test_i18ngrep'\n>> invocation.\n>\n> OK. Once upon a time I think we had trouble with \"grep -E\", since some\n> older systems had only \"egrep\". But I see we've introduced some \"grep\n> -E\" invocations as far back as 2013 and nobody has complained, so it's\n> probably fine.\n\nYeah, first I went with the more traditional \"\\(this\\|that\\)\" pattern,\nbut then noticed that 'grep -E' is already used in a couple of places,\nand picked the format that uses less escape characters.\n"},{"id":"338626","messageId":"20180207143807.GA27420@sigill.intra.peff.net","threadId":"47699","inReplyTo":"CAM0VKjkf=51i1YPqdNm=pyPHaNNguXLu0T1iHDYv28jW92QTow@mail.gmail.com","subject":"Re: [PATCH 02/10] t5812: add 'test_i18ngrep's missing filename parameter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-07T14:38:07Z","receivedAt":"2018-02-07T14:38:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 07, 2018 at 02:53:17PM +0100, SZEDER Gábor wrote:\n\n> > The \"too old\" curl is older than 7.19.4, which we actually fail to build\n> > with since v2.12.0. So they probably did not even get as far as the\n> > tests. ;)\n> \n> Oh, OK, I was not aware of that. The oldest non-maintenance release\n> with the missing filename parameter is v2.7.0, so that's still a 5\n> releases time frame to notice it.\n\nActually, I'm wrong. It looks like we did finally fix it in f18777ba6e\n(http: fix handling of missing CURLPROTO_*, 2017-08-11), which is in\nv2.15. So:\n\n> Anyway, I'm preparing v2 of this series, and I'm not sure what to do\n> about this.\n> \n> - Should I simply drop the \"your curl version is too old\" pattern? It\n> would make sense, but it just doesn't feel quite right to remove it\n> while the corresponding printf() is still there, even if it can't be\n> triggered anymore. However, cleaning up the curl version checks in\n> http.c to remove this message is beyond the scope of this patch\n> series.\n> \n> - Or leave it almost-as-is, only dropping the now unnecessary curly\n> braces as Simon pointed out. And perhaps a bit of update to the\n> commit message.\n> \n> I'd prefer the second option.\n\nYeah, I think just leave it as-is. Thanks.\n\n-Peff\n"},{"id":"338707","messageId":"20180208155656.9831-1-szeder.dev@gmail.com","threadId":"47699","inReplyTo":"20180126123708.21722-1-szeder.dev@gmail.com","subject":"[PATCH v2 0/9] 'test_i18ngrep'-related fixes and improvements","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-02-08T15:56:47Z","receivedAt":"2018-02-08T15:57:16Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"This is the second version of 'sg/test-i18ngrep'.\n\nTo recap, this patch series fixes a couple of bogus 'test_i18ngrep'\ninvocations (patches 1-4), tries to prevent similar bugs in the future\n(patch 8), teaches 'test_i18ngrep' to be more informative on failure\n(patch 9), and a bit of cleanups in between (patches 5-7).\n\nChanges since the previous version [1]:\n\n - Use Junio's \"last parameter must be file\" suggestion instead of\n trying to read stdin in patch 8.\n - Squashed together the patches validating 'test_i18ngrep's\n parameters (patches 8 and 9), in the hope that this way I can\n better explain that the two checks are not redundant but\n complement each other.\n - Followed Simon's suggestion and dropped the now unnecessary curly\n brackets in patch 2.\n - Dropped a subshell in the last patch. I initially used it to\n prevent the variable $f from leaking into the tests, since we\n can't use the 'local' keyword (yet), but other test helper\n function don't seem to care.\n - Fixed the placements of single quotes and '!' in error messages\n and redirected one more error message to stderr in the last patch.\n - Fixed a couple of typos in commit messages (the one Eric pointed\n out, but later noticed maybe 2-3 more).\n\n\n[1] - https://public-inbox.org/git/20180126123708.21722-1-szeder.dev@gmail.com/T/\n\n\nSZEDER Gábor (9):\n t5541: add 'test_i18ngrep's missing filename parameter\n t5812: add 'test_i18ngrep's missing filename parameter\n t6022: don't run 'git merge' upstream of a pipe\n t4001: don't run 'git status' upstream of a pipe\n t5510: consolidate 'grep' and 'test_i18ngrep' patterns\n t5536: let 'test_i18ngrep' read the file without redirection\n t: move 'test_i18ncmp' and 'test_i18ngrep' to 'test-lib-functions.sh'\n t: validate 'test_i18ngrep's parameters\n t: make 'test_i18ngrep' more informative on failure\n\n t/t4001-diff-rename.sh | 11 ++++++---\n t/t5510-fetch.sh | 9 +++-----\n t/t5536-fetch-conflicts.sh | 2 +-\n t/t5541-http-push-smart.sh | 2 +-\n t/t5812-proto-disable-http.sh | 5 +---\n t/t6022-merge-rename.sh | 6 +++--\n t/test-lib-functions.sh | 54 +++++++++++++++++++++++++++++++++++++++++++\n t/test-lib.sh | 26 ---------------------\n 8 files changed, 72 insertions(+), 43 deletions(-)\n\n-- \n2.16.1.158.ge6451079d\n\n\ndiff --git a/t/t5812-proto-disable-http.sh b/t/t5812-proto-disable-http.sh\nindex 226a4920cd..872788ac8c 100755\n--- a/t/t5812-proto-disable-http.sh\n+++ b/t/t5812-proto-disable-http.sh\n@@ -20,9 +20,7 @@ test_expect_success 'curl redirects respect whitelist' '\n \ttest_must_fail env GIT_ALLOW_PROTOCOL=http:https \\\n \t\t\t GIT_SMART_HTTP=0 \\\n \t\tgit clone \"$HTTPD_URL/ftp-redir/repo.git\" 2>stderr &&\n-\t{\n-\t\ttest_i18ngrep -E \"(ftp.*disabled|your curl version is too old)\" stderr\n-\t}\n+\ttest_i18ngrep -E \"(ftp.*disabled|your curl version is too old)\" stderr\n '\n \n test_expect_success 'curl limits redirects' '\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 1f1d89d7ad..d936ecc0a5 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -719,9 +719,11 @@ test_i18ncmp () {\n # under GETTEXT_POISON this pretends that the command produced expected\n # results.\n test_i18ngrep () {\n-\t( read line ) &&\n-\terror \"bug in the test script: data on test_i18ngrep's stdin;\" \\\n-\t \"perhaps a git command's output is piped into it?\"\n+\teval \"last_arg=\\\"\\${$#}\\\"\"\n+\n+\ttest -f \"$last_arg\" ||\n+\terror \"bug in the test script: test_i18ngrep requires a file\" \\\n+\t \"to read as the last parameter\"\n \n \tif test $# -lt 2 ||\n \t { test \"x!\" = \"x$1\" && test $# -lt 3 ; }\n@@ -740,21 +742,20 @@ test_i18ngrep () {\n \t\tshift\n \t\t! grep \"$@\" && return 0\n \n-\t\techo >&2 \"error: grep '! $@' did find a match in:\"\n+\t\techo >&2 \"error: '! grep $@' did find a match in:\"\n \telse\n \t\tgrep \"$@\" && return 0\n \n-\t\techo >&2 \"error: grep '$@' didn't find a match in:\"\n+\t\techo >&2 \"error: 'grep $@' didn't find a match in:\"\n \tfi\n-\t(\n-\t\teval \"f=\\\"\\${$#}\\\"\"\n-\t\tif test -s \"$f\"\n-\t\tthen\n-\t\t\tcat >&2 \"$f\"\n-\t\telse\n-\t\t\techo \"\"\n-\t\tfi\n-\t)\n+\n+\tif test -s \"$last_arg\"\n+\tthen\n+\t\tcat >&2 \"$last_arg\"\n+\telse\n+\t\techo >&2 \"\"\n+\tfi\n+\n \treturn 1\n }\n \n"},{"id":"338708","messageId":"20180208155656.9831-2-szeder.dev@gmail.com","threadId":"47699","inReplyTo":"20180208155656.9831-1-szeder.dev@gmail.com","subject":"[PATCH v2 1/9] t5541: add 'test_i18ngrep's missing filename parameter","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-02-08T15:56:48Z","receivedAt":"2018-02-08T15:57:18Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The test 'push --no-progress silences progress but not status' runs\n'test_i18ngrep' without specifying a filename parameter. This has\nremained unnoticed since its introduction in e304aeba2 (t5541: test\nmore combinations of --progress, 2012-05-01), because that\n'test_i18ngrep' is supposed to check that the given pattern is not\npresent in its input, and of course it won't find that pattern if its\ninput is empty (as it comes from /dev/null). This also means that\nthis test could miss a potential breakage of 'git push --no-progress'.\n\nSigned-off-by: SZEDER Gábor \n---\n t/t5541-http-push-smart.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh\nindex d38bf32470..21340e89c9 100755\n--- a/t/t5541-http-push-smart.sh\n+++ b/t/t5541-http-push-smart.sh\n@@ -234,7 +234,7 @@ test_expect_success TTY 'push --no-progress silences progress but not status' '\n \ttest_commit no-progress &&\n \ttest_terminal git push --no-progress >output 2>&1 &&\n \ttest_i18ngrep \"^To http\" output &&\n-\ttest_i18ngrep ! \"^Writing objects\"\n+\ttest_i18ngrep ! \"^Writing objects\" output\n '\n \n test_expect_success 'push --progress shows progress to non-tty' '\n-- \n2.16.1.158.ge6451079d\n\n"},{"id":"338709","messageId":"20180208155656.9831-5-szeder.dev@gmail.com","threadId":"47699","inReplyTo":"20180208155656.9831-1-szeder.dev@gmail.com","subject":"[PATCH v2 4/9] t4001: don't run 'git status' upstream of a pipe","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-02-08T15:56:51Z","receivedAt":"2018-02-08T15:57:20Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The primary purpose of three tests in 't4001-diff-rename.sh' is to\ncheck rename detection in 'git status', but all three do so by running\n'git status' upstream of a pipe, hiding its exit code. Consequently,\nthe test could continue even if 'git status' exited with error.\n\nUse an intermediate file between 'git status' and 'test_i18ngrep' to\ncatch a potential failure of the former.\n\nSigned-off-by: SZEDER Gábor \n---\n t/t4001-diff-rename.sh | 11 ++++++++---\n 1 file changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t4001-diff-rename.sh b/t/t4001-diff-rename.sh\nindex eadf4f6244..a07816d560 100755\n--- a/t/t4001-diff-rename.sh\n+++ b/t/t4001-diff-rename.sh\n@@ -134,11 +134,15 @@ test_expect_success 'favour same basenames over different ones' '\n \tgit rm path1 &&\n \tmkdir subdir &&\n \tgit mv another-path subdir/path1 &&\n-\tgit status | test_i18ngrep \"renamed: .*path1 -> subdir/path1\"'\n+\tgit status >out &&\n+\ttest_i18ngrep \"renamed: .*path1 -> subdir/path1\" out\n+'\n \n test_expect_success 'favour same basenames even with minor differences' '\n \tgit show HEAD:path1 | sed \"s/15/16/\" > subdir/path1 &&\n-\tgit status | test_i18ngrep \"renamed: .*path1 -> subdir/path1\"'\n+\tgit status >out &&\n+\ttest_i18ngrep \"renamed: .*path1 -> subdir/path1\" out\n+'\n \n test_expect_success 'two files with same basename and same content' '\n \tgit reset --hard &&\n@@ -148,7 +152,8 @@ test_expect_success 'two files with same basename and same content' '\n \tgit add dir &&\n \tgit commit -m 2 &&\n \tgit mv dir other-dir &&\n-\tgit status | test_i18ngrep \"renamed: .*dir/A/file -> other-dir/A/file\"\n+\tgit status >out &&\n+\ttest_i18ngrep \"renamed: .*dir/A/file -> other-dir/A/file\" out\n '\n \n test_expect_success 'setup for many rename source candidates' '\n-- \n2.16.1.158.ge6451079d\n\n"},{"id":"338710","messageId":"20180208155656.9831-9-szeder.dev@gmail.com","threadId":"47699","inReplyTo":"20180208155656.9831-1-szeder.dev@gmail.com","subject":"[PATCH v2 8/9] t: validate 'test_i18ngrep's parameters","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-02-08T15:56:55Z","receivedAt":"2018-02-08T15:57:23Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Some of the previous patches in this series fixed bogus\n'test_i18ngrep' invocations:\n\n - Two invocations where the tested git command's standard output is\n directly piped into 'test_i18ngrep'. While convenient, this is an\n antipattern, because the pipe hides the git command's exit code,\n and the test could continue even if the command exited with error.\n\n - Two invocations that had neither a filename parameter nor anything\n piped into their standard input, yet both managed to remain\n unnoticed for years. A third similarly bogus invocation is\n currently lurking in 'pu' for a couple of weeks now.\n\nPrevent similar mistakes in the future by validating 'test_i18ngrep's\nparameters requiring that\n\n - The last parameter names an existing file to be read, effectively\n forbiding piping into 'test_i18ngrep'.\n\n Note that this change will also forbid cases where 'test_i18ngrep'\n would legitimately read its standard input, e.g. when its standard\n input is redirected from a file, or when a git command's standard\n output is first written to an intermediate file, which is then\n preprocessed by a non-git command before the results are piped\n into 'test_i18ngrep'. See two of the previous patches for the\n only such cases we had in our test suite. However, reliably\n preventing the piping antipattern is arguably more important than\n supporting these cases, which can be easily worked around by\n opening the file directly or using an intermediate file anyway.\n\n - There are at least two parameters, not including the optional '!'\n to negate the pattern. This ought to catch corner cases when\n 'test_i18ngrep' looks for the name of an existing file on its\n standard input; the above check would miss this case becase the\n filename as pattern would be the last parameter.\n\n Note that this is not quite perfect, as it doesn't account for any\n 'grep --options' given as parameters. However, doing so would be\n far too complicated, considering that patterns can start with\n dashes as well, and in the majority of the cases we don't use any\n such options anyway.\n\nSigned-off-by: SZEDER Gábor \n---\n t/test-lib-functions.sh | 12 ++++++++++++\n 1 file changed, 12 insertions(+)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 92ed029371..a1676e0386 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -719,6 +719,18 @@ test_i18ncmp () {\n # under GETTEXT_POISON this pretends that the command produced expected\n # results.\n test_i18ngrep () {\n+\teval \"last_arg=\\\"\\${$#}\\\"\"\n+\n+\ttest -f \"$last_arg\" ||\n+\terror \"bug in the test script: test_i18ngrep requires a file\" \\\n+\t \"to read as the last parameter\"\n+\n+\tif test $# -lt 2 ||\n+\t { test \"x!\" = \"x$1\" && test $# -lt 3 ; }\n+\tthen\n+\t\terror \"bug in the test script: too few parameters to test_i18ngrep\"\n+\tfi\n+\n \tif test -n \"$GETTEXT_POISON\"\n \tthen\n \t : # pretend success\n-- \n2.16.1.158.ge6451079d\n\n"},{"id":"338711","messageId":"20180208155656.9831-10-szeder.dev@gmail.com","threadId":"47699","inReplyTo":"20180208155656.9831-1-szeder.dev@gmail.com","subject":"[PATCH v2 9/9] t: make 'test_i18ngrep' more informative on failure","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-02-08T15:56:56Z","receivedAt":"2018-02-08T15:57:24Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"When 'test_i18ngrep' can't find the expected pattern, it exits\ncompletely silently; when its negated form does find the pattern that\nshouldn't be there, it prints the matching line(s) but otherwise exits\nwithout any error message. This leaves the developer puzzled about\nwhat could have gone wrong.\n\nMake 'test_i18ngrep' more informative on failure by printing an error\nmessage including the invoked 'grep' command and the contents of the\nfile it had to scan through.\n\nNote that this \"dump the scanned file\" part is not quite perfect, as\nit dumps only the file specified as the function's last positional\nparameter, thus assuming that there is only a single file parameter.\nI think that's a reasonable assumption to make, one that holds true in\nthe current code base. And even if someone were to scan multiple\nfiles at once in the future, the worst thing that could happen is that\nthe verbose error message won't include the contents of all those\nfiles, only the last one. Alas, we can't really do any better than\nthis, because checking whether the other positional parameters match a\nfilename can result in false positives: 't3400-rebase.sh' and\n't3404-rebase-interactive.sh' contain one test each, where the\n'test_i18ngrep's pattern verbatimly matches a file in the trash\ndirectory.\n\nSigned-off-by: SZEDER Gábor \n---\n t/test-lib-functions.sh | 24 ++++++++++++++++++++----\n 1 file changed, 20 insertions(+), 4 deletions(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex a1676e0386..d936ecc0a5 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -733,14 +733,30 @@ test_i18ngrep () {\n \n \tif test -n \"$GETTEXT_POISON\"\n \tthen\n-\t : # pretend success\n-\telif test \"x!\" = \"x$1\"\n+\t\t# pretend success\n+\t\treturn 0\n+\tfi\n+\n+\tif test \"x!\" = \"x$1\"\n \tthen\n \t\tshift\n-\t\t! grep \"$@\"\n+\t\t! grep \"$@\" && return 0\n+\n+\t\techo >&2 \"error: '! grep $@' did find a match in:\"\n \telse\n-\t\tgrep \"$@\"\n+\t\tgrep \"$@\" && return 0\n+\n+\t\techo >&2 \"error: 'grep $@' didn't find a match in:\"\n \tfi\n+\n+\tif test -s \"$last_arg\"\n+\tthen\n+\t\tcat >&2 \"$last_arg\"\n+\telse\n+\t\techo >&2 \"\"\n+\tfi\n+\n+\treturn 1\n }\n \n # Call any command \"$@\" but be more verbose about its\n-- \n2.16.1.158.ge6451079d\n\n"},{"id":"338712","messageId":"20180208155656.9831-6-szeder.dev@gmail.com","threadId":"47699","inReplyTo":"20180208155656.9831-1-szeder.dev@gmail.com","subject":"[PATCH v2 5/9] t5510: consolidate 'grep' and 'test_i18ngrep' patterns","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-02-08T15:56:52Z","receivedAt":"2018-02-08T15:57:28Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"One of the tests in 't5510-fetch.sh' checks the output of 'git fetch'\nusing 'test_i18ngrep', and while doing so it prefilters the output\nwith 'grep' before piping the result into 'test_i18ngrep'.\n\nThis prefiltering is unnecessary, with the appropriate pattern\n'test_i18ngrep' can do it all by itself. Furthermore, piping data\ninto 'test_i18ngrep' will interfere with the linting that will be\nadded in a later patch.\n\nSigned-off-by: SZEDER Gábor \n---\n t/t5510-fetch.sh | 9 +++------\n 1 file changed, 3 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 668c54be41..3debc87d4a 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -222,12 +222,9 @@ test_expect_success 'fetch uses remote ref names to describe new refs' '\n \t(\n \t\tcd descriptive &&\n \t\tgit fetch o 2>actual &&\n-\t\tgrep \" -> refs/crazyheads/descriptive-branch$\" actual |\n-\t\ttest_i18ngrep \"new branch\" &&\n-\t\tgrep \" -> descriptive-tag$\" actual |\n-\t\ttest_i18ngrep \"new tag\" &&\n-\t\tgrep \" -> crazy$\" actual |\n-\t\ttest_i18ngrep \"new ref\"\n+\t\ttest_i18ngrep \"new branch.* -> refs/crazyheads/descriptive-branch$\" actual &&\n+\t\ttest_i18ngrep \"new tag.* -> descriptive-tag$\" actual &&\n+\t\ttest_i18ngrep \"new ref.* -> crazy$\" actual\n \t) &&\n \tgit checkout master\n '\n-- \n2.16.1.158.ge6451079d\n\n"},{"id":"338713","messageId":"20180208155656.9831-7-szeder.dev@gmail.com","threadId":"47699","inReplyTo":"20180208155656.9831-1-szeder.dev@gmail.com","subject":"[PATCH v2 6/9] t5536: let 'test_i18ngrep' read the file without redirection","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-02-08T15:56:53Z","receivedAt":"2018-02-08T15:57:30Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Redirecting 'test_i18ngrep's standard input from a file will interfere\nwith the linting that will be added in a later patch.\n\nSigned-off-by: SZEDER Gábor \n---\n t/t5536-fetch-conflicts.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t5536-fetch-conflicts.sh b/t/t5536-fetch-conflicts.sh\nindex 2e42cf3316..644736b8a3 100755\n--- a/t/t5536-fetch-conflicts.sh\n+++ b/t/t5536-fetch-conflicts.sh\n@@ -22,7 +22,7 @@ verify_stderr () {\n \tcat >expected &&\n \t# We're not interested in the error\n \t# \"fatal: The remote end hung up unexpectedly\":\n-\ttest_i18ngrep -E '^(fatal|warning):' actual | sort &&\n+\ttest_i18ngrep -E '^(fatal|warning):' error | grep -v 'hung up' >actual | sort &&\n \ttest_i18ncmp expected actual\n }\n \n-- \n2.16.1.158.ge6451079d\n\n"},{"id":"338714","messageId":"20180208155656.9831-8-szeder.dev@gmail.com","threadId":"47699","inReplyTo":"20180208155656.9831-1-szeder.dev@gmail.com","subject":"[PATCH v2 7/9] t: move 'test_i18ncmp' and 'test_i18ngrep' to 'test-lib-functions.sh'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-02-08T15:56:54Z","receivedAt":"2018-02-08T15:57:31Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Both 'test_i18ncmp' and 'test_i18ngrep' helper functions are supposed\nto be called from our test scripts, so they should be in\n'test-lib-functions.sh'.\n\nSigned-off-by: SZEDER Gábor \n---\n t/test-lib-functions.sh | 26 ++++++++++++++++++++++++++\n t/test-lib.sh | 26 --------------------------\n 2 files changed, 26 insertions(+), 26 deletions(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 1701fe2a06..92ed029371 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -705,6 +705,32 @@ test_cmp_bin() {\n \tcmp \"$@\"\n }\n \n+# Use this instead of test_cmp to compare files that contain expected and\n+# actual output from git commands that can be translated. When running\n+# under GETTEXT_POISON this pretends that the command produced expected\n+# results.\n+test_i18ncmp () {\n+\ttest -n \"$GETTEXT_POISON\" || test_cmp \"$@\"\n+}\n+\n+# Use this instead of \"grep expected-string actual\" to see if the\n+# output from a git command that can be translated either contains an\n+# expected string, or does not contain an unwanted one. When running\n+# under GETTEXT_POISON this pretends that the command produced expected\n+# results.\n+test_i18ngrep () {\n+\tif test -n \"$GETTEXT_POISON\"\n+\tthen\n+\t : # pretend success\n+\telif test \"x!\" = \"x$1\"\n+\tthen\n+\t\tshift\n+\t\t! grep \"$@\"\n+\telse\n+\t\tgrep \"$@\"\n+\tfi\n+}\n+\n # Call any command \"$@\" but be more verbose about its\n # failure. This is handy for commands like \"test\" which do\n # not output anything when they fail.\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 9a0a21f49a..852b22c80a 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1062,32 +1062,6 @@ else\n \ttest_set_prereq C_LOCALE_OUTPUT\n fi\n \n-# Use this instead of test_cmp to compare files that contain expected and\n-# actual output from git commands that can be translated. When running\n-# under GETTEXT_POISON this pretends that the command produced expected\n-# results.\n-test_i18ncmp () {\n-\ttest -n \"$GETTEXT_POISON\" || test_cmp \"$@\"\n-}\n-\n-# Use this instead of \"grep expected-string actual\" to see if the\n-# output from a git command that can be translated either contains an\n-# expected string, or does not contain an unwanted one. When running\n-# under GETTEXT_POISON this pretends that the command produced expected\n-# results.\n-test_i18ngrep () {\n-\tif test -n \"$GETTEXT_POISON\"\n-\tthen\n-\t : # pretend success\n-\telif test \"x!\" = \"x$1\"\n-\tthen\n-\t\tshift\n-\t\t! grep \"$@\"\n-\telse\n-\t\tgrep \"$@\"\n-\tfi\n-}\n-\n test_lazy_prereq PIPE '\n \t# test whether the filesystem supports FIFOs\n \ttest_have_prereq !MINGW,!CYGWIN &&\n-- \n2.16.1.158.ge6451079d\n\n"},{"id":"338715","messageId":"20180208155656.9831-3-szeder.dev@gmail.com","threadId":"47699","inReplyTo":"20180208155656.9831-1-szeder.dev@gmail.com","subject":"[PATCH v2 2/9] t5812: add 'test_i18ngrep's missing filename parameter","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-02-08T15:56:49Z","receivedAt":"2018-02-08T15:57:37Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The second 'test_i18ngrep' invocation in the test 'curl redirects\nrespect whitelist' is missing its filename parameter. This has\nremained unnoticed since its introduction in f4113cac0 (http: limit\nredirection to protocol-whitelist, 2015-09-22), because it would only\ncause the test to fail if Git was built with a sufficiently old\nlibcurl version. The test's two ||-chained 'test_i18ngrep'\ninvocations are supposed to check that either one of the two patterns\nis present in 'git clone's error message. As it happens, the first\ninvocation covers the error message from any reasonably up-to-date\nlibcurl, thus the second invocation, the one without the filename\nparameter, isn't executed at all. Apparently no one has run the test\nsuite's httpd tests with such an old libcurl in the last 2+ years, or\nat least they haven't bothered to notify us about the failed test.\n\nFix this by consolidating the two patterns into a single extended\nregexp, eliminating the need for an ||-chained second 'test_i18ngrep'\ninvocation.\n\nSigned-off-by: SZEDER Gábor \n---\n t/t5812-proto-disable-http.sh | 5 +----\n 1 file changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/t/t5812-proto-disable-http.sh b/t/t5812-proto-disable-http.sh\nindex d911afd24c..872788ac8c 100755\n--- a/t/t5812-proto-disable-http.sh\n+++ b/t/t5812-proto-disable-http.sh\n@@ -20,10 +20,7 @@ test_expect_success 'curl redirects respect whitelist' '\n \ttest_must_fail env GIT_ALLOW_PROTOCOL=http:https \\\n \t\t\t GIT_SMART_HTTP=0 \\\n \t\tgit clone \"$HTTPD_URL/ftp-redir/repo.git\" 2>stderr &&\n-\t{\n-\t\ttest_i18ngrep \"ftp.*disabled\" stderr ||\n-\t\ttest_i18ngrep \"your curl version is too old\"\n-\t}\n+\ttest_i18ngrep -E \"(ftp.*disabled|your curl version is too old)\" stderr\n '\n \n test_expect_success 'curl limits redirects' '\n-- \n2.16.1.158.ge6451079d\n\n"},{"id":"338716","messageId":"20180208155656.9831-4-szeder.dev@gmail.com","threadId":"47699","inReplyTo":"20180208155656.9831-1-szeder.dev@gmail.com","subject":"[PATCH v2 3/9] t6022: don't run 'git merge' upstream of a pipe","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-02-08T15:56:50Z","receivedAt":"2018-02-08T15:57:39Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The primary purpose of 't6022-merge-rename.sh' is to test 'git merge',\nbut one of the tests runs it upstream of a pipe, hiding its exit code.\nConsequently, the test could continue even if 'git merge' exited with\nerror.\n\nUse an intermediate file between 'git merge' and 'test_i18ngrep' to\ncatch a potential failure of the former.\n\nSigned-off-by: SZEDER Gábor \n---\n t/t6022-merge-rename.sh | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t6022-merge-rename.sh b/t/t6022-merge-rename.sh\nindex 05ebba7afa..c01f721f13 100755\n--- a/t/t6022-merge-rename.sh\n+++ b/t/t6022-merge-rename.sh\n@@ -242,10 +242,12 @@ test_expect_success 'merge of identical changes in a renamed file' '\n \trm -f A M N &&\n \tgit reset --hard &&\n \tgit checkout change+rename &&\n-\tGIT_MERGE_VERBOSITY=3 git merge change | test_i18ngrep \"^Skipped B\" &&\n+\tGIT_MERGE_VERBOSITY=3 git merge change >out &&\n+\ttest_i18ngrep \"^Skipped B\" out &&\n \tgit reset --hard HEAD^ &&\n \tgit checkout change &&\n-\tGIT_MERGE_VERBOSITY=3 git merge change+rename | test_i18ngrep \"^Skipped B\"\n+\tGIT_MERGE_VERBOSITY=3 git merge change+rename >out &&\n+\ttest_i18ngrep \"^Skipped B\" out\n '\n \n test_expect_success 'setup for rename + d/f conflicts' '\n-- \n2.16.1.158.ge6451079d\n\n"},{"id":"338738","messageId":"20180208163416.GA13078@sigill.intra.peff.net","threadId":"47699","inReplyTo":"20180208155656.9831-9-szeder.dev@gmail.com","subject":"Re: [PATCH v2 8/9] t: validate 'test_i18ngrep's parameters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-08T16:34:16Z","receivedAt":"2018-02-08T16:34:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 08, 2018 at 04:56:55PM +0100, SZEDER Gábor wrote:\n\n> Prevent similar mistakes in the future by validating 'test_i18ngrep's\n> parameters requiring that\n> \n> - The last parameter names an existing file to be read, effectively\n> forbiding piping into 'test_i18ngrep'.\n\ns/forbiding/forbidding/\n\n> Note that this change will also forbid cases where 'test_i18ngrep'\n> would legitimately read its standard input, e.g. when its standard\n> input is redirected from a file, or when a git command's standard\n> output is first written to an intermediate file, which is then\n> preprocessed by a non-git command before the results are piped\n> into 'test_i18ngrep'. See two of the previous patches for the\n> only such cases we had in our test suite. However, reliably\n> preventing the piping antipattern is arguably more important than\n> supporting these cases, which can be easily worked around by\n> opening the file directly or using an intermediate file anyway.\n> \n> - There are at least two parameters, not including the optional '!'\n> to negate the pattern. This ought to catch corner cases when\n> 'test_i18ngrep' looks for the name of an existing file on its\n> standard input; the above check would miss this case becase the\n> filename as pattern would be the last parameter.\n> \n> Note that this is not quite perfect, as it doesn't account for any\n> 'grep --options' given as parameters. However, doing so would be\n> far too complicated, considering that patterns can start with\n> dashes as well, and in the majority of the cases we don't use any\n> such options anyway.\n\nAnd most importantly, we never err on the side of complaining\nunnecessarily. So our safety might not kick in, but as long as it kicks\nin most of the time, we're fine.\n\nI like this approach much better than the previous round.\n\n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index 92ed029371..a1676e0386 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -719,6 +719,18 @@ test_i18ncmp () {\n> # under GETTEXT_POISON this pretends that the command produced expected\n> # results.\n> test_i18ngrep () {\n> +\teval \"last_arg=\\\"\\${$#}\\\"\"\n\nThese embedded double-quotes are unnecessary, I think, because it's a\nvariable assignment: E.g.:\n\n set -- one two 'foo bar'\n eval \"last_arg=\\${$#}\"\n echo $last_arg\n\nshould produce \"foo bar\".\n\nUsually not a big deal, but because of the extra quoting it may make the\nwhole thing a bit more readable to drop them.\n\n-Peff\n"},{"id":"338739","messageId":"20180208163647.GB13078@sigill.intra.peff.net","threadId":"47699","inReplyTo":"20180208155656.9831-1-szeder.dev@gmail.com","subject":"Re: [PATCH v2 0/9] 'test_i18ngrep'-related fixes and improvements","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-08T16:36:47Z","receivedAt":"2018-02-08T16:36:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 08, 2018 at 04:56:47PM +0100, SZEDER Gábor wrote:\n\n> This is the second version of 'sg/test-i18ngrep'.\n> \n> To recap, this patch series fixes a couple of bogus 'test_i18ngrep'\n> invocations (patches 1-4), tries to prevent similar bugs in the future\n> (patch 8), teaches 'test_i18ngrep' to be more informative on failure\n> (patch 9), and a bit of cleanups in between (patches 5-7).\n> \n> Changes since the previous version [1]:\n> [...]\n\nThis round looks good to me. I left a very minor comment on patch 8, but\notherwise didn't find anything wrong. Thanks.\n\n-Peff\n"}]}