bug#4451: 23.1; EOL problems with vc-diff and cygwin

bug#4451: 23.1; EOL problems with vc-diff and cygwin

[Reiner Steib]

[-- Attachment #1: Type: text/plain, Size: 2517 bytes --]

Hi,

on Windows (with diff.exe from cygwin), I get annoying `^M' characters
in the diff output (see attached file vc-diff-2.output) when using
`C-x v =' (`vc-diff') on a file with Unix-style EOL.

[I now noticed that the attached file vc-diff-2.output has _two_ ^M at
each line ending.  I'm not sure if these were there already or 
if it happened during transport.  But it's unlikely because M$ Outlook
sent it base64 encoded.]

The `^M' charaters also break `C-c C-u' (`diff-context->unified').

When using `=' (`cvs-mode-diff') from PCL-CVS mode, there is no problem
(see attached file cvs-mode-diff.output).

Expected behavior: `vc-diff' should behave like `cvs-mode-diff' in this
case.

The file in question is K3.xml; "mode line" -> "describe coding
system" says:

| U -- utf-8-unix (alias: mule-utf-8-unix)
| 
| UTF-8 (no signature (BOM))
| Type: utf-8 (UTF-8: Emacs internal multibyte form)
| EOL type: LF
| This coding system encodes the following charsets:
|   unicode

More information about/from Emacs:

| ELISP> (executable-find "diff")
| "c:/P.../cygwin/bin/diff.exe"
| ELISP> (shell-command-to-string "diff --version")
| "diff (GNU diffutils) 2.8.7\nWritten [...]"
| ELISP>
| 
| In GNU Emacs 23.1.1 (i386-mingw-nt6.0.6001)
|  of 2009-07-30 on SOFT-MJASON
| Windowing system distributor `Microsoft Corp.', version 6.0.6001
| configured using `configure --with-gcc (4.4)'
| 
| Important settings:
|   value of $LC_ALL: nil
|   value of $LC_COLLATE: nil
|   value of $LC_CTYPE: nil
|   value of $LC_MESSAGES: nil
|   value of $LC_MONETARY: nil
|   value of $LC_NUMERIC: nil
|   value of $LC_TIME: nil
|   value of $LANG: DEU
|   value of $XMODIFIERS: nil
|   locale-coding-system: cp1252
|   default-enable-multibyte-characters: t
| 
| Major mode: Diff
| 
| Minor modes in effect:
|   cvs-minor-mode: t
|   diff-auto-refine-mode: t
|   tooltip-mode: t
|   tool-bar-mode: t
|   mouse-wheel-mode: t
|   menu-bar-mode: t
|   file-name-shadow-mode: t
|   global-font-lock-mode: t
|   font-lock-mode: t
|   blink-cursor-mode: t
|   global-auto-composition-mode: t
|   auto-composition-mode: t
|   auto-encryption-mode: t
|   auto-compression-mode: t
|   line-number-mode: t
|   transient-mark-mode: t

Information about/from Cygwin:

| GNU bash, version 3.2.39(20)-release (i686-pc-cygwin)
| 
| $ file K3.xml 
| K3.xml: XML 1.0 document text
| $ file --version
| file-4.21
| magic file from /usr/share/file/magic
| 
| $ diff --version
| diff (GNU diffutils) 2.8.7
| $ grep diff ~/.cvsrc
| diff -u

Bye, Reiner


