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

From: Andrei Rybak <rybak.a.v@gmail.com>
Date: Aug 4, 2018, 00:39 UTC
Message-ID: <80465ead-3bf5-ffc7-59d1-7ab3770430b6@gmail.com>
In-Reply-To: <xmqqh8kbuk4t.fsf@gitster-ct.c.googlers.com>

On 2018-08-04 01:04, Junio C Hamano wrote:

Show 5 quoted lines

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

Show 15 quoted lines

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