[PATCH v3] git-am: fix maildir support regression: accept email file as patch

[PATCH v3] git-am: fix maildir support regression: accept email file as patch

[Nicolas Sebrecht]

Patch format detection introduced by a5a6755a1d4707bf2fab7752e5c974ebf63d086a
may refuse valid patches.

We keep detection on the first three lines. Emails may have:
 - header fields in a random order;
 - folded lines.

Signed-off-by: Nicolas Sebrecht <nicolas.s.dev@gmx.fr>
---
 git-am.sh |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/git-am.sh b/git-am.sh
index d64d997..6190297 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -146,6 +146,7 @@ clean_abort () {
 }
 
 patch_format=
+is_email=
 
 check_patch_format () {
 	# early return if patch_format was set from the command line
@@ -191,6 +192,22 @@ check_patch_format () {
 			esac
 			;;
 		esac
+		# Keep maildir workflows support.
+		# Emails may have header fields in random order.
+		is_email='true'
+		for line in "$l1" "$l2" "$l3"
+		do
+			printf "$line" |
+				# The line may be a folded line
+				sed -e '/^$/q' -e '/^[ ]/d' |
+				grep -E -e '^[A-Za-z]+(-[A-Za-z]+)*:' >/dev/null ||
+				is_email='false'
+		done
+		# next treatments don't differ from mailbox format
+		if [ $is_email == 'true' ]
+		then
+			patch_format=mbox
+		fi
 	} < "$1" || clean_abort
 }
 
-- 
1.6.4.rc0.128.g69018

[PATCH v3] Re: git-am: fix maildir support regression: accept email file as patch

[Nicolas Sebrecht]

The 16/07/09, Nicolas Sebrecht wrote:

> Patch format detection introduced by a5a6755a1d4707bf2fab7752e5c974ebf63d086a
> may refuse valid patches.
> 
> We keep detection on the first three lines. Emails may have:
>  - header fields in a random order;
>  - folded lines.

I realized this patch is wrong. Please ignore it.

-- 
Nicolas Sebrecht

Re: [PATCH v3] git-am: fix maildir support regression: accept email file as patch

[Junio C Hamano]

Nicolas Sebrecht <nicolas.s.dev@gmx.fr> writes:

> Patch format detection introduced by a5a6755a1d4707bf2fab7752e5c974ebf63d086a
> may refuse valid patches.

This message is even worse than the previous round.

By definition what git-am does not accept is invalid, and you are
loosening that definition to include something else newly as valid.

Please describe what that new something is.

If you are claiming a5a6755 (git-am foreign patch support: introduce
patch_format, 2009-05-27) is an regression, and before it X, Y and Z
format were supported but after that only X and Y format are, then for the
same reason please specify what that Z you are resurrecting the support
for is.

That way, people who have been frustrated that their randomly formatted
files were not processible without first converting them to mbox format
will know that now their favorite format is now/again also supported.

> We keep detection on the first three lines. Emails may have:
>  - header fields in a random order;
>  - folded lines.
>
> Signed-off-by: Nicolas Sebrecht <nicolas.s.dev@gmx.fr>
> ---
>  git-am.sh |   17 +++++++++++++++++
>  1 files changed, 17 insertions(+), 0 deletions(-)
>

Please do not break the thread.  Make this message a response to my
message you lifted the idea from, which in turn was sent as a response in
the thread of v2 patch.


> +		for line in "$l1" "$l2" "$l3"
> +		do
> +			printf "$line" |
> +				# The line may be a folded line
> +				sed -e '/^$/q' -e '/^[ ]/d' |
> +				grep -E -e '^[A-Za-z]+(-[A-Za-z]+)*:' >/dev/null ||
> +				is_email='false'

Running three independent printf piped to two processes in a loop is
quite silly.