[-- Attachment #2: vc-diff-2.output --]
[-- Type: application/octet-stream, Size: 1301 bytes --]

Index: K3.xml
===================================================================
RCS file: x:/path/to/K3.xml,v
retrieving revision 1.2
diff -u -c -r1.2 K3.xml
cvs.exe diff: conflicting specifications of output style
*** K3.xml	24 Jul 2009 16:09:12 -0000	1.2
--- K3.xml	16 Sep 2009 11:20:27 -0000
***************
*** 1,7 ****
  <?xml version='1.0'	
   encoding='UTF-8'	
  ?>	
! <aaaaaa aaaa='K3'	
   version='2.0.0'	
   schema-version='1.0.0'	
   valid='true'	
--- 1,7 ----
  <?xml version='1.0'	
   encoding='UTF-8'	
  ?>	
! <aaaaaa aaaa='K3_test'	
   version='2.0.0'	
   schema-version='1.0.0'	
   valid='true'	
***************
*** 4322,4327 ****
--- 4322,4341 ----
   checkDate='11/2005'	
   isBlockOverridden='No'	
   isSeverityOverridden='No'	
+  />	
+ <aaaaaaaaaaaaaaaaaa aaaaa='aaaa://aaa.aaa.aaa/aaa/aaaaaaaaa/aaa'	
+  aaaaaaa='1111111'	
+  aaaaaaa='aaaa'	
+  aaaaaaaa='aaaaaa'	
+  aaaaaaaaa='aaaa_aaaaaa_aaaaaaaaaa'	
+  aaaaaaaa='aaaaaa'	
+  aaaaaaaa='aaaa'	
+  aaaaa='aaaaa'	
+  aaaaaaaaaaaa=''	
+  aaa='aaa 11.111'	
+  aaaaaaaaa='1/1111'	
+  aaaaaaaaaaaaaaaaa='aa'	
+  aaaaaaaaaaaaaaaaaaaa='aaa'	
   /></aaaaaaaaaaaaaaaaaaaaa>
  <aaaaaaaaaaa aaaaa='aaaa://aaa.aaa.aaa/aaa/aaaaaaaaa/aaa'	
   >	

[-- Attachment #3: cvs-mode-diff.output --]
[-- Type: application/octet-stream, Size: 984 bytes --]

Index: K3.xml
===================================================================
RCS file: x:/path/to//K3.xml,v
retrieving revision 1.2
diff -u -r1.2 K3.xml
--- K3.xml	24 Jul 2009 16:09:12 -0000	1.2
+++ K3.xml	16 Sep 2009 11:20:27 -0000
@@ -1,7 +1,7 @@
 <?xml version='1.0'	
  encoding='UTF-8'	
 ?>	
-<aaaaaa aaaa='K3'	
+<aaaaaa aaaa='K3_test'	
  version='2.0.0'	
  schema-version='1.0.0'	
  valid='true'	
@@ -4322,6 +4322,20 @@
  aaaaaaaaa='11/1111'	
  aaaaaaaaaaaaaaaaa='aa'	
  aaaaaaaaaaaaaaaaaaaa='aa'	
+ />	
+<aaaaaaaaaaaaaaaaaa aaaaa='aaaa://aaa.aaa.aaa/aaa/aaaaaaaaa/aaa'	
+ aaaaaaa='1111111'	
+ aaaaaaa='aaaa'	
+ aaaaaaaa='aaaaaa'	
+ aaaaaaaaa='aaaa_aaaaaa_aaaaaaaaaa'	
+ aaaaaaaa='aaaaaa'	
+ aaaaaaaa='aaaa'	
+ aaaaa='aaaaa'	
+ aaaaaaaaaaaa=''	
+ aaa='aaa 11.111'	
+ aaaaaaaaa='1/1111'	
+ aaaaaaaaaaaaaaaaa='aa'	
+ aaaaaaaaaaaaaaaaaaaa='aaa'	
  /></aaaaaaaaaaaaaaaaaaaaa>
 <aaaaaaaaaaa aaaaa='aaaa://aaa.aaa.aaa/aaa/aaaaaaaaa/aaa'	
  >	

bug#4451: 23.1; EOL problems with vc-diff and cygwin

[Eli Zaretskii]

> From: Reiner Steib <reinersteib+gmane@imap.cc>
> Date: Wed, 16 Sep 2009 20:08:53 +0200
> Cc: 
> 
> on Windows (with diff.exe from cygwin), I get annoying `^M' characters
> in the diff output (see attached file vc-diff-2.output) when using
> `C-x v =' (`vc-diff') on a file with Unix-style EOL.

If you diff the same two files from the shell prompt using the same
diff.exe, what kind of EOLs do you get in the output?  Is the result
similar to what you see in vc-diff?

bug#4451: 23.1; EOL problems with vc-diff and cygwin

[Reiner Steib]

Eli Zaretskii <eliz@gnu.org> wrote:

>> From: Reiner Steib <reinersteib+gmane@imap.cc>
>>
>> on Windows (with diff.exe from cygwin), I get annoying `^M' characters
>> in the diff output (see attached file vc-diff-2.output) when using
>> `C-x v =' (`vc-diff') on a file with Unix-style EOL.
>
> If you diff the same two files from the shell prompt using the same
> diff.exe, what kind of EOLs do you get in the output?  Is the result
> similar to what you see in vc-diff?

I don't diff two files, but I use CVS on a single file.  So I guess  
you mean the "cvs diff" output.  The output of "cvs diff" in cygwin's  
bash has DOS line endings, no duplicate "^M", see below.

The saved Emacs buffer from vc-diff (vc-diff-2.output) has "^M^M$"  
whereas the buffer from cvs-mode-diff (cvs-mode-diff.output) has  
"^M$".  I checked the original files now, to avoid some mail transfer  
mangling.

Bye, Reiner


$ cvs diff K3.xml > /tmp/cygwin-cvs-diff.output

$ file /tmp/cygwin-cvs-diff.output
/tmp/cygwin-cvs-diff.output: RCS/CVS diff output text

$ tac /tmp/cygwin-cvs-diff.output | file -
/dev/stdin: ASCII text, with CRLF line terminators

$ cvs diff SK3.xml | cat --show-all
Aaaaa: AA1.aaa^M$
===================================================================^M$
AAA aaaa: A:\a\A\Aa\A/A/A/A/Aa/K3.xml,v^M$
aaaaaaaaaa aaaaaaaa 1.1^M$
aaaa -a -a1.1 AA1.aaa^M$
--- K3.xml^I11 Aaa 1111 11:11:11 -1111^I1.1^M$
+++ K3.xml^I11 Aaa 1111 11:11:11 -1111^M$
@@ -1,1 +1,1 @@^M$
  <?aaa aaaaaaa='1.1'^I^M$
   aaaaaaaa='AAA-1'^I^M$
  ?>^I^M$
-<aaaaaa aaaa='AA1'^I^M$
+<aaaaaa aaaa='AA1_aaaa_aaa11111'^I^M$
   aaaaaaa='1.1.1'^I^M$
   aaaaaa-aaaaaaa='1.1.1'^I^M$
   aaaaa='aaaa'^I^M$
@@ -1111,1 +1111,11 @@^M$
   aaaaaAaaa='11/1111'^I^M$
   aaAaaaaAaaaaaaaaa='Aa'^I^M$
   aaAaaaaaaaAaaaaaaaaa='Aa'^I^M$
+ />^I^M$
+<aaaaaaaaAaaaaaAaaa aaaaa='aaaa://aaa.aaa.aaa/aaa/AaaaAaaaa/AAA'^I^M$
+ aaaaaAA='1111111'^I^M$
+ Aaaaaaa='aaaa'^I^M$
+ aaAaaaaa='Aaaaaa'^I^M$
+ aaaaaAaaa='AAAA_Aaaaaa_AaaaAaaaaa'^I^M$
+ aaaaaaaa='aaaaaa'^I^M$
+ aaaaaaaa='aaaa'^I^M$
+ aaaaa='aaaaa'^I^M$
+ aaaaaaaaAaaa=''^I^M$
+ aaa='AAA 11.111'^I^M$
+ aaaaaAaaa='1/1111'^I^M$
+ aaAaaaaAaaaaaaaaa='Aa'^I^M$
+ aaAaaaaaaaAaaaaaaaaa='Aaa'^I^M$
   /></AaaaaaaaAaaaaaAaaaaaa>^M$
  <AaaaAaaaaaa aaaaa='aaaa://aaa.aaa.aaa/aaa/AaaaAaaaa/AAA'^I^M$
   >^I^M$

