bug#6454: 24.0.50; kill-buffer switches current-buffer

bug#6454: 24.0.50; kill-buffer switches current-buffer

[Helmut Eller]

The third assertion in this code fails:

(eval-when-compile
  (require 'cl))  ; for assert

(let ((buffer (generate-new-buffer "test"))
      (saved-buffer (current-buffer)))
  (unwind-protect
      (with-current-buffer buffer
	(goto-line 1 buffer))
    (assert (and buffer (not (eq buffer (current-buffer)))))
    (assert (eq (current-buffer) saved-buffer) () "before kill-buffer")
    (kill-buffer buffer)
    (assert (eq (current-buffer) saved-buffer) () "after kill-buffer")))

Since the argument to kill-buffer is not the current-buffer, there's no
reason to switch that.  Seems like a bug to me.

Also note that if the buffer argument to goto-line is nil then
kill-buffer works as expected.

In GNU Emacs 24.0.50.5 (i686-pc-linux-gnu, GTK+ Version 2.12.12)
 of 2010-06-12 on ix

bug#6454: 24.0.50; kill-buffer switches current-buffer

[martin rudalics]

 > 	(goto-line 1 buffer))

makes sure there's a window showing BUFFER and selects that window.

 >     (kill-buffer buffer)

detects that the window showing BUFFER is the selected window, shows
another buffer in that window, and makes the other buffer current.

Tha according lines in window_loop are:

     Fset_window_buffer (window, buffer, Qnil);
     if (EQ (window, selected_window))
       Fset_buffer (w->buffer);

with the most recent change

1989-10-21  Richard Stallman  (rms@sugar-bombs.ai.mit.edu)

	* window.c (window_loop): For UNSHOW_BUFFER, don't Fset_buffer
	unless window is the selected one.

So this is another incarnation of the "buffer shown in the selected
window is not necessarily the current buffer" dichotomy.  Looks like
a bug but I'm not sure whether fixing it could break other things.

martin

bug#6454: 24.0.50; kill-buffer switches current-buffer

[Stefan Monnier]

> So this is another incarnation of the "buffer shown in the selected
> window is not necessarily the current buffer" dichotomy.

I know that kill-buffer should not change current-buffer.  It does do so
in corner cases and that's a bug (which I've already worked around in
some parts of the Elisp code because I couldn't figure out how to fix
Fkill_buffer instead).

> Looks like a bug but I'm not sure whether fixing it could break
> other things.

If the fix only affects kill-buffer, then it's likely safe.


        Stefan

bug#6454: 24.0.50; kill-buffer switches current-buffer

[martin rudalics]

 > I know that kill-buffer should not change current-buffer.

What about C-x k  ;-)

 > It does do so
 > in corner cases and that's a bug (which I've already worked around in
 > some parts of the Elisp code because I couldn't figure out how to fix
 > Fkill_buffer instead).
 >
 >> Looks like a bug but I'm not sure whether fixing it could break
 >> other things.
 >
 > If the fix only affects kill-buffer, then it's likely safe.

It's not that trivial.  Fkill_buffer already calls Fset_buffer before
eventually calling it again in window_loop.  If the first call fails to
produce a good buffer, the buffer in the selected window might be the
better choice.

So we would have to check whether we want to kill the current buffer and
not change it if we don't.  Unfortunately, there's no proof that the
buffer current when calling `kill-buffer' is the buffer considered
current by the outermost calling application.  It might be just current
because some intermittent part (the actual caller of `kill-buffer')
wanted to have a look at the buffer's local variables.  So `kill-buffer'
can't be sure about the "real identity of the current buffer" either.

martin

bug#6454: 24.0.50; kill-buffer switches current-buffer

[Stefan Monnier]

>> I know that kill-buffer should not change current-buffer.
> What about C-x k  ;-)

C-x k is no exception: the kill-buffer function there should not (and
usually will not) change current-buffer (other than in the obvious case
where we kill the current-buffer, obviously).

