GNU bug report logs - #7277
24.0.50; Can't revert diff-buffer-with-file

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 7277 in the body.
You can then email your comments to 7277 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: rogers-emacs <at> rgrjr.dyndns.org
To: bug-gnu-emacs <at> gnu.org
Subject: 24.0.50; Can't revert diff-buffer-with-file
Date: Sun, 24 Oct 2010 16:30:17 -0400

[Message part 1 (text/plain, inline)]

   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-buffer-revert-1.patch (text/plain, inline)]

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

Message #10 received at 7277-done <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: rogers-emacs <at> rgrjr.dyndns.org
Subject: Re: 24.0.50; Can't revert diff-buffer-with-file
Date: Mon, 22 Nov 2010 14:22:22 -0500

> 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  <monnier <at> iro.umontreal.ca>
+
+	* 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  <rogers-emacs <at> rgrjr.dyndns.org>
 
 	* 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

Message #13 received at 7277 <at> debbugs.gnu.org (full text, mbox):

From: Glenn Morris <rgm <at> gnu.org>
To: 7277 <at> debbugs.gnu.org
Cc: monnier <at> IRO.UMontreal.CA
Subject: Re: bug#7277: 24.0.50; Can't revert diff-buffer-with-file
Date: Tue, 23 Nov 2010 14:44:20 -0500

Stefan Monnier wrote:

> +   (let ((newf (buffer-file-name))
> +         (oldf (file-newest-backup newf)))

You probably wanted let*. (This is flagged by the compiler.)

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.