Subject: bug#29935: recipe and improved patch

From: Stephen Leake <[email-masked]>
Date: Wed, 10 Jan 2018 13:22:52 -0600

Here is a recipe for reproducing the bug in emacs-26.0.90 -Q:

Create a directory /tmp/test-copyright
Copy the attached file_1.c-save to that directory
Start emacs -Q

(require 'copyright)
(add-hook 'before-save-hook 'copyright-update)
(find-file "/tmp/test-copyright/file_1.c")

C-x 2
C-x o
C-end
;; insert '// changed line'
C-x b *Messages*
C-x o
C-x b *scratch*
C-x o
M-x save-some-buffers
!
y
C-x b file_1.c

This shows the bug: ', 2018' was inserted on the last line in file_1.c,
not the copyright line.

The fix in the patch given above is "include 'switch-to-buffer' in the
'save-excursion'"; it is not sufficient for this test case, although I
assume it is sufficient in other cases.

A discussion on emacs-devel starting at
https://lists.gnu.org/archive/html/emacs-devel/2018-01/msg00024.html
includes the suggestion to delete 'save-window-excursion'; that does not
fix this test case, and in addition it causes file_1.c to be not
displayed while asking about updating the copyright.

The root cause of the bug is that emacs saves a point for each buffer in
each window; when file_1.c is displayed in the lower window, point is
restored to where it was last displayed in that window; on the last
line. 

The attached patch copyright.diff fixes this test case in a minimal way,
by saving the copyright-end point, and restoring it after displaying the
buffer. It also includes 'switch-to-buffer' in the 'save-exursion' to
address the other use cases.

The email thread mentioned above also discusses trying to preserve
frames as well, since switch-to-buffer can pop up a new frame with
certain user settings. This patch does not address that, since it is not
a regression in emacs 26. A more thorough redesign of this function, to
be cleaner and address restoring frames, is left for emacs 27.

-- 
-- Stephe
--- a/lisp/emacs-lisp/copyright.el
+++ b/lisp/emacs-lisp/copyright.el
@@ -181,44 +181,54 @@ copyright-update-year
   ;; This uses the match-data from copyright-find-copyright/end.
   (goto-char (match-end 1))
   (copyright-find-end)
-  (setq copyright-current-year (format-time-string "%Y"))
-  (unless (string= (buffer-substring (- (match-end 3) 2) (match-end 3))
-		   (substring copyright-current-year -2))
-    (if (or noquery
-	    (save-window-excursion
-	      (switch-to-buffer (current-buffer))
-	      ;; Fixes some point-moving oddness (bug#2209).
-	      (save-excursion
-		(y-or-n-p (if replace
-			      (concat "Replace copyright year(s) by "
-				      copyright-current-year "? ")
-			    (concat "Add " copyright-current-year
-				    " to copyright? "))))))
-	(if replace
-	    (replace-match copyright-current-year t t nil 3)
-	  (let ((size (save-excursion (skip-chars-backward "0-9"))))
-	    (if (and (eq (% (- (string-to-number copyright-current-year)
-			       (string-to-number (buffer-substring
-						  (+ (point) size)
-						  (point))))
-			    100)
-			 1)
-		     (or (eq (char-after (+ (point) size -1)) ?-)
-			 (eq (char-after (+ (point) size -2)) ?-)))
-		;; This is a range so just replace the end part.
-		(delete-char size)
-	      ;; Insert a comma with the preferred number of spaces.
-	      (insert
-	       (save-excursion
-		 (if (re-search-backward "[0-9]\\( *, *\\)[0-9]"
-					 (line-beginning-position) t)
-		     (match-string 1)
-		   ", ")))
-	      ;; If people use the '91 '92 '93 scheme, do that as well.
-	      (if (eq (char-after (+ (point) size -3)) ?')
-		  (insert ?')))
-	    ;; Finally insert the new year.
-	    (insert (substring copyright-current-year size)))))))
+  (let ((copyright-end (point)))
+    (setq copyright-current-year (format-time-string "%Y"))
+    (unless (string= (buffer-substring (- (match-end 3) 2) (match-end 3))
+		     (substring copyright-current-year -2))
+      (if (or noquery
+              ;; ‘switch-to-buffer’ can pop up a new frame, or use
+              ;; another window, so preserve the current window
+              ;; config.
+              (save-window-excursion
+	        ;; Fixes some point-moving oddness (bug#2209, bug#29935).
+	        (save-excursion
+	          (switch-to-buffer (current-buffer))
+                  ;; Ensure the copyright line is displayed;
+                  ;; switch-to-buffer has moved point to where it was
+                  ;; when this buffer was last displayed in this
+                  ;; window.
+                  (goto-char copyright-end)
+		  (y-or-n-p (if replace
+			        (concat "Replace copyright year(s) by "
+				        copyright-current-year "? ")
+			      (concat "Add " copyright-current-year
+				      " to copyright? "))))))
+	  (if replace
+	      (replace-match copyright-current-year t t nil 3)
+	    (let ((size (save-excursion (skip-chars-backward "0-9"))))
+	      (if (and (eq (% (- (string-to-number copyright-current-year)
+			         (string-to-number (buffer-substring
+						    (+ (point) size)
+						    (point))))
+			      100)
+			   1)
+		       (or (eq (char-after (+ (point) size -1)) ?-)
+			   (eq (char-after (+ (point) size -2)) ?-)))
+		  ;; This is a range so just replace the end part.
+		  (delete-char size)
+	        ;; Insert a comma with the preferred number of spaces.
+	        (insert
+	         (save-excursion
+		   (if (re-search-backward "[0-9]\\( *, *\\)[0-9]"
+					   (line-beginning-position) t)
+		       (match-string 1)
+		     ", ")))
+	        ;; If people use the '91 '92 '93 scheme, do that as well.
+	        (if (eq (char-after (+ (point) size -3)) ?')
+		    (insert ?')))
+	      ;; Finally insert the new year.
+	      (insert (substring copyright-current-year size))))
+        ))))
 
 ;;;###autoload
 (defun copyright-update (&optional arg interactivep)