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

[Eli Zaretskii <eliz@gnu.org>]

> From: Tino Calancha <tino.calancha@gmail.com>
> Cc: 38796@debbugs.gnu.org, Eli Zaretskii <eliz@gnu.org>, Stefan Monnier
>  <monnier@iro.umontreal.ca>,uyennhi.qm@gmail.com, tino.calancha@gmail.com
> Date: Fri, 26 Jun 2020 23:58:01 +0200
> 
> 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. 

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.