bug#23933: 25.1.50; Run a buffer-local hook with mapc

bug#23933: 25.1.50; Run a buffer-local hook with mapc

[Tino Calancha]

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


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@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;

bug#23933: 25.1.50; Run a buffer-local hook with mapc

[Eli Zaretskii]

> From: Tino Calancha <tino.calancha@gmail.com>
> Date: Sun, 10 Jul 2016 19:18:33 +0900 (JST)
> Cc: kifer@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?

bug#23933: 25.1.50; Run a buffer-local hook with mapc

[Stefan Monnier]

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

bug#23933: 25.1.50; Run a buffer-local hook with mapc

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Tino Calancha <tino.calancha@gmail.com>,  23933@debbugs.gnu.org,  kifer@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?

bug#23933: 25.1.50; Run a buffer-local hook with mapc

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Tino Calancha <tino.calancha@gmail.com>,  23933@debbugs.gnu.org,  kifer@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.

bug#23933: 25.1.50; Run a buffer-local hook with mapc

[Stefan Monnier]

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

bug#23933: 25.1.50; Run a buffer-local hook with mapc

[Stefan Monnier]

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

bug#23933: 25.1.50; Run a buffer-local hook with mapc

[npostavs]

Stefan Monnier <monnier@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)
   ))

bug#23933: 25.1.50; Run a buffer-local hook with mapc

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: tino.calancha@gmail.com,  23933@debbugs.gnu.org,  kifer@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?

bug#23933: 25.1.50; Run a buffer-local hook with mapc

[Tino Calancha]

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

bug#23933: 25.1.50; Run a buffer-local hook with mapc

[Noam Postavsky]

On Sun, Jul 10, 2016 at 1:31 PM, Tino Calancha <tino.calancha@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.

bug#23933: 25.1.50; Run a buffer-local hook with mapc

[Eli Zaretskii]

> From: Tino Calancha <tino.calancha@gmail.com>
> Date: Mon, 11 Jul 2016 02:31:52 +0900 (JST)
> cc: Stefan Monnier <monnier@iro.umontreal.ca>, tino.calancha@gmail.com, 
>     23933@debbugs.gnu.org, kifer@cs.stonybrook.edu, 
>     npostavs@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?

bug#23933: 25.1.50; Run a buffer-local hook with mapc

[Tino Calancha]

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?

bug#23933: 25.1.50; Run a buffer-local hook with mapc

[Andreas Schwab]

Eli Zaretskii <eliz@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@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

bug#23933: 25.1.50; Run a buffer-local hook with mapc

[Stefan Monnier]

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

bug#23933: 25.1.50; Run a buffer-local hook with mapc

[Tino Calancha]

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

bug#23933: 25.1.50; Run a buffer-local hook with mapc

[Tino Calancha]

On Tue, 12 Jul 2016, Tino Calancha wrote:

> Thanks also to Noam and Eli to helping fixing this bug.
And thanks also to Andreas.

bug#23933: (no subject)

[Tino Calancha]

Fixed in the master branch