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

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

[João Távora]

Hello maintainers,

1. Navigate to a repository where files are tracked with CRLF line
   endings.
   
2. Modify one of those files.

3. Use M-x vc-root-diff. Observe how, in the latest trunk, the carriage
   return (represented as ^M) are visible as a part of the diff hunk's
   content. This is correct, in my opinion, if a little visually
   distracting.

4. Undo of of those diffs with C-c C-a

5. Redo the same hunk with C-c C-a again

6. Notive how diff-apply-hunk incorrectly applies the carriage return
   characters themselves.

Here's a trivial patch to lisp/vc/diff-mode.el, or simply cherry-pick
the commit 

  7ed2b6299425c4b2ea6bd8d8e8eb5eaa5e5ddf7e

which is available in this url

   https://github.com/capitaomorte/emacs.git

 @@ -1799,7 +1799,8 @@ 
       (with-current-buffer buf
 	(goto-char (car pos))
 	(delete-region (car pos) (cdr pos))
-	(insert (car new)))
+	(insert (decode-coding-string (car new)
+	                              buffer-file-coding-system)))
       ;; Display BUF in a window
       (set-window-point (display-buffer buf) (+ (car pos) (cdr new)))
       (diff-hunk-status-msg line-offset (diff-xor switched reverse) nil)


The commit message is

    Consider coding system when applying diff-mode hunks.
     
    Without this, applying hunks may garble line endings in the
    destination buffer.
     
    * lisp/vc/diff-mode.el (diff-apply-hunk): Use decode-coding-string and
      buffer-file-coding-system.
      
Sorry I couldn't properly inline a patch,

Thanks,
João

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

[Stefan Monnier]

> -	(insert (car new)))
> +	(insert (decode-coding-string (car new)
> +	                              buffer-file-coding-system)))

The diff-mode buffer will usually already be decoded (i.e. it contains
chars rather than bytes, and for example your name should appear as
"João" rather than as "Jo<bytes>o").  So calling decode-coding-string on
it unconditionally can't be right.

In some cases, the diff-mode buffer will have undecoded bytes (because
the auto-detection failed, common when the diff is involves various
encodings), in which case decode-coding-string could make sense.
In other cases, only the EOL is faulty (typically because the diff tool
itself output LF while the files contain CRLF), in which case we should
use something else which just strips the CRs.

Of course, in yet other cases, the diff itself adds/removes CRs, in
which case stripping them would be an error.

So, Emacs could/should do something about your use-case, but it has to
be careful first to double-check that it's really your use-case and not
some other case.


        Stefan

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

[Eli Zaretskii]

> From: joaotavora@gmail.com (João Távora)
> Date: Wed, 24 Jun 2015 12:29:05 +0100
> 
>  @@ -1799,7 +1799,8 @@ 
>        (with-current-buffer buf
>  	(goto-char (car pos))
>  	(delete-region (car pos) (cdr pos))
> -	(insert (car new)))
> +	(insert (decode-coding-string (car new)
> +	                              buffer-file-coding-system)))

That doesn't sound right: the string you are passing to
decode-coding-string is already a multibyte string in Emacs's internal
representation.  So decoding it one more time is not what you want.

I think we should simply remove the CR characters from the end of each
line, if buffer-file-coding-system states DOS EOL format.  (You can
check the latter with coding-system-eol-type.)

Thanks.

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

[João Távora]

On Wed, Jun 24, 2015 at 3:57 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: joaotavora@gmail.com (João Távora)
> > Date: Wed, 24 Jun 2015 12:29:05 +0100
> >
> >  @@ -1799,7 +1799,8 @@
> >        (with-current-buffer buf
> >       (goto-char (car pos))
> >       (delete-region (car pos) (cdr pos))
> > -     (insert (car new)))
> > +     (insert (decode-coding-string (car new)
> > +                                   buffer-file-coding-system)))
>
> That doesn't sound right: the string you are passing to
> decode-coding-string is already a multibyte string in Emacs's internal
> representation.  So decoding it one more time is not what you want.
>
> I think we should simply remove the CR characters from the end of each
> line, if buffer-file-coding-system states DOS EOL format.  (You can
> check the latter with coding-system-eol-type.)

I see my mistake now, thanks. (also thanks Stefan). It worked so I
assumed magically it's the right thing without thinking too hard. But
I don't think simply removing the CR characters is the right thing, we
should interpret them according to coding-system-eol-type, right?