> So we would have to check whether we want to kill the current buffer and
> not change it if we don't.

That's right.

> Unfortunately, there's no proof that the buffer current when calling
> `kill-buffer' is the buffer considered current by the outermost
> calling application.  It might be just current because some
> intermittent part (the actual caller of `kill-buffer') wanted to have
> a look at the buffer's local variables.  So `kill-buffer' can't be
> sure about the "real identity of the current buffer" either.

I don't see a problem here.  Can you give a more detailed scenario?


        Stefan

bug#6454: 24.0.50; kill-buffer switches current-buffer

[martin rudalics]

 >>> I know that kill-buffer should not change current-buffer.
 >> What about C-x k  ;-)
 >
 > C-x k is no exception: the kill-buffer function there should not (and
 > usually will not) change current-buffer (other than in the obvious case
 > where we kill the current-buffer, obviously).

I obviously meant the obvious case ;-)

 >> So we would have to check whether we want to kill the current buffer and
 >> not change it if we don't.
 >
 > That's right.
 >
 >> Unfortunately, there's no proof that the buffer current when calling
 >> `kill-buffer' is the buffer considered current by the outermost
 >> calling application.  It might be just current because some
 >> intermittent part (the actual caller of `kill-buffer') wanted to have
 >> a look at the buffer's local variables.  So `kill-buffer' can't be
 >> sure about the "real identity of the current buffer" either.
 >
 > I don't see a problem here.  Can you give a more detailed scenario?

When `kill-buffer' kills the current buffer it has to find a new current
buffer.  It does so as follows:

   if (b == current_buffer)
     {
       tem = Fother_buffer (buffer, Qnil, Qnil);
       Fset_buffer (tem);
       if (b == current_buffer)
	return Qnil;
     }

I had code in mind that does something not entirely unreasonable like

(dolist (buffer buffer-list)
   (with-current-buffer buffer
       (when this-is-a-buffer-that-should-be-killed
	(kill-buffer))))

where this-is-a-buffer-that-should-be-killed is a buffer-local variable.
In this case `kill-buffer' will rotate through all possible candidates
in order to make another buffer current.

martin

bug#6454: 24.0.50; kill-buffer switches current-buffer

[martin rudalics]

 > (dolist (buffer buffer-list)
 >   (with-current-buffer buffer
 >       (when this-is-a-buffer-that-should-be-killed
 >     (kill-buffer))))
 >
 > where this-is-a-buffer-that-should-be-killed is a buffer-local variable.
 > In this case `kill-buffer' will rotate through all possible candidates
 > in order to make another buffer current.

Looking at this again it seems there are no problems indeed.  The case
described (and the subsequent Fset_buffer in window_loop) is handled by
`with-current-buffer'.  And otherwise the current buffer can be quietly
replaced by another buffer.

martin

bug#6454: 24.0.50; kill-buffer switches current-buffer

[martin rudalics]

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

> (eval-when-compile
>   (require 'cl))  ; for assert
> 
> (let ((buffer (generate-new-buffer "test"))
>       (saved-buffer (current-buffer)))
>   (unwind-protect
>       (with-current-buffer buffer
> 	(goto-line 1 buffer))
>     (assert (and buffer (not (eq buffer (current-buffer)))))
>     (assert (eq (current-buffer) saved-buffer) () "before kill-buffer")
>     (kill-buffer buffer)
>     (assert (eq (current-buffer) saved-buffer) () "after kill-buffer")))

Helmut, please try whether the attached patch fixes your bug.

Thanks, martin


[-- Attachment #2: window.c.diff --]
[-- Type: text/plain, Size: 827 bytes --]

*** src/window.c	2010-06-12 11:30:48 +0000
--- src/window.c	2010-06-19 10:10:41 +0000
***************
*** 2371,2377 ****
  		    /* Otherwise show a different buffer in the window.  */
  		    w->dedicated = Qnil;
  		    Fset_window_buffer (window, buffer, Qnil);
! 		    if (EQ (window, selected_window))
  		      Fset_buffer (w->buffer);
  		  }
  	      }
--- 2371,2381 ----
  		    /* Otherwise show a different buffer in the window.  */
  		    w->dedicated = Qnil;
  		    Fset_window_buffer (window, buffer, Qnil);
! 		    /* If WINDOW is the selected window, make its buffer
! 		       current.  But do so only if the window shows the
! 		       current buffer (Bug#6454).  */
! 		    if (EQ (window, selected_window)
! 			&& XBUFFER (buffer) == current_buffer)
  		      Fset_buffer (w->buffer);
  		  }
  	      }

