bug#29916: 26.0.90; CRLF in diff-command output breaks smerge hunk header parsing

bug#29916: 26.0.90; CRLF in diff-command output breaks smerge hunk header parsing

[Richard Copley]

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

On Windows, CRLF line endings in the output of diff-command can lead to
an error in `smerge-refine-regions'. To reproduce, download this patch:

https://lists.gnu.org/archive/html/emacs-devel/2017-06/txtWF9rI8yqfI.txt

(It is an example of a perfectly ordinary patch, with Unix line endings.)

From 'emacs -Q', visit the patch file, do "M-x diff-mode RET", then
move point into the second diff hunk (in editfns.c) and type RET
(diff-goto-source).

The minibuffer shows this error, with a literal carriage return, shown
here as ^M:

  Error running timer: (error "Unexpected patch hunk header: 41a42,121^M")

The error doesn't seem to be critical (diff-goto-source still succeeds)
but it's annoying, as it obscures a minibuffer prompt.

As a data point, the error doesn't happen after applying the attached
patch which partially reverts this commit:

  commit 184d74ce002ecb7399cb2b47fc671bfb2feb9855
  Author: Stefan Monnier <monnier@iro.umontreal.ca>
  Date:   Wed May 17 15:44:36 2017 -0400
  * lisp/vc/smerge-mode.el (smerge-refine-regions): Work in multi-bufs

The Lisp backtrace is:

Debugger entered--Lisp error: (error "Unexpected patch hunk header:
41a42,121\015")
  signal(error ("Unexpected patch hunk header: 41a42,121\015"))
  error("Unexpected patch hunk header: %s" "41a42,121\015")
  smerge-refine-regions(1019 1212 1212 1485 nil diff-refine-preproc
((diff-mode . fine) (face diff-refine-removed)) ((diff-mode . fine)
(face diff-refine-added)))
  diff-refine-hunk()
  #f(compiled-function () #<bytecode 0x1b0d50d>)()
  apply(#f(compiled-function () #<bytecode 0x1b0d50d>) nil)
  timer-event-handler([t 23113 10990 354825 nil #f(compiled-function
() #<bytecode 0x1b0d50d>) nil nil 0])
  read-from-minibuffer("Use file
c:/Users/xxxxxx/AppData/Local/Temp/b/src/editfns.c: "
("c:/Users/xxxxxx/AppData/Local/Temp/b/src/editfns.c" . 42) (keymap
...) nil file-name-history
"c:/Users/xxxxxx/AppData/Local/Temp/b/src/editfns.c" nil)
  completing-read-default("Use file
c:/Users/xxxxxx/AppData/Local/Temp/b/src/editfns.c: "
read-file-name-internal file-exists-p t
("c:/Users/xxxxxx/AppData/Local/Temp/b/src/editfns.c" . 41)
file-name-history "c:/Users/xxxxxx/AppData/Local/Temp/b/src/editfns.c"
nil)
  completing-read("Use file
c:/Users/xxxxxx/AppData/Local/Temp/b/src/editfns.c: "
read-file-name-internal file-exists-p t
("c:/Users/xxxxxx/AppData/Local/Temp/b/src/editfns.c" . 41)
file-name-history
"c:/Users/xxxxxx/AppData/Local/Temp/b/src/editfns.c")
  read-file-name-default("Use file
c:/Users/xxxxxx/AppData/Local/Temp/b/src/editfns.c: "
"c:/Users/xxxxxx/AppData/Local/Temp/b/src/"
"c:/Users/xxxxxx/AppData/Local/Temp/b/src/editfns.c" t "editfns.c"
nil)
  read-file-name("Use file
c:/Users/xxxxxx/AppData/Local/Temp/b/src/editfns.c: "
"c:/Users/xxxxxx/AppData/Local/Temp/b/src/"
"c:/Users/xxxxxx/AppData/Local/Temp/b/src/editfns.c" t "editfns.c")
  diff-find-file-name(nil nil)
  diff-find-source-location(nil t)
  diff-goto-source(nil return)
  funcall-interactively(diff-goto-source nil return)
  call-interactively(diff-goto-source nil nil)
  command-execute(diff-goto-source)



In GNU Emacs 26.0.90 (build 1, x86_64-w64-mingw32)
 of 2017-12-31 built on MACHINE
Repository revision: 39ca289a7a33d514c2a46f005db4e7173fb7e9f5
Windowing system distributor 'Microsoft Corp.', version 10.0.16299
Recent messages:
Wrote c:/projects/emacs/lisp/vc/smerge-mode.el
Quit [2 times]
Finding changes in c:/projects/emacs/lisp/vc/smerge-mode.el...done
Hunk already applied
Annotating...
Redisplaying annotation...done (Spanned from 6597.8 to 16.8 days old)
Annotating... done
Mark set
Mark deactivated
x is undefined

Configured using:
 'configure --config-cache --with-modules --without-pop 'CFLAGS=-O3
 -ggdb3''

Configured features:
XPM JPEG TIFF GIF PNG SOUND DBUS NOTIFY ACL GNUTLS LIBXML2 ZLIB
TOOLKIT_SCROLL_BARS MODULES LCMS2

Important settings:
  value of $EMACSLOADPATH: c:\emacs-lisp;
  value of $LANG: ENG
  locale-coding-system: cp1252

Major mode: Git-Log-View

Minor modes in effect:
  diff-auto-refine-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  buffer-read-only: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc puny format-spec rfc822 mml
mml-sec password-cache epa derived epg epg-config gnus-util rmail
rmail-loaddefs mm-decode mm-bodies mm-encode mail-parse rfc2231
mailabbrev gmm-utils mailheader sendmail rfc2047 rfc2045 ietf-drums
mm-util mail-prsvr mail-utils vc-annotate vc-dir ewoc smerge-mode
m4-mode whitespace add-log log-view pcvs-util cl-extra help-mode vc-git
diff-mode easymenu easy-mmode vc-bzr vc-src vc-sccs vc-svn vc-cvs vc-rcs
vc vc-dispatcher map seq byte-opt gv bytecomp byte-compile cconv
cl-loaddefs cl-lib dired dired-loaddefs elec-pair time-date mule-util
tooltip eldoc electric uniquify ediff-hook vc-hooks lisp-float-type
mwheel dos-w32 ls-lisp disp-table term/w32-win w32-win w32-vars
term/common-win tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode elisp-mode lisp-mode
prog-mode register page menu-bar rfn-eshadow isearch timer select
scroll-bar mouse jit-lock font-lock syntax facemenu font-core
term/tty-colors frame cl-generic cham georgian utf-8-lang misc-lang
vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms cp51932
hebrew greek romanian slovak czech european ethiopic indian cyrillic
chinese composite charscript charprop case-table epa-hook jka-cmpr-hook
help simple abbrev obarray minibuffer cl-preloaded nadvice loaddefs
button faces cus-face macroexp files text-properties overlay sha1 md5
base64 format env code-pages mule custom widget hashtable-print-readable
backquote w32notify dbusbind w32 lcms2 multi-tty make-network-process
emacs)

Memory information:
((conses 16 130359 22263)
 (symbols 56 22153 2)
 (miscs 48 105 257)
 (strings 32 36280 1509)
 (string-bytes 1 943844)
 (vectors 16 23670)
 (vector-slots 8 1322832 187678)
 (floats 8 85 611)
 (intervals 56 4285 3571)
 (buffers 992 18))

[-- Attachment #2: decode-diff-output.patch --]
[-- Type: application/octet-stream, Size: 582 bytes --]

diff --git a/lisp/vc/smerge-mode.el b/lisp/vc/smerge-mode.el
index ea1e0c726f..89af37d637 100644
--- a/lisp/vc/smerge-mode.el
+++ b/lisp/vc/smerge-mode.el
@@ -1084,7 +1084,7 @@ smerge-refine-regions
     ;; Call diff on those files.
     (unwind-protect
         (with-temp-buffer
-          (let ((coding-system-for-read 'emacs-internal))
+          (let ((coding-system-for-read 'emacs-mule))
             (call-process diff-command nil t nil
                           (if (and smerge-refine-ignore-whitespace
                                    (not smerge-refine-weight-hack))

bug#29916: 26.0.90; CRLF in diff-command output breaks smerge hunk header parsing

[Eli Zaretskii]

> From: Richard Copley <rcopley@gmail.com>
> Date: Sun, 31 Dec 2017 18:33:56 +0000
> 
> On Windows, CRLF line endings in the output of diff-command can lead to
> an error in `smerge-refine-regions'. To reproduce, download this patch:
> 
> https://lists.gnu.org/archive/html/emacs-devel/2017-06/txtWF9rI8yqfI.txt
> 
> (It is an example of a perfectly ordinary patch, with Unix line endings.)
> 
> >From 'emacs -Q', visit the patch file, do "M-x diff-mode RET", then
> move point into the second diff hunk (in editfns.c) and type RET
> (diff-goto-source).

