GNU bug report logs - #38796
26.3; `view-lossage': Use a variable for the lossage limit

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 38796 in the body.
You can then email your comments to 38796 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Drew Adams <drew.adams <at> oracle.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 26.3; `view-lossage': Use a variable for the lossage limit
Date: Sun, 29 Dec 2019 09:04:54 -0800 (PST)

This report is a follow-up to this emacs-devel thread:

https://lists.gnu.org/archive/html/emacs-devel/2019-12/msg00678.html

and to the older thread that it references:

https://lists.gnu.org/archive/html/emacs-devel/2006-03/msg01012.html

Please use a variable for the limit of how many characters to use for
recording lossage.

Doing that will give users (and Lisp code) control over the limit.
Having a limit that is hard-coded in C is not user-friendly - it's even
unemacsy.

The latter thread mentioned above ended with RMS's decision to have this
taken care of after the release that was then incipient.  But this was
never done.

I'd prefer to see this as a user option, but an ordinary defvar would
already be a great improvement.

I don't think it's uncommon to have mistakenly typed some keys, gotten
in a strange or unfamiliar situation as a result of that, and then tried
to find out just what keys led to that.  Depending on what else you did
in the meantime, before hitting `C-h l' (e.g., to get out of the strange
situation), `view-lossage' might not reach far enough backward to show
you the mistake you made.

It shouldn't be a big deal to move this limit to a Lisp variable.  Can
this please be done now?

---

Not part of this bug report, but `view-lossage' could also be improved
by letting you (or Lisp code) filter the recorded "keystroke" history,
to remove stuff that you feel is really extraneous (e.g. not keyboard
keystrokes).  Such filtering could be just hiding that from view.

(This is all the more relevant if we can control the lossage limit.  The
really relevant user input is sometimes a relatively small subset of the
full list of recorded events you see now with `C-h l'.)


In GNU Emacs 26.3 (build 1, x86_64-w64-mingw32)
 of 2019-08-29
Repository revision: 96dd0196c28bc36779584e47fffcca433c9309cd
Windowing system distributor `Microsoft Corp.', version 10.0.17763
Configured using:
 `configure --without-dbus --host=x86_64-w64-mingw32
 --without-compress-install 'CFLAGS=-O2 -static -g3''

Message #8 received at 38796 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Drew Adams <drew.adams <at> oracle.com>
Cc: 38796 <at> debbugs.gnu.org
Subject: Re: bug#38796: 26.3;
 `view-lossage': Use a variable for the lossage limit
Date: Sun, 29 Dec 2019 19:30:15 +0200

severity 38796 wishlist
thanks

> Date: Sun, 29 Dec 2019 09:04:54 -0800 (PST)
> From: Drew Adams <drew.adams <at> oracle.com>
> 
> This report is a follow-up to this emacs-devel thread:
> 
> https://lists.gnu.org/archive/html/emacs-devel/2019-12/msg00678.html
> 
> and to the older thread that it references:
> 
> https://lists.gnu.org/archive/html/emacs-devel/2006-03/msg01012.html
> 
> Please use a variable for the limit of how many characters to use for
> recording lossage.
> 
> Doing that will give users (and Lisp code) control over the limit.
> Having a limit that is hard-coded in C is not user-friendly - it's even
> unemacsy.

It's a constant size of a vector that records the keys in a cyclical
fashion.  So it isn't enough to expose the value to Lisp; one must
also write code that reallocates the vector when the value changes,
and copies the keystrokes from the old to the new vector in the
correct order.  Not rocket science, but still.

Patches are welcome.

Message #13 received at 38796 <at> debbugs.gnu.org (full text, mbox):

From: Drew Adams <drew.adams <at> oracle.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 38796 <at> debbugs.gnu.org
Subject: RE: bug#38796: 26.3; `view-lossage': Use a variable for the lossage
 limit
Date: Sun, 29 Dec 2019 09:34:17 -0800 (PST)

> It's a constant size of a vector that records the keys in a cyclical
> fashion.  So it isn't enough to expose the value to Lisp; one must
> also write code that reallocates the vector when the value changes,
> and copies the keystrokes from the old to the new vector in the
> correct order.  Not rocket science, but still.

I see.  I hope such an enhancement is made.

> Patches are welcome.

A C patch will have to come from someone other than me.

Message #16 received at 38796 <at> debbugs.gnu.org (full text, mbox):

From: Tino Calancha <tino.calancha <at> gmail.com>
To: 38796 <at> debbugs.gnu.org
Cc: Eli Zaretskii <eliz <at> gnu.org>, tino.calancha <at> gmail.com,
 Stefan Monnier <monnier <at> iro.umontreal.ca>, 38796 <at> debbugs.gnu.org,
 uyennhi.qm <at> gmail.com
Subject: Re: bug#38796: 26.3; `view-lossage': Use a variable for the lossage
 limit
Date: Fri, 26 Jun 2020 23:58:01 +0200

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Drew Adams <drew.adams <at> oracle.com>
>> This report is a follow-up to this emacs-devel thread:
>> https://lists.gnu.org/archive/html/emacs-devel/2019-12/msg00678.html
>> and to the older thread that it references:
>> https://lists.gnu.org/archive/html/emacs-devel/2006-03/msg01012.html
>> Please use a variable for the limit of how many characters to use for
>> recording lossage.

> it isn't enough to expose the value to Lisp; one must
> also write code that reallocates the vector when the value changes,
> and copies the keystrokes from the old to the new vector in the
> correct order.
>
> Patches are welcome.

I have uploaded the branch feature/bug#38796-lossage-limit
[I've been using it during last 10 days with no issues]

Initially I decided to hardcode a sensible minimum of 100 (keeping
the current default of 300).  That is the first commit.
Actually, the code works for N > 0, but as Drew suggested, a value < 100
(which gives only ~ 50 lines of past inputs) is too small to be useful
for the use case of checking typing mistakes.

After some time I thought that, for some people (anyone in this thread),
a very small value in fact can be seen as a security feature;
see for instance the docstrings of `term-read-noecho' and
`comint-send-invisible'.  That motivates the second commit. 

--8<-----------------------------cut here---------------start------------->8---
commit 9723c2645c8f47e651a81a023e80f882d181cc3f
Author: Tino Calancha <tino.calancha <at> gmail.com>
Date:   Sun Jun 7 23:37:59 2020 +0200

    Give Lisp control on the lossage size
    
    Add an user option to control the maximum number of
    recorded keystrokes (a.k.a lossage limit) (Bug#38796).
    
    * src/keyboard.c (lossage-limit): Add new variable.
    (MIN_NUM_RECENT_KEYS): Renamed from NUM_RECENT_KEYS.
    Set it as 100 and use it as the minimum value for lossage-limit.
    Keep the same default for the vector size as before (300).
    (update-lossage-limit): New function.
    (update_recent_keys): Helper function.
    (command_loop_1)
    (record_char)
    (recent-keys)
    (syms_of_keyboard): Replace NUM_RECENT_KEYS with lossage_limit as
    the vector size.
    (clear-this-command-keys): Fix docstring.
    
    * lisp/help.el (view-lossage): Mention lossage-limit in the docstring.
    
    * lisp/cus-start.el (lossage-limit): Register it as an user option.
    
    * lisp/edmacro.el (edit-kbd-macro): Update docstring and commentary header.
    
    * etc/NEWS (Changes in Emacs 28.1): Announce the new option.
    * doc/emacs/help.texi (Misc Help): Document it.
    * test/src/keyboard-tests.el (keyboard-lossage-limit): Add test.

diff --git a/doc/emacs/help.texi b/doc/emacs/help.texi
index 167c32c4d2..e2c0dc6802 100644
--- a/doc/emacs/help.texi
+++ b/doc/emacs/help.texi
@@ -563,10 +563,13 @@ Misc Help
 
 @kindex C-h l
 @findex view-lossage
+@vindex lossage-limit
   If something surprising happens, and you are not sure what you typed,
 use @kbd{C-h l} (@code{view-lossage}).  @kbd{C-h l} displays your last
-300 input keystrokes and the commands they invoked.  If you see
-commands that you are not familiar with, you can use @kbd{C-h k} or
+input keystrokes and the commands they invoked.  By default, Emacs
+stores the last 300 events; if you wish, you can change this number
+with the option @code{lossage-limit}.
+If you see commands that you are not familiar with, you can use @kbd{C-h k} or
 @kbd{C-h f} to find out what they do.
 
 @kindex C-h e
diff --git a/etc/NEWS b/etc/NEWS
index d58a61be21..da204c5825 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -70,6 +70,10 @@ useful on systems such as FreeBSD which ships only with "etc/termcap".
 
 * Changes in Emacs 28.1
 
++++
+** The new option 'lossage-limit' controls the maximum number
+of keystrokes and commands recorded.
+
 ** Support for '(box . SIZE)' 'cursor-type'.
 By default, 'box' cursor always has a filled box shape.  But if you
 specify 'cursor-type' to be '(box . SIZE)', the cursor becomes a hollow
diff --git a/lisp/cus-start.el b/lisp/cus-start.el
index 6632687da4..1ebf554b2b 100644
--- a/lisp/cus-start.el
+++ b/lisp/cus-start.el
@@ -352,6 +352,8 @@ minibuffer-prompt-properties--setter
 	     ;; indent.c
 	     (indent-tabs-mode indent boolean)
 	     ;; keyboard.c
+             (lossage-limit keyboard integer "28.1"
+                            :set (lambda (_ val) (update-lossage-limit val)))
 	     (meta-prefix-char keyboard character)
 	     (auto-save-interval auto-save integer)
              (auto-save-no-message auto-save boolean "27.1")
diff --git a/lisp/edmacro.el b/lisp/edmacro.el
index 71474c0289..089fea17ef 100644
--- a/lisp/edmacro.el
+++ b/lisp/edmacro.el
@@ -35,8 +35,8 @@
 ;;  * `M-x' followed by a command name, to edit a named command
 ;;    whose definition is a keyboard macro.
 ;;
-;;  * `C-h l' (view-lossage), to edit the 300 most recent keystrokes
-;;    and install them as the "current" macro.
+;;  * `C-h l' (view-lossage), to edit the `lossage-limit' most recent
+;;    keystrokes and install them as the "current" macro.
 ;;
 ;;  * any key sequence whose definition is a keyboard macro.
 ;;
@@ -88,7 +88,7 @@ edit-kbd-macro
   "Edit a keyboard macro.
 At the prompt, type any key sequence which is bound to a keyboard macro.
 Or, type `\\[kmacro-end-and-call-macro]' or RET to edit the last
-keyboard macro, `\\[view-lossage]' to edit the last 300
+keyboard macro, `\\[view-lossage]' to edit the last `lossage-limit'
 keystrokes as a keyboard macro, or `\\[execute-extended-command]'
 to edit a macro by its command name.
 With a prefix argument, format the macro in a more concise way."
diff --git a/lisp/help.el b/lisp/help.el
index b7d867eb70..9e3d0922d0 100644
--- a/lisp/help.el
+++ b/lisp/help.el
@@ -456,9 +456,10 @@ view-external-packages
   (info "(efaq)Packages that do not come with Emacs"))
 
 (defun view-lossage ()
-  "Display last few input keystrokes and the commands run.
+  "Display last input keystrokes and the commands run.
 For convenience this uses the same format as
 `edit-last-kbd-macro'.
+See `lossage-limit' to update the number of recorded keystrokes.
 
 To record all your input, use `open-dribble-file'."
   (interactive)
diff --git a/src/keyboard.c b/src/keyboard.c
index f9b9399d50..a4d27c94fb 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -103,7 +103,7 @@ static KBOARD *all_kboards;
 /* True in the single-kboard state, false in the any-kboard state.  */
 static bool single_kboard;
 
-#define NUM_RECENT_KEYS (300)
+#define MIN_NUM_RECENT_KEYS (100)
 
 /* Index for storing next element into recent_keys.  */
 static int recent_keys_index;
@@ -111,7 +111,7 @@ static int recent_keys_index;
 /* Total number of elements stored into recent_keys.  */
 static int total_keys;
 
-/* This vector holds the last NUM_RECENT_KEYS keystrokes.  */
+/* This vector holds the last lossage_limit keystrokes.  */
 static Lisp_Object recent_keys;
 
 /* Vector holding the key sequence that invoked the current command.
@@ -294,6 +294,7 @@ static union buffered_input_event *kbd_fetch_ptr;
 /* Pointer to next place to store character in kbd_buffer.  */
 static union buffered_input_event *kbd_store_ptr;
 
+
 /* The above pair of variables forms a "queue empty" flag.  When we
    enqueue a non-hook event, we increment kbd_store_ptr.  When we
    dequeue a non-hook event, we increment kbd_fetch_ptr.  We say that
@@ -1421,10 +1422,10 @@ command_loop_1 (void)
       /* Execute the command.  */
 
       {
-	total_keys += total_keys < NUM_RECENT_KEYS;
+	total_keys += total_keys < lossage_limit;
 	ASET (recent_keys, recent_keys_index,
 	      Fcons (Qnil, cmd));
-	if (++recent_keys_index >= NUM_RECENT_KEYS)
+	if (++recent_keys_index >= lossage_limit)
 	  recent_keys_index = 0;
       }
       Vthis_command = cmd;
@@ -3248,15 +3249,15 @@ record_char (Lisp_Object c)
       int ix1, ix2, ix3;
 
       if ((ix1 = recent_keys_index - 1) < 0)
-	ix1 = NUM_RECENT_KEYS - 1;
+	ix1 = lossage_limit - 1;
       ev1 = AREF (recent_keys, ix1);
 
       if ((ix2 = ix1 - 1) < 0)
-	ix2 = NUM_RECENT_KEYS - 1;
+	ix2 = lossage_limit - 1;
       ev2 = AREF (recent_keys, ix2);
 
       if ((ix3 = ix2 - 1) < 0)
-	ix3 = NUM_RECENT_KEYS - 1;
+	ix3 = lossage_limit - 1;
       ev3 = AREF (recent_keys, ix3);
 
       if (EQ (XCAR (c), Qhelp_echo))
@@ -3307,12 +3308,12 @@ record_char (Lisp_Object c)
     {
       if (!recorded)
 	{
-	  total_keys += total_keys < NUM_RECENT_KEYS;
+	  total_keys += total_keys < lossage_limit;
 	  ASET (recent_keys, recent_keys_index,
                 /* Copy the event, in case it gets modified by side-effect
                    by some remapping function (bug#30955).  */
                 CONSP (c) ? Fcopy_sequence (c) : c);
-	  if (++recent_keys_index >= NUM_RECENT_KEYS)
+	  if (++recent_keys_index >= lossage_limit)
 	    recent_keys_index = 0;
 	}
       else if (recorded < 0)
@@ -3326,10 +3327,10 @@ record_char (Lisp_Object c)
 
 	  while (recorded++ < 0 && total_keys > 0)
 	    {
-	      if (total_keys < NUM_RECENT_KEYS)
+	      if (total_keys < lossage_limit)
 		total_keys--;
 	      if (--recent_keys_index < 0)
-		recent_keys_index = NUM_RECENT_KEYS - 1;
+		recent_keys_index = lossage_limit - 1;
 	      ASET (recent_keys, recent_keys_index, Qnil);
 	    }
 	}
@@ -10410,6 +10411,55 @@ If CHECK-TIMERS is non-nil, timers that are ready to run will do so.  */)
 	  ? Qt : Qnil);
 }
 
