bug#29935: 26.0.90; copyright-update adds year in middle of buffer

bug#29935: 26.0.90; copyright-update adds year in middle of buffer

[Stephen Leake]

With copyright-update in before-save-hook, edit multiple files that need
a copyright update, then execute save-some-buffers.

In some of the files, the new copyright year is inserted in the middle
of the buffer (where point last was?), not in the copyright line.

See https://lists.gnu.org/archive/html/emacs-devel/2017-10/msg00848.html
for more discussion. The fix posted there is:

  --- copyright-orig.el 2017-10-31 18:54:20.957330092 +0100
  +++ copyright.el      2017-10-31 18:55:11.624754452 +0100
  @@ -181,19 +181,22 @@
     ;; This uses the match-data from copyright-find-copyright/end.
     (goto-char (match-end 1))
     (copyright-find-end)
  +  (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
  +                (save-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? "))))))
  +          (progn
  +            (goto-char copyright-end)
           (if replace
               (replace-match copyright-current-year t t nil 3)
             (let ((size (save-excursion (skip-chars-backward "0-9"))))
  @@ -218,7 +221,7 @@
                 (if (eq (char-after (+ (point) size -3)) ?')
                     (insert ?')))
               ;; Finally insert the new year.
  -            (insert (substring copyright-current-year size)))))))
  +                (insert (substring copyright-current-year size)))))))))

   ;;;###autoload
   (defun copyright-update (&optional arg interactivep)


In GNU Emacs 26.0.90 (build 1, x86_64-w64-mingw32)
 of 2018-01-01 built on TAKVER4
Repository revision: 63b04c11d530f4a6a41f112d1b3ba1ed1eb81195
Windowing system distributor 'Microsoft Corp.', version 6.3.9600
Recent messages:
Quit
Mark set
Auto-saving...done
Mark set
Auto-saving...done
Quit
Mark set
Quit
Making completion list...
Mark saved where search started

Configured using:
 'configure --prefix=/c/emacs/emacs-26'

Configured features:
XPM JPEG TIFF GIF PNG RSVG SOUND NOTIFY ACL GNUTLS LIBXML2 ZLIB
TOOLKIT_SCROLL_BARS LCMS2

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: cp1252

Major mode: Emacs-Lisp

Minor modes in effect:
  shell-dirtrack-mode: t
  xref-ada-mode: t
  diff-auto-refine-mode: t
  other-frame-window-mode: t
  display-time-mode: t
  delete-selection-mode: t
  icomplete-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  electric-quote-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
c:/Projects/org.wisitoken.grammar_mode/autoloads hides c:/Projects/org.emacs.ada-mode.stephe-2/autoloads
c:/home/stephe/.emacs.d/elpa/ada-ref-man-2012.3/ada-ref-man hides c:/Projects/org.emacs.ada-mode.stephe-2/ada-ref-man
c:/Projects/org.emacs.ada-mode.stephe-2/ada-xref hides c:/Projects/emacs/emacs-26/lisp/progmodes/ada-xref
c:/Projects/org.emacs.ada-mode.stephe-2/ada-stmt hides c:/Projects/emacs/emacs-26/lisp/progmodes/ada-stmt
c:/Projects/org.emacs.ada-mode.stephe-2/ada-prj hides c:/Projects/emacs/emacs-26/lisp/progmodes/ada-prj
c:/Projects/org.emacs.ada-mode.stephe-2/ada-mode hides c:/Projects/emacs/emacs-26/lisp/progmodes/ada-mode

Features:
(shadow sort mail-extr emacsbug sendmail vc-git map help-fns radix-tree
copyright tabify shell ada-project env-project project-menu make-mode
misearch multi-isearch wisi-grammar-mode wisi_grammar-process
ada-process xref-ada ada-skel ada-skeletons skeleton ada-compiler
ada-gnat-compile gpr-query gnat-core ada-mode-compat ada-wisi
wisi-process-parse ada-indent-user-options ada-fix-error ada-elisp
wisi-elisp-parse wisi-compile wisi wisi-elisp-lexer wisi-parse-common
wisi-compat-24.2 semantic/lex semantic/fw mode-local ada-build ada-mode
find-file align cl-extra help-mode xmtn-dvc xmtn-conflicts xmtn-ids
dvc-persistence dvc-config xmtn-automate xmtn-run xmtn-match
xmtn-basic-io xmtn-base dvc-status dvc-diff dvc-log-edit vc ediff-merg
ediff-wind ediff-diff ediff-mult ediff-help ediff-init ediff-util ediff
add-log diff-mode dvc-propagate dvc-state vc-dispatcher vc-mtn org-rmail
org-mhe org-irc org-info org-gnus nnir gnus-sum gnus-group gnus-undo
gnus-start gnus-cloud nnimap nnmail mail-source tls gnutls utf7 netrc
nnoo parse-time gnus-spec gnus-int gnus-range message rmc puny rfc822
mml mml-sec epa derived epg mm-decode mm-bodies mm-encode mail-parse
rfc2231 mailabbrev gmm-utils mailheader gnus-win gnus nnheader gnus-util
rmail rmail-loaddefs rfc2047 rfc2045 ietf-drums mail-utils mm-util
mail-prsvr org-docview doc-view jka-compr image-mode org-bibtex bibtex
org-bbdb org-w3m org-element avl-tree generator org-mode-keys org advice
org-macro org-footnote org-pcomplete pcomplete org-list org-faces
org-entities org-version ob-emacs-lisp ob ob-tangle org-src ob-ref
ob-lob ob-table ob-keys ob-exp ob-comint ob-core ob-eval org-compat
org-macs org-loaddefs format-spec find-func cal-menu calendar
cal-loaddefs xgit-core xmtn-minimal dvc-autoloads dvc-unified ffap
thingatpt dvc-revlist dvc-fileinfo dvc-core dvc-buffers dvc-ui
dvc-register dvc-utils ewoc dvc-emacs dvc-defs other-frame-window
elec-pair time delsel cus-start cus-load color-theme edmacro kmacro
wid-edit cl noutline outline easy-mmode xref whitespace dired-x
dired-aux dired dired-loaddefs compile comint ansi-color ring
project-patches project uniquify-files path-iterator icomplete
finder-inf info package easymenu epg-config url-handlers url-parse
auth-source cl-seq eieio eieio-core cl-macs eieio-loaddefs
password-cache url-vars seq byte-opt gv bytecomp byte-compile cconv
cl-loaddefs cl-lib autoloads time-date mule-util tooltip eldoc electric
uniquify ediff-hook vc-hooks lisp-float-type mwheel dos-w32 ls-lisp
disp-table term/w32-win w32-win w32-vars term/common-win tool-bar dnd
fontset image regexp-opt fringe tabulated-list replace newcomment
text-mode elisp-mode lisp-mode prog-mode register page menu-bar
rfn-eshadow isearch timer select scroll-bar mouse jit-lock font-lock
syntax facemenu font-core term/tty-colors frame cl-generic cham georgian
utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao korean
japanese eucjp-ms cp51932 hebrew greek romanian slovak czech european
ethiopic indian cyrillic chinese composite charscript charprop
case-table epa-hook jka-cmpr-hook help simple abbrev obarray minibuffer
cl-preloaded nadvice loaddefs button faces cus-face macroexp files
text-properties overlay sha1 md5 base64 format env code-pages mule
custom widget hashtable-print-readable backquote w32notify w32 lcms2
multi-tty make-network-process emacs)

Memory information:
((conses 16 483260 59015)
 (symbols 56 42118 2)
 (miscs 48 767 834)
 (strings 32 109482 4257)
 (string-bytes 1 3118077)
 (vectors 16 43616)
 (vector-slots 8 835898 21082)
 (floats 8 349 609)
 (intervals 56 10467 182)
 (buffers 992 58))

bug#29935: 26.0.90; copyright-update adds year in middle of buffer

[Eli Zaretskii]

> Date: Mon, 01 Jan 2018 13:41:51 -0600
> From: Stephen Leake <stephen_leake@stephe-leake.org>
> 
> With copyright-update in before-save-hook, edit multiple files that need
> a copyright update, then execute save-some-buffers.
> 
> In some of the files, the new copyright year is inserted in the middle
> of the buffer (where point last was?), not in the copyright line.

This happens when save-window-excursion is used whose body switches
buffers.  This pattern should be avoided at all costs.

bug#29935: recipe and improved patch

[Stephen Leake]

[-- Attachment #1: Type: text/plain, Size: 1816 bytes --]

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

[-- Attachment #2: file_1.c-save --]
[-- Type: application/octet-stream, Size: 296 bytes --]

// file_1.c
//
// Copyright 2017 FSF
//
// lots of text to allow scrolling to get copyright line out of view
//
//
//
//
//
//
//
//
//
//
//
//
//
//
//
//
//
//
//
//
//
//
//
//
//
//
//
//
//
//
//
//
//
//
//
// ok, now copyright line is out of view

[-- Attachment #3: copyright.diff --]
[-- Type: text/x-patch, Size: 3976 bytes --]

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

bug#29935: recipe and improved patch

[martin rudalics]

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

Then the problem seems to be the familiar one: Calling
`switch-to-buffer' instead of `display-buffer-same-window'.

martin

bug#29935: recipe and improved patch

[Lars Ingebrigtsen]

martin rudalics <rudalics@gmx.at> writes:

>> 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.
>
> Then the problem seems to be the familiar one: Calling
> `switch-to-buffer' instead of `display-buffer-same-window'.

So would replacing those calls in the copyright-* functions fix these
problems?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#29935: recipe and improved patch

[martin rudalics]

 >> Then the problem seems to be the familiar one: Calling
 >> `switch-to-buffer' instead of `display-buffer-same-window'.
 >
 > So would replacing those calls in the copyright-* functions fix these
 > problems?

As a rule, applications should avoid calling 'switch-to-buffer' unless
they want to adhere to the default value or the user's customization
of 'switch-to-buffer-preserve-window-point'.  If that can be excluded,
replacing those calls should fix problems like the one seen here.

martin

bug#29935: recipe and improved patch

[Lars Ingebrigtsen]

Stephen Leake <stephen_leake@stephe-leake.org> writes:

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

I tried this in Emacs 28, and but I was unable to reproduce this bug.
Are you still seeing this?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#29935: recipe and improved patch

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> I tried this in Emacs 28, and but I was unable to reproduce this bug.
> Are you still seeing this?

There was no response in five weeks, so I'm closing this bug report.  If
this is still an issue, please respond to the debbugs address and we'll
reopen.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no