emacs-bug-tracker
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[Emacs-bug-tracker] bug#7277: closed (24.0.50; Can't revert diff-buffer-


From: GNU bug Tracking System
Subject: [Emacs-bug-tracker] bug#7277: closed (24.0.50; Can't revert diff-buffer-with-file)
Date: Mon, 22 Nov 2010 19:53:01 +0000

Your message dated Mon, 22 Nov 2010 14:22:22 -0500
with message-id <address@hidden>
and subject line Re: 24.0.50; Can't revert diff-buffer-with-file
has caused the GNU bug report #7277,
regarding 24.0.50; Can't revert diff-buffer-with-file
to be marked as done.

(If you believe you have received this mail in error, please contact
address@hidden)


-- 
7277: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=7277
GNU Bug Tracking System
Contact address@hidden with problems
--- Begin Message --- Subject: 24.0.50; Can't revert diff-buffer-with-file Date: Sun, 24 Oct 2010 16:30:17 -0400
   The problem, of course, is that the temp file is deleted shortly
after "diff" exits, so the default revert-buffer function installed by
"diff" fails.  The simplest solution seems to be to install a new
revert-buffer function that redoes the whole process of checking for a
file buffer and writing the temp file.  The attached patch does a
minimal refactoring of "diff" and "diff-buffer-with-file" to do just
that, while trying to DTRT if the user renames the diff buffer or kills
the original buffer.

                                        -- Bob Rogers
                                           http://www.rgrjr.com/

diff --git a/lisp/files.el b/lisp/files.el
index d5f60b7..603c014 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -4512,24 +4512,42 @@ Before and after saving the buffer, this function runs
   "View the differences between BUFFER and its associated file.
 This requires the external program `diff' to be in your `exec-path'."
   (interactive "bBuffer: ")
-  (with-current-buffer (get-buffer (or buffer (current-buffer)))
+  (diff-buffer-internal (get-buffer (or buffer (current-buffer)))
+                       (get-buffer-create "*Diff*"))
+  ;; return always nil, so that save-buffers-kill-emacs will not move
+  ;; over to the next unsaved buffer when calling `d'.
+  nil)
+
+(defvar diff-buffer-buffer)    ;; suppress compiler warnings.
+
+(defun diff-buffer-internal (buffer result-buffer)
+  (if (not (and buffer (buffer-name buffer)))
+      (error "Original buffer deleted."))
+  (with-current-buffer buffer
     (if (and buffer-file-name
             (file-exists-p buffer-file-name))
        (let ((tempfile (make-temp-file "buffer-content-")))
          (unwind-protect
              (progn
                (write-region nil nil tempfile nil 'nomessage)
-               (diff buffer-file-name tempfile nil t)
-               (sit-for 0))
+               ;; No asynch so we don't delete the temp file prematurely.
+               (diff-into-buffer result-buffer buffer-file-name tempfile
+                                 nil t)
+               (sit-for 0)
+               ;; Now revise the revert-buffer-function, since the
+               ;; default will not be able to find the temp file.
+               (with-current-buffer result-buffer
+                 (set (make-local-variable 'diff-buffer-buffer) buffer)
+                 (setq revert-buffer-function
+                       (lambda (ignore-auto noconfirm)
+                         (diff-buffer-internal diff-buffer-buffer
+                                               (current-buffer))))))
            (when (file-exists-p tempfile)
              (delete-file tempfile))))
       (message "Buffer %s has no associated file on disc" (buffer-name))
       ;; Display that message for 1 second so that user can read it
       ;; in the minibuffer.
-      (sit-for 1)))
-  ;; return always nil, so that save-buffers-kill-emacs will not move
-  ;; over to the next unsaved buffer when calling `d'.
-  nil)
+      (sit-for 1))))
 
 (defvar save-some-buffers-action-alist
   `((?\C-r
diff --git a/lisp/vc/diff.el b/lisp/vc/diff.el
index e79e72c..1a835b5 100644
--- a/lisp/vc/diff.el
+++ b/lisp/vc/diff.el
@@ -108,11 +108,16 @@ specified in `diff-switches' are passed to the diff 
command."
                  (read-file-name "Diff original file: "
                                  (file-name-directory newf) nil t)))
      (list oldf newf (diff-switches))))