bug#4451: 23.1; EOL problems with vc-diff and cygwin

[Stefan Monnier]

I think the best solution is to change vc-coding-system-for-diff so that
when it takes the coding system from (with-current-buffer buf
buffer-file-coding-system), it ignores the EOL part of that
coding system.

Does the patch below help?


        Stefan


=== modified file 'lisp/vc.el'
--- lisp/vc.el	2009-09-14 15:39:41 +0000
+++ lisp/vc.el	2009-09-17 16:35:00 +0000
@@ -1470,7 +1470,11 @@
       ;; use the buffer's coding system
       (let ((buf (find-buffer-visiting file)))
         (when buf (with-current-buffer buf
-		    buffer-file-coding-system)))
+                    (if buffer-file-coding-system
+                        ;; Don't inherit the EOL part of the coding-system,
+                        ;; because some diff tools may choose to use
+                        ;; another one.
+                        (coding-system-base buffer-file-coding-system)))))
       ;; otherwise, try to find one based on the file name
       (car (find-operation-coding-system 'insert-file-contents file))
       ;; and a final fallback

bug#4451: 23.1; EOL problems with vc-diff and cygwin

[Eli Zaretskii]

> Date: Thu, 17 Sep 2009 16:26:12 +0200
> From: Reiner Steib <reiner.steib@gmx.de>
> Cc: 4451@emacsbugs.donarmstrong.com
> 
> > If you diff the same two files from the shell prompt using the same
> > diff.exe, what kind of EOLs do you get in the output?  Is the result
> > similar to what you see in vc-diff?
> 
> I don't diff two files, but I use CVS on a single file.  So I guess  
> you mean the "cvs diff" output.  The output of "cvs diff" in cygwin's  
> bash has DOS line endings, no duplicate "^M", see below.

So does it help to add --binary to the diff switches?  (Sorry, I have
no idea how to do that with vc-diff, I just hope there is a way.)

> The saved Emacs buffer from vc-diff (vc-diff-2.output) has "^M^M$"  
> whereas the buffer from cvs-mode-diff (cvs-mode-diff.output) has  
> "^M$".

What is the value of buffer-file-coding-system in both of these
buffers?

bug#4451: 23.1; EOL problems with vc-diff and cygwin

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: 4451@emacsbugs.donarmstrong.com, Eli Zaretskii <eliz@gnu.org>
> Date: Thu, 17 Sep 2009 12:35:37 -0400
> 
> I think the best solution is to change vc-coding-system-for-diff so that
> when it takes the coding system from (with-current-buffer buf
> buffer-file-coding-system), it ignores the EOL part of that
> coding system.

Why do you think so?

My line of thinking was that a diff of a file with Unix EOLs should
always yield a diffs file with Unix EOLs.

bug#4451: 23.1; EOL problems with vc-diff and cygwin

[Stefan Monnier]

>> I think the best solution is to change vc-coding-system-for-diff so that
>> when it takes the coding system from (with-current-buffer buf
>> buffer-file-coding-system), it ignores the EOL part of that
>> coding system.
> Why do you think so?

If we let Emacs (re)discover the EOL of the diff output, we are less
likely to get it wrong:
- if both the diff markers and the text use Unix EOL, we'll detect it right.
- if both the diff markers and the text use DOS EOL, we'll detect it right.
- if the diff markers uses DOS EOLs and the text use Unix EOL,
  we'll detect it as Unix which is about as good as it gets, short of
  decoding the markers and the text separately.
- if the diff markers uses Unix EOLs and the text use DOS EOL, we'll
  detect it as Unix which is not always ideal, admittedly, but it's
  still the correct thing to do in some cases (e.g. if the old version
  of the file used Unix EOL).

So maybe, we should drop the provided EOL info (as in my patch) if it
says "-unix" but keep it if it says "-dos".

> My line of thinking was that a diff of a file with Unix EOLs should
> always yield a diffs file with Unix EOLs.

Apparently this is not the case for Reiner's setup.


        Stefan

bug#4451: 23.1; EOL problems with vc-diff and cygwin

[Reiner Steib]

Stefan Monnier <monnier@iro.umontreal.ca> wrote:

> I think the best solution is to change vc-coding-system-for-diff so that
> when it takes the coding system from (with-current-buffer buf
> buffer-file-coding-system), it ignores the EOL part of that
> coding system.
>
> Does the patch below help?

It does help.  Thanks.

> -		    buffer-file-coding-system)))
> +                    (if buffer-file-coding-system
> +                        ;; Don't inherit the EOL part of the coding-system,
> +                        ;; because some diff tools may choose to use
> +                        ;; another one.
> +                        (coding-system-base buffer-file-coding-system)))))

Shouldn't the comment mention "bug#4451"?

Bye, Reiner

bug#4451: 23.1; EOL problems with vc-diff and cygwin

[Reiner Steib]

Eli Zaretskii <eliz@gnu.org> wrote:

> So does it help to add --binary to the diff switches?  (Sorry, I have
> no idea how to do that with vc-diff, I just hope there is a way.)

There is:

