{"thread":{"id":"49018","subject":"[RFC] broken test for git am --scissors","startedAt":"2018-08-03T13:38:09Z","lastAt":"2018-08-07T11:24:40Z","messageCount":12,"participants":["Andrei Rybak","Junio C Hamano","Paul Tan"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"354387","messageId":"f91c7393-4f1b-1cf5-b870-f42e9bd18d64@gmail.com","threadId":"49018","inReplyTo":null,"subject":"[RFC] broken test for git am --scissors","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2018-08-03T13:38:03Z","receivedAt":"2018-08-03T13:38:09Z","isPatch":false,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"I was tweaking is_scissors_line function in mailinfo.c and tried writing\nsome tests for it.  And discovered that existing test for git am option\n--scissors is broken.  I then confirmed that by intentionally breaking\nis_scissors_line, like this:\n\n--- 8< ---\nSubject: [PATCH 1/2] mailinfo.c: intentionally break is_scissors_line\n\nIt seems that tests for \"git am\" don't actually test the --scissor\noption logic.  Break is_scissors_line function by using bogus symbols to\nbe able to check the tests.\n\nNote that test suite does not pass with this patch applied.  The\nexpected failure does not happen.\n---\n mailinfo.c    | 4 ++--\n t/t4150-am.sh | 3 ++-\n 2 files changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex 3281a37d5..7938d85e3 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -682,8 +682,8 @@ static int is_scissors_line(const char *line)\n \t\t\tperforation++;\n \t\t\tcontinue;\n \t\t}\n-\t\tif ((!memcmp(c, \">8\", 2) || !memcmp(c, \"8<\", 2) ||\n-\t\t     !memcmp(c, \">%\", 2) || !memcmp(c, \"%<\", 2))) {\n+\t\tif ((!memcmp(c, \">o\", 2) || !memcmp(c, \"o<\", 2) ||\n+\t\t     !memcmp(c, \">/\", 2) || !memcmp(c, \"/<\", 2))) {\n \t\t\tin_perforation = 1;\n \t\t\tperforation += 2;\n \t\t\tscissors += 2;\ndiff --git a/t/t4150-am.sh b/t/t4150-am.sh\nindex 1ebc587f8..59bcb5afd 100755\n--- a/t/t4150-am.sh\n+++ b/t/t4150-am.sh\n@@ -412,7 +412,8 @@ test_expect_success 'am with failing post-applypatch hook' '\n \ttest_cmp head.expected head.actual\n '\n \n-test_expect_success 'am --scissors cuts the message at the scissors line' '\n+# Test should fail, but succeeds\n+test_expect_failure 'am --scissors cuts the message at the scissors line' '\n \trm -fr .git/rebase-apply &&\n \tgit reset --hard &&\n \tgit checkout second &&\n\n--- >8 ---\n\nHere's a proof-of-concept patch for the test, to make it actually fail\nwhen is_scissors_line is broken.  It is the easiest way to do so, that I\ncould come up with, it is not ready to be applied.  I think the two\ntests for --scissors should be rewritten pretty much from scratch, with\nmore obvious naming of files used.\n\n(I made the changes to files in both tests the same just to be able to\nre-use file \"no-scissors-patch.eml\", it's not relevant to the scissor\nline in the commit messages.)\n\n--- 8< ---\nSubject: [PATCH 2/2] t4150-am.sh: fix test for --scissors\n\nTest for option --scissors should check that the eml file with a scissor\nline inside will be cut up and only the part under the cut will be\nturned into commit.\n\nHowever, the test for --scissors generates eml file without such line.\nFix the test for --scissors option.\n\nSigned-off-by: Andrei Rybak <rybak.a.v@gmail.com>\n---\n t/t4150-am.sh | 19 ++++++++++++-------\n 1 file changed, 12 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t4150-am.sh b/t/t4150-am.sh\nindex 59bcb5afd..5ad71fe05 100755\n--- a/t/t4150-am.sh\n+++ b/t/t4150-am.sh\n@@ -69,17 +69,22 @@ test_expect_success 'setup: messages' '\n \n \tEOF\n \n+\tcat >new-scissors-msg <<-\\EOF &&\n+\tSubject: Test git-am with scissors line\n+\n+\tThis line should be included in the commit message.\n+\tEOF\n+\n \tcat >scissors-msg <<-\\EOF &&\n \tTest git-am with scissors line\n \n \tThis line should be included in the commit message.\n \tEOF\n \n-\tcat - scissors-msg >no-scissors-msg <<-\\EOF &&\n+\tcat - new-scissors-msg >no-scissors-msg <<-\\EOF &&\n \tThis line should not be included in the commit message with --scissors enabled.\n \n \t - - >8 - - remove everything above this line - - >8 - -\n-\n \tEOF\n \n \tsignoff=\"Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"\n@@ -148,15 +153,15 @@ test_expect_success setup '\n \t} >patch1-hg.eml &&\n \n \n-\techo scissors-file >scissors-file &&\n-\tgit add scissors-file &&\n+\techo file >file &&\n+\tgit add file &&\n \tgit commit -F scissors-msg &&\n \tgit tag scissors &&\n \tgit format-patch --stdout scissors^ >scissors-patch.eml &&\n \tgit reset --hard HEAD^ &&\n \n-\techo no-scissors-file >no-scissors-file &&\n-\tgit add no-scissors-file &&\n+\techo file >file &&\n+\tgit add file &&\n \tgit commit -F no-scissors-msg &&\n \tgit tag no-scissors &&\n \tgit format-patch --stdout no-scissors^ >no-scissors-patch.eml &&\n@@ -417,7 +422,7 @@ test_expect_failure 'am --scissors cuts the message at the scissors line' '\n \trm -fr .git/rebase-apply &&\n \tgit reset --hard &&\n \tgit checkout second &&\n-\tgit am --scissors scissors-patch.eml &&\n+\tgit am --scissors no-scissors-patch.eml &&\n \ttest_path_is_missing .git/rebase-apply &&\n \tgit diff --exit-code scissors &&\n \ttest_cmp_rev scissors HEAD\n--\n\n--- >8 ---\n\nRelevant old threads:\n\n1. https://public-inbox.org/git/1435861000-25278-11-git-send-email-pyokagan@gmail.com\n2. https://public-inbox.org/git/1436278114-28057-11-git-send-email-pyokagan@gmail.com\n"},{"id":"354454","messageId":"8f69d82b-0f35-754f-0096-853d6b463db7@gmail.com","threadId":"49018","inReplyTo":"f91c7393-4f1b-1cf5-b870-f42e9bd18d64@gmail.com","subject":"[PATCH] t4150: fix broken test for am --scissors","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2018-08-03T22:39:40Z","receivedAt":"2018-08-03T22:39:47Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"Tests for \"git am --[no-]scissors\" [1] work in the following way:\n\n 1. Create files with commit messages\n 2. Use these files to create expected commits\n 3. Generate eml file with patch from expected commits\n 4. Create commits using git am with these eml files\n 5. Compare these commits with expected\n\nThe test for \"git am --scissors\" is supposed to take a message with a\nscissors line above commit message and demonstrate that only the text\nbelow the scissors line is included in the commit created by invocation\nof \"git am --scissors\".  However, the setup of the test uses commits\nwithout the scissors line in the commit message, therefore creating an\neml file without scissors line.\n\nThis can be checked by intentionally breaking is_scissors_line function\nin mailinfo.c. Test t4150-am.sh should fail, but does not.\n\nFix broken test by generating only one eml file--with scissors line, and\nby using it both for --scissors and --no-scissors. To clarify the\nintention of the test, give files and tags more explicit names.\n\n[1]: introduced in bf72ac17d (t4150: tests for am --[no-]scissors,\n     2015-07-19)\n\nSigned-off-by: Andrei Rybak <rybak.a.v@gmail.com>\n---\n\nApplies on top of 980a3d3dd (Merge branch 'pt/am-tests', 2015-08-03).\nThis patch is also available at\n\n  https://github.com/rybak/git fix-am-scissors-test\n\n\n t/t4150-am.sh | 40 ++++++++++++++++++++--------------------\n 1 file changed, 20 insertions(+), 20 deletions(-)\n\ndiff --git a/t/t4150-am.sh b/t/t4150-am.sh\nindex e9b6f8158..23e3b0e91 100755\n--- a/t/t4150-am.sh\n+++ b/t/t4150-am.sh\n@@ -67,17 +67,18 @@ test_expect_success 'setup: messages' '\n \n \tEOF\n \n-\tcat >scissors-msg <<-\\EOF &&\n-\tTest git-am with scissors line\n+\tcat >msg-without-scissors-line <<-\\EOF &&\n+\tTest that git-am --scissors cuts at the scissors line\n \n \tThis line should be included in the commit message.\n \tEOF\n \n-\tcat - scissors-msg >no-scissors-msg <<-\\EOF &&\n+\tprintf \"Subject: \" >subject-prefix &&\n+\n+\tcat - subject-prefix msg-without-scissors-line >msg-with-scissors-line <<-\\EOF &&\n \tThis line should not be included in the commit message with --scissors enabled.\n \n \t - - >8 - - remove everything above this line - - >8 - -\n-\n \tEOF\n \n \tsignoff=\"Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"\n@@ -150,18 +151,17 @@ test_expect_success setup '\n \t} >patch1-hg.eml &&\n \n \n-\techo scissors-file >scissors-file &&\n-\tgit add scissors-file &&\n-\tgit commit -F scissors-msg &&\n-\tgit tag scissors &&\n-\tgit format-patch --stdout scissors^ >scissors-patch.eml &&\n+\techo file >file &&\n+\tgit add file &&\n+\tgit commit -F msg-without-scissors-line &&\n+\tgit tag scissors-used &&\n \tgit reset --hard HEAD^ &&\n \n-\techo no-scissors-file >no-scissors-file &&\n-\tgit add no-scissors-file &&\n-\tgit commit -F no-scissors-msg &&\n-\tgit tag no-scissors &&\n-\tgit format-patch --stdout no-scissors^ >no-scissors-patch.eml &&\n+\techo file >file &&\n+\tgit add file &&\n+\tgit commit -F msg-with-scissors-line &&\n+\tgit tag scissors-not-used &&\n+\tgit format-patch --stdout scissors-not-used^ >patch-with-scissors-line.eml &&\n \tgit reset --hard HEAD^ &&\n \n \tsed -n -e \"3,\\$p\" msg >file &&\n@@ -418,10 +418,10 @@ test_expect_success 'am --scissors cuts the message at the scissors line' '\n \trm -fr .git/rebase-apply &&\n \tgit reset --hard &&\n \tgit checkout second &&\n-\tgit am --scissors scissors-patch.eml &&\n+\tgit am --scissors patch-with-scissors-line.eml &&\n \ttest_path_is_missing .git/rebase-apply &&\n-\tgit diff --exit-code scissors &&\n-\ttest_cmp_rev scissors HEAD\n+\tgit diff --exit-code scissors-used &&\n+\ttest_cmp_rev scissors-used HEAD\n '\n \n test_expect_success 'am --no-scissors overrides mailinfo.scissors' '\n@@ -429,10 +429,10 @@ test_expect_success 'am --no-scissors overrides mailinfo.scissors' '\n \tgit reset --hard &&\n \tgit checkout second &&\n \ttest_config mailinfo.scissors true &&\n-\tgit am --no-scissors no-scissors-patch.eml &&\n+\tgit am --no-scissors patch-with-scissors-line.eml &&\n \ttest_path_is_missing .git/rebase-apply &&\n-\tgit diff --exit-code no-scissors &&\n-\ttest_cmp_rev no-scissors HEAD\n+\tgit diff --exit-code scissors-not-used &&\n+\ttest_cmp_rev scissors-not-used HEAD\n '\n \n test_expect_success 'setup: new author and committer' '\n-- \n2.18.0\n\n\n"},{"id":"354456","messageId":"xmqqh8kbuk4t.fsf@gitster-ct.c.googlers.com","threadId":"49018","inReplyTo":"8f69d82b-0f35-754f-0096-853d6b463db7@gmail.com","subject":"Re: [PATCH] t4150: fix broken test for am --scissors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-03T23:04:50Z","receivedAt":"2018-08-03T23:04:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andrei Rybak <rybak.a.v@gmail.com> writes:\n\n> Tests for \"git am --[no-]scissors\" [1] work in the following way:\n>\n>  1. Create files with commit messages\n>  2. Use these files to create expected commits\n>  3. Generate eml file with patch from expected commits\n>  4. Create commits using git am with these eml files\n>  5. Compare these commits with expected\n>\n> The test for \"git am --scissors\" is supposed to take a message with a\n> scissors line above commit message and demonstrate that only the text\n> below the scissors line is included in the commit created by invocation\n> of \"git am --scissors\".  However, the setup of the test uses commits\n> without the scissors line in the commit message, therefore creating an\n> eml file without scissors line.\n>\n> This can be checked by intentionally breaking is_scissors_line function\n> in mailinfo.c. Test t4150-am.sh should fail, but does not.\n>\n> Fix broken test by generating only one eml file--with scissors line, and\n> by using it both for --scissors and --no-scissors. To clarify the\n> intention of the test, give files and tags more explicit names.\n>\n> [1]: introduced in bf72ac17d (t4150: tests for am --[no-]scissors,\n>      2015-07-19)\n>\n> Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>\n\nThanks for digging and fixing.\n\n> diff --git a/t/t4150-am.sh b/t/t4150-am.sh\n> index e9b6f8158..23e3b0e91 100755\n> --- a/t/t4150-am.sh\n> +++ b/t/t4150-am.sh\n> @@ -67,17 +67,18 @@ test_expect_success 'setup: messages' '\n>  \n>  \tEOF\n>  \n> -\tcat >scissors-msg <<-\\EOF &&\n> -\tTest git-am with scissors line\n> +\tcat >msg-without-scissors-line <<-\\EOF &&\n> +\tTest that git-am --scissors cuts at the scissors line\n>  \n>  \tThis line should be included in the commit message.\n>  \tEOF\n>  \n> -\tcat - scissors-msg >no-scissors-msg <<-\\EOF &&\n> +\tprintf \"Subject: \" >subject-prefix &&\n> +\n> +\tcat - subject-prefix msg-without-scissors-line >msg-with-scissors-line <<-\\EOF &&\n>  \tThis line should not be included in the commit message with --scissors enabled.\n>  \n>  \t - - >8 - - remove everything above this line - - >8 - -\n> -\n>  \tEOF\n\nSo... the original prepared a scissors-msg file that does not have a\nline with the scissors sign, and used it to create no-scissors-msg\nfile that does have a line with the scissors sign?  That does sound\nlike a backward way to name these files X-<.  And the renamed result\nobviously looks much saner.\n\nI see two more differences; the new one has \"Subject: \" in front of\nthe first line, and also it lacks a blank line immediately after the\nscissors line.  Do these differences matter, a reader wonders, and\nreads on...\n\n> @@ -150,18 +151,17 @@ test_expect_success setup '\n>  \t} >patch1-hg.eml &&\n>  \n>  \n> -\techo scissors-file >scissors-file &&\n> -\tgit add scissors-file &&\n> -\tgit commit -F scissors-msg &&\n> -\tgit tag scissors &&\n> -\tgit format-patch --stdout scissors^ >scissors-patch.eml &&\n\nOK, the old one created the message with formta-patch, so it\nshouldn't have \"Subject: \" there to begin with.  How does the new\none work, a reader now wonders, and reads on...\n\n> +\techo file >file &&\n> +\tgit add file &&\n> +\tgit commit -F msg-without-scissors-line &&\n> +\tgit tag scissors-used &&\n\nSo far, the commit created is more or less the same from the original,\nbut we do not yet create an e-mail message yet.\n\n>  \tgit reset --hard HEAD^ &&\n>  \n> -\techo no-scissors-file >no-scissors-file &&\n> -\tgit add no-scissors-file &&\n> -\tgit commit -F no-scissors-msg &&\n> -\tgit tag no-scissors &&\n> -\tgit format-patch --stdout no-scissors^ >no-scissors-patch.eml &&\n\nThe old one committed with scissors and told format-patch to create\nan e-mail, which does not make much sense to me, but I guess users\nare allowed to do this.\n\n> +\techo file >file &&\n> +\tgit add file &&\n> +\tgit commit -F msg-with-scissors-line &&\n> +\tgit tag scissors-not-used &&\n> +\tgit format-patch --stdout scissors-not-used^ >patch-with-scissors-line.eml &&\n\nNow we get an e-mail out of the system, presumably with a line with\nscissors marker on it in the log message.\n\n>  \tgit reset --hard HEAD^ &&\n>  \n>  \tsed -n -e \"3,\\$p\" msg >file &&\n> @@ -418,10 +418,10 @@ test_expect_success 'am --scissors cuts the message at the scissors line' '\n>  \trm -fr .git/rebase-apply &&\n>  \tgit reset --hard &&\n>  \tgit checkout second &&\n> -\tgit am --scissors scissors-patch.eml &&\n> +\tgit am --scissors patch-with-scissors-line.eml &&\n>  \ttest_path_is_missing .git/rebase-apply &&\n> -\tgit diff --exit-code scissors &&\n> -\ttest_cmp_rev scissors HEAD\n> +\tgit diff --exit-code scissors-used &&\n> +\ttest_cmp_rev scissors-used HEAD\n\nHmph, I am not quite sure what is going on.  Is the only bug in the\noriginal that scissors-patch.eml and no-scissors-patch.eml files were\nincorrectly named?  IOW, if we fed no-scissors-patch.eml (which has\na scissors line in it) with --scissors option to am, would it have\nworked just fine without other changes in this patch?\n\nI am not saying that we shouldn't make other changes or renaming the\nconfusing .eml files.  I am just trying to understand what the\nnature of the breakage was.  For example, it is not immediately\nobvious why the new test needs to prepare the message _with_\n\"Subject:\" in front of the first line when it prepares the commit\nto be used for testing.\n\n\t... goes back and thinks a bit ...\n\nOK, the Subject: thing that appears after the scissors line serves\nas an in-body header to override the subject line of the e-mail\nitself.  That change is necessary to _drop_ the subject from the\nouter e-mail and replace it with the subject we do want to use.\n\nSo I can explain why \"Subject:\" thing needed to be added.\n\nI cannot still explain why a blank line needs to be removed after\nthe scissors line, though.  We should be able to have blank lines\nbefore the in-body header, IIRC.\n\n>  '\n>  \n>  test_expect_success 'am --no-scissors overrides mailinfo.scissors' '\n> @@ -429,10 +429,10 @@ test_expect_success 'am --no-scissors overrides mailinfo.scissors' '\n>  \tgit reset --hard &&\n>  \tgit checkout second &&\n>  \ttest_config mailinfo.scissors true &&\n> -\tgit am --no-scissors no-scissors-patch.eml &&\n> +\tgit am --no-scissors patch-with-scissors-line.eml &&\n>  \ttest_path_is_missing .git/rebase-apply &&\n> -\tgit diff --exit-code no-scissors &&\n> -\ttest_cmp_rev no-scissors HEAD\n> +\tgit diff --exit-code scissors-not-used &&\n> +\ttest_cmp_rev scissors-not-used HEAD\n>  '\n>  \n>  test_expect_success 'setup: new author and committer' '\n"},{"id":"354463","messageId":"80465ead-3bf5-ffc7-59d1-7ab3770430b6@gmail.com","threadId":"49018","inReplyTo":"xmqqh8kbuk4t.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] t4150: fix broken test for am --scissors","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2018-08-04T00:39:13Z","receivedAt":"2018-08-04T00:39:19Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"On 2018-08-04 01:04, Junio C Hamano wrote:\n> Hmph, I am not quite sure what is going on.  Is the only bug in the\n> original that scissors-patch.eml and no-scissors-patch.eml files were\n> incorrectly named?  IOW, if we fed no-scissors-patch.eml (which has\n> a scissors line in it) with --scissors option to am, would it have\n> worked just fine without other changes in this patch?\n\nJust swapping eml files wouldn't be enough, because in old tests the\nprepared commits touch different files: scissors-file and\nno-scissors-file. And since tests are about cutting/keeping commit\nmessage, it doesn't make much sense to keep two eml files which differ\nonly in contents of their diffs. I'll try to reword the commit message\nto also include this bit.\n \n> I am not saying that we shouldn't make other changes or renaming the\n> confusing .eml files.  I am just trying to understand what the\n> nature of the breakage was.  For example, it is not immediately\n> obvious why the new test needs to prepare the message _with_\n> \"Subject:\" in front of the first line when it prepares the commit\n> to be used for testing.\n> \n> \t... goes back and thinks a bit ...\n> \n> OK, the Subject: thing that appears after the scissors line serves\n> as an in-body header to override the subject line of the e-mail\n> itself.  That change is necessary to _drop_ the subject from the\n> outer e-mail and replace it with the subject we do want to use.\n> \n> So I can explain why \"Subject:\" thing needed to be added.\n\nYes, the adding of \"Subject: \" prefix is completely overlooked in the\ncommit message. I'll add explanation in re-send.\n\n> I cannot still explain why a blank line needs to be removed after\n> the scissors line, though.  We should be able to have blank lines\n> before the in-body header, IIRC.\n\nI'll double check this and restore the blank line in v2, if the\nremoval is not needed. IIRC, I removed it by accident and didn't\nthink too much of it.\n\nThank you for review.\n"},{"id":"354500","messageId":"bea0e5d0-d944-ddd8-c3ab-a95355352b47@gmail.com","threadId":"49018","inReplyTo":"8f69d82b-0f35-754f-0096-853d6b463db7@gmail.com","subject":"[PATCH v2] t4150: fix broken test for am --scissors","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2018-08-04T18:10:55Z","receivedAt":"2018-08-04T18:11:01Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"Tests for \"git am --[no-]scissors\" [1] work in the following way:\n\n 1. Create files with commit messages\n 2. Use these files to create expected commits\n 3. Generate eml file with patch from expected commits\n 4. Create commits using git am with these eml files\n 5. Compare these commits with expected\n\nThe test for \"git am --scissors\" is supposed to take a message with a\nscissors line and demonstrate that the subject line from the e-mail\nitself is overridden by the in-body \"Subject:\" header and that only text\nbelow the scissors line is included in the commit message of the commit\ncreated by the invocation of \"git am --scissors\". However, the setup of\nthe test incorrectly uses a commit without the scissors line and in-body\n\"Subject:\" header in the commit message, and thus, creates eml file not\nsuitable for testing of \"git am --scissors\".\n\nThis can be checked by intentionally breaking is_scissors_line function\nin mailinfo.c, for example, by changing string \">8\", which is used by\nthe test. With such change the test should fail, but does not.\n\nFix broken test by generating eml file with scissors line and in-body\nheader \"Subject:\". Since the two tests for --scissors and --no-scissors\noptions are there to test cutting or keeping the commit message, update\nboth tests to change the test file in the same way, which allows us to\ngenerate only one eml file to be passed to git am. To clarify the\nintention of the test, give files and tags more explicit names.\n\n[1]: introduced in bf72ac17d (t4150: tests for am --[no-]scissors,\n     2015-07-19)\n\nSigned-off-by: Andrei Rybak <rybak.a.v@gmail.com>\n---\n\nApplies on top of 980a3d3dd (Merge branch 'pt/am-tests', 2015-08-03).\nThis patch is also available at\n\n  https://github.com/rybak/git fix-am-scissors-test-v2\n\nChanges since v1:\n\n - Reword commit message after feedback from Junio\n - Keep the empty line under scissors in the test e-mail, as it does not\n   affect the test\n\n t/t4150-am.sh | 39 ++++++++++++++++++++-------------------\n 1 file changed, 20 insertions(+), 19 deletions(-)\n\ndiff --git a/t/t4150-am.sh b/t/t4150-am.sh\nindex e9b6f8158..bb2d951a7 100755\n--- a/t/t4150-am.sh\n+++ b/t/t4150-am.sh\n@@ -67,13 +67,15 @@ test_expect_success 'setup: messages' '\n \n \tEOF\n \n-\tcat >scissors-msg <<-\\EOF &&\n-\tTest git-am with scissors line\n+\tcat >msg-without-scissors-line <<-\\EOF &&\n+\tTest that git-am --scissors cuts at the scissors line\n \n \tThis line should be included in the commit message.\n \tEOF\n \n-\tcat - scissors-msg >no-scissors-msg <<-\\EOF &&\n+\tprintf \"Subject: \" >subject-prefix &&\n+\n+\tcat - subject-prefix msg-without-scissors-line >msg-with-scissors-line <<-\\EOF &&\n \tThis line should not be included in the commit message with --scissors enabled.\n \n \t - - >8 - - remove everything above this line - - >8 - -\n@@ -150,18 +152,17 @@ test_expect_success setup '\n \t} >patch1-hg.eml &&\n \n \n-\techo scissors-file >scissors-file &&\n-\tgit add scissors-file &&\n-\tgit commit -F scissors-msg &&\n-\tgit tag scissors &&\n-\tgit format-patch --stdout scissors^ >scissors-patch.eml &&\n+\techo file >file &&\n+\tgit add file &&\n+\tgit commit -F msg-without-scissors-line &&\n+\tgit tag scissors-used &&\n \tgit reset --hard HEAD^ &&\n \n-\techo no-scissors-file >no-scissors-file &&\n-\tgit add no-scissors-file &&\n-\tgit commit -F no-scissors-msg &&\n-\tgit tag no-scissors &&\n-\tgit format-patch --stdout no-scissors^ >no-scissors-patch.eml &&\n+\techo file >file &&\n+\tgit add file &&\n+\tgit commit -F msg-with-scissors-line &&\n+\tgit tag scissors-not-used &&\n+\tgit format-patch --stdout scissors-not-used^ >patch-with-scissors-line.eml &&\n \tgit reset --hard HEAD^ &&\n \n \tsed -n -e \"3,\\$p\" msg >file &&\n@@ -418,10 +419,10 @@ test_expect_success 'am --scissors cuts the message at the scissors line' '\n \trm -fr .git/rebase-apply &&\n \tgit reset --hard &&\n \tgit checkout second &&\n-\tgit am --scissors scissors-patch.eml &&\n+\tgit am --scissors patch-with-scissors-line.eml &&\n \ttest_path_is_missing .git/rebase-apply &&\n-\tgit diff --exit-code scissors &&\n-\ttest_cmp_rev scissors HEAD\n+\tgit diff --exit-code scissors-used &&\n+\ttest_cmp_rev scissors-used HEAD\n '\n \n test_expect_success 'am --no-scissors overrides mailinfo.scissors' '\n@@ -429,10 +430,10 @@ test_expect_success 'am --no-scissors overrides mailinfo.scissors' '\n \tgit reset --hard &&\n \tgit checkout second &&\n \ttest_config mailinfo.scissors true &&\n-\tgit am --no-scissors no-scissors-patch.eml &&\n+\tgit am --no-scissors patch-with-scissors-line.eml &&\n \ttest_path_is_missing .git/rebase-apply &&\n-\tgit diff --exit-code no-scissors &&\n-\ttest_cmp_rev no-scissors HEAD\n+\tgit diff --exit-code scissors-not-used &&\n+\ttest_cmp_rev scissors-not-used HEAD\n '\n \n test_expect_success 'setup: new author and committer' '\n-- \n2.18.0\n\n"},{"id":"354588","messageId":"CACRoPnTWxYvGfqgGt2r5ETOhfJZnqr1fTO9Fzwp-u25XbMGPPQ@mail.gmail.com","threadId":"49018","inReplyTo":"bea0e5d0-d944-ddd8-c3ab-a95355352b47@gmail.com","subject":"Re: [PATCH v2] t4150: fix broken test for am --scissors","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2018-08-06T08:58:52Z","receivedAt":"2018-08-06T08:59:06Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Hi,\n\nI've taken a look at the original test, and it is pretty broken. My\ndeepest apologies for this mess.\n\nOn Sun, Aug 5, 2018 at 2:10 AM, Andrei Rybak <rybak.a.v@gmail.com> wrote:\n> Tests for \"git am --[no-]scissors\" [1] work in the following way:\n>\n>  1. Create files with commit messages\n>  2. Use these files to create expected commits\n>  3. Generate eml file with patch from expected commits\n>  4. Create commits using git am with these eml files\n>  5. Compare these commits with expected\n>\n> The test for \"git am --scissors\" is supposed to take a message with a\n> scissors line and demonstrate that the subject line from the e-mail\n> itself is overridden by the in-body \"Subject:\" header and that only text\n> below the scissors line is included in the commit message of the commit\n> created by the invocation of \"git am --scissors\". However, the setup of\n> the test incorrectly uses a commit without the scissors line and in-body\n> \"Subject:\" header in the commit message, and thus, creates eml file not\n> suitable for testing of \"git am --scissors\".\n\nI think what really happened was that I simply forgot that the first\nline of the commit message would be pulled out into the formatted\npatch's \"Subject\" header, and would thus not be affected by the\nscissors line :-S.\n\n> This can be checked by intentionally breaking is_scissors_line function\n> in mailinfo.c, for example, by changing string \">8\", which is used by\n> the test. With such change the test should fail, but does not.\n\nThe main reason why the test still passes even with a broken\nis_scissors_line() would be because it uses the wrong patch to pass to\n\"git am --scissors\" -- it uses the patch _without_ a scissors line\nrather than the patch _with_ the scissors line.\n\nHowever, after fixing this problem, which I'll call problem (1), the\ntest will actually fail, due to:\n\n(2) The trees of the commits `scissors` and `no-scissors` not being\nidentical, thus making test_cmp_rev fail even though the commit\nmessages of the commits are identical.\n\n(3) As mentioned above, the test not accounting for the first line of\nthe commit message being used as the \"Subject\" header and thus not\naffected by the scissors line.\n\nSo, there are 3 problems that will need to be fixed.\n\n>\n> Fix broken test by generating eml file with scissors line and in-body\n> header \"Subject:\". Since the two tests for --scissors and --no-scissors\n> options are there to test cutting or keeping the commit message, update\n> both tests to change the test file in the same way, which allows us to\n> generate only one eml file to be passed to git am. To clarify the\n> intention of the test, give files and tags more explicit names.\n>\n> [1]: introduced in bf72ac17d (t4150: tests for am --[no-]scissors,\n>      2015-07-19)\n>\n> Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>\n> ---\n>  t/t4150-am.sh | 39 ++++++++++++++++++++-------------------\n>  1 file changed, 20 insertions(+), 19 deletions(-)\n>\n> diff --git a/t/t4150-am.sh b/t/t4150-am.sh\n> index e9b6f8158..bb2d951a7 100755\n> --- a/t/t4150-am.sh\n> +++ b/t/t4150-am.sh\n> @@ -67,13 +67,15 @@ test_expect_success 'setup: messages' '\n>\n>         EOF\n>\n> -       cat >scissors-msg <<-\\EOF &&\n> -       Test git-am with scissors line\n> +       cat >msg-without-scissors-line <<-\\EOF &&\n> +       Test that git-am --scissors cuts at the scissors line\n>\n>         This line should be included in the commit message.\n>         EOF\n>\n> -       cat - scissors-msg >no-scissors-msg <<-\\EOF &&\n> +       printf \"Subject: \" >subject-prefix &&\n> +\n> +       cat - subject-prefix msg-without-scissors-line >msg-with-scissors-line <<-\\EOF &&\n>         This line should not be included in the commit message with --scissors enabled.\n>\n>          - - >8 - - remove everything above this line - - >8 - -\n\nThis fixes problem (3) by using an in-body header.\n\n> @@ -150,18 +152,17 @@ test_expect_success setup '\n>         } >patch1-hg.eml &&\n>\n>\n> -       echo scissors-file >scissors-file &&\n> -       git add scissors-file &&\n> -       git commit -F scissors-msg &&\n> -       git tag scissors &&\n> -       git format-patch --stdout scissors^ >scissors-patch.eml &&\n> +       echo file >file &&\n> +       git add file &&\n\nThis fixes the first half of problem (2) by making the naming of the\nfiles the same.\n\n> +       git commit -F msg-without-scissors-line &&\n> +       git tag scissors-used &&\n\nNit: I'm not quite sure about naming the tag \"scissors-used\" though,\nsince this commit was not created from the output of \"git am\n--scissors\". Maybe it should be named `commit-without-scissors-line`\nor something?\n\nThis hunk removes the line:\n\n    git format-patch --stdout scissors^ >scissors-patch.eml &&\n\nwithout a corresponding replacement, but that is fine because the test\nshould not be using a patch without a scissors line.\n\n>         git reset --hard HEAD^ &&\n>\n> -       echo no-scissors-file >no-scissors-file &&\n> -       git add no-scissors-file &&\n> -       git commit -F no-scissors-msg &&\n> -       git tag no-scissors &&\n> -       git format-patch --stdout no-scissors^ >no-scissors-patch.eml &&\n> +       echo file >file &&\n> +       git add file &&\n\nThis fixes the other half of problem (2) by making the naming of the\nfiles the same.\n\n> +       git commit -F msg-with-scissors-line &&\n> +       git tag scissors-not-used &&\n\nNit: Likewise, perhaps this tag could be named `commit-with-scissors-line`?\n\n> +       git format-patch --stdout scissors-not-used^ >patch-with-scissors-line.eml &&\n>         git reset --hard HEAD^ &&\n>\n>         sed -n -e \"3,\\$p\" msg >file &&\n> @@ -418,10 +419,10 @@ test_expect_success 'am --scissors cuts the message at the scissors line' '\n>         rm -fr .git/rebase-apply &&\n>         git reset --hard &&\n>         git checkout second &&\n> -       git am --scissors scissors-patch.eml &&\n> +       git am --scissors patch-with-scissors-line.eml &&\n\nThis fixes problem (1) by using the correct patch, where the commit\nmessage has a scissors line.\n\n>         test_path_is_missing .git/rebase-apply &&\n> -       git diff --exit-code scissors &&\n> -       test_cmp_rev scissors HEAD\n> +       git diff --exit-code scissors-used &&\n> +       test_cmp_rev scissors-used HEAD\n>  '\n>\n>  test_expect_success 'am --no-scissors overrides mailinfo.scissors' '\n> @@ -429,10 +430,10 @@ test_expect_success 'am --no-scissors overrides mailinfo.scissors' '\n>         git reset --hard &&\n>         git checkout second &&\n>         test_config mailinfo.scissors true &&\n> -       git am --no-scissors no-scissors-patch.eml &&\n> +       git am --no-scissors patch-with-scissors-line.eml &&\n>         test_path_is_missing .git/rebase-apply &&\n> -       git diff --exit-code no-scissors &&\n> -       test_cmp_rev no-scissors HEAD\n> +       git diff --exit-code scissors-not-used &&\n> +       test_cmp_rev scissors-not-used HEAD\n>  '\n>\n>  test_expect_success 'setup: new author and committer' '\n> --\n> 2.18.0\n>\n\nSo, this patch fixes the 3 problems with the tests, and so looks correct to me.\n\nThanks,\nPaul\n"},{"id":"354615","messageId":"xmqqpnyvr0yn.fsf@gitster-ct.c.googlers.com","threadId":"49018","inReplyTo":"CACRoPnTWxYvGfqgGt2r5ETOhfJZnqr1fTO9Fzwp-u25XbMGPPQ@mail.gmail.com","subject":"Re: [PATCH v2] t4150: fix broken test for am --scissors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-06T15:04:00Z","receivedAt":"2018-08-06T15:04:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paul Tan <pyokagan@gmail.com> writes:\n\n> I've taken a look at the original test, and it is pretty broken. My\n> ...\n> So, there are 3 problems that will need to be fixed.\n> ...\n> This fixes problem (3) by using an in-body header.\n> ...\n> This fixes the first half of problem (2) by making the naming of the\n> files the same.\n> ...\n> Nit: I'm not quite sure about naming the tag \"scissors-used\" though,\n> since this commit was not created from the output of \"git am\n> --scissors\". Maybe it should be named `commit-without-scissors-line`\n> or something?\n>\n> This hunk removes the line:\n>\n>     git format-patch --stdout scissors^ >scissors-patch.eml &&\n>\n> without a corresponding replacement, but that is fine because the test\n> should not be using a patch without a scissors line.\n> ...\n> This fixes the other half of problem (2) by making the naming of the\n> files the same.\n> ...\n> So, this patch fixes the 3 problems with the tests, and so looks correct to me.\n\nBeautifully explained.  There are many styles of patch review, and\nI'd personally call this \"think aloud to follow the patch author's\nflow of thought.\" style.  Your review is probably one of the best\nexamples of reviews in the style.  Very readable, helping readers to\nreach the same degree of understanding of what problem the patch\ntries to address and how, and demonostrates the reviewer thought\nthrough the problem just as deeply as the patch author, all of which\ngives weight to the final \"looks correct to me\".\n\nThanks, both.  Will queue.\n\n"},{"id":"354655","messageId":"a50da318-1f15-3dc8-53e0-c40365a053f7@gmail.com","threadId":"49018","inReplyTo":"CACRoPnTWxYvGfqgGt2r5ETOhfJZnqr1fTO9Fzwp-u25XbMGPPQ@mail.gmail.com","subject":"Re: [PATCH v2] t4150: fix broken test for am --scissors","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2018-08-06T17:42:54Z","receivedAt":"2018-08-06T17:42:59Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"On 2018-08-06 10:58, Paul Tan wrote:\n>> +       git commit -F msg-without-scissors-line &&\n>> +       git tag scissors-used &&\n> \n> Nit: I'm not quite sure about naming the tag \"scissors-used\" though,\n> since this commit was not created from the output of \"git am\n> --scissors\". Maybe it should be named `commit-without-scissors-line`\n> or something?\n> \n>> +       git commit -F msg-with-scissors-line &&\n>> +       git tag scissors-not-used &&\n> \n> Nit: Likewise, perhaps this tag could be named `commit-with-scissors-line`?\n\nHow about \"expected-for-scissors\" and \"expected-for-no-scissors\"?\nJunio, I'll send out v3 with updated tag names, if that's OK.\nAlso, squash-able patch below.\n\n> So, this patch fixes the 3 problems with the tests, and so looks correct to me.\n\nThank you for such thorough review.\n\n--- 8< ---\nFrom: Andrei Rybak <rybak.a.v@gmail.com>\nDate: Mon, 6 Aug 2018 19:29:03 +0200\nSubject: [PATCH] squash! t4150: fix broken test for am --scissors\n\nclarify tag names\n---\n t/t4150-am.sh | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t4150-am.sh b/t/t4150-am.sh\nindex bb2d951a70..a821dfda54 100755\n--- a/t/t4150-am.sh\n+++ b/t/t4150-am.sh\n@@ -155,14 +155,14 @@ test_expect_success setup '\n \techo file >file &&\n \tgit add file &&\n \tgit commit -F msg-without-scissors-line &&\n-\tgit tag scissors-used &&\n+\tgit tag expected-for-scissors &&\n \tgit reset --hard HEAD^ &&\n \n \techo file >file &&\n \tgit add file &&\n \tgit commit -F msg-with-scissors-line &&\n-\tgit tag scissors-not-used &&\n-\tgit format-patch --stdout scissors-not-used^ >patch-with-scissors-line.eml &&\n+\tgit tag expected-for-no-scissors &&\n+\tgit format-patch --stdout expected-for-no-scissors^ >patch-with-scissors-line.eml &&\n \tgit reset --hard HEAD^ &&\n \n \tsed -n -e \"3,\\$p\" msg >file &&\n@@ -421,8 +421,8 @@ test_expect_success 'am --scissors cuts the message at the scissors line' '\n \tgit checkout second &&\n \tgit am --scissors patch-with-scissors-line.eml &&\n \ttest_path_is_missing .git/rebase-apply &&\n-\tgit diff --exit-code scissors-used &&\n-\ttest_cmp_rev scissors-used HEAD\n+\tgit diff --exit-code expected-for-scissors &&\n+\ttest_cmp_rev expected-for-scissors HEAD\n '\n \n test_expect_success 'am --no-scissors overrides mailinfo.scissors' '\n@@ -432,8 +432,8 @@ test_expect_success 'am --no-scissors overrides mailinfo.scissors' '\n \ttest_config mailinfo.scissors true &&\n \tgit am --no-scissors patch-with-scissors-line.eml &&\n \ttest_path_is_missing .git/rebase-apply &&\n-\tgit diff --exit-code scissors-not-used &&\n-\ttest_cmp_rev scissors-not-used HEAD\n+\tgit diff --exit-code expected-for-no-scissors &&\n+\ttest_cmp_rev expected-for-no-scissors HEAD\n '\n \n test_expect_success 'setup: new author and committer' '\n-- \n2.18.0\n"},{"id":"354659","messageId":"27bb8e3b-5b1d-dcc2-b002-df6941c62ee6@gmail.com","threadId":"49018","inReplyTo":"bea0e5d0-d944-ddd8-c3ab-a95355352b47@gmail.com","subject":"[PATCH v3] t4150: fix broken test for am --scissors","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2018-08-06T17:49:38Z","receivedAt":"2018-08-06T17:49:44Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"Tests for \"git am --[no-]scissors\" [1] work in the following way:\n\n 1. Create files with commit messages\n 2. Use these files to create expected commits\n 3. Generate eml file with patch from expected commits\n 4. Create commits using git am with these eml files\n 5. Compare these commits with expected\n\nThe test for \"git am --scissors\" is supposed to take an e-mail with a\nscissors line and in-body \"Subject:\" header and demonstrate that the\nsubject line from the e-mail itself is overridden by the in-body header\nand that only text below the scissors line is included in the commit\nmessage of the commit created by the invocation of \"git am --scissors\".\nHowever, the setup of the test incorrectly uses a commit without the\nscissors line and without the in-body header in the commit message,\nproducing eml file not suitable for testing of \"git am --scissors\".\n\nThis can be checked by intentionally breaking is_scissors_line function\nin mailinfo.c, for example, by changing string \">8\", which is used by\nthe test. With such change the test should fail, but does not.\n\nFix broken test by generating eml file with scissors line and in-body\nheader \"Subject:\". Since the two tests for --scissors and --no-scissors\noptions are there to test cutting or keeping the commit message, update\nboth tests to change the test file in the same way, which allows us to\ngenerate only one eml file to be passed to git am. To clarify the\nintention of the test, give files and tags more explicit names.\n\n[1]: introduced in bf72ac17d (t4150: tests for am --[no-]scissors,\n     2015-07-19)\n\nSigned-off-by: Andrei Rybak <rybak.a.v@gmail.com>\n---\n\nApplies on top of 980a3d3dd (Merge branch 'pt/am-tests', 2015-08-03).\nThis patch is also available at\n\n  https://github.com/rybak/git fix-am-scissors-test-v3\n\nOnly changes since v2 are more clear tag names.\n\n t/t4150-am.sh | 39 ++++++++++++++++++++-------------------\n 1 file changed, 20 insertions(+), 19 deletions(-)\n\ndiff --git a/t/t4150-am.sh b/t/t4150-am.sh\nindex e9b6f8158..a821dfda5 100755\n--- a/t/t4150-am.sh\n+++ b/t/t4150-am.sh\n@@ -67,13 +67,15 @@ test_expect_success 'setup: messages' '\n \n \tEOF\n \n-\tcat >scissors-msg <<-\\EOF &&\n-\tTest git-am with scissors line\n+\tcat >msg-without-scissors-line <<-\\EOF &&\n+\tTest that git-am --scissors cuts at the scissors line\n \n \tThis line should be included in the commit message.\n \tEOF\n \n-\tcat - scissors-msg >no-scissors-msg <<-\\EOF &&\n+\tprintf \"Subject: \" >subject-prefix &&\n+\n+\tcat - subject-prefix msg-without-scissors-line >msg-with-scissors-line <<-\\EOF &&\n \tThis line should not be included in the commit message with --scissors enabled.\n \n \t - - >8 - - remove everything above this line - - >8 - -\n@@ -150,18 +152,17 @@ test_expect_success setup '\n \t} >patch1-hg.eml &&\n \n \n-\techo scissors-file >scissors-file &&\n-\tgit add scissors-file &&\n-\tgit commit -F scissors-msg &&\n-\tgit tag scissors &&\n-\tgit format-patch --stdout scissors^ >scissors-patch.eml &&\n+\techo file >file &&\n+\tgit add file &&\n+\tgit commit -F msg-without-scissors-line &&\n+\tgit tag expected-for-scissors &&\n \tgit reset --hard HEAD^ &&\n \n-\techo no-scissors-file >no-scissors-file &&\n-\tgit add no-scissors-file &&\n-\tgit commit -F no-scissors-msg &&\n-\tgit tag no-scissors &&\n-\tgit format-patch --stdout no-scissors^ >no-scissors-patch.eml &&\n+\techo file >file &&\n+\tgit add file &&\n+\tgit commit -F msg-with-scissors-line &&\n+\tgit tag expected-for-no-scissors &&\n+\tgit format-patch --stdout expected-for-no-scissors^ >patch-with-scissors-line.eml &&\n \tgit reset --hard HEAD^ &&\n \n \tsed -n -e \"3,\\$p\" msg >file &&\n@@ -418,10 +419,10 @@ test_expect_success 'am --scissors cuts the message at the scissors line' '\n \trm -fr .git/rebase-apply &&\n \tgit reset --hard &&\n \tgit checkout second &&\n-\tgit am --scissors scissors-patch.eml &&\n+\tgit am --scissors patch-with-scissors-line.eml &&\n \ttest_path_is_missing .git/rebase-apply &&\n-\tgit diff --exit-code scissors &&\n-\ttest_cmp_rev scissors HEAD\n+\tgit diff --exit-code expected-for-scissors &&\n+\ttest_cmp_rev expected-for-scissors HEAD\n '\n \n test_expect_success 'am --no-scissors overrides mailinfo.scissors' '\n@@ -429,10 +430,10 @@ test_expect_success 'am --no-scissors overrides mailinfo.scissors' '\n \tgit reset --hard &&\n \tgit checkout second &&\n \ttest_config mailinfo.scissors true &&\n-\tgit am --no-scissors no-scissors-patch.eml &&\n+\tgit am --no-scissors patch-with-scissors-line.eml &&\n \ttest_path_is_missing .git/rebase-apply &&\n-\tgit diff --exit-code no-scissors &&\n-\ttest_cmp_rev no-scissors HEAD\n+\tgit diff --exit-code expected-for-no-scissors &&\n+\ttest_cmp_rev expected-for-no-scissors HEAD\n '\n \n test_expect_success 'setup: new author and committer' '\n-- \n2.18.0\n"},{"id":"354681","messageId":"xmqqd0uvntgg.fsf@gitster-ct.c.googlers.com","threadId":"49018","inReplyTo":"27bb8e3b-5b1d-dcc2-b002-df6941c62ee6@gmail.com","subject":"Re: [PATCH v3] t4150: fix broken test for am --scissors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-06T20:14:23Z","receivedAt":"2018-08-06T20:14:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andrei Rybak <rybak.a.v@gmail.com> writes:\n\n> Tests for \"git am --[no-]scissors\" [1] work in the following way:\n>\n>  1. Create files with commit messages\n>  2. Use these files to create expected commits\n>  3. Generate eml file with patch from expected commits\n>  4. Create commits using git am with these eml files\n>  5. Compare these commits with expected\n>\n> The test for \"git am --scissors\" is supposed to take an e-mail with a\n> scissors line and in-body \"Subject:\" header and demonstrate that the\n> subject line from the e-mail itself is overridden by the in-body header\n> and that only text below the scissors line is included in the commit\n> message of the commit created by the invocation of \"git am --scissors\".\n> However, the setup of the test incorrectly uses a commit without the\n> scissors line and without the in-body header in the commit message,\n> producing eml file not suitable for testing of \"git am --scissors\".\n>\n> This can be checked by intentionally breaking is_scissors_line function\n> in mailinfo.c, for example, by changing string \">8\", which is used by\n> the test. With such change the test should fail, but does not.\n>\n> Fix broken test by generating eml file with scissors line and in-body\n> header \"Subject:\". Since the two tests for --scissors and --no-scissors\n> options are there to test cutting or keeping the commit message, update\n> both tests to change the test file in the same way, which allows us to\n> generate only one eml file to be passed to git am. To clarify the\n> intention of the test, give files and tags more explicit names.\n>\n> [1]: introduced in bf72ac17d (t4150: tests for am --[no-]scissors,\n>      2015-07-19)\n>\n> Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>\n> ---\n>\n> Applies on top of 980a3d3dd (Merge branch 'pt/am-tests', 2015-08-03).\n> This patch is also available at\n>\n>   https://github.com/rybak/git fix-am-scissors-test-v3\n>\n> Only changes since v2 are more clear tag names.\n\n... and updated log message, which I think makes it worthwhile to\nreplace the previous one plus the squash/fixup with this version.\n\nThanks.\n\n\n>  t/t4150-am.sh | 39 ++++++++++++++++++++-------------------\n>  1 file changed, 20 insertions(+), 19 deletions(-)\n>\n> diff --git a/t/t4150-am.sh b/t/t4150-am.sh\n> index e9b6f8158..a821dfda5 100755\n> --- a/t/t4150-am.sh\n> +++ b/t/t4150-am.sh\n> @@ -67,13 +67,15 @@ test_expect_success 'setup: messages' '\n>  \n>  \tEOF\n>  \n> -\tcat >scissors-msg <<-\\EOF &&\n> -\tTest git-am with scissors line\n> +\tcat >msg-without-scissors-line <<-\\EOF &&\n> +\tTest that git-am --scissors cuts at the scissors line\n>  \n>  \tThis line should be included in the commit message.\n>  \tEOF\n>  \n> -\tcat - scissors-msg >no-scissors-msg <<-\\EOF &&\n> +\tprintf \"Subject: \" >subject-prefix &&\n> +\n> +\tcat - subject-prefix msg-without-scissors-line >msg-with-scissors-line <<-\\EOF &&\n>  \tThis line should not be included in the commit message with --scissors enabled.\n>  \n>  \t - - >8 - - remove everything above this line - - >8 - -\n> @@ -150,18 +152,17 @@ test_expect_success setup '\n>  \t} >patch1-hg.eml &&\n>  \n>  \n> -\techo scissors-file >scissors-file &&\n> -\tgit add scissors-file &&\n> -\tgit commit -F scissors-msg &&\n> -\tgit tag scissors &&\n> -\tgit format-patch --stdout scissors^ >scissors-patch.eml &&\n> +\techo file >file &&\n> +\tgit add file &&\n> +\tgit commit -F msg-without-scissors-line &&\n> +\tgit tag expected-for-scissors &&\n>  \tgit reset --hard HEAD^ &&\n>  \n> -\techo no-scissors-file >no-scissors-file &&\n> -\tgit add no-scissors-file &&\n> -\tgit commit -F no-scissors-msg &&\n> -\tgit tag no-scissors &&\n> -\tgit format-patch --stdout no-scissors^ >no-scissors-patch.eml &&\n> +\techo file >file &&\n> +\tgit add file &&\n> +\tgit commit -F msg-with-scissors-line &&\n> +\tgit tag expected-for-no-scissors &&\n> +\tgit format-patch --stdout expected-for-no-scissors^ >patch-with-scissors-line.eml &&\n>  \tgit reset --hard HEAD^ &&\n>  \n>  \tsed -n -e \"3,\\$p\" msg >file &&\n> @@ -418,10 +419,10 @@ test_expect_success 'am --scissors cuts the message at the scissors line' '\n>  \trm -fr .git/rebase-apply &&\n>  \tgit reset --hard &&\n>  \tgit checkout second &&\n> -\tgit am --scissors scissors-patch.eml &&\n> +\tgit am --scissors patch-with-scissors-line.eml &&\n>  \ttest_path_is_missing .git/rebase-apply &&\n> -\tgit diff --exit-code scissors &&\n> -\ttest_cmp_rev scissors HEAD\n> +\tgit diff --exit-code expected-for-scissors &&\n> +\ttest_cmp_rev expected-for-scissors HEAD\n>  '\n>  \n>  test_expect_success 'am --no-scissors overrides mailinfo.scissors' '\n> @@ -429,10 +430,10 @@ test_expect_success 'am --no-scissors overrides mailinfo.scissors' '\n>  \tgit reset --hard &&\n>  \tgit checkout second &&\n>  \ttest_config mailinfo.scissors true &&\n> -\tgit am --no-scissors no-scissors-patch.eml &&\n> +\tgit am --no-scissors patch-with-scissors-line.eml &&\n>  \ttest_path_is_missing .git/rebase-apply &&\n> -\tgit diff --exit-code no-scissors &&\n> -\ttest_cmp_rev no-scissors HEAD\n> +\tgit diff --exit-code expected-for-no-scissors &&\n> +\ttest_cmp_rev expected-for-no-scissors HEAD\n>  '\n>  \n>  test_expect_success 'setup: new author and committer' '\n"},{"id":"354725","messageId":"CACRoPnSJm4bwRs-k5kkz4VDMCQ_r00JhQxpXUTrFUvRVODMWHg@mail.gmail.com","threadId":"49018","inReplyTo":"a50da318-1f15-3dc8-53e0-c40365a053f7@gmail.com","subject":"Re: [PATCH v2] t4150: fix broken test for am --scissors","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2018-08-07T10:53:44Z","receivedAt":"2018-08-07T10:53:46Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"On Tue, Aug 7, 2018 at 1:42 AM, Andrei Rybak <rybak.a.v@gmail.com> wrote:\n> On 2018-08-06 10:58, Paul Tan wrote:\n>>> +       git commit -F msg-without-scissors-line &&\n>>> +       git tag scissors-used &&\n>>\n>> Nit: I'm not quite sure about naming the tag \"scissors-used\" though,\n>> since this commit was not created from the output of \"git am\n>> --scissors\". Maybe it should be named `commit-without-scissors-line`\n>> or something?\n>>\n>>> +       git commit -F msg-with-scissors-line &&\n>>> +       git tag scissors-not-used &&\n>>\n>> Nit: Likewise, perhaps this tag could be named `commit-with-scissors-line`?\n>\n> How about \"expected-for-scissors\" and \"expected-for-no-scissors\"?\n\nYep that's fine.\n\nThanks,\nPaul\n"},{"id":"354726","messageId":"2757a1be-408c-27a5-f30c-1606a3a558db@gmail.com","threadId":"49018","inReplyTo":"xmqqd0uvntgg.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3] t4150: fix broken test for am --scissors","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2018-08-07T11:24:33Z","receivedAt":"2018-08-07T11:24:40Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"On 2018-08-06 22:14, Junio C Hamano wrote:\n> Andrei Rybak <rybak.a.v@gmail.com> writes:\n>>\n>> Only changes since v2 are more clear tag names.\n> \n> ... and updated log message, which I think makes it worthwhile to\n> replace the previous one plus the squash/fixup with this version.\n\nMy bad, totally forgot that I had been tweaking it after I sent out v2.\n"}]}