{"thread":{"id":"49319","subject":"[PATCH 0/1] Add coverage for 'git format-patch --range-diff' single-patch case","startedAt":"2018-09-11T20:21:08Z","lastAt":"2018-09-12T14:31:07Z","messageCount":11,"participants":["Derrick Stolee via GitGitGadget","Derrick Stolee","Junio C Hamano","Stefan Beller","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"357906","messageId":"pull.37.git.gitgitgadget@gmail.com","threadId":"49319","inReplyTo":null,"subject":"[PATCH 0/1] Add coverage for 'git format-patch --range-diff' single-patch case","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-09-11T20:21:04Z","receivedAt":"2018-09-11T20:21:08Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"In es/format-patch-rangediff, we added a '--range-diff' option to\ngit-format-patch to automatically add a range-diff. We also added an option\nto write the diff as commentary to a single patch submission. However, this\ncheck was not tested.\n\nI discovered this test gap by running 'make coverage-test coverage-report'\non 'next' and then comparing the uncovered lines with those in the diff\nbetween 'master' and 'next'. I have a script that automates this process,\nand I'm still working on polishing it. You can see an earlier version at\n[1].\n\nBased on es/format-patch-rangediff\n\nCc: sunshine@sunshine.co\n\nCc: peff@peff.net\n\n[1] \nhttps://github.com/derrickstolee/git/blob/coverage/contrib/coverage-diff.shA\nbash script to report uncovered lines that were added in a diff.\n\nDerrick Stolee (1):\n t3206-range-diff.sh: cover single-patch case\n\n t/t3206-range-diff.sh | 5 +++++\n 1 file changed, 5 insertions(+)\n\n\nbase-commit: 40ce41604daf200cdc85abded0133d40faafc2f8\nPublished-As: https://github.com/gitgitgadget/git/releases/tags/pr-37%2Fderrickstolee%2Frange-diff-test-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-37/derrickstolee/range-diff-test-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/37\n-- \ngitgitgadget\n"},{"id":"357907","messageId":"58347a962438852be0d37c3957686ea5000b2dbd.1536697263.git.gitgitgadget@gmail.com","threadId":"49319","inReplyTo":"pull.37.git.gitgitgadget@gmail.com","subject":"[PATCH 1/1] t3206-range-diff.sh: cover single-patch case","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-09-11T20:21:06Z","receivedAt":"2018-09-11T20:21:10Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe commit 40ce4160 \"format-patch: allow --range-diff to apply to\na lone-patch\" added the ability to see a range-diff as commentary\nafter the commit message of a single patch series (i.e. [PATCH]\ninstead of [PATCH X/N]). However, this functionality was not\ncovered by a test case.\n\nAdd a simple test case that checks that a range-diff is written as\ncommentary to the patch.\n\nSigned-off-by: Derrick Stolee \n---\n t/t3206-range-diff.sh | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\nindex 3d7a2d8a4d..05ef3263d2 100755\n--- a/t/t3206-range-diff.sh\n+++ b/t/t3206-range-diff.sh\n@@ -154,4 +154,9 @@ do\n \t'\n done\n \n+test_expect_success 'format-patch --range-diff as commentary' '\n+\tgit format-patch --stdout --range-diff=HEAD~1 HEAD~1 >actual &&\n+\tgrep -A 1 -e \"\\-\\-\\-\" actual | grep \"Range-diff:\"\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"357909","messageId":"61316ea9-50e9-a57a-291c-34515e1763bc@gmail.com","threadId":"49319","inReplyTo":"pull.37.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/1] Add coverage for 'git format-patch --range-diff' single-patch case","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-09-11T20:29:29Z","receivedAt":"2018-09-11T20:29:31Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/11/2018 4:21 PM, Derrick Stolee via GitGitGadget wrote:\n> In es/format-patch-rangediff, we added a '--range-diff' option to\n> git-format-patch to automatically add a range-diff. We also added an option\n> to write the diff as commentary to a single patch submission. However, this\n> check was not tested.\n>\n> I discovered this test gap by running 'make coverage-test coverage-report'\n> on 'next' and then comparing the uncovered lines with those in the diff\n> between 'master' and 'next'. I have a script that automates this process,\n> and I'm still working on polishing it. You can see an earlier version at\n> [1].\n>\n> Based on es/format-patch-rangediff\n>\n> Cc: sunshine@sunshine.co\n>\n> Cc: peff@peff.net\n\nSorry that these Cc's didn't work (again). Adding them manually and \ninvestigating the GGG code.\n\n-Stolee\n\n"},{"id":"357915","messageId":"xmqq4levoinl.fsf@gitster-ct.c.googlers.com","threadId":"49319","inReplyTo":"58347a962438852be0d37c3957686ea5000b2dbd.1536697263.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] t3206-range-diff.sh: cover single-patch case","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-11T20:58:54Z","receivedAt":"2018-09-11T20:58:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> From: Derrick Stolee \n>\n> The commit 40ce4160 \"format-patch: allow --range-diff to apply to\n> a lone-patch\" added the ability to see a range-diff as commentary\n> after the commit message of a single patch series (i.e. [PATCH]\n> instead of [PATCH X/N]). However, this functionality was not\n> covered by a test case.\n>\n> Add a simple test case that checks that a range-diff is written as\n> commentary to the patch.\n>\n> Signed-off-by: Derrick Stolee \n> ---\n> t/t3206-range-diff.sh | 5 +++++\n> 1 file changed, 5 insertions(+)\n>\n> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\n> index 3d7a2d8a4d..05ef3263d2 100755\n> --- a/t/t3206-range-diff.sh\n> +++ b/t/t3206-range-diff.sh\n> @@ -154,4 +154,9 @@ do\n> \t'\n> done\n> \n> +test_expect_success 'format-patch --range-diff as commentary' '\n> +\tgit format-patch --stdout --range-diff=HEAD~1 HEAD~1 >actual &&\n> +\tgrep -A 1 -e \"\\-\\-\\-\" actual | grep \"Range-diff:\"\n\nIsn't \"grep -A\" GNUism?\n"},{"id":"357917","messageId":"CAGZ79kYZWRS3p+dP-3AtDSs0JqCCOd1rycugRZawpbkB0ydw3A@mail.gmail.com","threadId":"49319","inReplyTo":"58347a962438852be0d37c3957686ea5000b2dbd.1536697263.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] t3206-range-diff.sh: cover single-patch case","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-11T21:09:55Z","receivedAt":"2018-09-11T21:10:09Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Sep 11, 2018 at 1:21 PM Derrick Stolee via GitGitGadget\n wrote:\n>\n> From: Derrick Stolee \n>\n> The commit 40ce4160 \"format-patch: allow --range-diff to apply to\n> a lone-patch\" added the ability to see a range-diff as commentary\n> after the commit message of a single patch series (i.e. [PATCH]\n> instead of [PATCH X/N]). However, this functionality was not\n> covered by a test case.\n>\n> Add a simple test case that checks that a range-diff is written as\n> commentary to the patch.\n>\n> Signed-off-by: Derrick Stolee \n> ---\n> t/t3206-range-diff.sh | 5 +++++\n> 1 file changed, 5 insertions(+)\n>\n> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\n> index 3d7a2d8a4d..05ef3263d2 100755\n> --- a/t/t3206-range-diff.sh\n> +++ b/t/t3206-range-diff.sh\n> @@ -154,4 +154,9 @@ do\n> '\n> done\n>\n> +test_expect_success 'format-patch --range-diff as commentary' '\n> + git format-patch --stdout --range-diff=HEAD~1 HEAD~1 >actual &&\n\nThis is an interesting use of range-diff, as it basically tells us\n\"Range-diff: This is a new patch\", but it works to make sure\nthere is a range diff section. (I shortly wondered if we would\never omit the range diff for \"obvious\" cases or word it differently)\n\n> + grep -A 1 -e \"\\-\\-\\-\" actual | grep \"Range-diff:\"\n\nSo the first grep finds the three dashes, presumably those\nafter the commit message/ but others as well, e.g. in\n --- a/\n +++ b/\nand then the second grep should find the string \"Range-diff\".\nBy having the greps chained with a pipe, only one return\ncode can be delivered to the test suite, and as we get the last\ncommands return code, we get reported if we found the string\nin the preselected part.\n\nI was wondering if we could get away with just one command to\ncheck for that multi line pattern\n\n sed -n -e '/---/,/^Range/p' actual\n\nseems to detect that pattern, and prints from there on to the\nrest of the file.\n\n\n> +'\n> +\n> test_done\n> --\n> gitgitgadget\n"},{"id":"357918","messageId":"xmqqzhwnn3hw.fsf@gitster-ct.c.googlers.com","threadId":"49319","inReplyTo":"xmqq4levoinl.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] t3206-range-diff.sh: cover single-patch case","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-11T21:11:39Z","receivedAt":"2018-09-11T21:11:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> +test_expect_success 'format-patch --range-diff as commentary' '\n>> +\tgit format-patch --stdout --range-diff=HEAD~1 HEAD~1 >actual &&\n>> +\tgrep -A 1 -e \"\\-\\-\\-\" actual | grep \"Range-diff:\"\n>\n> Isn't \"grep -A\" GNUism?\n\nSorry for short-write(2) X-<.\n\nPerhaps\n\n\tsed -ne \"/^---$/,+1/s/^Range-diff:/&/p\"\n\nor something along that line.\n\n"},{"id":"357919","messageId":"CAPig+cT4HeX85SXZgyVQpAZarDFaafeBri7zLpExA=3O403qRw@mail.gmail.com","threadId":"49319","inReplyTo":"pull.37.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/1] Add coverage for 'git format-patch --range-diff' single-patch case","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-09-11T21:21:00Z","receivedAt":"2018-09-11T21:21:14Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Sep 11, 2018 at 4:26 PM Derrick Stolee via GitGitGadget\n wrote:\n> In es/format-patch-rangediff, we added a '--range-diff' option to\n> git-format-patch to automatically add a range-diff. We also added an option\n> to write the diff as commentary to a single patch submission. However, this\n> check was not tested.\n\nIt was an intentional (though unjustified) oversight. Thanks for\nworking on this.\n\n> I discovered this test gap by running 'make coverage-test coverage-report'\n> on 'next' and then comparing the uncovered lines with those in the diff\n> between 'master' and 'next'. I have a script that automates this process,\n> and I'm still working on polishing it.\n\nInteresting. Does your script detect that the --creation-factor option\nadded by patch 13/14 [1] also lacks a test? (I'm not necessarily\nsuggesting adding a test at the moment. Even Dscho's git-range-diff\nseries doesn't have a test for \"creation factor\", so it wouldn't\nnecessarily make sense to add a test only for the git-format-patch\ncase. Instead, we'd want, at some point, to add tests covering both\ngit-range-diff and git-format-patch.)\n\n[1]: https://public-inbox.org/git/20180722095717.17912-14-sunshine@sunshineco.com/\n"},{"id":"357920","messageId":"CAPig+cQviMsbhdQty8DnBUWQx4hxNvH-FzXkNeHNBKy6mHGz5A@mail.gmail.com","threadId":"49319","inReplyTo":"58347a962438852be0d37c3957686ea5000b2dbd.1536697263.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] t3206-range-diff.sh: cover single-patch case","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-09-11T21:34:29Z","receivedAt":"2018-09-11T21:34:43Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Sep 11, 2018 at 4:26 PM Derrick Stolee via GitGitGadget\n wrote:\n> The commit 40ce4160 \"format-patch: allow --range-diff to apply to\n> a lone-patch\" added the ability to see a range-diff as commentary\n> after the commit message of a single patch series (i.e. [PATCH]\n> instead of [PATCH X/N]). However, this functionality was not\n> covered by a test case.\n>\n> Add a simple test case that checks that a range-diff is written as\n> commentary to the patch.\n>\n> Signed-off-by: Derrick Stolee \n> ---\n> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\n> @@ -154,4 +154,9 @@ do\n> +test_expect_success 'format-patch --range-diff as commentary' '\n> + git format-patch --stdout --range-diff=HEAD~1 HEAD~1 >actual &&\n> + grep -A 1 -e \"\\-\\-\\-\" actual | grep \"Range-diff:\"\n> +'\n\nAside from Junio's and Stefan's comments...\n\nPatch 6/14 [1], in addition to checking that a solo patch contains an\ninterdiff, takes the extra step of checking that individual patches\n_don't_ contain an interdiff when --cover-letter is used. I wonder if\nthe same should be done here, though I don't feel too strongly about\nit. If you do go that route, it might make sense to move this test to\nt4014 as neighbor to the --interdiff tests. The reason 10/14 [2] added\nthe \"git format-patch --range-diff\" test to t3206 instead of t4014 was\nso it could do a thorough check of the embedded range-diff by re-using\nthe specially crafted test repo set up by t3206. Your new test is much\nlooser, thus could be moved alongside the --interdiff tests. Not a big\ndeal, though. Either way is fine. Thanks for working on this.\n\n[1]: https://public-inbox.org/git/20180722095717.17912-7-sunshine@sunshineco.com/\n[2]: https://public-inbox.org/git/20180722095717.17912-11-sunshine@sunshineco.com/\n"},{"id":"357964","messageId":"14fdfb44-8024-113b-8c34-2a60feedffff@gmail.com","threadId":"49319","inReplyTo":"CAPig+cQviMsbhdQty8DnBUWQx4hxNvH-FzXkNeHNBKy6mHGz5A@mail.gmail.com","subject":"Re: [PATCH 1/1] t3206-range-diff.sh: cover single-patch case","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-09-12T14:20:21Z","receivedAt":"2018-09-12T14:20:25Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/11/2018 5:34 PM, Eric Sunshine wrote:\n> On Tue, Sep 11, 2018 at 4:26 PM Derrick Stolee via GitGitGadget\n> wrote:\n>> The commit 40ce4160 \"format-patch: allow --range-diff to apply to\n>> a lone-patch\" added the ability to see a range-diff as commentary\n>> after the commit message of a single patch series (i.e. [PATCH]\n>> instead of [PATCH X/N]). However, this functionality was not\n>> covered by a test case.\n>>\n>> Add a simple test case that checks that a range-diff is written as\n>> commentary to the patch.\n>>\n>> Signed-off-by: Derrick Stolee \n>> ---\n>> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\n>> @@ -154,4 +154,9 @@ do\n>> +test_expect_success 'format-patch --range-diff as commentary' '\n>> + git format-patch --stdout --range-diff=HEAD~1 HEAD~1 >actual &&\n>> + grep -A 1 -e \"\\-\\-\\-\" actual | grep \"Range-diff:\"\n>> +'\n> Aside from Junio's and Stefan's comments...\n>\n> Patch 6/14 [1], in addition to checking that a solo patch contains an\n> interdiff, takes the extra step of checking that individual patches\n> _don't_ contain an interdiff when --cover-letter is used. I wonder if\n> the same should be done here, though I don't feel too strongly about\n> it. If you do go that route, it might make sense to move this test to\n> t4014 as neighbor to the --interdiff tests. The reason 10/14 [2] added\n> the \"git format-patch --range-diff\" test to t3206 instead of t4014 was\n> so it could do a thorough check of the embedded range-diff by re-using\n> the specially crafted test repo set up by t3206. Your new test is much\n> looser, thus could be moved alongside the --interdiff tests. Not a big\n> deal, though. Either way is fine. Thanks for working on this.\n>\n> [1]: https://public-inbox.org/git/20180722095717.17912-7-sunshine@sunshineco.com/\n> [2]: https://public-inbox.org/git/20180722095717.17912-11-sunshine@sunshineco.com/\nThanks for these links! In particular, [2] uses this line to test the \ninter-diff appears:\n\n+    test_i18ngrep \"^Interdiff:$\" 0001-fleep.patch &&\n\nThat's a better way to test, especially with the translation. It would \nbe enough for my needs.\n\nThanks,\n\n-Stolee\n\nP.S. Resending because apparently I had HTML in the last response\n\n"},{"id":"357967","messageId":"pull.37.v2.git.gitgitgadget@gmail.com","threadId":"49319","inReplyTo":"pull.37.git.gitgitgadget@gmail.com","subject":"[PATCH v2 0/1] Add coverage for 'git format-patch --range-diff' single-patch case","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-09-12T14:31:02Z","receivedAt":"2018-09-12T14:31:05Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"In es/format-patch-rangediff, we added a '--range-diff' option to\ngit-format-patch to automatically add a range-diff. We also added an option\nto write the diff as commentary to a single patch submission. However, this\ncheck was not tested.\n\nI discovered this test gap by running 'make coverage-test coverage-report'\non 'next' and then comparing the uncovered lines with those in the diff\nbetween 'master' and 'next'. I have a script that automates this process,\nand I'm still working on polishing it. You can see an earlier version at\n[1].\n\nBased on es/format-patch-rangediff\n\nCc: sunshine@sunshine.co\n\nCc: peff@peff.net\n\n[1] \nhttps://github.com/derrickstolee/git/blob/coverage/contrib/coverage-diff.shA\nbash script to report uncovered lines that were added in a diff.\n\nDerrick Stolee (1):\n t3206-range-diff.sh: cover single-patch case\n\n t/t3206-range-diff.sh | 5 +++++\n 1 file changed, 5 insertions(+)\n\n\nbase-commit: 40ce41604daf200cdc85abded0133d40faafc2f8\nPublished-As: https://github.com/gitgitgadget/git/releases/tags/pr-37%2Fderrickstolee%2Frange-diff-test-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-37/derrickstolee/range-diff-test-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/37\n\nRange-diff vs v1:\n\n 1: 58347a9624 ! 1: 277a4d2bd8 t3206-range-diff.sh: cover single-patch case\n @@ -22,7 +22,7 @@\n \n +test_expect_success 'format-patch --range-diff as commentary' '\n +\tgit format-patch --stdout --range-diff=HEAD~1 HEAD~1 >actual &&\n -+\tgrep -A 1 -e \"\\-\\-\\-\" actual | grep \"Range-diff:\"\n ++\ttest_i18ngrep \"^Range-diff:$\" actual\n +'\n +\n test_done\n\n-- \ngitgitgadget\n"},{"id":"357968","messageId":"277a4d2bd8bcb20323f0a5a01ccdf431f3c08350.1536762661.git.gitgitgadget@gmail.com","threadId":"49319","inReplyTo":"pull.37.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 1/1] t3206-range-diff.sh: cover single-patch case","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-09-12T14:31:03Z","receivedAt":"2018-09-12T14:31:07Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe commit 40ce4160 \"format-patch: allow --range-diff to apply to\na lone-patch\" added the ability to see a range-diff as commentary\nafter the commit message of a single patch series (i.e. [PATCH]\ninstead of [PATCH X/N]). However, this functionality was not\ncovered by a test case.\n\nAdd a simple test case that checks that a range-diff is written as\ncommentary to the patch.\n\nSigned-off-by: Derrick Stolee \n---\n t/t3206-range-diff.sh | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\nindex 3d7a2d8a4d..6babc0e276 100755\n--- a/t/t3206-range-diff.sh\n+++ b/t/t3206-range-diff.sh\n@@ -154,4 +154,9 @@ do\n \t'\n done\n \n+test_expect_success 'format-patch --range-diff as commentary' '\n+\tgit format-patch --stdout --range-diff=HEAD~1 HEAD~1 >actual &&\n+\ttest_i18ngrep \"^Range-diff:$\" actual\n+'\n+\n test_done\n-- \ngitgitgadget\n"}]}