| vc-cvs-diff-switches is a variable defined in `vc-cvs.el'.
| Its value is "-u"

Adding "--binary" results in completely unusable result:

| Index: K3.xml^M
| ===================================================================^M
| RCS file: [...]/K3.xml,v^M
| retrieving revision 1.2^M
| diff -u --binary -u -r1.2 K3.xml^M
| cvs.exe diff: warning: did not set stdout to binary mode^M
| --- K3.xml	24 Jul 2009 16:09:12 -0000	1.2^M
| +++ K3.xml	16 Sep 2009 11:20:27 -0000^M
| @@ -1,4365 +1,4379 @@^M
| -<?xml version='1.0'	^M^M
| - encoding='UTF-8'	^M^M
| -?>	^M^M
| -<policy name='K3'	^M^M
| - version='2.0.0'	^M^M
| - schema-version='1.0.0'	^M^M
| [... complete file follows ...]
| -</bar>^M^M
| -</foo>^M^M
| +<?xml version='1.0'	^M
| + encoding='UTF-8'	^M
| +?>	^M
| +<policy name='K3_test'	^M
| + version='2.0.0'	^M
| + schema-version='1.0.0'	^M
| [... complete file follows ...]

>> The saved Emacs buffer from vc-diff (vc-diff-2.output) has "^M^M$"
>> whereas the buffer from cvs-mode-diff (cvs-mode-diff.output) has
>> "^M$".
>
> What is the value of buffer-file-coding-system in both of these
> buffers?

vc-diff / buffer *vc-diff*: iso-latin-1-dos

(saving this buffer and using `find-file-literally' display "^M^M$".)

cvs-mode-diff / buffer *cvs-diff*: iso-latin-1-dos

The diffed file file K3.xml: utf-8-unix

Stefan's patch seem to work.  See my other mail.

Bye, Reiner

bug#4451: 23.1; EOL problems with vc-diff and cygwin

[Reiner Steib]

On Fri, Sep 18 2009, Reiner Steib wrote:

> Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>
>> I think the best solution is to change vc-coding-system-for-diff so that
>> when it takes the coding system from (with-current-buffer buf
>> buffer-file-coding-system), it ignores the EOL part of that
>> coding system.
>>
>> Does the patch below help?
>
> It does help.  Thanks.

Any reason not to commit the patch?

>> -		    buffer-file-coding-system)))
>> +                    (if buffer-file-coding-system
>> +                        ;; Don't inherit the EOL part of the coding-system,
>> +                        ;; because some diff tools may choose to use
>> +                        ;; another one.
>> +                        (coding-system-base buffer-file-coding-system)))))
>
> Shouldn't the comment mention "bug#4451"?

Bye, Reiner.
-- 
       ,,,
      (o o)
---ooO-(_)-Ooo---  |  PGP key available  |  http://rsteib.home.pages.de/

bug#4451: 23.1; EOL problems with vc-diff and cygwin

[Stefan Monnier]

>>> I think the best solution is to change vc-coding-system-for-diff so that
>>> when it takes the coding system from (with-current-buffer buf
>>> buffer-file-coding-system), it ignores the EOL part of that
>>> coding system.
>>> Does the patch below help?
>> It does help.  Thanks.
> Any reason not to commit the patch?

Eli didn't seem convinced about it.  And it may cause poor results
in other cases as well.  I'm not familiar enough with the issue (which
seems to be very w32-specific and depend heavily on the particular
diff.exe tool you use), to make a good judgment call.


        Stefan

bug#4451: 23.1; EOL problems with vc-diff and cygwin

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: Eli Zaretskii <eliz@gnu.org>
> Date: Thu, 24 Sep 2009 18:07:08 -0400
> 
> > Any reason not to commit the patch?
> 
> Eli didn't seem convinced about it.  And it may cause poor results
> in other cases as well.  I'm not familiar enough with the issue (which
> seems to be very w32-specific and depend heavily on the particular
> diff.exe tool you use), to make a good judgment call.

What would be good is to investigate the original problem thoroughly
and post the exact reason(s) of the failure.  Reiner, could you perhaps
do that?  I'm afraid I'm too busy with other things lately.

With the precise diagnostics in hand, I'm quite sure we will be able
to find a reliable solution.

bug#4451: 23.1; EOL problems with vc-diff and cygwin

[Reiner Steib]

On Fri, Sep 25 2009, Eli Zaretskii wrote:

>> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
[...]
>> Eli didn't seem convinced about it.  And it may cause poor results
>> in other cases as well.  I'm not familiar enough with the issue (which
>> seems to be very w32-specific and depend heavily on the particular
>> diff.exe tool you use), to make a good judgment call.
>
> What would be good is to investigate the original problem thoroughly
> and post the exact reason(s) of the failure.  Reiner, could you perhaps
> do that?  

I think I posted all data you asked for, or did I forget something?

Bye, Reiner.
-- 
       ,,,
      (o o)
---ooO-(_)-Ooo---  |  PGP key available  |  http://rsteib.home.pages.de/

bug#4451: 23.1; EOL problems with vc-diff and cygwin

[Eli Zaretskii]

> From: Reiner Steib <reinersteib+gmane@imap.cc>
> Cc: 4451@emacsbugs.donarmstrong.com, Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Sat, 26 Sep 2009 10:27:38 +0200
> 
> On Fri, Sep 25 2009, Eli Zaretskii wrote:
> 
> >> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> [...]
> >> Eli didn't seem convinced about it.  And it may cause poor results
> >> in other cases as well.  I'm not familiar enough with the issue (which
> >> seems to be very w32-specific and depend heavily on the particular
> >> diff.exe tool you use), to make a good judgment call.
> >
> > What would be good is to investigate the original problem thoroughly
> > and post the exact reason(s) of the failure.  Reiner, could you perhaps
> > do that?  
> 
> I think I posted all data you asked for, or did I forget something?

You posted the data I asked for, yes (thanks!), but what I'm asking
now is different.

What I'd like to see is where in Emacs sources we examine the output
we get from Diff, and where and why we err as to what EOL format
should be used for decoding that output.

One possibility for this mistake might be that Diff produces
inconsistent EOL format in its output, for example if Diff or its VC
front-end outputs some headers that have Unix EOLs and then the actual
diffs with DOS EOLs.

Another possibility is that somewhere along the chain of processing
the output, we force EOL conversion to be Unix-style, instead of
detecting EOLs dynamically, or maybe even forcing it to DOS (if we
have clear evidence for doing the latter).

It could also be some mix of these.

These are just the hypotheses; only by tracing the processing through
its stages and taking notes for every stage that deals with decoding
output from external processes or from temporary files, it will be
possible to establish facts and determine what and where goes wrong.