+static void
+update_recent_keys (int new_size, int kept_keys)
+{
+  int osize = ASIZE (recent_keys);
+  eassert (recent_keys_index < osize);
+  eassert (kept_keys <= min (osize, new_size));
+  Lisp_Object v = make_nil_vector (new_size);
+  int i, idx;
+  for (i = 0; i < kept_keys; ++i)
+    {
+      idx = recent_keys_index - kept_keys + i;
+      while (idx < 0)
+        idx += osize;
+      ASET (v, i, AREF (recent_keys, idx));
+    }
+  recent_keys = v;
+  total_keys = kept_keys;
+  recent_keys_index = total_keys % new_size;
+  lossage_limit = new_size;
+
+}
+
+/* Reallocate recent_keys copying the keystrokes in the right order */
+DEFUN ("update-lossage-limit", Fupdate_lossage_limit,
+       Supdate_lossage_limit, 1, 1, 0,
+       doc: /* Update the maximum number of saved keystrokes to ARG.
+usage: (update-lossage-limit ARG)  */)
+  (Lisp_Object arg)
+{
+  if (!FIXNATP (arg))
+    user_error ("Value must be a positive integer");
+  int osize = ASIZE (recent_keys);
+  eassert (lossage_limit == osize);
+  int min_size = MIN_NUM_RECENT_KEYS;
+  int new_size = XFIXNAT (arg);
+
+  if (new_size == osize)
+    return Qnil;
+  if (new_size < min_size)
+    {
+      AUTO_STRING (fmt, "Value must be >= %d");
+      Fsignal (Quser_error, list1 (CALLN (Fformat, fmt, make_fixnum (min_size))));
+    }
+  int kept_keys = new_size > osize ? total_keys : min (new_size, total_keys);
+  update_recent_keys (new_size, kept_keys);
+
+  return Qnil;
+}
+
 DEFUN ("recent-keys", Frecent_keys, Srecent_keys, 0, 1, 0,
        doc: /* Return vector of last few events, not counting those from keyboard macros.
 If INCLUDE-CMDS is non-nil, include the commands that were run,
@@ -10419,21 +10469,21 @@ represented as pseudo-events of the form (nil . COMMAND).  */)
   bool cmds = !NILP (include_cmds);
 
   if (!total_keys
-      || (cmds && total_keys < NUM_RECENT_KEYS))
+      || (cmds && total_keys < lossage_limit))
     return Fvector (total_keys,
 		    XVECTOR (recent_keys)->contents);
   else
     {
       Lisp_Object es = Qnil;
-      int i = (total_keys < NUM_RECENT_KEYS
+      int i = (total_keys < lossage_limit
 	       ? 0 : recent_keys_index);
-      eassert (recent_keys_index < NUM_RECENT_KEYS);
+      eassert (recent_keys_index < lossage_limit);
       do
 	{
 	  Lisp_Object e = AREF (recent_keys, i);
 	  if (cmds || !CONSP (e) || !NILP (XCAR (e)))
 	    es = Fcons (e, es);
-	  if (++i >= NUM_RECENT_KEYS)
+	  if (++i >= lossage_limit)
 	    i = 0;
 	} while (i != recent_keys_index);
       es = Fnreverse (es);
@@ -10531,8 +10581,8 @@ The value is always a vector.  */)
 DEFUN ("clear-this-command-keys", Fclear_this_command_keys,
        Sclear_this_command_keys, 0, 1, 0,
        doc: /* Clear out the vector that `this-command-keys' returns.
-Also clear the record of the last 100 events, unless optional arg
-KEEP-RECORD is non-nil.  */)
+Also clear the record of the last `lossage-limit' keystroke events,
+unless optional arg KEEP-RECORD is non-nil.  */)
   (Lisp_Object keep_record)
 {
   int i;
@@ -11686,7 +11736,23 @@ syms_of_keyboard (void)
     staticpro (&modifier_symbols);
   }
 
-  recent_keys = make_nil_vector (NUM_RECENT_KEYS);
+  DEFVAR_INT ("lossage-limit", lossage_limit,
+              doc: /* Maximum number of stored keyboard events and commands run.
+
+Please, do not set this variable in Lisp with `setq' neither
+let-bind it, that will likely crash Emacs.  This is because
+`setq' only changes the variable, but it doesn't update
+the size of the internal vector that holds the keystrokes.
+
+To update this variable use the customization menu, or
+call from Lisp the following expression:
+
+  (update-lossage-limit new-limit)
+
+That takes care of both, the variable and the internal vector.*/);
+  lossage_limit = 3 * MIN_NUM_RECENT_KEYS;
+
+  recent_keys = make_nil_vector (lossage_limit);
   staticpro (&recent_keys);
 
   this_command_keys = make_nil_vector (40);
@@ -11736,6 +11802,7 @@ syms_of_keyboard (void)
   defsubr (&Srecursive_edit);
   defsubr (&Sinternal_track_mouse);
   defsubr (&Sinput_pending_p);
+  defsubr (&Supdate_lossage_limit);
   defsubr (&Srecent_keys);
   defsubr (&Sthis_command_keys);
   defsubr (&Sthis_command_keys_vector);
diff --git a/test/src/keyboard-tests.el b/test/src/keyboard-tests.el
index 1988ba51a7..017d239246 100644
--- a/test/src/keyboard-tests.el
+++ b/test/src/keyboard-tests.el
@@ -32,5 +32,16 @@
                         (read-event nil nil 2))
                  ?\C-b)))
 
+(ert-deftest keyboard-lossage-limit ()
+  "Test `lossage-limit' updates."
+  (dolist (val (list 100 100 200 500 300 1000 700))
+    (update-lossage-limit val)
+    (should (= val lossage-limit)))
+  (let ((current-limit lossage-limit))
+    (should-error (update-lossage-limit 5))
+    (should-error (update-lossage-limit "200"))
+    (should (= lossage-limit current-limit))))
+
+
 (provide 'keyboard-tests)
 ;;; keyboard-tests.el ends here

commit a60a05994aff16bc27f153ea8f765e15b92f21be
Author: Tino Calancha <tino.calancha <at> gmail.com>
Date:   Mon Jun 22 22:41:34 2020 +0200

    Allow disable the record of keystrokes (lossage)
    
    Use 1 as the minimum value for lossage-limit; such a value
    is equivalent to not recording the keystrokes: having just 1 entry,
    will be overwritten with the view-lossage call itself.
    
    * test/src/keyboard-tests.el (keyboard-lossage-limit): Update test.
    * src/keyboard.c (MIN_NUM_RECENT_KEYS): Delete it.
    (lossage_limit): Add security note in the doctring.
    * lisp/cus-start.el (lossage-limit): Let users choose to disable
    the record of the keystrokes.
    * doc/emacs/help.texi (Misc Help): Update manual.
    * etc/NEWS (Changes in Emacs 28.1):
    Mention that it's possible to disable the record of keystrokes.

diff --git a/doc/emacs/help.texi b/doc/emacs/help.texi
index e2c0dc6802..69ea96763b 100644
--- a/doc/emacs/help.texi
+++ b/doc/emacs/help.texi
@@ -569,8 +569,14 @@ Misc Help
 input keystrokes and the commands they invoked.  By default, Emacs
 stores the last 300 events; if you wish, you can change this number
 with the option @code{lossage-limit}.
-If you see commands that you are not familiar with, you can use @kbd{C-h k} or
-@kbd{C-h f} to find out what they do.
+If you see commands that you are not familiar with, you can use @kbd{C-h k}
+or @kbd{C-h f} to find out what they do.
+If you don't like that Emacs saves your keystrokes, then you can
+set @code{lossage-limit} equal to 1; such a value effectively disables the
+record of the keystrokes.  Please, do not set this option with @code{setq}
+neither let-bind it; that will likely crash Emacs.  Use instead the
+customization menu, which also updates the internal structure holding
+the keystrokes.
 
 @kindex C-h e
 @findex view-echo-area-messages
diff --git a/etc/NEWS b/etc/NEWS
index da204c5825..17776173e0 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -72,7 +72,8 @@ useful on systems such as FreeBSD which ships only with "etc/termcap".
 
 +++
 ** The new option 'lossage-limit' controls the maximum number
-of keystrokes and commands recorded.
+of keystrokes and commands recorded.  The value 1 disables
+the record of keystrokes.
 
 ** Support for '(box . SIZE)' 'cursor-type'.
 By default, 'box' cursor always has a filled box shape.  But if you
diff --git a/lisp/cus-start.el b/lisp/cus-start.el
index 1ebf554b2b..bd4ff3a74b 100644
--- a/lisp/cus-start.el
+++ b/lisp/cus-start.el
@@ -352,7 +352,11 @@ minibuffer-prompt-properties--setter
 	     ;; indent.c
 	     (indent-tabs-mode indent boolean)
 	     ;; keyboard.c
-             (lossage-limit keyboard integer "28.1"
+             (lossage-limit keyboard
+                            (choice (const :tag "Do not record keystrokes" 1)
+                                    integer)
+                            "28.1"
+                            :standard 300
                             :set (lambda (_ val) (update-lossage-limit val)))
 	     (meta-prefix-char keyboard character)
 	     (auto-save-interval auto-save integer)
diff --git a/src/keyboard.c b/src/keyboard.c
index a4d27c94fb..9af1b9ba50 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -103,8 +103,6 @@ static KBOARD *all_kboards;
 /* True in the single-kboard state, false in the any-kboard state.  */
 static bool single_kboard;
 
-#define MIN_NUM_RECENT_KEYS (100)
-
 /* Index for storing next element into recent_keys.  */
 static int recent_keys_index;
 
@@ -10444,7 +10442,7 @@ usage: (update-lossage-limit ARG)  */)
     user_error ("Value must be a positive integer");
   int osize = ASIZE (recent_keys);
   eassert (lossage_limit == osize);
-  int min_size = MIN_NUM_RECENT_KEYS;
+  int min_size = 1;
   int new_size = XFIXNAT (arg);
 
   if (new_size == osize)
@@ -11749,8 +11747,11 @@ call from Lisp the following expression:
 
   (update-lossage-limit new-limit)
 
-That takes care of both, the variable and the internal vector.*/);
-  lossage_limit = 3 * MIN_NUM_RECENT_KEYS;
+That takes care of both, the variable and the internal vector.
+
+Security note: The value 1 makes impossible to recover a typed string
+with `view-lossage'.*/);
+  lossage_limit = 300;
 
   recent_keys = make_nil_vector (lossage_limit);
   staticpro (&recent_keys);
diff --git a/test/src/keyboard-tests.el b/test/src/keyboard-tests.el
index 017d239246..4541c389d0 100644
--- a/test/src/keyboard-tests.el
+++ b/test/src/keyboard-tests.el
@@ -38,7 +38,7 @@
     (update-lossage-limit val)
     (should (= val lossage-limit)))
   (let ((current-limit lossage-limit))
-    (should-error (update-lossage-limit 5))
+    (should-error (update-lossage-limit 0))
     (should-error (update-lossage-limit "200"))
     (should (= lossage-limit current-limit))))
 

--8<-----------------------------cut here---------------end--------------->8---

In GNU Emacs 28.0.50 (build 18, x86_64-pc-linux-gnu, GTK+ Version 3.24.5, cairo version 1.16.0)
 of 2020-06-26 built on calancha-pc.dy.bbexcite.jp
Repository revision: ffb89ed5f07491e33fc79d8b4be49d9deba2ad4a
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12004000
System Description: Debian GNU/Linux 10 (buster)

Message #19 received at 38796 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Tino Calancha <tino.calancha <at> gmail.com>
Cc: monnier <at> iro.umontreal.ca, 38796 <at> debbugs.gnu.org, uyennhi.qm <at> gmail.com
Subject: Re: bug#38796: 26.3; `view-lossage': Use a variable for the lossage
 limit