It's more complicated, but I can give it a try (maybe meanwhile commit
a patch that does what you say to fix the current unusable state of
diff-apply-hunk in this situation)

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?

João

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

[Eli Zaretskii]

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

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

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

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

[Eli Zaretskii]

> From: joaotavora@gmail.com (João Távora)
> Cc: monnier@iro.umontreal.ca,  20892@debbugs.gnu.org
> Date: Fri, 01 Apr 2016 11:22:50 +0100
> 
> 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?

How come kill-all-local-variables can have any effect on the binding
of coding-system-for-read.  Can you explain, or show the evidence that
vc-setup-buffer is the culprit here?

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

I think that fixing EOL decoding shouldn't touch the value of
coding-system-for-read, only its EOL decoding part, if at all.  And I
also don't see how does binding in vc-git interfere with the EOL
format of the diffs.  Can you tell the details, i.e. how did you
arrive at the conclusion that the above binding is the culprit?

Thanks.

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

[João Távora]

On Apr 1, 2016 21:36, "Eli Zaretskii" <eliz@gnu.org> wrote:
>
> > From: joaotavora@gmail.com (João Távora)
> > Cc: monnier@iro.umontreal.ca,  20892@debbugs.gnu.org
> > Date: Fri, 01 Apr 2016 11:22:50 +0100
> >
> > 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?
>
> How come kill-all-local-variables can have any effect on the binding
> of coding-system-for-read.

I don't know. I said I don't fully understand that part.

> Can you explain, or show the evidence that
> vc-setup-buffer is the culprit here?

I edebugged and evalled coding-system-for-read before and after the
call to vc-setup-buffer. It lost its non-nil value before and after
the call to that vc-setup-buffer.

What may possibly have happened is that when testing I set it globally
and hence violated some important assumption. I am away from the w32
machine where this happened, but I will try again monday.

> I think that fixing EOL decoding shouldn't touch the value of
> coding-system-for-read, only its EOL decoding part, if at all.

Although this bug was issued for the eolness. I've additionally
noticed I get misdecoded diacritics in the vc-diff buffer. This is why
I think coding-system-for-read

The test case is easy I think. Version a file with git, using crlf and latin-1.

$ mkdir something && cd something
$ git init
$ echo João Távora > bla.txt
$ emacs --batch -q bla.txt --eval "(setq buffer-file-coding-system
'iso-latin-1-dos)" --eval "(write-file \"bla.txt\")"
$ git add bla.txt && git commit -m "test"
$ emacs --batch -q bla.txt --eval "(insert \"blibli\\n\")" --eval
"(write-file \"bla.txt\")"

emacs -Q bla.txt
M-x vc-diff

The pesky ^M *and* escape sequences appear in the output. I just tried
this on mac and the same happens. Two things fix this:

1) The second part of my patch, to `vc-git-command'. It seems to
respect the (apparent) fact that coding-system-for-{read,write} are
supposed to be dynamically bindable overrides.
2) file-locally or dir-locally seting vc-git-commits-coding-system in
my configuration.

From reading the code and your messages I was under the impression
that you agreed this was a bug. It is in my opinion (and I also think
it is a regression).

Anyway I think 1) is a sound fix but 2) will also do the job perfectly
well apparently. On monday I will investigate the kill-local-variables
thing again.

Thanks,
João

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

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Fri, 1 Apr 2016 23:51:47 +0100
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, 20892@debbugs.gnu.org
> 
> On Apr 1, 2016 21:36, "Eli Zaretskii" <eliz@gnu.org> wrote:
> >
> > > From: joaotavora@gmail.com (João Távora)
> > > Cc: monnier@iro.umontreal.ca,  20892@debbugs.gnu.org
> > > Date: Fri, 01 Apr 2016 11:22:50 +0100
> > >
> > > 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?
> >
> > How come kill-all-local-variables can have any effect on the binding
> > of coding-system-for-read.
> 
> I don't know. I said I don't fully understand that part.
> 
> > Can you explain, or show the evidence that
> > vc-setup-buffer is the culprit here?
> 
> I edebugged and evalled coding-system-for-read before and after the
> call to vc-setup-buffer. It lost its non-nil value before and after
> the call to that vc-setup-buffer.
> 
> What may possibly have happened is that when testing I set it globally
> and hence violated some important assumption. I am away from the w32
> machine where this happened, but I will try again monday.

I cannot reproduce your result: when I edebug the relevant code,
coding-system-for-read gets bound to undecided-dos before the call to
vc-setup-buffer, and stays at that value after the call returns.

> emacs -Q bla.txt
> M-x vc-diff
> 
> The pesky ^M *and* escape sequences appear in the output. I just tried
> this on mac and the same happens. Two things fix this:
> 
> 1) The second part of my patch, to `vc-git-command'. It seems to
> respect the (apparent) fact that coding-system-for-{read,write} are
> supposed to be dynamically bindable overrides.
> 2) file-locally or dir-locally seting vc-git-commits-coding-system in
> my configuration.
> 
> >From reading the code and your messages I was under the impression
> that you agreed this was a bug. It is in my opinion (and I also think
> it is a regression).
> 
> Anyway I think 1) is a sound fix but 2) will also do the job perfectly
> well apparently. On monday I will investigate the kill-local-variables
> thing again.