TIA

bug#4451: 23.1; EOL problems with vc-diff and cygwin

[Stefan Monnier]

> What I'd like to see is where in Emacs sources we examine the output
> we get from Diff, and where and why we err as to what EOL format
> should be used for decoding that output.

I don't know what you mean here.

> One possibility for this mistake might be that Diff produces
> inconsistent EOL format in its output, for example if Diff or its VC
> front-end outputs some headers that have Unix EOLs and then the actual
> diffs with DOS EOLs.

AFAICT, Reiner's situation is that he has a file with (consistent)
Unix-style EOLs, and "cvs diff <file>" on that file returns a diff using
(consistently) DOS-style EOLs.

Currently, VC reads diff's output using the coding-system used for the
file, which seems like a sane choice, but in the above case it leads to the
problem we've seen.


        Stefan

bug#4451: 23.1; EOL problems with vc-diff and cygwin

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Reiner Steib <Reiner.Steib@gmx.de>,  4451@emacsbugs.donarmstrong.com
> Date: Sat, 26 Sep 2009 20:36:54 -0400
> 
> Currently, VC reads diff's output using the coding-system used for the
> file, which seems like a sane choice, but in the above case it leads to the
> problem we've seen.

If this is the whole truth, then how come using the --binary switch to
Diff (which is supposed to produce Unix-style EOLs in Diff's output)
results in an even more garbled text in the Emacs buffer?

bug#4451: 23.1; EOL problems with vc-diff and cygwin

[Stefan Monnier]

>> Currently, VC reads diff's output using the coding-system used for the
>> file, which seems like a sane choice, but in the above case it leads to the
>> problem we've seen.

> If this is the whole truth, then how come using the --binary switch to
> Diff (which is supposed to produce Unix-style EOLs in Diff's output)
> results in an even more garbled text in the Emacs buffer?

No sure.  One interesting thing is that his --binary output seems to
indicate that his file's revision 1.2 was using DOS-style EOLs wereas
the current version is using Unix-style EOLs.  So maybe that has
something to do with the problem, indeed.  Also, of course it may also
depend on the CVS version used on the repository's server.


        Stefan

bug#4451: 23.1; EOL problems with vc-diff and cygwin

[Reiner Steib]

On Sun, Sep 27 2009, Stefan Monnier wrote:

> One interesting thing is that his --binary output seems to indicate
> that his file's revision 1.2 was using DOS-style EOLs wereas the
> current version is using Unix-style EOLs.  So maybe that has
> something to do with the problem, indeed.

All revisions of the XML file _should_ have Unix-style EOLs.  I'm not
100% sure they have in the example I showed (I can't verify this
before October, sorry).  But I had the very same symptoms with
different XML files.

> Also, of course it may also depend on the CVS version used on the
> repository's server.

The repository is on a mounted Windows share.

BTW, why does `=' (`cvs-mode-diff') from PCL-CVS mode handle EOLs
different to VC?

Bye, Reiner.
-- 
       ,,,
      (o o)
---ooO-(_)-Ooo---  |  PGP key available  |  http://rsteib.home.pages.de/

bug#4451: 23.1; EOL problems with vc-diff and cygwin

[Stefan Monnier]

>> One interesting thing is that his --binary output seems to indicate
>> that his file's revision 1.2 was using DOS-style EOLs wereas the
>> current version is using Unix-style EOLs.  So maybe that has
>> something to do with the problem, indeed.

> All revisions of the XML file _should_ have Unix-style EOLs.  I'm not
> 100% sure they have in the example I showed (I can't verify this
> before October, sorry).  But I had the very same symptoms with
> different XML files.

The reason why I think your file had DOS-style EOLs on revision 1.2 is
that the --binary diff you showed had ^M^M^J on the "old text" and just
^M^J on the "new text".

>> Also, of course it may also depend on the CVS version used on the
>> repository's server.
> The repository is on a mounted Windows share.

Could it be that the RCS files accessed this way get an accidental
LF->CRLF conversion done by the network-file-system?
Seems pretty unlikely.  But could you try and copy (part of) the
repository to a local directory and try the operation again, just to
rule out any funny business from this side?

> BTW, why does `=' (`cvs-mode-diff') from PCL-CVS mode handle EOLs
> different to VC?

Because cvs-mode-diff applies to several files and doesn't presume that
those files are probably currently visited in Emacs buffers, so it just
always relies on Emacs's auto-detection instead.  Sometimes it works
well, but in general it's not as good as what VC does (typically diffs
involving iso-2022 files in a "latin" locale don't decode the iso-2022
encoding).


        Stefan

bug#4451: 23.1; EOL problems with vc-diff and cygwin

[Reiner Steib]

Eli Zaretskii wrote:

> What I'd like to see is where in Emacs sources we examine the output
> we get from Diff, and where and why we err as to what EOL format
> should be used for decoding that output.

`vc-coding-system-for-diff' is called 2 times when I do `C-x v ='

1)
   vc-coding-system-for-diff("c:/Users/x123456/tmp/check-out/K3.xml")
   vc-diff-internal(t (CVS ("c:/Users/x123456/tmp/check-out/K3.xml"))  
nil nil t)
   vc-diff(nil t)
   call-interactively(vc-diff nil nil)

2)
   vc-coding-system-for-diff("c:/Users/x123456/tmp/check-out/K3.xml")
   vc-cvs-diff(("c:/Users/x123456/tmp/check-out/K3.xml") nil nil "*vc-diff*")
   apply(vc-cvs-diff (("c:/Users/x123456/tmp/check-out/K3.xml") nil  
nil "*vc-diff*"))
   vc-call-backend(CVS diff ("c:/Users/x123456/tmp/check-out/K3.xml")  
nil nil "*vc-diff*")
   vc-diff-internal(t (CVS ("c:/Users/x123456/tmp/check-out/K3.xml"))  
nil nil t)
   vc-diff(nil t)
   call-interactively(vc-diff nil nil)

