{"thread":{"id":"52482","subject":"[PATCH RESEND] t4015: improve coverage of function context test","startedAt":"2019-12-18T18:06:15Z","lastAt":"2019-12-19T17:51:17Z","messageCount":5,"participants":["René Scharfe","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"388448","messageId":"fedd48bd-28e8-ccc8-ae50-42d9b2ea1c16@web.de","threadId":"52482","inReplyTo":null,"subject":"[PATCH RESEND] t4015: improve coverage of function context test","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-12-18T18:05:54Z","receivedAt":"2019-12-18T18:06:15Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Include an actual function line in the test files to check if context is\nexpanded to include the whole function, and add an ignored change before\nfunction context to check if that one stays hidden, while the originally\nignored change within function context is shown.\n\nSigned-off-by: René Scharfe \n---\nOriginal submission:\nhttps://lore.kernel.org/git/e47b77b4-7b7a-3d59-9e24-934528c5e297@web.de/\n\n t/t4015-diff-whitespace.sh | 15 ++++++++++-----\n 1 file changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex 65615e2fa9..59b2412c22 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -2051,19 +2051,24 @@ test_expect_success 'compare mixed whitespace delta across moved blocks' '\n '\n\n test_expect_success 'combine --ignore-blank-lines with --function-context' '\n-\ttest_write_lines 1 \"\" 2 3 4 5 >a &&\n-\ttest_write_lines 1 2 3 4 >b &&\n+\ttest_write_lines 1 2 3 \"\" function 1 2 3 4 5 \"\" 6 7 8 9 >a &&\n+\ttest_write_lines \"\" 1 2 3 \"\" function 1 2 3 4 5 6 7 8 >b &&\n \ttest_must_fail git diff --no-index \\\n \t\t--ignore-blank-lines --function-context a b >actual.raw &&\n \tsed -n \"/@@/,\\$p\" actual &&\n \tcat <<-\\EOF >expect &&\n-\t@@ -1,6 +1,4 @@\n+\t@@ -5,11 +6,9 @@\n+\t function\n \t 1\n-\t-\n \t 2\n \t 3\n \t 4\n-\t-5\n+\t 5\n+\t-\n+\t 6\n+\t 7\n+\t 8\n+\t-9\n \tEOF\n \ttest_cmp expect actual\n '\n--\n2.24.1\n"},{"id":"388474","messageId":"20191218201241.GA6452@coredump.intra.peff.net","threadId":"52482","inReplyTo":"fedd48bd-28e8-ccc8-ae50-42d9b2ea1c16@web.de","subject":"Re: [PATCH RESEND] t4015: improve coverage of function context test","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-12-18T20:12:41Z","receivedAt":"2019-12-18T20:12:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 18, 2019 at 07:05:54PM +0100, René Scharfe wrote:\n\n> Include an actual function line in the test files to check if context is\n> expanded to include the whole function, and add an ignored change before\n> function context to check if that one stays hidden, while the originally\n> ignored change within function context is shown.\n>\n> [...]\n> test_expect_success 'combine --ignore-blank-lines with --function-context' '\n> -\ttest_write_lines 1 \"\" 2 3 4 5 >a &&\n> -\ttest_write_lines 1 2 3 4 >b &&\n> +\ttest_write_lines 1 2 3 \"\" function 1 2 3 4 5 \"\" 6 7 8 9 >a &&\n> +\ttest_write_lines \"\" 1 2 3 \"\" function 1 2 3 4 5 6 7 8 >b &&\n\nI'm a little mixed on this one. This _is_ a much better test of how the\ntwo features should be have together. But I think the reason the\noriginal was so short was that it was added when fixing a bug where we'd\niterate off the beginning of list of lines, which now no longer happens.\n\nMaybe we should cover both cases in two separate tests?\n\nI'd also suggest using \"a b c\" for the first three lines to avoid\nconfusion (I don't think it's important that they're the same as the\nlines inside the \"function\").\n\n-Peff\n"},{"id":"388527","messageId":"7d454f2e-eaf6-1897-d6f6-c2e3e2a1f75f@web.de","threadId":"52482","inReplyTo":"20191218201241.GA6452@coredump.intra.peff.net","subject":"Re: [PATCH RESEND] t4015: improve coverage of function context test","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-12-19T17:35:28Z","receivedAt":"2019-12-19T17:35:48Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 18.12.19 um 21:12 schrieb Jeff King:\n> On Wed, Dec 18, 2019 at 07:05:54PM +0100, René Scharfe wrote:\n>\n>> Include an actual function line in the test files to check if context is\n>> expanded to include the whole function, and add an ignored change before\n>> function context to check if that one stays hidden, while the originally\n>> ignored change within function context is shown.\n>>\n>> [...]\n>> test_expect_success 'combine --ignore-blank-lines with --function-context' '\n>> -\ttest_write_lines 1 \"\" 2 3 4 5 >a &&\n>> -\ttest_write_lines 1 2 3 4 >b &&\n>> +\ttest_write_lines 1 2 3 \"\" function 1 2 3 4 5 \"\" 6 7 8 9 >a &&\n>> +\ttest_write_lines \"\" 1 2 3 \"\" function 1 2 3 4 5 6 7 8 >b &&\n>\n> I'm a little mixed on this one. This _is_ a much better test of how the\n> two features should be have together. But I think the reason the\n> original was so short was that it was added when fixing a bug where we'd\n> iterate off the beginning of list of lines, which now no longer happens.\n\nThat fix, b777f3fd61 (xdiff: clamp function context indices in post-image,\n2019-07-23), should no longer be necessary, but I didn't check that\nthoroughly. Since we still have it (and I don't intend to remove it,\nbetter keep that extra safety), it makes sense to keep the specific test.\n\n> Maybe we should cover both cases in two separate tests?\n\nThat's easy enough to do. The hardest part is coming up with a name, but\nsimply counting up should do the trick here.\n\n> I'd also suggest using \"a b c\" for the first three lines to avoid\n> confusion (I don't think it's important that they're the same as the\n> lines inside the \"function\").\n\nGood point. That turns the last line into a function line, though, which\nis unnecessary and confused me a bit, but I think it's a net win.\n\nRené\n"},{"id":"388528","messageId":"86fbb0fd-6b77-5d11-69cd-f0994a8ed759@web.de","threadId":"52482","inReplyTo":"fedd48bd-28e8-ccc8-ae50-42d9b2ea1c16@web.de","subject":"[PATCH v2] t4015: improve coverage of function context test","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-12-19T17:35:43Z","receivedAt":"2019-12-19T17:35:54Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Add a test that includes an actual function line in the test file to\ncheck if context is expanded to include the whole function, and add an\nignored change before function context to check if that one stays hidden\nwhile the originally ignored change within function context is shown.\n\nHelped-by: Jeff King \nSigned-off-by: René Scharfe \n---\nChanges:\n* Keep existing test intact, add a new one instead.\n* Use unique content for the part before the function context to make\n clear that it's independent.\n\n t/t4015-diff-whitespace.sh | 23 +++++++++++++++++++++++\n 1 file changed, 23 insertions(+)\n\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex 65615e2fa9..88d3026894 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -2068,4 +2068,27 @@ test_expect_success 'combine --ignore-blank-lines with --function-context' '\n \ttest_cmp expect actual\n '\n\n+test_expect_success 'combine --ignore-blank-lines with --function-context 2' '\n+\ttest_write_lines a b c \"\" function 1 2 3 4 5 \"\" 6 7 8 9 >a &&\n+\ttest_write_lines \"\" a b c \"\" function 1 2 3 4 5 6 7 8 >b &&\n+\ttest_must_fail git diff --no-index \\\n+\t\t--ignore-blank-lines --function-context a b >actual.raw &&\n+\tsed -n \"/@@/,\\$p\" actual &&\n+\tcat <<-\\EOF >expect &&\n+\t@@ -5,11 +6,9 @@ c\n+\t function\n+\t 1\n+\t 2\n+\t 3\n+\t 4\n+\t 5\n+\t-\n+\t 6\n+\t 7\n+\t 8\n+\t-9\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_done\n--\n2.24.1\n"},{"id":"388531","messageId":"20191219175115.GA677976@coredump.intra.peff.net","threadId":"52482","inReplyTo":"86fbb0fd-6b77-5d11-69cd-f0994a8ed759@web.de","subject":"Re: [PATCH v2] t4015: improve coverage of function context test","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-12-19T17:51:15Z","receivedAt":"2019-12-19T17:51:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 19, 2019 at 06:35:43PM +0100, René Scharfe wrote:\n\n> Add a test that includes an actual function line in the test file to\n> check if context is expanded to include the whole function, and add an\n> ignored change before function context to check if that one stays hidden\n> while the originally ignored change within function context is shown.\n> \n> Helped-by: Jeff King \n> Signed-off-by: René Scharfe \n\nThanks, the patch looks good.\n\nMaybe worth noting in the commit message why there are two tests.\nSomething like:\n\n This differs from the existing test, which is concerned with the case\n where there is no function line at all in the file (and we might look\n past the beginning of the file).\n\n-Peff\n"}]}