bug#39291: M-: history doesn't store erroneous input

bug#39291: M-: history doesn't store erroneous input

[Paul Pogonyshev]

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

M-: (oops-i-forgot-to-type-the-closing-paren RET
=> End of file during parsing

Error itself is fine, but the problem is if you now
type M-: again, you won't find your input in the
history and have to retype everything from scratch
instead of just fixing the typo.

This is a regression in trunk compared at least to
26.3.

[-- Attachment #2: Type: text/html, Size: 454 bytes --]

bug#39291: M-: history doesn't store erroneous input

[Juri Linkov]

> M-: (oops-i-forgot-to-type-the-closing-paren RET
> => End of file during parsing
>
> Error itself is fine, but the problem is if you now
> type M-: again, you won't find your input in the
> history and have to retype everything from scratch
> instead of just fixing the typo.
>
> This is a regression in trunk compared at least to
> 26.3.

This was changed by
http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=9042ece787c
via https://debbugs.gnu.org/cgi/bugreport.cgi?bug=38317#23
and https://lists.gnu.org/archive/html/emacs-devel/2019-12/msg00241.html

bug#39291: M-: history doesn't store erroneous input

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Date: Wed, 29 Jan 2020 01:35:33 +0200
> Cc: 39291@debbugs.gnu.org
> 
> > M-: (oops-i-forgot-to-type-the-closing-paren RET
> > => End of file during parsing
> >
> > Error itself is fine, but the problem is if you now
> > type M-: again, you won't find your input in the
> > history and have to retype everything from scratch
> > instead of just fixing the typo.
> >
> > This is a regression in trunk compared at least to
> > 26.3.
> 
> This was changed by
> http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=9042ece787c
> via https://debbugs.gnu.org/cgi/bugreport.cgi?bug=38317#23
> and https://lists.gnu.org/archive/html/emacs-devel/2019-12/msg00241.html

Federico, please look into fixing this regression.

TIA

bug#39291: M-: history doesn't store erroneous input

[Federico Tedin]

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

On Wed, 29 Jan 2020 at 18:16 Eli Zaretskii <eliz@gnu.org> wrote:

> > From: Juri Linkov <juri@linkov.net>
> > Date: Wed, 29 Jan 2020 01:35:33 +0200
> > Cc: 39291@debbugs.gnu.org
> >
> > > M-: (oops-i-forgot-to-type-the-closing-paren RET
> > > => End of file during parsing
> > >
> > > Error itself is fine, but the problem is if you now
> > > type M-: again, you won't find your input in the
> > > history and have to retype everything from scratch
> > > instead of just fixing the typo.
> > >
> > > This is a regression in trunk compared at least to
> > > 26.3.
> >
> > This was changed by
> > http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=9042ece787c
> > via https://debbugs.gnu.org/cgi/bugreport.cgi?bug=38317#23
> > and https://lists.gnu.org/archive/html/emacs-devel/2019-12/msg00241.html
>
> Federico, please look into fixing this regression.
>
> TIA


Thanks for letting me know, I will take a look at it.

>

[-- Attachment #2: Type: text/html, Size: 1902 bytes --]

bug#39291: M-: history doesn't store erroneous input

[Federico Tedin]

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

> Federico, please look into fixing this regression.
>
> TIA

I found that the minibuffer input string in `read_minibuf' was being
added to the history list after we tried to parse an object from it (if
`expflag' was true). So if the parsing failed, then the value wasn't
added to the history. I'm attaching a patch with my changes.

My only doubt is that now, the call to `string_to_object' (which calls
`read-from-string', which calls `read1', and then `read0') is located
outside the context (not sure what the right term is) set up with
`specbind'. Could this be a problem? Do any of these functions depend on
the context set up in `read_minibuf'?  (They don't appear to, just want
to be sure).

Thanks.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch --]
[-- Type: text/x-diff, Size: 1310 bytes --]

From 6a35a1390d1f1924d0fd52dbb885af66d089273c Mon Sep 17 00:00:00 2001
From: Federico Tedin <federicotedin@gmail.com>
Date: Wed, 29 Jan 2020 22:24:40 +0100
Subject: [PATCH 1/1] Ensure input is added to history in read_minibuf

* src/minibuf.c (read_minibuf): Parse input string after saving the
string to the history list instead of before, in case parsing
fails. (Bug#39291)
---
 src/minibuf.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/minibuf.c b/src/minibuf.c
index 8ebdff1252..9d870ce364 100644
--- a/src/minibuf.c
+++ b/src/minibuf.c
@@ -697,10 +697,6 @@ read_minibuf (Lisp_Object map, Lisp_Object initial, Lisp_Object prompt,
   else
     histstring = Qnil;
 
-  /* If Lisp form desired instead of string, parse it.  */
-  if (expflag)
-    val = string_to_object (val, defalt);
-
   /* The appropriate frame will get selected
      in set-window-configuration.  */
   unbind_to (count, Qnil);
@@ -711,6 +707,10 @@ read_minibuf (Lisp_Object map, Lisp_Object initial, Lisp_Object prompt,
   if (! (NILP (Vhistory_add_new_input) || NILP (histstring)))
     call2 (intern ("add-to-history"), histvar, histstring);
 
+  /* If Lisp form desired instead of string, parse it.  */
+  if (expflag)
+    val = string_to_object (val, defalt);
+
   return val;
 }
 
-- 
2.17.1

bug#39291: M-: history doesn't store erroneous input

[Eli Zaretskii]

> From: Federico Tedin <federicotedin@gmail.com>
> Cc: Juri Linkov <juri@linkov.net>,  39291@debbugs.gnu.org,  pogonyshev@gmail.com
> Date: Wed, 29 Jan 2020 22:32:16 +0100
> 
> I found that the minibuffer input string in `read_minibuf' was being
> added to the history list after we tried to parse an object from it (if
> `expflag' was true). So if the parsing failed, then the value wasn't
> added to the history. I'm attaching a patch with my changes.

Thanks.

> My only doubt is that now, the call to `string_to_object' (which calls
> `read-from-string', which calls `read1', and then `read0') is located
> outside the context (not sure what the right term is) set up with
> `specbind'. Could this be a problem? Do any of these functions depend on
> the context set up in `read_minibuf'?  (They don't appear to, just want
> to be sure).

Why is it a problem to move the call to unbind_to to after the call to
add-to-history?  AFAIU, the original code, before you changed it,
actually did that.  It just didn't call string_to_object, but I'm not
sure why would that change anything.  Does anyone see a problem here?
Stefan, any comments?

bug#39291: M-: history doesn't store erroneous input

[Stefan Monnier]

> Why is it a problem to move the call to unbind_to to after the call to
> add-to-history?

Because it would make `add-to-history` add to the minibuffer-local value
rather than to the callingbuffer-local value, as the comment explains:

  /* Add the value to the appropriate history list, if any.  This is
     done after the previous buffer has been made current again, in
     case the history variable is buffer-local.  */


-- Stefan

bug#39291: M-: history doesn't store erroneous input

[Stefan Monnier]

> My only doubt is that now, the call to `string_to_object' (which calls
> `read-from-string', which calls `read1', and then `read0') is located
> outside the context (not sure what the right term is) set up with
> `specbind'. Could this be a problem? Do any of these functions depend on
> the context set up in `read_minibuf'?  (They don't appear to, just want
> to be sure).

AFAIK they don't depend on Lisp variables, no, so it should be safe to
move the call to `string_to_object` to after the `unbind_to`.


        Stefan

bug#39291: M-: history doesn't store erroneous input

[Federico Tedin]

> AFAIK they don't depend on Lisp variables, no, so it should be safe to
> move the call to `string_to_object` to after the `unbind_to`.
>
>
>         Stefan

Thanks for the information. In that case, the patch I sent should be the
right fix for this bug.

- Fede

bug#39291: M-: history doesn't store erroneous input

[Eli Zaretskii]

> From: Federico Tedin <federicotedin@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  39291@debbugs.gnu.org,  pogonyshev@gmail.com,  Juri Linkov <juri@linkov.net>
> Date: Fri, 31 Jan 2020 01:44:34 +0100
> 
> > AFAIK they don't depend on Lisp variables, no, so it should be safe to
> > move the call to `string_to_object` to after the `unbind_to`.
> >
> Thanks for the information. In that case, the patch I sent should be the
> right fix for this bug.

Thanks, pushed to the release branch.

> * src/minibuf.c (read_minibuf): Parse input string after saving the
> string to the history list instead of before, in case parsing
> fails. (Bug#39291)
       ^^
Please in the future leave 2 blanks between sentences, per our
conventions.