bug#6454: 24.0.50; kill-buffer switches current-buffer

[Helmut Eller]

* martin rudalics [2010-06-19 15:38+0200] writes:

> Helmut, please try whether the attached patch fixes your bug.

Yes, the patch fixes my immediate problem. Thanks.  

I'm wondering a bit though: kill-buffer protects the current buffer
while calling kill-buffer-hook.  Wouldn't it be prudent to do that for
the entire function?

Actually, I think there is a small bug there: if kill-buffer-hook is a
list of functions, the first function could potentially switch buffer
and the second function would be called in the wrong buffer.

[The change in window_loop looks like good thing either way.]

Helmut

bug#6454: 24.0.50; kill-buffer switches current-buffer

[martin rudalics]

 > I'm wondering a bit though: kill-buffer protects the current buffer
 > while calling kill-buffer-hook.  Wouldn't it be prudent to do that for
 > the entire function?

You mean for the case where the buffer we want to kill is not the
current buffer?

 > Actually, I think there is a small bug there: if kill-buffer-hook is a
 > list of functions, the first function could potentially switch buffer
 > and the second function would be called in the wrong buffer.

I suppose you're right.  Could you propose a patch for this and the
above issue?

 > [The change in window_loop looks like good thing either way.]

`delete-windows-on' has the same problem.

martin

bug#6454: 24.0.50; kill-buffer switches current-buffer

[Helmut Eller]

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

* martin rudalics [2010-06-20 12:42+0200] writes:

>> I'm wondering a bit though: kill-buffer protects the current buffer
>> while calling kill-buffer-hook.  Wouldn't it be prudent to do that for
>> the entire function?
>
> You mean for the case where the buffer we want to kill is not the
> current buffer?

Both cases.  Even in the case where the to-be-killed buffer is not
current, kill-buffer may decide not to actually kill it and instead
return nil; this case should also not switch the buffer.

>> Actually, I think there is a small bug there: if kill-buffer-hook is a
>> list of functions, the first function could potentially switch buffer
>> and the second function would be called in the wrong buffer.
>
> I suppose you're right.  Could you propose a patch for this and the
> above issue?

Below are two patches.  The first essentially adds a save-excursion
around the whole function.

The second patch adds a function run_hook_in_buffer. It iterates over
the functions in the hook and for each function explicitly sets the
buffer before calling it.  I had to introduce a new macro
DO_HOOK_FUNCTIONS which is a big hammer for this, but I couldn't find a
better way.

Helmut


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: kill-buffer-1.patch --]
[-- Type: text/x-diff, Size: 2533 bytes --]

=== modified file 'src/buffer.c'
--- src/buffer.c	2010-06-05 00:41:32 +0000
+++ src/buffer.c	2010-06-20 17:24:02 +0000
@@ -1413,6 +1413,9 @@
   register struct Lisp_Marker *m;
   struct gcpro gcpro1;
 
+  int count = SPECPDL_INDEX ();
+  record_unwind_protect (save_excursion_restore, save_excursion_save ());
+
   if (NILP (buffer_or_name))
     buffer = Fcurrent_buffer ();
   else
@@ -1424,7 +1427,7 @@
 
   /* Avoid trouble for buffer already dead.  */
   if (NILP (b->name))
-    return Qnil;
+    return unbind_to (count, Qnil);
 
   /* Query if the buffer is still modified.  */
   if (INTERACTIVE && !NILP (b->filename)
@@ -1435,15 +1438,13 @@
 				     b->name, make_number (0)));
       UNGCPRO;
       if (NILP (tem))
-	return Qnil;
+	return unbind_to (count, Qnil);
     }
 
   /* Run hooks with the buffer to be killed the current buffer.  */
   {
-    int count = SPECPDL_INDEX ();
     Lisp_Object arglist[1];
 
-    record_unwind_protect (save_excursion_restore, save_excursion_save ());
     set_buffer_internal (b);
 
     /* First run the query functions; if any query is answered no,
@@ -1455,7 +1456,6 @@
 
     /* Then run the hooks.  */
     Frun_hooks (1, &Qkill_buffer_hook);
-    unbind_to (count, Qnil);
   }
 
   /* We have no more questions to ask.  Verify that it is valid
@@ -1464,10 +1464,10 @@
 
   /* Don't kill the minibuffer now current.  */
   if (EQ (buffer, XWINDOW (minibuf_window)->buffer))
-    return Qnil;
+    return unbind_to (count, Qnil);
 
   if (NILP (b->name))
-    return Qnil;
+    return unbind_to (count, Qnil);
 
   /* When we kill a base buffer, kill all its indirect buffers.
      We do it at this stage so nothing terrible happens if they
@@ -1499,7 +1499,7 @@
       tem = Fother_buffer (buffer, Qnil, Qnil);
       Fset_buffer (tem);
       if (b == current_buffer)
-	return Qnil;
+	return unbind_to (count, Qnil);
     }
 
   /* Notice if the buffer to kill is the sole visible buffer
@@ -1509,7 +1509,7 @@
     {
       tem = Fother_buffer (buffer, Qnil, Qnil);
       if (EQ (buffer, tem))
-	return Qnil;
+	return unbind_to (count, Qnil);
     }
 
   /* Now there is no question: we can kill the buffer.  */
@@ -1527,7 +1527,7 @@
      have called kill-buffer.  */
 
   if (NILP (b->name))
-    return Qnil;
+    return unbind_to (count, Qnil);
 
   clear_charpos_cache (b);
 
@@ -1609,7 +1609,7 @@
   UNBLOCK_INPUT;
   b->undo_list = Qnil;
 
-  return Qt;
+  return unbind_to (count, Qt);
 }
 \f
 /* Move the assoc for buffer BUF to the front of buffer-alist.  Since


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: kill-buffer-2.patch --]
[-- Type: text/x-diff, Size: 7056 bytes --]

=== modified file 'src/buffer.c'
--- src/buffer.c	2010-06-20 17:24:02 +0000
+++ src/buffer.c	2010-06-20 17:35:19 +0000
@@ -1382,6 +1382,34 @@
   return Qnil;
 }
 
+/* Run the hook HOOK in buffer BUFFER.
+   Make sure that each function is called in BUFFER.
+   Return the value returned by the last function. */
+static Lisp_Object
+run_hook_in_buffer (Lisp_Object hook, Lisp_Object buffer)
+{
+  Lisp_Object ret = Qnil;
+  struct gcpro gcpro1;
+  register struct buffer *b = XBUFFER (buffer);
+  int count = SPECPDL_INDEX ();
+  GCPRO1 (ret);
+
+  record_unwind_protect (set_buffer_if_live, Fcurrent_buffer ());
+  set_buffer_internal (b);
+  {
+    DO_HOOK_FUNCTIONS (state, hook, fun) 
+      {
+	eassert (b->name);
+	/* set again, because calling a function may switch buffer */
+	set_buffer_internal (b);
+	ret = call0 (fun);
+      }
+    UNGCPRO;
+  }
+  UNGCPRO;
+  return unbind_to (count, ret);
+}
+
 /*
   DEFVAR_LISP ("kill-buffer-hook", no_cell, "\
 Hook to be run (by `run-hooks', which see) when a buffer is killed.\n\
@@ -1455,7 +1483,7 @@
       return unbind_to (count, Qnil);
 
     /* Then run the hooks.  */