> One possibility for this mistake might be that Diff produces
> inconsistent EOL format in its output, for example if Diff or its VC
> front-end outputs some headers that have Unix EOLs and then the actual
> diffs with DOS EOLs.

The repository file (K3.xml,v) has Unix EOLs.  But if I do a fresh
checkout, I get a file K3.xml with DOS EOLs (I think this is the usual
behavoir of the Windows cvs binaries[1] for text files unless you
specify the switch "-ko").  However, in my workflow I overwrite the file
with a Unix EOL file (exported from some application), do modifications,
diffs and check it in.

Probably this conversion is also the reason that "cvs diff --binary"
outputs "^M^M$" for the old file and "^M$" for the new file.  "cvs diff"
outputs consitent DOS EOLs (both diff markers and the text):

$ cvs diff SK3.xml | cat --show-all | grep -F -v '^M' | wc -l


> Another possibility is that somewhere along the chain of processing
> the output, we force EOL conversion to be Unix-style, instead of
> detecting EOLs dynamically, or maybe even forcing it to DOS (if we
> have clear evidence for doing the latter).

`vc-coding-system-for-diff' returns `utf-8-unix' in both calls.


Stefan Monnier wrote:
> Could it be that the RCS files accessed this way get an accidental
> LF->CRLF conversion done by the network-file-system?
> Seems pretty unlikely.  But could you try and copy (part of) the
> repository to a local directory and try the operation again, just to
> rule out any funny business from this side?

I copied the repository file to a local drive and also used local drive
working copy (drive c:).  But that doesn't make any difference.

Bye, Reiner

[1] $ cvs --version

Concurrent Versions System (CVS) 1.11.22 (client)

Copyright (C) 2006 Free Software Foundation, Inc.

bug#4451: 23.1; EOL problems with vc-diff and cygwin

[Stefan Monnier]

> `vc-coding-system-for-diff' is called 2 times when I do `C-x v ='

> 1)
>   vc-coding-system-for-diff("c:/Users/x123456/tmp/check-out/K3.xml")
>   vc-diff-internal(t (CVS ("c:/Users/x123456/tmp/check-out/K3.xml"))  nil
> nil t)
>   vc-diff(nil t)
>   call-interactively(vc-diff nil nil)

> 2)
>   vc-coding-system-for-diff("c:/Users/x123456/tmp/check-out/K3.xml")
>   vc-cvs-diff(("c:/Users/x123456/tmp/check-out/K3.xml") nil nil "*vc-diff*")
>   apply(vc-cvs-diff (("c:/Users/x123456/tmp/check-out/K3.xml") nil  nil
> "*vc-diff*"))
>   vc-call-backend(CVS diff ("c:/Users/x123456/tmp/check-out/K3.xml")  nil
> nil "*vc-diff*")
>   vc-diff-internal(t (CVS ("c:/Users/x123456/tmp/check-out/K3.xml"))  nil
> nil t)
>   vc-diff(nil t)
>   call-interactively(vc-diff nil nil)

Looks like vc-cvs-diff sets up the coding system redundantly.

>> One possibility for this mistake might be that Diff produces
>> inconsistent EOL format in its output, for example if Diff or its VC
>> front-end outputs some headers that have Unix EOLs and then the actual
>> diffs with DOS EOLs.

> The repository file (K3.xml,v) has Unix EOLs.  But if I do a fresh
> checkout, I get a file K3.xml with DOS EOLs (I think this is the usual
> behavoir of the Windows cvs binaries[1] for text files unless you
> specify the switch "-ko").  However, in my workflow I overwrite the file
> with a Unix EOL file (exported from some application), do modifications,
> diffs and check it in.

Well, that's probably a good explanation for why you see the problem and
others don't: overwriting a file with another is not exaclty standard
practice in revision-control-systems.  I'm surprised that CVS doesn't
consider that every line in the file is modified (AFAIK that's what it
does if you copy the DOS-EOL version of a file atop a Unix-EOL file).

IOW: don't do that.  Either convince CVS to use Unix-EOLs on this file,
or use DOS-EOLs.


        Stefan

bug#4451: 23.1; EOL problems with vc-diff and cygwin

[Eli Zaretskii]

> Date: Mon, 05 Oct 2009 18:07:53 +0200
> From: Reiner Steib <reiner.steib@gmx.de>
> Cc: 4451@emacsbugs.donarmstrong.com, monnier@iro.umontreal.ca
> 
> The repository file (K3.xml,v) has Unix EOLs.  But if I do a fresh
> checkout, I get a file K3.xml with DOS EOLs (I think this is the usual
> behavoir of the Windows cvs binaries[1] for text files unless you
> specify the switch "-ko").

Yes, that's normal behavior of the ported CVS.

> However, in my workflow I overwrite the file with a Unix EOL file
> (exported from some application)

Why are you doing that, and what is that ``some application'' which
converts the DOS EOLs into Unix?

> Probably this conversion is also the reason that "cvs diff --binary"
> outputs "^M^M$" for the old file and "^M$" for the new file.  "cvs diff"
> outputs consitent DOS EOLs (both diff markers and the text):
> 
> $ cvs diff SK3.xml | cat --show-all | grep -F -v '^M' | wc -l

It sounds like your port of CVS assumes that text files have DOS
EOLs.  If you violate that assumption, it produces badly formatted
files, as far as EOL is concerned, and all hell will break loose on
you; Emacs showing the ugly ^M characters is just one of the
symptoms.

> > Another possibility is that somewhere along the chain of processing
> > the output, we force EOL conversion to be Unix-style, instead of
> > detecting EOLs dynamically, or maybe even forcing it to DOS (if we
> > have clear evidence for doing the latter).
> 
> `vc-coding-system-for-diff' returns `utf-8-unix' in both calls.

Because the file is already visited in an Emacs buffer, I presume, and
vc-coding-system-for-diff uses that.

