Strange problem with emacs-lisp/copyright.el

Strange problem with emacs-lisp/copyright.el

[Lele Gaifax]

Hi all,

since a couple of week I switched my main development environment to Emacs
master, and I'm quite happy.

The only problem I had is one strange defect I'm hitting that, despite all my
efforts to reproduce it, I'm still unable to understand the nature. It seems
very similar to bug#2209, now archived.

In my setup I've configured

  (add-hook 'before-save-hook #'copyright-update)

to automatically update the copyright years range in files header. Accordingly
to the VC tool, I installed that setup more than one year ago and never
noticed any problem, with Emacs 25.

With the new Emacs, when I save multiple files that wasn't yet touched in the
current year, *sometime* and *some files* the new year is added in the wrong
location, that is the new year (that is, the string ", 2017") gets added
somewhere in the file, not where it should go: most of the times at the
beginning of a line, sometime in the middle, in any case apparently at the
`point' location, where the cursor was at save time.

As said, I've tried several times to understand what's going on, either with
edebug-ing the `copyright-update-year' function or decorating it with a few
`message' calls to emit the updated location and such, but it seems some sort
of eisenbug... I even reverted my source tree to the exact state before this
happened, and applied the very same set of mechanical replacements (I was
fixing a single typo and I used wgrep to spot and correct all instances), with
no luck.

Do this ring any bell, or can some one hints me on a better/more effective way
to investigate the issue, before opening a new bug report?

Thanks in advance,
ciao, lele.
-- 
nickname: Lele Gaifax | Quando vivrò di quello che ho pensato ieri
real: Emanuele Gaifas | comincerò ad aver paura di chi mi copia.
lele@metapensiero.it  |                 -- Fortunato Depero, 1929.

Re: Strange problem with emacs-lisp/copyright.el

[Alan Third]

On Tue, Apr 11, 2017 at 08:53:21AM +0200, Lele Gaifax wrote:
> 
> The only problem I had is one strange defect I'm hitting that, despite all my
> efforts to reproduce it, I'm still unable to understand the nature. It seems
> very similar to bug#2209, now archived.

There’s also bug#7179 which is similar, but it doesn’t sound like it’s
the same problem.

https://debbugs.gnu.org/cgi/bugreport.cgi?bug=7179

-- 
Alan Third

Re: Strange problem with emacs-lisp/copyright.el

[Lele Gaifax]

Some more details on the issue: I added two messages to the workhorse
function, trying to spot where the "point" moves to the wrong location:

(defun copyright-update-year (replace noquery)
  ;; This uses the match-data from copyright-find-copyright/end.
  (goto-char (match-end 1))
  (copyright-find-end)
  ;; LELE
  (message "copyright-find-end says %d" (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
              (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
          ;; LELE
          (message "Updating copyright of %s at point %d" (buffer-file-name) (point))
          (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))))))))

With this in place, I see the following messages when the error happens (I
shortened the filenames for readability):

Saving file /home/lele/[omissis]/grid/Custom.js...
copyright-find-end says 277
Add 2017 to copyright? (y or n) y
Updating copyright of /home/lele/[omissis]/grid/Custom.js at point 5912
Wrote ‘/home/lele/[omissis]/grid/Custom.js’ (11117 characters)
Saving file /home/lele/[omissis]/data/MetaData.js...
copyright-find-end says 288
Add 2017 to copyright? (y or n) y
Updating copyright of /home/lele/[omissis]/data/MetaData.js at point 12155
Wrote ‘/home/lele/[omissis]/data/MetaData.js’ (27857 characters)

As you can see, it seems that the (save-window-excursion ...) that asks
confirmation restores the point where I left it before saving the buffer, not
where (copyright-find-end) moved it.

Does this bring any new information on the issue?

I will try to store the point determined by copyright-find-end in a local
variable and explicitly move there in the inner progn, but I'm obviously
missing... the point :)

Thank you,
ciao, lele.
-- 
nickname: Lele Gaifax | Quando vivrò di quello che ho pensato ieri
real: Emanuele Gaifas | comincerò ad aver paura di chi mi copia.
lele@metapensiero.it  |                 -- Fortunato Depero, 1929.

Re: Strange problem with emacs-lisp/copyright.el

[Lele Gaifax]

Lele Gaifax <lele@metapensiero.it> writes:

> I will try to store the point determined by copyright-find-end in a local
> variable and explicitly move there in the inner progn, but I'm obviously
> missing... the point :)

FYI, since a couple of days I'm using this override, that apparently fixes my
issue:

  (defun copyright-update-year (replace noquery)
    ;; 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
                  (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"))))
                  (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)))))))))

It just saves and restore the point position found by `copyright-find-end': I
tried to understand why the `save-excursion' isn't enough, but failed.

All the best,
ciao, lele.
-- 
nickname: Lele Gaifax | Quando vivrò di quello che ho pensato ieri
real: Emanuele Gaifas | comincerò ad aver paura di chi mi copia.
lele@metapensiero.it  |                 -- Fortunato Depero, 1929.

Re: Strange problem with emacs-lisp/copyright.el

[Johan Bockgård]

Lele Gaifax <lele@metapensiero.it> writes:

> It just saves and restore the point position found by `copyright-find-end': I
> tried to understand why the `save-excursion' isn't enough, but failed.

Does it work correctly if you change

  (switch-to-buffer (current-buffer))
  ;; Fixes some point-moving oddness (bug#2209).
  (save-excursion
    (y-or-n-p (if replace

to

  (save-excursion
    (switch-to-buffer (current-buffer))
    (y-or-n-p (if replace

?

Re: Strange problem with emacs-lisp/copyright.el

[Lele Gaifax]

Johan Bockgård <bojohan@gnu.org> writes:

> Lele Gaifax <lele@metapensiero.it> writes:
>
>> It just saves and restore the point position found by `copyright-find-end': I
>> tried to understand why the `save-excursion' isn't enough, but failed.
>
> Does it work correctly if you change
>
>   (switch-to-buffer (current-buffer))
>   ;; Fixes some point-moving oddness (bug#2209).
>   (save-excursion
>     (y-or-n-p (if replace
>
> to
>
>   (save-excursion
>     (switch-to-buffer (current-buffer))
>     (y-or-n-p (if replace
>
> ?

I will try that and report back. As said, unfortunately I was not able to
reproduce the issue in a deterministic way.

Thank you,
ciao, lele.
-- 
nickname: Lele Gaifax | Quando vivrò di quello che ho pensato ieri
real: Emanuele Gaifas | comincerò ad aver paura di chi mi copia.
lele@metapensiero.it  |                 -- Fortunato Depero, 1929.

Re: Strange problem with emacs-lisp/copyright.el

[Lele Gaifax]

Lele Gaifax <lele@metapensiero.it> writes:

> Johan Bockgård <bojohan@gnu.org> writes:
>
>> Lele Gaifax <lele@metapensiero.it> writes:
>>
>>> It just saves and restore the point position found by `copyright-find-end': I
>>> tried to understand why the `save-excursion' isn't enough, but failed.
>>
>> Does it work correctly if you change
>>
>>   (switch-to-buffer (current-buffer))
>>   ;; Fixes some point-moving oddness (bug#2209).
>>   (save-excursion
>>     (y-or-n-p (if replace
>>
>> to
>>
>>   (save-excursion
>>     (switch-to-buffer (current-buffer))
>>     (y-or-n-p (if replace
>>
>> ?
>
> I will try that and report back. As said, unfortunately I was not able to
> reproduce the issue in a deterministic way.

As said, I installed your proposed fix and it seems to be effective: neither
my explicit attempts to reproduce the issue, nor my normal workflow did
exhibit the problem in the last two weeks.

Thank you again,
ciao, lele.
-- 
nickname: Lele Gaifax | Quando vivrò di quello che ho pensato ieri
real: Emanuele Gaifas | comincerò ad aver paura di chi mi copia.
lele@metapensiero.it  |                 -- Fortunato Depero, 1929.

Re: Strange problem with emacs-lisp/copyright.el

[Lele Gaifax]

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

Hi all,

this is just to properly end the thread [1], and ask whether I should open an
issue, or add an annotation to the existing (but not exactly the same) issue
#7179 [2], or what.

Back in April I applied Johan Bockgård's suggestion and since then I haven't
seen the reported problem again:

>> Johan Bockgård <bojohan@gnu.org> writes:
>>
>>>
>>> Does it work correctly if you change
>>>
>>>   (switch-to-buffer (current-buffer))
>>>   ;; Fixes some point-moving oddness (bug#2209).
>>>   (save-excursion
>>>     (y-or-n-p (if replace
>>>
>>> to
>>>
>>>   (save-excursion
>>>     (switch-to-buffer (current-buffer))
>>>     (y-or-n-p (if replace
>>>
>>> ?

To summarize, I shuffled just a couple of lines: the attached `diff -u` does
not make that clear, but hopefully the following `diff -u --ignore-space-change` 
gives a better idea since it ignores the indentation changes:

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

thanks&bye, lele.

[1] http://lists.gnu.org/archive/html/emacs-devel/2017-04/msg00271.html
[2] https://debbugs.gnu.org/cgi/bugreport.cgi?bug=7179


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: copyright.el.patch --]
[-- Type: text/x-diff, Size: 4607 bytes --]

--- copyright-orig.el	2017-10-31 18:54:20.957330092 +0100
+++ copyright.el	2017-10-31 18:55:11.624754452 +0100
@@ -181,44 +181,47 @@
   ;; 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
+              (save-window-excursion
+                (save-excursion
+                  (switch-to-buffer (current-buffer))
+                ;; Fixes some point-moving oddness (bug#2209).
+                  (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"))))
+                (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)

[-- Attachment #3: Type: text/plain, Size: 206 bytes --]


-- 
nickname: Lele Gaifax | Quando vivrò di quello che ho pensato ieri
real: Emanuele Gaifas | comincerò ad aver paura di chi mi copia.
lele@metapensiero.it  |                 -- Fortunato Depero, 1929.

Re: Strange problem with emacs-lisp/copyright.el

[Noam Postavsky]

On Tue, Oct 31, 2017 at 2:14 PM, Lele Gaifax <lele@metapensiero.it> wrote:

> this is just to properly end the thread [1], and ask whether I should open an
> issue, or add an annotation to the existing (but not exactly the same) issue
> #7179 [2], or what.

Please don't add to #7179, it's not the same bug. Making a new bug
should be fine.

>   +                (save-excursion
>                  (switch-to-buffer (current-buffer))
>                  ;; Fixes some point-moving oddness (bug#2209).
>   -              (save-excursion

Keep the comment with the save-excursion though.

>   +            (goto-char copyright-end)

Is this part really still needed?

Re: Strange problem with emacs-lisp/copyright.el

[Johan Bockgård]

Lele Gaifax <lele@metapensiero.it> writes:

> Hi all,
>
> this is just to properly end the thread [1], and ask whether I should open an
> issue, or add an annotation to the existing (but not exactly the same) issue
> #7179 [2], or what.
>
> Back in April I applied Johan Bockgård's suggestion and since then I haven't
> seen the reported problem again:

I've been intending to check in the fix below. Sorry for not getting
around to it sooner. I don't think we need a new bug report.


    copyright.el: Avoid inadvertent point motion
    
    * lisp/emacs-lisp/copyright.el (copyright-update-year): Enlarge the
    scope of save-excursion. Reported in:
    https://lists.gnu.org/archive/html/emacs-devel/2017-04/msg00271.html

diff --git a/lisp/emacs-lisp/copyright.el b/lisp/emacs-lisp/copyright.el
index 11569e4..25dc77c 100644
--- a/lisp/emacs-lisp/copyright.el
+++ b/lisp/emacs-lisp/copyright.el
@@ -186,9 +186,10 @@ (defun copyright-update-year (replace noquery)
                    (substring copyright-current-year -2))
     (if (or noquery
             (save-window-excursion
-             (switch-to-buffer (current-buffer))
-              ;; Fixes some point-moving oddness (bug#2209).
+              ;; switch-to-buffer might move point when
+              ;; switch-to-buffer-preserve-window-point is non-nil.
               (save-excursion
+                (switch-to-buffer (current-buffer))
                 (y-or-n-p (if replace
                               (concat "Replace copyright year(s) by "
                                       copyright-current-year "? ")