threads / patch / 20125

v3git-am: fix maildir support regression: accept email file as patch

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

## tl;dr

26 messages between Jul 15, 2009 and Aug 6, 2009. Diffs are folded; open one to read it.

replies: 25people: 5as markdown or json

Nicolas Sebrecht· Jul 15, 2009, 22:19 UTC · lore

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(-)
Show changes to git-am.sh +17 −0
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
Nicolas Sebrecht· Jul 15, 2009, 22:43 UTC · re: Nicolas Sebrecht · lore

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

The 16/07/09, Nicolas Sebrecht wrote:
Show 6 quoted lines
> 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
Junio C Hamano· Jul 15, 2009, 22:54 UTC · re: Nicolas Sebrecht · lore

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

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.

Show 9 quoted lines
> 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.

Show 7 quoted lines
> +		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.
Junio C Hamano· Jul 15, 2009, 23:56 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 5 quoted lines
> 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(-)
Show changes to git-am.sh +14 −0
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
 }
 
Nicolas Sebrecht· Jul 16, 2009, 01:00 UTC · re: Junio C Hamano · lore

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

The 15/07/09, Junio C Hamano wrote:
Show 6 quoted lines
> 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
Nicolas Sebrecht· Jul 16, 2009, 02:06 UTC · re: Nicolas Sebrecht · lore

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

The 16/07/09, Nicolas Sebrecht wrote:
Show 6 quoted lines
> 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
Junio C Hamano· Jul 16, 2009, 02:30 UTC · re: Nicolas Sebrecht · lore

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

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.

Nicolas Sebrecht· Jul 16, 2009, 02:59 UTC · re: Junio C Hamano · lore

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

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.

Show 5 quoted lines
> 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
Nicolas Sebrecht· Jul 16, 2009, 00:49 UTC · re: Junio C Hamano · lore

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

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).
Show 12 quoted lines
>     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
Show 12 quoted lines
> 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
Junio C Hamano· Jul 16, 2009, 02:41 UTC · re: Nicolas Sebrecht · lore

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

Nicolas Sebrecht <nicolas.s.dev@gmx.fr> writes:
Show 8 quoted lines
> 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.

Nicolas Sebrecht· Jul 16, 2009, 04:05 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to 2 files +11 −3

Documentation/git-am.txt, git-am.sh

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
Nicolas Sebrecht· Jul 16, 2009, 04:10 UTC · re: Nicolas Sebrecht · lore

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

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
Nicolas Sebrecht· Jul 16, 2009, 05:23 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to 2 files +14 −3

Documentation/git-am.txt, git-am.sh

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
Stephen Boyd· Jul 16, 2009, 07:09 UTC · re: Nicolas Sebrecht · lore

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

Nicolas Sebrecht wrote:
Show 22 quoted lines
> 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.
Junio C Hamano· Jul 16, 2009, 07:24 UTC · re: Stephen Boyd · lore

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

Stephen Boyd <bebarino@gmail.com> writes:
Show 27 quoted lines
> 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.

Nicolas Sebrecht· Jul 16, 2009, 07:50 UTC · re: Junio C Hamano · lore

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

The 16/07/09, Junio C Hamano wrote:
Show 21 quoted lines
> 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
Nicolas Sebrecht· Jul 16, 2009, 08:06 UTC · re: Nicolas Sebrecht · lore

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

The 16/07/09, Nicolas Sebrecht wrote:
Show 32 quoted lines
> 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.

Show 12 quoted lines
> >                                         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
Johannes Sixt· Jul 16, 2009, 08:17 UTC · re: Nicolas Sebrecht · lore

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

Nicolas Sebrecht schrieb:
Show 8 quoted lines
> 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
Johannes Sixt· Jul 16, 2009, 08:12 UTC · re: Nicolas Sebrecht · lore

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

Nicolas Sebrecht schrieb:
Show 6 quoted lines
> 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
Nicolas Sebrecht· Jul 16, 2009, 17:45 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to 3 files +71 −3

Documentation/git-am.txt, git-am.sh, t/t4150-am.sh

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
Junio C Hamano· Jul 17, 2009, 01:05 UTC · re: Nicolas Sebrecht · lore

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

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

Show 12 quoted lines
> 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.

Show 8 quoted lines
> +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.

Show 10 quoted lines
> @@ -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.

Nicolas Sebrecht· Jul 17, 2009, 02:20 UTC · re: Junio C Hamano · lore

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

The 16/07/09, Junio C Hamano wrote:
Show 7 quoted lines
> 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.
Show 7 quoted lines
> > +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.
Show 5 quoted lines
> 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
Nanako Shiraishi· Jul 17, 2009, 10:06 UTC · re: Nicolas Sebrecht · lore

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

Quoting Nicolas Sebrecht <nicolas.s.dev@gmx.fr>:
Show 20 quoted lines
> 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)? 
Show 29 quoted lines
>  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/
Junio C Hamano· Jul 17, 2009, 19:54 UTC · re: Nanako Shiraishi · lore

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

Nanako Shiraishi <nanako3@lavabit.com> writes:
Show 11 quoted lines
>>  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.
Nicolas Sebrecht· Jul 17, 2009, 22:04 UTC · re: Junio C Hamano · lore

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

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. )
Show 9 quoted lines
> 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.
Show 11 quoted lines
> 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
Nicolas Sebrecht· Aug 6, 2009, 17:07 UTC · re: Nicolas Sebrecht · lore

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

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(-)
Show changes to 3 files +52 −5

Documentation/git-am.txt, git-am.sh, t/t4150-am.sh

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

← back to recent threads