Refraining from overriding coding-system-for-read/write if they are
already bound is a Good Thing, so I installed that change in the
emacs-25 branch.

However, re-reading the original bug report, I'm now confused.
Originally, you said that the ^M characters in the vc-diff output were
correct, and your problem was with applying the hunks.  If you now say
that the problem is with those ^M characters, and applying the hunks
after that is no longer a problem, then we can now close this bug.
Please clarify.

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

[João Távora]

On Sat, Apr 2, 2016 at 10:31 AM, Eli Zaretskii <eliz@gnu.org> wrote:

> Refraining from overriding coding-system-for-read/write if they are
> already bound is a Good Thing, so I installed that change in the
> emacs-25 branch.

Thanks.

> However, re-reading the original bug report, I'm now confused.
> Originally, you said that the ^M characters in the vc-diff output were
> correct, and your problem was with applying the hunks.  If you now say
> that the problem is with those ^M characters, and applying the hunks
> after that is no longer a problem, then we can now close this bug.
> Please clarify.

Indeed, it is a bit confusing, so here's the story:

1. Sometime in June 2015, upgrade my emacs. vc-diff some CRLF file

2. "Cool, a new feature now makes it show precisely which chars were
   added and deleted even the CRs"

3. "Oh, but vc-apply-hunk stopped working, let's open bug 20892"

4. Discuss bug, argue that showing ^M is cool for when you really want
   to apply patches that add and remove just one or two of those.

5. Apply some reasonable fix to my config. Be side-tracked by
   dayjob. Lose interest in discussing the bug,

6. Eventually, realize 4 is silly and if you're mixing CRLF and
   non-CRLF line endings you've already lost.

7. Recently, March 2016, also realize that diacritics are also wrong
   in latin-1 files.

8. Decide to reevaluate situation, read some code and commit messages,
   and propose a new fix that makes the vc-diff buffer have the same
   encoding as the originating file, as apparently was intended.
   vc-apply-hunk is fixed, if I ever need to do something like 4
   I'll do it outside of emacs.

I think you can close the bug.

-- 
João Távora

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

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Sat, 2 Apr 2016 14:42:48 +0100
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, 20892@debbugs.gnu.org
> 
> 1. Sometime in June 2015, upgrade my emacs. vc-diff some CRLF file
> 
> 2. "Cool, a new feature now makes it show precisely which chars were
>    added and deleted even the CRs"
> 
> 3. "Oh, but vc-apply-hunk stopped working, let's open bug 20892"
> 
> 4. Discuss bug, argue that showing ^M is cool for when you really want
>    to apply patches that add and remove just one or two of those.
> 
> 5. Apply some reasonable fix to my config. Be side-tracked by
>    dayjob. Lose interest in discussing the bug,
> 
> 6. Eventually, realize 4 is silly and if you're mixing CRLF and
>    non-CRLF line endings you've already lost.
> 
> 7. Recently, March 2016, also realize that diacritics are also wrong
>    in latin-1 files.
> 
> 8. Decide to reevaluate situation, read some code and commit messages,
>    and propose a new fix that makes the vc-diff buffer have the same
>    encoding as the originating file, as apparently was intended.
>    vc-apply-hunk is fixed, if I ever need to do something like 4
>    I'll do it outside of emacs.
> 
> I think you can close the bug.

Thanks, closing.