GNU bug report logs -
#23933
25.1.50; Run a buffer-local hook with mapc
Previous Next
Reported by: Tino Calancha <tino.calancha <at> gmail.com>
Date: Sun, 10 Jul 2016 10:19:02 UTC
Severity: minor
Found in version 25.1.50
Done: Tino Calancha <tino.calancha <at> gmail.com>
Bug is archived. No further changes may be made.
To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 23933 in the body.
You can then email your comments to 23933 AT debbugs.gnu.org in the normal way.
Toggle the display of automated, internal messages from the tracker.
Report forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#23933; Package
emacs.
(Sun, 10 Jul 2016 10:19:02 GMT)
Full text and
rfc822 format available.
Acknowledgement sent
to
Tino Calancha <tino.calancha <at> gmail.com>:
New bug report received and forwarded. Copy sent to
bug-gnu-emacs <at> gnu.org.
(Sun, 10 Jul 2016 10:19:02 GMT)
Full text and
rfc822 format available.
Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Prevent calling (funcall t) when running a buffer-local hook
as (mapc #'funcall LOCAL-HOOK).
emacs -Q /tmp
M-! for f in foo bar foo-new bar-new; do echo $f>/tmp/$f;done RET
M-! for f in foo bar; do diff -u /tmp/$f /tmp/${f}-new >> /tmp/patch;done
RET
M-x epatch RET n patch RET C-k RET y n v q y
;; funcall: Symbol’s function definition is void: t
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
From de693cda8c6a174149b523fb4adcfae6d28bc202 Mon Sep 17 00:00:00 2001
From: Tino Calancha <tino.calancha <at> gmail.com>
Date: Sun, 10 Jul 2016 18:02:16 +0900
Subject: [PATCH] Run a buffer-local hook with mapc
* lisp/vc/ediff-util.el (ediff-really-quit): Avoid to apply funcall on 't'
(Bug#23933).
---
lisp/vc/ediff-util.el | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/lisp/vc/ediff-util.el b/lisp/vc/ediff-util.el
index a6b88d5..3d2b9a3 100644
--- a/lisp/vc/ediff-util.el
+++ b/lisp/vc/ediff-util.el
@@ -2522,7 +2522,8 @@ ediff-really-quit
(frame-selected-window warp-frame))
2 1))
- (mapc #'funcall after-quit-hook-internal)
+ ;; after-quit-hook-internal is buffer-local; see
`ediff-filegroup-action'.
+ (mapc (lambda (f) (or (eq f t) (funcall f))) after-quit-hook-internal)
))
;; Returns frame under mouse, if this frame is not a minibuffer
--
2.8.1
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
In GNU Emacs 25.1.50.5 (x86_64-pc-linux-gnu, GTK+ Version 3.20.6)
of 2016-07-10
Repository revision: 466ee1b3ea76425d201b5d59950e88251870c836
Ediff 2.81.5 of July 4, 2013;
Severity set to 'minor' from 'normal'
Request was from
Tino Calancha <tino.calancha <at> gmail.com>
to
control <at> debbugs.gnu.org.
(Sun, 10 Jul 2016 10:21:01 GMT)
Full text and
rfc822 format available.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#23933; Package
emacs.
(Sun, 10 Jul 2016 14:45:02 GMT)
Full text and
rfc822 format available.
Message #10 received at 23933 <at> debbugs.gnu.org (full text, mbox):
> From: Tino Calancha <tino.calancha <at> gmail.com>
> Date: Sun, 10 Jul 2016 19:18:33 +0900 (JST)
> Cc: kifer <at> cs.stonybrook.edu
>
> --- a/lisp/vc/ediff-util.el
> +++ b/lisp/vc/ediff-util.el
> @@ -2522,7 +2522,8 @@ ediff-really-quit
> (frame-selected-window warp-frame))
> 2 1))
>
> - (mapc #'funcall after-quit-hook-internal)
> + ;; after-quit-hook-internal is buffer-local; see
> `ediff-filegroup-action'.
> + (mapc (lambda (f) (or (eq f t) (funcall f))) after-quit-hook-internal)
> ))
Thanks, but why do we use mapc to run hooks? We used to have
run-hooks there.
Stefan, why did you make that change? The log message says just "use
lexical-binding"; is something wrong with run-hooks and
lexical-binding living together?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#23933; Package
emacs.
(Sun, 10 Jul 2016 15:20:02 GMT)
Full text and
rfc822 format available.
Message #13 received at 23933 <at> debbugs.gnu.org (full text, mbox):
>> - (mapc #'funcall after-quit-hook-internal)
>> + ;; after-quit-hook-internal is buffer-local; see `ediff-filegroup-action'.
>> + (mapc (lambda (f) (or (eq f t) (funcall f))) after-quit-hook-internal)
>> ))
> Thanks, but why do we use mapc to run hooks? We used to have
> run-hooks there.
after-quit-hook-internal is a variable, not a hook.
A hook is a symbol whose symbol-value slot holds a list of functions;
this happens to match a dynamically-bound variable, but not
a lexically-bound variable.
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#23933; Package
emacs.
(Sun, 10 Jul 2016 15:56:01 GMT)
Full text and
rfc822 format available.
Message #16 received at 23933 <at> debbugs.gnu.org (full text, mbox):
> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Cc: Tino Calancha <tino.calancha <at> gmail.com>, 23933 <at> debbugs.gnu.org, kifer <at> cs.stonybrook.edu
> Date: Sun, 10 Jul 2016 11:18:56 -0400
>
> >> - (mapc #'funcall after-quit-hook-internal)
> >> + ;; after-quit-hook-internal is buffer-local; see `ediff-filegroup-action'.
> >> + (mapc (lambda (f) (or (eq f t) (funcall f))) after-quit-hook-internal)
> >> ))
> > Thanks, but why do we use mapc to run hooks? We used to have
> > run-hooks there.
>
> after-quit-hook-internal is a variable, not a hook.
Then how come it includes t in its value?
> A hook is a symbol whose symbol-value slot holds a list of functions;
> this happens to match a dynamically-bound variable, but not
> a lexically-bound variable.
So you are saying we should convert that variable to a hook?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#23933; Package
emacs.
(Sun, 10 Jul 2016 15:58:02 GMT)
Full text and
rfc822 format available.
Message #19 received at 23933 <at> debbugs.gnu.org (full text, mbox):
> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Cc: Tino Calancha <tino.calancha <at> gmail.com>, 23933 <at> debbugs.gnu.org, kifer <at> cs.stonybrook.edu
> Date: Sun, 10 Jul 2016 11:18:56 -0400
>
> A hook is a symbol whose symbol-value slot holds a list of functions;
> this happens to match a dynamically-bound variable, but not
> a lexically-bound variable.
Maybe I misunderstand this, but are you saying a hook (and run-hooks)
cannot be used in a file that uses lexical-binding? If so, we had
better documented that.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#23933; Package
emacs.
(Sun, 10 Jul 2016 17:03:02 GMT)
Full text and
rfc822 format available.
Message #22 received at 23933 <at> debbugs.gnu.org (full text, mbox):
>> after-quit-hook-internal is a variable, not a hook.
> Then how come it includes t in its value?
A variable can very well hold a list with t inside. As to why/where was
this t added, I don't know. It's probably a good idea to try and track
it down. Maybe because the value of that var was copied from the value
of an actual hook.
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#23933; Package
emacs.
(Sun, 10 Jul 2016 17:03:03 GMT)
Full text and
rfc822 format available.
Message #25 received at 23933 <at> debbugs.gnu.org (full text, mbox):
>> A hook is a symbol whose symbol-value slot holds a list of functions;
>> this happens to match a dynamically-bound variable, but not
>> a lexically-bound variable.
> Maybe I misunderstand this, but are you saying a hook (and run-hooks)
> cannot be used in a file that uses lexical-binding? If so, we had
> better documented that.
No, the issue is simply that `run-hooks' takes a symbol as argument.
You can do (run-hooks '<mydynvar>) because (symbol-value '<mydynvar>)
works, but you can't do (run-hooks '<mylexvar>) because (symbol-value
'<mylexvar>) won't work.
Using `run-hooks' on a let-bound variable is just a bad idea (just like
mixing let-binding and buffer-local binding on the same variable is
asking for trouble).
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#23933; Package
emacs.
(Sun, 10 Jul 2016 17:13:01 GMT)
Full text and
rfc822 format available.
Message #28 received at 23933 <at> debbugs.gnu.org (full text, mbox):
Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>>> after-quit-hook-internal is a variable, not a hook.
>> Then how come it includes t in its value?
>
> [...] Maybe because the value of that var was copied from the value
> of an actual hook.
Yes:
(defun ediff-really-quit (reverse-default-keep-variants)
...
(let (...
(after-quit-hook-internal ediff-after-quit-hook-internal)
And as mentioned in the patch, ediff-filegroup-action calls add-hook
with non-nil LOCAL on ediff-after-quit-hook-internal.
- (mapc #'funcall after-quit-hook-internal)
+ ;; after-quit-hook-internal is buffer-local; see `ediff-filegroup-action'.
+ (mapc (lambda (f) (or (eq f t) (funcall f))) after-quit-hook-internal)
))
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#23933; Package
emacs.
(Sun, 10 Jul 2016 17:17:01 GMT)
Full text and
rfc822 format available.
Message #31 received at 23933 <at> debbugs.gnu.org (full text, mbox):
> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Cc: tino.calancha <at> gmail.com, 23933 <at> debbugs.gnu.org, kifer <at> cs.stonybrook.edu
> Date: Sun, 10 Jul 2016 13:01:55 -0400
>
> >> after-quit-hook-internal is a variable, not a hook.
> > Then how come it includes t in its value?
>
> A variable can very well hold a list with t inside. As to why/where was
> this t added, I don't know. It's probably a good idea to try and track
> it down. Maybe because the value of that var was copied from the value
> of an actual hook.
Tino, can you figure out why t is there? Perhaps the solution is
simply to remove it?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#23933; Package
emacs.
(Sun, 10 Jul 2016 17:33:02 GMT)
Full text and
rfc822 format available.
Message #34 received at 23933 <at> debbugs.gnu.org (full text, mbox):
On Sun, 10 Jul 2016, Eli Zaretskii wrote:
> Tino, can you figure out why t is there? Perhaps the solution is
> simply to remove it?
Just explained in previous e-mail by Noam.
I have tried another approach: drop the let binding
and call run-hooks over the actual hook, as follows:
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
diff --git a/lisp/vc/ediff-util.el b/lisp/vc/ediff-util.el
index a6b88d5..b50ac6d 100644
--- a/lisp/vc/ediff-util.el
+++ b/lisp/vc/ediff-util.el
@@ -2439,7 +2439,6 @@ ediff-really-quit
;; restore buffer mode line id's in buffer-A/B/C
(let ((control-buffer ediff-control-buffer)
(meta-buffer ediff-meta-buffer)
- (after-quit-hook-internal ediff-after-quit-hook-internal)
(session-number ediff-meta-session-number)
;; suitable working frame
(warp-frame (if (and (ediff-window-display-p) (eq ediff-grab-mouse
t))
@@ -2522,7 +2521,7 @@ ediff-really-quit
(frame-selected-window warp-frame))
2 1))
- (mapc #'funcall after-quit-hook-internal)
+ (run-hooks 'ediff-after-quit-hook-internal)
))
;; Returns frame under mouse, if this frame is not a minibuffer
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
but it doesn't work: the meta-buffer is not shown.
I prefer my initial patch which behaves the same as before
adding the lexical-binding.
Tino
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#23933; Package
emacs.
(Sun, 10 Jul 2016 17:42:02 GMT)
Full text and
rfc822 format available.
Message #37 received at 23933 <at> debbugs.gnu.org (full text, mbox):
On Sun, Jul 10, 2016 at 1:31 PM, Tino Calancha <tino.calancha <at> gmail.com> wrote:
> I have tried another approach: drop the let binding
> and call run-hooks over the actual hook, as follows:
[...]
> but it doesn't work: the meta-buffer is not shown.
> I prefer my initial patch which behaves the same as before
> adding the lexical-binding.
The easiest fix might be to just make after-quit-hook-internal a
dynamic variable (i.e., declare it with defvar). But perhaps better is
to stop using add-hook for ediff-after-quit-hook-internal, and just
use push.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#23933; Package
emacs.
(Sun, 10 Jul 2016 17:53:01 GMT)
Full text and
rfc822 format available.
Message #40 received at 23933 <at> debbugs.gnu.org (full text, mbox):
> From: Tino Calancha <tino.calancha <at> gmail.com>
> Date: Mon, 11 Jul 2016 02:31:52 +0900 (JST)
> cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, tino.calancha <at> gmail.com,
> 23933 <at> debbugs.gnu.org, kifer <at> cs.stonybrook.edu,
> npostavs <at> users.sourceforge.net
>
> > Tino, can you figure out why t is there? Perhaps the solution is
> > simply to remove it?
> Just explained in previous e-mail by Noam.
> I have tried another approach: drop the let binding
> and call run-hooks over the actual hook, as follows:
Can't we simply remove the t? If not, why not?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#23933; Package
emacs.
(Sun, 10 Jul 2016 18:15:02 GMT)
Full text and
rfc822 format available.
Message #43 received at 23933 <at> debbugs.gnu.org (full text, mbox):
On Sun, 10 Jul 2016, Eli Zaretskii wrote:
> Can't we simply remove the t? If not, why not?
Yes, we can and it is more readable:
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
diff --git a/lisp/vc/ediff-util.el b/lisp/vc/ediff-util.el
index a6b88d5..f98c2f4 100644
--- a/lisp/vc/ediff-util.el
+++ b/lisp/vc/ediff-util.el
@@ -2522,7 +2522,7 @@ ediff-really-quit
(frame-selected-window warp-frame))
2 1))
- (mapc #'funcall after-quit-hook-internal)
+ (mapc #'funcall (delq t after-quit-hook-internal))
))
;; Returns frame under mouse, if this frame is not a minibuffer
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
I also tried Noam suggestion about using push instead of add-hook on
ediff-after-quit-hook-internal, that works also OK:
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
diff --git a/lisp/vc/ediff-mult.el b/lisp/vc/ediff-mult.el
index 7f0db5d..a092218 100644
--- a/lisp/vc/ediff-mult.el
+++ b/lisp/vc/ediff-mult.el
@@ -1904,13 +1904,12 @@ ediff-filegroup-action
file1 file2
;; provide startup hooks
`(list (lambda ()
- (add-hook
- 'ediff-after-quit-hook-internal
+ (push
(lambda ()
(if (ediff-buffer-live-p
,(current-buffer))
(ediff-show-meta-buffer
,(current-buffer) ,session-number)))
- nil 'local)
+ ediff-after-quit-hook-internal)
(setq ediff-meta-buffer ,(current-buffer)
ediff-meta-session-number
,session-number)
@@ -1933,15 +1932,14 @@ ediff-filegroup-action
file1
;; provide startup hooks
`(list (lambda ()
- (add-hook
- 'ediff-after-quit-hook-internal
+ (push
(lambda ()
(if (ediff-buffer-live-p
,(current-buffer))
(ediff-show-meta-buffer
,(current-buffer)
,session-number)))
- nil 'local)
+ ediff-after-quit-hook-internal)
(setq ediff-meta-buffer ,(current-buffer)
ediff-meta-session-number
,session-number)
@@ -1962,15 +1960,14 @@ ediff-filegroup-action
file1 file2
;; provide startup hooks
`(list (lambda ()
- (add-hook
- 'ediff-after-quit-hook-internal
+ (push
(lambda ()
(if (ediff-buffer-live-p
,(current-buffer))
(ediff-show-meta-buffer
,(current-buffer)
,session-number)))
- nil 'local)
+ ediff-after-quit-hook-internal)
(setq ediff-meta-buffer ,(current-buffer)
ediff-meta-session-number
,session-number)
@@ -1991,15 +1988,14 @@ ediff-filegroup-action
file1 file2 file3
;; arrange startup hooks
`(list (lambda ()
- (add-hook
- 'ediff-after-quit-hook-internal
+ (push
(lambda ()
(if (ediff-buffer-live-p
,(current-buffer))
(ediff-show-meta-buffer
,(current-buffer)
,session-number)))
- nil 'local)
+ ediff-after-quit-hook-internal)
(setq ediff-merge-store-file
,(if (ediff-nonempty-string-p
merge-autostore-dir)
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
What aproach looks better?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#23933; Package
emacs.
(Sun, 10 Jul 2016 18:29:02 GMT)
Full text and
rfc822 format available.
Message #46 received at 23933 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
> Maybe I misunderstand this, but are you saying a hook (and run-hooks)
> cannot be used in a file that uses lexical-binding?
A hook is always a global variable.
Andreas.
--
Andreas Schwab, schwab <at> linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#23933; Package
emacs.
(Sun, 10 Jul 2016 21:13:02 GMT)
Full text and
rfc822 format available.
Message #49 received at 23933 <at> debbugs.gnu.org (full text, mbox):
> (defun ediff-really-quit (reverse-default-keep-variants)
> ...
> (let (...
> (after-quit-hook-internal ediff-after-quit-hook-internal)
> And as mentioned in the patch, ediff-filegroup-action calls add-hook
> with non-nil LOCAL on ediff-after-quit-hook-internal.
And there we have a problem. When t is encountered in the buffer-local
part of ediff-after-quit-hook-internal, it means to run the global part
of ediff-after-quit-hook-internal, but when we copy the list to
after-quit-hook-internal, this link is broken and run-hooks can't know
that a t in after-quit-hook-internal means to run the functions found in
the global part of ediff-after-quit-hook-internal.
Or looked at it another way, this let-binding does not copy the whole
content of the hook, only the buffer-local part of it which is not
sufficient when the hook has a buffer-local value as well as a non-nil
global value.
Most likely it works OK in practice because
ediff-after-quit-hook-internal typically has a nil global binding.
It might make sense to use something like:
;; FIXME: Here we ignore the global part of the
;; ediff-after-quit-hook-internal hook.
(after-quit-hook-internal (remq t ediff-after-quit-hook-internal))
-- Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#23933; Package
emacs.
(Mon, 11 Jul 2016 16:04:02 GMT)
Full text and
rfc822 format available.
Message #52 received at 23933 <at> debbugs.gnu.org (full text, mbox):
On Sun, 10 Jul 2016, Stefan Monnier wrote:
> It might make sense to use something like:
>
> ;; FIXME: Here we ignore the global part of the
> ;; ediff-after-quit-hook-internal hook.
> (after-quit-hook-internal (remq t ediff-after-quit-hook-internal))
Thank you very much Stefan for the detailed comments; they really help
me to understand deeply the issue.
I have pushed your suggested patch to the master branch.
Thanks also to Noam and Eli to helping fixing this bug.
Tino
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#23933; Package
emacs.
(Mon, 11 Jul 2016 16:06:01 GMT)
Full text and
rfc822 format available.
Message #55 received at 23933 <at> debbugs.gnu.org (full text, mbox):
On Tue, 12 Jul 2016, Tino Calancha wrote:
> Thanks also to Noam and Eli to helping fixing this bug.
And thanks also to Andreas.
Reply sent
to
Tino Calancha <tino.calancha <at> gmail.com>:
You have taken responsibility.
(Mon, 11 Jul 2016 16:07:02 GMT)
Full text and
rfc822 format available.
Notification sent
to
Tino Calancha <tino.calancha <at> gmail.com>:
bug acknowledged by developer.
(Mon, 11 Jul 2016 16:07:02 GMT)
Full text and
rfc822 format available.
Message #60 received at 23933-done <at> debbugs.gnu.org (full text, mbox):
Fixed in the master branch
bug archived.
Request was from
Debbugs Internal Request <help-debbugs <at> gnu.org>
to
internal_control <at> debbugs.gnu.org.
(Tue, 09 Aug 2016 11:24:03 GMT)
Full text and
rfc822 format available.
This bug report was last modified 8 years and 318 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.