[PATCH] am: fix patch format detection for Thunderbird "Save As" emails

[PATCH] am: fix patch format detection for Thunderbird "Save As" emails

[Stephen Boyd]

The patch detection wants to inspect all the headers of a rfc2822 message
and ensure that they look like header field names. The headers are always
separated from the message body with a blank line. When Thunderbird3 saves
the message the blank line separating the headers from the body includes a
CR. The patch detection is failing because a CRLF doesn't match /^$/. Fix
this by allowing a CR to exist on the separating line.
---

I'm not sure how portable \r in a sed invocation is. Perhaps just checking
that l1, l2, and l3 are rfc2822 header fields (or indented lines) is better
than trying to check all of the headers?

This seems related to

  am fails to apply patches for files with CRLF lineendings
  http://article.gmane.org/gmane.comp.version-control.git/135229

but seems necessary because check_patch_format() is called before any
splitting with mailsplit is done (where I assume the fix for the issue
will be done).

 git-am.sh     |    2 +-
 t/t4150-am.sh |   15 +++++++++++++++
 2 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/git-am.sh b/git-am.sh
index 4838cdb..bb106b7 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -204,7 +204,7 @@ check_patch_format () {
 			# 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" |
+			sed -n -e '/^\r*$/q' -e '/^[ 	]/d' -e p "$1" |
 			sane_egrep -v '^[!-9;-~]+:' >/dev/null ||
 			patch_format=mbox
 		fi
diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index 8296605..578bc81 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -83,6 +83,12 @@ test_expect_success setup '
 		echo "X-Fake-Field: Line Three" &&
 		git format-patch --stdout first | sed -e "1d"
 	} > patch1.eml &&
+	{
+		echo "X-Fake-Field: Line One" &&
+		echo "X-Fake-Field: Line Two" &&
+		echo "X-Fake-Field: Line Three" &&
+		git format-patch --stdout first | sed -e "1d"
+	} | sed -e "s/$/\r/" > patch1-crlf.eml &&
 	sed -n -e "3,\$p" msg >file &&
 	git add file &&
 	test_tick &&
@@ -123,6 +129,15 @@ test_expect_success 'am applies patch e-mail not in a mbox' '
 	test "$(git rev-parse second^)" = "$(git rev-parse HEAD^)"
 '
 
+test_expect_success 'am applies patch e-mail not in a mbox with CRLF' '
+	git checkout first &&
+	git am patch1-crlf.eml &&
+	! test -d .git/rebase-apply &&
+	test -z "$(git diff second)" &&
+	test "$(git rev-parse second)" = "$(git rev-parse HEAD)" &&
+	test "$(git rev-parse second^)" = "$(git rev-parse HEAD^)"
+'
+
 GIT_AUTHOR_NAME="Another Thor"
 GIT_AUTHOR_EMAIL="a.thor@example.com"
 GIT_COMMITTER_NAME="Co M Miter"
-- 
1.6.6.rc3.1.g8df51

Re: [PATCH] am: fix patch format detection for Thunderbird "Save As" emails

[Junio C Hamano]

Stephen Boyd <bebarino@gmail.com> writes:

> I'm not sure how portable \r in a sed invocation is.

Not very portable.

Adding

	tr -d '\015' <"$1" |

in front of the original "sed" invocation might be a better choice.

> but seems necessary because check_patch_format() is called before any
> splitting with mailsplit is done (where I assume the fix for the issue
> will be done).

I agree that the way non-native mbox format was bolted onto "am" is
somewhat unfortunate.

[PATCHv2] am: fix patch format detection for Thunderbird "Save As" emails

[Stephen Boyd]

The patch detection wants to inspect all the headers of a rfc2822 message
and ensure that they look like header field names. The headers are always
separated from the message body with a blank line. When Thunderbird saves
the message the blank line separating the headers from the body includes a
CR. The patch detection is failing because a CRLF doesn't match /^$/. Fix
this by allowing a CR to exist on the separating line.

Signed-off-by: Stephen Boyd <bebarino@gmail.com>
---

Changes since v1:
 - More portable code using tr (thanks Junio)
 - More portable testing by manually adding CRLFs

 git-am.sh     |    3 ++-
 t/t4150-am.sh |   24 ++++++++++++++++++++++++
 2 files changed, 26 insertions(+), 1 deletions(-)