So I agree with Stefan that you should simply not overwrite the
checked-out file with Unix EOLs.  We could install the change
suggested by Stefan (based on this analysis, I no longer object to
it), but I'm quite sure this won't be the end of your trouble.  For
example, Patch will most probably fail to apply the diffs, because
they have DOS EOLs while the source file has Unix EOLs.  This means
that CVS merges will also fail.  Etc., etc. -- that way lies madness.

If you really want Unix EOLs, you will have to hack CVS to use binary
I/O.  (I think you can force that by using some special Cygwin options
when ``mounting'' the volume, but don't ask me for details, because I
don't use Cygwin.)

Thanks.

bug#4451: 23.1; EOL problems with vc-diff and cygwin

[Reiner Steib]

tags 4451 + patch
quit
----- end of commands for control@..., bcc-ed -----

[ Sorry for the very late reply.  With Stefan's patch, I had no
  problems so far.  But an upgrade to Emacs 23.2 brought the unpatched
  version and thus the problem back. ]

On Mon, Oct 05 2009, Eli Zaretskii wrote:

>> Date: Mon, 05 Oct 2009 18:07:53 +0200
>> From: Reiner Steib <reiner.steib@gmx.de>
>> Cc: 4451@emacsbugs.donarmstrong.com, monnier@iro.umontreal.ca
>> 
>> The repository file (K3.xml,v) has Unix EOLs.  But if I do a fresh
>> checkout, I get a file K3.xml with DOS EOLs (I think this is the usual
>> behavoir of the Windows cvs binaries[1] for text files unless you
>> specify the switch "-ko").
>
> Yes, that's normal behavior of the ported CVS.
>
>> However, in my workflow I overwrite the file with a Unix EOL file
>> (exported from some application)
>
> Why are you doing that, and what is that ``some application'' which
> converts the DOS EOLs into Unix?

It doesn't convert EOLs.  It's a commercial application (used by my
employer).  It stores its configuration in XML files including very
limited version control.  But it doesn't offer stuff like diff etc
which CVS does.  But we want to be able to diff revisions and/or
different configuration files.  So we export the config files from the
application (-> Unix EOL) - more precise it is out testing environment
-, add newlines to simplify diffing, review the changes and commit the
file to CVS.  We also specific (stable, tested) versions of the config
file from CVS and import it into the application (production
environment).

>> Probably this conversion is also the reason that "cvs diff --binary"
>> outputs "^M^M$" for the old file and "^M$" for the new file.  "cvs diff"
>> outputs consitent DOS EOLs (both diff markers and the text):
>> 
>> $ cvs diff SK3.xml | cat --show-all | grep -F -v '^M' | wc -l
>
> It sounds like your port of CVS assumes that text files have DOS
> EOLs.  If you violate that assumption, it produces badly formatted
> files, as far as EOL is concerned, and all hell will break loose on
> you; Emacs showing the ugly ^M characters is just one of the
> symptoms.
>
>> > Another possibility is that somewhere along the chain of processing
>> > the output, we force EOL conversion to be Unix-style, instead of
>> > detecting EOLs dynamically, or maybe even forcing it to DOS (if we
>> > have clear evidence for doing the latter).
>> 
>> `vc-coding-system-for-diff' returns `utf-8-unix' in both calls.
>
> Because the file is already visited in an Emacs buffer, I presume, and
> vc-coding-system-for-diff uses that.
>
> So I agree with Stefan that you should simply not overwrite the
> checked-out file with Unix EOLs.  We could install the change
> suggested by Stefan (based on this analysis, I no longer object to
> it), 

If nobody else objects, may I install[1] Stefan's patch?  Or we could
make it depend on some variable as well ...

--8<---------------cut here---------------start------------->8---
--- vc.el.~1~   2010-04-03 18:26:12.000000000 +0200
+++ vc.el       2010-07-13 12:09:53.972342500 +0200
@@ -1398,6 +1398,12 @@
 ;;          (vc-call-backend ',(vc-backend f)
 ;;                           'diff (list ',f) ',rev1 ',rev2))))))