+  (diff-into-buffer nil old new switches no-async))
+
+(defun diff-into-buffer (buf old new &optional switches no-async)
+  ;; Noninteractive helper for creating and reverting diff buffers.
   (setq new (expand-file-name new)
        old (expand-file-name old))
   (or switches (setq switches diff-switches)) ; If not specified, use default.
+  (or buf (setq buf (get-buffer-create "*Diff*")))
   (let* ((old-alt (file-local-copy old))
-       (new-alt (file-local-copy new))
+        (new-alt (file-local-copy new))
         (command
          (mapconcat 'identity
                     `(,diff-command
@@ -123,7 +128,6 @@ specified in `diff-switches' are passed to the diff 
command."
                       ,(shell-quote-argument (or old-alt old))
                       ,(shell-quote-argument (or new-alt new)))
                     " "))
-        (buf (get-buffer-create "*Diff*"))
         (thisdir default-directory)
         proc)
     (save-excursion

--- End Message ---
--- Begin Message --- Subject: Re: 24.0.50; Can't revert diff-buffer-with-file Date: Mon, 22 Nov 2010 14:22:22 -0500 User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.0.50 (gnu/linux)
> The attached patch does a minimal refactoring of "diff" and
> "diff-buffer-with-file" to do just that, while trying to DTRT if the
> user renames the diff buffer or kills the original buffer.

Thanks, installed.
Actually, I then started to take a closer look at the code and ended up
rewriting most of it to fix various problems.  See patch below.


        Stefan
        
        
=== modified file 'lisp/ChangeLog'
--- lisp/ChangeLog      2010-11-22 17:57:46 +0000
+++ lisp/ChangeLog      2010-11-22 19:19:24 +0000
@@ -1,3 +1,20 @@
+2010-11-22  Stefan Monnier  <address@hidden>
+
+       * vc/diff.el (diff-old-temp-file, diff-new-temp-file): Remove.
+       (diff-sentinel): Get them as arguments instead.
+       (diff-old-file, diff-new-file, diff-extra-args): Remove.
+       (diff-file-local-copy, diff-better-file-name): New funs.
+       (diff-no-select): Rename from diff-into-buffer.
+       Support buffers additionally to files.  Move `buf' arg.  Don't display 
buf.
+       Prefer closures to buffer-local variables.
+       (diff): Adjust accordingly.
+       (diff-buffer-with-file): Move from files.el.
+       * files.el (diff-buffer-with-file): Move to vc/diff.el.
+       (diff-buffer-internal): Remove.
+       (diff-buffer-buffer): Remove.
+       (save-some-buffers-action-alist): Use diff-no-select so as not to guess
+       the buffer name used, and so as not to mess up windows and frames.
+
 2010-11-22  Bob Rogers  <address@hidden>
 
        * files.el: Make revert work with diff-buffer-with-file (bug#7277).

=== modified file 'lisp/files.el'
--- lisp/files.el       2010-11-22 17:57:46 +0000
+++ lisp/files.el       2010-11-22 19:13:31 +0000
@@ -4487,47 +4487,6 @@
                   (setq buffer-backed-up nil))))))
     setmodes))
 
-(defun diff-buffer-with-file (&optional buffer)
-  "View the differences between BUFFER and its associated file.
-This requires the external program `diff' to be in your `exec-path'."
-  (interactive "bBuffer: ")
-  (diff-buffer-internal (get-buffer (or buffer (current-buffer)))
-                       (get-buffer-create "*Diff*"))
-  ;; return always nil, so that save-buffers-kill-emacs will not move
-  ;; over to the next unsaved buffer when calling `d'.
-  nil)
-
-(defvar diff-buffer-buffer)    ;; suppress compiler warnings.
-
-(defun diff-buffer-internal (buffer result-buffer)
-  (if (not (and buffer (buffer-name buffer)))
-      (error "Original buffer deleted."))
-  (with-current-buffer buffer
-    (if (and buffer-file-name
-            (file-exists-p buffer-file-name))
-       (let ((tempfile (make-temp-file "buffer-content-")))
-         (unwind-protect
-             (progn
-               (write-region nil nil tempfile nil 'nomessage)
-               ;; No asynch so we don't delete the temp file prematurely.
-               (diff-into-buffer result-buffer buffer-file-name tempfile
-                                 nil t)
-               (sit-for 0)
-               ;; Now revise the revert-buffer-function, since the
-               ;; default will not be able to find the temp file.
-               (with-current-buffer result-buffer
-                 (set (make-local-variable 'diff-buffer-buffer) buffer)
-                 (setq revert-buffer-function
-                       (lambda (ignore-auto noconfirm)
-                         (diff-buffer-internal diff-buffer-buffer
-                                               (current-buffer))))))
-           (when (file-exists-p tempfile)
-             (delete-file tempfile))))
-      (message "Buffer %s has no associated file on disc" (buffer-name))
-      ;; Display that message for 1 second so that user can read it
-      ;; in the minibuffer.
-      (sit-for 1))))
-
 (defvar save-some-buffers-action-alist
   `((?\C-r
      ,(lambda (buf)
@@ -4542,13 +4501,14 @@
     (?d ,(lambda (buf)
            (if (null (buffer-file-name buf))
                (message "Not applicable: no file")
-             (save-window-excursion (diff-buffer-with-file buf))
+             (require 'diff)            ;for diff-no-select.
+             (let ((diffbuf (diff-no-select (buffer-file-name buf) buf
+                                            nil 'noasync)))
              (if (not enable-recursive-minibuffers)
-                 (progn (display-buffer (get-buffer-create "*Diff*"))
-                        (setq other-window-scroll-buffer "*Diff*"))
-               (view-buffer (get-buffer-create "*Diff*")
-                            (lambda (_) (exit-recursive-edit)))
-               (recursive-edit)))
+                   (progn (display-buffer diffbuf)
+                          (setq other-window-scroll-buffer diffbuf))
+                 (view-buffer diffbuf (lambda (_) (exit-recursive-edit)))
+                 (recursive-edit))))
            ;; Return nil to ask about BUF again.
            nil)
        ,(purecopy "view changes in this buffer")))

=== modified file 'lisp/vc/diff.el'
--- lisp/vc/diff.el     2010-11-22 17:57:46 +0000
+++ lisp/vc/diff.el     2010-11-22 19:19:14 +0000
@@ -31,6 +31,8 @@
 
 ;;; Code:
 
+(eval-when-compile (require 'cl))
+
 (defgroup diff nil
   "Comparing files with `diff'."
   :group 'tools)
@@ -47,11 +49,6 @@
   :type 'string
   :group 'diff)
 
-(defvar diff-old-temp-file nil
-  "This is the name of a temp file to be deleted after diff finishes.")
-(defvar diff-new-temp-file nil
-  "This is the name of a temp file to be deleted after diff finishes.")
-
 ;; prompt if prefix arg present
 (defun diff-switches ()
   (if current-prefix-arg
@@ -60,12 +57,12 @@
                       diff-switches
                     (mapconcat 'identity diff-switches " ")))))
 
-(defun diff-sentinel (code)
+(defun diff-sentinel (code old-temp-file new-temp-file)
   "Code run when the diff process exits.
 CODE is the exit code of the process.  It should be 0 only if no diffs
 were found."
-  (if diff-old-temp-file (delete-file diff-old-temp-file))
-  (if diff-new-temp-file (delete-file diff-new-temp-file))
+  (if old-temp-file (delete-file old-temp-file))
+  (if new-temp-file (delete-file new-temp-file))
   (save-excursion
     (goto-char (point-max))
     (let ((inhibit-read-only t))
@@ -75,10 +72,6 @@
                            (t ""))
                      (current-time-string))))))
 
-(defvar diff-old-file nil)
-(defvar diff-new-file nil)
-(defvar diff-extra-args nil)
-
 ;;;###autoload
 (defun diff (old new &optional switches no-async)
   "Find and display the differences between OLD and NEW files.
@@ -91,16 +84,15 @@
 interactively for diff switches.  Otherwise, the switches
 specified in `diff-switches' are passed to the diff command."
   (interactive
-   (let (oldf newf)
-     (setq newf (buffer-file-name)
-          newf (if (and newf (file-exists-p newf))
+   (let ((newf (buffer-file-name))
+         (oldf (file-newest-backup newf)))
+     (setq newf (if (and newf (file-exists-p newf))
                    (read-file-name
                     (concat "Diff new file (default "
                             (file-name-nondirectory newf) "): ")
                     nil newf t)
                  (read-file-name "Diff new file: " nil nil t)))
-     (setq oldf (file-newest-backup newf)
-          oldf (if (and oldf (file-exists-p oldf))
+     (setq oldf (if (and oldf (file-exists-p oldf))
                    (read-file-name
                     (concat "Diff original file (default "
                             (file-name-nondirectory oldf) "): ")
@@ -108,63 +100,82 @@
                  (read-file-name "Diff original file: "
                                  (file-name-directory newf) nil t)))
      (list oldf newf (diff-switches))))
-  (diff-into-buffer nil old new switches no-async))
+  (display-buffer
+   (diff-no-select old new switches no-async)))
 
-(defun diff-into-buffer (buf old new &optional switches no-async)
-  ;; Noninteractive helper for creating and reverting diff buffers.
-  (setq new (expand-file-name new)
-       old (expand-file-name old))
+(defun diff-file-local-copy (file-or-buf)
+  (if (bufferp file-or-buf)
+      (with-current-buffer file-or-buf
+        (let ((tempfile (make-temp-file "buffer-content-")))
+          (write-region nil nil tempfile nil 'nomessage)
+          tempfile))
+    (file-local-copy file-or-buf)))
+
+(defun diff-better-file-name (file)
+  (if (bufferp file) file
+    (let ((rel (file-relative-name file))
+          (abbr (abbreviate-file-name (expand-file-name file))))
+      (if (< (length abbr) (length rel))
+          abbr
+        rel))))
+
+(defun diff-no-select (old new &optional switches no-async buf)
+  ;; Noninteractive helper for creating and reverting diff buffers
+  (setq new (diff-better-file-name new)
+       old (diff-better-file-name old))
   (or switches (setq switches diff-switches)) ; If not specified, use default.
+  (unless (listp switches) (setq switches (list switches)))
   (or buf (setq buf (get-buffer-create "*Diff*")))
-  (let* ((old-alt (file-local-copy old))
-        (new-alt (file-local-copy new))
+  (let* ((old-alt (diff-file-local-copy old))
+        (new-alt (diff-file-local-copy new))
         (command
          (mapconcat 'identity
                     `(,diff-command
                       ;; Use explicitly specified switches
-                      ,@(if (listp switches) switches (list switches))
-                      ,@(if (or old-alt new-alt)
-                            (list "-L" old "-L" new))
-                      ,(shell-quote-argument (or old-alt old))
-                      ,(shell-quote-argument (or new-alt new)))
+                      ,@switches
+                       ,@(mapcar #'shell-quote-argument
+                                 (nconc
+                                  (when (or old-alt new-alt)
+                                    (list "-L" (if (stringp old)
+                                                   old (prin1-to-string old))
+                                          "-L" (if (stringp new)
+                                                   new (prin1-to-string new))))
+                                  (list (or old-alt old)
+                                        (or new-alt new)))))
                     " "))
-        (thisdir default-directory)
-        proc)
-    (save-excursion
-      (display-buffer buf)
-      (set-buffer buf)
-      (setq buffer-read-only nil)
+        (thisdir default-directory))
+    (with-current-buffer buf
+      (setq buffer-read-only t)
       (buffer-disable-undo (current-buffer))
       (let ((inhibit-read-only t))
        (erase-buffer))
       (buffer-enable-undo (current-buffer))
       (diff-mode)
-      ;; Use below 2 vars for backward-compatibility.
-      (set (make-local-variable 'diff-old-file) old)
-      (set (make-local-variable 'diff-new-file) new)
-      (set (make-local-variable 'diff-extra-args) (list switches no-async))
       (set (make-local-variable 'revert-buffer-function)
+           (lexical-let ((old old) (new new)
+                         (switches switches)
+                         (no-async no-async))
           (lambda (ignore-auto noconfirm)
-             (apply 'diff diff-old-file diff-new-file diff-extra-args)))
-      (set (make-local-variable 'diff-old-temp-file) old-alt)
-      (set (make-local-variable 'diff-new-temp-file) new-alt)
+               (diff-no-select old new switches no-async (current-buffer)))))
       (setq default-directory thisdir)
       (let ((inhibit-read-only t))
        (insert command "\n"))
       (if (and (not no-async) (fboundp 'start-process))
-         (progn
-           (setq proc (start-process "Diff" buf shell-file-name
-                                     shell-command-switch command))
+         (let ((proc (start-process "Diff" buf shell-file-name
+                                     shell-command-switch command)))
            (set-process-filter proc 'diff-process-filter)
+            (lexical-let ((old-alt old-alt) (new-alt new-alt))
            (set-process-sentinel
             proc (lambda (proc msg)
                    (with-current-buffer (process-buffer proc)
-                     (diff-sentinel (process-exit-status proc))))))
+                        (diff-sentinel (process-exit-status proc)
+                                       old-alt new-alt))))))
        ;; Async processes aren't available.
        (let ((inhibit-read-only t))
          (diff-sentinel
           (call-process shell-file-name nil buf nil
-                        shell-command-switch command)))))
+                        shell-command-switch command)
+           old-alt new-alt))))
     buf))
 
 (defun diff-process-filter (proc string)
@@ -203,6 +214,14 @@
        (funcall handler 'diff-latest-backup-file fn)
       (file-newest-backup fn))))
 
+;;;###autoload
+(defun diff-buffer-with-file (&optional buffer)
+  "View the differences between BUFFER and its associated file.
+This requires the external program `diff' to be in your `exec-path'."
+  (interactive "bBuffer: ")
+  (with-current-buffer (get-buffer (or buffer (current-buffer)))
+    (diff buffer-file-name (current-buffer) nil 'noasync)))
+
 (provide 'diff)
 
 ;; arch-tag: 7de2c29b-7ea5-4b85-9b9d-72dd860de2bd



--- End Message ---

reply via email to

[Prev in Thread] Current Thread [Next in Thread]