diff --git a/git-am.sh b/git-am.sh
index 4838cdb..9e64deb 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -204,7 +204,8 @@ check_patch_format () {
 			# 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" |
+			tr -d '\015' <"$1" |
+			sed -n -e '/^$/q' -e '/^[ 	]/d' -e p |
 			sane_egrep -v '^[!-9;-~]+:' >/dev/null ||
 			patch_format=mbox
 		fi
diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index 8296605..7b6269d 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -83,6 +83,21 @@ test_expect_success setup '
 		echo "X-Fake-Field: Line Three" &&
 		git format-patch --stdout first | sed -e "1d"
 	} > patch1.eml &&
+	{
+		echo "X-Fake-Field: Line One\015" &&
+		echo "X-Fake-Field: Line Two\015" &&
+		echo "X-Fake-Field: Line Three\015" &&
+		git format-patch --stdout first |
+		sed -e "1d" -e "3,\$d" | tr -d "\n" &&
+		echo "\015" &&
+		git format-patch --stdout first |
+		sed -e "1,2d" -e "4,\$d" | tr -d "\n" &&
+		echo "\015" &&
+		git format-patch --stdout first |
+		sed -e "1,3d" -e "5,\$d" | tr -d "\n" &&
+		echo "\015\n\015" &&
+		git format-patch --stdout first | sed -e "1,5d"
+	} > patch1-crlf.eml &&
 	sed -n -e "3,\$p" msg >file &&
 	git add file &&
 	test_tick &&
@@ -123,6 +138,15 @@ test_expect_success 'am applies patch e-mail not in a mbox' '
 	test "$(git rev-parse second^)" = "$(git rev-parse HEAD^)"
 '
 
+test_expect_success 'am applies patch e-mail not in a mbox with CRLF' '
+	git checkout first &&
+	git am patch1-crlf.eml &&
+	! test -d .git/rebase-apply &&
+	test -z "$(git diff second)" &&
+	test "$(git rev-parse second)" = "$(git rev-parse HEAD)" &&
+	test "$(git rev-parse second^)" = "$(git rev-parse HEAD^)"
+'
+
 GIT_AUTHOR_NAME="Another Thor"
 GIT_AUTHOR_EMAIL="a.thor@example.com"
 GIT_COMMITTER_NAME="Co M Miter"
-- 
1.6.6.rc3.1.g8df51

Re: [PATCHv2] am: fix patch format detection for Thunderbird "Save As" emails

[Eric Blake]

Stephen Boyd <bebarino <at> gmail.com> writes:

> +	{
> +		echo "X-Fake-Field: Line One\015" &&

echo and \ do not portably mix.  For that matter, shell double quotes and 
backslash escapes that are not required by POSIX do not portably mix.  To 
reliably create carriage returns in shell, you need to use printf, or else 
something like:

echo "...@" | tr '@' '\015'

-- 
Eric Blake

Re: [PATCHv2] am: fix patch format detection for Thunderbird "Save As" emails

[Stephen Boyd]

On Fri, 2009-12-18 at 21:42 +0000, Eric Blake wrote:
> Stephen Boyd <bebarino <at> gmail.com> writes:
> 
> > +	{
> > +		echo "X-Fake-Field: Line One\015" &&
> 
> echo and \ do not portably mix.  For that matter, shell double quotes and 
> backslash escapes that are not required by POSIX do not portably mix.  To 
> reliably create carriage returns in shell, you need to use printf, or else 
> something like:
> 
> echo "...@" | tr '@' '\015'
> 

Thanks. Hopefully squashing this in will make it even more portable?

--->8---
diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index 7b6269d..19d5ca1 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -84,18 +84,18 @@ test_expect_success setup '
                git format-patch --stdout first | sed -e "1d"
        } > patch1.eml &&
        {
-               echo "X-Fake-Field: Line One\015" &&
-               echo "X-Fake-Field: Line Two\015" &&
-               echo "X-Fake-Field: Line Three\015" &&
+               printf "X-Fake-Field: Line One\015\n" &&
+               printf "X-Fake-Field: Line Two\015\n" &&
+               printf "X-Fake-Field: Line Three\015\n" &&
                git format-patch --stdout first |
                sed -e "1d" -e "3,\$d" | tr -d "\n" &&
-               echo "\015" &&
+               printf "\015\n" &&
                git format-patch --stdout first |
                sed -e "1,2d" -e "4,\$d" | tr -d "\n" &&
-               echo "\015" &&
+               printf "\015\n" &&
                git format-patch --stdout first |
                sed -e "1,3d" -e "5,\$d" | tr -d "\n" &&
-               echo "\015\n\015" &&
+               printf "\015\n\015\n" &&
                git format-patch --stdout first | sed -e "1,5d"
        } > patch1-crlf.eml &&
        sed -n -e "3,\$p" msg >file &&

Re: [PATCHv2] am: fix patch format detection for Thunderbird "Save As" emails

[Eric Blake]

Stephen Boyd <bebarino <at> gmail.com> writes:

> > echo and \ do not portably mix.  For that matter, shell double quotes and 
> > backslash escapes that are not required by POSIX do not portably mix.
> 
> Thanks. Hopefully squashing this in will make it even more portable?
> 
> +               printf "X-Fake-Field: Line One\015\n" &&

Nope.  You need either "\\015\\n" or '\015\n', since "\015" and "\n" are both 
undefined in portable shell.

-- 
Eric Blake

Re: [PATCHv2] am: fix patch format detection for Thunderbird "Save As" emails

[Stephen Boyd]

On 12/18/2009 02:42 PM, Eric Blake wrote:
> Stephen Boyd<bebarino<at>  gmail.com>  writes:
>>> echo and \ do not portably mix.  For that matter, shell double quotes and
>>> backslash escapes that are not required by POSIX do not portably mix.
>>
>> Thanks. Hopefully squashing this in will make it even more portable?
>>
>> +               printf "X-Fake-Field: Line One\015\n"&&
>
> Nope.  You need either "\\015\\n" or '\015\n', since "\015" and "\n" are both
> undefined in portable shell.

So, how about this?

         {
                 echo "X-Fake-Field: Line One"&&
                 echo "X-Fake-Field: Line Two"&&
                 echo "X-Fake-Field: Line Three"&&
                 git format-patch --stdout first | sed -e "1d"
         } | sed -e "s/$/;/" | tr "'";"'" "'"\015"'">  patch1-crlf.eml

Or maybe this?

         {
                 echo "X-Fake-Field: Line One"&&
                 echo "X-Fake-Field: Line Two"&&
                 echo "X-Fake-Field: Line Three"&&
                 git format-patch --stdout first | sed -e "1d"
         } | sed -e "s/$/;/" | tr ";" "\\015">  patch1-crlf.eml

Re: [PATCHv2] am: fix patch format detection for Thunderbird "Save As" emails

[Eric Blake]

According to Stephen Boyd on 12/18/2009 7:24 PM:
>> Nope.  You need either "\\015\\n" or '\015\n', since "\015" and "\n"
>> are both
>> undefined in portable shell.
> 
> So, how about this?
> 
>         {
>                 echo "X-Fake-Field: Line One"&&
>                 echo "X-Fake-Field: Line Two"&&
>                 echo "X-Fake-Field: Line Three"&&
>                 git format-patch --stdout first | sed -e "1d"
>         } | sed -e "s/$/;/" | tr "'";"'" "'"\015"'">  patch1-crlf.eml

Syntax error.  "$/" is not defined, so the argument to sed is not
portable.  Then, following the tr, you have an unquoted ;, meaning you
invoked 'tr "'"', followed by invoking the (non-existent) command '.

> 
> Or maybe this?
> 
>         {
>                 echo "X-Fake-Field: Line One"&&
>                 echo "X-Fake-Field: Line Two"&&
>                 echo "X-Fake-Field: Line Three"&&
>                 git format-patch --stdout first | sed -e "1d"
>         } | sed -e "s/$/;/" | tr ";" "\\015">  patch1-crlf.eml

Closer, but not there yet.  "$/" is still not defined.  Then, as a matter
of style, '\' is more readable than "\\" for representing a backslash.  So
as long as we are shifting to '', we might as well do it everywhere in
that line - write it like this:

} | sed -e 's/$/;/' | tr ';' '\015' > patch1-crlf.eml

and you should be set.

-- 
Don't work too hard, make some time for fun as well!

Eric Blake             ebb9@byu.net

Re: [PATCHv2] am: fix patch format detection for Thunderbird "Save As" emails

[Stephen Boyd]

On 12/18/2009 09:38 PM, Eric Blake wrote:
> Closer, but not there yet.  "$/" is still not defined.  Then, as a matter
> of style, '\' is more readable than "\\" for representing a backslash.  So
> as long as we are shifting to '', we might as well do it everywhere in
> that line - write it like this:
>
> } | sed -e 's/$/;/' | tr ';' '\015'>  patch1-crlf.eml
>
> and you should be set.

