Bugfix for ediff, two patches

Bugfix for ediff, two patches

[Tom Breton (Tehom)]

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

**** Explanation of the bug:

When told to "Compare currently highlighted difference regions",
ediff-inferior-compare-regions does *not* use the highlighted region as it
thinks it does.  It uses some other region that has nothing to do with
that.  This effectively defeats use of ediff for merging in git.

**** Instructions for reproducing it

I have attached files the demonstrate the bug.
 * Merge file.a.txt with file.b.txt using ancestor file.c.txt
   (Suggestion: use the code immediately below)
 * "n" to go to first clash
 * "a" to partly merge - just so it's merging nicely and not seeing
   "<<<<<<" ">>>>>>" "#####Ancestor" etc
 * "=" to start an inferior merge
 * "b" to compare to buffer B
 * Notice that ediff did not use the highlighted region like it said
   it would, but pulled some other text from B and Merge.

Here's code I found useful in seeing this bug and the fix for it, so I'm
including it here.  It just starts an merge-with-ancestor with the
respective files - saves time.  It works with the filenames I used for the
attachments that show the bug.

  (defun bug-ediff-merge-files-with-ancestor (dir)
     ""

     (interactive "DDirectory: ")
     (ediff-merge-files-with-ancestor
        (expand-file-name "file1.a.txt" dir)
        (expand-file-name "file1.b.txt" dir)
        (expand-file-name "file1.c.txt" dir)))

I also found it useful to give each set of {a,b,c} its own directory and
keep the filenames the same across directories.  I can't attach them to
this email with directory names, though.

**** Final diagnosis:

ediff-inferior-compare-regions was passing the wrong symbol to
ediff-clone-buffer-for-current-diff-comparison.  It assumed it was always
first 'A, then 'B.  This was accidentally correct for inferior comparisons
of 2-way comparisons, but wrong for inferior comparisons of other
comparisons.

**** Bugfix: Patch attached

I'm attaching a patch for this.

ediff-inferior-compare-regions has 3 branches for types of comparison that
it supports.  This fix seems to apply straightforwardly to all 3.  I have
exercised the fix in the "merge with ancestor" and "2-way" branches, but
not in the "3 way comparison" branch because I don't use it so I don't
have reasonable examples to try it on.

**** Related comments, for what they are worth:

 * There at least remain two bugs in ediff inferior comparisons, both much
less serious:

   * Wordwise merging doesn't handle whitespace reasonably when merging to
a blank element.  It just plops the non-blank element at the end of
whatever whitespace it ends up with, even across line breaks.  IMO it
would be more correct to infer the division of whitespace from the
whitespace around the non-blank element.  I've appended files that show
this behavior: The odd-whitespace and odd-whitespace-2 files, six in
all.

     It may not be worth the effort of coding it, though.  I don't plan to
fix this one.

   * Possibly related: Inferior comparisons wrongly include the line after
the comparison region.

     ediff-inferior-compare-regions deliberately gets whole lines.  Ie, if
comparison's end is somehow in the middle of a line,
ediff-inferior-compare-regions will advance it to the beginning of
the next line.  But it always assumes comparison's end is in the
middle of a line, so it always advances one line.

     I've attached a patch for this one too.

 * Some code in ediff could really use factoring.

**** Purpose of attachments

  * ediff-util.el.diff :: patch for main bug, from ediff-util.old.el to
ediff-util.fix.el

  * ediff-util.el.fix-line-bug.diff :: patch for extra-line bug, from
ediff-util.fix.el to ediff-util.fix2.el

       This patch is on top of the previous patch, because ISTM there's
little point in it if the more serious bug is not fixed.

  * Demo: the main bug, 3 files:

    * file1.{abc}.txt

  * Demo: the other behavior I think is wrong, 6 files:

    * odd-whitespace-2-file1.{abc}.txt

    * odd-whitespace-file1.{abc}.txt


       Tom Breton (Tehom)

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: ediff-util.el.fix.diff --]
[-- Type: text/x-patch; name="ediff-util.el.fix.diff", Size: 3266 bytes --]