-    Frun_hooks (1, &Qkill_buffer_hook);
+    run_hook_in_buffer (Qkill_buffer_hook, buffer);
   }
 
   /* We have no more questions to ask.  Verify that it is valid

=== modified file 'src/eval.c'
--- src/eval.c	2010-05-14 17:53:42 +0000
+++ src/eval.c	2010-06-20 17:35:19 +0000
@@ -2673,13 +2673,62 @@
   return run_hook_with_args (nargs, args, until_failure);
 }
 
+/* Move to the next iteration state and return the current function.
+   Return Qunbound as end indicator. */
+Lisp_Object
+hook_iterator_next (struct hook_iterator_state* state)
+{
+  Lisp_Object val = state->val;
+  Lisp_Object globals = state->globals;
+
+  if (NILP (globals))
+    {
+      if (EQ (val, Qunbound) || NILP (val))
+	return Qunbound;  /* loop end marker */
+      if (!CONSP (val) || EQ (XCAR (val), Qlambda))
+	{
+	  state->val = Qnil;
+	  return val;
+	}
+      if (EQ (XCAR (val), Qt))
+	{
+	  /* t indicates this hook has a local binding;
+	     it means to run the global binding too.  */
+	  state->val = XCDR (val);
+	  state->globals = Fdefault_value (state->sym);
+	  return hook_iterator_next (state);
+	}
+      else
+	{
+	  state->val = XCDR (val);
+	  return XCAR (val);
+	}
+    }
+
+  else if (!CONSP (globals) || EQ (XCAR (globals), Qlambda))
+    {
+      state->globals = Qnil;
+      return globals;
+    }
+  else
+    {
+      state->globals = XCDR (globals);
+      /* In a global value, t should not occur.  If it does, we
+	 must ignore it to avoid an endless loop.  */
+      if (XCAR (globals) == Qt)
+	return hook_iterator_next (state);
+      else
+	return XCAR (globals);
+    }
+}
+
 /* ARGS[0] should be a hook symbol.
    Call each of the functions in the hook value, passing each of them
    as arguments all the rest of ARGS (all NARGS - 1 elements).
    COND specifies a condition to test after each call
    to decide whether to stop.
    The caller (or its caller, etc) must gcpro all of ARGS,
-   except that it isn't necessary to gcpro ARGS[0].  */
+   except that it isn't necessary to gcpro ARGS[0]. */
 
 static Lisp_Object
 run_hook_with_args (nargs, args, cond)
