am fails to apply patches for files with CRLF lineendings

am fails to apply patches for files with CRLF lineendings

[Björn Steinbrink]

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

Re: am fails to apply patches for files with CRLF lineendings

[Junio C Hamano]

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

> Commit c2ca1d7 "Allow mailsplit ... to handle mails with CRLF line-endings"
> seems to be responsible.

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

Re: am fails to apply patches for files with CRLF lineendings

[Junio C Hamano]

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

> Björn Steinbrink <B.Steinbrink@gmx.de> writes:
>
>> Commit c2ca1d7 "Allow mailsplit ... to handle mails with CRLF line-endings"
>> seems to be responsible.
>
> 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-<.

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.

Re: am fails to apply patches for files with CRLF lineendings

[Björn Steinbrink]

On 2009.12.14 12:38:30 -0800, Jason King wrote:
> On Dec 14, 2009, at 12:27 PM, Junio C Hamano wrote:
> 
> >Björn Steinbrink <B.Steinbrink@gmx.de> writes:
> >
> >>Commit c2ca1d7 "Allow mailsplit ... to handle mails with CRLF
> >>line-endings"
> >>seems to be responsible.
> >
> >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-<.
> 
> I dunno Junio, the back story doesn't really seem like a convincing
> argument for totally breaking am's handling of real CRLFs.  Right
> now, it seems to be a very bad thing that git can create a patch
> that it can't apply.  The default am should always be able to apply
> whatever format-patch has generated.
> 
> If it's desirable to have am translate CRLFs to LFs, then why not
> provide this as an option to am so as not to break merging of real
> CRLF patches?  Eg.:
> 
>   git am --convert-crlf

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

Re: am fails to apply patches for files with CRLF lineendings

[Jason King]

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

> On 2009.12.14 12:38:30 -0800, Jason King wrote:
>> On Dec 14, 2009, at 12:27 PM, Junio C Hamano wrote:
>>
>>> Björn Steinbrink <B.Steinbrink@gmx.de> writes:
>>>
>>>> Commit c2ca1d7 "Allow mailsplit ... to handle mails with CRLF
>>>> line-endings"
>>>> seems to be responsible.
>>>
>>> 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-<.
>>
>> I dunno Junio, the back story doesn't really seem like a convincing
>> argument for totally breaking am's handling of real CRLFs.  Right
>> now, it seems to be a very bad thing that git can create a patch
>> that it can't apply.  The default am should always be able to apply
>> whatever format-patch has generated.
>>
>> If it's desirable to have am translate CRLFs to LFs, then why not
>> provide this as an option to am so as not to break merging of real
>> CRLF patches?  Eg.:
>>
>>  git am --convert-crlf
>
> 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.

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

Re: am fails to apply patches for files with CRLF lineendings

[Brandon Casey]

Junio C Hamano wrote:
> Björn Steinbrink <B.Steinbrink@gmx.de> writes:
> 
>> Commit c2ca1d7 "Allow mailsplit ... to handle mails with CRLF line-endings"
>> seems to be responsible.
> 
> 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-<.

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

Re: am fails to apply patches for files with CRLF lineendings

[Junio C Hamano]

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

> My understanding of the problem is that rfc2822 dictates that...

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.

Re: am fails to apply patches for files with CRLF lineendings

[Björn Steinbrink]

On 2009.12.14 18:50:44 -0600, Brandon Casey wrote:
> 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.

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

Re: am fails to apply patches for files with CRLF lineendings

[Andreas Schwab]

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

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

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

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: am fails to apply patches for files with CRLF lineendings

[Andreas Schwab]

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

> Björn Steinbrink <B.Steinbrink@gmx.de> writes:
>
>> 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.
>
> If you want to send mail containing a bare CR you need to encode it.

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.

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

Fwd: am fails to apply patches for files with CRLF lineendings

[Brandon Casey]

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:
>
> Brandon Casey <brandon.casey.ctr@nrlssc.navy.mil> writes:
>
> > My understanding of the problem is that rfc2822 dictates that...
>
> 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.

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.

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

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.

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

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.

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

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

Re: am fails to apply patches for files with CRLF lineendings

[Sverre Rabbelier]

Heya,

On Tue, Dec 15, 2009 at 03:09, Brandon Casey <drafnel@gmail.com> wrote:
> 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).

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

--
Cheers,

Sverre Rabbelier

Re: am fails to apply patches for files with CRLF lineendings

[Brandon Casey]

Sverre Rabbelier wrote:
> Heya,
> 
> On Tue, Dec 15, 2009 at 03:09, Brandon Casey <drafnel@gmail.com> wrote:
>> 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).
> 
> You can, it remembers when you click "« Plain Text", at least, it does
> for me :P.

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

-brandon

Re: Fwd: am fails to apply patches for files with CRLF lineendings

[Andreas Schwab]

Brandon Casey <drafnel@gmail.com> writes:

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

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.

-- 
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: am fails to apply patches for files with CRLF lineendings

[Junio C Hamano]

Brandon Casey <drafnel@gmail.com> writes:

>> 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.
>
> But isn't each email in the mbox file supposed to be RFC-2822 formatted
> anyway?

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.

Re: am fails to apply patches for files with CRLF lineendings

[Brandon Casey]

On Mon, Dec 14, 2009 at 8:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Brandon Casey <drafnel@gmail.com> writes:
>
>>> 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.
>>
>> But isn't each email in the mbox file supposed to be RFC-2822 formatted
>> anyway?
>
> 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.

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.

> Their lines are terminated with a LF, and if you insert CR at the end of the
> line it would appear as true payload.

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.

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

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

> Whether it is a bug or not, we still need to help
> people with such a program without breaking others.

I agree.

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

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

Re: am fails to apply patches for files with CRLF lineendings

[Brandon Casey]

Jason King wrote:
> I just grabbed 1.6.6 and noticed that this problem still exists.  I just
> wanted to ping this thread to make sure this hadn't been forgotten.

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

-brandon

Re: am fails to apply patches for files with CRLF lineendings

[Jason King]

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:

> Jason King wrote:
>> I just grabbed 1.6.6 and noticed that this problem still exists.  I  
>> just
>> wanted to ping this thread to make sure this hadn't been forgotten.
>
> I am interested in fixing this but have not been able to get to it.
>
> -brandon
>