Date: Sat, 27 Jun 2020 11:32:57 +0300

> From: Tino Calancha <tino.calancha <at> gmail.com>
> Cc: 38796 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>, Stefan Monnier
>  <monnier <at> iro.umontreal.ca>,uyennhi.qm <at> gmail.com, tino.calancha <at> gmail.com
> Date: Fri, 26 Jun 2020 23:58:01 +0200
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> >> From: Drew Adams <drew.adams <at> oracle.com>
> >> This report is a follow-up to this emacs-devel thread:
> >> https://lists.gnu.org/archive/html/emacs-devel/2019-12/msg00678.html
> >> and to the older thread that it references:
> >> https://lists.gnu.org/archive/html/emacs-devel/2006-03/msg01012.html
> >> Please use a variable for the limit of how many characters to use for
> >> recording lossage.
> 
> > it isn't enough to expose the value to Lisp; one must
> > also write code that reallocates the vector when the value changes,
> > and copies the keystrokes from the old to the new vector in the
> > correct order.
> >
> > Patches are welcome.
> 
> I have uploaded the branch feature/bug#38796-lossage-limit
> [I've been using it during last 10 days with no issues]
> 
> Initially I decided to hardcode a sensible minimum of 100 (keeping
> the current default of 300).  That is the first commit.
> Actually, the code works for N > 0, but as Drew suggested, a value < 100
> (which gives only ~ 50 lines of past inputs) is too small to be useful
> for the use case of checking typing mistakes.
> 
> After some time I thought that, for some people (anyone in this thread),
> a very small value in fact can be seen as a security feature;
> see for instance the docstrings of `term-read-noecho' and
> `comint-send-invisible'.  That motivates the second commit. 

Thanks.  Some initial comments below:

 . The original request was for allowing to keep _more_ than just 300
   keystrokes, there was no request for recording less than that
   number.  I'm not sure we should allow reducing the number; if
   someone thinks it could be a security issue, then we should have a
   separate feature that disabled recording the keys, not as a side
   effect of reducing the vector size.

 . The size of the vector shouldn't be exposed to Lisp, there's no
   reason for doing that.  You say in the doc string of the variable
   not to set nor bind it, but documenting the restriction is not
   enough, as someone, perhaps even some malicious code, could just do
   what you ask not to.  Instead, there should be a function to get
   the value and a function to set the value (which should also resize
   the vector as a side effect).

 . Using 1 as the value that disables recording is unnatural; it might
   be convenient from the implementation POV, but this implementation
   convenience cannot justify asking the users to remember such a
   strange convention.  The usual way of disabling a feature is either
   by setting the value to zero or using a suitable symbol, like nil.
   (But I'm not sure we should provide this feature as part of this
   changeset, see below.)

 . Last, but not least: if we are going to allow to disable recording
   of keys, we should carefully audit all the places that call
   recent-keys, and make sure they will not break due to such a
   radical change.

My personal view is that we should allow only growing the size and
resetting it to the original size of 300.  Disabling the key record
should be a separate feature, most probably implemented by means other
than shrinking the recent_keys vector.

Thanks.

Message #22 received at 38796 <at> debbugs.gnu.org (full text, mbox):

From: Tino Calancha <tino.calancha <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: uyennhi.qm <at> gmail.com, monnier <at> iro.umontreal.ca, 38796 <at> debbugs.gnu.org,
 Tino Calancha <tino.calancha <at> gmail.com>
Subject: Re: bug#38796: 26.3; `view-lossage': Use a variable for the lossage
 limit
Date: Sun, 28 Jun 2020 18:55:09 +0200 (CEST)

On Sat, 27 Jun 2020, Eli Zaretskii wrote:

> My personal view is that we should allow only growing the size and
> resetting it to the original size of 300.  Disabling the key record
> should be a separate feature, most probably implemented by means other
> than shrinking the recent_keys vector.

I totally agree: they are clearly 2 separated features.

Thanks for the comments, I will work on them and get back once
completed and tested.

Message #25 received at 38796 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Tino Calancha <tino.calancha <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, uyennhi.qm <at> gmail.com, 38796 <at> debbugs.gnu.org
Subject: Re: bug#38796: 26.3; `view-lossage': Use a variable for the lossage
 limit
Date: Sun, 28 Jun 2020 14:00:33 -0400

>> My personal view is that we should allow only growing the size and
>> resetting it to the original size of 300.  Disabling the key record
>> should be a separate feature, most probably implemented by means other
>> than shrinking the recent_keys vector.
>
> I totally agree: they are clearly 2 separated features.
>
> Thanks for the comments, I will work on them and get back once
> completed and tested.

I agree that disabling should not necessarily be implemented by
"abusing" the max-lossage setting.

But I don't see any reason to impose a 300 minimum either.  I think it's
fine to impose a minimum, but it should be dictated by the limits of the
code.  I'm not saying we should work to push this lower limit down, but
just that it should reflect what the code can do safely rather than
being an arbitrary number like 300 (I'm pretty sure 100 would be safe as
well, since that's what we've used for many years).


        Stefan

Message #28 received at 38796 <at> debbugs.gnu.org (full text, mbox):

From: Drew Adams <drew.adams <at> oracle.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>, Tino Calancha
 <tino.calancha <at> gmail.com>
Cc: uyennhi.qm <at> gmail.com, 38796 <at> debbugs.gnu.org
Subject: RE: bug#38796: 26.3; `view-lossage': Use a variable for the lossage
 limit
Date: Sun, 28 Jun 2020 20:01:07 +0000 (UTC)

> I agree that disabling should not necessarily be implemented by
> "abusing" the max-lossage setting.
> 
> But I don't see any reason to impose a 300 minimum either.  I think it's
> fine to impose a minimum, but it should be dictated by the limits of the
> code.  I'm not saying we should work to push this lower limit down, but
> just that it should reflect what the code can do safely rather than
> being an arbitrary number like 300 (I'm pretty sure 100 would be safe as
> well, since that's what we've used for many years).

OP here.

 ++++
 +** The new option 'lossage-limit' controls the
 +maximum number of keystrokes and commands recorded.

I don't feel strongly about what's done wrt low values
or turning such logging off.  As Eli said, the point
of the enhancement request is to let users control the
max, in particular to be able to make it _larger_ than
the default (300).

I think I understand why some have argued for a second
option for turning logging off.  But I don't think
it's a great argument (IIUC).

I agree with Stefan that disabling should not
_necessarily_ be carried out by setting the max limit
(e.g. to 0).

Not _necessary_, but isn't that logical, from a user
point of view?  I wouldn't call that "abusing" the
max-limit option.

1. In general in Emacs, setting a maximum of zero does
   (and should) do just what it says: not allow ANY
   <whatevers>.

2. Having 2 options here goes against Occam, and is
   likely to lead to some confusion (perhaps even some
   mistakes, in terms of security?).

   It'll require the doc of each option to mention the
   other, in hopes that users will consult both.  Iffy
   if really you see a security problem here.

   It'll mean that the defcustom for the max one needs
   to prevent a value as low as zero, in order to avoid
   misunderstanding.  E.g., what would it even mean for
   a history of length zero that does not, in effect,
   disable logging?

Is the real reason for not letting zero turn off
logging a C-implementation reason?  What's really
wrong, from a user point of view, with doing that?

Again, this isn't super important.  The request
was for users to be able to customize the length,
in particular to be able to _increase_ it.

But if we allow such length customization, why
complicate things by adding another option for
getting the natural effect of length zero, i.e.,
turning logging off?

I'm more often in the position of arguing for
more, not fewer, options, even when they combine
to control something.  But this time (until I
understand better perhaps), that's not the case.
___

Another possibility is to have, for the same
option, a specific value other than zero, to
indicate disabling.  E.g. `disable' or some other
non-number value.  But then you have the same
confusion, if the numeric value can itself be 0.
___

Although it's not part of this enhancement request,
as was mentioned from the outset the main problem
with `view-lossage' output (of any length) is the
low signal-to-noise ratio.  It would really be good
to be able to (on the fly) filter out certain kinds
of input, in particular kinds of non-keyboard input.

Besides on-the-fly filtering, it might be good to
have a user variable, to define a preference for
(default) filtering.

Another nice-to-have would be a way (e.g. filter)
to not show the commands long with their keys.
For someone who knows the commands associated with
keys, they are pretty much noise.  And we could
perhaps put links on the keys to their commands in
the current keymap context.

Increasing the logging length is only one possible
improvement.  In general, we should be looking for
ways to help users see what they think is important
in a log - ways to show/hide different things.

Message #31 received at 38796 <at> debbugs.gnu.org (full text, mbox):

From: Tino Calancha <tino.calancha <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Eli Zaretskii <eliz <at> gnu.org>, uyennhi.qm <at> gmail.com, 38796 <at> debbugs.gnu.org
Subject: Re: bug#38796: 26.3; `view-lossage': Use a variable for the lossage
 limit
Date: Sun, 28 Jun 2020 23:52:57 +0200

On Sun, 28 Jun 2020, Drew Adams wrote:
> OP here.
I was waiting for you.  Welcome!! :-)


