Subject: Re: 24.0.50; Can't revert diff-buffer-with-file
From: Stefan Monnier <[email-masked]>
To: [email-masked]
Date: Mon, 22 Nov 2010 14:22:22 -0500
List-Id: <debbugs-submit.debbugs.gnu.org>

> 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  <[email-masked]>
+
+	* 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  <[email-masked]>
 
 	* 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