Subject: bug#33567: Syntactic fontification of diff hunks

From: Juri Linkov <[email-masked]>
Date: Wed, 05 Dec 2018 00:58:49 +0200

>> -(defun vc-find-revision-no-save (file revision &optional backend)
>> -  "Read REVISION of FILE into a buffer and return the buffer.
>> +(defun vc-find-revision-no-save (file revision &optional backend buffer)
>> +  "Read REVISION of FILE into a BUFFER and return the buffer.
>
> Since you use BUFFER, please drop the "a" part.
>
> Also, the doc string should tell what BUFFER defaults to if omitted or
> nil.

Updated.

>> +  (let ((filebuf (or buffer (get-file-buffer file) (current-buffer)))
>                      ^^^^^^^^^
> I would use bufferp here.  What if someone calls the function with
> something that is not a buffer?

Added buffer-live-p.

> And, btw, are buffer names allowed?

I see that most VC functions work with buffer objects only.
So buffer names could be allowed only when really necessary.

> Similarly with other places where you make the same test (why not make
> it once and bind some local variable to the result, btw?).

Done:

diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index de43544864..dbbc3e2038 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -1998,33 +1998,41 @@ vc-find-revision-save
 	(set (make-local-variable 'vc-parent-buffer) filebuf))
       result-buf)))
 
-(defun vc-find-revision-no-save (file revision &optional backend)
-  "Read REVISION of FILE into a buffer and return the buffer.
-Unlike `vc-find-revision-save', doesn't save the created buffer to file."
-  (let ((filebuf (or (get-file-buffer file) (current-buffer)))
-        (filename (vc-version-backup-file-name file revision 'manual)))
-    (unless (or (get-file-buffer filename)
-                (file-exists-p filename))
+(defun vc-find-revision-no-save (file revision &optional backend buffer)
+  "Read REVISION of FILE into BUFFER and return the buffer.
+If BUFFER omitted or nil, this function creates a new buffer and sets
+`buffer-file-name' to the name constructed from the file name and the
+revision number.
+Unlike `vc-find-revision-save', doesn't save the buffer to the file."
+  (let* ((buffer (when (buffer-live-p buffer) buffer))
+         (filebuf (or buffer (get-file-buffer file) (current-buffer)))
+         (filename (unless buffer (vc-version-backup-file-name file revision 'manual))))
+    (unless (and (not buffer)
+                 (or (get-file-buffer filename)
+                     (file-exists-p filename)))
       (with-current-buffer filebuf
 	(let ((failed t))
 	  (unwind-protect
 	      (let ((coding-system-for-read 'no-conversion)
-		    (coding-system-for-write 'no-conversion))
-		(with-current-buffer (create-file-buffer filename)
-                  (setq buffer-file-name filename)
+                    (coding-system-for-write 'no-conversion))
+		(with-current-buffer (or buffer (create-file-buffer filename))
+                  (unless buffer (setq buffer-file-name filename))
 		  (let ((outbuf (current-buffer)))
 		    (with-current-buffer filebuf
 		      (if backend
 			  (vc-call-backend backend 'find-revision file revision outbuf)
 			(vc-call find-revision file revision outbuf))))
                   (goto-char (point-min))
-                  (normal-mode)
+                  (if buffer (let ((buffer-file-name file)) (normal-mode)) (normal-mode))
 	          (set-buffer-modified-p nil)
                   (setq buffer-read-only t))
 		(setq failed nil))
-	    (when (and failed (get-file-buffer filename))
+	    (when (and failed (unless buffer (get-file-buffer filename)))
+	      (with-current-buffer (get-file-buffer filename)
+		(set-buffer-modified-p nil))
 	      (kill-buffer (get-file-buffer filename)))))))
-    (let ((result-buf (or (get-file-buffer filename)
+    (let ((result-buf (or buffer
+                          (get-file-buffer filename)
                           (find-file-noselect filename))))
       (with-current-buffer result-buf
 	(set (make-local-variable 'vc-parent-buffer) filebuf))