Stefan Monnier <monnier <at> iro.umontreal.ca> writes:

> I agree that disabling should not necessarily be implemented by
> "abusing" the max-lossage setting.
Yeah, it was appealing to me at first look: the code was offering
another feature for free!
After your comments I rethink about it and become less excited: it
always posible that we might be hit back in other parts of the code
if they assume a 'sensible large enough' value.

> But I don't see any reason to impose a 300 minimum either.  I think it's
> fine to impose a minimum, but it should be dictated by the limits of the
> code.  I'm not saying we should work to push this lower limit down, but
> just that it should reflect what the code can do safely rather than
> being an arbitrary number like 300 (I'm pretty sure 100 would be safe as
> well, since that's what we've used for many years).
Sure, 100 must be absolutely fine.

I have updated the branch 'bug#38796-lossage-limit'.
Now, the limit is not exposed to Lisp anymore.
The function to update it now is a command 'update-lossage-size'
We also have a 'lossage-size' function to retrieve its current value.

--8<-----------------------------cut here---------------start------------->8---
commit c86d060b460bdaecdd71d297fa84012fcd397d66
Author: Tino Calancha <tino.calancha <at> gmail.com>
Date:   Sun Jun 28 23:30:07 2020 +0200

    Do not expose the size of recent_keys to Lisp
    
    That prevents from unintentional crashes if the users
    modify the variable with setq or if they let-bind it.
    
    Users can still safely modify the lossage limit with the
    command `update-lossage-size'.  For convenience, add
    a function `lossage-size' to return the current limit.
    
    * src/keyboard.c (lossage_limit): Do not expose it
    to Lisp; make it a static variable.
    Keep the current Emacs default (300); accept only values >= 100.
    (lossage-size): New function; it returns the current size
    of recent_keys.
    (update-lossage-size):
    Rename it from update-lossage-limit (all callers updated);
    make it a command.
    
    * doc/emacs/help.texi (Misc Help)
    * etc/NEWS
    * lisp/cus-start.el
    * lisp/edmacro.el
    * lisp/help.el:
    Update all references with the new name.
    
    * test/src/keyboard-tests.el (keyboard-lossage-limit):
    Amend the test.

diff --git a/doc/emacs/help.texi b/doc/emacs/help.texi
index e2c0dc6802..26cdf161d5 100644
--- a/doc/emacs/help.texi
+++ b/doc/emacs/help.texi
@@ -563,12 +563,14 @@ Misc Help
 
 @kindex C-h l
 @findex view-lossage
-@vindex lossage-limit
+@findex update-lossage-size
+@findex lossage-size
   If something surprising happens, and you are not sure what you typed,
 use @kbd{C-h l} (@code{view-lossage}).  @kbd{C-h l} displays your last
 input keystrokes and the commands they invoked.  By default, Emacs
-stores the last 300 events; if you wish, you can change this number
-with the option @code{lossage-limit}.
+stores the last 300 events; if you wish, you can change this number with
+the command @code{update-lossage-size}.  The function @code{lossage-size}
+returns the current value for the lossage limit.
 If you see commands that you are not familiar with, you can use @kbd{C-h k} or
 @kbd{C-h f} to find out what they do.
 
diff --git a/etc/NEWS b/etc/NEWS
index da204c5825..5fc4cb4d87 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -71,8 +71,9 @@ useful on systems such as FreeBSD which ships only with "etc/termcap".
 * Changes in Emacs 28.1
 
 +++
-** The new option 'lossage-limit' controls the maximum number
-of keystrokes and commands recorded.
+** The new command 'update-lossage-size' allow users to set the maximum
+number of keystrokes and commands recorded.  The new function 'lossage-size'
+returns the current value of this limit
 
 ** Support for '(box . SIZE)' 'cursor-type'.
 By default, 'box' cursor always has a filled box shape.  But if you
diff --git a/lisp/cus-start.el b/lisp/cus-start.el
index 1ebf554b2b..6632687da4 100644
--- a/lisp/cus-start.el
+++ b/lisp/cus-start.el
@@ -352,8 +352,6 @@ minibuffer-prompt-properties--setter
 	     ;; indent.c
 	     (indent-tabs-mode indent boolean)
 	     ;; keyboard.c
-             (lossage-limit keyboard integer "28.1"
-                            :set (lambda (_ val) (update-lossage-limit val)))
 	     (meta-prefix-char keyboard character)
 	     (auto-save-interval auto-save integer)
              (auto-save-no-message auto-save boolean "27.1")
diff --git a/lisp/edmacro.el b/lisp/edmacro.el
index 089fea17ef..52cbf5b5a6 100644
--- a/lisp/edmacro.el
+++ b/lisp/edmacro.el
@@ -35,7 +35,7 @@
 ;;  * `M-x' followed by a command name, to edit a named command
 ;;    whose definition is a keyboard macro.
 ;;
-;;  * `C-h l' (view-lossage), to edit the `lossage-limit' most recent
+;;  * `C-h l' (view-lossage), to edit the 300 most recent
 ;;    keystrokes and install them as the "current" macro.
 ;;
 ;;  * any key sequence whose definition is a keyboard macro.
@@ -88,7 +88,7 @@ edit-kbd-macro
   "Edit a keyboard macro.
 At the prompt, type any key sequence which is bound to a keyboard macro.
 Or, type `\\[kmacro-end-and-call-macro]' or RET to edit the last
-keyboard macro, `\\[view-lossage]' to edit the last `lossage-limit'
+keyboard macro, `\\[view-lossage]' to edit the last 300
 keystrokes as a keyboard macro, or `\\[execute-extended-command]'
 to edit a macro by its command name.
 With a prefix argument, format the macro in a more concise way."
diff --git a/lisp/help.el b/lisp/help.el
index 9e3d0922d0..01ee739a16 100644
--- a/lisp/help.el
+++ b/lisp/help.el
@@ -459,7 +459,7 @@ view-lossage
   "Display last input keystrokes and the commands run.
 For convenience this uses the same format as
 `edit-last-kbd-macro'.
-See `lossage-limit' to update the number of recorded keystrokes.
+See `update-lossage-size' to update the number of recorded keystrokes.
 
 To record all your input, use `open-dribble-file'."
   (interactive)
diff --git a/src/keyboard.c b/src/keyboard.c
index a4d27c94fb..29b157700c 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -103,6 +103,7 @@ static KBOARD *all_kboards;
 /* True in the single-kboard state, false in the any-kboard state.  */
 static bool single_kboard;
 
+/* Minimum allowed size of the recent_keys vector */
 #define MIN_NUM_RECENT_KEYS (100)
 
 /* Index for storing next element into recent_keys.  */
@@ -111,6 +112,9 @@ static int recent_keys_index;
 /* Total number of elements stored into recent_keys.  */
 static int total_keys;
 
+/* Size of the recent_keys vector */
+static int lossage_limit = 3 * MIN_NUM_RECENT_KEYS;
+
 /* This vector holds the last lossage_limit keystrokes.  */
 static Lisp_Object recent_keys;
 
@@ -10411,6 +10415,15 @@ If CHECK-TIMERS is non-nil, timers that are ready to run will do so.  */)
 	  ? Qt : Qnil);
 }
 
+DEFUN ("lossage-size", Flossage_size, Slossage_size, 0, 0, 0,
+       doc: /* Return the maximum number of saved keystrokes.
+These are the records shown by `view-lossage'. */ )
+  (void)
+{
+  return make_fixnum(lossage_limit);
+}
+
+/* Reallocate recent_keys copying the keystrokes in the right order */
 static void
 update_recent_keys (int new_size, int kept_keys)
 {
@@ -10433,10 +10446,10 @@ update_recent_keys (int new_size, int kept_keys)
 
 }
 
