Re: [PATCH] t4150: fix broken test for am --scissors

[Junio C Hamano <gitster@pobox.com>]

Andrei Rybak <rybak.a.v@gmail.com> 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@gmail.com>

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' '