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

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

[Spencer Baugh]

1. Create a file "broken.el" containing:
(defun broken ()
  (map-y-or-n-p ""  #'ignore '(1)))
(execute-kbd-macro (read-kbd-macro "M-: (broken) RET"))
2. emacs -Q -l ./broken.el
3. Emacs properly executes the keyboard macro and errors.
4. emacs -Q --batch -l ./broken.el
5. Notice Emacs infinite loops, printing the map-y-or-n-p prompt repeatedly.

map-y-or-n-p in a keyboard macro terminates in non-batch Emacs because
it calls (ding), which terminates the currently running keyboard macro.
However, (ding) doesn't terminate keyboard macros in batch mode.  This
seems to just be an oversight.

A patch to fix this by making (ding) always terminate keyboard macros
will follow.

(BTW, when not running in a keyboard macro, map-y-or-n-p will error in
batch Emacs, but only because of a separate bug where it runs (listp
last-nonmenu-event) to decide whether to use menus, which returns true
since last-nonmenu-event is nil.  I may fix this separately.)


In GNU Emacs 29.1.90 (build 2, x86_64-pc-linux-gnu, X toolkit, cairo
 version 1.15.12, Xaw scroll bars) of 2023-11-20 built on
 igm-qws-u22796a
Repository revision: dd8669b14b8a2b9a6d214a9d142dd8ac604f83d2
Repository branch: emacs-29
Windowing system distributor 'The X.Org Foundation', version 11.0.12011000
System Description: Rocky Linux 8.9 (Green Obsidian)

Configured using:
 'configure --config-cache --with-x-toolkit=lucid
 --with-gif=ifavailable'

Configured features:
CAIRO DBUS FREETYPE GLIB GMP GNUTLS GSETTINGS HARFBUZZ JPEG JSON
LIBSELINUX LIBSYSTEMD LIBXML2 MODULES NOTIFY INOTIFY PDUMPER PNG RSVG
SECCOMP SOUND SQLITE3 THREADS TIFF TOOLKIT_SCROLL_BARS X11 XDBE XIM
XINPUT2 XPM LUCID ZLIB

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: ELisp/l

Minor modes in effect:
  global-undo-tree-mode: t
  bug-reference-prog-mode: t
  delete-selection-mode: t
  global-so-long-mode: t
  pixel-scroll-precision-mode: t
  jane-fe-minor-mode: t
  jane-fe-jenga-minor-mode: t
  editorconfig-mode: t
  which-function-mode: t
  global-git-commit-mode: t
  magit-auto-revert-mode: t
  auto-revert-mode: t
  shell-dirtrack-mode: t
  server-mode: t
  windmove-mode: t
  savehist-mode: t
  save-place-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tab-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  context-menu-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  line-number-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Features:
(shadow emacsbug make-mode finder lisp-mnt autoinsert etags org-agenda
pcmpl-gnu canlock nndraft nnmh nnfolder term ehelp cc-mode cc-fonts
cc-guess cc-menus cc-cmds cc-styles cc-align cc-engine cc-vars cc-defs
flow-fill shr-color qp smiley gnus-cite gnus-async gnus-bcklg gnus-agent
gnus-srvr gnus-score score-mode nnvirtual nntp gnus-ml gnus-msg
disp-table nndoc gnus-cache gnus-dup mm-archive debbugs-gnu
debbugs-compat debbugs soap-client rng-xsd rng-dt rng-util xsd-regexp
two-column evil-tests evil-test-helpers evil evil-integration undo-tree
evil-maps evil-commands evil-jumps evil-command-window evil-types
evil-search evil-ex evil-macros evil-repeat evil-states evil-core
evil-common cl evil-digraphs elp evil-vars latexenc fileloop textsec
uni-scripts idna-mapping uni-confusable textsec-check mail-extr wdired
doctor xscheme scheme apropos tramp-adb tramp-archive tramp-cmds
tramp-container tramp-ftp tramp-gvfs tramp-sh tramp-cache time-stamp
emoji-labels emoji multisession sqlite comp comp-cstr info-look flyspell
ispell pcmpl-unix emacs-news-mode reposition mode-local hippie-exp
vc-bzr vc-src vc-sccs vc-svn vc-cvs vc-rcs vc-dir cus-start sql log-view
vc-fe vc-hg vc magit-imenu git-rebase vc-git vc-dispatcher face-remap
hl-line display-line-numbers tabify man misc dired-aux rect sort
sh-script treesit thai-util thai-word ucs-normalize mule-util completion
dabbrev external-completion org-element org-persist org-id org-refile
avl-tree generator oc-basic ol-eww eww xdg url-queue mm-url ol-rmail
ol-mhe ol-irc ol-info ol-gnus nnselect gnus-art mm-uu mml2015 mm-view
mml-smime smime gnutls dig gnus-sum shr pixel-fill kinsoku url-file svg
dom gnus-group gnus-undo gnus-start gnus-dbus dbus xml gnus-cloud nnimap
nnmail mail-source utf7 nnoo gnus-spec gnus-int gnus-range gnus-win
ol-docview doc-view jka-compr image-mode exif ol-bibtex bibtex ol-bbdb
ol-w3m ol-doi org-link-doi jane-aide cl-print misearch multi-isearch
pulse bug-reference shortdoc help-fns radix-tree executable
url-http-ntlm ntlm hmac-md5 hex-util md4 network-stream url-cache
url-http url-gw nsm codeium find-dired goto-addr let-alist delsel
so-long jane-fe-read-feature pixel-scroll cua-base tramp tramp-loaddefs
trampver tramp-integration tramp-compat parse-time iso8601 ffap
jane-merlin merlin-imenu merlin-xref merlin-cap merlin jane-async-merlin
jane-completion grep jane-common site-start jane-customization
jane-comint org-protocol jane-fe-project xref files-x jane-fe-menu
ecaml_plugin view gopcaml magit-bookmark bookmark image+ advice
image-file image-converter editorconfig editorconfig-core
editorconfig-core-handle editorconfig-fnmatch whitespace jane-auto-modes
vba-mode markdown-mode color jane jane-yasnippet jane-micro-features ert
ewoc debug backtrace jane-diff unified-test-mode shell-file core
core-buffer core-error jane-sexp jane-python jane-ocaml
jane-tuareg-theme tuareg tuareg-compat tuareg-opam skeleton flymake-proc
flymake warnings thingatpt smie caml-types caml-help caml-emacs
find-file compile jane-cr jane-codeium jane-align jane-deprecated
jane-smerge gnu-elpa-keyring-update jane-ocp-indent ocp-indent
jane-eglot yasnippet-autoloads swiper-autoloads htmlize-autoloads
eglot-autoloads editorconfig-autoloads codeium-autoloads jane-autoloads
jane-util ob-shell page-ext dired-x magit-extras project magit-submodule
magit-obsolete magit-blame magit-stash magit-reflog magit-bisect
magit-push magit-pull magit-fetch magit-clone magit-remote magit-commit
magit-sequence magit-notes magit-worktree magit-tag magit-merge
magit-branch magit-reset magit-files magit-refs magit-status magit
magit-repos magit-apply magit-wip magit-log which-func imenu magit-diff
smerge-mode diff diff-mode git-commit log-edit message sendmail
yank-media puny dired dired-loaddefs rfc822 mml mml-sec epa derived epg
rfc6068 epg-config mm-decode mm-bodies mm-encode mail-parse rfc2231
rfc2047 rfc2045 ietf-drums mailabbrev gmm-utils mailheader pcvs-util
add-log magit-core magit-autorevert autorevert filenotify magit-margin
magit-transient magit-process with-editor shell server magit-mode
transient edmacro kmacro magit-git magit-section magit-utils crm dash
gnus nnheader gnus-util text-property-search mail-utils range mm-util
mail-prsvr cl-extra help-mode windmove org ob ob-tangle ob-ref ob-lob
ob-table ob-exp org-macro org-src ob-comint org-pcomplete pcomplete
org-list org-footnote org-faces org-entities time-date noutline outline
ob-emacs-lisp ob-core ob-eval org-cycle org-table ol rx org-fold
org-fold-core org-keys oc org-loaddefs find-func cal-menu calendar
cal-loaddefs org-version org-compat org-macs format-spec gdb-mi bindat
gud easy-mmode comint ansi-osc ansi-color ring vundo modus-vivendi-theme
modus-themes pcase savehist saveplace cus-edit pp cus-load icons
wid-edit adaptive-wrap-autoloads csv-mode-autoloads
cyberpunk-theme-autoloads evil-autoloads exwm-autoloads helm-autoloads
helm-core-autoloads async-autoloads ivy-autoloads magit-autoloads
git-commit-autoloads finder-inf magit-section-autoloads dash-autoloads
popup-autoloads url-http-ntlm-autoloads url-auth vc-hgcmd-autoloads
vundo-autoloads info with-editor-autoloads xelb-autoloads package
browse-url url url-proxy url-privacy url-expand url-methods url-history
url-cookie generate-lisp-file url-domsuf url-util mailcap url-handlers
url-parse auth-source cl-seq eieio eieio-core cl-macs password-cache
json subr-x map byte-opt gv bytecomp byte-compile url-vars cl-loaddefs
cl-lib rmc iso-transl tooltip cconv eldoc paren electric uniquify
ediff-hook vc-hooks lisp-float-type elisp-mode mwheel term/x-win x-win
term/common-win x-dnd tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode lisp-mode prog-mode register
page tab-bar menu-bar rfn-eshadow isearch easymenu timer select
scroll-bar mouse jit-lock font-lock syntax font-core term/tty-colors
frame minibuffer nadvice seq simple cl-generic indonesian philippine
cham georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese eucjp-ms cp51932 hebrew greek romanian slovak czech
european ethiopic indian cyrillic chinese composite emoji-zwj charscript
charprop case-table epa-hook jka-cmpr-hook help abbrev obarray oclosure
cl-preloaded button loaddefs theme-loaddefs faces cus-face macroexp
files window text-properties overlay sha1 md5 base64 format env
code-pages mule custom widget keymap hashtable-print-readable backquote
threads dbusbind inotify dynamic-setting system-font-setting
font-render-setting cairo x-toolkit xinput2 x multi-tty
make-network-process emacs)

Memory information:
((conses 16 3200047 267481)
 (symbols 48 76910 1)
 (strings 32 371340 77106)
 (string-bytes 1 16177813)
 (vectors 16 163678)
 (vector-slots 8 3169215 359463)
 (floats 8 850 969)
 (intervals 56 550774 4661)
 (buffers 976 591))

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

[Spencer Baugh]

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


Patch fixing this.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Make-ding-terminate-keyboard-macros-even-in-batch-mo.patch --]
[-- Type: text/x-patch, Size: 3728 bytes --]

From f894cbede9b65e6449c80393efb9d3417c9f661c Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh@janestreet.com>
Date: Fri, 15 Dec 2023 09:57:19 -0500
Subject: [PATCH] Make ding terminate keyboard macros even in batch mode

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.

Generally, this means that some functions may infinite loop while run
in a keyboard macro in batch mode.

One concrete example: map-y-or-n-p will infinite-loop if run in a
keyboard macro while in batch mode.

Now ding properly terminates keyboard macros while running in batch
mode.  A test showing map-y-or-n-p behaves correctly in keyboard
macros is included.

* src/dispnew.c (bitch_at_user): Always signal while in a keyboard
macro, even if in batch mode. (bug#67836)
* test/lisp/emacs-lisp/map-ynp-tests.el (map-ynp-tests-simple-call)
(test-map-ynp-kmacro): Add.
---
 src/dispnew.c                         |  6 ++--
 test/lisp/emacs-lisp/map-ynp-tests.el | 46 +++++++++++++++++++++++++++
 2 files changed, 49 insertions(+), 3 deletions(-)
 create mode 100644 test/lisp/emacs-lisp/map-ynp-tests.el

diff --git a/src/dispnew.c b/src/dispnew.c
index e4037494775..645311be8c0 100644
--- a/src/dispnew.c
+++ b/src/dispnew.c
@@ -6185,14 +6185,14 @@ DEFUN ("ding", Fding, Sding, 0, 1, 0,
 void
 bitch_at_user (void)
 {
-  if (noninteractive)
-    putchar (07);
-  else if (!INTERACTIVE)  /* Stop executing a keyboard macro.  */
+  if (!NILP (Vexecuting_kbd_macro))  /* Stop executing a keyboard macro.  */
     {
       const char *msg
 	= "Keyboard macro terminated by a command ringing the bell";
       Fsignal (Quser_error, list1 (build_string (msg)));
     }
+  else if (noninteractive)
+    putchar (07);
   else
     ring_bell (XFRAME (selected_frame));
 }
diff --git a/test/lisp/emacs-lisp/map-ynp-tests.el b/test/lisp/emacs-lisp/map-ynp-tests.el
new file mode 100644
index 00000000000..4f5d10ee7f9
--- /dev/null
+++ b/test/lisp/emacs-lisp/map-ynp-tests.el
@@ -0,0 +1,46 @@
+;;; map-ynp-tests.el --- Tests for map-ynp.el        -*- lexical-binding: t; -*-
+
+;; Copyright (C) 2023 Free Software Foundation, Inc.
+
+;; Author: Spencer Baugh <sbaugh@catern.com>
+;; Maintainer: emacs-devel@gnu.org
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;; Tests for map-ynp.el.
+
+;;; Code:
+
+(require 'ert)
+
+(defun map-ynp-tests-simple-call ()
+  (map-y-or-n-p ""  #'ignore '(1)))
+
+(ert-deftest test-map-ynp-kmacro ()
+  "Test that map-y-or-n-p in a kmacro terminates on end of input"
+  (execute-kbd-macro (read-kbd-macro "M-: (map-ynp-tests-simple-call) RET y"))
+  (should-error
+   (execute-kbd-macro (read-kbd-macro "M-: (map-ynp-tests-simple-call) RET")))
+  (unless noninteractive
+    (let ((noninteractive t))
+      (execute-kbd-macro (read-kbd-macro "M-: (map-ynp-tests-simple-call) RET y"))
+      (should-error
+       (execute-kbd-macro (read-kbd-macro "M-: (map-ynp-tests-simple-call) RET"))))))
+
+(provide 'map-ynp-tests)
+;;; map-ynp-tests.el ends here
-- 
2.39.3

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

[Eli Zaretskii]

> Cc: Stefan Monnier <monnier@iro.umontreal.ca>
> From: Spencer Baugh <sbaugh@janestreet.com>
> Date: Fri, 15 Dec 2023 10:44:47 -0500
> 
> Patch fixing this.

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.  If there's a particular problem with
map-y-or-n-p in batch mode, and if the scenario where you find the
problem is important enough, let's please find a solution that targets
only that special situation.

> 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.

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> 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.

>> 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


-- Stefan

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

[Eli Zaretskii]

> 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.

> >> 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.

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.

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.

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

[Eli Zaretskii]

> From: sbaugh@catern.com
> Date: Sat, 16 Dec 2023 13:13:25 +0000 (UTC)
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, sbaugh@janestreet.com,
> 	67836@debbugs.gnu.org
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> >> 
> >> 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.

One way is by mocking of functions that read input.  AFAIR we do that
in several places in the test suite (which I always run in batch
mode).

> 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?

Will look into the documentation issue, once we agree on resolving
this bug that way.

> 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.

It is definitely okay, and your work is certainly appreciated.  I'm
just trying to explain the POV of the Emacs maintainers, in the hope
that you could look for solutions in places other than low-level code
which is used all over Emacs, when problems are specific to some
higher-level API or specific situation.  That would make the review
and acceptance of the changes more efficient, and will probably
prevent you from doing extra unnecessary work.

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> >> 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.
>
> One way is by mocking of functions that read input.  AFAIR we do that
> in several places in the test suite (which I always run in batch
> mode).

Interesting!
I wrote a few test cases which needed such interaction, and I used
neither of those approaches: I relied on `unread-command-events`.
Take a look at

    grep unread-command-events test/**/*.el

For some examples.  Could you two point me to examples of uses of the
techniques you propose?  I'm curious to see how they compare.

BTW, regarding mocking, it might be a good idea to make sure
`bitch_at_user` is always called via `Qding` so that its behavior can be
adjusted via mocking.

>> 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.
>
> It is definitely okay, and your work is certainly appreciated.  I'm
> just trying to explain the POV of the Emacs maintainers, in the hope
> that you could look for solutions in places other than low-level code
> which is used all over Emacs, when problems are specific to some
> higher-level API or specific situation.  That would make the review
> and acceptance of the changes more efficient, and will probably
> prevent you from doing extra unnecessary work.

There's a tension where fixing such problems at low-level can have
longer term benefits (at the cost of backward incompatibilities), so
I think the best is to start by sending a patch that fixes the problem
at the place you judge to be The Right Place™.

Regarding `ding` in particular.  I don't really know what should be its
behavior in general.  I've always been surprised that it (usually)
doesn't actually signal an error even though its intention is to signal
to the user that there was a problem.

As a user, after I see/hear a "ding", I expect that Emacs has not done
any further processing.  Yet that's not what `ding` does.

IOW, I guess what I'd expect is that ELisp code basically never calls
`ding` directly but that it's called from the command-loop when it
catches an error.  And that suggests that maybe it should be the
command-loop's responsibility to exit the keyboard macro when it catches
an error, which in turn suggests that `bitch_at_user` when called from
a keyboard macro should signal a "real" error rather than a user-error.


        Stefan

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: sbaugh@catern.com,  sbaugh@janestreet.com,  67836@debbugs.gnu.org
> Date: Sat, 16 Dec 2023 10:11:47 -0500
> 
> > One way is by mocking of functions that read input.  AFAIR we do that
> > in several places in the test suite (which I always run in batch
> > mode).
> 
> Interesting!
> I wrote a few test cases which needed such interaction, and I used
> neither of those approaches: I relied on `unread-command-events`.
> Take a look at
> 
>     grep unread-command-events test/**/*.el

Yes, that is also possible.  But that will not fit Spencer's bill,
AFAIU.

> Could you two point me to examples of uses of the
> techniques you propose?  I'm curious to see how they compare.

I'd start by grepping our test suite for "mock".

> BTW, regarding mocking, it might be a good idea to make sure
> `bitch_at_user` is always called via `Qding` so that its behavior can be
> adjusted via mocking.

No objections.

> There's a tension where fixing such problems at low-level can have
> longer term benefits (at the cost of backward incompatibilities), so
> I think the best is to start by sending a patch that fixes the problem
> at the place you judge to be The Right Place™.

There's no disagreement on this level, the disagreement is about
where's The Right Place™ ;-)

> Regarding `ding` in particular.  I don't really know what should be its
> behavior in general.  I've always been surprised that it (usually)
> doesn't actually signal an error even though its intention is to signal
> to the user that there was a problem.

You don't think we should be able to "ding" without signaling an
error?  Ringing a bell is also a means to attract attention; on modern
GUI desktops, for example, this will produce some visual cue even when
the frame is minimized and otherwise invisible.

> IOW, I guess what I'd expect is that ELisp code basically never calls
> `ding` directly but that it's called from the command-loop when it
> catches an error.  And that suggests that maybe it should be the
> command-loop's responsibility to exit the keyboard macro when it catches
> an error, which in turn suggests that `bitch_at_user` when called from
> a keyboard macro should signal a "real" error rather than a user-error.

Since I disagree that 'ding' should only be a side effect of signaling
an error, I also disagree with your conclusion from that premise.

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> Could you two point me to examples of uses of the
>> techniques you propose?  I'm curious to see how they compare.
> I'd start by grepping our test suite for "mock".

Thanks.  I found some uses in `shadowfile-tests.el` as well as one in
`tramp-test47-read-password`.

That got me to

    grep "(symbol-function .\?'read" test/**/*.el

which points to many more uses.

>> There's a tension where fixing such problems at low-level can have
>> longer term benefits (at the cost of backward incompatibilities), so
>> I think the best is to start by sending a patch that fixes the problem
>> at the place you judge to be The Right Place™.
> There's no disagreement on this level, the disagreement is about
> where's The Right Place™ ;-)

But before submitting the bug&patch there's no way to know that.
It's best if we don't try to second guess what the other one will think
is best.  Instead, we start by stating what we judge to be best, and
then we can reconcile differences via discussions.

The only really important aspect is to accept that opinions can differ
and that we may not always get what we want (and have the humility to
accept that even when we're convinced we're right, because even in that
case, we may just be missing the point :-)

>> Regarding `ding` in particular.  I don't really know what should be its
>> behavior in general.  I've always been surprised that it (usually)
>> doesn't actually signal an error even though its intention is to signal
>> to the user that there was a problem.
> You don't think we should be able to "ding" without signaling an
> error?

I think we should, but in practice `ding` should almost never be called
from "normal" ELisp code.  Most ELisp code signals an error instead
(which is then caught by the command loop which in turn emits a ding).

From that point of view, the fact that `ding` itself signals an error
when used from a keyboard macro is a bit weird.


        Stefan

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: sbaugh@catern.com,  sbaugh@janestreet.com,  67836@debbugs.gnu.org
> Date: Sat, 16 Dec 2023 11:55:21 -0500
> 
> >> There's a tension where fixing such problems at low-level can have
> >> longer term benefits (at the cost of backward incompatibilities), so
> >> I think the best is to start by sending a patch that fixes the problem
> >> at the place you judge to be The Right Place™.
> > There's no disagreement on this level, the disagreement is about
> > where's The Right Place™ ;-)
> 
> But before submitting the bug&patch there's no way to know that.

I think there is: look at how general the original issue is, and
compare that with the range of applications in Emacs that the
proposed solution will affect.  That is what I do, and I don't suppose
you are saying that my judgment is arbitrary.

But I'm okay if that judgment is left to me, I just thought that
understanding my considerations well enough could make the process
more efficient.  If not, so be it.

> It's best if we don't try to second guess what the other one will think
> is best.  Instead, we start by stating what we judge to be best, and
> then we can reconcile differences via discussions.

That is true, but having heard the same arguments from me N times, I
presume one can guess what I will say one the N+1st opportunity.

> > You don't think we should be able to "ding" without signaling an
> > error?
> 
> I think we should, but in practice `ding` should almost never be called
> from "normal" ELisp code.

So you think we should be able to do that in theory but not in
practice? ;-)

> >From that point of view, the fact that `ding` itself signals an error
> when used from a keyboard macro is a bit weird.

That's a feature, I believe.