-/* Reallocate recent_keys copying the keystrokes in the right order */
-DEFUN ("update-lossage-limit", Fupdate_lossage_limit,
-       Supdate_lossage_limit, 1, 1, 0,
+DEFUN ("update-lossage-size", Fupdate_lossage_size,
+       Supdate_lossage_size, 1, 1, "nnew-size: ",
        doc: /* Update the maximum number of saved keystrokes to ARG.
+These are the records shown by `view-lossage'.
 usage: (update-lossage-limit ARG)  */)
   (Lisp_Object arg)
 {
@@ -11736,22 +11749,6 @@ syms_of_keyboard (void)
     staticpro (&modifier_symbols);
   }
 
-  DEFVAR_INT ("lossage-limit", lossage_limit,
-              doc: /* Maximum number of stored keyboard events and commands run.
-
-Please, do not set this variable in Lisp with `setq' neither
-let-bind it, that will likely crash Emacs.  This is because
-`setq' only changes the variable, but it doesn't update
-the size of the internal vector that holds the keystrokes.
-
-To update this variable use the customization menu, or
-call from Lisp the following expression:
-
-  (update-lossage-limit new-limit)
-
-That takes care of both, the variable and the internal vector.*/);
-  lossage_limit = 3 * MIN_NUM_RECENT_KEYS;
-
   recent_keys = make_nil_vector (lossage_limit);
   staticpro (&recent_keys);
 
@@ -11802,7 +11799,8 @@ That takes care of both, the variable and the internal vector.*/);
   defsubr (&Srecursive_edit);
   defsubr (&Sinternal_track_mouse);
   defsubr (&Sinput_pending_p);
-  defsubr (&Supdate_lossage_limit);
+  defsubr (&Slossage_size);
+  defsubr (&Supdate_lossage_size);
   defsubr (&Srecent_keys);
   defsubr (&Sthis_command_keys);
   defsubr (&Sthis_command_keys_vector);
diff --git a/test/src/keyboard-tests.el b/test/src/keyboard-tests.el
index 017d239246..8f2cfe4adc 100644
--- a/test/src/keyboard-tests.el
+++ b/test/src/keyboard-tests.el
@@ -32,15 +32,15 @@
                         (read-event nil nil 2))
                  ?\C-b)))
 
-(ert-deftest keyboard-lossage-limit ()
-  "Test `lossage-limit' updates."
-  (dolist (val (list 100 100 200 500 300 1000 700))
-    (update-lossage-limit val)
-    (should (= val lossage-limit)))
-  (let ((current-limit lossage-limit))
-    (should-error (update-lossage-limit 5))
-    (should-error (update-lossage-limit "200"))
-    (should (= lossage-limit current-limit))))
+(ert-deftest keyboard-lossage-size ()
+  "Test `update-lossage-size'."
+  (dolist (val (list 100 300 400 400 500 1000 700 300))
+    (update-lossage-size val)
+    (should (= val (lossage-size))))
+  (let ((current-size (lossage-size)))
+    (should-error (update-lossage-size 5))
+    (should-error (update-lossage-size "200"))
+    (should (= (lossage-size) current-size))))
 
 
 (provide 'keyboard-tests)

--8<-----------------------------cut here---------------end--------------->8---
In GNU Emacs 28.0.50 (build 25, x86_64-pc-linux-gnu, GTK+ Version 3.24.5, cairo version 1.16.0)
 of 2020-06-28 built on calancha-pc.dy.bbexcite.jp
Repository revision: e13fd1fc373b35f510b70998bfddbb7af5989912
Repository branch: bug#38796-lossage-limit
Windowing system distributor 'The X.Org Foundation', version 11.0.12004000
System Description: Debian GNU/Linux 10 (buster)

Message #34 received at 38796 <at> debbugs.gnu.org (full text, mbox):

From: Drew Adams <drew.adams <at> oracle.com>
To: Tino Calancha <tino.calancha <at> gmail.com>, Stefan Monnier
 <monnier <at> iro.umontreal.ca>
Cc: uyennhi.qm <at> gmail.com, 38796 <at> debbugs.gnu.org
Subject: RE: bug#38796: 26.3; `view-lossage': Use a variable for the lossage
 limit
Date: Mon, 29 Jun 2020 00:05:45 +0000 (UTC)

> > OP here.
> I was waiting for you.  Welcome!! :-)

Sure, but the rest of your reply is a reply to Stefan's
message, not mine.  And the `>' quoting suggests you're
quoting me in the rest of your message too, but you're
quoting him.

Message #39 received at 38796 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Tino Calancha <tino.calancha <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, Stefan Monnier <monnier <at> iro.umontreal.ca>,
 38796 <at> debbugs.gnu.org, uyennhi.qm <at> gmail.com
Subject: Re: bug#38796: 26.3; `view-lossage': Use a variable for the lossage
 limit
Date: Sat, 22 Aug 2020 23:24:02 +0200

Tino Calancha <tino.calancha <at> gmail.com> writes:

>> I agree that disabling should not necessarily be implemented by
>> "abusing" the max-lossage setting.
> Yeah, it was appealing to me at first look: the code was offering
> another feature for free!
> After your comments I rethink about it and become less excited: it
> always posible that we might be hit back in other parts of the code
> if they assume a 'sensible large enough' value.

I thought it sounded quite natural to use the same mechanism to switch
lossage logging off.  `(update-lossage-size 0)' seems like a natural
thing to do to have no lossage.

>     Users can still safely modify the lossage limit with the
>     command `update-lossage-size'.  For convenience, add
>     a function `lossage-size' to return the current limit.

Hm...  other functions use the convention of having a zero-parameter
function return the data, and having a parameter to set it.  So
(lossage-size 500) could set it and (lossage-size) could return the
current size?

Anyway, it doesn't look like you've merged the branch?  I think
everybody was in favour of adding this feature.

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

Message #42 received at 38796 <at> debbugs.gnu.org (full text, mbox):

From: Drew Adams <drew.adams <at> oracle.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>, Tino Calancha <tino.calancha <at> gmail.com>
Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 38796 <at> debbugs.gnu.org,
 uyennhi.qm <at> gmail.com
Subject: RE: bug#38796: 26.3; `view-lossage': Use a variable for the lossage
 limit
Date: Sat, 22 Aug 2020 15:54:09 -0700 (PDT)

> I thought it sounded quite natural to use the same mechanism to switch
> lossage logging off.  `(update-lossage-size 0)' seems like a natural
> thing to do to have no lossage.

FWIW, I said the same thing:

https://debbugs.gnu.org/cgi/bugreport.cgi?bug=38796#28

It's not unusual or weird for a zero-valued size
parameter to have such an effect. 

> >     Users can still safely modify the lossage limit with the
> >     command `update-lossage-size'.  For convenience, add
> >     a function `lossage-size' to return the current limit.
> 
> Hm...  other functions use the convention of having a zero-parameter
> function return the data, and having a parameter to set it.  So
> (lossage-size 500) could set it and (lossage-size) could return the
> current size?

FWIW, most `set-*' functions are commands.
And a command is what we should shoot for here.

Message #45 received at 38796 <at> debbugs.gnu.org (full text, mbox):

From: Tino Calancha <tino.calancha <at> gmail.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: Eli Zaretskii <eliz <at> gnu.org>, stefankangas <at> gmail.com,
 Stefan Monnier <monnier <at> iro.umontreal.ca>, 38796 <at> debbugs.gnu.org,
 uyennhi.qm <at> gmail.com
Subject: Re: bug#38796: 26.3; `view-lossage': Use a variable for the lossage
 limit
Date: Thu, 27 Aug 2020 23:28:03 +0200

Lars Ingebrigtsen <larsi <at> gnus.org> writes:

> Tino Calancha <tino.calancha <at> gmail.com> writes:
>
>>> I agree that disabling should not necessarily be implemented by
>>> "abusing" the max-lossage setting.
>> Yeah, it was appealing to me at first look: the code was offering
>> another feature for free!
>> After your comments I rethink about it and become less excited: it
>> always posible that we might be hit back in other parts of the code
>> if they assume a 'sensible large enough' value.
>
> I thought it sounded quite natural to use the same mechanism to switch
> lossage logging off.  `(update-lossage-size 0)' seems like a natural
> thing to do to have no lossage.
> Anyway, it doesn't look like you've merged the branch?  I think
> everybody was in favour of adding this feature.
People agree with that.  The friction arises because I kept the current
implementation, which can be seen as abusing on it, and/or risky.
[ I have neither plan nor time to redo the current implementation. ]
>>     Users can still safely modify the lossage limit with the
>>     command `update-lossage-size'.  For convenience, add
>>     a function `lossage-size' to return the current limit.
>
> Hm...  other functions use the convention of having a zero-parameter
> function return the data, and having a parameter to set it.  So
> (lossage-size 500) could set it and (lossage-size) could return the
> current size?
I have amended the patch to do this.  Thank you.

FWIW I have been running Emacs almost 3 months using this branch
with no issues.
If Eli is OK with it, I can merge it to master next week.

Message #48 received at 38796 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Tino Calancha <tino.calancha <at> gmail.com>
Cc: larsi <at> gnus.org, stefankangas <at> gmail.com, monnier <at> iro.umontreal.ca,
 38796 <at> debbugs.gnu.org, uyennhi.qm <at> gmail.com
Subject: Re: bug#38796: 26.3; `view-lossage': Use a variable for the lossage
 limit
Date: Fri, 28 Aug 2020 09:04:06 +0300

> From: Tino Calancha <tino.calancha <at> gmail.com>
> Cc: Eli Zaretskii <eliz <at> gnu.org>,  Stefan Monnier
>  <monnier <at> iro.umontreal.ca>,  38796 <at> debbugs.gnu.org,  uyennhi.qm <at> gmail.com,
>  stefankangas <at> gmail.com
> Date: Thu, 27 Aug 2020 23:28:03 +0200
> 
> People agree with that.  The friction arises because I kept the current
> implementation, which can be seen as abusing on it, and/or risky.
> [ I have neither plan nor time to redo the current implementation. ]
> >>     Users can still safely modify the lossage limit with the
> >>     command `update-lossage-size'.  For convenience, add
> >>     a function `lossage-size' to return the current limit.
> >
> > Hm...  other functions use the convention of having a zero-parameter
> > function return the data, and having a parameter to set it.  So
> > (lossage-size 500) could set it and (lossage-size) could return the
> > current size?
> I have amended the patch to do this.  Thank you.
> 
> FWIW I have been running Emacs almost 3 months using this branch
> with no issues.
> If Eli is OK with it, I can merge it to master next week.

Sorry, OK with what? where's the patch which I should agree with?

Message #51 received at 38796 <at> debbugs.gnu.org (full text, mbox):

From: Tino Calancha <tino.calancha <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: uyennhi.qm <at> gmail.com, larsi <at> gnus.org, stefankangas <at> gmail.com,
 38796 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
Subject: Re: bug#38796: 26.3; `view-lossage': Use a variable for the lossage
 limit
Date: Fri, 04 Sep 2020 11:31:33 +0200

Eli Zaretskii <eliz <at> gnu.org> writes:

>> FWIW I have been running Emacs almost 3 months using this branch
>> with no issues.
>> If Eli is OK with it, I can merge it to master next week.
>
> Sorry, OK with what? where's the patch which I should agree with?

Branch origin/bug#38796-lossage-limit

The following is the difference with master branch when I updated
the branch (last August 27th):
--8<-----------------------------cut here---------------start------------->8---
diff --git a/doc/emacs/help.texi b/doc/emacs/help.texi
index 06ad5a583d..34a354891b 100644
--- a/doc/emacs/help.texi
+++ b/doc/emacs/help.texi
@@ -573,10 +573,13 @@ Misc Help
 
 @kindex C-h l
 @findex view-lossage
