am fails to apply patches for files with CRLF lineendings

Hi,

Jason King (cc'd) reported that a patch for a file with CRLF lineendings fails to apply, even if generated and applied in the same repo.

doener@atjola:x $ git init

doener@atjola:x (master) $ for x in $(seq 10); do echo -e "$x\r" >> foo; done
doener@atjola:x (master) $ vim foo
doener@atjola:x (master) $ git add foo; git commit -m init
[master (root-commit) b59b963] init
 1 files changed, 10 insertions(+), 0 deletions(-)
 create mode 100644 foo

doener@atjola:x (master) $ sed -ie s/5/changed/ foo
doener@atjola:x (master) $ git commit -am changed
[master fe4ee44] changed
 1 files changed, 1 insertions(+), 1 deletions(-)

doener@atjola:x (master) $ git format-patch HEAD^ 0001-changed.patch

doener@atjola:x (master) $ git checkout HEAD^
Note: moving to 'HEAD^' which isn't a local branch

doener@atjola:x ((b59b963...)) $ git am 0001-changed.patch
Applying: changed
error: patch failed: foo:2
error: foo: patch does not apply
Patch failed at 0001 changed
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".

Using "--whitespace=fix" makes the patch apply, but converts the changed line and the context area from CRLF to LF.

Commit c2ca1d7 "Allow mailsplit ... to handle mails with CRLF line-endings" seems to be responsible. Using "git am --rebasing" to trigger the --keep-cr flag to mailsplit makes things work:

doener@atjola:x ((b59b963...)) $ git am --rebasing 0001-changed.patch
Applying: changed

And reverting that commit also gives the expected whitespace warning (which is somehow squelched by the --rebasing flag it seems).

doener@atjola:x ((b59b963...)) $ git am 0001-changed.patch
Applying: changed
/home/doener/x/.git/rebase-apply/patch:14: trailing whitespace.
changed
warning: 1 line adds whitespace errors.

Björn

Björn Steinbrink <B.Steinbrink@gmx.de> writes:

Yes, that commit is not only responsible but was deliberate. For a better backstory, see:

  http://thread.gmane.org/gmane.comp.version-control.git/124718/focus=124721

You'd notice that I was one of the people who didn't want to have this change, so you don't need to convince _me_ that this was not a change to keep everybody happy, but you'd need to try a better job than I did back then to convince people who thought that "am" should directly work on "Thunderbird saved mails" that what they want was a bad idea X-<.

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

Having said that, I think you can tell "format-patch" to emit it as mime attachment to work this around. It _might_ even make sense to do so automatically when the payload contains CRLF but that is a separate issue.

Junio C Hamano wrote:

My understanding of the problem is that rfc2822 dictates that CRLF is the line ending in an email message for _every_ line, and that CR cannot occur without LF and vice versa. So there is no reliable way to extract patches from the body of an email and expect line endings to be conveyed accurately. Some email clients save emails with the line-endings of the platform, some save in what they call "raw" format with rfc2822's CRLF line endings. So we have to _assume_ that the patch extracted from the email has a particular line ending and make-it-so. For better or worse (better for me), commit c2ca1d7 chose LF line-endings as the line-ending of choice.

I agree that git-am should be able to apply everything that git-format-patch produces. Perhaps the diff machinery should be modified to treat files containing \r as binary when generating the output for format-patch. Then we'd get a binary diff in the email.

-brandon

Brandon Casey <brandon.casey.ctr@nrlssc.navy.mil> writes:

I think the fundamental problem is that what MUA uses as the internal storage format doesn't necessarily have to even be RFC-2822, which only specifies what should be on-the-wire. The blamed commit took things too far.

It actually is the norm to use LF as the line terminator in the body text in saved messages (and trailing CR as a true part of the payload), and "am" traditionally used that definition. It is meant to read from "mbox" format to begin with.

Before the blamed commit, "am" took what was given literally, and it treated the trailing CR as part of the payload in a text file, each of whose line is LF terminated. This meant that if you sent and your MUA didn't corrupt, or more importantly if you ran format-patch yourself to produce a patch on content with CRLF line endings and fed it to am without any e-mail involved, your CRLF would have been preserved. So in that sense, unlike what you said in your message, the blamed commit didn't decide that the line termination must be LF. It decided that the line termination does not matter, which is a lot worse.

As long as the use of CR is an internal storage matter and "Save As..." doesn't add extra CR that wasn't in the original contents, I wouldn't say that such a MUA is broken. In the use case that led to the blamed commit, the user is choosing to read directly from the internal storage of MUA, bypassing its "Save As..." interface meant to be used to externalize the messages, and the user is responsible for dealing with the fallout, hence my "dos2unix" suggestion in the original thread.

Probably we should revert that commit, unless somebody comes up with a better solution _or_ somebody convincingly argues that there shouldn't be CRLF in your committed history.

On 2009.12.14 12:38:30 -0800, Jason King wrote:

Hm, currently it checks everyline. I didn't really think this through, but wouldn't it makes sense to have a per-mail flag, that checks just the first line, and if that has CRLF, then enable the dropping (unless --keep-cr is given), otherwise, keep things verbatim. That should (I think) make things work with non-messed up patches regardless of the files being patch, as well as with messed up patches as long as they don't try to patch files with CRLF line-endings.

I can't seem to come up with a clean patch though (btw, is the comment for split_one that says that buf may contain a partial line still true?), but maybe I'll find some time to try again later this week (don't count on it though, I'm pretty stressed...)

Björn

On Dec 14, 2009, at 12:55 PM, Björn Steinbrink wrote:

Sure, it won't affect LFers at all if you test the header lines of the patch file - we'll be immune from that conversion then. Works for me :)

