[RFC] broken test for git am --scissors

[RFC] broken test for git am --scissors

[Andrei Rybak]

I was tweaking is_scissors_line function in mailinfo.c and tried writing
some tests for it.  And discovered that existing test for git am option
--scissors is broken.  I then confirmed that by intentionally breaking
is_scissors_line, like this:

--- 8< ---
Subject: [PATCH 1/2] mailinfo.c: intentionally break is_scissors_line

It seems that tests for "git am" don't actually test the --scissor
option logic.  Break is_scissors_line function by using bogus symbols to
be able to check the tests.

Note that test suite does not pass with this patch applied.  The
expected failure does not happen.
---
 mailinfo.c    | 4 ++--
 t/t4150-am.sh | 3 ++-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/mailinfo.c b/mailinfo.c
index 3281a37d5..7938d85e3 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -682,8 +682,8 @@ static int is_scissors_line(const char *line)
 			perforation++;
 			continue;
 		}
-		if ((!memcmp(c, ">8", 2) || !memcmp(c, "8<", 2) ||
-		     !memcmp(c, ">%", 2) || !memcmp(c, "%<", 2))) {
+		if ((!memcmp(c, ">o", 2) || !memcmp(c, "o<", 2) ||
+		     !memcmp(c, ">/", 2) || !memcmp(c, "/<", 2))) {
 			in_perforation = 1;
 			perforation += 2;
 			scissors += 2;
diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index 1ebc587f8..59bcb5afd 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -412,7 +412,8 @@ test_expect_success 'am with failing post-applypatch hook' '
 	test_cmp head.expected head.actual
 '
 
-test_expect_success 'am --scissors cuts the message at the scissors line' '
+# Test should fail, but succeeds
+test_expect_failure 'am --scissors cuts the message at the scissors line' '
 	rm -fr .git/rebase-apply &&
 	git reset --hard &&
 	git checkout second &&

--- >8 ---

Here's a proof-of-concept patch for the test, to make it actually fail
when is_scissors_line is broken.  It is the easiest way to do so, that I
could come up with, it is not ready to be applied.  I think the two
tests for --scissors should be rewritten pretty much from scratch, with
more obvious naming of files used.

(I made the changes to files in both tests the same just to be able to
re-use file "no-scissors-patch.eml", it's not relevant to the scissor
line in the commit messages.)

--- 8< ---
Subject: [PATCH 2/2] t4150-am.sh: fix test for --scissors

Test for option --scissors should check that the eml file with a scissor
line inside will be cut up and only the part under the cut will be
turned into commit.

However, the test for --scissors generates eml file without such line.
Fix the test for --scissors option.

Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
---
 t/t4150-am.sh | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index 59bcb5afd..5ad71fe05 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -69,17 +69,22 @@ test_expect_success 'setup: messages' '
 
 	EOF
 
+	cat >new-scissors-msg <<-\EOF &&
+	Subject: Test git-am with scissors line
+
+	This line should be included in the commit message.
+	EOF
+
 	cat >scissors-msg <<-\EOF &&
 	Test git-am with scissors line
 
 	This line should be included in the commit message.
 	EOF
 
-	cat - scissors-msg >no-scissors-msg <<-\EOF &&
+	cat - new-scissors-msg >no-scissors-msg <<-\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>"
@@ -148,15 +153,15 @@ test_expect_success setup '
 	} >patch1-hg.eml &&
 
 
-	echo scissors-file >scissors-file &&
-	git add scissors-file &&
+	echo file >file &&
+	git add file &&
 	git commit -F scissors-msg &&
 	git tag scissors &&
 	git format-patch --stdout scissors^ >scissors-patch.eml &&
 	git reset --hard HEAD^ &&
 
-	echo no-scissors-file >no-scissors-file &&
-	git add no-scissors-file &&
+	echo file >file &&
+	git add file &&
 	git commit -F no-scissors-msg &&
 	git tag no-scissors &&
 	git format-patch --stdout no-scissors^ >no-scissors-patch.eml &&
@@ -417,7 +422,7 @@ test_expect_failure '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 no-scissors-patch.eml &&
 	test_path_is_missing .git/rebase-apply &&
 	git diff --exit-code scissors &&
 	test_cmp_rev scissors HEAD
--

--- >8 ---

Relevant old threads:

1. https://public-inbox.org/git/1435861000-25278-11-git-send-email-pyokagan@gmail.com
2. https://public-inbox.org/git/1436278114-28057-11-git-send-email-pyokagan@gmail.com

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

[Andrei Rybak]

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

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

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

[Junio C Hamano]

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

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

[Andrei Rybak]

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.

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

[Andrei Rybak]

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 and demonstrate that the subject line from the e-mail
itself is overridden by the in-body "Subject:" header and that only text
below the scissors line is included in the commit message of the commit
created by the invocation of "git am --scissors". However, the setup of
the test incorrectly uses a commit without the scissors line and in-body
"Subject:" header in the commit message, and thus, creates eml file not
suitable for testing of "git am --scissors".

This can be checked by intentionally breaking is_scissors_line function
in mailinfo.c, for example, by changing string ">8", which is used by
the test. With such change the test should fail, but does not.

Fix broken test by generating eml file with scissors line and in-body
header "Subject:". Since the two tests for --scissors and --no-scissors
options are there to test cutting or keeping the commit message, update
both tests to change the test file in the same way, which allows us to
generate only one eml file to be passed to git am. 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>
---

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

Changes since v1:

 - Reword commit message after feedback from Junio
 - Keep the empty line under scissors in the test e-mail, as it does not
   affect the test

 t/t4150-am.sh | 39 ++++++++++++++++++++-------------------
 1 file changed, 20 insertions(+), 19 deletions(-)

diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index e9b6f8158..bb2d951a7 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -67,13 +67,15 @@ 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 - -
@@ -150,18 +152,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 +419,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 +430,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

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

[Paul Tan]

Hi,

I've taken a look at the original test, and it is pretty broken. My
deepest apologies for this mess.

On Sun, Aug 5, 2018 at 2:10 AM, Andrei Rybak <rybak.a.v@gmail.com> wrote:
> 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 and demonstrate that the subject line from the e-mail
> itself is overridden by the in-body "Subject:" header and that only text
> below the scissors line is included in the commit message of the commit
> created by the invocation of "git am --scissors". However, the setup of
> the test incorrectly uses a commit without the scissors line and in-body
> "Subject:" header in the commit message, and thus, creates eml file not
> suitable for testing of "git am --scissors".

I think what really happened was that I simply forgot that the first
line of the commit message would be pulled out into the formatted
patch's "Subject" header, and would thus not be affected by the
scissors line :-S.

> This can be checked by intentionally breaking is_scissors_line function
> in mailinfo.c, for example, by changing string ">8", which is used by
> the test. With such change the test should fail, but does not.

The main reason why the test still passes even with a broken
is_scissors_line() would be because it uses the wrong patch to pass to
"git am --scissors" -- it uses the patch _without_ a scissors line
rather than the patch _with_ the scissors line.

However, after fixing this problem, which I'll call problem (1), the
test will actually fail, due to:

(2) The trees of the commits `scissors` and `no-scissors` not being
identical, thus making test_cmp_rev fail even though the commit
messages of the commits are identical.

(3) As mentioned above, the test not accounting for the first line of
the commit message being used as the "Subject" header and thus not
affected by the scissors line.

So, there are 3 problems that will need to be fixed.

>
> Fix broken test by generating eml file with scissors line and in-body
> header "Subject:". Since the two tests for --scissors and --no-scissors
> options are there to test cutting or keeping the commit message, update
> both tests to change the test file in the same way, which allows us to
> generate only one eml file to be passed to git am. 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>
> ---
>  t/t4150-am.sh | 39 ++++++++++++++++++++-------------------
>  1 file changed, 20 insertions(+), 19 deletions(-)
>
> diff --git a/t/t4150-am.sh b/t/t4150-am.sh
> index e9b6f8158..bb2d951a7 100755
> --- a/t/t4150-am.sh
> +++ b/t/t4150-am.sh
> @@ -67,13 +67,15 @@ 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 - -

This fixes problem (3) by using an in-body header.

> @@ -150,18 +152,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 &&

This fixes the first half of problem (2) by making the naming of the
files the same.

> +       git commit -F msg-without-scissors-line &&
> +       git tag scissors-used &&

Nit: I'm not quite sure about naming the tag "scissors-used" though,
since this commit was not created from the output of "git am
--scissors". Maybe it should be named `commit-without-scissors-line`
or something?

This hunk removes the line:

    git format-patch --stdout scissors^ >scissors-patch.eml &&

without a corresponding replacement, but that is fine because the test
should not be using a patch without a scissors line.

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

This fixes the other half of problem (2) by making the naming of the
files the same.

> +       git commit -F msg-with-scissors-line &&
> +       git tag scissors-not-used &&

Nit: Likewise, perhaps this tag could be named `commit-with-scissors-line`?

> +       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 +419,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 &&

This fixes problem (1) by using the correct patch, where the commit
message has a scissors line.

>         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 +430,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
>

So, this patch fixes the 3 problems with the tests, and so looks correct to me.

Thanks,
Paul

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

[Junio C Hamano]

Paul Tan <pyokagan@gmail.com> writes:

> I've taken a look at the original test, and it is pretty broken. My
> ...
> So, there are 3 problems that will need to be fixed.
> ...
> This fixes problem (3) by using an in-body header.
> ...
> This fixes the first half of problem (2) by making the naming of the
> files the same.
> ...
> Nit: I'm not quite sure about naming the tag "scissors-used" though,
> since this commit was not created from the output of "git am
> --scissors". Maybe it should be named `commit-without-scissors-line`
> or something?
>
> This hunk removes the line:
>
>     git format-patch --stdout scissors^ >scissors-patch.eml &&
>
> without a corresponding replacement, but that is fine because the test
> should not be using a patch without a scissors line.
> ...
> This fixes the other half of problem (2) by making the naming of the
> files the same.
> ...
> So, this patch fixes the 3 problems with the tests, and so looks correct to me.

Beautifully explained.  There are many styles of patch review, and
I'd personally call this "think aloud to follow the patch author's
flow of thought." style.  Your review is probably one of the best
examples of reviews in the style.  Very readable, helping readers to
reach the same degree of understanding of what problem the patch
tries to address and how, and demonostrates the reviewer thought
through the problem just as deeply as the patch author, all of which
gives weight to the final "looks correct to me".

Thanks, both.  Will queue.

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

[Andrei Rybak]

On 2018-08-06 10:58, Paul Tan wrote:
>> +       git commit -F msg-without-scissors-line &&
>> +       git tag scissors-used &&
> 
> Nit: I'm not quite sure about naming the tag "scissors-used" though,
> since this commit was not created from the output of "git am
> --scissors". Maybe it should be named `commit-without-scissors-line`
> or something?
> 
>> +       git commit -F msg-with-scissors-line &&
>> +       git tag scissors-not-used &&
> 
> Nit: Likewise, perhaps this tag could be named `commit-with-scissors-line`?

How about "expected-for-scissors" and "expected-for-no-scissors"?
Junio, I'll send out v3 with updated tag names, if that's OK.
Also, squash-able patch below.

> So, this patch fixes the 3 problems with the tests, and so looks correct to me.

Thank you for such thorough review.

--- 8< ---
From: Andrei Rybak <rybak.a.v@gmail.com>
Date: Mon, 6 Aug 2018 19:29:03 +0200
Subject: [PATCH] squash! t4150: fix broken test for am --scissors

clarify tag names
---
 t/t4150-am.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index bb2d951a70..a821dfda54 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -155,14 +155,14 @@ test_expect_success setup '
 	echo file >file &&
 	git add file &&
 	git commit -F msg-without-scissors-line &&
-	git tag scissors-used &&
+	git tag expected-for-scissors &&
 	git reset --hard HEAD^ &&
 
 	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 tag expected-for-no-scissors &&
+	git format-patch --stdout expected-for-no-scissors^ >patch-with-scissors-line.eml &&
 	git reset --hard HEAD^ &&
 
 	sed -n -e "3,\$p" msg >file &&
@@ -421,8 +421,8 @@ test_expect_success 'am --scissors cuts the message at the scissors line' '
 	git checkout second &&
 	git am --scissors patch-with-scissors-line.eml &&
 	test_path_is_missing .git/rebase-apply &&
-	git diff --exit-code scissors-used &&
-	test_cmp_rev scissors-used HEAD
+	git diff --exit-code expected-for-scissors &&
+	test_cmp_rev expected-for-scissors HEAD
 '
 
 test_expect_success 'am --no-scissors overrides mailinfo.scissors' '
@@ -432,8 +432,8 @@ test_expect_success 'am --no-scissors overrides mailinfo.scissors' '
 	test_config mailinfo.scissors true &&
 	git am --no-scissors patch-with-scissors-line.eml &&
 	test_path_is_missing .git/rebase-apply &&
-	git diff --exit-code scissors-not-used &&
-	test_cmp_rev scissors-not-used HEAD
+	git diff --exit-code expected-for-no-scissors &&
+	test_cmp_rev expected-for-no-scissors HEAD
 '
 
 test_expect_success 'setup: new author and committer' '
-- 
2.18.0

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

[Paul Tan]

On Tue, Aug 7, 2018 at 1:42 AM, Andrei Rybak <rybak.a.v@gmail.com> wrote:
> On 2018-08-06 10:58, Paul Tan wrote:
>>> +       git commit -F msg-without-scissors-line &&
>>> +       git tag scissors-used &&
>>
>> Nit: I'm not quite sure about naming the tag "scissors-used" though,
>> since this commit was not created from the output of "git am
>> --scissors". Maybe it should be named `commit-without-scissors-line`
>> or something?
>>
>>> +       git commit -F msg-with-scissors-line &&
>>> +       git tag scissors-not-used &&
>>
>> Nit: Likewise, perhaps this tag could be named `commit-with-scissors-line`?
>
> How about "expected-for-scissors" and "expected-for-no-scissors"?

Yep that's fine.

Thanks,
Paul

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

[Andrei Rybak]

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 an e-mail with a
scissors line and in-body "Subject:" header and demonstrate that the
subject line from the e-mail itself is overridden by the in-body header
and that only text below the scissors line is included in the commit
message of the commit created by the invocation of "git am --scissors".
However, the setup of the test incorrectly uses a commit without the
scissors line and without the in-body header in the commit message,
producing eml file not suitable for testing of "git am --scissors".

This can be checked by intentionally breaking is_scissors_line function
in mailinfo.c, for example, by changing string ">8", which is used by
the test. With such change the test should fail, but does not.

Fix broken test by generating eml file with scissors line and in-body
header "Subject:". Since the two tests for --scissors and --no-scissors
options are there to test cutting or keeping the commit message, update
both tests to change the test file in the same way, which allows us to
generate only one eml file to be passed to git am. 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>
---

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

Only changes since v2 are more clear tag names.

 t/t4150-am.sh | 39 ++++++++++++++++++++-------------------
 1 file changed, 20 insertions(+), 19 deletions(-)

diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index e9b6f8158..a821dfda5 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -67,13 +67,15 @@ 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 - -
@@ -150,18 +152,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 expected-for-scissors &&
 	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 expected-for-no-scissors &&
+	git format-patch --stdout expected-for-no-scissors^ >patch-with-scissors-line.eml &&
 	git reset --hard HEAD^ &&
 
 	sed -n -e "3,\$p" msg >file &&
@@ -418,10 +419,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 expected-for-scissors &&
+	test_cmp_rev expected-for-scissors HEAD
 '
 
 test_expect_success 'am --no-scissors overrides mailinfo.scissors' '
@@ -429,10 +430,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 expected-for-no-scissors &&
+	test_cmp_rev expected-for-no-scissors HEAD
 '
 
 test_expect_success 'setup: new author and committer' '
-- 
2.18.0

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

[Junio C Hamano]

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 an e-mail with a
> scissors line and in-body "Subject:" header and demonstrate that the
> subject line from the e-mail itself is overridden by the in-body header
> and that only text below the scissors line is included in the commit
> message of the commit created by the invocation of "git am --scissors".
> However, the setup of the test incorrectly uses a commit without the
> scissors line and without the in-body header in the commit message,
> producing eml file not suitable for testing of "git am --scissors".
>
> This can be checked by intentionally breaking is_scissors_line function
> in mailinfo.c, for example, by changing string ">8", which is used by
> the test. With such change the test should fail, but does not.
>
> Fix broken test by generating eml file with scissors line and in-body
> header "Subject:". Since the two tests for --scissors and --no-scissors
> options are there to test cutting or keeping the commit message, update
> both tests to change the test file in the same way, which allows us to
> generate only one eml file to be passed to git am. 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>
> ---
>
> 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-v3
>
> Only changes since v2 are more clear tag names.

... and updated log message, which I think makes it worthwhile to
replace the previous one plus the squash/fixup with this version.

Thanks.


>  t/t4150-am.sh | 39 ++++++++++++++++++++-------------------
>  1 file changed, 20 insertions(+), 19 deletions(-)
>
> diff --git a/t/t4150-am.sh b/t/t4150-am.sh
> index e9b6f8158..a821dfda5 100755
> --- a/t/t4150-am.sh
> +++ b/t/t4150-am.sh
> @@ -67,13 +67,15 @@ 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 - -
> @@ -150,18 +152,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 expected-for-scissors &&
>  	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 expected-for-no-scissors &&
> +	git format-patch --stdout expected-for-no-scissors^ >patch-with-scissors-line.eml &&
>  	git reset --hard HEAD^ &&
>  
>  	sed -n -e "3,\$p" msg >file &&
> @@ -418,10 +419,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 expected-for-scissors &&
> +	test_cmp_rev expected-for-scissors HEAD
>  '
>  
>  test_expect_success 'am --no-scissors overrides mailinfo.scissors' '
> @@ -429,10 +430,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 expected-for-no-scissors &&
> +	test_cmp_rev expected-for-no-scissors HEAD
>  '
>  
>  test_expect_success 'setup: new author and committer' '

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

[Andrei Rybak]

On 2018-08-06 22:14, Junio C Hamano wrote:
> Andrei Rybak <rybak.a.v@gmail.com> writes:
>>
>> Only changes since v2 are more clear tag names.
> 
> ... and updated log message, which I think makes it worthwhile to
> replace the previous one plus the squash/fixup with this version.

My bad, totally forgot that I had been tweaking it after I sent out v2.