+(defvar vc-coding-system-strip-eol t ;; nil
+  "When non-nil, don't inherit the EOL part of the coding-system.")
+;; Don't inherit the EOL part of the coding-system in
+;; `vc-coding-system-for-diff', because some diff tools may choose to
+;; use another one.  bug#4451.
+
 (defun vc-coding-system-for-diff (file)
   "Return the coding system for reading diff output for FILE."
   (or coding-system-for-read
@@ -1405,7 +1411,12 @@
       ;; use the buffer's coding system
       (let ((buf (find-buffer-visiting file)))
         (when buf (with-current-buffer buf
-                   buffer-file-coding-system)))
+                   (if vc-coding-system-strip-eol
+                       ;; Don't inherit the EOL part of the coding-system,
+                       ;; because some diff tools may choose to use
+                       ;; another one.  bug#4451.
+                       (coding-system-base buffer-file-coding-system)
+                     buffer-file-coding-system))))
       ;; otherwise, try to find one based on the file name
       (car (find-operation-coding-system 'insert-file-contents file))
       ;; and a final fallback

--8<---------------cut here---------------end--------------->8---

> but I'm quite sure this won't be the end of your trouble.  For
> example, Patch will most probably fail to apply the diffs, because
> they have DOS EOLs while the source file has Unix EOLs.  This means
> that CVS merges will also fail.  Etc., etc. -- that way lies
> madness.

Up to now, I didn't see other EOL related problems.  Patch works fine
with the diffs from Emacs buffer.

> If you really want Unix EOLs, you will have to hack CVS to use binary
> I/O.  (I think you can force that by using some special Cygwin options
> when ``mounting'' the volume, but don't ask me for details, because I
> don't use Cygwin.)

I can't do that because I don't control the cygwin configuration (and
I would not like to change it since it might cause problems).

> Thanks.

Thanks for your help.

Bye, Reiner.

[1] It will take a while since I didn't set up bazaar upto now.
-- 
       ,,,
      (o o)
---ooO-(_)-Ooo---  |  PGP key available  |  http://rsteib.home.pages.de/

bug#4451: 23.1; EOL problems with vc-diff and cygwin

[Eli Zaretskii]

> From: Reiner Steib <reinersteib+gmane@imap.cc>
> Cc: 4451@debbugs.gnu.org, Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Thu, 15 Jul 2010 09:22:39 +0200
> 
> > So I agree with Stefan that you should simply not overwrite the
> > checked-out file with Unix EOLs.  We could install the change
> > suggested by Stefan (based on this analysis, I no longer object to
> > it), 
> 
> If nobody else objects, may I install[1] Stefan's patch?  Or we could
> make it depend on some variable as well ...

Fine with me.

> +(defvar vc-coding-system-strip-eol t ;; nil
> +  "When non-nil, don't inherit the EOL part of the coding-system.")

Instead of a slightly misleading name (you don't really "strip" EOL)
and double negation, I would suggest an option named
vc-coding-system-inherit-eol, non-nil by default (to keep the current
operation), and reverse the condition in the patch.

bug#4451: 23.1; EOL problems with vc-diff and cygwin

[Reiner Steib]

On Thu, Jul 15 2010, Eli Zaretskii wrote:

>> From: Reiner Steib <reinersteib+gmane@imap.cc>
>> Cc: 4451@debbugs.gnu.org, Stefan Monnier <monnier@iro.umontreal.ca>
>> Date: Thu, 15 Jul 2010 09:22:39 +0200
>> 
>> > So I agree with Stefan that you should simply not overwrite the
>> > checked-out file with Unix EOLs.  We could install the change
>> > suggested by Stefan (based on this analysis, I no longer object to
>> > it), 
>> 
>> If nobody else objects, may I install[1] Stefan's patch?  Or we could
>> make it depend on some variable as well ...
>
> Fine with me.

Thanks.

>> +(defvar vc-coding-system-strip-eol t ;; nil
>> +  "When non-nil, don't inherit the EOL part of the coding-system.")
>
> Instead of a slightly misleading name (you don't really "strip" EOL)
> and double negation, I would suggest an option named
> vc-coding-system-inherit-eol, non-nil by default (to keep the current
> operation), and reverse the condition in the patch.

Heres an updated patch.  Could someone please apply it (I don't have
the current sources - no bzr checkout yet)?

--8<---------------cut here---------------start------------->8---
--- vc.el	7 Dec 2009 09:02:16 -0000	1.746
+++ vc.el	16 Jul 2010 06:47:30 -0000
@@ -1388,6 +1388,12 @@
 ;;          (vc-call-backend ',(vc-backend f)
 ;;                           'diff (list ',f) ',rev1 ',rev2))))))
 
+(defvar vc-coding-system-inherit-eol t
+  "When non-nil, inherit the EOL part of the coding-system from buffer.
+Set this variable to nil if your diff tools chooses to use other
+EOL type than the current buffer.  Then auto-detection gives
+better.") ;; Cf. bug#4451.
+
 (defun vc-coding-system-for-diff (file)
   "Return the coding system for reading diff output for FILE."
   (or coding-system-for-read
@@ -1395,7 +1401,12 @@
       ;; use the buffer's coding system
       (let ((buf (find-buffer-visiting file)))
         (when buf (with-current-buffer buf
-		    buffer-file-coding-system)))
+		    (if vc-coding-system-inherit-eol
+			buffer-file-coding-system
+		      ;; Don't inherit the EOL part of the coding-system,
+		      ;; because some diff tools may choose to use
+		      ;; another one.  bug#4451.
+		      (coding-system-base buffer-file-coding-system)))))
       ;; otherwise, try to find one based on the file name
       (car (find-operation-coding-system 'insert-file-contents file))
       ;; and a final fallback
--8<---------------cut here---------------end--------------->8---

Bye, Reiner.
-- 
       ,,,
      (o o)
---ooO-(_)-Ooo---  |  PGP key available  |  http://rsteib.home.pages.de/

bug#4451: 23.1; EOL problems with vc-diff and cygwin

[Andreas Schwab]

Reiner Steib <Reiner.Steib@gmx.de> writes:

> +(defvar vc-coding-system-inherit-eol t
> +  "When non-nil, inherit the EOL part of the coding-system from buffer.

Which buffer?

> +Set this variable to nil if your diff tools chooses to use other
> +EOL type than the current buffer.  Then auto-detection gives
> +better.") ;; Cf. bug#4451.

Better what?

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

bug#4451: 23.1; EOL problems with vc-diff and cygwin

[Eli Zaretskii]

> From: Reiner Steib <Reiner.Steib@gmx.de>
> Cc: 4451@debbugs.gnu.org
> Date: Fri, 16 Jul 2010 08:51:37 +0200
> 
> Heres an updated patch.  Could someone please apply it (I don't have
> the current sources - no bzr checkout yet)?

Done.  Thanks.

bug#4451: 23.1; EOL problems with vc-diff and cygwin

[Reiner Steib]

On Fri, Jul 16 2010, Andreas Schwab wrote:

> Reiner Steib <Reiner.Steib@gmx.de> writes:
>> +  "When non-nil, inherit the EOL part of the coding-system from buffer.
>
> Which buffer?
>
>> [...]Then auto-detection gives better.") ;; Cf. bug#4451.
>
> Better what?

Oh, I shouldn't prepare patches in a hurry. :-(

On Fri, Jul 16 2010, Eli Zaretskii wrote:

>> From: Reiner Steib <Reiner.Steib@gmx.de>
>> Heres an updated patch.  Could someone please apply it [...]?
>
> Done.  Thanks.

Thanks for fixing and committing the patch!

Bye, Reiner.
-- 
       ,,,
      (o o)
---ooO-(_)-Ooo---  |  PGP key available  |  http://rsteib.home.pages.de/