Thanks again, Jason

On 2009.12.14 18:50:44 -0600, Brandon Casey wrote:

Right. And checking, after sending a patch containing CRs with mutt, it lost those CRs. Even the local copy saved directly by mutt, which didn't leave my box, lacks the CRs. So it seems basically impossible to send patches to CRLF files inline.

RFC-822 still allowed bare CRs/LFs :-/

So the commit didn't break with anything mails conforming to RFC-2822, those won't work for files with CR being patch. But it still breaks the the raw format-patch generated patches, so even attaching them to the actual email as a workaround won't do.

That makes a "use the first line to decide whether or not to strip CRs" approach look like a good idea. Real mails are broken anyway, and the format-patch output has LF on the first line, so mailsplit wouldn't mess it up... Unless git on windows produces CRLF format-patch output...

Björn

Björn Steinbrink <B.Steinbrink@gmx.de> writes:

If you want to send mail containing a bare CR you need to encode it.

Andreas.

Andreas Schwab <schwab@linux-m68k.org> writes:

Btw, this mail had a stray CR added to the last line when I sent it out (properly encoded as quoted-printable), but that didn't make it through gmane. :-)

Andreas.

Forwarding to the list. The original was bounced since gmail sent a multipart mime version with html. Seems we can't disable html composing in the gmail settings anymore (I thought we used to be able to).

---------- Forwarded message ----------
From: Brandon Casey <drafnel@gmail.com>
Date: Mon, Dec 14, 2009 at 6:50 PM
Subject: Re: am fails to apply patches for files with CRLF lineendings
To: Junio C Hamano <gitster@pobox.com>
Cc: Brandon Casey <brandon.casey.ctr@nrlssc.navy.mil>, Björn
Steinbrink <B.Steinbrink@gmx.de>, jk@silentcow.com,
git@vger.kernel.org

On Mon, Dec 14, 2009 at 5:22 PM, Junio C Hamano <gitster@pobox.com> wrote:

