Andrei Rybak <[email protected]> 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 <[email protected]>
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' '