You mean M-RET, not RET, right?

> The minibuffer shows this error, with a literal carriage return, shown
> here as ^M:
> 
>   Error running timer: (error "Unexpected patch hunk header: 41a42,121^M")

I cannot reproduce this.  I get "Hunk not yet applied at offset 205
lines" and no error message.  Are your Emacs source files checked out
with CRLF EOL format or something?  If not, where did you get the
ported Diff command?

> As a data point, the error doesn't happen after applying the attached
> patch which partially reverts this commit:

I suggest to use utf-8-emacs instead of emacs-mule (you _really_ don't
want the latter).  I do agree that forcing -unix EOL when decoding the
output of Diff is probably wrong.  Stefan?

bug#29916: 26.0.90; CRLF in diff-command output breaks smerge hunk header parsing

[Richard Copley]

On 31 December 2017 at 19:06, Eli Zaretskii <eliz@gnu.org> wrote:
>> From: Richard Copley <rcopley@gmail.com>
>> Date: Sun, 31 Dec 2017 18:33:56 +0000
>>
>> On Windows, CRLF line endings in the output of diff-command can lead to
>> an error in `smerge-refine-regions'. To reproduce, download this patch:
>>
>> https://lists.gnu.org/archive/html/emacs-devel/2017-06/txtWF9rI8yqfI.txt
>>
>> (It is an example of a perfectly ordinary patch, with Unix line endings.)
>>
>> >From 'emacs -Q', visit the patch file, do "M-x diff-mode RET", then
>> move point into the second diff hunk (in editfns.c) and type RET
>> (diff-goto-source).
>
> You mean M-RET, not RET, right?

No, with that recipe it is bound to o, <mouse-2>, RET, C-c C-c, M-o,
ESC <mouse-2>, M-RET, <menu-bar> <Diff> <Jump to Source>.

>> The minibuffer shows this error, with a literal carriage return, shown
>> here as ^M:
>>
>>   Error running timer: (error "Unexpected patch hunk header: 41a42,121^M")
>
> I cannot reproduce this.  I get "Hunk not yet applied at offset 205
> lines" and no error message.  Are your Emacs source files checked out
> with CRLF EOL format or something?

No.

> If not, where did you get the ported Diff command?

It's the (ubiquitous) gnuwin32 port of diff 2.8.7.

>> As a data point, the error doesn't happen after applying the attached
>> patch which partially reverts this commit:
>
> I suggest to use utf-8-emacs instead of emacs-mule (you _really_ don't
> want the latter).  I do agree that forcing -unix EOL when decoding the
> output of Diff is probably wrong.  Stefan?

Do you mean utf-8, with the end of line conversion left unspecified?
According to the manual (33.10.1), "'emacs-internal’ is an alias for
‘utf-8-emacs’."

bug#29916: 26.0.90; CRLF in diff-command output breaks smerge hunk header parsing

[Eli Zaretskii]

> From: Richard Copley <rcopley@gmail.com>
> Date: Sun, 31 Dec 2017 21:13:21 +0000
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, 29916@debbugs.gnu.org
> 
> >> >From 'emacs -Q', visit the patch file, do "M-x diff-mode RET", then
> >> move point into the second diff hunk (in editfns.c) and type RET
> >> (diff-goto-source).
> >
> > You mean M-RET, not RET, right?
> 
> No, with that recipe it is bound to o, <mouse-2>, RET, C-c C-c, M-o,
> ESC <mouse-2>, M-RET, <menu-bar> <Diff> <Jump to Source>.

Sorry, what recipe?  It's "emacs -Q", isn't it?  After visiting
txtWF9rI8yqfI.txt and typing "M-x diff-mode RET", "C-h c RET" says RET
is bound to 'newline'.  And "C-h w diff-goto-source RET" says

  diff-goto-source is on C-c C-c, M-o, ESC <mouse-2>, M-RET, <menu-bar> <Diff> <Jump to Source>

What am I missing?  Why do we get different results both for the
command binding and for the recipe?

> > If not, where did you get the ported Diff command?
> 
> It's the (ubiquitous) gnuwin32 port of diff 2.8.7.

Same here.

> > I suggest to use utf-8-emacs instead of emacs-mule (you _really_ don't
> > want the latter).  I do agree that forcing -unix EOL when decoding the
> > output of Diff is probably wrong.  Stefan?
> 
> Do you mean utf-8, with the end of line conversion left unspecified?
> According to the manual (33.10.1), "'emacs-internal’ is an alias for
> ‘utf-8-emacs’."

"C-h C emacs-internal RET" says:

  U -- emacs-internal (alias of utf-8-emacs-unix)

  Support for all Emacs characters (including non-Unicode characters).
  Type: utf-8 (UTF-8: Emacs internal multibyte form)
  EOL type: LF

So emacs-internal fixes the EOL format at Unix-style LF.  By contrast,
utf-8-emacs does not:

  U -- utf-8-emacs

  Support for all Emacs characters (including non-Unicode characters).
  Type: utf-8 (UTF-8: Emacs internal multibyte form)
  EOL type: Automatic selection from:
	  [utf-8-emacs-unix utf-8-emacs-dos utf-8-emacs-mac]

bug#29916: 26.0.90; CRLF in diff-command output breaks smerge hunk header parsing

[Richard Copley]

> What am I missing?  Why do we get different results both for the
> command binding and for the recipe?

Read-only mode. For some reason, my copy of the patch file has
the read-only attribute set. Sorry, I hadn't noticed that.

>> > If not, where did you get the ported Diff command?
>>
>> It's the (ubiquitous) gnuwin32 port of diff 2.8.7.
>
> Same here.
>
>> > I suggest to use utf-8-emacs instead of emacs-mule (you _really_ don't
>> > want the latter).  I do agree that forcing -unix EOL when decoding the
>> > output of Diff is probably wrong.  Stefan?
>>
>> Do you mean utf-8, with the end of line conversion left unspecified?
>> According to the manual (33.10.1), "'emacs-internal’ is an alias for
>> ‘utf-8-emacs’."
>
> "C-h C emacs-internal RET" says:
>
>   U -- emacs-internal (alias of utf-8-emacs-unix)
>
>   Support for all Emacs characters (including non-Unicode characters).
>   Type: utf-8 (UTF-8: Emacs internal multibyte form)
>   EOL type: LF
>
> So emacs-internal fixes the EOL format at Unix-style LF.  By contrast,
> utf-8-emacs does not:
>
>   U -- utf-8-emacs
>
>   Support for all Emacs characters (including non-Unicode characters).
>   Type: utf-8 (UTF-8: Emacs internal multibyte form)
>   EOL type: Automatic selection from:
>           [utf-8-emacs-unix utf-8-emacs-dos utf-8-emacs-mac]

OK, thanks. Then I think the sentence I quoted is a documentation bug.

bug#29916: 26.0.90; CRLF in diff-command output breaks smerge hunk header parsing

[Richard Copley]

On 1 January 2018 at 11:02, Richard Copley <rcopley@gmail.com> wrote:
>> What am I missing?  Why do we get different results both for the
>> command binding and for the recipe?
>
> Read-only mode. For some reason, my copy of the patch file has
> the read-only attribute set. Sorry, I hadn't noticed that.

(But that doesn't explain why we're getting different results from
diff-goto-source.)

By the way, I can also trigger the error at the top level (i.e., not inside
a timer callback) by doing "diff-refine-hunk" interactively instead of
"diff-goto-source".

bug#29916: 26.0.90; CRLF in diff-command output breaks smerge hunk header parsing

[Eli Zaretskii]

> From: Richard Copley <rcopley@gmail.com>
> Date: Mon, 1 Jan 2018 11:02:18 +0000
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, 29916@debbugs.gnu.org
> 
> > "C-h C emacs-internal RET" says:
> >
> >   U -- emacs-internal (alias of utf-8-emacs-unix)
> >
> >   Support for all Emacs characters (including non-Unicode characters).
> >   Type: utf-8 (UTF-8: Emacs internal multibyte form)
> >   EOL type: LF
> >
> > So emacs-internal fixes the EOL format at Unix-style LF.  By contrast,
> > utf-8-emacs does not:
> >
> >   U -- utf-8-emacs
> >
> >   Support for all Emacs characters (including non-Unicode characters).
> >   Type: utf-8 (UTF-8: Emacs internal multibyte form)
> >   EOL type: Automatic selection from:
> >           [utf-8-emacs-unix utf-8-emacs-dos utf-8-emacs-mac]
> 
> OK, thanks. Then I think the sentence I quoted is a documentation bug.

Agreed and fixed.

bug#29916: 26.0.90; CRLF in diff-command output breaks smerge hunk header parsing

[Eli Zaretskii]

> From: Richard Copley <rcopley@gmail.com>
> Date: Mon, 1 Jan 2018 11:51:51 +0000
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, 29916@debbugs.gnu.org
> 
> By the way, I can also trigger the error at the top level (i.e., not inside
> a timer callback) by doing "diff-refine-hunk" interactively instead of
> "diff-goto-source".

That does show the error here.

So let's wait for Stefan to chime in, and then use your solution if no
objections are voiced.

Thanks.

bug#29916: 26.0.90; CRLF in diff-command output breaks smerge hunk header parsing

[Stefan Monnier]

> I suggest to use utf-8-emacs instead of emacs-mule (you _really_ don't
> want the latter).  I do agree that forcing -unix EOL when decoding the
> output of Diff is probably wrong.  Stefan?

I guess utf-8-emacs would work as well, yes (I don't think the forcing
of Unix EOL was intended when I changed emacs-mule into emacs-internal).


        Stefan

bug#29916: 26.0.90; CRLF in diff-command output breaks smerge hunk header parsing

[Richard Copley]

On 1 January 2018 at 23:23, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>> I suggest to use utf-8-emacs instead of emacs-mule (you _really_ don't
>> want the latter).  I do agree that forcing -unix EOL when decoding the
>> output of Diff is probably wrong.  Stefan?
>
> I guess utf-8-emacs would work as well, yes (I don't think the forcing
> of Unix EOL was intended when I changed emacs-mule into emacs-internal).

Thanks Stefan,

Eli, will you make the change please? I don't have write access and
I'm not ready to assign copyright. Sorry to be a bother! (Forget about
the patch I attached to the report. It wasn't meant to be a proposed
solution - it was for  a "data point" as I said.)

bug#29916: 26.0.90; CRLF in diff-command output breaks smerge hunk header parsing

[Eli Zaretskii]

> From: Richard Copley <rcopley@gmail.com>
> Date: Tue, 2 Jan 2018 21:41:03 +0000
> Cc: Eli Zaretskii <eliz@gnu.org>, 29916@debbugs.gnu.org
> 
> Eli, will you make the change please?

Will do.

bug#29916: 26.0.90; CRLF in diff-command output breaks smerge hunk header parsing

[Eli Zaretskii]

> Date: Wed, 03 Jan 2018 17:18:08 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: monnier@iro.umontreal.ca, 29916@debbugs.gnu.org
> 
> > From: Richard Copley <rcopley@gmail.com>
> > Date: Tue, 2 Jan 2018 21:41:03 +0000
> > Cc: Eli Zaretskii <eliz@gnu.org>, 29916@debbugs.gnu.org
> > 
> > Eli, will you make the change please?
> 
> Will do.

Done.