If CRLF is what is on-the-wire, how can the MUA tell whether the original was also CRLF or whether it was only LF?  My assumption was that the MUA cannot tell, and that things worked for most people because those people who wanted LF terminated output were on platforms that used LF termination and their MUA produced output using the native line termination.  Things broke recently for some people since thunderbird devels decided to start saving emails with CRLF termination on linux.

But isn't each email in the mbox file supposed to be RFC-2822 formatted anyway?  If so, then my reading of RFC-2822 says that there should only be CRLF everywhere and no bare CR or bare LF.  But maybe everyone has just been ignoring that part of RFC-2822?  I'm not an email expert, so I really don't know.

I think it is more correct to say that the line termination in an email is ambiguous.  CRLF does not necessarily mean that the original had CRLF line termination if RFC-2822 is followed explicitly.

No, we're using "Save As..." in thunderbird, and it saves with CRLF line endings.  I don't really care for thunderbird and its proclivity for munging my emails and _not_ doing-the-right-thing in my opinion.

Maybe someone can suggest a mail client that can use imap and provide the nice sorting of emails into folders like thunderbird does.

-brandon

Heya,

On Tue, Dec 15, 2009 at 03:09, Brandon Casey <drafnel@gmail.com> wrote:

You can, it remembers when you click "« Plain Text", at least, it does for me :P.

-- Cheers,

Sverre Rabbelier

Sverre Rabbelier wrote:

Thanks, that seemed to be the case for the last few emails I wrote. They defaulted to plain text.

-brandon

Brandon Casey <drafnel@gmail.com> writes:

RFC 2822 specifies a *transport* format, where each text lines is represented by terminating it with CRLF. Any other use is outside the scope of that RFC. The mbox format normally uses the native format for text files, which means LF terminated lines when used under Unix.

Andreas.

Brandon Casey <drafnel@gmail.com> writes:

If you are talking about the same "mbox" I was talking about, which is what I see when I peek "/var/mail/junio", then the answer is no. Their lines are terminated with a LF, and if you insert CR at the end of the line it would appear as true payload. DOSsy boxes can have C:\mail\user or whatever that has DOS text, of course, so there is no "supposed to be".

Having said that, it does not matter an iota in the real world if somebody declares on _this list_ that it a bug that Thunderbird spits out CRLF text in response to "Save As..." on platforms where LF is the natural line terminator [*1*]. Whether it is a bug or not, we still need to help people with such a program without breaking others.

I saw "peeking the line ending of the first line" as suggested as a solution, and my gut feeling, without thinking too much about it, is that it is likely to be the right thing to do, especially if we do both the check and the necessary conversion in either mailinfo or even in mailsplit.

[Footnote]

*1* It is a different matter if it was done on _their_ mailing list, and it would even be better if such a discussion on _their_ mailing list resulted in a fix over there.

On Mon, Dec 14, 2009 at 8:12 PM, Junio C Hamano <gitster@pobox.com> wrote:

Yes, that is what I was talking about, but I did not know whether the individual mails which are separated by "From user@host ..." were supposed to be in RFC-2822 format or not.

How do you insert CR at the end of the line? Can you use mutt or something like it to send a mail which contains a CR? I have tried, and I have not been able to do so. I have tried mutt, mailx, and sendmail. For sendmail, I of course constructed the email headers by hand and piped it through sendmail. The CRLF in my tests were converted to LF by the time they reached /var/mail/casey.

I'm not sure it is a bug, just a change in behavior.

I agree.

Yes, I think it will work as a work-around, unfortunately I cannot work on implementing this at the moment. I think the better solution, if it is not too costly, is to detect the presence of CR and produce a binary patch that can be sent through email and applied by git-am.

-brandon

Jason King wrote:

I am interested in fixing this but have not been able to get to it.

-brandon

Just noticed 1.7 was just tagged, so I thought I'd ping you guys about this CRLF issue in `am` again - squeaky wheel sort of thing. Thanks :)

On Jan 5, 2010, at 8:41 AM, Brandon Casey wrote: