bug#29935: recipe and improved patch

[Stephen Leake]

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

file_1.c-save
Description: Binary data

--- 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)