Continue reading in 'read-event' etc. at the end of a keyboard macro

This fixes a bug that could make 'read-event', 'read-char', and
'read-char-exclusive' erroneously return -1, an internal magic return
value of 'read_char' leaked from C to lisp. Instead of returning -1, the
aforementioned lisp functions now transparently continue reading
available input (e.g., from the keyboard) when reaching the end of a
keyboard macro.

* src/keyboard.c (read_char, read_key_sequence): Move handling
of the end of a keyboard macro from 'read_char' to its caller
'read_key_sequence', which is the only caller that can
meaningfully deal with this case.
* src/macros.c (Fexecute_kbd_macro): Document how the end of keyboard
macro is processed.
* etc/NEWS: Announce this change.

etc/NEWS

@@ -2141,6 +2141,18 @@ Like the variable with the same name, it adds menus from the list that
 is the value of the property to context menus shown when clicking on the
 text which as this property.
 
+---
+** Detecting the end of an iteration of a keyboard macro
+'read-event', 'read-char', and 'read-char-exclusive' no longer return -1
+when called at the end of an iteration of a the execution of a keyboard
+macro.  Instead, they will transparently continue reading available input
+(e.g., from the keyboard).  If you need to detect the end of a macro
+iteration, check the following condition before calling one of the
+aforementioned functions:
+
+    (and (arrayp executing-kbd-macro)
+         (>= executing-kbd-macro-index (length executing-kbd-macro))))
+
 
 * Changes in Emacs 30.1 on Non-Free Operating Systems
 

src/keyboard.c

@@ -2620,7 +2620,8 @@ read_char (int commandflag, Lisp_Object map,
       goto reread_for_input_method;
     }
 
-  if (!NILP (Vexecuting_kbd_macro))
+  /* If we're executing a macro, process it unless we are at its end. */
+  if (!NILP (Vexecuting_kbd_macro) && !at_end_of_macro_p ())
     {
       /* We set this to Qmacro; since that's not a frame, nobody will
 	 try to switch frames on us, and the selected window will
@@ -2634,15 +2635,6 @@ read_char (int commandflag, Lisp_Object map,
 	 selected.  */
       Vlast_event_frame = internal_last_event_frame = Qmacro;
 
-      /* Exit the macro if we are at the end.
-	 Also, some things replace the macro with t
-	 to force an early exit.  */
-      if (at_end_of_macro_p ())
-	{
-	  XSETINT (c, -1);
-	  goto exit;
-	}
-
       c = Faref (Vexecuting_kbd_macro, make_int (executing_kbd_macro_index));
       if (STRINGP (Vexecuting_kbd_macro)
 	  && (XFIXNAT (c) & 0x80) && (XFIXNAT (c) <= 0xff))
@@ -10694,8 +10686,19 @@ read_key_sequence (Lisp_Object *keybuf, Lisp_Object prompt,
 	    }
 	  used_mouse_menu = used_mouse_menu_history[t];
 	}
-
-      /* If not, we should actually read a character.  */
+      /* If we're at the end of a macro, exit it by returning 0,
+	 unless there are unread events pending.  */
+      else if (!NILP (Vexecuting_kbd_macro)
+	  && at_end_of_macro_p ()
+	  && !requeued_events_pending_p ())
+	{
+	  t = 0;
+	  /* The Microsoft C compiler can't handle the goto that
+	     would go here.  */
+	  dummyflag = true;
+	  break;
+	}
+      /* Otherwise, we should actually read a character.  */
       else
 	{
 	  {
@@ -10787,18 +10790,6 @@ read_key_sequence (Lisp_Object *keybuf, Lisp_Object prompt,
 	      return -1;
 	    }
 
-	  /* read_char returns -1 at the end of a macro.
-	     Emacs 18 handles this by returning immediately with a
-	     zero, so that's what we'll do.  */
-	  if (FIXNUMP (key) && XFIXNUM (key) == -1)
-	    {
-	      t = 0;
-	      /* The Microsoft C compiler can't handle the goto that
-		 would go here.  */
-	      dummyflag = true;
-	      break;
-	    }
-
 	  /* If the current buffer has been changed from under us, the
 	     keymap may have changed, so replay the sequence.  */
 	  if (BUFFERP (key))

src/macros.c

@@ -314,6 +314,48 @@ buffer before the macro is executed.  */)
 		      Vreal_this_command));
   record_unwind_protect (pop_kbd_macro, tem);
 
+  /* The following loop starts the execution of possibly multiple
+     iterations of the macro.
+
+     The state variables that control the execution of a single
+     iteration are Vexecuting_kbd_macro and executing_kbd_macro_index,
+     which can be accessed from lisp. The purpose of the variables
+     executing_kbd_macro and executing_kbd_macro_iteration is to
+     remember the most recently started macro and its iteration count.
+     This makes it possible to produce a meaningful message in case of
+     errors during the execution of the macro.
+
+     In a single iteration, individual characters from the macro are
+     read by read_char, which takes care of incrementing
+     executing_kbd_macro_index after each character.
+
+     The end of a macro iteration is handled as follows:
+      - read_key_sequence asks at_end_of_macro_p whether the end of the
+        iteration has been reached.  If so, it returns the magic value 0
+        to command_loop_1.
+      - command_loop_1 returns Qnil to command_loop_2.
+      - command_loop_2 returns Qnil to this function
+        (but only the returning is relevant, not the actual value).
+
+     Macro executions form a stack.  After the last iteration of the
+     execution of one stack item, or in case of an error during one of
+     the iterations, pop_kbd_macro (invoked via unwind-protect) will
+     restore Vexecuting_kbd_macro and executing_kbd_macro_index, and
+     run 'kbd-macro-termination-hook'.
+
+     If read_char happens to be called at the end of a macro interation,
+     but before read_key_sequence could handle the end (e.g., when lisp
+     code calls 'read-event', 'read-char', or 'read-char-exclusive'),
+     read_char will simply continue reading other available input
+     (Bug#68272).  Vexecuting_kbd_macro and executing_kbd_macro remain
+     untouched until the end of the iteration is handled.
+
+     This is similar (in observable behavior) to a posibly simpler
+     implementation of keyboard macros in which this function pushed all
+     characters of the macro into the incoming event queue and returned
+     immediately.  Maybe this is the implementation that we ideally
+     would like to have, but switching to it will require a larger code
+     change.  */
   do
     {
       Vexecuting_kbd_macro = final;