{"thread":{"id":"40836","subject":"[PATCH 1/2] t1450: add tests for NUL in headers of commits and tags","startedAt":"2015-11-19T16:20:14Z","lastAt":"2015-11-21T09:35:24Z","messageCount":14,"participants":["René Scharfe","Eric Sunshine","Jeff King","Johannes Schindelin","SZEDER Gábor","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"273506","messageId":"564DF6BE.6020609@web.de","threadId":"40836","inReplyTo":null,"subject":"[PATCH 1/2] t1450: add tests for NUL in headers of commits and tags","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2015-11-19T16:20:14Z","receivedAt":"2015-11-19T16:20:14Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Signed-off-by: Rene Scharfe <l.s.r@web.de>\n---\n t/t1450-fsck.sh | 32 ++++++++++++++++++++++++++++++++\n 1 file changed, 32 insertions(+)\n\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex dc09797..6c96953 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -176,6 +176,18 @@ test_expect_success 'integer overflow in timestamps is reported' '\n \tgrep \"error in commit $new.*integer overflow\" out\n '\n \n+test_expect_success 'commit with NUL in header' '\n+\tgit cat-file commit HEAD >basis &&\n+\tsed \"s/author ./author Q/\" <basis | q_to_nul >commit-NUL-header &&\n+\tnew=$(git hash-object -t commit -w --stdin <commit-NUL-header) &&\n+\ttest_when_finished \"remove_object $new\" &&\n+\tgit update-ref refs/heads/bogus \"$new\" &&\n+\ttest_when_finished \"git update-ref -d refs/heads/bogus\" &&\n+\ttest_must_fail git fsck 2>out &&\n+\tcat out &&\n+\tgrep \"error in commit $new.*unterminated header: NUL at offset\" out\n+'\n+\n test_expect_success 'malformatted tree object' '\n \ttest_when_finished \"git update-ref -d refs/tags/wrong\" &&\n \ttest_when_finished \"remove_object \\$T\" &&\n@@ -276,6 +288,26 @@ test_expect_success 'tag with bad tagger' '\n \tgrep \"error in tag .*: invalid author/committer\" out\n '\n \n+test_expect_failure 'tag with NUL in header' '\n+\tsha=$(git rev-parse HEAD) &&\n+\tq_to_nul >tag-NUL-header <<-EOF &&\n+\tobject $sha\n+\ttype commit\n+\ttag contains-Q-in-header\n+\ttagger T A Gger <tagger@example.com> 1234567890 -0000\n+\n+\tThis is an invalid tag.\n+\tEOF\n+\n+\ttag=$(git hash-object --literally -t tag -w --stdin <tag-NUL-header) &&\n+\ttest_when_finished \"remove_object $tag\" &&\n+\techo $tag >.git/refs/tags/wrong &&\n+\ttest_when_finished \"git update-ref -d refs/tags/wrong\" &&\n+\ttest_must_fail git fsck --tags 2>out &&\n+\tcat out &&\n+\tgrep \"error in tag $tag.*unterminated header: NUL at offset\" out\n+'\n+\n test_expect_success 'cleaned up' '\n \tgit fsck >actual 2>&1 &&\n \ttest_cmp empty actual\n-- \n2.6.3\n"},{"id":"273507","messageId":"564DF7FB.3090704@web.de","threadId":"40836","inReplyTo":"564DF6BE.6020609@web.de","subject":"[PATCH 2/2] fsck: treat a NUL in a tag header as an error","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2015-11-19T16:25:31Z","receivedAt":"2015-11-19T16:25:31Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"We check the return value of verify_header() for commits already, so do\nthe same for tags as well.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n fsck.c          | 3 ++-\n t/t1450-fsck.sh | 2 +-\n 2 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex e41e753..4060f1f 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -711,7 +711,8 @@ static int fsck_tag_buffer(struct tag *tag, const char *data,\n \t\t}\n \t}\n \n-\tif (verify_headers(buffer, size, &tag->object, options))\n+\tret = verify_headers(buffer, size, &tag->object, options);\n+\tif (ret)\n \t\tgoto done;\n \n \tif (!skip_prefix(buffer, \"object \", &buffer)) {\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 6c96953..e66b7cb 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -288,7 +288,7 @@ test_expect_success 'tag with bad tagger' '\n \tgrep \"error in tag .*: invalid author/committer\" out\n '\n \n-test_expect_failure 'tag with NUL in header' '\n+test_expect_success 'tag with NUL in header' '\n \tsha=$(git rev-parse HEAD) &&\n \tq_to_nul >tag-NUL-header <<-EOF &&\n \tobject $sha\n-- \n2.6.3\n"},{"id":"273516","messageId":"CAPig+cSXiGVLvq59sUFAzTRoQFf-QswAoMSk20LvJqntzaf65A@mail.gmail.com","threadId":"40836","inReplyTo":"564DF6BE.6020609@web.de","subject":"Re: [PATCH 1/2] t1450: add tests for NUL in headers of commits and tags","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-11-19T20:33:04Z","receivedAt":"2015-11-19T20:33:04Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Nov 19, 2015 at 11:20 AM, René Scharfe <l.s.r@web.de> wrote:\n> Signed-off-by: Rene Scharfe <l.s.r@web.de>\n> ---\n> diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\n> @@ -176,6 +176,18 @@ test_expect_success 'integer overflow in timestamps is reported' '\n> +test_expect_success 'commit with NUL in header' '\n> +       git cat-file commit HEAD >basis &&\n> +       sed \"s/author ./author Q/\" <basis | q_to_nul >commit-NUL-header &&\n> +       new=$(git hash-object -t commit -w --stdin <commit-NUL-header) &&\n> +       test_when_finished \"remove_object $new\" &&\n> +       git update-ref refs/heads/bogus \"$new\" &&\n> +       test_when_finished \"git update-ref -d refs/heads/bogus\" &&\n> +       test_must_fail git fsck 2>out &&\n> +       cat out &&\n\nWhat is the purpose of this 'cat'?\n\n> +       grep \"error in commit $new.*unterminated header: NUL at offset\" out\n> +'\n> @@ -276,6 +288,26 @@ test_expect_success 'tag with bad tagger' '\n> +test_expect_failure 'tag with NUL in header' '\n> +       sha=$(git rev-parse HEAD) &&\n> +       q_to_nul >tag-NUL-header <<-EOF &&\n> +       object $sha\n> +       type commit\n> +       tag contains-Q-in-header\n> +       tagger T A Gger <tagger@example.com> 1234567890 -0000\n> +\n> +       This is an invalid tag.\n> +       EOF\n> +\n> +       tag=$(git hash-object --literally -t tag -w --stdin <tag-NUL-header) &&\n> +       test_when_finished \"remove_object $tag\" &&\n> +       echo $tag >.git/refs/tags/wrong &&\n> +       test_when_finished \"git update-ref -d refs/tags/wrong\" &&\n> +       test_must_fail git fsck --tags 2>out &&\n> +       cat out &&\n\nDitto.\n\n> +       grep \"error in tag $tag.*unterminated header: NUL at offset\" out\n> +'\n> +\n> --\n> 2.6.3\n"},{"id":"273517","messageId":"564E371E.50606@web.de","threadId":"40836","inReplyTo":"CAPig+cSXiGVLvq59sUFAzTRoQFf-QswAoMSk20LvJqntzaf65A@mail.gmail.com","subject":"Re: [PATCH 1/2] t1450: add tests for NUL in headers of commits and tags","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2015-11-19T20:54:54Z","receivedAt":"2015-11-19T20:54:54Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 19.11.2015 um 21:33 schrieb Eric Sunshine:\n> On Thu, Nov 19, 2015 at 11:20 AM, René Scharfe <l.s.r@web.de> wrote:\n>> Signed-off-by: Rene Scharfe <l.s.r@web.de>\n>> ---\n>> diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\n>> @@ -176,6 +176,18 @@ test_expect_success 'integer overflow in timestamps is reported' '\n>> +test_expect_success 'commit with NUL in header' '\n>> +       git cat-file commit HEAD >basis &&\n>> +       sed \"s/author ./author Q/\" <basis | q_to_nul >commit-NUL-header &&\n>> +       new=$(git hash-object -t commit -w --stdin <commit-NUL-header) &&\n>> +       test_when_finished \"remove_object $new\" &&\n>> +       git update-ref refs/heads/bogus \"$new\" &&\n>> +       test_when_finished \"git update-ref -d refs/heads/bogus\" &&\n>> +       test_must_fail git fsck 2>out &&\n>> +       cat out &&\n>\n> What is the purpose of this 'cat'?\n\nIt shows the full error message when the test is run with --debug, which \nis convenient when the following grep doesn't match.  The same is done \nin most tests in that file.\n\n>\n>> +       grep \"error in commit $new.*unterminated header: NUL at offset\" out\n>> +'\n>> @@ -276,6 +288,26 @@ test_expect_success 'tag with bad tagger' '\n>> +test_expect_failure 'tag with NUL in header' '\n>> +       sha=$(git rev-parse HEAD) &&\n>> +       q_to_nul >tag-NUL-header <<-EOF &&\n>> +       object $sha\n>> +       type commit\n>> +       tag contains-Q-in-header\n>> +       tagger T A Gger <tagger@example.com> 1234567890 -0000\n>> +\n>> +       This is an invalid tag.\n>> +       EOF\n>> +\n>> +       tag=$(git hash-object --literally -t tag -w --stdin <tag-NUL-header) &&\n>> +       test_when_finished \"remove_object $tag\" &&\n>> +       echo $tag >.git/refs/tags/wrong &&\n>> +       test_when_finished \"git update-ref -d refs/tags/wrong\" &&\n>> +       test_must_fail git fsck --tags 2>out &&\n>> +       cat out &&\n>\n> Ditto.\n>\n>> +       grep \"error in tag $tag.*unterminated header: NUL at offset\" out\n>> +'\n>> +\n>> --\n>> 2.6.3\n"},{"id":"273529","messageId":"20151120111312.GA11198@sigill.intra.peff.net","threadId":"40836","inReplyTo":"564DF7FB.3090704@web.de","subject":"Re: [PATCH 2/2] fsck: treat a NUL in a tag header as an error","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-11-20T11:13:14Z","receivedAt":"2015-11-20T11:13:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 19, 2015 at 05:25:31PM +0100, René Scharfe wrote:\n\n> We check the return value of verify_header() for commits already, so do\n> the same for tags as well.\n> \n> Signed-off-by: Rene Scharfe <l.s.r@web.de>\n> ---\n>  fsck.c          | 3 ++-\n>  t/t1450-fsck.sh | 2 +-\n>  2 files changed, 3 insertions(+), 2 deletions(-)\n> \n> diff --git a/fsck.c b/fsck.c\n> index e41e753..4060f1f 100644\n> --- a/fsck.c\n> +++ b/fsck.c\n> @@ -711,7 +711,8 @@ static int fsck_tag_buffer(struct tag *tag, const char *data,\n>  \t\t}\n>  \t}\n>  \n> -\tif (verify_headers(buffer, size, &tag->object, options))\n> +\tret = verify_headers(buffer, size, &tag->object, options);\n> +\tif (ret)\n>  \t\tgoto done;\n\nGood catch. The patch look reasonable to me.\n\n-Peff\n"},{"id":"273530","messageId":"20151120111454.GB11198@sigill.intra.peff.net","threadId":"40836","inReplyTo":"564E371E.50606@web.de","subject":"Re: [PATCH 1/2] t1450: add tests for NUL in headers of commits and tags","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-11-20T11:14:55Z","receivedAt":"2015-11-20T11:14:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 19, 2015 at 09:54:54PM +0100, René Scharfe wrote:\n\n> >>diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\n> >>@@ -176,6 +176,18 @@ test_expect_success 'integer overflow in timestamps is reported' '\n> >>+test_expect_success 'commit with NUL in header' '\n> >>+       git cat-file commit HEAD >basis &&\n> >>+       sed \"s/author ./author Q/\" <basis | q_to_nul >commit-NUL-header &&\n> >>+       new=$(git hash-object -t commit -w --stdin <commit-NUL-header) &&\n> >>+       test_when_finished \"remove_object $new\" &&\n> >>+       git update-ref refs/heads/bogus \"$new\" &&\n> >>+       test_when_finished \"git update-ref -d refs/heads/bogus\" &&\n> >>+       test_must_fail git fsck 2>out &&\n> >>+       cat out &&\n> >\n> >What is the purpose of this 'cat'?\n> \n> It shows the full error message when the test is run with --debug, which is\n> convenient when the following grep doesn't match.  The same is done in most\n> tests in that file.\n\nI'm slightly negative on such a construct, just because it wastes a\nprocess in the case where we are not in --verbose mode. I don't mind it\nin this patch in the spirit of consistency within t1450, but I think we\nshould probably avoid spreading it.\n\n-Peff\n"},{"id":"273553","messageId":"alpine.DEB.1.00.1511202118340.1686@s15462909.onlinehome-server.info","threadId":"40836","inReplyTo":"564DF7FB.3090704@web.de","subject":"Re: [PATCH 2/2] fsck: treat a NUL in a tag header as an error","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-11-20T20:18:52Z","receivedAt":"2015-11-20T20:18:52Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi René,\n\nOn Thu, 19 Nov 2015, René Scharfe wrote:\n\n> We check the return value of verify_header() for commits already, so do\n> the same for tags as well.\n\nThanks for catching and fixing my bug!\n\nCiao,\nDscho"},{"id":"273556","messageId":"564F8776.1050706@web.de","threadId":"40836","inReplyTo":"20151120111454.GB11198@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] t1450: add tests for NUL in headers of commits and tags","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2015-11-20T20:49:58Z","receivedAt":"2015-11-20T20:49:58Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 20.11.2015 um 12:14 schrieb Jeff King:\n> On Thu, Nov 19, 2015 at 09:54:54PM +0100, René Scharfe wrote:\n>\n>>>> diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\n>>>> @@ -176,6 +176,18 @@ test_expect_success 'integer overflow in timestamps is reported' '\n>>>> +test_expect_success 'commit with NUL in header' '\n>>>> +       git cat-file commit HEAD >basis &&\n>>>> +       sed \"s/author ./author Q/\" <basis | q_to_nul >commit-NUL-header &&\n>>>> +       new=$(git hash-object -t commit -w --stdin <commit-NUL-header) &&\n>>>> +       test_when_finished \"remove_object $new\" &&\n>>>> +       git update-ref refs/heads/bogus \"$new\" &&\n>>>> +       test_when_finished \"git update-ref -d refs/heads/bogus\" &&\n>>>> +       test_must_fail git fsck 2>out &&\n>>>> +       cat out &&\n>>>\n>>> What is the purpose of this 'cat'?\n>>\n>> It shows the full error message when the test is run with --debug, which is\n>> convenient when the following grep doesn't match.  The same is done in most\n>> tests in that file.\n>\n> I'm slightly negative on such a construct, just because it wastes a\n> process in the case where we are not in --verbose mode. I don't mind it\n> in this patch in the spirit of consistency within t1450, but I think we\n> should probably avoid spreading it.\n\nThis practice is not used that much (yet).  We can contain it by\nproviding an alternative.  I'll send patches for adding a helper\nfunction similar to test_must_be_empty for that.\n\nRené\n"},{"id":"273557","messageId":"564F8781.1080902@web.de","threadId":"40836","inReplyTo":"20151120111454.GB11198@sigill.intra.peff.net","subject":"[PATCH 3/2] test: factor out helper function test_must_contain","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2015-11-20T20:50:09Z","receivedAt":"2015-11-20T20:50:09Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Extract a helper function for searching for a pattern in a file and\nprinting the whole file if the pattern is not found.  It is useful\nwhen starting tests with --verbose for debugging purposes.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n t/t0008-ignores.sh      | 10 +---------\n t/test-lib-functions.sh | 12 ++++++++++++\n 2 files changed, 13 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh\nindex 4ef5ed4..0414c01 100755\n--- a/t/t0008-ignores.sh\n+++ b/t/t0008-ignores.sh\n@@ -46,15 +46,7 @@ broken_c_unquote_verbose () {\n }\n \n stderr_contains () {\n-\tregexp=\"$1\"\n-\tif grep \"$regexp\" \"$HOME/stderr\"\n-\tthen\n-\t\treturn 0\n-\telse\n-\t\techo \"didn't find /$regexp/ in $HOME/stderr\"\n-\t\tcat \"$HOME/stderr\"\n-\t\treturn 1\n-\tfi\n+\ttest_must_contain \"$1\" \"$HOME/stderr\"\n }\n \n stderr_empty_on_success () {\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 73e37a1..eb44cb4 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -689,6 +689,18 @@ test_must_be_empty () {\n \tfi\n }\n \n+# Check if a file contains an expected pattern.\n+test_must_contain () {\n+\tif grep \"$1\" \"$2\"\n+\tthen\n+\t\treturn 0\n+\telse\n+\t\techo \"didn't find /$1/ in '$2', it contains:\"\n+\t\tcat \"$2\"\n+\t\treturn 1\n+\tfi\n+}\n+\n # Tests that its two parameters refer to the same revision\n test_cmp_rev () {\n \tgit rev-parse --verify \"$1\" >expect.rev &&\n-- \n2.6.3\n"},{"id":"273558","messageId":"564F878B.3090508@web.de","threadId":"40836","inReplyTo":"20151120111454.GB11198@sigill.intra.peff.net","subject":"[PATCH 4/2] test: use test_must_contain","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2015-11-20T20:50:19Z","receivedAt":"2015-11-20T20:50:19Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Some tests print a file using cat and then grep it for some pattern.\nThis is done to aid debugging in case the pattern is unexpectedly not\nfound; the file contents can easily inspected by running the test with\nthe option --verbose.\n\nReplace these combinations by calls of test_must_contain.  It only\nshows the whole file if the pattern is not found, saving a cat call\n(and thus a fork(2)) in the normal case.  It also just needs a single\nline and makes the intent clearer.\n\nThe flag -q in t5801 is dropped as there's no pressing reason for\nhiding the matching line in this test.  Consistency is more important,\nand none of the remaining grep calls in that file use that flag or\nredirect the results to /dev/null.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n t/t1450-fsck.sh           | 52 ++++++++++++++++-------------------------------\n t/t5510-fetch.sh          |  3 +--\n t/t5801-remote-helpers.sh |  3 +--\n t/t9810-git-p4-rcs.sh     |  3 +--\n 4 files changed, 21 insertions(+), 40 deletions(-)\n\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex e66b7cb..190c97f 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -70,16 +70,14 @@ test_expect_success 'object with bad sha1' '\n \ttest_when_finished \"git update-ref -d refs/heads/bogus\" &&\n \n \ttest_must_fail git fsck 2>out &&\n-\tcat out &&\n-\tgrep \"$sha.*corrupt\" out\n+\ttest_must_contain \"$sha.*corrupt\" out\n '\n \n test_expect_success 'branch pointing to non-commit' '\n \tgit rev-parse HEAD^{tree} >.git/refs/heads/invalid &&\n \ttest_when_finished \"git update-ref -d refs/heads/invalid\" &&\n \ttest_must_fail git fsck 2>out &&\n-\tcat out &&\n-\tgrep \"not a commit\" out\n+\ttest_must_contain \"not a commit\" out\n '\n \n test_expect_success 'HEAD link pointing at a funny object' '\n@@ -88,8 +86,7 @@ test_expect_success 'HEAD link pointing at a funny object' '\n \techo 0000000000000000000000000000000000000000 >.git/HEAD &&\n \t# avoid corrupt/broken HEAD from interfering with repo discovery\n \ttest_must_fail env GIT_DIR=.git git fsck 2>out &&\n-\tcat out &&\n-\tgrep \"detached HEAD points\" out\n+\ttest_must_contain \"detached HEAD points\" out\n '\n \n test_expect_success 'HEAD link pointing at a funny place' '\n@@ -98,8 +95,7 @@ test_expect_success 'HEAD link pointing at a funny place' '\n \techo \"ref: refs/funny/place\" >.git/HEAD &&\n \t# avoid corrupt/broken HEAD from interfering with repo discovery\n \ttest_must_fail env GIT_DIR=.git git fsck 2>out &&\n-\tcat out &&\n-\tgrep \"HEAD points to something strange\" out\n+\ttest_must_contain \"HEAD points to something strange\" out\n '\n \n test_expect_success 'email without @ is okay' '\n@@ -122,8 +118,7 @@ test_expect_success 'email with embedded > is not okay' '\n \tgit update-ref refs/heads/bogus \"$new\" &&\n \ttest_when_finished \"git update-ref -d refs/heads/bogus\" &&\n \ttest_must_fail git fsck 2>out &&\n-\tcat out &&\n-\tgrep \"error in commit $new\" out\n+\ttest_must_contain \"error in commit $new\" out\n '\n \n test_expect_success 'missing < email delimiter is reported nicely' '\n@@ -134,8 +129,7 @@ test_expect_success 'missing < email delimiter is reported nicely' '\n \tgit update-ref refs/heads/bogus \"$new\" &&\n \ttest_when_finished \"git update-ref -d refs/heads/bogus\" &&\n \ttest_must_fail git fsck 2>out &&\n-\tcat out &&\n-\tgrep \"error in commit $new.* - bad name\" out\n+\ttest_must_contain \"error in commit $new.* - bad name\" out\n '\n \n test_expect_success 'missing email is reported nicely' '\n@@ -146,8 +140,7 @@ test_expect_success 'missing email is reported nicely' '\n \tgit update-ref refs/heads/bogus \"$new\" &&\n \ttest_when_finished \"git update-ref -d refs/heads/bogus\" &&\n \ttest_must_fail git fsck 2>out &&\n-\tcat out &&\n-\tgrep \"error in commit $new.* - missing email\" out\n+\ttest_must_contain \"error in commit $new.* - missing email\" out\n '\n \n test_expect_success '> in name is reported' '\n@@ -158,8 +151,7 @@ test_expect_success '> in name is reported' '\n \tgit update-ref refs/heads/bogus \"$new\" &&\n \ttest_when_finished \"git update-ref -d refs/heads/bogus\" &&\n \ttest_must_fail git fsck 2>out &&\n-\tcat out &&\n-\tgrep \"error in commit $new\" out\n+\ttest_must_contain \"error in commit $new\" out\n '\n \n # date is 2^64 + 1\n@@ -172,8 +164,7 @@ test_expect_success 'integer overflow in timestamps is reported' '\n \tgit update-ref refs/heads/bogus \"$new\" &&\n \ttest_when_finished \"git update-ref -d refs/heads/bogus\" &&\n \ttest_must_fail git fsck 2>out &&\n-\tcat out &&\n-\tgrep \"error in commit $new.*integer overflow\" out\n+\ttest_must_contain \"error in commit $new.*integer overflow\" out\n '\n \n test_expect_success 'commit with NUL in header' '\n@@ -184,8 +175,7 @@ test_expect_success 'commit with NUL in header' '\n \tgit update-ref refs/heads/bogus \"$new\" &&\n \ttest_when_finished \"git update-ref -d refs/heads/bogus\" &&\n \ttest_must_fail git fsck 2>out &&\n-\tcat out &&\n-\tgrep \"error in commit $new.*unterminated header: NUL at offset\" out\n+\ttest_must_contain \"error in commit $new.*unterminated header: NUL at offset\" out\n '\n \n test_expect_success 'malformatted tree object' '\n@@ -205,7 +195,7 @@ test_expect_success 'malformatted tree object' '\n \t\tgit hash-object -w -t tree --stdin\n \t) &&\n \ttest_must_fail git fsck 2>out &&\n-\tgrep \"error in tree .*contains duplicate file entries\" out\n+\ttest_must_contain \"error in tree .*contains duplicate file entries\" out\n '\n \n test_expect_success 'tag pointing to nonexistent' '\n@@ -223,8 +213,7 @@ test_expect_success 'tag pointing to nonexistent' '\n \techo $tag >.git/refs/tags/invalid &&\n \ttest_when_finished \"git update-ref -d refs/tags/invalid\" &&\n \ttest_must_fail git fsck --tags >out &&\n-\tcat out &&\n-\tgrep \"broken link\" out\n+\ttest_must_contain \"broken link\" out\n '\n \n test_expect_success 'tag pointing to something else than its type' '\n@@ -285,7 +274,7 @@ test_expect_success 'tag with bad tagger' '\n \techo $tag >.git/refs/tags/wrong &&\n \ttest_when_finished \"git update-ref -d refs/tags/wrong\" &&\n \ttest_must_fail git fsck --tags 2>out &&\n-\tgrep \"error in tag .*: invalid author/committer\" out\n+\ttest_must_contain \"error in tag .*: invalid author/committer\" out\n '\n \n test_expect_success 'tag with NUL in header' '\n@@ -304,8 +293,7 @@ test_expect_success 'tag with NUL in header' '\n \techo $tag >.git/refs/tags/wrong &&\n \ttest_when_finished \"git update-ref -d refs/tags/wrong\" &&\n \ttest_must_fail git fsck --tags 2>out &&\n-\tcat out &&\n-\tgrep \"error in tag $tag.*unterminated header: NUL at offset\" out\n+\ttest_must_contain \"error in tag $tag.*unterminated header: NUL at offset\" out\n '\n \n test_expect_success 'cleaned up' '\n@@ -335,8 +323,7 @@ test_expect_success 'rev-list --verify-objects with bad sha1' '\n \ttest_when_finished \"git update-ref -d refs/heads/bogus\" &&\n \n \ttest_might_fail git rev-list --verify-objects refs/heads/bogus >/dev/null 2>out &&\n-\tcat out &&\n-\tgrep -q \"error: sha1 mismatch 63ffffffffffffffffffffffffffffffffffffff\" out\n+\ttest_must_contain \"error: sha1 mismatch 63ffffffffffffffffffffffffffffffffffffff\" out\n '\n \n test_expect_success 'force fsck to ignore double author' '\n@@ -360,8 +347,7 @@ test_expect_success 'fsck notices blob entry pointing to null sha1' '\n \t sha=$(printf \"100644 file$_bz$_bz20\" |\n \t       git hash-object -w --stdin -t tree) &&\n \t  git fsck 2>out &&\n-\t  cat out &&\n-\t  grep \"warning.*null sha1\" out\n+\t  test_must_contain \"warning.*null sha1\" out\n \t)\n '\n \n@@ -371,8 +357,7 @@ test_expect_success 'fsck notices submodule entry pointing to null sha1' '\n \t sha=$(printf \"160000 submodule$_bz$_bz20\" |\n \t       git hash-object -w --stdin -t tree) &&\n \t  git fsck 2>out &&\n-\t  cat out &&\n-\t  grep \"warning.*null sha1\" out\n+\t  test_must_contain \"warning.*null sha1\" out\n \t)\n '\n \n@@ -392,8 +377,7 @@ while read name path pretty; do\n \t\t\tprintf \"$mode $type %s\\t%s\" \"$value\" \"$path\" >bad &&\n \t\t\tbad_tree=$(git mktree <bad) &&\n \t\t\tgit fsck 2>out &&\n-\t\t\tcat out &&\n-\t\t\tgrep \"warning.*tree $bad_tree\" out\n+\t\t\ttest_must_contain \"warning.*tree $bad_tree\" out\n \t\t)'\n \tdone <<-\\EOF\n \t100644 blob\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 0ba9db0..f5023b0 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -393,8 +393,7 @@ test_expect_success 'fetch from GIT URL with a non-applying branch.<name>.merge\n # the strange name is: a\\!'b\n test_expect_success 'quoting of a strangely named repo' '\n \ttest_must_fail git fetch \"a\\\\!'\\''b\" > result 2>&1 &&\n-\tcat result &&\n-\tgrep \"fatal: '\\''a\\\\\\\\!'\\''b'\\''\" result\n+\ttest_must_contain \"fatal: '\\''a\\\\\\\\!'\\''b'\\''\" result\n '\n \n test_expect_success 'bundle should record HEAD correctly' '\ndiff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh\nindex 362b158..57ec852 100755\n--- a/t/t5801-remote-helpers.sh\n+++ b/t/t5801-remote-helpers.sh\n@@ -245,8 +245,7 @@ clean_mark () {\n test_expect_success 'proper failure checks for fetching' '\n \t(cd local &&\n \ttest_must_fail env GIT_REMOTE_TESTGIT_FAILURE=1 git fetch 2>error &&\n-\tcat error &&\n-\tgrep -q \"Error while running fast-import\" error\n+\ttest_must_contain \"Error while running fast-import\" error\n \t)\n '\n \ndiff --git a/t/t9810-git-p4-rcs.sh b/t/t9810-git-p4-rcs.sh\nindex 8134ab4..e02b490 100755\n--- a/t/t9810-git-p4-rcs.sh\n+++ b/t/t9810-git-p4-rcs.sh\n@@ -294,8 +294,7 @@ test_expect_success 'cope with rcs keyword file deletion' '\n \t\techo \"\\$Revision\\$\" >kwdelfile.c &&\n \t\tp4 add -t ktext kwdelfile.c &&\n \t\tp4 submit -d \"Add file to be deleted\" &&\n-\t\tcat kwdelfile.c &&\n-\t\tgrep 1 kwdelfile.c\n+\t\ttest_must_constain 1 kwdelfile.c\n \t) &&\n \tgit p4 clone --dest=\"$git\" //depot &&\n \t(\n-- \n2.6.3\n"},{"id":"273564","messageId":"1448068603-2112-1-git-send-email-szeder@ira.uka.de","threadId":"40836","inReplyTo":"564F878B.3090508@web.de","subject":"Re: [PATCH 4/2] test: use test_must_contain","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2015-11-21T01:16:43Z","receivedAt":"2015-11-21T01:16:43Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Hi,\n\n> diff --git a/t/t9810-git-p4-rcs.sh b/t/t9810-git-p4-rcs.sh\n> index 8134ab4..e02b490 100755\n> --- a/t/t9810-git-p4-rcs.sh\n> +++ b/t/t9810-git-p4-rcs.sh\n> @@ -294,8 +294,7 @@ test_expect_success 'cope with rcs keyword file deletion' '\n>  \t\techo \"\\$Revision\\$\" >kwdelfile.c &&\n>  \t\tp4 add -t ktext kwdelfile.c &&\n>  \t\tp4 submit -d \"Add file to be deleted\" &&\n> -\t\tcat kwdelfile.c &&\n> -\t\tgrep 1 kwdelfile.c\n> +\t\ttest_must_constain 1 kwdelfile.c\n\ns/constain/contain/\n"},{"id":"273567","messageId":"564FD731.6070804@web.de","threadId":"40836","inReplyTo":"1448068603-2112-1-git-send-email-szeder@ira.uka.de","subject":"Re: [PATCH 4/2] test: use test_must_contain","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2015-11-21T02:30:09Z","receivedAt":"2015-11-21T02:30:09Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 21.11.2015 um 02:16 schrieb SZEDER Gábor:\n> Hi,\n>\n>> diff --git a/t/t9810-git-p4-rcs.sh b/t/t9810-git-p4-rcs.sh\n>> index 8134ab4..e02b490 100755\n>> --- a/t/t9810-git-p4-rcs.sh\n>> +++ b/t/t9810-git-p4-rcs.sh\n>> @@ -294,8 +294,7 @@ test_expect_success 'cope with rcs keyword file deletion' '\n>>   \t\techo \"\\$Revision\\$\" >kwdelfile.c &&\n>>   \t\tp4 add -t ktext kwdelfile.c &&\n>>   \t\tp4 submit -d \"Add file to be deleted\" &&\n>> -\t\tcat kwdelfile.c &&\n>> -\t\tgrep 1 kwdelfile.c\n>> +\t\ttest_must_constain 1 kwdelfile.c\n>\n> s/constain/contain/\n>\n\nThank you!  I need to work on my vi fu..  Or install all optional tools \nto be able to run the full test suite. ;)\n\nRené\n"},{"id":"273570","messageId":"56502745.2000307@kdbg.org","threadId":"40836","inReplyTo":"564F8781.1080902@web.de","subject":"Re: [PATCH 3/2] test: factor out helper function test_must_contain","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2015-11-21T08:11:49Z","receivedAt":"2015-11-21T08:11:49Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 20.11.2015 um 21:50 schrieb René Scharfe:\n> Extract a helper function for searching for a pattern in a file and\n> printing the whole file if the pattern is not found.  It is useful\n> when starting tests with --verbose for debugging purposes.\n\n> +# Check if a file contains an expected pattern.\n> +test_must_contain () {\n> +\tif grep \"$1\" \"$2\"\n> +\tthen\n> +\t\treturn 0\n> +\telse\n> +\t\techo \"didn't find /$1/ in '$2', it contains:\"\n> +\t\tcat \"$2\"\n> +\t\treturn 1\n> +\tfi\n> +}\n\nThere is already test_i18n_grep. Should it be folded into this function? \nWouldn't we also want to have a function test_must_not_contain?\n\nIMHO, we should not increase the number of functions that give a bonus \nonly when there is a test case failure. They do not scale well: There is \na permanent mental burden on every reviewer to watch out that they are \nused in new tests. But without those functions, the burden is on the one \nperson investigating a test case failure, who has to live without the \ndebugging support.\n\n-- Hannes\n"},{"id":"273571","messageId":"56503ADC.50908@web.de","threadId":"40836","inReplyTo":"56502745.2000307@kdbg.org","subject":"Re: [PATCH 3/2] test: factor out helper function test_must_contain","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2015-11-21T09:35:24Z","receivedAt":"2015-11-21T09:35:24Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 21.11.2015 um 09:11 schrieb Johannes Sixt:\n> Am 20.11.2015 um 21:50 schrieb René Scharfe:\n>> Extract a helper function for searching for a pattern in a file and\n>> printing the whole file if the pattern is not found.  It is useful\n>> when starting tests with --verbose for debugging purposes.\n> \n>> +# Check if a file contains an expected pattern.\n>> +test_must_contain () {\n>> +    if grep \"$1\" \"$2\"\n>> +    then\n>> +        return 0\n>> +    else\n>> +        echo \"didn't find /$1/ in '$2', it contains:\"\n>> +        cat \"$2\"\n>> +        return 1\n>> +    fi\n>> +}\n> \n> There is already test_i18n_grep. Should it be folded into this function? \n\nThat's a good point.  But how?  test_i18ngrep can also work as a\nfilter and pass on options, so we'd need to parse all parameters and\nredirect stdin to a temporary file unless a file was specified.  Or\nwe could be sloppy and just check if the last parameter is a file and\nif yes then spew it out.\n\n> Wouldn't we also want to have a function test_must_not_contain?\n\nI doubt it.  In such a function grep would display the lines that match\nunexpectedly already, so showing the whole file after that won't add\nmuch more of interest.\n\n> IMHO, we should not increase the number of functions that give a bonus \n> only when there is a test case failure. They do not scale well: There is \n> a permanent mental burden on every reviewer to watch out that they are \n> used in new tests. But without those functions, the burden is on the one \n> person investigating a test case failure, who has to live without the \n> debugging support.\n\ntest_must_contain doesn't have to be used everywhere, only in cases\nwhere a file is shown and grepped.  I agree that letting an existing\nfunction do that job (or deciding that the job is not worth doing) is\npreferable.\n\nHere's how I imagine the sloppy add-on to test_i18ncmp to look:\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 16c4d7b..db64600 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -985,13 +985,28 @@ test_i18ncmp () {\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+\n+\t\trc=$?\n+\t\tif test $rc != 0\n+\t\tthen\n+\t\t\twhile test $# -gt 1\n+\t\t\tdo\n+\t\t\t\tshift\n+\t\t\tdone\n+\t\t\tif test -f \"$1\"\n+\t\t\tthen\n+\t\t\t\techo \"Expected pattern not found, content is:\"\n+\t\t\t\tcat \"$1\"\n+\t\t\tfi\n+\t\t\treturn $rc\n+\t\tfi\n \tfi\n }\n \n"}]}