git-apply can't apply patches to CRLF-files

git-apply can't apply patches to CRLF-files

[Salikh Zakirov]

Hello, 

git-apply can't apply the patch to file with windows-style CRLF line endings,
even if the patch was generated by git-format-patch.

Is this a bug or known deficiency?

The following script reproduces the problem
---------
#!/bin/sh
set -e
mkdir trash
cd trash
git init-db
echo "abc" > a
unix2dos a
git add a
git commit -m "a added" a
echo "cde" >> a
unix2dos a
git commit -m "a modified" a
git format-patch HEAD^
git reset --hard HEAD^
git am 0001*.txt
---------

The resulting output is
---------
$ ./test
defaulting to local storage area
a: done.
Committing initial tree 357c56061b96c1548b15168bc0d02e8d1a319e0b
a: done.
0001-a-modified.txt

Applying 'a modified'

error: patch failed: a:1
error: a: patch does not apply
Patch failed at 0001.
When you have resolved this problem run "git-am --resolved".
If you would prefer to skip this patch, instead run "git-am --skip".
---------

If I remove unix2dos calls and so the file has normal unix LF line endings,
then the result is correct as expected

---------
$ ./test
defaulting to local storage area
Committing initial tree 6afc8719a182fed19980da0e53d13fba1f94dd3f
0001-a-modified.txt

Applying 'a modified'

Wrote tree 49f5181a399bbcaac1da3bf693c466a281c4a255
Committed: 2b0a2936d0a65b3511882b8e88586ab054dd15b2
---------

Re: git-apply can't apply patches to CRLF-files

[Junio C Hamano]

Salikh Zakirov <Salikh.Zakirov@Intel.com> writes:

> git-apply can't apply the patch to file with windows-style CRLF line endings,
> even if the patch was generated by git-format-patch.

I do not think that is the case.

> Is this a bug or known deficiency?

This particular reproduction recipe looks like a PEBCAK; it does
not reproduce for me, but I do not have/use unix2dos so I did
DOSsy line endings a bit differently.

	git init-db
	echo 'abc@' | tr '[@]' '[\015]' >a
        git add a
        git commit -m initial
	echo 'def@' | tr '[@]' '[\015]' >>a
        git commit -a -m second
        git format-patch HEAD^
	git reset --hard HEAD^
        git am 0*.txt

> The following script reproduces the problem
> ---------
> #!/bin/sh
> set -e
> mkdir trash
> cd trash
> git init-db
> echo "abc" > a
> unix2dos a
> git add a
> git commit -m "a added" a
> echo "cde" >> a
> unix2dos a

Here the first line of a ends with \r\n already end the second
line ends with a \n.  Does running unix2dos on that do a
sensible thing on the first line?  Compare it with my above
recipe which appends DOSsy line at the end of the file.

Having said that, CRLF is unsafe for E-mail transfers anyway, so
I think we would need a special option to tell git-apply that it
should match '\n' that appears in the patch with '\r\n' in the
file being patched.  But I do not think that has anything to do
with the breakage you saw in your reproduction recipe.

[PATCH] Fixed Cygwin CR-munging problem in mailsplit

[Salikh Zakirov]

Do not open mailbox file as fopen(..., "rt")
as this strips CR characters from the diff,
thus breaking the patch context for changes 
in CRLF files.

Signed-off-by: Salikh Zakirov <Salikh.Zakirov@Intel.com>

---

Junio C Hamano wrote:
> 
> 	git init-db
> 	echo 'abc@' | tr '[@]' '[\015]' >a
>         git add a
>         git commit -m initial
> 	echo 'def@' | tr '[@]' '[\015]' >>a
>         git commit -a -m second
>         git format-patch HEAD^
> 	git reset --hard HEAD^
>         git am 0*.txt
> 

This reproduction scenario results in exactly the same problem.
The problem is observed on Cygwin.
My initial evaluation of the problem turned out to be completely bogus.

I've tracked the problem down to the fopen(file, "rt") in mailsplit.c,
which then truncates the CR character from the patch file.
This changes the patch context lines and it no longer applies.
Changing it to fopen(file, "r") fixes the problem.

> Having said that, CRLF is unsafe for E-mail transfers anyway, so
> I think we would need a special option to tell git-apply that it
> should match '\n' that appears in the patch with '\r\n' in the
> file being patched.  But I do not think that has anything to do
> with the breakage you saw in your reproduction recipe.

My use case does not involve e-mail transfers at all.
I'm using git-format-patch and git-am to rewrite the
patch sequence with different commit messages. 

Unfortunately, some of my fellow developers are not quite
careful, and occasionally some of the source files acquire
CR characters, sometimes in several lines only.

fd405a0843f3efd474bc7897b06d813d6498fbf4
 mailsplit.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