Ah, I think you missed that this stuff is inside single quotes already. 
I would love to just do what you suggest here.

I'm a little confused because I see this in a test (am 
--committer-date-is-author-date) a ways down in the same file

     git cat-file commit HEAD | sed -e "/^$/q">head1&&


and following your reasoning that wouldn't be portable?

Either way, I'll look for a better replacement character instead of 
semi-colon.

Re: [PATCHv2] am: fix patch format detection for Thunderbird "Save As" emails

[Stephen Boyd]

I found this in t0022

sed -e "s/\$/
^M/" "$TEST_DIRECTORY"/t0022-crlf-rename.sh>elpmas&&

so I'd like to use that if possible.

Re: [PATCHv2] am: fix patch format detection for Thunderbird "Save As" emails

[Junio C Hamano]

Stephen Boyd <bebarino@gmail.com> writes:

> I found this in t0022
>
> sed -e "s/\$/
> ^M/" "$TEST_DIRECTORY"/t0022-crlf-rename.sh>elpmas&&
>
> so I'd like to use that if possible.

That needs fixing; I think we caught something similar from Shawn before
it got in, primarily because the mail path corrupted the message and
turned the literal CR into LF.

Re: [PATCHv2] am: fix patch format detection for Thunderbird "Save As" emails

[Stephen Boyd]

On 12/18/2009 11:39 PM, Junio C Hamano wrote:
> Stephen Boyd<bebarino@gmail.com>  writes:
>    
>> I found this in t0022
>>
>> sed -e "s/\$/
>> ^M/" "$TEST_DIRECTORY"/t0022-crlf-rename.sh>elpmas&&
>>
>> so I'd like to use that if possible.
>>      
> That needs fixing; I think we caught something similar from Shawn before
> it got in, primarily because the mail path corrupted the message and
> turned the literal CR into LF

Sorry it looks like my mailer turned the CR into a LF. That should all 
be one line.

Are you saying that t0022 needs fixing?

Re: [PATCHv2] am: fix patch format detection for Thunderbird "Save As" emails

[Andreas Schwab]

Stephen Boyd <bebarino@gmail.com> writes:

> On 12/18/2009 09:38 PM, Eric Blake wrote:
>> Closer, but not there yet.  "$/" is still not defined.  Then, as a matter
>> of style, '\' is more readable than "\\" for representing a backslash.  So
>> as long as we are shifting to '', we might as well do it everywhere in
>> that line - write it like this:
>>
>> } | sed -e 's/$/;/' | tr ';' '\015'>  patch1-crlf.eml
>>
>> and you should be set.
>
> Ah, I think you missed that this stuff is inside single quotes already. I
> would love to just do what you suggest here.

You can replace every use of ' by '\''.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCHv2] am: fix patch format detection for Thunderbird "Save As" emails

[Junio C Hamano]

Eric Blake <ebb9@byu.net> writes:

> Stephen Boyd <bebarino <at> gmail.com> writes:
>
>> +	{
>> +		echo "X-Fake-Field: Line One\015" &&
>
> echo and \ do not portably mix.  For that matter, shell double quotes and 
> backslash escapes that are not required by POSIX do not portably mix.  To 
> reliably create carriage returns in shell, you need to use printf, or else 
> something like:
>
> echo "...@" | tr '@' '\015'

Thanks.

Also we probably want to change the "only munge the first three lines" to
something like:

	format-patch --stdout |
        sed -e 's/$/Q/' |
        tr 'Q' '\015'

picking some 'Q' that we know does not appear in the text.

Re: [PATCHv2] am: fix patch format detection for Thunderbird "Save As" emails

[Nanako Shiraishi]

Junio, could you tell us what happened to this thread?

After a lengthy discussion, nothing happened.

Re: [PATCHv2] am: fix patch format detection for Thunderbird "Save As" emails

[Stephen Boyd]

On Tue, Jan 5, 2010 at 2:38 PM, Nanako Shiraishi <nanako3@lavabit.com> wrote:
> Junio, could you tell us what happened to this thread?
>
> After a lengthy discussion, nothing happened.
>

Sorry. I lost interest during the holidays and then went on a vacation
after. I've returned and will try and get back to it this weekend.