@@ -2687,74 +2736,32 @@
      Lisp_Object *args;
      enum run_hooks_condition cond;
 {
-  Lisp_Object sym, val, ret;
-  struct gcpro gcpro1, gcpro2, gcpro3;
+  Lisp_Object ret;
+  struct gcpro gcpro1;
 
   /* If we are dying or still initializing,
      don't do anything--it would probably crash if we tried.  */
   if (NILP (Vrun_hooks))
     return Qnil;
 
-  sym = args[0];
-  val = find_symbol_value (sym);
   ret = (cond == until_failure ? Qt : Qnil);
-
-  if (EQ (val, Qunbound) || NILP (val))
-    return ret;
-  else if (!CONSP (val) || EQ (XCAR (val), Qlambda))
-    {
-      args[0] = val;
-      return Ffuncall (nargs, args);
-    }
-  else
-    {
-      Lisp_Object globals = Qnil;
-      GCPRO3 (sym, val, globals);
-
-      for (;
-	   CONSP (val) && ((cond == to_completion)
-			   || (cond == until_success ? NILP (ret)
-			       : !NILP (ret)));
-	   val = XCDR (val))
-	{
-	  if (EQ (XCAR (val), Qt))
-	    {
-	      /* t indicates this hook has a local binding;
-		 it means to run the global binding too.  */
-	      globals = Fdefault_value (sym);
-	      if (NILP (globals)) continue;
-
-	      if (!CONSP (globals) || EQ (XCAR (globals), Qlambda))
-		{
-		  args[0] = globals;
-		  ret = Ffuncall (nargs, args);
-		}
-	      else
-		{
-		  for (;
-		       CONSP (globals) && ((cond == to_completion)
-					   || (cond == until_success ? NILP (ret)
-					       : !NILP (ret)));
-		       globals = XCDR (globals))
-		    {
-		      args[0] = XCAR (globals);
-		      /* In a global value, t should not occur.  If it does, we
-			 must ignore it to avoid an endless loop.  */
-		      if (!EQ (args[0], Qt))
-			ret = Ffuncall (nargs, args);
-		    }
-		}
-	    }
-	  else
-	    {
-	      args[0] = XCAR (val);
-	      ret = Ffuncall (nargs, args);
-	    }
-	}
-
-      UNGCPRO;
-      return ret;
-    }
+  GCPRO1 (ret);
+
+  {
+    DO_HOOK_FUNCTIONS (state, args[0], fun)
+      {
+	if ((cond == until_success) && !NILP (ret))
+	  break;
+	if ((cond == until_failure) && NILP (ret))
+	  break;
+	args[0] = fun;
+	ret = Ffuncall (nargs, args);
+      }
+    UNGCPRO;
+  }
+
+  UNGCPRO;
+  return ret;
 }
 
 /* Run a hook symbol ARGS[0], but use FUNLIST instead of the actual

=== modified file 'src/lisp.h'
--- src/lisp.h	2010-06-09 22:08:50 +0000
+++ src/lisp.h	2010-06-20 17:35:19 +0000
@@ -2853,6 +2853,40 @@
 EXFUN (Frun_hook_with_args_until_failure, MANY);
 extern Lisp_Object run_hook_list_with_args P_ ((Lisp_Object, int, Lisp_Object *));
 extern void run_hook_with_args_2 P_ ((Lisp_Object, Lisp_Object, Lisp_Object));
+
+/* Set up a while loop to iterate over the functions of a hook.
+   STATE a variable used to hold iteration state
+   HOOK_SYMBOL an expression which evaluates to the hook symbol
+   VAR variable to bind to function values
+
+   The macro needs to be used at the beginning of a block and
+   UNGCPRO must be called at the end, e.g.:
+   {
+     DO_HOOK_FUNCTIONS (state, Qkill_buffer_hook, fun) {
+       call0 (fun);
+     }
+     UNGCPRO;
+   }
+
+   Since this is a while loop, "break" can be used to terminate the
+   loop. */
+#define DO_HOOK_FUNCTIONS(STATE, HOOK_SYMBOL, VAR)		\
+  struct hook_iterator_state STATE = { Qnil, Qnil, Qnil };	\
+  struct gcpro gcpro1, gcpro2, gcpro3;				\
+  Lisp_Object VAR;						\
+  GCPRO3 ((STATE.sym), (STATE.val), (STATE.globals));		\
+  STATE.sym = HOOK_SYMBOL;					\
+  STATE.val = find_symbol_value (STATE.sym);			\
+  while (!EQ ((VAR = hook_iterator_next (&STATE)), Qunbound))
+
+/* used by the macro above */
+struct hook_iterator_state {
+  Lisp_Object sym;		/* hook symbol; read-only */
+  Lisp_Object val;		/* (buffer-local) value */
+  Lisp_Object globals;		/* global value */
+};
+Lisp_Object hook_iterator_next (struct hook_iterator_state*);
+
 EXFUN (Fand, UNEVALLED);
 EXFUN (For, UNEVALLED);
 EXFUN (Fif, UNEVALLED);