fd405a0843f3efd474bc7897b06d813d6498fbf4
diff --git mailsplit.c mailsplit.c
index c529e2d..70a569c 100644
--- mailsplit.c
+++ mailsplit.c
@@ -162,7 +162,7 @@ int main(int argc, const char **argv)
 
 	while (*argp) {
 		const char *file = *argp++;
-		FILE *f = !strcmp(file, "-") ? stdin : fopen(file, "rt");
+		FILE *f = !strcmp(file, "-") ? stdin : fopen(file, "r");
 		int file_done = 0;
 
 		if ( !f )
-- 
1.3.3.gfd40

Re: [PATCH] Fixed Cygwin CR-munging problem in mailsplit

[Junio C Hamano]

Salikh Zakirov <Salikh.Zakirov@Intel.com> writes:

> Do not open mailbox file as fopen(..., "rt")
> as this strips CR characters from the diff,
> thus breaking the patch context for changes 
> in CRLF files.
>
> Signed-off-by: Salikh Zakirov <Salikh.Zakirov@Intel.com>
>
> ---
> fd405a0843f3efd474bc7897b06d813d6498fbf4
> diff --git mailsplit.c mailsplit.c
> index c529e2d..70a569c 100644
> --- mailsplit.c
> +++ mailsplit.c
> @@ -162,7 +162,7 @@ int main(int argc, const char **argv)
>  
>  	while (*argp) {
>  		const char *file = *argp++;
> -		FILE *f = !strcmp(file, "-") ? stdin : fopen(file, "rt");
> +		FILE *f = !strcmp(file, "-") ? stdin : fopen(file, "r");
>  		int file_done = 0;
>  
>  		if ( !f )

I personally think this is a right change.  Provided if MTAs on
the path between patch originator and you are not broken and
your MUA saved the message with CR/LF distinction in the
contents intact, this should do more right thing.

I see broken patches every once in a while, but when they are
mangled by the mailpath, CRLF is the least of the problem; they
have other whitespace breakage that makes them unapplicable
anyway.

Having said that, however, that historically used to be a big IF
with capital letters.


I have a feeling that Linus did this on purpose.  For the
projects we originally cared about, a patch to introduce CRLF
in the tracked content was a broken patch 100% of the time (not
99%), and most likely caused by a breakage somewhere on the
mailpath.  At least in the original git context, protecting
UNIX/POSIX people from broken MTA/MUA counted far more than
catering to people who deals with DOSsy contents.

So I am slightly in favor of the change, but just barely.

Re: [PATCH] Fixed Cygwin CR-munging problem in mailsplit

[Junio C Hamano]

Junio C Hamano <junkio@cox.net> writes:

> I personally think this is a right change.  Provided if MTAs on
> the path between patch originator and you are not broken and
> your MUA saved the message with CR/LF distinction in the
> contents intact, this should do more right thing.
>
> I see broken patches every once in a while, but when they are
> mangled by the mailpath, CRLF is the least of the problem; they
> have other whitespace breakage that makes them unapplicable
> anyway.
>
> Having said that, however, that historically used to be a big IF
> with capital letters.
>
>
> I have a feeling that Linus did this on purpose.  For the

Heh, I had a trailing CR after the "with capital letters." and
one blank line between paragraphs, but I now see two blank lines
there.  So even in this modern day, preserving CRLF is not
something that happens by default -- you would need to make sure
that everybody on your mailpath to the recipient is set up the
right way.

> So I am slightly in favor of the change, but just barely.

So now I am less in favor of the change than when I wrote that
response.

RE: [PATCH] Fixed Cygwin CR-munging problem in mailsplit

[Zakirov, Salikh]

Junio C Hamano <junkio@cox.net> writes:
> So even in this modern day, preserving CRLF is not
> something that happens by default -- you would need to make sure
> that everybody on your mailpath to the recipient is set up the
> right way.

> So now I am less in favor of the change than when I wrote that
> response.

I understand this reasoning, and I am not sure if the fix is correct
from the "GIT world" point of view.

However, I believe that the command sequence git-format-patch, git-am
without any e-mail transfer in between and in the same repository
should work perfectly regardless of the contents of the files, 
no matter if they are binary, text, or "CRLF text" or even 
"broken LF and CRLF text". This is a requirement from a nasty "real
world".

Junio, could you point at a right place to fix to get git-format-patch, 
git-am sequence work flawlessly on Cygwin?

By the way, the change affects only non-Unix users, as fopen(..., "rt")
is equivalent to fopen(..., "rb") on all Unixes anyway.

Re: [PATCH] Fixed Cygwin CR-munging problem in mailsplit

[Christopher Faylor]

On Sun, May 28, 2006 at 12:57:35AM +0400, Zakirov, Salikh wrote:
>Junio C Hamano <junkio@cox.net> writes:
>> So even in this modern day, preserving CRLF is not
>> something that happens by default -- you would need to make sure
>> that everybody on your mailpath to the recipient is set up the
>> right way.
>
>> So now I am less in favor of the change than when I wrote that
>> response.
>
>I understand this reasoning, and I am not sure if the fix is correct
>from the "GIT world" point of view.
>
>However, I believe that the command sequence git-format-patch, git-am
>without any e-mail transfer in between and in the same repository
>should work perfectly regardless of the contents of the files, 
>no matter if they are binary, text, or "CRLF text" or even 
>"broken LF and CRLF text". This is a requirement from a nasty "real
>world".
>
>Junio, could you point at a right place to fix to get git-format-patch, 
>git-am sequence work flawlessly on Cygwin?
>
>By the way, the change affects only non-Unix users, as fopen(..., "rt")
>is equivalent to fopen(..., "rb") on all Unixes anyway.

But fopen(..., "r") is not equivalent to fopen(..., "rb") on Cygwin.

Wouldn't you want to add the "b" there to be assured of a binary open?

cgf

Re: [PATCH] Fixed Cygwin CR-munging problem in mailsplit

[Junio C Hamano]

"Zakirov, Salikh" <salikh.zakirov@intel.com> writes:

> However, I believe that the command sequence git-format-patch, git-am
> without any e-mail transfer in between and in the same repository
> should work perfectly regardless of the contents of the files, 
> no matter if they are binary, text, or "CRLF text" or even 
> "broken LF and CRLF text". This is a requirement from a nasty "real
> world".

OK, that's a very valid argument.  Thanks for straighten me out.