+@findex lossage-size
   If something surprising happens, and you are not sure what you typed,
 use @kbd{C-h l} (@code{view-lossage}).  @kbd{C-h l} displays your last
-300 input keystrokes and the commands they invoked.  If you see
-commands that you are not familiar with, you can use @kbd{C-h k} or
+input keystrokes and the commands they invoked.  By default, Emacs
+stores the last 300 events; if you wish, you can change this number with
+the command @code{lossage-size}.
+If you see commands that you are not familiar with, you can use @kbd{C-h k} or
 @kbd{C-h f} to find out what they do.
 
 @kindex C-h e
diff --git a/etc/NEWS b/etc/NEWS
index 964b626d7b..8bb3527d85 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -85,6 +85,10 @@ useful on systems such as FreeBSD which ships only with "etc/termcap".
 
 * Changes in Emacs 28.1
 
++++
+** The new command 'lossage-limit' controls the maximum number
+of keystrokes and commands recorded.
+
 +++
 ** New variables that hold default buffer names for shell output.
 The new constants 'shell-command-buffer-name' and
@@ -92,6 +96,9 @@ The new constants 'shell-command-buffer-name' and
 for the output of, respectively, synchronous and async shell
 commands.
 
+** The new command lossage-size' allow users to set the maximum
+number of keystrokes and commands recorded.
+
 ** Support for '(box . SIZE)' 'cursor-type'.
 By default, 'box' cursor always has a filled box shape.  But if you
 specify 'cursor-type' to be '(box . SIZE)', the cursor becomes a hollow
diff --git a/lisp/comint.el b/lisp/comint.el
index be0e32b9e0..afc8395b36 100644
--- a/lisp/comint.el
+++ b/lisp/comint.el
@@ -2393,7 +2393,7 @@ comint-send-invisible
 Then send it to the process running in the current buffer.
 The string is sent using `comint-input-sender'.
 Security bug: your string can still be temporarily recovered with
-\\[view-lossage]; `clear-this-command-keys' can fix that."
+\\[view-lossage]; `clear-this-command-keys' and `lossage-size' can fix that."
   (interactive "P")			; Defeat snooping via C-x ESC ESC
   (let ((proc (get-buffer-process (current-buffer)))
 	(prefix
diff --git a/lisp/edmacro.el b/lisp/edmacro.el
index 71474c0289..52cbf5b5a6 100644
--- a/lisp/edmacro.el
+++ b/lisp/edmacro.el
@@ -35,8 +35,8 @@
 ;;  * `M-x' followed by a command name, to edit a named command
 ;;    whose definition is a keyboard macro.
 ;;
-;;  * `C-h l' (view-lossage), to edit the 300 most recent keystrokes
-;;    and install them as the "current" macro.
+;;  * `C-h l' (view-lossage), to edit the 300 most recent
+;;    keystrokes and install them as the "current" macro.
 ;;
 ;;  * any key sequence whose definition is a keyboard macro.
 ;;
diff --git a/lisp/help.el b/lisp/help.el
index 1b0149616f..d3b0e02f08 100644
--- a/lisp/help.el
+++ b/lisp/help.el
@@ -455,9 +455,10 @@ view-external-packages
   (info "(efaq)Packages that do not come with Emacs"))
 
 (defun view-lossage ()
-  "Display last few input keystrokes and the commands run.
+  "Display last input keystrokes and the commands run.
 For convenience this uses the same format as
 `edit-last-kbd-macro'.
+See `lossage-size' to update the number of recorded keystrokes.
 
 To record all your input, use `open-dribble-file'."
   (interactive)
diff --git a/lisp/term.el b/lisp/term.el
index e77c2c1331..e39d03e8cd 100644
--- a/lisp/term.el
+++ b/lisp/term.el
@@ -2236,7 +2236,8 @@ term-read-noecho
 
 Note that the keystrokes comprising the text can still be recovered
 \(temporarily) with \\[view-lossage].  This may be a security bug for some
-applications."
+applications.  See `clear-this-command-keys' and `lossage-size' for ways
+to fix that."
   (declare (obsolete read-passwd "27.1"))
   (let ((ans "")
 	(c 0)
diff --git a/src/keyboard.c b/src/keyboard.c
index 5fa58abce1..bf8d75b602 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -103,15 +103,16 @@ Copyright (C) 1985-1989, 1993-1997, 1999-2020 Free Software Foundation,
 /* True in the single-kboard state, false in the any-kboard state.  */
 static bool single_kboard;
 
-#define NUM_RECENT_KEYS (300)
-
 /* Index for storing next element into recent_keys.  */
 static int recent_keys_index;
 
 /* Total number of elements stored into recent_keys.  */
 static int total_keys;
 
-/* This vector holds the last NUM_RECENT_KEYS keystrokes.  */
+/* Size of the recent_keys vector */
+static int lossage_limit = 300;
+
+/* This vector holds the last lossage_limit keystrokes.  */
 static Lisp_Object recent_keys;
 
 /* Vector holding the key sequence that invoked the current command.
@@ -294,6 +295,7 @@ #define GROW_RAW_KEYBUF							\
 /* Pointer to next place to store character in kbd_buffer.  */
 static union buffered_input_event *kbd_store_ptr;
 
+
 /* The above pair of variables forms a "queue empty" flag.  When we
    enqueue a non-hook event, we increment kbd_store_ptr.  When we
    dequeue a non-hook event, we increment kbd_fetch_ptr.  We say that
@@ -1421,10 +1423,10 @@ command_loop_1 (void)
       /* Execute the command.  */
 
       {
-	total_keys += total_keys < NUM_RECENT_KEYS;
+	total_keys += total_keys < lossage_limit;
 	ASET (recent_keys, recent_keys_index,
 	      Fcons (Qnil, cmd));
-	if (++recent_keys_index >= NUM_RECENT_KEYS)
+	if (++recent_keys_index >= lossage_limit)
 	  recent_keys_index = 0;
       }
       Vthis_command = cmd;
@@ -3248,15 +3250,15 @@ record_char (Lisp_Object c)
       int ix1, ix2, ix3;
 
       if ((ix1 = recent_keys_index - 1) < 0)
-	ix1 = NUM_RECENT_KEYS - 1;
+	ix1 = lossage_limit - 1;
       ev1 = AREF (recent_keys, ix1);
 
       if ((ix2 = ix1 - 1) < 0)
-	ix2 = NUM_RECENT_KEYS - 1;
+	ix2 = lossage_limit - 1;
       ev2 = AREF (recent_keys, ix2);
 
       if ((ix3 = ix2 - 1) < 0)
-	ix3 = NUM_RECENT_KEYS - 1;
+	ix3 = lossage_limit - 1;
       ev3 = AREF (recent_keys, ix3);
 
       if (EQ (XCAR (c), Qhelp_echo))
@@ -3307,12 +3309,12 @@ record_char (Lisp_Object c)
     {
       if (!recorded)
 	{
-	  total_keys += total_keys < NUM_RECENT_KEYS;
+	  total_keys += total_keys < lossage_limit;
 	  ASET (recent_keys, recent_keys_index,
                 /* Copy the event, in case it gets modified by side-effect
                    by some remapping function (bug#30955).  */
                 CONSP (c) ? Fcopy_sequence (c) : c);
-	  if (++recent_keys_index >= NUM_RECENT_KEYS)
+	  if (++recent_keys_index >= lossage_limit)
 	    recent_keys_index = 0;
 	}
       else if (recorded < 0)
@@ -3326,10 +3328,10 @@ record_char (Lisp_Object c)
 
 	  while (recorded++ < 0 && total_keys > 0)
 	    {
-	      if (total_keys < NUM_RECENT_KEYS)
+	      if (total_keys < lossage_limit)
 		total_keys--;
 	      if (--recent_keys_index < 0)
-		recent_keys_index = NUM_RECENT_KEYS - 1;
+		recent_keys_index = lossage_limit - 1;
 	      ASET (recent_keys, recent_keys_index, Qnil);
 	    }
 	}
@@ -10410,6 +10412,62 @@ DEFUN ("input-pending-p", Finput_pending_p, Sinput_pending_p, 0, 1, 0,
 	  ? Qt : Qnil);
 }
 
+/* Reallocate recent_keys copying the keystrokes in the right order */
+static void
+update_recent_keys (int new_size, int kept_keys)
+{
+  int osize = ASIZE (recent_keys);
+  eassert (recent_keys_index < osize);
+  eassert (kept_keys <= min (osize, new_size));
+  Lisp_Object v = make_nil_vector (new_size);
+  int i, idx;
+  for (i = 0; i < kept_keys; ++i)
+    {
+      idx = recent_keys_index - kept_keys + i;
+      while (idx < 0)
+        idx += osize;
+      ASET (v, i, AREF (recent_keys, idx));
+    }
+  recent_keys = v;
+  total_keys = kept_keys;
+  recent_keys_index = total_keys % new_size;
+  lossage_limit = new_size;
+
+}
+
+DEFUN ("lossage-size", Flossage_size, Slossage_size, 0, 1,
+       "(list (read-number \"new-size: \" (lossage-size)))",
+       doc: /* Return the maximum number of saved keystrokes.
+Called with ARG, then set this number to ARG.
+
+The saved keystrokes are the records shown by `view-lossage'.
+If you want to disable the lossage records, then set this maximum to a
+small number, e.g. 0.
+usage: (lossage-size &optional ARG) */)
+  (Lisp_Object arg)
+{
+  if (NILP(arg))
+    return make_fixnum(lossage_limit == 1 ? 0 : lossage_limit);
+
+  if (!FIXNATP (arg))
+    user_error ("Value must be a non-negative integer");
+  int osize = ASIZE (recent_keys);
+  eassert (lossage_limit == osize);
+  int new_size = XFIXNAT (arg);
+
+  if (new_size == osize)
+    return Qnil;
+  /* Internally, the minimum lossage_limit is 1; users will likely use
+     0 to disable the lossage, thus here we change 0 -> 1.  */
+  if (new_size == 0)
+    new_size = 1;
+
+  int kept_keys = new_size > osize ? total_keys : min (new_size, total_keys);
+  update_recent_keys (new_size, kept_keys);
+
+  return Qnil;
+}
+
 DEFUN ("recent-keys", Frecent_keys, Srecent_keys, 0, 1, 0,
        doc: /* Return vector of last few events, not counting those from keyboard macros.
 If INCLUDE-CMDS is non-nil, include the commands that were run,
@@ -10419,21 +10477,21 @@ DEFUN ("recent-keys", Frecent_keys, Srecent_keys, 0, 1, 0,
   bool cmds = !NILP (include_cmds);
 
   if (!total_keys
-      || (cmds && total_keys < NUM_RECENT_KEYS))
+      || (cmds && total_keys < lossage_limit))
     return Fvector (total_keys,
 		    XVECTOR (recent_keys)->contents);
   else
     {
       Lisp_Object es = Qnil;
-      int i = (total_keys < NUM_RECENT_KEYS
+      int i = (total_keys < lossage_limit
 	       ? 0 : recent_keys_index);
-      eassert (recent_keys_index < NUM_RECENT_KEYS);
+      eassert (recent_keys_index < lossage_limit);
       do
 	{
 	  Lisp_Object e = AREF (recent_keys, i);
 	  if (cmds || !CONSP (e) || !NILP (XCAR (e)))
 	    es = Fcons (e, es);
-	  if (++i >= NUM_RECENT_KEYS)
+	  if (++i >= lossage_limit)
 	    i = 0;
 	} while (i != recent_keys_index);
       es = Fnreverse (es);
@@ -10531,7 +10589,7 @@ DEFUN ("this-single-command-raw-keys", Fthis_single_command_raw_keys,
 DEFUN ("clear-this-command-keys", Fclear_this_command_keys,
        Sclear_this_command_keys, 0, 1, 0,
        doc: /* Clear out the vector that `this-command-keys' returns.
-Also clear the record of the last 300 input events, unless optional arg
+Also clear the record of the last input events, unless optional arg
 KEEP-RECORD is non-nil.  */)
   (Lisp_Object keep_record)
 {
@@ -11686,7 +11744,7 @@ syms_of_keyboard (void)
     staticpro (&modifier_symbols);
   }
 
-  recent_keys = make_nil_vector (NUM_RECENT_KEYS);
+  recent_keys = make_nil_vector (lossage_limit);
   staticpro (&recent_keys);
 
   this_command_keys = make_nil_vector (40);
@@ -11736,6 +11794,7 @@ syms_of_keyboard (void)
   defsubr (&Srecursive_edit);
   defsubr (&Sinternal_track_mouse);
   defsubr (&Sinput_pending_p);
+  defsubr (&Slossage_size);
   defsubr (&Srecent_keys);
   defsubr (&Sthis_command_keys);
   defsubr (&Sthis_command_keys_vector);
diff --git a/test/src/keyboard-tests.el b/test/src/keyboard-tests.el
index 1988ba51a7..d4ac4a518d 100644
--- a/test/src/keyboard-tests.el
+++ b/test/src/keyboard-tests.el
@@ -32,5 +32,23 @@ keyboard-unread-command-events
                         (read-event nil nil 2))
                  ?\C-b)))
 