bug#6454: 24.0.50; kill-buffer switches current-buffer

[martin rudalics]

 > Below are two patches.  The first essentially adds a save-excursion
 > around the whole function.
 >
 > The second patch adds a function run_hook_in_buffer. It iterates over
 > the functions in the hook and for each function explicitly sets the
 > buffer before calling it.  I had to introduce a new macro
 > DO_HOOK_FUNCTIONS which is a big hammer for this, but I couldn't find a
 > better way.

Thanks.  I applied them here and will run them for a while to see
whether and what they change.  BTW run_window_scroll_functions in
xdisp.c has

       run_hook_with_args_2 (Qwindow_scroll_functions, window,
			    make_number (CHARPOS (startp)));
       SET_TEXT_POS_FROM_MARKER (startp, w->start);
       /* In case the hook functions switch buffers.  */
       if (current_buffer != XBUFFER (w->buffer))
	set_buffer_internal_1 (XBUFFER (w->buffer));

so with your patch this is probably no more needed?

Now, when a function run by a hook wants to change the current buffer it
cannot do so any more.  Admittedly this was unreliable before so we
probably won't lose much ...

martin

bug#6454: 24.0.50; kill-buffer switches current-buffer

[Helmut Eller]

* martin rudalics [2010-06-21 12:46+0200] writes:

