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

[Tino Calancha <tino.calancha@gmail.com>]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Drew Adams <drew.adams@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@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".
 \f
 * 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@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)