bug#20892: 25.0.50; Applying vc-diff hunks on CRLF tracked files

[joaotavora@gmail.com (João Távora)]

Hi,

So I'm reviving this ancient thread, since I've possibly come up with a
more interesting patch.

In lisp/vc/vc.el, in vc-diff-internal, dynamically binding
`coding-system-for-read' seems to be defeated by a call to
`vc-setup-buffer', which in turn kills all local variables. 

I don't fully understand the interaction between buffer-local and
lexically/dinamically bound variables but this seems wrong, right?

Anyway, I believe the `vc-setup-buffer' can be pulled up to outside the
let.

    diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
    index 25b41e3..c245c3f 100644
    --- a/lisp/vc/vc.el
    +++ b/lisp/vc/vc.el
    @@ -1678,6 +1678,7 @@ BUFFER, if non-nil, should be a buffer or a buffer name.
     Return t if the buffer had changes, nil otherwise."
       (unless buffer
         (setq buffer "*vc-diff*"))
    +  (vc-setup-buffer buffer)
       (let* ((files (cadr vc-fileset))
             (messages (cons (format "Finding changes in %s..."
                                      (vc-delistify files))
    @@ -1696,7 +1697,6 @@ Return t if the buffer had changes, nil otherwise."
            (setq coding-system-for-read
                  (coding-system-change-eol-conversion coding-system-for-read
                                                       'dos)))
    -    (vc-setup-buffer buffer)
         (message "%s" (car messages))
         ;; Many backends don't handle well the case of a file that has been
         ;; added but not yet committed to the repo (notably CVS and Subversion).

If, to this, we add a fix in lisp/vc/vc-git.el and don't let it override
an existing `coding-system-for-read'...

    diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
    index 8498cc8..c60125c 100644
    --- a/lisp/vc/vc-git.el
    +++ b/lisp/vc/vc-git.el
    @@ -1387,8 +1387,10 @@ This command shares argument histories with \\[rgrep] and \\[grep]."
       "A wrapper around `vc-do-command' for use in vc-git.el.
     The difference to vc-do-command is that this function always invokes
     `vc-git-program'."
    -  (let ((coding-system-for-read vc-git-commits-coding-system)
    -       (coding-system-for-write vc-git-commits-coding-system))
    +  (let ((coding-system-for-read (or coding-system-for-read
    +                                    vc-git-commits-coding-system))
    +       (coding-system-for-write (or coding-system-for-write
    +                                     vc-git-commits-coding-system)))
         (apply 'vc-do-command (or buffer "*vc*") okstatus vc-git-program
               ;; http://debbugs.gnu.org/16897
               (unless (and (not (cdr-safe file-or-list))

the system seems to do the right thing and honour the intention of

   commit 0e2c793ffefa72c40c7731847d8210c2d7d0e515
   Author: Eli Zaretskii <eliz@gnu.org>
   Date:   Tue Nov 26 21:17:55 2013 +0200
    
       Fix ugly ^M characters in Diff output shown by "C-x v u".

What do you think? 

João





Eli Zaretskii <eliz@gnu.org> writes:

>> From: João Távora <joaotavora@gmail.com>
>> Date: Thu, 25 Jun 2015 14:54:26 +0100
>> Cc: 20892@debbugs.gnu.org
>> 
>> I don't think simply removing the CR characters is the right thing, we
>> should interpret them according to coding-system-eol-type, right?
>
> You should remove them if coding-system-eol-type for
> buffer-file-coding-system of the buffer which the hunk comes from
> returns 1.
>
>> So, say a diff hunk contains just a couple of lines with CRLF and a
>> couple of lines just LF. If applied to a buffer that is all CRLF, I
>> would say that applying this hunk should eventually provoke the normal
>> Emacs behaviour of highlighting all CR's except for the two lines with
>> LF. Is it worth it? What do you both think?
>
> It depends on what the user wants/needs.  The Patch utility has
> options which will cause it to strip the CR characters when applying
> the diffs; I guess we should have a similar flexibility here.