I think you did not understand the point of the three liner I sent you.

	sed -e '/^$/q' -e '/^[ 	]/d' "$1" |

    The point of this is not to use the silly "we only look at the first
    three lines" rule.  Instead, it ignores these l1/l2/l3, but grabs all
    the header lines, but discards second and subsequent physical lines if
    a logical line was folded.  Which means that the effect of this is to
    pipe the whole header (again, without worrying about the indented
    remainder of folded lines) to downsream, which is the grep -v below

        grep -v -E -e '^[A-Za-z]+(-[A-Za-z]+)*:' >/dev/null ||

    This checks if there is a line that does _NOT_ match the usual
    e-mail header pattern.  If there is such a line, it means that the
    file is not an e-mail message.  If there is no such line, we say...

        patch_format=mbox

One caveat is that the above logic alone won't catch a random file that
does not have _any_ e-mail headers in it.  So you might need to do
something like:

	LF='
        '
	case "$l1$LF$l2$LF$l3" in
        *"$LF$LF"*)
        	# has a completely empty line in there?
                # that means the message has only two headers at most;
                # that cannot be an email.
		;;
	*)
                sed -e '/^$/q' -e '/^[ 	]/d' "$1" |
                grep -v -E -e '^[A-Za-z]+(-[A-Za-z]+)*:' >/dev/null ||
                patch_format=mbox
	esac

to completely replace that "for line in..." loop.

Re: [PATCH v3] git-am: fix maildir support regression: accept email file as patch

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Please describe what that new something is.
>...
> Running three independent printf piped to two processes in a loop is
> quite silly.
> ...

That is, something like this.

-- >8 --
Subject: mailinfo: allow individual e-mail files as input

We traditionally allowed a mbox file or a directory name of a maildir (but
never an individual file inside 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.  Running
mailsplit on such a file feels stupid, but it does not hurt.

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: Junio C Hamano <gitster@pobox.com>
---
 git-am.sh |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

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
 }
 

[PATCH v3] Re: git-am: fix maildir support regression: accept email file as patch

[Nicolas Sebrecht]

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

