patcham: fix patch format detection for Thunderbird "Save As" emails

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

Stephen Boyd <bebarino@gmail.com> writes:

Not very portable.

Adding

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

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

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

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

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. To reliably create carriage returns in shell, you need to use printf, or else something like:

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

On Fri, 2009-12-18 at 21:42 +0000, Eric Blake wrote:

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

--->8---

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

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

On 12/18/2009 02:42 PM, Eric Blake wrote:

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

According to Stephen Boyd on 12/18/2009 7:24 PM:

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

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.

On 12/18/2009 09:38 PM, Eric Blake wrote:

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.

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.

Stephen Boyd <bebarino@gmail.com> writes:

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.

On 12/18/2009 11:39 PM, Junio C Hamano wrote:

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?

Stephen Boyd <bebarino@gmail.com> writes:

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

Andreas.

Eric Blake <ebb9@byu.net> writes:

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.

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

After a lengthy discussion, nothing happened.

On Tue, Jan 5, 2010 at 2:38 PM, Nanako Shiraishi <nanako3@lavabit.com> wrote:

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.