Re: [PATCH v5] git-am: allow e-mail file(s) as input

Stephen Boyd [off-list ref] writes:

Nicolas Sebrecht wrote:

quoted

diff --git a/git-am.sh b/git-am.sh
index d64d997..2b55ddc 100755
--- a/git-am.sh
+++ b/git-am.sh

@@ -162,6 +162,17 @@ check_patch_format () {
 		return 0
 	fi
 
+	# Then, accept what really looks like (series of) email(s).
+	# the first sed select headers but the folded ones
+	sed -e '/^$/q' -e '/^[[:blank:]]/d' "$1" |
+	# this one is necessary for the next 'grep -v'
+	sed -e '/^$/d' |
+	grep -v -E -e '^[A-Za-z]+(-[A-Za-z]+)*:' ||
+	{
+		patch_format=mbox
+		return 0
+	}
+
 	# otherwise, check the first few lines of the first patch to try
 	# to detect its format
 	{

This fails t4150-am.sh #10 (am -3 -q is quiet). You should redirect the
output of the sed and grep to /dev/null like Junio did in his "how about
this" patch.

Honestly speaking, I do not understand why Nicolas changed my patch at
all.

This patch wastes an extra sed process, introduces [[:blank::]] where
space and tab inside [] is perfectly adequate, and we know the latter is
understood by everybody's sed.

The worst part is that this check was moved before the most common case of
mbox file for which none of the overhead for this this extra processing is
necessary.

Admittedly, it was a "something like this" patch and wasn't tested at all,
and I would not be entirely surprised if he saw some breakages in it after
testing with my patch, but if that was the case, some comment after ---
would have been very helpful.  Nicolas?

Also, writing some tests would be helpful.

That is true.  A test would illustrate why this more expensive test must
come before the existing cheaper tests for common cases (if such a
breakage was the reason his patch looks different from mine), for example.

The 16/07/09, Junio C Hamano wrote:

Stephen Boyd [off-list ref] writes:

quoted

Nicolas Sebrecht wrote:

quoted

quoted

+	# Then, accept what really looks like (series of) email(s).
+	# the first sed select headers but the folded ones
+	sed -e '/^$/q' -e '/^[[:blank:]]/d' "$1" |
+	# this one is necessary for the next 'grep -v'
+	sed -e '/^$/d' |
+	grep -v -E -e '^[A-Za-z]+(-[A-Za-z]+)*:' ||
+	{
+		patch_format=mbox
+		return 0
+	}
+
 	# otherwise, check the first few lines of the first patch to try
 	# to detect its format
 	{

This fails t4150-am.sh #10 (am -3 -q is quiet). You should redirect the
output of the sed and grep to /dev/null like Junio did in his "how about
this" patch.

Thank you.

Honestly speaking, I do not understand why Nicolas changed my patch at
all.

This patch wastes an extra sed process

Should we really worry about that in a script like git-am.sh? I mean,
does it matter in a day to day work?

                                        introduces [[:blank::]] where
space and tab inside [] is perfectly adequate, and we know the latter is
understood by everybody's sed.

But is harder to read in editors.

The worst part is that this check was moved before the most common case of
mbox file for which none of the overhead for this this extra processing is
necessary.

Well, I did this move just because of the logical structure of the code.
That said, you're right about the overhead.

-- 
Nicolas Sebrecht

The 16/07/09, Nicolas Sebrecht wrote:

The 16/07/09, Junio C Hamano wrote:

quoted

Stephen Boyd [off-list ref] writes:

quoted

Nicolas Sebrecht wrote:

quoted

quoted

+	# Then, accept what really looks like (series of) email(s).
+	# the first sed select headers but the folded ones
+	sed -e '/^$/q' -e '/^[[:blank:]]/d' "$1" |
+	# this one is necessary for the next 'grep -v'
+	sed -e '/^$/d' |
+	grep -v -E -e '^[A-Za-z]+(-[A-Za-z]+)*:' ||
+	{
+		patch_format=mbox
+		return 0
+	}
+
 	# otherwise, check the first few lines of the first patch to try
 	# to detect its format
 	{

This fails t4150-am.sh #10 (am -3 -q is quiet). You should redirect the
output of the sed and grep to /dev/null like Junio did in his "how about
this" patch.

Thank you.

quoted

Honestly speaking, I do not understand why Nicolas changed my patch at
all.

This patch wastes an extra sed process

Should we really worry about that in a script like git-am.sh? I mean,
does it matter in a day to day work?

Oh I've forgotten, yes we need it: sed -e '/^$/q' leaves this matching
line to the output. An extra CRLF makes 'grep -v' fail.

quoted

                                        introduces [[:blank::]] where
space and tab inside [] is perfectly adequate, and we know the latter is
understood by everybody's sed.

But is harder to read in editors.

quoted

The worst part is that this check was moved before the most common case of
mbox file for which none of the overhead for this this extra processing is
necessary.

Well, I did this move just because of the logical structure of the code.
That said, you're right about the overhead.

-- 
Nicolas Sebrecht

Nicolas Sebrecht schrieb:

The 16/07/09, Junio C Hamano wrote:

quoted

                                        introduces [[:blank::]] where
space and tab inside [] is perfectly adequate, and we know the latter is
understood by everybody's sed.

But is harder to read in editors.

Tough luck. [[:blank:]] is a *portability* obstacle.

-- Hannes

Nicolas Sebrecht schrieb:

The 16/07/09, Nicolas Sebrecht wrote:

quoted

The 16/07/09, Junio C Hamano wrote:

quoted

This patch wastes an extra sed process

Should we really worry about that in a script like git-am.sh? I mean,
does it matter in a day to day work?

Oh I've forgotten, yes we need it: sed -e '/^$/q' leaves this matching
line to the output. An extra CRLF makes 'grep -v' fail.

Then make it

    sed -n -e '/^$/q' -e p

This won't print the trailing empty line.

-- Hannes

We traditionally allowed a mbox file or a directory name of a maildir to be
given to "git am".  Even though an individual file in a maildir (or more
generally, a piece of RFC2822 e-mail) is not a mbox file, it contains enough
information to create a commit out of it, so there is no reason to reject one.
It allows to run 'git am' with an email list argument, something like:

 $ git am dir/*
 $ git am email1 email2

This builds on top of a5a6755 (git-am foreign patch support: introduce
patch_format, 2009-05-27) that introduced mailbox format detection.  The
codepath to deal with a mbox requires it to begin with "From " line and
also allows it to begin with "From: ", but a random piece of e-mail can
and often do begin with any valid RFC2822 header lines.

Instead of checking the first line, we extract all the lines up to the
first empty line, and make sure they look like e-mail headers.

Signed-off-by: Nicolas Sebrecht <redacted>
---
 Documentation/git-am.txt |    6 ++--
 git-am.sh                |   14 ++++++++++++
 t/t4150-am.sh            |   54 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 71 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt
index 32e689b..2a930a7 100644
--- a/Documentation/git-am.txt
+++ b/Documentation/git-am.txt

@@ -14,7 +14,7 @@ SYNOPSIS
 	 [--ignore-date]
 	 [--whitespace=<option>] [-C<n>] [-p<n>] [--directory=<dir>]
 	 [--reject] [-q | --quiet]
-	 [<mbox> | <Maildir>...]
+	 [<mbox> | <Maildir>... | <email>... ]
 'git am' (--skip | --resolved | --abort)
 
 DESCRIPTION

@@ -25,8 +25,8 @@ current branch.
 
 OPTIONS
 -------
-<mbox>|<Maildir>...::
-	The list of mailbox files to read patches from. If you do not
+<mbox>|<Maildir>...|<email>...::
+	The list of mailbox files or email to read patches from. If you do not
 	supply this argument, the command reads from the standard input.
 	If you supply directories, they will be treated as Maildirs.
 

diff --git a/git-am.sh b/git-am.sh
index d64d997..617ca2f 100755
--- a/git-am.sh
+++ b/git-am.sh

@@ -191,6 +191,20 @@ check_patch_format () {
 			esac
 			;;
 		esac
+		if test -z "$patch_format" &&
+			test -n "$l1" &&
+			test -n "$l2" &&
+			test -n "$l3"
+		then
+			# This begins with three non-empty lines.  Is this a
+			# piece of e-mail a-la RFC2822?  Grab all the headers,
+			# discarding the indented remainder of folded lines,
+			# and see if it looks like that they all begin with the
+			# header field names...
+			sed -n -e '/^$/q' -e '/^[ 	]/d' -e p "$1" |
+			grep -v -E -e '^[A-Za-z]+(-[A-Za-z]+)*:' >/dev/null ||
+			patch_format=mbox
+		fi
 	} < "$1" || clean_abort
 }
 

diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index a12bf84..4c99240 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh

@@ -63,6 +63,53 @@ with the data reset to initial values.
 
 EOF
 
+cat >rfc2822_email <<EOF
+Return-Path: <user@domain.name>
+X-Flags: 0000
+	999
+Delivered-To: delivery to user@domain.name
+Received: (qmail invoked by alias); 16 Jul 2009 05:25:49 -0000
+Received: from vger.knl.xyz (EHLO vger.knl.xyz) [4.3.2.1]
+  by mx0.gmx.com (mx-us004) with SMTP; 16 Jul 2009 01:25:49 -0400
+Received: (majordomo@vger.knl.xyz) by vger.knl.xyz via listexpand
+	id S1757506AbZGPFZp (ORCPT <rfc822;user@domain.name>);
+	Thu, 16 Jul 2009 01:25:45 -0400
+Received: (majordomo@vger.knl.xyz) by vger.knl.xyz id F1757505AbZGPPER
+	(ORCPT <rfc822;git-outgoing>); Thu, 16 Jul 2009 01:25:45 -0400
+Received: from hsmail.qwknetllc.com ([208.71.137.138]:35086 "EHLO
+	hsmail.qwknetllc.com" rhost-flags-OK-OK-OK-OK) by vger.knl.xyz
+	with ESMTP id F1757505AbZGPPER (ORCPT <rfc822;git@vger.knl.xyz>);
+	Thu, 16 Jul 2009 01:25:44 -0400
+X-Greylist: delayed 401 seconds by postgrey-1.27 at vger.knl.xyz; Thu, 16 Jul 2009 01:25:44 EDT
+Received: (qmail 31380 invoked by uid 399); 15 Jul 2009 23:19:01 -0600
+Received: from unknown (HELO ?192.168.1.107?) (user@domain.name@1.2.3.4)
+  by hsmail.qwknetllc.com with ESMTPAM; 15 Jul 2009 23:19:01 -0600
+X-Originating-IP: 1.2.3.4
+Message-ID: <ADDDASSSS.123456789@domain.name>
+Date:	Wed, 15 Jul 2009 23:19:05 -0600
+From:	sender <user@domain.name>
+User-Agent: Thunderbird 2.0.0.22 (Windows/20090605)
+MIME-Version: 1.0
+To:	git@vger.knl.xyz
+Subject: [PATCH] apply patch from rfc2822 formated email
+Content-Type: text/plain; charset=ISO-8859-1; format=flowed
+Content-Transfer-Encoding: 7bit
+Sender:	git-owner@vger.knl.xyz
+Precedence: bulk
+List-ID: <git.vger.knl.xyz>
+X-Mailing-List:	git@vger.knl.xyz
+X-Antivirus: 0 (no virus found)
+X-Antispam: -2 (not scanned, spam filter disabled)
+X-UID: PIhtafixEX1VXO6puPmJy7wxySDc4NMwX
+Content-Length: 123465
+
+This text is part of the internal format of your mail folder, and is not
+a real message.  It is created automatically by the mail system software.
+If deleted, important folder data will be lost, and it will be re-created
+with the data reset to initial values.
+
+EOF
+
 echo "Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>" >expected
 
 test_expect_success setup '

@@ -222,6 +269,13 @@ test_expect_success 'am takes patches from a Pine mailbox' '
 	test -z "$(git diff master^..HEAD)"
 '
 
+test_expect_success 'am takes patches from a RFC2822 formated email' '
+	git checkout first &&
+	cat rfc2822_email patch1 | git am &&
+	! test -d .git/rebase-apply &&
+	test -z "$(git diff master^..HEAD)"
+'
+
 test_expect_success 'am fails on mail without patch' '
 	test_must_fail git am <failmail &&
 	rm -r .git/rebase-apply/

-- 
1.6.4.rc1.169.gd0406

Quoting Nicolas Sebrecht [off-list ref]:

We traditionally allowed a mbox file or a directory name of a maildir to be
given to "git am".  Even though an individual file in a maildir (or more
generally, a piece of RFC2822 e-mail) is not a mbox file, it contains enough
information to create a commit out of it, so there is no reason to reject one.
It allows to run 'git am' with an email list argument, something like:

 $ git am dir/*
 $ git am email1 email2

This builds on top of a5a6755 (git-am foreign patch support: introduce
patch_format, 2009-05-27) that introduced mailbox format detection.  The
codepath to deal with a mbox requires it to begin with "From " line and
also allows it to begin with "From: ", but a random piece of e-mail can
and often do begin with any valid RFC2822 header lines.

Instead of checking the first line, we extract all the lines up to the
first empty line, and make sure they look like e-mail headers.

Signed-off-by: Nicolas Sebrecht <redacted>
---

Could you summarize the changes since v5 here?  Is the change the same as Junio's patch (if so shouldn't you credit him in the commit log message)? 

I wasn't following the discussion closely, and at first I didn't understand this change to the documentation, because it doesn't say how <mbox> and <email> are different. I'm afraid many readers of the documentation don't understand it either.

Why does this description have ... in it? If I'm reading it correctly, the code in check_patch_format function checks only the first file.

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/