bug#17388: 24.4.50; REGRESSION: Ediff - 1) wrong face, 2) incorrect diffing

bug#17388: 24.4.50; REGRESSION: Ediff - 1) wrong face, 2) incorrect diffing

[Drew Adams]

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

1. A diff that is not the current one should use face
`ediff-(odd|even)-diff-(A|B').  This is now broken.

It does use that face for the main diff, but it still tries to show fine
differences within that diff - which is new.  That would be OK, I guess,
but it should not use face `default' to show such fine diffs.  It should
use an Ediff face, which users can customize without affecting other
things (as happens with `default').

Using face `default' here is particular misguided, as it suggests that
there is no difference at those locations, whereas there likely is.

See the attached screenshot.  The first diff shown, with the gray
highlighting, shows the regression.  


2. The fine diffs are also not correct.  See the same screenshot.  For
the first diff, "advertise" and "d-signature-table" should be
highlighted the same as "(defvar " and ")", and for the second diff,
"fil" and "s-alist" should be highlighted the same as "(defvar " and
")".  These are not differences.

I am using Cygwin `diff', which could affect #2 presumably.  But #2 is
still a regression wrt prior Emacs versions.  With Emacs 24.3, for
instance (using the same Cygwin `diff'), there is no fine diff shown
here.  Instead, the diff, which is shown only as a main diff, is between
"file-local-variables-alist" and "filxxxxxxxxxxxxxxxxxxxxxs-alist".
Which is correct.

Emacs should not show fine diffs that are are patently wrong.
Better not to mislead.



In GNU Emacs 24.4.50.1 (i686-pc-mingw32)
 of 2014-04-29 on ODIEONE
Bzr revision: 117031 monnier@iro.umontreal.ca-20140429151607-qnkgbymwfaj5ut08
Windowing system distributor `Microsoft Corp.', version 6.1.7601
Configured using:
 `configure --prefix=/c/Devel/emacs/snapshot/trunk
 --enable-checking=yes,glyphs 'CFLAGS=-O0 -g3'
 LDFLAGS=-Lc:/Devel/emacs/lib 'CPPFLAGS=-DGC_MCHECK=1
 -Ic:/Devel/emacs/include''

[-- Attachment #2: throw-emacs-bug-ediff.png --]
[-- Type: image/png, Size: 19965 bytes --]

bug#17388: 24.4.50; REGRESSION: Ediff - 1) wrong face, 2) incorrect diffing

[Stephen Berman]

On Fri, 2 May 2014 08:15:09 -0700 (PDT) Drew Adams <drew.adams@oracle.com> wrote:

> 1. A diff that is not the current one should use face
> `ediff-(odd|even)-diff-(A|B').  This is now broken.
>
> It does use that face for the main diff, but it still tries to show fine
> differences within that diff - which is new.  That would be OK, I guess,
> but it should not use face `default' to show such fine diffs.  It should
> use an Ediff face, which users can customize without affecting other
> things (as happens with `default').
>
> Using face `default' here is particular misguided, as it suggests that
> there is no difference at those locations, whereas there likely is.
>
> See the attached screenshot.  The first diff shown, with the gray
> highlighting, shows the regression.  
>
>
> 2. The fine diffs are also not correct.  See the same screenshot.  For
> the first diff, "advertise" and "d-signature-table" should be
> highlighted the same as "(defvar " and ")", and for the second diff,
> "fil" and "s-alist" should be highlighted the same as "(defvar " and
> ")".  These are not differences.
>
> I am using Cygwin `diff', which could affect #2 presumably.  But #2 is
> still a regression wrt prior Emacs versions.  With Emacs 24.3, for
> instance (using the same Cygwin `diff'), there is no fine diff shown
> here.  Instead, the diff, which is shown only as a main diff, is between
> "file-local-variables-alist" and "filxxxxxxxxxxxxxxxxxxxxxs-alist".
> Which is correct.
>
> Emacs should not show fine diffs that are are patently wrong.
> Better not to mislead.
>
>
>
> In GNU Emacs 24.4.50.1 (i686-pc-mingw32)
>  of 2014-04-29 on ODIEONE
> Bzr revision: 117031 monnier@iro.umontreal.ca-20140429151607-qnkgbymwfaj5ut08
> Windowing system distributor `Microsoft Corp.', version 6.1.7601
> Configured using:
>  `configure --prefix=/c/Devel/emacs/snapshot/trunk
>  --enable-checking=yes,glyphs 'CFLAGS=-O0 -g3'
>  LDFLAGS=-Lc:/Devel/emacs/lib 'CPPFLAGS=-DGC_MCHECK=1
>  -Ic:/Devel/emacs/include''

I see both of these problematic highlightings on GNU/Linux builds from
both the trunk (bzr 117042) and the emacs-24 branch (bzr 117049).

Steve Berman

bug#17388: 24.4.50; REGRESSION: Ediff - 1) wrong face, 2) incorrect diffing

[Stefan Monnier]

>> See the attached screenshot.  The first diff shown, with the gray
>> highlighting, shows the regression.  

I installed the patch below which should fix those problems, thanks.

>> 2. The fine diffs are also not correct.  See the same screenshot.  For
>> the first diff, "advertise" and "d-signature-table" should be
>> highlighted the same as "(defvar " and ")", and for the second diff,
>> "fil" and "s-alist" should be highlighted the same as "(defvar " and
>> ")".  These are not differences.

You mean you want finer granularity of fine diffs.

>> still a regression wrt prior Emacs versions.  With Emacs 24.3, for
>> instance (using the same Cygwin `diff'), there is no fine diff shown
>> here.  Instead, the diff, which is shown only as a main diff, is between
>> "file-local-variables-alist" and "filxxxxxxxxxxxxxxxxxxxxxs-alist".
>> Which is correct.

I don't see that here with Debian's Emacs-24.3, and neither with 23.4.
The behavior you describe seems to correspond to ediff-word-mode, IIRC,
so I assume this is not really a bug/regression but just a pilot error
on your part.  If not, feel free to re-open this bug report, providing
more details about the problem.


        Stefan


=== modified file 'lisp/ChangeLog'
--- lisp/ChangeLog	2014-05-02 11:11:35 +0000
+++ lisp/ChangeLog	2014-05-03 02:10:48 +0000
@@ -1,3 +1,13 @@
+2014-05-03  Stefan Monnier  <monnier@iro.umontreal.ca>
+
+	* vc/ediff-diff.el (ediff-set-fine-diff-properties-in-one-buffer):
+	Use nil rather than `default' for the "default" appearance (bug#17388).
+	* vc/ediff-util.el (ediff-inferior-compare-regions)
+	(ediff-toggle-autorefine, ediff-unselect-difference): Don't use
+	a misleading `default' value when it's really a boolean.
+	* vc/ediff-init.el (ediff-set-overlay-face): Don't set help-echo if the
+	overlay is not visible.
+
 2014-05-02  Leo Liu  <sdl.web@gmail.com>
 
 	* emacs-lisp/cl-macs.el (cl-deftype): Fix indentation.

=== modified file 'lisp/vc/ediff-diff.el'
--- lisp/vc/ediff-diff.el	2014-04-14 02:21:12 +0000
+++ lisp/vc/ediff-diff.el	2014-05-03 01:49:01 +0000
@@ -818,10 +818,9 @@
 						     n &optional default)
   (let ((fine-diff-vector  (ediff-get-fine-diff-vector n buf-type))
 	(face (if default
-		  'default
+		  nil
 		(ediff-get-symbol-from-alist
-		 buf-type ediff-fine-diff-face-alist)
-		)))
+		 buf-type ediff-fine-diff-face-alist))))
     (mapc (lambda (overl)
 	    (ediff-set-overlay-face overl face))
 	  fine-diff-vector)))

=== modified file 'lisp/vc/ediff-init.el'
--- lisp/vc/ediff-init.el	2014-04-10 19:15:01 +0000
+++ lisp/vc/ediff-init.el	2014-05-03 02:05:06 +0000
@@ -807,7 +807,7 @@
 
 (defun ediff-set-overlay-face (extent face)
   (ediff-overlay-put extent 'face face)
-  (ediff-overlay-put extent 'help-echo 'ediff-region-help-echo))
+  (ediff-overlay-put extent 'help-echo (if face 'ediff-region-help-echo)))
 
 (defun ediff-region-help-echo (extent-or-window &optional overlay _point)
   (unless overlay

=== modified file 'lisp/vc/ediff-util.el'
--- lisp/vc/ediff-util.el	2014-04-10 19:15:01 +0000
+++ lisp/vc/ediff-util.el	2014-05-03 01:46:35 +0000
@@ -958,7 +958,7 @@
 	 (message "Auto-refining is OFF")
 	 (setq ediff-auto-refine 'off))
 	(t ;; nix 'em
-	 (ediff-set-fine-diff-properties ediff-current-difference 'default)
+	 (ediff-set-fine-diff-properties ediff-current-difference t)
 	 (message "Refinements are HIDDEN")
 	 (setq ediff-auto-refine 'nix))
 	))
@@ -2973,7 +2973,7 @@
 	       ))
 
 	;; unhighlight fine diffs
-	(ediff-set-fine-diff-properties ediff-current-difference 'default)
+	(ediff-set-fine-diff-properties ediff-current-difference t)
 	(run-hooks 'ediff-unselect-hook))))
 
 
@@ -3492,7 +3492,7 @@
 
     (if (ediff-valid-difference-p ediff-current-difference)
 	(progn
-	  (ediff-set-fine-diff-properties ediff-current-difference 'default)
+	  (ediff-set-fine-diff-properties ediff-current-difference t)
 	  (ediff-unhighlight-diff)))
     (ediff-paint-background-regions 'unhighlight)
 

bug#17388: 24.4.50; REGRESSION: Ediff - 1) wrong face, 2) incorrect diffing

[Drew Adams]

> >> See the attached screenshot.  The first diff shown, with the gray
> >> highlighting, shows the regression.
> 
> I installed the patch below which should fix those problems, thanks.
> 
> >> 2. The fine diffs are also not correct.  See the same screenshot.  For
> >> the first diff, "advertise" and "d-signature-table" should be
> >> highlighted the same as "(defvar " and ")", and for the second diff,
> >> "fil" and "s-alist" should be highlighted the same as "(defvar " and
> >> ")".  These are not differences.
> 
> You mean you want finer granularity of fine diffs.

Not at all.  How do you get that from what I said?

There should be EITHER, (a) as previously, NO fine diffs shown for
other than the current diff OR (b) CORRECT (helpful) fine diffs
shown for the non-current diffs.

In the text you quoted I spoke only of (a): return to the previous
behavior of NOT showing fine diffs except for the current diff.

> >> still a regression wrt prior Emacs versions.  With Emacs 24.3, for
> >> instance (using the same Cygwin `diff'), there is no fine diff shown
> >> here.  Instead, the diff, which is shown only as a main diff, is between
> >> "file-local-variables-alist" and "filxxxxxxxxxxxxxxxxxxxxxs-alist".
> >> Which is correct.
> 
> I don't see that here with Debian's Emacs-24.3, and neither with 23.4.

It's not very clear from the above what you see and do not see,
especially since you apparently did not understand the description.

Do you see a bug for Emacs 24.3 or 23.4?  There is none that I see -
the behavior is as I described it.  No fine diffs are shown for the
non-current diff highlighting with Emacs 24.3 or 23.4.

Are you sure you are looking at the non-current diffs (the gray ones)?
That's where both bugs are: (1) wrong face (`default'), (2) incorrect
fine diffing.

I just repeated everything, again starting from emacs -Q with the
above recipe, using 24.3.  That's 5 times now I've done it (2x for
24.4, 24.3, 1x for 23.4).  Same thing - just what I described originally.

> The behavior you describe seems to correspond to ediff-word-mode, IIRC,
> so I assume this is not really a bug/regression but just a pilot error
> on your part.  If not, feel free to re-open this bug report, providing
> more details about the problem.

I can no longer reopen bugs - Glenn apparently banned me some time ago.

This has nothing to do with `ediff-word-mode' or pilot error.
What I showed and described was already from emacs -Q.  And S. Berman
had no trouble following the recipe and confirming the behavior.
But let me try again.

emacs -Q

Load file `cygwin-mount.el', then `setup-cygwin.el' (from Emacs Wiki).
Cygwin `diff' is apparently irrelevant, but that's what I used anyway.

In foo.el type some text. E.g.:

(defvar advertised-signature-table)
(defvar dir-local-variables-alist)
(defvar dir-locals-file)
(defvar file-local-variables-alist)
(defvar Info-indexed-nodes)

Put the same text in bar.el.  Then modify it a bit in bar.el:

(defvar advertiseyyyyyyyyyyyd-signature-table)
(defvar dir-local-variables-alist)
(defvar dir-locals-file)
(defvar filxxxxxxxxxxxxxxs-alist)
(defvar Info-indexed-nodes)

M-x ediff-buffers ; for foo.el and bar.el

Cycle among the two diffs.  You will see the screenshot I sent.
Now read the bug report as to what is wrong with what you see,
if it is still not clear why it is a bug (2 bugs).

Stephen Berman's confirmation indicates that Cygwin `diff' is
irrelevant:

> I see both of these problematic highlightings on GNU/Linux builds from
> both the trunk (bzr 117042) and the emacs-24 branch (bzr 117049).

bug#17388: 24.4.50; REGRESSION: Ediff - 1) wrong face, 2) incorrect diffing

[Eli Zaretskii]

> Date: Fri, 2 May 2014 20:11:54 -0700 (PDT)
> From: Drew Adams <drew.adams@oracle.com>
> Cc: 17388-done@debbugs.gnu.org, Michael Kifer <kifer@cs.stonybrook.edu>
> 
> There should be EITHER, (a) as previously, NO fine diffs shown for
> other than the current diff OR (b) CORRECT (helpful) fine diffs
> shown for the non-current diffs.

Ediff's "fine diffs" are word-granular.  That is, Ediff breaks each
line into "words", then passes the result to the Diff program for
comparisoon, and reflects the results with different faces.  AFAIR,
this has always been that way.

The Ediff manual says:

  `ediff-forward-word-function'
       This variable controls how fine differences are computed.  The
       value must be a Lisp function that determines how the current
       difference region should be split into words.

       Fine differences are computed by first splitting the current
       difference region into words and then passing the result to
       `ediff-diff-program'.  For the default forward word function
       (which is `ediff-forward-word'), a word is a string consisting of
       letters, `-', or `_'; a string of punctuation symbols; a string of
       digits, or a string consisting of symbols that are neither space,
       nor a letter.

       This default behavior is controlled by four variables:
       `ediff-word-1', ..., `ediff-word-4'.  See the on-line
       documentation for these variables and for the function
       `ediff-forward-word' for an explanation of how to modify these
       variables.

I think what you describe in your item #2 is exactly the above
behavior.  (The problem described under #1 is now fixed by Stefan.)

> Stephen Berman's confirmation indicates that Cygwin `diff' is
> irrelevant:
> 
> > I see both of these problematic highlightings on GNU/Linux builds from
> > both the trunk (bzr 117042) and the emacs-24 branch (bzr 117049).

I can confirm that too, but (a) I don't think the 2nd issue
constitutes a "problem" (see above), and (b) it is definitely not a
"REGRESSION", because older Emacsen behaved the same wrt fine diffs
inside a line.

bug#17388: 24.4.50; REGRESSION: Ediff - 1) wrong face, 2) incorrect diffing

[Drew Adams]

> > There should be EITHER, (a) as previously, NO fine diffs shown for
> > other than the current diff OR (b) CORRECT (helpful) fine diffs
> > shown for the non-current diffs.
> 
> Ediff's "fine diffs" are word-granular.  That is, Ediff breaks each
> line into "words", then passes the result to the Diff program for
> comparisoon, and reflects the results with different faces.  AFAIR,
> this has always been that way.

OK, so you are saying that Emacs has silently changed to (b) from (a),
and the way it does fine diffs corresponds to what is shown.  So be it.

> > > I see both of these problematic highlightings on GNU/Linux builds from
> > > both the trunk (bzr 117042) and the emacs-24 branch (bzr 117049).
> 
> I can confirm that too, but (a) I don't think the 2nd issue
> constitutes a "problem" (see above), and (b) it is definitely not a
> "REGRESSION", because older Emacsen behaved the same wrt fine diffs
> inside a line.

It is a change in behavior wrt older Emacsen, which do not show fine
diffs within the non-current diffs.  Regression or improvement - we
can have different opinions.  (BTW, I see nothing in NEWS about this
behavior change.)

But more importantly, "REGRESSION" in the subject line is for the bug
report, and #1 is the more serious part: removing diff highlighting
from part of a diff gives the impression that that unhighlighted text
is not different.

bug#17388: 24.4.50; REGRESSION: Ediff - 1) wrong face, 2) incorrect diffing

[Michael Kifer]

[-- Attachment #1: Type: text/html, Size: 2740 bytes --]

bug#17388: 24.4.50; REGRESSION: Ediff - 1) wrong face, 2) incorrect diffing

[Eli Zaretskii]

> Date: Sat, 3 May 2014 07:01:13 -0700 (PDT)
> From: Drew Adams <drew.adams@oracle.com>
> Cc: monnier@iro.umontreal.ca, 17388@debbugs.gnu.org, kifer@cs.stonybrook.edu
> 
> > > There should be EITHER, (a) as previously, NO fine diffs shown for
> > > other than the current diff OR (b) CORRECT (helpful) fine diffs
> > > shown for the non-current diffs.
> > 
> > Ediff's "fine diffs" are word-granular.  That is, Ediff breaks each
> > line into "words", then passes the result to the Diff program for
> > comparisoon, and reflects the results with different faces.  AFAIR,
> > this has always been that way.
> 
> OK, so you are saying that Emacs has silently changed to (b) from (a),
> and the way it does fine diffs corresponds to what is shown.

Yes.  But Stefan now changed it back.

Therefore, I was talking only about the 2nd part of your report, which
complains that the fine diffs are incorrect.

> > > > I see both of these problematic highlightings on GNU/Linux builds from
> > > > both the trunk (bzr 117042) and the emacs-24 branch (bzr 117049).
> > 
> > I can confirm that too, but (a) I don't think the 2nd issue
> > constitutes a "problem" (see above), and (b) it is definitely not a
> > "REGRESSION", because older Emacsen behaved the same wrt fine diffs
> > inside a line.
> 
> It is a change in behavior wrt older Emacsen, which do not show fine
> diffs within the non-current diffs.

Again, that part is now gone; Emacs behaves like before: it shows fine
diffs only in the current hnunk.

> But more importantly, "REGRESSION" in the subject line is for the bug
> report, and #1 is the more serious part: removing diff highlighting
> from part of a diff gives the impression that that unhighlighted text
> is not different.

#1 is solved; do you agree that #2 is not a bug, but the intended
behavior that was always there?

bug#17388: 24.4.50; REGRESSION: Ediff - 1) wrong face, 2) incorrect diffing

[Drew Adams]

>>>> There should be EITHER, (a) as previously, NO fine diffs shown for
>>>> other than the current diff OR (b) CORRECT (helpful) fine diffs
>>>> shown for the non-current diffs.
>>> 
>>> Ediff's "fine diffs" are word-granular.  That is, Ediff breaks each
>>> line into "words", then passes the result to the Diff program for
>>> comparisoon, and reflects the results with different faces.  AFAIR,
>>> this has always been that way.
>> 
>> OK, so you are saying that Emacs has silently changed to (b) from (a),
>> and the way it does fine diffs corresponds to what is shown.  So be it.
> 
> Ediff never showed fine diffs for any region but the current one. 

Precisely.  To (b) from (a).  Emacs never showed fine diffs for
non-current diffs before.

It's OK with me if it does so now.  I thought that the inaccurate
fine diffs shown for this were new and particular to this non-current
diff highlighting.  I was corrected: apparently fine diffs are this
inaccurate in general.

bug#17388: 24.4.50; REGRESSION: Ediff - 1) wrong face, 2) incorrect diffing

[Drew Adams]

> > > > There should be EITHER, (a) as previously, NO fine diffs shown for
> > > > other than the current diff OR (b) CORRECT (helpful) fine diffs
> > > > shown for the non-current diffs.
> > >
> > > Ediff's "fine diffs" are word-granular.  That is, Ediff breaks each
> > > line into "words", then passes the result to the Diff program for
> > > comparisoon, and reflects the results with different faces.  AFAIR,
> > > this has always been that way.
> >
> > OK, so you are saying that Emacs has silently changed to (b) from (a),
> > and the way it does fine diffs corresponds to what is shown.
> 
> Yes.  But Stefan now changed it back.

He did?  I thought he fixed only #1: the use of face `default'.
I didn't think that he also got rid of the new fine-diffing for
non-current diffs.  If he did, then both #1 and #2 should presumably
be fixed.

> Therefore, I was talking only about the 2nd part of your report, which
> complains that the fine diffs are incorrect.

If Stefan got rid of the change to fine-diffing for non-current diffs
then it doesn't matter whether that fine-diffing was inaccurate.

> > It is a change in behavior wrt older Emacsen, which do not show fine
> > diffs within the non-current diffs.
> 
> Again, that part is now gone; Emacs behaves like before: it shows fine
> diffs only in the current hnunk.

OK, if you say so.  Great.  I didn't notice that in the patch he sent.

> > But more importantly, "REGRESSION" in the subject line is for the bug
> > report, and #1 is the more serious part: removing diff highlighting
> > from part of a diff gives the impression that that unhighlighted text
> > is not different.
> 
> #1 is solved; do you agree that #2 is not a bug, but the intended
> behavior that was always there?

#2 was that non-current diffs were being fine-diffed, and that
fine-diffing was inaccurate.  It was explained to me that fine-diffing
is inaccurate in this way generally.  IOW, this has nothing to do with
the fact that they were now applied to non-current diffs.

If fine-diffing is inaccurate in general (call it word-diffing or
whatever, if that helps), then so be it.  I am OK with #1 being fixed.

I am OK with #2 also being reverted to not showing fine diffs for
non-current.  I am OK also with fine diffs being shown for non-current
(given that #1 is fixed, so they are not shown with face `default').

I understand now that the inaccuracy of fine diffs that I pointed out
is apparently general and not something new for only non-current fine
diffing.

bug#17388: 24.4.50; REGRESSION: Ediff - 1) wrong face, 2) incorrect diffing

[Stefan Monnier]

> Precisely.  To (b) from (a).  Emacs never showed fine diffs for
> non-current diffs before.

For fucking christ's sake, Drew.  Can you fucking try the patch before
assuming people didn't understand you and before complaining any further?


        Stefan

bug#17388: 24.4.50; REGRESSION: Ediff - 1) wrong face, 2) incorrect diffing

[Drew Adams]

> For fucking christ's sake, Drew.  Can you fucking try the patch before
> assuming people didn't understand you and before complaining any further?
> 
>         Stefan

No idea what that rant is all about.  I was not complaining at all.
Thanks for fixing the bug, and I hope you are feeling better now.