git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Andrei Rybak <rybak.a.v@gmail.com>
Cc: git@vger.kernel.org,
	Johannes Schindelin <johannes.schindelin@gmx.de>,
	Stefan Beller <sbeller@google.com>, Paul Tan <pyokagan@gmail.com>
Subject: Re: [PATCH] t4150: fix broken test for am --scissors
Date: Fri, 03 Aug 2018 16:04:50 -0700	[thread overview]
Message-ID: <xmqqh8kbuk4t.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <8f69d82b-0f35-754f-0096-853d6b463db7@gmail.com> (Andrei Rybak's message of "Sat, 4 Aug 2018 00:39:40 +0200")

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

  reply	other threads:[~2018-08-03 23:04 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-03 13:38 [RFC] broken test for git am --scissors Andrei Rybak
2018-08-03 22:39 ` [PATCH] t4150: fix broken test for " Andrei Rybak
2018-08-03 23:04   ` Junio C Hamano [this message]
2018-08-04  0:39     ` Andrei Rybak
2018-08-04 18:10   ` [PATCH v2] " Andrei Rybak
2018-08-06  8:58     ` Paul Tan
2018-08-06 15:04       ` Junio C Hamano
2018-08-06 17:42       ` Andrei Rybak
2018-08-07 10:53         ` Paul Tan
2018-08-06 17:49     ` [PATCH v3] " Andrei Rybak
2018-08-06 20:14       ` Junio C Hamano
2018-08-07 11:24         ` Andrei Rybak

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqh8kbuk4t.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=johannes.schindelin@gmx.de \
    --cc=pyokagan@gmail.com \
    --cc=rybak.a.v@gmail.com \
    --cc=sbeller@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).