diff -c -b /home/tehom/.emacs.d/ediff/ediff-util.old.el /home/tehom/.emacs.d/ediff/ediff-util.fix.el
*** /home/tehom/.emacs.d/ediff/ediff-util.old.el	Mon Nov 15 22:33:34 2010
--- /home/tehom/.emacs.d/ediff/ediff-util.fix.el	Wed Nov 17 13:47:53 2010
***************
*** 3477,3483 ****
  	(possibilities (list ?A ?B ?C))
  	(zmacs-regions t)
  	use-current-diff-p
! 	begA begB endA endB bufA bufB)
  
      (if (ediff-valid-difference-p ediff-current-difference)
  	(progn
--- 3477,3483 ----
  	(possibilities (list ?A ?B ?C))
  	(zmacs-regions t)
  	use-current-diff-p
! 	begA begB endA endB bufA bufB symA symB)
  
      (if (ediff-valid-difference-p ediff-current-difference)
  	(progn
***************
*** 3487,3499 ****
--- 3487,3502 ----
  
      (cond ((ediff-merge-job)
  	   (setq bufB ediff-buffer-C)
+ 	   (setq symB 'C)
  	   ;; ask which buffer to compare to the merge buffer
  	   (while (cond ((eq answer ?A)
  			 (setq bufA ediff-buffer-A
+ 			       symA 'A
  			       possibilities '(?B))
  			 nil)
  			((eq answer ?B)
  			 (setq bufA ediff-buffer-B
+ 			       symA 'B
  			       possibilities '(?A))
  			 nil)
  			((equal answer ""))
***************
*** 3514,3519 ****
--- 3517,3523 ----
  			       (eval
  				(ediff-get-symbol-from-alist
  				 answer ediff-buffer-alist)))
+ 			 (setq symA (intern (string answer)))
  			 nil)
  			((equal answer ""))
  			(t (beep 1)
***************
*** 3533,3538 ****
--- 3537,3544 ----
  			       (eval
  				(ediff-get-symbol-from-alist
  				 answer ediff-buffer-alist)))
+ 			 (setq symB (intern (string answer)))
+ 			   
  			 nil)
  			((equal answer ""))
  			(t (beep 1)
***************
*** 3548,3553 ****
--- 3554,3561 ----
  	  (t ; 2way comparison
  	   (setq bufA ediff-buffer-A
  		 bufB ediff-buffer-B
+ 	         symA 'A
+ 	         symB 'B
  		 possibilities nil)))
  
      (if (and (ediff-valid-difference-p ediff-current-difference)
***************
*** 3556,3562 ****
  
      (setq bufA (if use-current-diff-p
  		   (ediff-clone-buffer-for-current-diff-comparison
! 		    bufA 'A "-Region.A-")
  		 (ediff-clone-buffer-for-region-comparison bufA "-Region.A-")))
      (ediff-with-current-buffer bufA
        (setq begA (region-beginning)
--- 3564,3570 ----
  
      (setq bufA (if use-current-diff-p
  		   (ediff-clone-buffer-for-current-diff-comparison
! 		    bufA symA "-Region.A-")
  		 (ediff-clone-buffer-for-region-comparison bufA "-Region.A-")))
      (ediff-with-current-buffer bufA
        (setq begA (region-beginning)
***************
*** 3571,3577 ****
  
      (setq bufB (if use-current-diff-p
  		   (ediff-clone-buffer-for-current-diff-comparison
! 		    bufB 'B "-Region.B-")
  		 (ediff-clone-buffer-for-region-comparison bufB "-Region.B-")))
      (ediff-with-current-buffer bufB
        (setq begB (region-beginning)
--- 3579,3585 ----
  
      (setq bufB (if use-current-diff-p
  		   (ediff-clone-buffer-for-current-diff-comparison
! 		    bufB symB "-Region.B-")
  		 (ediff-clone-buffer-for-region-comparison bufB "-Region.B-")))
      (ediff-with-current-buffer bufB
        (setq begB (region-beginning)

Diff finished.  Wed Nov 17 14:14:57 2010

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: ediff-util.el.fix-line-bug.diff --]
[-- Type: text/x-patch; name="ediff-util.el.fix-line-bug.diff", Size: 1228 bytes --]

diff -c -b /home/tehom/.emacs.d/ediff/ediff-util.fix.el /home/tehom/.emacs.d/ediff/ediff-util.fix2.el
*** /home/tehom/.emacs.d/ediff/ediff-util.fix.el	Wed Nov 17 13:47:53 2010
--- /home/tehom/.emacs.d/ediff/ediff-util.fix2.el	Wed Nov 17 14:06:44 2010
***************
*** 3573,3580 ****
        (beginning-of-line)
        (setq begA (point))
        (goto-char endA)
!       (end-of-line)
!       (or (eobp) (forward-char)) ; include the newline char
        (setq endA (point)))
  
      (setq bufB (if use-current-diff-p
--- 3573,3579 ----
        (beginning-of-line)
        (setq begA (point))
        (goto-char endA)
!       (unless (bolp) (forward-line 1))
        (setq endA (point)))
  
      (setq bufB (if use-current-diff-p
***************
*** 3588,3595 ****
        (beginning-of-line)
        (setq begB (point))
        (goto-char endB)
!       (end-of-line)
!       (or (eobp) (forward-char)) ; include the newline char
        (setq endB (point)))
  
  
--- 3587,3593 ----
        (beginning-of-line)
        (setq begB (point))
        (goto-char endB)
!       (unless (bolp) (forward-line 1))
        (setq endB (point)))
  
  

Diff finished.  Wed Nov 17 14:15:19 2010

[-- Attachment #4: file1.a.txt --]
[-- Type: text/plain, Size: 103 bytes --]

Line 1
Line 2
Line 3
Line X-2
Line X-1
  ABC DEF
Line X+1
Line X+2
Line X+3

Line -2
Line -1

[-- Attachment #5: file1.b.txt --]
[-- Type: text/plain, Size: 37 bytes --]

Line 1
Line 2
ABC
Line -2
Line -1

[-- Attachment #6: file1.c.txt --]
[-- Type: text/plain, Size: 87 bytes --]

Line 1
Line 2

Line X-2
Line X-1
  ABC DEF
Line X+1
Line X+2

Line -2
Line -1

[-- Attachment #7: odd-whitespace-file1.a.txt --]
[-- Type: text/plain, Size: 85 bytes --]

Line 1
Line 2
Line 3 A
Line 4
Line 5 A
Line 6
Line 7 A
Line 8
Line 9
Line 10

[-- Attachment #8: odd-whitespace-file1.b.txt --]
[-- Type: text/plain, Size: 85 bytes --]

Line 1
Line 2
Line 3
Line 4 B
Line 5
Line 6 B
Line 7
Line 8 B
Line 9
Line 10

[-- Attachment #9: odd-whitespace-file1.c.txt --]
[-- Type: text/plain, Size: 79 bytes --]

Line 1
Line 2
Line 3
Line 4
Line 5
Line 6
Line 7
Line 8
Line 9
Line 10

[-- Attachment #10: odd-whitespace-2-file1.a.txt --]
[-- Type: text/plain, Size: 189 bytes --]

Line 1          X
Line 2          X
Line 3 A        X
Line 4          X
Line 5 A        X
Line 6          X
Line 7 A        X
Line 8          X
Line 9          X
Line 10          X

[-- Attachment #11: odd-whitespace-2-file1.b.txt --]
[-- Type: text/plain, Size: 189 bytes --]

Line 1          X
Line 2          X
Line 3          X
Line 4 B        X
Line 5          X
Line 6 B        X
Line 7          X
Line 8 B        X
Line 9          X
Line 10          X

[-- Attachment #12: odd-whitespace-2-file1.c.txt --]
[-- Type: text/plain, Size: 189 bytes --]

Line 1          X
Line 2          X
Line 3          X
Line 4          X
Line 5          X
Line 6          X
Line 7          X
Line 8          X
Line 9          X
Line 10          X