+(ert-deftest keyboard-lossage-size ()
+  "Test `lossage-size'."
+  (dolist (val (list 100 300 400 400 500 1000 700 300))
+    (lossage-size val)
+    (should (= val (lossage-size))))
+  (let ((current-size (lossage-size)))
+    ;; Set lossage size to 1 and 0 are equivalent
+    (lossage-size 0)
+    (should (zerop (lossage-size)))
+    (lossage-size 1)
+    (should (zerop (lossage-size)))
+    (lossage-size current-size)
+    ;; Argument must be a natural number
+    (should-error (lossage-size -5))
+    (should-error (lossage-size "200"))
+    (should (= (lossage-size) current-size))))
+
+
 (provide 'keyboard-tests)
 ;;; keyboard-tests.el ends here

--8<-----------------------------cut here---------------end--------------->8---

Message #54 received at 38796 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Tino Calancha <tino.calancha <at> gmail.com>
Cc: uyennhi.qm <at> gmail.com, larsi <at> gnus.org, stefankangas <at> gmail.com,
 38796 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
Subject: Re: bug#38796: 26.3; `view-lossage': Use a variable for the lossage
 limit
Date: Fri, 04 Sep 2020 15:07:50 +0300

> From: Tino Calancha <tino.calancha <at> gmail.com>
> Cc: larsi <at> gnus.org,  stefankangas <at> gmail.com,  monnier <at> iro.umontreal.ca,
>   38796 <at> debbugs.gnu.org,  uyennhi.qm <at> gmail.com
> Date: Fri, 04 Sep 2020 11:31:33 +0200
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> >> FWIW I have been running Emacs almost 3 months using this branch
> >> with no issues.
> >> If Eli is OK with it, I can merge it to master next week.
> >
> > Sorry, OK with what? where's the patch which I should agree with?
> 
> Branch origin/bug#38796-lossage-limit
> 
> The following is the difference with master branch when I updated
> the branch (last August 27th):

Not sure what that means.  Is this the patch you suggest installing,
or does it need more work to adapt it to the current master?

>    If something surprising happens, and you are not sure what you typed,
>  use @kbd{C-h l} (@code{view-lossage}).  @kbd{C-h l} displays your last
> -300 input keystrokes and the commands they invoked.  If you see
> -commands that you are not familiar with, you can use @kbd{C-h k} or
> +input keystrokes and the commands they invoked.  By default, Emacs
> +stores the last 300 events; if you wish, you can change this number with
              ^^^^^^^^^^^^^^^
The first sentence talks about keystrokes, but the last sentence talks
about "events".  The reader might become confused whether these two
terms refer to the same entity.

> ++++
> +** The new command 'lossage-limit' controls the maximum number
> +of keystrokes and commands recorded.
> +
>  +++
>  ** New variables that hold default buffer names for shell output.
>  The new constants 'shell-command-buffer-name' and
> @@ -92,6 +96,9 @@ The new constants 'shell-command-buffer-name' and
>  for the output of, respectively, synchronous and async shell
>  commands.
>  
> +** The new command lossage-size' allow users to set the maximum

Missing opening quote for lossage-size.

> +number of keystrokes and commands recorded.

NEWS mentions 2 separate commands, but I see only one in the
implementation.

> --- a/lisp/edmacro.el
> +++ b/lisp/edmacro.el
> @@ -35,8 +35,8 @@
>  ;;  * `M-x' followed by a command name, to edit a named command
>  ;;    whose definition is a keyboard macro.
>  ;;
> -;;  * `C-h l' (view-lossage), to edit the 300 most recent keystrokes
> -;;    and install them as the "current" macro.
> +;;  * `C-h l' (view-lossage), to edit the 300 most recent
> +;;    keystrokes and install them as the "current" macro.

This change is a no-op; why is it needed?

>  (defun view-lossage ()
> -  "Display last few input keystrokes and the commands run.
> +  "Display last input keystrokes and the commands run.

Why this change?

> -/* This vector holds the last NUM_RECENT_KEYS keystrokes.  */
> +/* Size of the recent_keys vector */
                                    ^^^
A comment should end in a period and 2 spaces before "*/".

>  /* Pointer to next place to store character in kbd_buffer.  */
>  static union buffered_input_event *kbd_store_ptr;
>  
> +

Do we really need to add an empty line here?

> +DEFUN ("lossage-size", Flossage_size, Slossage_size, 0, 1,
> +       "(list (read-number \"new-size: \" (lossage-size)))",
> +       doc: /* Return the maximum number of saved keystrokes.

The first line describes only one of the two functionalities of this
command; it should describe both.

> +Called with ARG, then set this number to ARG.

"ARG non-nil means set the maximum number of keystrokes to that number."

> +The saved keystrokes are the records shown by `view-lossage'.
> +If you want to disable the lossage records, then set this maximum to a
> +small number, e.g. 0.

The "small number, e.g." part is inaccurate: it _must_ be zero, right?

> +usage: (lossage-size &optional ARG) */)

Is this "usage" needed? what happens if you don't use it?

> +  if (NILP(arg))
> +    return make_fixnum(lossage_limit == 1 ? 0 : lossage_limit);
                         ^
Space here.

So if the user sets the limit to 1, the next call to lossage-size will
return zero?  Isn't that confusing?

> +  /* Internally, the minimum lossage_limit is 1; users will likely use
> +     0 to disable the lossage, thus here we change 0 -> 1.  */
> +  if (new_size == 0)
> +    new_size = 1;

I still don't like this.  I think it will cause confusion and errors.

> +  return Qnil;

Why return nil when setting the limit? why not the previous limit?

