bug#5924: 23.1; accept-process-output switching current-buffer

[Stefan Monnier]

>> > Reading the elisp manual doesn't indicate anywhere that a call such as 
>> >    (accept-process-output process)
>> > should change the current-buffer.
>> That depends on the code run during the wait.  I.e. it depends on the
>> code run by the process filters, sentinels, timers, ...
> Yes, indeed!  If there is code being run during accept-process-output
> then the state can be changed by that code.  But in the example that I
> witnessed, there was *no code* being run.

That's very odd: both the sentinel and the filter code are careful to
preserve the current_buffer when there's no sentinel or no filter set.

I've just installed a change in the Emacs Bzr trunk so that the
current-buffer is preserved when running the Elisp code of process
filters and sentinels.  If you can try this code (or try the patch
below) to see if it fixes your problem, it would be helpful.

> In the second "way" that is being talked about, we can reasonably
> expect that current-buffer might change during the execution of the
> filter function.  But in the first "way", where Emacs is doing all the
> work of accepting the process output, I don't think it should change
> the current-buffer.

Indeed and it shouldn't.  And with my new changes, even when running
Elisp it shouldn't change current-buffer either.

> I don't fully understand this, but let me say that I am just talking
> about problem with the Elisp semantics, not Emacs libraries.  If a
> filter function changed the current-buffer, one would regard it as
> buggy.  But if Elisp itself changes the current-buffer...?

Given that it's code run asynchronously, letting it change
current-buffer is asking for trouble.

> The other thing that concerns me is that the same behaviour persisted
> even if I set the JUST-THIS-ONE flag to t.  In that case, one would
> expect that Elisp would ignore the output from all other processes.

Indeed, that's what it means.

> So, it should have no reason to change the current-buffer to the other
> process.  But it did.  I am not sure if the JUST-THIS-ONE flag is
> doing anything at all.

I don't know of a bug in this regard, so if you have evidence that
JUST-THIS-ONE doesn't prevent reading other process's output, please
report it.


        Stefan


=== modified file 'src/process.c'
--- src/process.c       2010-04-02 03:10:33 +0000
+++ src/process.c       2010-04-11 16:15:09 +0000
@@ -5314,6 +5314,8 @@
   struct coding_system *coding = proc_decode_coding_system[channel];
   int carryover = p->decoding_carryover;
   int readmax = 4096;
+  int count = SPECPDL_INDEX ();
+  Lisp_Object odeactivate;
 
   chars = (char *) alloca (carryover + readmax);
   if (carryover)
@@ -5386,15 +5388,16 @@
   /* Now set NBYTES how many bytes we must decode.  */
   nbytes += carryover;
 
+  odeactivate = Vdeactivate_mark;
+  /* There's no good reason to let process filters change the current
+     buffer, and many callers of accept-process-output, sit-for, and
+     friends don't expect current-buffer to be changed from under them.  */
+  record_unwind_protect (Fset_buffer, Fcurrent_buffer ());
+
   /* Read and dispose of the process output.  */
   outstream = p->filter;
   if (!NILP (outstream))
     {
-      /* We inhibit quit here instead of just catching it so that
-        hitting ^G when a filter happens to be running won't screw
-        it up.  */
-      int count = SPECPDL_INDEX ();
-      Lisp_Object odeactivate;
       Lisp_Object obuffer, okeymap;
       Lisp_Object text;
       int outer_running_asynch_code = running_asynch_code;
@@ -5402,10 +5405,12 @@
 
       /* No need to gcpro these, because all we do with them later
         is test them for EQness, and none of them should be a string.  */
-      odeactivate = Vdeactivate_mark;
       XSETBUFFER (obuffer, current_buffer);
       okeymap = current_buffer->keymap;
 
+      /* We inhibit quit here instead of just catching it so that
+        hitting ^G when a filter happens to be running won't screw
+        it up.  */
       specbind (Qinhibit_quit, Qt);
       specbind (Qlast_nonmenu_event, Qt);
 
@@ -5474,9 +5479,6 @@
       restore_search_regs ();
       running_asynch_code = outer_running_asynch_code;
 
-      /* Handling the process output should not deactivate the mark.  */
-      Vdeactivate_mark = odeactivate;
-
       /* Restore waiting_for_user_input_p as it was
         when we were called, in case the filter clobbered it.  */
       waiting_for_user_input_p = waiting;
@@ -5492,27 +5494,19 @@
           cause trouble (for example it would make sit_for return).  */
        if (waiting_for_user_input_p == -1)
          record_asynch_buffer_change ();
-
-      unbind_to (count, Qnil);
-      return nbytes;
     }
 
   /* If no filter, write into buffer if it isn't dead.  */
-  if (!NILP (p->buffer) && !NILP (XBUFFER (p->buffer)->name))
+  else if (!NILP (p->buffer) && !NILP (XBUFFER (p->buffer)->name))
     {
       Lisp_Object old_read_only;
       int old_begv, old_zv;
       int old_begv_byte, old_zv_byte;
-      Lisp_Object odeactivate;
       int before, before_byte;
       int opoint_byte;
       Lisp_Object text;
       struct buffer *b;
-      int count = SPECPDL_INDEX ();
 
-      odeactivate = Vdeactivate_mark;
-
-      record_unwind_protect (Fset_buffer, Fcurrent_buffer ());
       Fset_buffer (p->buffer);
       opoint = PT;
       opoint_byte = PT_BYTE;
@@ -5610,13 +5604,14 @@
       if (old_begv != BEGV || old_zv != ZV)
        Fnarrow_to_region (make_number (old_begv), make_number (old_zv));
 
-      /* Handling the process output should not deactivate the mark.  */
-      Vdeactivate_mark = odeactivate;
 
       current_buffer->read_only = old_read_only;
       SET_PT_BOTH (opoint, opoint_byte);
-      unbind_to (count, Qnil);
     }
+  /* Handling the process output should not deactivate the mark.  */
+  Vdeactivate_mark = odeactivate;
+
+  unbind_to (count, Qnil);
   return nbytes;
 }
 
@@ -6845,6 +6840,11 @@
   XSETBUFFER (obuffer, current_buffer);
   okeymap = current_buffer->keymap;
 
+  /* There's no good reason to let sentinels change the current
+     buffer, and many callers of accept-process-output, sit-for, and
+     friends don't expect current-buffer to be changed from under them.  */
+  record_unwind_protect (Fset_buffer, Fcurrent_buffer ());
+
   sentinel = p->sentinel;
   if (NILP (sentinel))
     return;