bug#14095: 24.3.50; REGRESSION: `repeat' broken by use of `set-temporary-overlay-map'

bug#14095: 24.3.50; REGRESSION: `repeat' broken by use of `set-temporary-overlay-map'

[Drew Adams]

I believe this regression was introduced at the same time as the
regression of bug #12232 (which has been fixed).  The change that
introduced this problem was to use `set-temporary-overlay-map'.
 
The code for `repeat' before that change, e.g. the code from Emacs 23.4,
had this:
 
(when repeat-repeat-char
  (let (repeat-on-final-keystroke
        (undo-inhibit-record-point t))
    (setq real-last-command 'repeat)
    (setq repeat-undo-count 1)
    (unwind-protect
        (while (let ((evt (read-key)))
                 (eq (or (car-safe evt) evt)
                     (or (car-safe repeat-repeat-char)
                         repeat-repeat-char)))
          (repeat repeat-arg))
      ;; Make sure `repeat-undo-count' is reset.
      (setq repeat-undo-count nil))
    (setq unread-command-events (list last-input-event))))
 
That was changed to this (the current code):
 
(when repeat-repeat-char
  (set-temporary-overlay-map ; <===================
   (let ((map (make-sparse-keymap)))
     (define-key map (vector repeat-repeat-char)
       (if (null repeat-message-function) 'repeat
         (let ((fun repeat-message-function))
           (lambda ()
             (interactive)
             (let ((repeat-message-function fun))
               (setq this-command 'repeat)
               (setq real-this-command 'repeat)
               (call-interactively 'repeat))))))
     map)))
 
This change breaks the use of `repeat' in the following context, because
Isearch uses `overriding-terminal-local-map'.  Recipe:
 
emacs -Q

(defun isearchp-yank-line ()
    "Yank text from buffer up to end of line onto search string.
You can repeat this by hitting the last key again..."
    (interactive)
    (require 'repeat nil t)
    (isearchp-repeat-command 'isearch-yank-line))

(defun isearchp-repeat-command (command)
  "Repeat COMMAND."
  (let ((repeat-message-function  'ignore))
    (setq last-repeatable-command  command)
    (repeat nil)))

(define-key isearch-mode-map (kbd "C-y C-e")
            'isearchp-yank-line)
 
C-s  C-y  C-e  C-e  C-e
 
That works if the old definition of `repeat' is used.  It does not work
with the new definition, which uses `set-temporary-overlay-map', because
the temporary map is overruled by `overriding(-terminal)-local-map'.
 
Users should be able to use `repeat' in contexts such as this.  For
Isearch, for example, this lets you put multiple yank commands on the
same prefix key (e.g., `C-y').
 
For instance, I bind 8 yank commands to prefix C-y, 5 of which are
repeatable (i.e., they are repeatable in Emacs 22-23 and should also
be repeatable in later releases):
 
 C-y C-_   isearchp-yank-symbol-or-char
 C-y C-(   isearchp-yank-sexp-symbol-or-char
 C-y C-2   isearch-yank-secondary
 C-y C-c   isearchp-yank-char
 C-y C-e   isearchp-yank-line
 C-y C-w   isearchp-yank-word-or-char
 C-y C-y   isearch-yank-kill
 C-y M-y   isearch-yank-pop
 
(FWIW/FYI: It took me quite a while to figure out why this was not working.)

In GNU Emacs 24.3.50.1 (i386-mingw-nt5.1.2600)
 of 2013-03-23 on VBOX
Bzr revision: 112115 eliz@gnu.org-20130323093300-rjs0dgskxm9u0ya4
Windowing system distributor `Microsoft Corp.', version 5.1.2600
Configured using:
 `configure --with-gcc (4.7) --no-opt --enable-checking --cflags
 -IC:/emacs/libs/libXpm-3.5.10/include -IC:/emacs/libs/libXpm-3.5.10/src
 -IC:/emacs/libs/libpng-dev_1.4.3-1_win32/include
 -IC:/emacs/libs/zlib-dev_1.2.5-2_win32/include
 -IC:/emacs/libs/giflib-4.1.4-1-lib/include
 -IC:/emacs/libs/jpeg-6b-4-lib/include
 -IC:/emacs/libs/tiff-3.8.2-1-lib/include
 -IC:/emacs/libs/libxml2-2.7.8-w32-bin/include/libxml2
 -IC:/emacs/libs/gnutls-3.1.10-w32/include
 -IC:/emacs/libs/libiconv-1.14-2-mingw32-dev/include'
 

bug#14095: 24.3.50; REGRESSION: `repeat' broken by use of `set-temporary-overlay-map'

[Stefan Monnier]

> C-s  C-y  C-e  C-e  C-e
> That works if the old definition of `repeat' is used.  It does not work

This should work now, thank you,


        Stefan

bug#14095: 24.3.50; REGRESSION: `repeat' broken by use of `set-temporary-overlay-map'

[Drew Adams]

> > C-s  C-y  C-e  C-e  C-e
> > That works if the old definition of `repeat' is used.  It does not work
> 
> This should work now, thank you,

I want to say thank you!  But unfortunately there is still a problem,
in this build (which I believe includes your fix):

In GNU Emacs 24.3.50.1 (i686-pc-mingw32)
 of 2013-06-13 on ODIEONE
Bzr revision: 112978 xfq.free@gmail.com-20130613224333-3yfl8navh3c1vmxy
Windowing system distributor `Microsoft Corp.', version 6.1.7601
Configured using:
 `configure --prefix=/c/Devel/emacs/binary --enable-checking=yes,glyphs
 CFLAGS='-O0 -g3' CPPFLAGS='-Ic:/Devel/emacs/include'
 LDFLAGS='-Lc:/Devel/emacs/lib''

From `emacs -Q', I do this:

(defun isearchp-repeat-command (command)
  "..."
  (let ((repeat-message-function  'ignore))
    (setq last-repeatable-command  command)
    (repeat nil)))

(defun isearchp-yank-line ()
  "..."
  (interactive)
  (require 'repeat nil t)
  (isearchp-repeat-command 'isearch-yank-line))

(define-key isearch-mode-map "\C-y" nil)
(define-key isearch-mode-map (kbd "C-y C-e") 'isearchp-yank-line)

Things seem to work pretty much as they should, but if you do
`C-s C-y C-e C-e' then as soon as you hit the second `C-e' you
see this error msg: "Stack overflow in equal", which comes from
`set-temporary-overlay-map'.

With non-nil `debug-on-error' you get a backtrace like this:

Debugger entered--Lisp error: (error "Stack overflow in equal")
  add-hook(pre-command-hook #[0 "... [(keymap (5 . #[0 "..."
   [ignore repeat-message-function this-command real-this-command
    repeat call-interactively] 3 "\n\n(fn)" nil])) nil nil (#0)
    this-command nil t lookup-key this-command-keys-vector
    remove-hook pre-command-hook internal-pop-keymap
    overriding-terminal-local-map] 4 "\n\n(fn)"])

(I copied that backtrace from a session using my setup, not
emacs -Q, but it looks, superficially at least, the same as what
I see with emacs -Q.)

Hope this helps.  I will leave the bug closed for now.  Thanks
for working on this.

bug#14095: 24.3.50; REGRESSION: `repeat' broken by use of `set-temporary-overlay-map'

[Stefan Monnier]

> Things seem to work pretty much as they should, but if you do
> `C-s C-y C-e C-e' then as soon as you hit the second `C-e' you
> see this error msg: "Stack overflow in equal", which comes from
> `set-temporary-overlay-map'.

Good point, thanks.  I've installed a further fix (see below) which
should fix this problem.


        Stefan


=== modified file 'lisp/subr.el'
--- lisp/subr.el	2013-06-13 22:24:52 +0000
+++ lisp/subr.el	2013-06-14 04:08:45 +0000
@@ -3794,12 +3794,15 @@
                  (if (not load-file-name)
                      ;; Not being provided from a file, run func right now.
                      (funcall func)
-                   (let ((lfn load-file-name))
-                     (letrec ((fun (lambda (file)
+                   (let ((lfn load-file-name)
+                         ;; Don't use letrec, because equal (in
+                         ;; add/remove-hook) would get trapped in a cycle.
+                         (fun (make-symbol "eval-after-load-helper")))
+                     (fset fun (lambda (file)
                                      (when (equal file lfn)
                                        (remove-hook 'after-load-functions fun)
-                                       (funcall func)))))
-                       (add-hook 'after-load-functions fun))))))))
+                                   (funcall func))))
+                     (add-hook 'after-load-functions fun)))))))
         ;; Add FORM to the element unless it's already there.
         (unless (member delayed-func (cdr elt))
           (nconc elt (list delayed-func)))))))
@@ -4282,7 +4285,10 @@
 
 Optional ON-EXIT argument is a function that is called after the
 deactivation of MAP."
-  (letrec ((clearfun
+  (let ((clearfun (make-symbol "clear-temporary-overlay-map")))
+    ;; Don't use letrec, because equal (in add/remove-hook) would get trapped
+    ;; in a cycle.
+    (fset clearfun
             (lambda ()
               ;; FIXME: Handle the case of multiple temporary-overlay-maps
               ;; E.g. if isearch and C-u both use temporary-overlay-maps, Then
@@ -4298,7 +4304,7 @@
                             (t (funcall keep-pred)))
                 (remove-hook 'pre-command-hook clearfun)
                 (internal-pop-keymap map 'overriding-terminal-local-map)
-                (when on-exit (funcall on-exit))))))
+              (when on-exit (funcall on-exit)))))
     (add-hook 'pre-command-hook clearfun)
     (internal-push-keymap map 'overriding-terminal-local-map)))
 

bug#14095: 24.3.50; REGRESSION: `repeat' broken by use of `set-temporary-overlay-map'

[Drew Adams]

> Good point, thanks.  I've installed a further fix (see below) which
> should fix this problem.

Yes, the problem is fixed.  Thx.