> +(ert-deftest keyboard-lossage-size ()
> +  "Test `lossage-size'."
> +  (dolist (val (list 100 300 400 400 500 1000 700 300))
> +    (lossage-size val)
> +    (should (= val (lossage-size))))

This doesn't test the actual recording of VAL events.

Thanks.

Message #57 received at 38796 <at> debbugs.gnu.org (full text, mbox):

From: Tino Calancha <tino.calancha <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: larsi <at> gnus.org, monnier <at> iro.umontreal.ca, uyennhi.qm <at> gmail.com,
 38796 <at> debbugs.gnu.org, stefankangas <at> gmail.com
Subject: Re: bug#38796: 26.3; `view-lossage': Use a variable for the lossage
 limit
Date: Sat, 12 Sep 2020 14:29:45 +0200

Eli Zaretskii <eliz <at> gnu.org> writes:

Thank you for the review Eli.
I have fixed all your comments.

Regarding the feature to disable the recording of keystrokes, I am not
happy with my hackish implementation either: I have dropped it.

I prefer just to keep my patch within the scope of the original proposal:
i.e., be able to set the lossage limit with Lisp (within well tested limits).

I am glad to push the following patch next week if there are no
complaints.

--8<-----------------------------cut here---------------start------------->8---
commit bee193a5d6cd7889183cf811eb001e0071052724
Author: Tino Calancha <ccalancha <at> suse.com>
Date:   Sat Sep 12 14:14:45 2020 +0200

    Give Lisp control on the lossage size
    
    Add a command 'lossage-size' to set the maximum
    number or recorded keystrokes (Bug#38796).
    
    * src/keyboard.c (lossage_limit):
    Static variable with the current lossage size limit.
    (MIN_NUM_RECENT_KEYS): Renamed from NUM_RECENT_KEYS.
    Set it as 100 and use it as the minimum value for lossage_limit.
    Keep the same default for the vector size as before (300).
    (lossage-size): New command.
    (update_recent_keys): Helper function.
    (command_loop_1)
    (record_char)
    (recent-keys)
    (syms_of_keyboard): Use lossage_limit as the vector size.
    
    * lisp/help.el (view-lossage): Mention the new command in the docstring.
    * etc/NEWS (Changes in Emacs 28.1): Announce this change.
    * doc/emacs/help.texi (Misc Help): Update manual.
    * test/src/keyboard-tests.el (keyboard-lossage-size): Add test.

diff --git a/doc/emacs/help.texi b/doc/emacs/help.texi
index 06ad5a583d..232b611f41 100644
--- a/doc/emacs/help.texi
+++ b/doc/emacs/help.texi
@@ -573,10 +573,13 @@ Misc Help
 
 @kindex C-h l
 @findex view-lossage
+@findex lossage-size
   If something surprising happens, and you are not sure what you typed,
 use @kbd{C-h l} (@code{view-lossage}).  @kbd{C-h l} displays your last
-300 input keystrokes and the commands they invoked.  If you see
-commands that you are not familiar with, you can use @kbd{C-h k} or
+input keystrokes and the commands they invoked.  By default, Emacs
+stores the last 300 keystrokes; if you wish, you can change this number with
+the command @code{lossage-size}.
+If you see commands that you are not familiar with, you can use @kbd{C-h k} or
 @kbd{C-h f} to find out what they do.
 
 @kindex C-h e
diff --git a/etc/NEWS b/etc/NEWS
index 8ff62b6dc0..6fd23422cf 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -85,6 +85,10 @@ useful on systems such as FreeBSD which ships only with "etc/termcap".
 
 * Changes in Emacs 28.1
 
++++
+** The new command 'lossage-size' allow users to set the maximum
+number of keystrokes and commands recorded.
+
 +++
 *** Emacs now defaults to UTF-8 instead of ISO-8859-1.
 This is only for the default, where the user has set no LANG (or
diff --git a/lisp/help.el b/lisp/help.el
index 729684af6b..edef78d207 100644
--- a/lisp/help.el
+++ b/lisp/help.el
@@ -458,6 +458,7 @@ view-lossage
   "Display last few input keystrokes and the commands run.
 For convenience this uses the same format as
 `edit-last-kbd-macro'.
+See `lossage-size' to update the number of recorded keystrokes.
 
 To record all your input, use `open-dribble-file'."
   (interactive)
diff --git a/src/keyboard.c b/src/keyboard.c
index 590d183c4c..c0a41e6c4c 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -103,7 +103,8 @@ Copyright (C) 1985-1989, 1993-1997, 1999-2020 Free Software Foundation,
 /* True in the single-kboard state, false in the any-kboard state.  */
 static bool single_kboard;
 
-#define NUM_RECENT_KEYS (300)
+/* Minimum allowed size of the recent_keys vector.  */
+#define MIN_NUM_RECENT_KEYS (100)
 
 /* Index for storing next element into recent_keys.  */
 static int recent_keys_index;
@@ -111,7 +112,10 @@ #define NUM_RECENT_KEYS (300)
 /* Total number of elements stored into recent_keys.  */
 static int total_keys;
 
-/* This vector holds the last NUM_RECENT_KEYS keystrokes.  */
+/* Size of the recent_keys vector.  */
+static int lossage_limit = 3 * MIN_NUM_RECENT_KEYS;
+
+/* This vector holds the last lossage_limit keystrokes.  */
 static Lisp_Object recent_keys;
 
 /* Vector holding the key sequence that invoked the current command.
@@ -1421,10 +1425,10 @@ command_loop_1 (void)
       /* Execute the command.  */
 
       {
-	total_keys += total_keys < NUM_RECENT_KEYS;
+	total_keys += total_keys < lossage_limit;
 	ASET (recent_keys, recent_keys_index,
 	      Fcons (Qnil, cmd));
-	if (++recent_keys_index >= NUM_RECENT_KEYS)
+	if (++recent_keys_index >= lossage_limit)
 	  recent_keys_index = 0;
       }
       Vthis_command = cmd;
@@ -3248,15 +3252,15 @@ record_char (Lisp_Object c)
       int ix1, ix2, ix3;
 
       if ((ix1 = recent_keys_index - 1) < 0)
-	ix1 = NUM_RECENT_KEYS - 1;
+	ix1 = lossage_limit - 1;
       ev1 = AREF (recent_keys, ix1);
 
       if ((ix2 = ix1 - 1) < 0)
-	ix2 = NUM_RECENT_KEYS - 1;
+	ix2 = lossage_limit - 1;
       ev2 = AREF (recent_keys, ix2);
 
       if ((ix3 = ix2 - 1) < 0)
-	ix3 = NUM_RECENT_KEYS - 1;
+	ix3 = lossage_limit - 1;
       ev3 = AREF (recent_keys, ix3);
 
       if (EQ (XCAR (c), Qhelp_echo))
@@ -3307,12 +3311,12 @@ record_char (Lisp_Object c)
     {
       if (!recorded)
 	{
-	  total_keys += total_keys < NUM_RECENT_KEYS;
+	  total_keys += total_keys < lossage_limit;
 	  ASET (recent_keys, recent_keys_index,
                 /* Copy the event, in case it gets modified by side-effect
                    by some remapping function (bug#30955).  */
                 CONSP (c) ? Fcopy_sequence (c) : c);
-	  if (++recent_keys_index >= NUM_RECENT_KEYS)
+	  if (++recent_keys_index >= lossage_limit)
 	    recent_keys_index = 0;
 	}
       else if (recorded < 0)
@@ -3326,10 +3330,10 @@ record_char (Lisp_Object c)
 
 	  while (recorded++ < 0 && total_keys > 0)
 	    {
-	      if (total_keys < NUM_RECENT_KEYS)
+	      if (total_keys < lossage_limit)
 		total_keys--;
 	      if (--recent_keys_index < 0)
-		recent_keys_index = NUM_RECENT_KEYS - 1;
+		recent_keys_index = lossage_limit - 1;
 	      ASET (recent_keys, recent_keys_index, Qnil);
 	    }
 	}
@@ -10410,6 +10414,62 @@ DEFUN ("input-pending-p", Finput_pending_p, Sinput_pending_p, 0, 1, 0,
 	  ? Qt : Qnil);
 }
 
+/* Reallocate recent_keys copying the keystrokes in the right order */
+static void
+update_recent_keys (int new_size, int kept_keys)
+{
+  int osize = ASIZE (recent_keys);
+  eassert (recent_keys_index < osize);
+  eassert (kept_keys <= min (osize, new_size));
+  Lisp_Object v = make_nil_vector (new_size);
+  int i, idx;
+  for (i = 0; i < kept_keys; ++i)
+    {
+      idx = recent_keys_index - kept_keys + i;
+      while (idx < 0)
+        idx += osize;
+      ASET (v, i, AREF (recent_keys, idx));
+    }
+  recent_keys = v;
+  total_keys = kept_keys;
+  recent_keys_index = total_keys % new_size;
+  lossage_limit = new_size;
+
+}
+
+DEFUN ("lossage-size", Flossage_size, Slossage_size, 0, 1,
+       "(list (read-number \"new-size: \" (lossage-size)))",
+       doc: /* Return the maximum number of saved keystrokes.
+Called with ARG, then set this limit to ARG and return it.
+
+The saved keystrokes are the records shown by `view-lossage'.  */)
+  (Lisp_Object arg)
+{
+  if (NILP(arg))
+    return make_fixnum (lossage_limit);
+
+  if (!FIXNATP (arg))
+    user_error ("Value must be a positive integer");
+  int osize = ASIZE (recent_keys);
+  eassert (lossage_limit == osize);
+  int min_size = MIN_NUM_RECENT_KEYS;
+  int new_size = XFIXNAT (arg);
+
+  if (new_size == osize)
+    return make_fixnum (lossage_limit);
+
+  if (new_size < min_size)
+    {
+      AUTO_STRING (fmt, "Value must be >= %d");
+      Fsignal (Quser_error, list1 (CALLN (Fformat, fmt, make_fixnum (min_size))));
+    }
+
+  int kept_keys = new_size > osize ? total_keys : min (new_size, total_keys);
+  update_recent_keys (new_size, kept_keys);
+
+  return make_fixnum (lossage_limit);
+}
+
 DEFUN ("recent-keys", Frecent_keys, Srecent_keys, 0, 1, 0,
        doc: /* Return vector of last few events, not counting those from keyboard macros.
 If INCLUDE-CMDS is non-nil, include the commands that were run,
@@ -10419,21 +10479,21 @@ DEFUN ("recent-keys", Frecent_keys, Srecent_keys, 0, 1, 0,
   bool cmds = !NILP (include_cmds);
 
   if (!total_keys
-      || (cmds && total_keys < NUM_RECENT_KEYS))
+      || (cmds && total_keys < lossage_limit))
     return Fvector (total_keys,
 		    XVECTOR (recent_keys)->contents);
   else
     {
       Lisp_Object es = Qnil;
-      int i = (total_keys < NUM_RECENT_KEYS
+      int i = (total_keys < lossage_limit
 	       ? 0 : recent_keys_index);
-      eassert (recent_keys_index < NUM_RECENT_KEYS);
+      eassert (recent_keys_index < lossage_limit);
       do
 	{
 	  Lisp_Object e = AREF (recent_keys, i);
 	  if (cmds || !CONSP (e) || !NILP (XCAR (e)))
 	    es = Fcons (e, es);
-	  if (++i >= NUM_RECENT_KEYS)
+	  if (++i >= lossage_limit)
 	    i = 0;
 	} while (i != recent_keys_index);
       es = Fnreverse (es);
@@ -11686,7 +11746,7 @@ syms_of_keyboard (void)
     staticpro (&modifier_symbols);
   }
 
-  recent_keys = make_nil_vector (NUM_RECENT_KEYS);
+  recent_keys = make_nil_vector (lossage_limit);
   staticpro (&recent_keys);
 
   this_command_keys = make_nil_vector (40);
@@ -11736,6 +11796,7 @@ syms_of_keyboard (void)
   defsubr (&Srecursive_edit);
   defsubr (&Sinternal_track_mouse);
   defsubr (&Sinput_pending_p);
+  defsubr (&Slossage_size);
   defsubr (&Srecent_keys);
   defsubr (&Sthis_command_keys);
   defsubr (&Sthis_command_keys_vector);
diff --git a/test/src/keyboard-tests.el b/test/src/keyboard-tests.el
index 1988ba51a7..970a53555f 100644
--- a/test/src/keyboard-tests.el
+++ b/test/src/keyboard-tests.el
@@ -32,5 +32,20 @@ keyboard-unread-command-events
                         (read-event nil nil 2))
                  ?\C-b)))
 
+(ert-deftest keyboard-lossage-size ()
+  "Test `lossage-size'."
+  (let ((min-value 100)
+        (lossage-orig (lossage-size)))
+    (dolist (factor (list 1 3 4 5 10 7 3))
+      (let ((new-lossage (* factor min-value)))
+        (should (= new-lossage (lossage-size new-lossage)))))
+    ;; Wrong type
+    (should-error (lossage-size -5))
+    (should-error (lossage-size "200"))
+    ;; Less that minimum value
+    (should-error (lossage-size (1- min-value)))
+    (should (= lossage-orig (lossage-size lossage-orig)))))
+
+
 (provide 'keyboard-tests)
 ;;; keyboard-tests.el ends here

--8<-----------------------------cut here---------------end--------------->8---
In GNU Emacs 28.0.50 (build 6, x86_64-pc-linux-gnu, GTK+ Version 3.24.22, cairo version 1.16.0)
 of 2020-09-12 built on localhost.example.com
Repository revision: 4c3f3bf25623c936ca07249203147ae0332d64ed
Repository branch: master

Message #60 received at 38796 <at> debbugs.gnu.org (full text, mbox):

From: Tino Calancha <tino.calancha <at> gmail.com>
To: 38796-done <at> debbugs.gnu.org
Cc: larsi <at> gnus.org, stefankangas <at> gmail.com, monnier <at> iro.umontreal.ca,
 38796 <at> debbugs.gnu.org, uyennhi.qm <at> gmail.com
Subject: Re: bug#38796: 26.3; `view-lossage': Use a variable for the lossage
 limit
Date: Thu, 17 Sep 2020 16:35:22 +0200

Tino Calancha <tino.calancha <at> gmail.com> writes:

> I am glad to push the following patch next week if there are no
> complaints.

Pushed into master branch as commit
'Give Lisp control on the lossage size'
(23a3333b3ef0768f48f64f382ee899050b6103be)

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.