>> Below are two patches.  The first essentially adds a save-excursion
>> around the whole function.
>>
>> The second patch adds a function run_hook_in_buffer. It iterates over
>> the functions in the hook and for each function explicitly sets the
>> buffer before calling it.  I had to introduce a new macro
>> DO_HOOK_FUNCTIONS which is a big hammer for this, but I couldn't find a
>> better way.
>
> Thanks.  I applied them here and will run them for a while to see
> whether and what they change.  BTW run_window_scroll_functions in
> xdisp.c has
>
>       run_hook_with_args_2 (Qwindow_scroll_functions, window,
> 			    make_number (CHARPOS (startp)));
>       SET_TEXT_POS_FROM_MARKER (startp, w->start);
>       /* In case the hook functions switch buffers.  */
>       if (current_buffer != XBUFFER (w->buffer))
> 	set_buffer_internal_1 (XBUFFER (w->buffer));
>
> so with your patch this is probably no more needed?
>
> Now, when a function run by a hook wants to change the current buffer it
> cannot do so any more.  Admittedly this was unreliable before so we
> probably won't lose much ...

My run_hook_in_buffer is (static) in buffer.c and only used by
Fkill_buffer.  The refactoring in eval.c was only needed to make it
possible to iterate over the functions of a hook.  It shouldn't affect
other things (modulo bugs that I introduced).

I guess unintended buffer switching could be a problem with many hooks,
but it's not clear if/when it's fix-worthy.

Helmut

bug#6454: 24.0.50; kill-buffer switches current-buffer

[martin rudalics]

 > My run_hook_in_buffer is (static) in buffer.c and only used by
 > Fkill_buffer.  The refactoring in eval.c was only needed to make it
 > possible to iterate over the functions of a hook.  It shouldn't affect
 > other things (modulo bugs that I introduced).

I meant Frun_hook_with_args_until_failure which calls run_hook_with_args
which is now handled by DO_HOOK_FUNCTIONS.  IIUC in order to not affect
other invocations of run_hook_with_args you would have to iterate over
the Qkill_buffer_query_functions within Fkill_buffer.  Or am I missing
something?

martin

bug#6454: 24.0.50; kill-buffer switches current-buffer

[Helmut Eller]

* martin rudalics [2010-06-21 17:49+0200] writes:

>> My run_hook_in_buffer is (static) in buffer.c and only used by
>> Fkill_buffer.  The refactoring in eval.c was only needed to make it
>> possible to iterate over the functions of a hook.  It shouldn't affect
>> other things (modulo bugs that I introduced).
>
> I meant Frun_hook_with_args_until_failure which calls run_hook_with_args
> which is now handled by DO_HOOK_FUNCTIONS.  IIUC in order to not affect
> other invocations of run_hook_with_args you would have to iterate over
> the Qkill_buffer_query_functions within Fkill_buffer.  Or am I missing
> something?

Yes, right.  I forgot about Qkill_buffer_query_functions and only dealt
with Qkill_buffer_hook.

Helmut

bug#6454: 24.0.50; kill-buffer switches current-buffer

