bug#67836: 29.1.90; map-y-or-n-p doesn't terminate when run in a kmacro in batch mode

[sbaugh]

Eli Zaretskii <eliz@gnu.org> writes:
>> From: Stefan Monnier <monnier@iro.umontreal.ca>
>> Cc: Spencer Baugh <sbaugh@janestreet.com>,  67836@debbugs.gnu.org
>> Date: Fri, 15 Dec 2023 17:55:43 -0500
>> 
>> > Thanks, but let's please find a fix that doesn't make the tail wag the
>> > dog.  I don't want to make a change in bitch_at_user, which will
>> > affect every possible use of it in batch mode, something that we have
>> > been doing for eons.
>> 
>> I suspect keyboard macros have not been used very much in batch mode
>> over the last 32 years.
>
> I actually question the wisdom of doing so.  It isn't what keyboard
> macros are for.

How else can one test keyboard interaction with Emacs commands,
including their interactive specs?  I see no way to do that other than
with keyboard macros.  I'd be happy to hear that there's a better way,
though.

I'd like to add tests of interactive specs to the Emacs test suite, so
certainly I want to find a solution which is acceptable to you.

>> >> ding's docstring states that it terminates keyboard macros.  But, due
>> >> to what seems to be an oversight, it does not do that while executing
>> >> in batch mode.
>> > As the code clearly shows, it isn't an oversight.
>> 
>> AFAICT the current logic of code can be traced back to:
>> 
>>     commit 4588ec205730239596486e8ad4d18d541917199a
>>     Author: Jim Blandy <jimb@red-bean.com>
>>     Date:   Wed Jul 3 12:10:07 1991 +0000
>>     
>>         Initial revision
>>     
>>     diff --git a/src/dispnew.c b/src/dispnew.c
>>     --- /dev/null
>>     +++ b/src/dispnew.c
>>     @@ -0,0 +1813,9 @@
>>     +{
>>     +  if (noninteractive)
>>     +    putchar (07);
>>     +  else if (!INTERACTIVE)  /* Stop executing a keyboard macro. */
>>     +    error ("Keyboard macro terminated by a command ringing the bell");
>>     +  else
>>     +    ring_bell ();
>>     +  fflush (stdout);
>>     +}
>> 
>> I'm not sure this code can be said to show clearly that it's not
>> an oversight.
>> I read it to say that the author did not consider the intersection of
>> 
>>     noninteractive
>> and
>>     !INTERACTIVE
>
> Maybe so (we could ask Jim if we wanted, he is still around), but in
> any case I'm not interested in changing how bitch_at_user works
> 30-something years after it was coded, and has worked well since then.
> I'm okay with fixing this particular problem, but not via changes to
> bitch_at_user or any similar low-level functionality used everywhere.
> Such changes are IMO acceptable only if they fix a clear bug, which is
> not the case here.

There is definitely at least one bug, since the docstring of ding
currently erroneously says:

Also, unless an argument is given,
terminate any keyboard macro currently executing.

Making ding match its docstring was one way to fix this bug.  If you
don't want to do that, could you apply this patch or something like it?

--- a/src/dispnew.c
+++ b/src/dispnew.c
@@ -6111,8 +6111,8 @@ DEFUN ("send-string-to-terminal", 
Fsend_string_to_terminal,
 
 DEFUN ("ding", Fding, Sding, 0, 1, 0,
        doc: /* Beep, or flash the screen.
-Also, unless an argument is given,
-terminate any keyboard macro currently executing.  */)
+Also, unless an argument is given or the symbol `noninteractive' is
+non-nil, terminate any keyboard macro currently executing.  */)
   (Lisp_Object arg)
 {
   if (!NILP (arg))

I will send a separate patch to fix map-y-or-n-p.

> Spencer, I already noted once that many of your patches have this
> problem: you take some problem which happens to you in a very
> specialized use case, and propose to fix that by changes that affect
> all of Emacs.  This isn't going to fly in Emacs in general, since
> Emacs is a very old and stable program, where almost all of the old
> low-level code was tested by decades of usage, and any significant
> changes there run high risk of breaking something.  And yet you keep
> coming up with changes which do precisely that.  Please try to see
> things from the POV of the Emacs maintainers, and look for ways of
> fixing those problems either by much more localized changes, or maybe
> even just in your own code.  TIA.

As I said, there's definitely at least one bug.  I could either:

- make two separate changes, to fix the docstring of ding and separately
  fix map-y-or-n-p's infinite loop

- make one change (to make ding terminate keyboard macros in batch mode)
  which fixes both issues

All else being equal, making one change is better than making two
changes.  But in this case, the one change is a low-level change.

Often, a low-level change to Emacs is in fact acceptable.  I have little
way of knowing whether any given low-level change is acceptable, other
than by sending it in and seeing what others say.  I hope it is OK if I
continue to do that.