> We traditionally allowed a mbox file or a directory name of a maildir (but
> never an individual file inside 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.  Running
> mailsplit on such a file feels stupid, but it does not hurt.

Junio, I'm sorry but you missed the point.

It is not about adding a new feature. It's about keeping compatibility
with maildir. The current version _rejects_ good patches.

But, as it's very easy to move emails from a maildir _or_ have a symlink
which links to "maildir/cur" or whatever, we really should rely on the
content of the files.

-- 
Nicolas Sebrecht

[PATCH v3] Re: git-am: fix maildir support regression: accept email file as patch

[Nicolas Sebrecht]

The 16/07/09, Nicolas Sebrecht wrote:

> It is not about adding a new feature. It's about keeping compatibility
> with maildir. The current version _rejects_ good patches.
> 
> But, as it's very easy to move emails from a maildir _or_ have a symlink
> which links to "maildir/cur" or whatever, we really should rely on the
> content of the files.

Hum, it's not true. 

Symlink as parameter didn't work before. So, it breaks things like:

	$ git am symlink/*
	$ git am directory/{anything relying on shell globbing}
	$ git am patch1 patch2

This was traditionally permitted even if git-am was not designed to. I
wonder if we should add this feature as it could break end-user
workflows.

-- 
Nicolas Sebrecht

Re: [PATCH v3] Re: git-am: fix maildir support regression: accept email file as patch

[Junio C Hamano]

Nicolas Sebrecht <nicolas.s.dev@gmx.fr> writes:

> It is not about adding a new feature. It's about keeping compatibility
> with maildir. The current version _rejects_ good patches.

You almost gave me a heart attack.  I wondered if the detection code, like
it did when it saw a stgit series, saw that the parameter was a directory
(in which case the original code blindly took it as a maildir), and added
an extra check to see each file in that specified directory is a mbox or
begins with "From: ", and reject it otherwise.

If it were what Giuseppe's patch did, clearly that would have been a
breakage that rejects a good maildir.  So I looked at the code again.

A very early part of check_patch_format says "a directory?  we say it is
mbox (even though we should call it maildir) and let split_patches() call
git-mailsplit, as it knows how to deal with a maildir".

So I do not think there is any breakage that rejects good input with his
patch.

I am not opposed to add support for individual pieces of e-mail without
forcing them to be in Berkeley mbox format.  Not everybody uses mbox
format, and it is a logical thing to do.  Also I do not think the amount
of new code necessary to do so is excessive, nor such a change is risky
even late in a cycle after -rc0.

I however _do_ have issues with labeling other's patch that did not break
any documented behaviour as a regression, even if it is to get extra
attention to the issue.  That's not how we do things.

[PATCH v3] Re: git-am: fix maildir support regression: accept email file as patch

[Nicolas Sebrecht]

The 15/07/09, Junio C Hamano wrote:
> Nicolas Sebrecht <nicolas.s.dev@gmx.fr> writes:
>
> So I do not think there is any breakage that rejects good input with his
> patch.

Exact. It broke my workflow (at least) and was able to do it because the
previous git-am was permiting it. Looking closer to the code made me
understand where I was wrong.

> I am not opposed to add support for individual pieces of e-mail without
> forcing them to be in Berkeley mbox format.  Not everybody uses mbox
> format, and it is a logical thing to do.  Also I do not think the amount
> of new code necessary to do so is excessive, nor such a change is risky
> even late in a cycle after -rc0.

Will do, then.

> I however _do_ have issues with labeling other's patch that did not break
> any documented behaviour as a regression, even if it is to get extra
> attention to the issue.  That's not how we do things.

Of course. As we was able to do more than documented, I did not see it
in first place. It's a wrong assumption coming from my initial
git-bisect, some "hidden" globbing shell, and my learning curve of the
current code.

It is _NOT_ in my intention to blame anyone.

-- 
Nicolas Sebrecht

[PATCH v3] Re: git-am: fix maildir support regression: accept email file as patch

[Nicolas Sebrecht]

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

> I think you did not understand the point of the three liner I sent you.
> 
> 	sed -e '/^$/q' -e '/^[ 	]/d' "$1" |

I'll change it to

 	sed -e '/^$/q' -e '/^[[:blank:]]/d' "$1" |

to conform to reality (and the paragraph 3.4.2 of the RFC 822).

>     The point of this is not to use the silly "we only look at the first
>     three lines" rule.  Instead, it ignores these l1/l2/l3, but grabs all
>     the header lines, but discards second and subsequent physical lines if
>     a logical line was folded.  Which means that the effect of this is to
>     pipe the whole header (again, without worrying about the indented
>     remainder of folded lines) to downsream, which is the grep -v below
> 
>         grep -v -E -e '^[A-Za-z]+(-[A-Za-z]+)*:' >/dev/null ||
>
>     This checks if there is a line that does _NOT_ match the usual
>     e-mail header pattern.  If there is such a line, it means that the
>     file is not an e-mail message.  If there is no such line, we say...

I don't see the reason to have the option -v. It's only related to
what's printed to output and doesn't change the exit status which
tell us if an expression has matched.

This gives:

   grep -E -e '^[A-Za-z]+(-[A-Za-z]+)*:' >/dev/null &&
   patch_format=mbox

> One caveat is that the above logic alone won't catch a random file that
> does not have _any_ e-mail headers in it.  So you might need to do
> something like:
> 
> 	LF='
>         '
> 	case "$l1$LF$l2$LF$l3" in
>         *"$LF$LF"*)
>         	# has a completely empty line in there?
>                 # that means the message has only two headers at most;
>                 # that cannot be an email.
> 		;;

I think we can strip this part. The purpose is to accept what _may_ be a
patch. Any wrong patch or random file will be rejected later.

-- 
Nicolas Sebrecht

Re: [PATCH v3] Re: git-am: fix maildir support regression: accept email file as patch

[Junio C Hamano]

Nicolas Sebrecht <nicolas.s.dev@gmx.fr> writes:

> I don't see the reason to have the option -v. It's only related to
> what's printed to output and doesn't change the exit status which
> tell us if an expression has matched.
>
> This gives:
>
>    grep -E -e '^[A-Za-z]+(-[A-Za-z]+)*:' >/dev/null &&
>    patch_format=mbox

That grep says "if you see a single line that matches the pattern, even if
all the other lines are garbage, report it as a match".

See "something like this" patch in my other message. It filters the entire
header to make sure there is no line that does not match (that is what -v
is about), and make it report success when there is even a signle line
that does not.

    $ (echo yes; echo no) | grep -v -e yes ; echo $?
    no
    0
    $ (echo yes; echo yes) | grep -v -e yes ; echo $?
    1

That is why I wrote the "how about this" patch with "||" like this:

    grep -v -E -e '^[A-Za-z]+(-[A-Za-z]+)*:' >/dev/null ||
    patch_format=mbox

If everything is header, grep says "nothing matches", and patch_format
is set to mbox.

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

[Nicolas Sebrecht]

We traditionally allowed a mbox file or a directory name of a maildir to be
given to "git am". Even though 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.

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 <nicolas.s.dev@gmx.fr>
---
 Documentation/git-am.txt |    6 +++---
 git-am.sh                |    8 ++++++++
 2 files changed, 11 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..87a886d 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -162,6 +162,14 @@ check_patch_format () {
 		return 0
 	fi
 
+	# then, accept (series of) email(s)
+	sed -e '/^$/q' -e '/^[[:blank:]]/d' "$1" |
+	grep -v -E -e '^[A-Za-z]+(-[A-Za-z]+)*:' >/dev/null &&
+	{
+		patch_format=mbox
+		return 0
+	}
+
 	# otherwise, check the first few lines of the first patch to try
 	# to detect its format
 	{
-- 
1.6.4.rc0.129.gdc42

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

[Nicolas Sebrecht]

The 16/07/09, Nicolas Sebrecht wrote:

> +	# then, accept (series of) email(s)
> +	sed -e '/^$/q' -e '/^[[:blank:]]/d' "$1" |
> +	grep -v -E -e '^[A-Za-z]+(-[A-Za-z]+)*:' >/dev/null &&

And again, it's wrong here.

-- 
Nicolas Sebrecht

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

[Nicolas Sebrecht]

We traditionally allowed a mbox file or a directory name of a maildir to be
given to "git am". Even though 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.

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 <nicolas.s.dev@gmx.fr>
---

This should be the last version of this *%/£ patch.
Thanks Junio for your feedbacks.

 Documentation/git-am.txt |    6 +++---
 git-am.sh                |   11 +++++++++++
 2 files changed, 14 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..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
 	{
-- 
1.6.4.rc0.129.gdc42

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

[Stephen Boyd]

Nicolas Sebrecht wrote:
> 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.

Also, writing some tests would be helpful.

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

[Junio C Hamano]

Stephen Boyd <bebarino@gmail.com> writes:

> Nicolas Sebrecht wrote:
>> 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.

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

[Nicolas Sebrecht]

The 16/07/09, Junio C Hamano wrote:
> Stephen Boyd <bebarino@gmail.com> writes:
> > Nicolas Sebrecht wrote:
> 
> >> +	# 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

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

[Nicolas Sebrecht]

The 16/07/09, Nicolas Sebrecht wrote:
> The 16/07/09, Junio C Hamano wrote:
> > Stephen Boyd <bebarino@gmail.com> writes:
> > > Nicolas Sebrecht wrote:
> > 
> > >> +	# 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?

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.

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

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

[Johannes Sixt]

Nicolas Sebrecht schrieb:
> The 16/07/09, Nicolas Sebrecht wrote:
>> The 16/07/09, Junio C Hamano wrote:
>>> 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

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

[Johannes Sixt]

Nicolas Sebrecht schrieb:
> The 16/07/09, Junio C Hamano wrote:
>>                                         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

[PATCH v6] mailinfo: allow e-mail files as input

[Nicolas Sebrecht]

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 <nicolas.s.dev@gmx.fr>
---
 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

Re: [PATCH v6] mailinfo: allow e-mail files as input

[Junio C Hamano]

Nicolas Sebrecht <nicolas.s.dev@gmx.fr> writes:

> We traditionally allowed a mbox file or a directory name of a maildir to be
> ...
> Signed-off-by: Nicolas Sebrecht <nicolas.s.dev@gmx.fr>

Thanks.

I have one more comment on the test script, but it's something I can
locally fix (iow, there is no need to resend your patch if there is no
other issue pointed out by others, and if you agree to my suggested
improvements).

> 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

Please quote that EOF if you do not mean to have any $variable substituted
in the "here document", like this:

	cat >rfc2822_email <<\EOF
        ... here document comes here ...
	EOF

Otherwise reviewers need to waste time looking for what is being replaced
in the huge here document to understand what is going on.

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

The headers look a bit too excessive to my taste, but probably you wanted
to take a real-life example.  If that is the case, I suspect the manually
added X-Flags: at the beginning defeats that purpose, though.  I'd suggest
either removing the hand-munging, or triming the Received: sequence to
make it a bit shorter.

> @@ -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)"
> +'

These days we tend to write the last step

	git diff --exit-code master^ HEAD

which allows "sh t4150-am.sh -i -v" to be more useful when debugging.  But
the existing tests in this script are old fashioned, and you matched their
style in this patch, which is very good.

We probably would want a separate patch to modernize them after 1.6.4,
though.

[PATCH v6] Re: mailinfo: allow e-mail files as input

[Nicolas Sebrecht]

The 16/07/09, Junio C Hamano wrote:
> Nicolas Sebrecht <nicolas.s.dev@gmx.fr> writes:
> 
> > We traditionally allowed a mbox file or a directory name of a maildir to be
> > ...
> > Signed-off-by: Nicolas Sebrecht <nicolas.s.dev@gmx.fr>
> 
> Thanks.

Thank you in first place. I appreciate your patience and explanations.

> I have one more comment on the test script, but it's something I can
> locally fix (iow, there is no need to resend your patch if there is no
> other issue pointed out by others, and if you agree to my suggested
> improvements).

... and thank you for leting the door open to agreements.

> > +cat >rfc2822_email <<EOF
> > +Return-Path: <user@domain.name>
> > +X-Flags: 0000
> > +	999
> 
> The headers look a bit too excessive to my taste, but probably you wanted
> to take a real-life example.

Yes.

>                               If that is the case, I suspect the manually
> added X-Flags: at the beginning defeats that purpose, though.

I don't see what purpose it defeats. What am I missing?

>                                                                I'd suggest
> either removing the hand-munging, or triming the Received: sequence to
> make it a bit shorter.

It's fine here.

> These days we tend to write the last step
> 
> 	git diff --exit-code master^ HEAD
> 
> which allows "sh t4150-am.sh -i -v" to be more useful when debugging.

I'll look at that closer.

-- 
Nicolas Sebrecht

Re: [PATCH v6] mailinfo: allow e-mail files as input

[Nanako Shiraishi]

Quoting Nicolas Sebrecht <nicolas.s.dev@gmx.fr>:

> 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 <nicolas.s.dev@gmx.fr>
> ---

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

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

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/

Re: [PATCH v6] mailinfo: allow e-mail files as input

[Junio C Hamano]

Nanako Shiraishi <nanako3@lavabit.com> writes:

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

I could tell _you_ that I prefer to see people go back to the list archive
instead of saying "I wasn't following", but I cannot say that to readers
of the documentation after this patch is applied.  After re-reading the
above, you are right---it is unclear.

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

Good eyes.

This actually is an issue with the Guiseppe's multi-format support patch
in that we assume that the command line input are of uniform type, check
only $1 and assume $2 and subsequent are suitable to be fed to the same
splitter.

I do not think it is necessary to allow mixed input.  We certainly could,
but why bother?  It is not a sensible nor common thing to do.

Also the documentation said we take only one mbox or multiple Maildirs,
but in reality we can take multiple mboxes just fine, so <mbox> should
have had "..." at the end (we could lose the ellipses from all of them for
brevity).  Oh, and it should list the other formats Giuseppe added.

    <mbox>|<maildir>|<email>::

	    One or more of the same type of mail source to read e-mails
            from.  A directory is taken as a mailbox in the maildir
            format.  A file is taken as UNIX mbox, StGit patch series
            file, or a single piece of e-mail in RFC2822 format.

[PATCH v6] Re: mailinfo: allow e-mail files as input

[Nicolas Sebrecht]

The 17/07/09, Junio C Hamano wrote:
> Nanako Shiraishi <nanako3@lavabit.com> writes:
> 
> > 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.

( Please, wrap your lines to something convenient. )

> Good eyes.
> 
> This actually is an issue with the Guiseppe's multi-format support patch
> in that we assume that the command line input are of uniform type, check
> only $1 and assume $2 and subsequent are suitable to be fed to the same
> splitter.
> 
> I do not think it is necessary to allow mixed input.  We certainly could,
> but why bother?  It is not a sensible nor common thing to do.

I agree.

> Also the documentation said we take only one mbox or multiple Maildirs,
> but in reality we can take multiple mboxes just fine, so <mbox> should
> have had "..." at the end (we could lose the ellipses from all of them for
> brevity).  Oh, and it should list the other formats Giuseppe added.
> 
>     <mbox>|<maildir>|<email>::
> 
> 	    One or more of the same type of mail source to read e-mails
>             from.  A directory is taken as a mailbox in the maildir
>             format.  A file is taken as UNIX mbox, StGit patch series
>             file, or a single piece of e-mail in RFC2822 format.

Strictly speacking, this is not what I understand from the code: only
one StGit patch series can be given at the command line.

     <mbox>|<maildir>|<stgit>|<email>::
 
 	    One or more of the same type of mail source to read e-mails
             from.  A directory is taken as a mailbox in the maildir
             format.  A file is taken as UNIX mbox, StGit patch file,
             or a single piece of e-mail in RFC2822 format.  StGit
             patches series are also supported but only one series at
             a time. 


-- 
Nicolas Sebrecht

[PATCH v7] mailinfo: allow e-mail files as input

[Nicolas Sebrecht]

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.

Credit on most of how this has been written goes to Junio.

Signed-off-by: Nicolas Sebrecht <nicolas.s.dev@gmx.fr>
---

This round after v1.6.4 release and includes last comments pointed out in

	Subject: Re: [PATCH v6] mailinfo: allow e-mail files as input
	From: Junio C Hamano <gitster@pobox.com>
	Date: Thu, 16 Jul 2009 18:05:10 -0700
	Message-ID: <7v1vog6rw9.fsf@alter.siamese.dyndns.org>

	Subject: [PATCH v6] Re: mailinfo: allow e-mail files as input
	From: Nicolas Sebrecht <nicolas.s.dev@gmx.fr>
	Date: Sat, 18 Jul 2009 00:04:24 +0200
	Message-ID: <20090717220424.GA12968@vidovic>


 Documentation/git-am.txt |   13 ++++++++-----
 git-am.sh                |   14 ++++++++++++++
 t/t4150-am.sh            |   30 ++++++++++++++++++++++++++++++
 3 files changed, 52 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt
index 32e689b..a19a82d 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,10 +25,13 @@ current branch.
 
 OPTIONS
 -------
-<mbox>|<Maildir>...::
-	The list of mailbox files 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.
+<mbox>|<Maildir>|<stgit>|<email>::
+	One or more of the same type of mail source to read e-mails
+	from.  A directory is taken as a mailbox in the maildir
+	format.  A file is taken as UNIX mbox, StGit patch file,
+	or a single piece of e-mail in RFC2822 format.  StGit
+	patches series are also supported but only one series at
+	a time.
 
 -s::
 --signoff::
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..46adbc3 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -63,6 +63,29 @@ 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
+Date:	Wed, 15 Jul 2009 23:19:05 -0600
+From:	sender <user@domain.name>
+Subject: [PATCH] apply patch from rfc2822 formated email
+Content-Type: text/plain; charset=ISO-8859-1; format=flowed
+
+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 +245,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 &&
+	git diff --exit-code 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.124.gf10b