archive mirror
 help / color / mirror / Atom feed
From: Andrei Rybak <>
To: Junio C Hamano <>,
Cc: Johannes Schindelin <>,
	Stefan Beller <>, Paul Tan <>
Subject: Re: [PATCH] t4150: fix broken test for am --scissors
Date: Sat, 4 Aug 2018 02:39:13 +0200	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

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.

  reply	other threads:[~2018-08-04  0:39 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
2018-08-04  0:39     ` Andrei Rybak [this message]
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:

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

  git send-email \ \ \ \ \ \ \ \

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