From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: "Tom Breton (Tehom)" Newsgroups: gmane.emacs.devel Subject: Bugfix for ediff, two patches Date: Wed, 17 Nov 2010 14:35:41 -0500 Message-ID: NNTP-Posting-Host: lo.gmane.org Mime-Version: 1.0 Content-Type: multipart/mixed;boundary="----=_20101117143541_34381" X-Trace: dough.gmane.org 1290023074 23309 80.91.229.12 (17 Nov 2010 19:44:34 GMT) X-Complaints-To: usenet@dough.gmane.org NNTP-Posting-Date: Wed, 17 Nov 2010 19:44:34 +0000 (UTC) To: emacs-devel@gnu.org Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Wed Nov 17 20:44:30 2010 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([199.232.76.165]) by lo.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1PInve-0005eu-AD for ged-emacs-devel@m.gmane.org; Wed, 17 Nov 2010 20:44:26 +0100 Original-Received: from localhost ([127.0.0.1]:34369 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PInnK-0000Fs-Af for ged-emacs-devel@m.gmane.org; Wed, 17 Nov 2010 14:35:50 -0500 Original-Received: from [140.186.70.92] (port=55634 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PInnE-0000Fm-GV for emacs-devel@gnu.org; Wed, 17 Nov 2010 14:35:45 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PInnC-0005MN-Nb for emacs-devel@gnu.org; Wed, 17 Nov 2010 14:35:44 -0500 Original-Received: from mail1.panix.com ([166.84.1.72]:51349) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PInnC-0005MA-HI for emacs-devel@gnu.org; Wed, 17 Nov 2010 14:35:42 -0500 Original-Received: from mailbackend.panix.com (mailbackend.panix.com [166.84.1.89]) by mail1.panix.com (Postfix) with ESMTP id C372B1F084 for ; Wed, 17 Nov 2010 14:35:41 -0500 (EST) Original-Received: from mail.panix.com (localhost [127.0.0.1]) by mailbackend.panix.com (Postfix) with ESMTP id AD82432D5D for ; Wed, 17 Nov 2010 14:35:41 -0500 (EST) X-Panix-Received: from 96.252.49.112 (SquirrelMail authenticated user tehom@panix.com) by mail.panix.com with HTTP; Wed, 17 Nov 2010 14:35:41 -0500 User-Agent: SquirrelMail/1.4.19 X-Priority: 3 (Normal) Importance: Normal X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.devel:132792 Archived-At: ------=_20101117143541_34381 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable **** Explanation of the bug: When told to "Compare currently highlighted difference regions", ediff-inferior-compare-regions does *not* use the highlighted region as i= t 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 * "=3D" 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 th= e 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 comparison= s 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 tha= t 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 muc= h less serious: * Wordwise merging doesn't handle whitespace reasonably when merging t= o 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 t= o fix this one. * Possibly related: Inferior comparisons wrongly include the line afte= r the comparison region. ediff-inferior-compare-regions deliberately gets whole lines. Ie, i= f 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) ------=_20101117143541_34381 Content-Type: text/x-patch; name="ediff-util.el.fix.diff" Content-Disposition: attachment; filename="ediff-util.el.fix.diff" Content-Transfer-Encoding: quoted-printable diff -c -b /home/tehom/.emacs.d/ediff/ediff-util.old.el /home/tehom/.emac= s.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) =20 (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) =20 (if (ediff-valid-difference-p ediff-current-difference) (progn *************** *** 3487,3499 **** --- 3487,3502 ---- =20 (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))) + =20 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))) =20 (if (and (ediff-valid-difference-p ediff-current-difference) *************** *** 3556,3562 **** =20 (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 ---- =20 (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 **** =20 (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 ---- =20 (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 ------=_20101117143541_34381 Content-Type: text/x-patch; name="ediff-util.el.fix-line-bug.diff" Content-Disposition: attachment; filename="ediff-util.el.fix-line-bug.diff" Content-Transfer-Encoding: quoted-printable diff -c -b /home/tehom/.emacs.d/ediff/ediff-util.fix.el /home/tehom/.emac= s.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 201= 0 *************** *** 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))) =20 (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))) =20 (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))) =20 =20 --- 3587,3593 ---- (beginning-of-line) (setq begB (point)) (goto-char endB) ! (unless (bolp) (forward-line 1)) (setq endB (point))) =20 =20 Diff finished. Wed Nov 17 14:15:19 2010 ------=_20101117143541_34381 Content-Type: text/plain; name="file1.a.txt" Content-Disposition: attachment; filename="file1.a.txt" Content-Transfer-Encoding: quoted-printable 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 ------=_20101117143541_34381 Content-Type: text/plain; name="file1.b.txt" Content-Disposition: attachment; filename="file1.b.txt" Content-Transfer-Encoding: quoted-printable Line 1 Line 2 ABC Line -2 Line -1 ------=_20101117143541_34381 Content-Type: text/plain; name="file1.c.txt" Content-Disposition: attachment; filename="file1.c.txt" Content-Transfer-Encoding: quoted-printable Line 1 Line 2 Line X-2 Line X-1 ABC DEF Line X+1 Line X+2 Line -2 Line -1 ------=_20101117143541_34381 Content-Type: text/plain; name="odd-whitespace-file1.a.txt" Content-Disposition: attachment; filename="odd-whitespace-file1.a.txt" Content-Transfer-Encoding: quoted-printable Line 1 Line 2 Line 3 A Line 4 Line 5 A Line 6 Line 7 A Line 8 Line 9 Line 10 ------=_20101117143541_34381 Content-Type: text/plain; name="odd-whitespace-file1.b.txt" Content-Disposition: attachment; filename="odd-whitespace-file1.b.txt" Content-Transfer-Encoding: quoted-printable Line 1 Line 2 Line 3 Line 4 B Line 5 Line 6 B Line 7 Line 8 B Line 9 Line 10 ------=_20101117143541_34381 Content-Type: text/plain; name="odd-whitespace-file1.c.txt" Content-Disposition: attachment; filename="odd-whitespace-file1.c.txt" Content-Transfer-Encoding: quoted-printable Line 1 Line 2 Line 3 Line 4 Line 5 Line 6 Line 7 Line 8 Line 9 Line 10 ------=_20101117143541_34381 Content-Type: text/plain; name="odd-whitespace-2-file1.a.txt" Content-Disposition: attachment; filename="odd-whitespace-2-file1.a.txt" Content-Transfer-Encoding: quoted-printable 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 ------=_20101117143541_34381 Content-Type: text/plain; name="odd-whitespace-2-file1.b.txt" Content-Disposition: attachment; filename="odd-whitespace-2-file1.b.txt" Content-Transfer-Encoding: quoted-printable 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 ------=_20101117143541_34381 Content-Type: text/plain; name="odd-whitespace-2-file1.c.txt" Content-Disposition: attachment; filename="odd-whitespace-2-file1.c.txt" Content-Transfer-Encoding: quoted-printable 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 ------=_20101117143541_34381--