[martin rudalics]

 >> I meant Frun_hook_with_args_until_failure which calls run_hook_with_args
 >> which is now handled by DO_HOOK_FUNCTIONS.  IIUC in order to not affect
 >> other invocations of run_hook_with_args you would have to iterate over
 >> the Qkill_buffer_query_functions within Fkill_buffer.  Or am I missing
 >> something?
 >
 > Yes, right.  I forgot about Qkill_buffer_query_functions and only dealt
 > with Qkill_buffer_hook.

So this is quite a substantial change ;-)

Anyway.  As I said I shall run my Emacs now with your change and look
whether it breaks something.  If necessary, we should make this then a
`kill-buffer'-only change though.

martin

bug#6454: 24.0.50; kill-buffer switches current-buffer

[Stefan Monnier]

> Actually, I think there is a small bug there: if kill-buffer-hook is a
> list of functions, the first function could potentially switch buffer
> and the second function would be called in the wrong buffer.

This is true of many hooks, yes, and we have generally taken the point
of view that if a hook function changes the current buffer, there can be
the following cases:
- it's OK the hook expects this anyway (e.g. write-region-annotate-functions)
- it breaks the code that runs the hook (e.g. kill-buffer): if the
  breakage is serious, it's a bug in the code that runs the hook that
  should protect itself from such changes.  If the breakage is not too
  serious and/or the performance is considered a serious issue you may
  just say "don't do that" in the docstring of the hook.
- it doesn't break the code that runs the hook, but it can break
  subsequent functions on that hook: we say that the bug is in the hook
  function, which simply should not change the current buffer.  We may
  add a note in the hook's docstring telling coders no to do that.

I think kill-buffer-hook is in the third category.


-- Stefan

bug#6454: 24.0.50; kill-buffer switches current-buffer

[Lars Magne Ingebrigtsen]

martin rudalics <rudalics@gmx.at> writes:

>> Yes, right.  I forgot about Qkill_buffer_query_functions and only dealt
>> with Qkill_buffer_hook.
>
> So this is quite a substantial change ;-)
>
> Anyway.  As I said I shall run my Emacs now with your change and look
> whether it breaks something.  If necessary, we should make this then a
> `kill-buffer'-only change though.

The original bug reported was fixed, and then a followup was also
fixed.  But reading the thread a couple times, I'm not sure whether
everything was resolved here.  :-)

-- 
(domestic pets only, the antidote for overdose, milk.)
  bloggy blog http://lars.ingebrigtsen.no/

bug#6454: 24.0.50; kill-buffer switches current-buffer

[Helmut Eller]

* Lars Magne Ingebrigtsen [2011-09-21 20:44] writes:

> martin rudalics <rudalics@gmx.at> writes:
>
>>> Yes, right.  I forgot about Qkill_buffer_query_functions and only dealt
>>> with Qkill_buffer_hook.
>>
>> So this is quite a substantial change ;-)
>>
>> Anyway.  As I said I shall run my Emacs now with your change and look
>> whether it breaks something.  If necessary, we should make this then a
>> `kill-buffer'-only change though.
>
> The original bug reported was fixed, and then a followup was also
> fixed.  But reading the thread a couple times, I'm not sure whether
> everything was resolved here.  :-)

The original test case was fixed, so I think the bug should be closed.

The hook issue was essentially resolved here:
http://lists.gnu.org/archive/html/bug-gnu-emacs/2010-06/msg00527.html
Nobody changed the docstring.  kill-buffer still uses a save-excursion
which is not quite necessary because hook functions shouldn't change
the buffer anyway.

Helmut

bug#6454: 24.0.50; kill-buffer switches current-buffer

[Lars Magne Ingebrigtsen]

Helmut Eller <eller.helmut@gmail.com> writes:

> The original test case was fixed, so I think the bug should be closed.

Right; closing.

-- 
(domestic pets only, the antidote for overdose, milk.)
  bloggy blog http://lars.ingebrigtsen.no/