occur-hook changing the current buffer

occur-hook changing the current buffer

[Juanma Barranquero]

In my `occur-hook' I have a custom function which does not save the
current buffer, so the last two lines of `occur-1', i.e.:

      (setq buffer-read-only t)
      (set-buffer-modified-p nil))))

where affecting the wrong buffer (the one searched by `occur', in fact).

Now, I can easily change my function to do the right thing, but I
still feel that `occur-1' should be fixed by using
`save-current-buffer' around the call to "(run-hooks 'occur-hook). The
rationale being that setting the modified flag to nil on the wrong
buffer can lead to data loss.

Opinions?

-- 
                    /L/e/k/t/u


Index: replace.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/replace.el,v
retrieving revision 1.212
diff -u -2 -r1.212 replace.el
--- replace.el	8 Jun 2005 22:37:13 -0000	1.212
+++ replace.el	21 Jun 2005 01:48:51 -0000
@@ -1002,5 +1002,6 @@
  		(setq next-error-last-buffer occur-buf))
  	    (kill-buffer occur-buf)))
-	(run-hooks 'occur-hook))
+        (save-current-buffer
+          (run-hooks 'occur-hook)))
       (setq buffer-read-only t)
       (set-buffer-modified-p nil))))

Re: occur-hook changing the current buffer

[Stefan Monnier]

> In my `occur-hook' I have a custom function which does not save the
> current buffer,

Sounds like a serious bug.  Such bugs can lead to loss of data.


        Stefan

Re: occur-hook changing the current buffer

[Juanma Barranquero]

> Sounds like a serious bug.  Such bugs can lead to loss of data.

Does that mean that you agree that occur mode should run occur-hook via

  (save-current-buffer
    (run-hooks 'occur-hook))

?

And BTW, occur-mode is a major mode. Shouldn't it use run-mode-hooks?

-- 
                    /L/e/k/t/u

Re: occur-hook changing the current buffer

[Stefan Monnier]

>> Sounds like a serious bug.  Such bugs can lead to loss of data.
> Does that mean that you agree that occur mode should run occur-hook via

No, it means I agree that your code is buggy and should thus be fixed.

>   (save-current-buffer
>     (run-hooks 'occur-hook))

Once your code is fixed this is not needed any more.  If the occur-hook is
expected to set-buffer in most/many cases, it might make sense, but
otherwise it doesn't.


        Stefan

Re: occur-hook changing the current buffer

[Juanma Barranquero]

> No, it means I agree that your code is buggy and should thus be fixed.

Why was my function buggy? Where is it documented that occur-hook
functions should not change the current buffer? Is that a generic
requirement for hook functions?

In this particular case, if `occur-1' didn't set the read-only and
not-modified flags after running the hook, the requirement wouldn't be
needed at all (and why enforce more than needed?).

> Once your code is fixed this is not needed any more.  If the occur-hook is
> expected to set-buffer in most/many cases, it might make sense, but
> otherwise it doesn't.

Well, I think it makes sense even if occur-hook is not expected to
change the current buffer in most/many cases. Just one is enough, if
it makes the user lose work.

-- 
                    /L/e/k/t/u

Re: occur-hook changing the current buffer

[Miles Bader]

Juanma Barranquero <lekktu@gmail.com> writes:
>> Once your code is fixed this is not needed any more.  If the occur-hook is
>> expected to set-buffer in most/many cases, it might make sense, but
>> otherwise it doesn't.
>
> Well, I think it makes sense even if occur-hook is not expected to
> change the current buffer in most/many cases. Just one is enough, if
> it makes the user lose work.

The hook-invoker shouldn't have to protect against every possible
bizarre action that a hook _might_ perform -- it's impossible to do, and
even trying to do it adds unnecessary overhead.

I find it hard to believe that anybody would expect changing the current
buffer in a hook _not_ to cause weird problems.  Returning from a
function with the current buffer changed is a fairly serious thing; you
should never do it without considering the effects on your caller.  This
is not something esoteric, it's elisp programming 101.

-Miles
-- 
Come now, if we were really planning to harm you, would we be waiting here,
 beside the path, in the very darkest part of the forest?

Re: occur-hook changing the current buffer

[Juanma Barranquero]

> The hook-invoker shouldn't have to protect against every possible
> bizarre action that a hook _might_ perform

No. Sure I didn't say that. Did I?

What I'm saying is: *some* hooks should protect against *some*
possible actions/errors; which ones it's something to ponder. *In this
case*, occur-hook should IMO protect against changing the buffer,
because *in this case*, not protecting against it could lead to
accidental data loss.

> -- it's impossible to do, and
> even trying to do it adds unnecessary overhead.

Of course.

> I find it hard to believe that anybody would expect changing the current
> buffer in a hook _not_ to cause weird problems.  Returning from a
> function with the current buffer changed is a fairly serious thing; you
> should never do it without considering the effects on your caller. 

What part of "my function changed the buffer by accident, not by
design" and "I could have lost data" is not clear?

> This
> is not something esoteric, it's elisp programming 101.

Thanks for the introductory class :)

-- 
                    /L/e/k/t/u

Re: occur-hook changing the current buffer

[Miles Bader]

Juanma Barranquero <lekktu@gmail.com> writes:
> What part of "my function changed the buffer by accident, not by
> design" and "I could have lost data" is not clear?

Emacs can't, and shouldn't try to, protect against random crap in hooks,
except where a particular mistake is very common and easy to make.
There's no evidence this is such a case.

-Miles
-- 
Next to fried food, the South has suffered most from oratory.
  			-- Walter Hines Page

Re: occur-hook changing the current buffer

[Juanma Barranquero]

> Emacs can't, and shouldn't try to, protect against random crap in hooks,
> except where a particular mistake is very common and easy to make.

I couldn't agree less with you. It is the consequences of the "random
crap" which should dictate whether it makes sense to protect against
it. Easy to make mistakes that cause no harm should perhaps be
avoided, but *if not*, a simple warning on the documentation would
suffice. Not so with not-that-common, but potentially more serious,
problems.

Moreover, in this particular case, the function which causes the
buffer switch was in the hook for a long time. It was a change to
`occur-1' (http://savannah.gnu.org/cgi-bin/viewcvs/emacs/emacs/lisp/replace.el.diff?r1=1.208&r2=1.209,
if you're wondering) which suddenly made a
yes-I-admit-erroneous-but-not-dangerous-till-now-function to suddenly
become dangerous. I'm not putting the "blame" on the change (which is,
AFAICS, correct), only stressing that when a function runs a hook and
*afterwards* sets the not-modified flag, protecting against the change
of the buffer is not a defense against "random crap". It's helping the
user not to shoot his own foot.

-- 
                    /L/e/k/t/u

Re: occur-hook changing the current buffer

[Juri Linkov]

> I'm not putting the "blame" on the change (which is, AFAICS,
> correct), only stressing that when a function runs a hook and
> *afterwards* sets the not-modified flag, protecting against the
> change of the buffer is not a defense against "random crap".

Why setting the not-modified flag comes after running a hook?  AFAIK,
there is a convention that run-hook should be the last element in the
function body, so users could override every default settings of the
function, including in this particular case setting the modification flag.
So it seems the correct fix is just to move (run-hooks 'occur-hook) down
to be the last funcall in the `occur-1' function body.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: occur-hook changing the current buffer

[Juanma Barranquero]

> Why setting the not-modified flag comes after running a hook?  AFAIK,
> there is a convention that run-hook should be the last element in the
> function body, so users could override every default settings of the
> function, including in this particular case setting the modification flag.

The Emacs Lisp Reference makes that claim about major-mode hooks and,
less strongly, about minor-mode hooks. Certainly I'm not aware of such
a convention for non-mode hooks, like `occur-hook', and I don't think
it would make sense: hooks are set up at convenient moments, which do
not necessarily coincide with function boundaries.

> So it seems the correct fix is just to move (run-hooks 'occur-hook) down
> to be the last funcall in the `occur-1' function body.

Hey, it's funny: if I had proposed to move the code setting the
read-only and not-modified flags to before run-hooks, instead of
protecting the hook functions from changing the current buffer,
perhaps no one would have complained... :)

-- 
                    /L/e/k/t/u

Re: occur-hook changing the current buffer

[Miles Bader]

Juanma Barranquero <lekktu@gmail.com> writes:
> Hey, it's funny: if I had proposed to move the code setting the
> read-only and not-modified flags to before run-hooks, instead of
> protecting the hook functions from changing the current buffer,
> perhaps no one would have complained... :)

Probably true...

-Miles
-- 
`Suppose Korea goes to the World Cup final against Japan and wins,' Moon said.
`All the past could be forgiven.'   [NYT]

Re: occur-hook changing the current buffer

[Juanma Barranquero]

> Probably true...

What about the problem with using `fit-window-to-buffer' in occur-hook?

I'd be much grateful for a comment from experts in the inner workings
of `fit-window-to-buffer'...

-- 
                    /L/e/k/t/u

Re: occur-hook changing the current buffer

[Emilio Lopes]

Juanma Barranquero <lekktu <at> gmail.com> writes:

> What about the problem with using `fit-window-to-buffer' in occur-hook?

Don't know if it helps you, but I have the following in my "~/.emacs" and it
seems to work:

(defun occur-shrink-window ()
  "*Shrink the \"*Occur*\" window as much as possible to display its contents."
  (let ((win (get-buffer-window "*Occur*")))
    (when (windowp win)
      (shrink-window-if-larger-than-buffer win))))
(add-hook 'occur-hook 'occur-shrink-window)

Re: occur-hook changing the current buffer

[Juanma Barranquero]

On 6/23/05, Emilio Lopes <eclig@gmx.net> wrote:

> Don't know if it helps you, but I have the following in my "~/.emacs" and it
> seems to work:
> 
> (defun occur-shrink-window ()
>   "*Shrink the \"*Occur*\" window as much as possible to display its contents."
>   (let ((win (get-buffer-window "*Occur*")))
>     (when (windowp win)
>       (shrink-window-if-larger-than-buffer win))))
> (add-hook 'occur-hook 'occur-shrink-window)

I have something similar:

  (defun lk-fit-window (&optional buffer min-height)
    "Resize window showing BUFFER to fit its contents."
    (interactive)
    (fit-window-to-buffer (get-buffer-window (or buffer (current-buffer)))
                          (if (integerp max-mini-window-height)
                              max-mini-window-height
                            (floor (* (frame-height) max-mini-window-height)))
                          (or min-height 2)))

  (add-hook 'occur-hook #'lk-fit-window)

I use lk-fit-window at other places on my .emacs, that's why it is
more complex than needed just for `occur-hook'.

The issue is that `fit-window-to-buffer', when called from
`occur-hook', changes the current buffer. Puzzling. Not a problem now,
because I've wrapped the call in lk-fit-window with
`save-current-buffer', but still, it seems like a bug to me.

Just out of curiosity: do you usually work in a just-one-window
configuration? I don't like having to make C-x 1 continuously, so I've
set up help modes (and everything else that could be customized to do
so) to be "electric": use, press "q", and you're back to one window.
About the only time I use two windows is on inferior modes (when
running IELM, a Common Lisp, Haskell, MySQL, etc.), and even then I've
setup a way to show the inferior modes in a window (about a third of
the frame) on the lower, and I've set up F12 to switch between
bringing this window up and burying.

-- 
                    /L/e/k/t/u

Re: occur-hook changing the current buffer

[Jason Rumney]

Juanma Barranquero wrote:

>The issue is that `fit-window-to-buffer', when called from
>`occur-hook', changes the current buffer.
>
Yes, this seems like a real bug. I have noticed some other strange 
behaviour lately with things taking place in the wrong buffer.

I use BBDB along with Gnus, and when I quit Gnus, there is a hook to 
offer to save ~/.bbdb if it has changed. Recently, this started to 
prompt me for the filename to save to, which I thought was strange, and 
when I investigated I discovered it was trying to save the Gnus 
"*Groups*" buffer instead of ~/.bbdb. I tried to trace through, but this 
just resulted in the correct behaviour.

Another place I have noticed this is auto-insert. Sometimes (not 
consistently), when executing C-x C-f new-file.h, the auto-insert text 
for new-file.h is inserted into the buffer that was current before the 
C-x C-f, and that buffer stays current. Again, whenever I have tried to 
trace through the code, everything works perfectly.

Both of these also involve hooks, which leads me to think something has 
changed with the ways hooks are run. If we continue to insist that it is 
the hooks that are wrong, not Emacs, then this will cause a lot of 
disruption to users.

Re: occur-hook changing the current buffer

[Juanma Barranquero]

> Yes, this seems like a real bug.

And very weird. I traced the call, and the buffer changed on return of
`save-selected-window'. Nothing that I could see seemed like a likely
cause.

> If we continue to insist that it is
> the hooks that are wrong, not Emacs, then this will cause a lot of
> disruption to users.

Yes!

-- 
                    /L/e/k/t/u

Re: occur-hook changing the current buffer

[Lute Kamstra]

Jason Rumney <jasonr@gnu.org> writes:

> Juanma Barranquero wrote:
>
>>The issue is that `fit-window-to-buffer', when called from
>>`occur-hook', changes the current buffer.
>>
> Yes, this seems like a real bug. I have noticed some other strange
> behaviour lately with things taking place in the wrong buffer.
>
> I use BBDB along with Gnus, and when I quit Gnus, there is a hook to
> offer to save ~/.bbdb if it has changed. Recently, this started to
> prompt me for the filename to save to, which I thought was strange,
> and when I investigated I discovered it was trying to save the Gnus
> "*Groups*" buffer instead of ~/.bbdb. I tried to trace through, but
> this just resulted in the correct behaviour.
>
> Another place I have noticed this is auto-insert. Sometimes (not
> consistently), when executing C-x C-f new-file.h, the auto-insert text
> for new-file.h is inserted into the buffer that was current before the
> C-x C-f, and that buffer stays current. Again, whenever I have tried
> to trace through the code, everything works perfectly.
>
> Both of these also involve hooks, which leads me to think something
> has changed with the ways hooks are run. If we continue to insist that
> it is the hooks that are wrong, not Emacs, then this will cause a lot
> of disruption to users.

You are drawing conclusions on a hunch.  Maybe it's better to first
get a good understanding of the problems you're experiencing.

Lute.

Re: occur-hook changing the current buffer

[Juanma Barranquero]

On 6/23/05, Lute Kamstra <Lute.Kamstra.lists@xs4all.nl> wrote:

> Maybe it's better to first
> get a good understanding of the problems you're experiencing.

Well, we have at least one case of a fully reproducible bug where
running a function from a hook leads to an unexpected switch of the
current buffer. That's a starting point.

-- 
                    /L/e/k/t/u

Re: occur-hook changing the current buffer

[Lute Kamstra]

Juanma Barranquero <lekktu@gmail.com> writes:

> On 6/23/05, Lute Kamstra <Lute.Kamstra.lists@xs4all.nl> wrote:
>
>> Maybe it's better to first get a good understanding of the problems
>> you're experiencing.
>
> Well, we have at least one case of a fully reproducible bug where
> running a function from a hook leads to an unexpected switch of the
> current buffer. That's a starting point.

You mean the fit-window-to-buffer call, right?  That seems like a bug
in fit-window-to-buffer, not a bug in the way hooks are run.

Lute.

Re: occur-hook changing the current buffer

[Juanma Barranquero]

> You mean the fit-window-to-buffer call, right?  That seems like a bug
> in fit-window-to-buffer, not a bug in the way hooks are run.

Yes. Still, I tried to duplicate the problem by running it manually,
not from a hook, and couldn't. That doesn't mean it is a problem with
hooks, of course, but perhaps something changed in hooks that makes
triggering these kinds of bugs easier.

-- 
                    /L/e/k/t/u

Re: occur-hook changing the current buffer

[Richard M. Stallman]

    Why setting the not-modified flag comes after running a hook?  AFAIK,
    there is a convention that run-hook should be the last element in the
    function body, so users could override every default settings of the
    function, including in this particular case setting the modification flag.

There is no such convention, and many hooks are run from the middle
of a function.  However, you may have a valid point that, all else
being equal, it is usually better to run the hook at the end.

Re: occur-hook changing the current buffer

[Juanma Barranquero]

I've moved the code that sets `buffer-read-only' and the
buffer-modified flag to just before running `occur-hook'. This won't
fix the problem Jason and I are reporting, but at least reduces the
probability of someone losing data.

Any fix of the problem, or better reorganization of `occur-1' , are of
course welcome.

-- 
                    /L/e/k/t/u

Re: occur-hook changing the current buffer

[Juri Linkov]

> I've moved the code that sets `buffer-read-only' and the
> buffer-modified flag to just before running `occur-hook'. This won't
> fix the problem Jason and I are reporting, but at least reduces the
> probability of someone losing data.
>
> Any fix of the problem, or better reorganization of `occur-1' , are of
> course welcome.

Perhaps this change should be documented in the `Incompatible
Lisp Changes in Emacs 22.1' in etc/NEWS.  When users have hooks
that modify the contents of the *Occur* buffer then these hooks
will fail after the read-only flag was set on the *Occur* buffer.

A similar change made in compile.el a half year ago (that sets the
read-only flag before running `compilation-mode-hook') could be
mentioned in the same section of etc/NEWS too.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: occur-hook changing the current buffer

[Juanma Barranquero]

> Perhaps this change should be documented in the `Incompatible
> Lisp Changes in Emacs 22.1' in etc/NEWS.  When users have hooks
> that modify the contents of the *Occur* buffer then these hooks
> will fail after the read-only flag was set on the *Occur* buffer.

On one hand, `occur-hook' didn't even exist in 21.4.

On the other hand, `occur-hook' runs inside a (let ((inhibit-read-only t))...)

And finally, I think previously the hook ran after setting the buffer
read-only, till Eli's modification of a while back.

-- 
                    /L/e/k/t/u

Re: occur-hook changing the current buffer

[Richard M. Stallman]

    Perhaps this change should be documented in the `Incompatible
    Lisp Changes in Emacs 22.1' in etc/NEWS.  When users have hooks
    that modify the contents of the *Occur* buffer then these hooks
    will fail after the read-only flag was set on the *Occur* buffer.

We should not clutter NEWS with minor changes in details that were
never documented, that have only a small chance of affecting anyone.
NEWS is hard to absorb because it is so big.

Re: occur-hook changing the current buffer

[Werner LEMBERG]

> NEWS is hard to absorb because it is so big.

What about a table of contents and enumerating the sections,
subsections, and subsubsections?


    Werner

Re: occur-hook changing the current buffer

[Juri Linkov]

>> NEWS is hard to absorb because it is so big.
>
> What about a table of contents and enumerating the sections,
> subsections, and subsubsections?

In Emacs, NEWS is visited in outline-mode, so after typing `C-c C-t' or
`C-c C-q' the whole file looks like a table of contents.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: occur-hook changing the current buffer

[Werner LEMBERG]

> > What about a table of contents and enumerating the sections,
> > subsections, and subsubsections?
> 
> In Emacs, NEWS is visited in outline-mode, so after typing `C-c C-t'
> or `C-c C-q' the whole file looks like a table of contents.

I know that, but

  . RMS and others don't like to activate outline mode by default for
    this file.

  . it is still not enumerated.

  . there's a high chance that the NEWS file is read with a different
    editor which almost certainly doesn't have an equivalent to
    outline mode.


  Werner

Re: occur-hook changing the current buffer

[Richard M. Stallman]

    What about a table of contents and enumerating the sections,
    subsections, and subsubsections?

I don't know if a table of contents would help at all.
The problem is not the number of different pages
but the number of different items.  Likewise I don't see that
giving numbers to the items would help.

Re: occur-hook changing the current buffer

[Werner LEMBERG]

>     What about a table of contents and enumerating the sections,
>     subsections, and subsubsections?
> 
> I don't know if a table of contents would help at all.  The problem
> is not the number of different pages but the number of different
> items.

??? I don't understand.  Please try again.

> Likewise I don't see that giving numbers to the items would help.

It would help users to navigate in the NEWS file in case there is no
outline-mode.  Just compare it with, say, a big HOWTO file for
GNU/Linux.  If I find in the TOC that the I have to go to section
1.2.3, I just do a forward search to this item.  Additionally, while
doing a quick scrolling down, regularly increased section numbers
help the eye to find a section more easily.


    Werner

Re: occur-hook changing the current buffer

[Richard M. Stallman]

    > I don't know if a table of contents would help at all.  The problem
    > is not the number of different pages but the number of different
    > items.

    ??? I don't understand.  Please try again.

There are so many different change items in NEWS that people
are likely to get tired of reading it all.

    > Likewise I don't see that giving numbers to the items would help.

    It would help users to navigate in the NEWS file in case there is no
    outline-mode.  Just compare it with, say, a big HOWTO file for
    GNU/Linux.

Sorry, I don't understand.

Re: occur-hook changing the current buffer

[Juanma Barranquero]

> Emacs can't, and shouldn't try to, protect against random crap in hooks,

Excuse me if I find this quite funny.

  emacs -Q

  C-x b test RET
  aaaa RET
  bbbb RET
  bbbb RET
  cccc RET
 C-x 2
 C-x o
  M-x ielm RET
  ;; define hook in ielm
 (add-hook 'occur-hook
		 '(lambda ()
		    (let ((w (get-buffer-window "*Occur*")))
		      (message "Before: B(%S), W(%S)" (current-buffer) (selected-window))
		      (fit-window-to-buffer w)
		      (message "After : B(%S), W(%S)" (current-buffer) (selected-window)))))
C-x o
M-x occur RET
bbbb RET

 => Buffer test is read-only
 => *Messages* has:
Before: B(#<buffer *Occur*>), W(#<window 3 on test>)
After : B(#<buffer test>), W(#<window 3 on test>)

What's exactly "crappy" about the hook function? I don't see anything
in `fit-window-to-buffer 'docs that suggest it should produce this
effect...

-- 
                    /L/e/k/t/u

Re: occur-hook changing the current buffer

[Richard M. Stallman]

    And BTW, occur-mode is a major mode. Shouldn't it use run-mode-hooks?

It already does, to run occur-mode-hook.
occur-hook is different.

Re: occur-hook changing the current buffer

[Juanma Barranquero]

> It already does, to run occur-mode-hook.
> occur-hook is different.

Ah, you're right. Silly me.

-- 
                    /L/e/k/t/u

Re: occur-hook changing the current buffer

[Richard M. Stallman]

    Now, I can easily change my function to do the right thing, but I
    still feel that `occur-1' should be fixed by using
    `save-current-buffer' around the call to "(run-hooks 'occur-hook). The
    rationale being that setting the modified flag to nil on the wrong
    buffer can lead to data loss.

You may as well install the patch.  It can't hurt, and it can
prevent confusion.