Andrei Rybak <rybak.a.v@xxxxxxxxx> writes: > Tests for "git am --[no-]scissors" [1] work in the following way: > > 1. Create files with commit messages > 2. Use these files to create expected commits > 3. Generate eml file with patch from expected commits > 4. Create commits using git am with these eml files > 5. Compare these commits with expected > > The test for "git am --scissors" is supposed to take a message with a > scissors line above commit message and demonstrate that only the text > below the scissors line is included in the commit created by invocation > of "git am --scissors". However, the setup of the test uses commits > without the scissors line in the commit message, therefore creating an > eml file without scissors line. > > This can be checked by intentionally breaking is_scissors_line function > in mailinfo.c. Test t4150-am.sh should fail, but does not. > > Fix broken test by generating only one eml file--with scissors line, and > by using it both for --scissors and --no-scissors. To clarify the > intention of the test, give files and tags more explicit names. > > [1]: introduced in bf72ac17d (t4150: tests for am --[no-]scissors, > 2015-07-19) > > Signed-off-by: Andrei Rybak <rybak.a.v@xxxxxxxxx> Thanks for digging and fixing. > diff --git a/t/t4150-am.sh b/t/t4150-am.sh > index e9b6f8158..23e3b0e91 100755 > --- a/t/t4150-am.sh > +++ b/t/t4150-am.sh > @@ -67,17 +67,18 @@ test_expect_success 'setup: messages' ' > > EOF > > - cat >scissors-msg <<-\EOF && > - Test git-am with scissors line > + cat >msg-without-scissors-line <<-\EOF && > + Test that git-am --scissors cuts at the scissors line > > This line should be included in the commit message. > EOF > > - cat - scissors-msg >no-scissors-msg <<-\EOF && > + printf "Subject: " >subject-prefix && > + > + cat - subject-prefix msg-without-scissors-line >msg-with-scissors-line <<-\EOF && > This line should not be included in the commit message with --scissors enabled. > > - - >8 - - remove everything above this line - - >8 - - > - > EOF So... the original prepared a scissors-msg file that does not have a line with the scissors sign, and used it to create no-scissors-msg file that does have a line with the scissors sign? That does sound like a backward way to name these files X-<. And the renamed result obviously looks much saner. I see two more differences; the new one has "Subject: " in front of the first line, and also it lacks a blank line immediately after the scissors line. Do these differences matter, a reader wonders, and reads on... > @@ -150,18 +151,17 @@ test_expect_success setup ' > } >patch1-hg.eml && > > > - echo scissors-file >scissors-file && > - git add scissors-file && > - git commit -F scissors-msg && > - git tag scissors && > - git format-patch --stdout scissors^ >scissors-patch.eml && OK, the old one created the message with formta-patch, so it shouldn't have "Subject: " there to begin with. How does the new one work, a reader now wonders, and reads on... > + echo file >file && > + git add file && > + git commit -F msg-without-scissors-line && > + git tag scissors-used && So far, the commit created is more or less the same from the original, but we do not yet create an e-mail message yet. > git reset --hard HEAD^ && > > - echo no-scissors-file >no-scissors-file && > - git add no-scissors-file && > - git commit -F no-scissors-msg && > - git tag no-scissors && > - git format-patch --stdout no-scissors^ >no-scissors-patch.eml && The old one committed with scissors and told format-patch to create an e-mail, which does not make much sense to me, but I guess users are allowed to do this. > + echo file >file && > + git add file && > + git commit -F msg-with-scissors-line && > + git tag scissors-not-used && > + git format-patch --stdout scissors-not-used^ >patch-with-scissors-line.eml && Now we get an e-mail out of the system, presumably with a line with scissors marker on it in the log message. > git reset --hard HEAD^ && > > sed -n -e "3,\$p" msg >file && > @@ -418,10 +418,10 @@ test_expect_success 'am --scissors cuts the message at the scissors line' ' > rm -fr .git/rebase-apply && > git reset --hard && > git checkout second && > - git am --scissors scissors-patch.eml && > + git am --scissors patch-with-scissors-line.eml && > test_path_is_missing .git/rebase-apply && > - git diff --exit-code scissors && > - test_cmp_rev scissors HEAD > + git diff --exit-code scissors-used && > + test_cmp_rev scissors-used HEAD Hmph, I am not quite sure what is going on. Is the only bug in the original that scissors-patch.eml and no-scissors-patch.eml files were incorrectly named? IOW, if we fed no-scissors-patch.eml (which has a scissors line in it) with --scissors option to am, would it have worked just fine without other changes in this patch? I am not saying that we shouldn't make other changes or renaming the confusing .eml files. I am just trying to understand what the nature of the breakage was. For example, it is not immediately obvious why the new test needs to prepare the message _with_ "Subject:" in front of the first line when it prepares the commit to be used for testing. ... goes back and thinks a bit ... OK, the Subject: thing that appears after the scissors line serves as an in-body header to override the subject line of the e-mail itself. That change is necessary to _drop_ the subject from the outer e-mail and replace it with the subject we do want to use. So I can explain why "Subject:" thing needed to be added. I cannot still explain why a blank line needs to be removed after the scissors line, though. We should be able to have blank lines before the in-body header, IIRC. > ' > > test_expect_success 'am --no-scissors overrides mailinfo.scissors' ' > @@ -429,10 +429,10 @@ test_expect_success 'am --no-scissors overrides mailinfo.scissors' ' > git reset --hard && > git checkout second && > test_config mailinfo.scissors true && > - git am --no-scissors no-scissors-patch.eml && > + git am --no-scissors patch-with-scissors-line.eml && > test_path_is_missing .git/rebase-apply && > - git diff --exit-code no-scissors && > - test_cmp_rev no-scissors HEAD > + git diff --exit-code scissors-not-used && > + test_cmp_rev scissors-not-used HEAD > ' > > test_expect_success 'setup: new author and committer' '
