Re: [PATCH] t4150: fix broken test for am --scissors
On 2018-08-04 01:04, Junio C Hamano wrote: > 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? Just swapping eml files wouldn't be enough, because in old tests the prepared commits touch different files: scissors-file and no-scissors-file. And since tests are about cutting/keeping commit message, it doesn't make much sense to keep two eml files which differ only in contents of their diffs. I'll try to reword the commit message to also include this bit. > 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. Yes, the adding of "Subject: " prefix is completely overlooked in the commit message. I'll add explanation in re-send. > 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. I'll double check this and restore the blank line in v2, if the removal is not needed. IIRC, I removed it by accident and didn't think too much of it. Thank you for review.
Re: [PATCH] t4150: fix broken test for am --scissors
Andrei Rybak 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 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
[PATCH] t4150: fix broken test for am --scissors
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 --- Applies on top of 980a3d3dd (Merge branch 'pt/am-tests', 2015-08-03). This patch is also available at https://github.com/rybak/git fix-am-scissors-test t/t4150-am.sh | 40 1 file changed, 20 insertions(+), 20 deletions(-) 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 signoff="Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>" @@ -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 && + echo file >file && + git add file && + git commit -F msg-without-scissors-line && + git tag scissors-used && 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 && + 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 && 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 ' 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' ' -- 2.18.0
