Re: fix for bug#29935 copyright-update inserts year at random places

[Stephen Leake]

Eli Zaretskii <address@hidden> writes:

>> From: Stephen Leake <address@hidden>
>> Date: Mon, 01 Jan 2018 17:29:27 -0600
>>
>> Ok to commit to emacs-26?
>
> How important is it to have this in Emacs 26?
>
>> +  (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
>> +          (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.
>> +                  (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? "))))))
>
> Why does this need to use save-window-excursion?

'switch-to-buffer' can pop up a new frame, or use a different window,
depending on various settings, so save-window-excursion is needed. 

'(goto-char copyright-end)' is needed because in general, a buffer does
not have a well-defined 'point'; is has a point in each window it is
displayed in, and recent Emacsen preserve that point when the buffer is
swapped in and out of that window (which is a nice feature).
'copyright-find-end' changes point in the buffer, but that's not used if
it's not displayed in the current window.

You suggest "using other facilities", but isn't this precisely what
save-window-excursion and save-excursion are for?

Here's a new patch, with improved comments:

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

Ok to commit?

--
-- Stephe