23.0.50; savehist save invalid syntax

23.0.50; savehist save invalid syntax

[Leo]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=utf-8, Size: 6416 bytes --]


Please write in English if possible, because the Emacs maintainers
usually do not have translators to read other languages for them.

Your bug report will be posted to the emacs-pretest-bug@gnu.org mailing list.

Please describe exactly what actions triggered the bug
and the precise symptoms of the bug:

`savehist' saves something with invalid syntax and prevents emacs to
start up next time. The error is:

--8<---------------cut here---------------start------------->8---
Debugger entered--Lisp error: (invalid-read-syntax "#")
  signal(invalid-read-syntax ("#"))
  byte-code("Â\x10à ˆÄ	@	A\"‡" [savehist-mode errvar nil savehist-uninstall signal] 3)
  savehist-mode(t)
  eval-buffer(#<buffer  *load*> nil "/home/leo/.emacs.d/init" nil t)  ; Reading at buffer position 4494
  load-with-code-conversion("/home/leo/.emacs.d/init" "/home/leo/.emacs.d/init" t t)
  load("/home/leo/.emacs.d/init" t t)
  #[nil "\bÂ…¾\0	Æ=ƒ\x11\0Ç\bÈQ‚A\0	É=ƒ3\0ÊÇËÌ#ƒ#\0Í‚A\0ÊÇËÎ#ƒ/\0Ï‚A\0Í‚A\0	Ð=ƒ=\0Ñ‚A\0Ç\bÒQ\x1aÓ\x13Ô\nÓ‰#ˆ\vÓ=ƒi\0ÕÖ×Ç\bØQ!\"\x1cÔ\fÓ‰#ˆ\vÓ=ƒh\0\n\x13)\vƒ®\0Ù\v!Úšƒ®\0Û\v!‰\x1dÜP\x1e$Ý\x0e$!ƒŠ\0\x0e$‚•\0Ý\r!ƒ”\0\r‚•\0ˉ\x15ƒ­\0Þ\r\v\"ƒ«\0ßà\r\v#ˆáâ!ˆ\r\x13*\x0e%?Â…½\0Ë\x1e&ÔãÓ‰#))‡" [init-file-user system-type user-init-file-1 user-init-file otherfile source ms-dos "~" "/_emacs" windows-nt directory-files nil "^\\.emacs\\(\\.elc?\\)?$" "~/.emacs" "^_emacs\\(\\.elc?\\)?$" "~/_emacs" vax-vms "sys$login:.emacs" "/.emacs" t load expand-file-name "init" file-name-as-directory "/.emacs.d" file-name-extension "elc" file-name-sans-extension ".el" file-exists-p file-newer-than-file-p message "Warning: %s is newer than %s" sit-for 1 "default" alt inhibit-default-init inhibit-startup-message] 7]()
  command-line()
  normal-top-level()
--8<---------------cut here---------------end--------------->8---

An example from the history file:

(setq command-history '((eval-expression '(format "%s " nil) nil) (describe-function 'match-string) (switch-to-buffer "gnus") (switch-to-buffer "gnus-util.el") (find-file "/home/leo/.emacs.d/gnus" t) (find-library "gnus-util") (kill-buffer "gnus-msg.el") (switch-to-buffer "gnus-msg.el") (kill-buffer "gnus-util.el") (switch-to-buffer "gnus-util.el") (kill-buffer "Emacs.org") (switch-to-buffer "Emacs.org") (describe-key "^[[" 1 nil) (describe-key '[(down-mouse-1 (#<window 256> 1284 (75 . 30) 23317460 nil 1284 (75 . 30) nil (3 . 0) (1 . 0)))] 1 '(mouse-1 (#<window 256> 1284 (75 . 30) 23317560 nil 1284 (75 . 30) nil (3 . 0) (1 . 0)))) (describe-function 'mouse-drag-track) (switch-to-buffer "*w3m*<2>") (describe-function 'mouse-drag-track) (eval-expression '(ad-update 'mouse-drag-track) nil) (eval-expression '(ad-remove-advice 'mouse-drag-track 'after 'replace-mouse-2-event) nil) (switch-to-buffer "w3m") (switch-to-buffer "Emacs.org") (describe-function 'ad-remove-advice) (switch-to-buffer "w3m") (find-file "/home/leo/Wiki/org/Emacs.org" t) (find-file "/home/leo/.emacs.d/w3m" t) (switch-to-buffer "#emacs") (describe-function 'gnus-emacs-version) (find-library "gnus-msg") (kill-buffer "gnus.el") (kill-buffer "*Customize Option: Gnus User Agent*") (describe-variable 'gnus-user-agent)))

If Emacs crashed, and you have the Emacs process in the gdb debugger,
please include the output from the following gdb commands:
    `bt full' and `xbacktrace'.
If you would like to further debug the crash, please read the file
/usr/local/packages/emacs/share/emacs/23.0.50/etc/DEBUG for instructions.


In GNU Emacs 23.0.50.1 (i686-pc-linux-gnu, GTK+ Version 2.10.14, multi-tty)
 of 2007-09-01 on Fedora
configured using `configure  '--prefix=/usr/local/packages/emacs' '--with-kerberos5' '--enable-locallisppath=/usr/local/share/emacs/site-lisp' '--without-toolkit-scroll-bars' '--with-xft' '--enable-font-backend' '--with-x-toolkit=yes''

Important settings:
  value of $LC_ALL: nil
  value of $LC_COLLATE: nil
  value of $LC_CTYPE: nil
  value of $LC_MESSAGES: nil
  value of $LC_MONETARY: nil
  value of $LC_NUMERIC: nil
  value of $LC_TIME: nil
  value of $LANG: en_GB.UTF-8
  locale-coding-system: utf-8
  default-enable-multibyte-characters: t

Major mode: Group

Minor modes in effect:
  gnus-topic-mode: t
  gnus-undo-mode: t
  erc-spelling-mode: t
  erc-page-mode: t
  erc-menu-mode: t
  erc-services-mode: t
  erc-autojoin-mode: t
  erc-button-mode: t
  erc-ring-mode: t
  erc-pcomplete-mode: t
  erc-track-mode: t
  erc-track-minor-mode: t
  erc-match-mode: t
  erc-fill-mode: t
  erc-stamp-mode: t
  erc-netsplit-mode: t
  erc-smiley-mode: t
  erc-readonly-mode: t
  erc-scrolltobottom-mode: t
  dired-omit-mode: t
  recentf-mode: t
  icomplete-mode: t
  show-paren-mode: t
  savehist-mode: t
  xterm-mouse-mode: t
  delete-selection-mode: t
  global-auto-revert-mode: t
  display-time-mode: t
  minibuffer-indicate-depth-mode: t
  partial-completion-mode: t
  which-function-mode: t
  shell-dirtrack-mode: t
  tool-bar-mode: t
  mouse-wheel-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  unify-8859-on-encoding-mode: t
  utf-translate-cjk-mode: t
  auto-compression-mode: t
  column-number-mode: t
  line-number-mode: t
  transient-mark-mode: t

Recent input:
O C ESC O C ESC O C ESC O C ESC O C ESC O C ESC O C 
ESC O C ESC O C ESC O C ESC O C ESC O C ESC O C ESC 
O C ESC O C ESC O B ; SPC u s e SPC s t r i n g - m 
a t c h SPC i n s t e a d . RET ESC O B C-c C-c ESC 
O B ESC O B ESC O B ESC O B ESC O B ESC O B ESC O B 
ESC O B ESC O B ESC O B ESC O B ESC O B ESC O B ESC 
O B ESC O A ESC O A ESC O A ESC O A ESC O A ESC O A 
ESC O A ESC O A ESC O A ESC O A ESC O A ESC O A ESC 
C-x C-x 5 0 ESC O B ESC O B ESC O B ESC O B ESC O B 
ESC O B ESC O B ESC O B ESC O B ESC O B ESC O B ESC 
O A ESC O A ESC O A ESC O A ESC O A ESC O A ESC O A 
ESC O A ESC O A ESC O A ESC O A ESC O A C-x k RET C-x 
k RET C-x k RET ESC O B ESC O B ESC O B C-x k RET C-x 
b g r RET g p p p p RET C-x ] ESC O A RET ESC u q n 
n n n n g ESC O A ESC O A ESC O A ESC x r e p o r TAB 
b u TAB RET

Recent messages:
nnimap: Checking mailboxes...
nnimap: Mailbox INBOX modified
nnimap: Checking mailboxes...done
Reading active file from nus via nnimap...
Opening nnimap server on nus...done
nnimap: Checking mailboxes...
nnimap: Mailbox INBOX modified
nnimap: Checking mailboxes...done
Reading active file from archive via nnfolder...done
Checking new news...done

RE: 23.0.50; savehist save invalid syntax

[Drew Adams]

> `savehist' saves something with invalid syntax and prevents emacs to
> start up next time. The error is:
> 
> Debugger entered--Lisp error: (invalid-read-syntax "#")

I've seen this also. A workaround, to be able to start up, is to use emacs -Q and manually edit the history file, wiping out the history (or at least the parts with #).

Re: 23.0.50; savehist save invalid syntax

[Richard Stallman]

It looks like the unreadable data is in saved arguments of commands in
command-history.  The data are not wrong, so savehist will have to
cope with them.

I think the best approach is to discard any element of command-history
that contains anything unreadable.  If the arguments of a command
include a mouse event, repeating it in another session won't make much
sense anyway.

This suggests an implementation: when writing the file, print one
element at a time, then immediately try reading it and see if it gets
an error.  If so, delete the text.

Does the patch below give good results?

Checking on output, like this, might cause a slowdown.
It would be more efficient to check when reading the saved history,
but in order to do that, we would have to give up the ability
to read it with `load'.  That is undesirable, so first let's try
this approach.

*** savehist.el	25 Jul 2007 11:49:12 -0400	1.19.2.1
--- savehist.el	02 Sep 2007 16:13:15 -0400	
***************
*** 309,318 ****
  	(insert ?\n)
  	(dolist (symbol savehist-minibuffer-history-variables)
  	  (when (boundp symbol)
! 	    (let ((value (savehist-trim-history (symbol-value symbol))))
  	      (when value		; don't save empty histories
! 		(prin1 `(setq ,symbol ',value) (current-buffer))
! 		(insert ?\n))))))
        ;; Save the additional variables.
        (dolist (symbol savehist-additional-variables)
  	(when (boundp symbol)
--- 309,346 ----
  	(insert ?\n)
  	(dolist (symbol savehist-minibuffer-history-variables)
  	  (when (boundp symbol)
! 	    (let ((value (savehist-trim-history (symbol-value symbol)))
! 		  excess-space)
  	      (when value		; don't save empty histories
! 		(insert "(setq ")
! 		(prin1 symbol (current-buffer))
! 		(insert " '(")
! 		(setq excess-space (point))
! 		;; Print elements of VALUE one by one, carefully.
! 		(dolist (elt value)
! 		  (let ((start (point)))
! 		    (insert " ")
! 		    (prin1 elt (current-buffer))
! 		    ;; Try to read the element we just printed.
! 		    (condition-case nil
! 			(save-excursion
! 			  (goto-char start)
! 			  (read (current-buffer)))
! 		      (error
! 		       ;; If reading it gets an error, comment it out.
! 		       (goto-char start)
! 		       (insert "\n")
! 		       (while (not (eobp))
! 			 (insert ";;; ")
! 			 (forward-line 1))
! 		       (insert "\n")))
! 		    (goto-char (point-max))))
! 		;; Delete the extra space before the first element.
! 		(save-excursion
! 		  (goto-char excess-space)
! 		  (if (eq (following-char) ?\s)
! 		      (delete-region (point) (1+ (point)))))
! 		(insert "))\n"))))))
        ;; Save the additional variables.
        (dolist (symbol savehist-additional-variables)
  	(when (boundp symbol)

Re: 23.0.50; savehist save invalid syntax

[Davis Herring]

> I think the best approach is to discard any element of command-history
> that contains anything unreadable.  If the arguments of a command
> include a mouse event, repeating it in another session won't make much
> sense anyway.

With this much complexity, my temptation would be to provide a
print-readable function (or an optional argument to an existing printer)
that signalled if it  were asked to print something that has no read
syntax.  Of course, this could be done in Lisp by recursively checking the
type of every element of every sequence, but that would traverse the tree
twice and would fail on circular structures unless `print-circle' were
duplicated within it.

Davis

-- 
This product is sold by volume, not by mass.  If it appears too dense or
too sparse, it is because mass-energy conversion has occurred during
shipping.

Re: 23.0.50; savehist save invalid syntax

[Stefan Monnier]

>> I think the best approach is to discard any element of command-history
>> that contains anything unreadable.  If the arguments of a command
>> include a mouse event, repeating it in another session won't make much
>> sense anyway.

> With this much complexity, my temptation would be to provide a
> print-readable function (or an optional argument to an existing printer)
> that signalled if it  were asked to print something that has no read
> syntax.  Of course, this could be done in Lisp by recursively checking the
> type of every element of every sequence, but that would traverse the tree
> twice and would fail on circular structures unless `print-circle' were
> duplicated within it.

Agreed, except that rather than failing, the function should call a callback
which could choose to print those failed elements in some way (or to signal
an error).


        Stefan

Re: 23.0.50; savehist save invalid syntax

[Richard Stallman]

    With this much complexity, my temptation would be to provide a
    print-readable function (or an optional argument to an existing printer)
    that signalled if it  were asked to print something that has no read
    syntax.

I wouldn't mind adding that feature if someone wants to do it.
It should be controlled by a variable.

Meanwhile, does my fix work?

Re: 23.0.50; savehist save invalid syntax

[Davis Herring]

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

>     With this much complexity, my temptation would be to provide a
>     print-readable function (or an optional argument to an existing
> printer)
>     that signalled if it  were asked to print something that has no read
>     syntax.
>
> I wouldn't mind adding that feature if someone wants to do it.
> It should be controlled by a variable.

I've implemented that feature.  I took Stefan's suggestion and made the
variable be a function that took over printing (or t for signaling as in
my original suggestion).  The patch is attached; please bear with me as I
think it's my most complex foray into Emacs' C thus far and introduces a
new macro and such.  It is, however, tested.

On a side note, I see that this is an extension of the variable
`print-readably' in XEmacs.  It doesn't follow naming convention, but
perhaps my new variable `print-unreadable-function' should be named or
aliased to `print-readably' for compatibility.

Meanwhile, savehist needs to use this.  It actually already has a hack in
it that uses `prin1' and `read' (or `print-readably' on XEmacs) to detect
unreadability; one could certainly use this instead of the new printing
feature.  But for cleanliness and completeness, I removed that trick and
replaced it with handlers for the unreadable signals that, now, both
varities of Emacs will generate.  (Of course, I also actually implemented
the code to skip unprintable history elements, instead of skipping the
entire history or so.)  This second patch is completely untested because
I'm unfamiliar with savehist.

Davis

-- 
This product is sold by volume, not by mass.  If it appears too dense or
too sparse, it is because mass-energy conversion has occurred during
shipping.

[-- Attachment #2: print-unreadable.patch --]
[-- Type: application/octet-stream, Size: 7233 bytes --]

Index: print.c
===================================================================
RCS file: /sources/emacs/emacs/src/print.c,v
retrieving revision 1.237
diff -c -r1.237 print.c
*** print.c	29 Aug 2007 05:27:56 -0000	1.237
--- print.c	5 Sep 2007 18:03:24 -0000
***************
*** 163,168 ****
--- 163,174 ----
  int print_number_index;
  Lisp_Object Vprint_number_table;
  
+ /* Function to call to print objects with no read syntax. */
+ Lisp_Object Qprint_unreadable_function, Vprint_unreadable_function;
+ /* We can't use do-while(0) here, so use a dangling else. */
+ #define PRINT_UNREADABLE \
+   if (escapeflag && !NILP (Vprint_unreadable_function)) {unreadable = 1; break;} else
+ 
  /* PRINT_NUMBER_OBJECT returns the I'th object in Vprint_number_table TABLE.
     PRINT_NUMBER_STATUS returns the status of the I'th object in TABLE.
     See the comment of the variable Vprint_number_table.  */
***************
*** 1475,1480 ****
--- 1481,1489 ----
       int escapeflag;
  {
    char buf[40];
+   /* If we're asked to make readable output, and we can't, and there's a
+      handler for that, set this. */
+   int unreadable = 0;
  
    QUIT;
  
***************
*** 1883,1891 ****
  	{
  	  if (escapeflag)
  	    {
! 	      strout ("#<process ", -1, -1, printcharfun, 0);
! 	      print_string (XPROCESS (obj)->name, printcharfun);
! 	      PRINTCHAR ('>');
  	    }
  	  else
  	    print_string (XPROCESS (obj)->name, printcharfun);
--- 1892,1904 ----
  	{
  	  if (escapeflag)
  	    {
! 	      if (NILP (Vprint_unreadable_function))
! 		{
! 		  strout ("#<process ", -1, -1, printcharfun, 0);
! 		  print_string (XPROCESS (obj)->name, printcharfun);
! 		  PRINTCHAR ('>');
! 		}
! 	      else unreadable = 1;
  	    }
  	  else
  	    print_string (XPROCESS (obj)->name, printcharfun);
***************
*** 1949,1960 ****
--- 1962,1975 ----
  	}
        else if (SUBRP (obj))
  	{
+ 	  PRINT_UNREADABLE;
  	  strout ("#<subr ", -1, -1, printcharfun, 0);
  	  strout (XSUBR (obj)->symbol_name, -1, -1, printcharfun, 0);
  	  PRINTCHAR ('>');
  	}
        else if (WINDOWP (obj))
  	{
+ 	  PRINT_UNREADABLE;
  	  strout ("#<window ", -1, -1, printcharfun, 0);
  	  sprintf (buf, "%ld", (long) XFASTINT (XWINDOW (obj)->sequence_number));
  	  strout (buf, -1, -1, printcharfun, 0);
***************
*** 1967,1972 ****
--- 1982,1988 ----
  	}
        else if (HASH_TABLE_P (obj))
  	{
+ 	  PRINT_UNREADABLE;
  	  struct Lisp_Hash_Table *h = XHASH_TABLE (obj);
  	  strout ("#<hash-table", -1, -1, printcharfun, 0);
  	  if (SYMBOLP (h->test))
***************
*** 1987,1992 ****
--- 2003,2009 ----
  	}
        else if (BUFFERP (obj))
  	{
+ 	  PRINT_UNREADABLE;
  	  if (NILP (XBUFFER (obj)->name))
  	    strout ("#<killed buffer>", -1, -1, printcharfun, 0);
  	  else if (escapeflag)
***************
*** 2000,2009 ****
--- 2017,2028 ----
  	}
        else if (WINDOW_CONFIGURATIONP (obj))
  	{
+ 	  PRINT_UNREADABLE;
  	  strout ("#<window-configuration>", -1, -1, printcharfun, 0);
  	}
        else if (FRAMEP (obj))
  	{
+ 	  PRINT_UNREADABLE;
  	  strout ((FRAME_LIVE_P (XFRAME (obj))
  		   ? "#<frame " : "#<dead frame "),
  		  -1, -1, printcharfun, 0);
***************
*** 2062,2067 ****
--- 2081,2087 ----
        switch (XMISCTYPE (obj))
  	{
  	case Lisp_Misc_Marker:
+ 	  PRINT_UNREADABLE;
  	  strout ("#<marker ", -1, -1, printcharfun, 0);
  	  /* Do you think this is necessary?  */
  	  if (XMARKER (obj)->insertion_type != 0)
***************
*** 2079,2084 ****
--- 2099,2105 ----
  	  break;
  
  	case Lisp_Misc_Overlay:
+ 	  PRINT_UNREADABLE;
  	  strout ("#<overlay ", -1, -1, printcharfun, 0);
  	  if (! XMARKER (OVERLAY_START (obj))->buffer)
  	    strout ("in no buffer", -1, -1, printcharfun, 0);
***************
*** 2097,2123 ****
--- 2118,2149 ----
        /* Remaining cases shouldn't happen in normal usage, but let's print
  	 them anyway for the benefit of the debugger.  */
  	case Lisp_Misc_Free:
+ 	  PRINT_UNREADABLE;
  	  strout ("#<misc free cell>", -1, -1, printcharfun, 0);
  	  break;
  
  	case Lisp_Misc_Intfwd:
+ 	  PRINT_UNREADABLE;
  	  sprintf (buf, "#<intfwd to %ld>", (long) *XINTFWD (obj)->intvar);
  	  strout (buf, -1, -1, printcharfun, 0);
  	  break;
  
  	case Lisp_Misc_Boolfwd:
+ 	  PRINT_UNREADABLE;
  	  sprintf (buf, "#<boolfwd to %s>",
  		   (*XBOOLFWD (obj)->boolvar ? "t" : "nil"));
  	  strout (buf, -1, -1, printcharfun, 0);
  	  break;
  
  	case Lisp_Misc_Objfwd:
+ 	  PRINT_UNREADABLE;
  	  strout ("#<objfwd to ", -1, -1, printcharfun, 0);
  	  print_object (*XOBJFWD (obj)->objvar, printcharfun, escapeflag);
  	  PRINTCHAR ('>');
  	  break;
  
  	case Lisp_Misc_Buffer_Objfwd:
+ 	  PRINT_UNREADABLE;
  	  strout ("#<buffer_objfwd to ", -1, -1, printcharfun, 0);
  	  print_object (PER_BUFFER_VALUE (current_buffer,
  					  XBUFFER_OBJFWD (obj)->offset),
***************
*** 2126,2131 ****
--- 2152,2158 ----
  	  break;
  
  	case Lisp_Misc_Kboard_Objfwd:
+ 	  PRINT_UNREADABLE;
  	  strout ("#<kboard_objfwd to ", -1, -1, printcharfun, 0);
  	  print_object (*(Lisp_Object *) ((char *) current_kboard
  					  + XKBOARD_OBJFWD (obj)->offset),
***************
*** 2134,2142 ****
--- 2161,2171 ----
  	  break;
  
  	case Lisp_Misc_Buffer_Local_Value:
+ 	  PRINT_UNREADABLE;
  	  strout ("#<buffer_local_value ", -1, -1, printcharfun, 0);
  	  goto do_buffer_local;
  	case Lisp_Misc_Some_Buffer_Local_Value:
+ 	  PRINT_UNREADABLE;
  	  strout ("#<some_buffer_local_value ", -1, -1, printcharfun, 0);
  	do_buffer_local:
  	  strout ("[realvalue] ", -1, -1, printcharfun, 0);
***************
*** 2167,2172 ****
--- 2196,2202 ----
  	  break;
  
  	case Lisp_Misc_Save_Value:
+ 	  PRINT_UNREADABLE;
  	  strout ("#<save_value ", -1, -1, printcharfun, 0);
  	  sprintf(buf, "ptr=0x%08lx int=%d",
  		  (unsigned long) XSAVE_VALUE (obj)->pointer,
***************
*** 2198,2203 ****
--- 2228,2247 ----
        }
      }
  
+   if (unreadable)
+     {
+       /* Suppress print-unreadable-function.  If we have a real handler, it
+ 	 has no need to call itself; if we're signaling, a debugger may need
+ 	 to print what's not printable with the signal enabled!  */
+       int count = SPECPDL_INDEX ();
+       Lisp_Object handler = Vprint_unreadable_function;
+       specbind (Qprint_unreadable_function, Qnil);
+       if (EQ (handler, Qt))
+ 	xsignal1 (Qinvalid_read_syntax, obj);
+       call2 (handler, obj, printcharfun);
+       unbind_to (count, Qnil);
+     }
+ 
    print_depth--;
  }
  \f
***************
*** 2332,2337 ****
--- 2376,2390 ----
  that need to be recorded in the table.  */);
    Vprint_number_table = Qnil;
  
+   DEFVAR_LISP ("print-unreadable-function", &Vprint_unreadable_function,
+ 	       doc: /* A function to call to print objects having no read syntax.
+ It is called with two arguments: the object to print and the output stream.
+ If t, an error is signaled to prevent producing unreadable output.
+ If nil, hash notation is used.  */);
+   Vprint_unreadable_function = Qnil;
+   Qprint_unreadable_function = intern ("print-unreadable-function");
+   staticpro (&Qprint_unreadable_function);
+ 
    /* prin1_to_string_buffer initialized in init_buffer_once in buffer.c */
    staticpro (&Vprin1_to_string_buffer);
  

[-- Attachment #3: savehist-unreadable.patch --]
[-- Type: application/octet-stream, Size: 3900 bytes --]

*** savehist.el	01 Aug 2007 11:08:24 -0600	1.21
--- savehist.el	05 Sep 2007 12:13:43 -0600	
***************
*** 299,325 ****
  	  (print-string-length nil)
  	  (print-level nil)
  	  (print-readably t)
! 	  (print-quoted t))
        ;; Save the minibuffer histories, along with the value of
        ;; savehist-minibuffer-history-variables itself.
        (when savehist-save-minibuffer-history
  	(prin1 `(setq savehist-minibuffer-history-variables
! 		      ',savehist-minibuffer-history-variables)
! 	       (current-buffer))
  	(insert ?\n)
  	(dolist (symbol savehist-minibuffer-history-variables)
  	  (when (boundp symbol)
  	    (let ((value (savehist-trim-history (symbol-value symbol))))
  	      (when value		; don't save empty histories
! 		(prin1 `(setq ,symbol ',value) (current-buffer))
! 		(insert ?\n))))))
        ;; Save the additional variables.
        (dolist (symbol savehist-additional-variables)
  	(when (boundp symbol)
! 	  (let ((value (symbol-value symbol)))
! 	    (when (savehist-printable value)
! 	      (prin1 `(setq ,symbol ',value) (current-buffer))
! 	      (insert ?\n))))))
      ;; If autosaving, avoid writing if nothing has changed since the
      ;; last write.
      (let ((checksum (md5 (current-buffer) nil nil savehist-no-conversion)))
--- 299,328 ----
  	  (print-string-length nil)
  	  (print-level nil)
  	  (print-readably t)
! 	  (print-quoted t)
! 	  (print-unreadable-function t)
! 	  (standard-output (current-buffer)))
        ;; Save the minibuffer histories, along with the value of
        ;; savehist-minibuffer-history-variables itself.
        (when savehist-save-minibuffer-history
  	(prin1 `(setq savehist-minibuffer-history-variables
! 		      ',savehist-minibuffer-history-variables))
  	(insert ?\n)
  	(dolist (symbol savehist-minibuffer-history-variables)
  	  (when (boundp symbol)
  	    (let ((value (savehist-trim-history (symbol-value symbol))))
  	      (when value		; don't save empty histories
! 		(insert "(setq ")
! 		(prin1 symbol)
! 		(insert ?\n)
! 		(while value
! 		  (and (savehist-prin1-readable (car value))
! 		       (setq value (cdr value)) (insert "  ")))
! 		(insert ?\)))))))
        ;; Save the additional variables.
        (dolist (symbol savehist-additional-variables)
  	(when (boundp symbol)
! 	  (savehist-prin1-readable `(setq ,symbol ',(symbol-value symbol)))))
      ;; If autosaving, avoid writing if nothing has changed since the
      ;; last write.
      (let ((checksum (md5 (current-buffer) nil nil savehist-no-conversion)))
***************
*** 355,380 ****
        (loop repeat history-length collect (pop value))
      value))
  
! (defun savehist-printable (value)
!   "Return non-nil if VALUE is printable."
!   (cond
!    ;; Quick response for oft-encountered types known to be printable.
!    ((stringp value))
!    ((numberp value))
!    ((symbolp value))
!    (t
!     ;; For others, check explicitly.
!     (with-temp-buffer
!       (condition-case nil
! 	  (let ((print-readably t) (print-level nil))
! 	  ;; Print the value into a buffer...
  	  (prin1 value (current-buffer))
! 	  ;; ...and attempt to read it.
! 	  (read (point-min-marker))
! 	  ;; The attempt worked: the object is printable.
! 	  t)
! 	;; The attempt failed: the object is not printable.
! 	(error nil))))))
  
  (defun savehist-minibuffer-hook ()
    (unless (or (eq minibuffer-history-variable t)
--- 358,372 ----
        (loop repeat history-length collect (pop value))
      value))
  
! (defun savehist-prin1-readable (value)
!   "Print VALUE in the current buffer, if it's readable.
! Return non-nil if it was printed."
!   (let ((opoint (point)))
!     (condition-case nil
! 	(progn
  	  (prin1 value (current-buffer))
! 	  (insert ?\n) t)
!       (invalid-read-syntax (delete-region opoint (point)) nil))))
  
  (defun savehist-minibuffer-hook ()
    (unless (or (eq minibuffer-history-variable t)

[-- Attachment #4: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: 23.0.50; savehist save invalid syntax

[Leo]

On 2007-09-05 07:16 +0100, Richard Stallman wrote:
> Meanwhile, does my fix work?

No. I am getting the following in my history file:

--8<---------------cut here---------------start------------->8---
(setq command-history '((bbdb-create '["Test" "Test2" nil nil nil nil nil ((creation-date . "2007-09-05") (timestamp . "2007-09-05")) ["Test Test2" "test2test" #<marker at 53161 in bbdb> nil]]) (bbdb "test" nil) (bbdb "goo" nil) (eval-expression '(setq bbdb-file "~/.emacs.d/etc/bbdb") nil) (savehist-mode nil)))
--8<---------------cut here---------------end--------------->8---

But it seems there is a better solution in discussion.

-- 
Leo <sdl.web AT gmail.com>                (GPG Key: 9283AA3F)

      Gnus is one component of the Emacs operating system.

Re: 23.0.50; savehist save invalid syntax

[Richard Stallman]

The code looks clean.  How about writing text for NEWS and the Lisp
manual?

RE: 23.0.50; savehist save invalid syntax

[Drew Adams]

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

1. The most common problem I've run into here is that strings with text
properties are printed so that they cannot be read - for example: #("foobar"
0 6 (face font-lock-comment-face)).

IIUC, your patch would just eliminate such strings from the history. It is
better, IMO, to convert them to unpropertized strings in
`savehist-prin1-readable':

(when (stringp value) (setq value (format "%s" value)))
(prin1 value (current-buffer))

Note that this only works for whole history elements that are strings; it
does not convert any strings that might be contained in consp history
elements. I don't know if the latter are even possible - are the elements in
a history list that someone might use savehist for always atoms? In
particular, the user can set `savehist-additional-variables' to anything, so
the code is not protected against all possible read errors.

This approach of depropertizing strings unfortunately throws away useful
information (text properties), so it is not ideal. Minibuffer input that has
text properties is useful, and it will become more useful, IMO. Similarly,
other variables that savehist might save, such as `kill-ring', can have
strings with properties. Better than removing string properties would be to
print a sexp in place of the string that, when read, would reconstitute the
string with its properties in the right places.

Is there already a function that does that? It would need to apply the
pertinent properties to the right portions of the string. In fact, shouldn't
`prin1' do this? Shouldn't it print a string that has text properties as a
sexp that, when read, creates a string with the proper text properties?


2. I think your patch was missing a right paren here:

(savehist-prin1-readable `(setq ,symbol ',(symbol-value symbol))))))


3. I think your patch was missing inserting the second right paren and the
newline character here:

(insert "))\n")


4. I think your patch was missing inserting the space, quote, and left paren
here (and a newline is not needed here):

(insert "(setq ") (prin1 symbol) (insert " '(")


5. I don't see how the condition-case in `savehist-prin1-readable' can work.
How would an `invalid-read-syntax' error ever be raised here? Isn't it only
the Lisp reader that raises that error? I think the error type should be
just `error'.


Attached is a patch that seems to work. (See #1 above - this patch removes
text properties, rather than printing a sexp to reestablish them, and this
patch only treats strings that are at the top level of a history list.)

In addition to what is mentioned above:

6. In keeping with the doc string, I replaced octal 600 with decimal 384 as
the default value of `savehist-file-modes'.

7. I wrapped a condition-case around the body of `savehist-autosave'. I
added this long ago to my version, but I cannot recall exactly why it was
needed.

8. I left in your binding of `print-unreadable-function', but I did not test
that part (I did not build Emacs from C sources).

HTH - Drew


[-- Attachment #2: savehist-2007-09-09.patch --]
[-- Type: application/octet-stream, Size: 4655 bytes --]

*** savehist-CVS-2007-09-09a.el	Sun Sep  9 09:32:10 2007
--- savehist-patched-2007-09-09.el	Sun Sep  9 13:55:20 2007
***************
*** 118,124 ****
    :type 'file
    :group 'savehist)
  
! (defcustom savehist-file-modes #o600
    "*Default permissions of the history file.
  This is decimal, not octal.  The default is 384 (0600 in octal).
  Set to nil to use the default permissions that Emacs uses, typically
--- 118,124 ----
    :type 'file
    :group 'savehist)
  
! (defcustom savehist-file-modes 384      ; Octal: #o600
    "*Default permissions of the history file.
  This is decimal, not octal.  The default is 384 (0600 in octal).
  Set to nil to use the default permissions that Emacs uses, typically
***************
*** 299,325 ****
  	  (print-string-length nil)
  	  (print-level nil)
  	  (print-readably t)
! 	  (print-quoted t))
        ;; Save the minibuffer histories, along with the value of
        ;; savehist-minibuffer-history-variables itself.
        (when savehist-save-minibuffer-history
  	(prin1 `(setq savehist-minibuffer-history-variables
! 		      ',savehist-minibuffer-history-variables)
! 	       (current-buffer))
  	(insert ?\n)
  	(dolist (symbol savehist-minibuffer-history-variables)
  	  (when (boundp symbol)
  	    (let ((value (savehist-trim-history (symbol-value symbol))))
  	      (when value		; don't save empty histories
! 		(prin1 `(setq ,symbol ',value) (current-buffer))
! 		(insert ?\n))))))
        ;; Save the additional variables.
        (dolist (symbol savehist-additional-variables)
  	(when (boundp symbol)
! 	  (let ((value (symbol-value symbol)))
! 	    (when (savehist-printable value)
! 	      (prin1 `(setq ,symbol ',value) (current-buffer))
! 	      (insert ?\n))))))
      ;; If autosaving, avoid writing if nothing has changed since the
      ;; last write.
      (let ((checksum (md5 (current-buffer) nil nil savehist-no-conversion)))
--- 299,326 ----
  	  (print-string-length nil)
  	  (print-level nil)
  	  (print-readably t)
! 	  (print-quoted t)
!           (print-unreadable-function t)
!           (standard-output (current-buffer)))
        ;; Save the minibuffer histories, along with the value of
        ;; savehist-minibuffer-history-variables itself.
        (when savehist-save-minibuffer-history
  	(prin1 `(setq savehist-minibuffer-history-variables
!                  ',savehist-minibuffer-history-variables))
  	(insert ?\n)
  	(dolist (symbol savehist-minibuffer-history-variables)
  	  (when (boundp symbol)
  	    (let ((value (savehist-trim-history (symbol-value symbol))))
  	      (when value		; don't save empty histories
!  		(insert "(setq ") (prin1 symbol) (insert " '(")
!  		(while value
!  		  (and (savehist-prin1-readable (car value))
!  		       (setq value (cdr value)) (insert " ")))
!  		(insert "))\n"))))))
        ;; Save the additional variables.
        (dolist (symbol savehist-additional-variables)
  	(when (boundp symbol)
!           (savehist-prin1-readable `(setq ,symbol ',(symbol-value symbol))))))
      ;; If autosaving, avoid writing if nothing has changed since the
      ;; last write.
      (let ((checksum (md5 (current-buffer) nil nil savehist-no-conversion)))
***************
*** 340,347 ****
  (defun savehist-autosave ()
    "Save the minibuffer history if it has been modified since the last save.
  Does nothing if `savehist-mode' is off."
!   (when savehist-mode
!     (savehist-save t)))
  
  (defun savehist-trim-history (value)
    "Retain only the first `history-length' items in VALUE.
--- 341,347 ----
  (defun savehist-autosave ()
    "Save the minibuffer history if it has been modified since the last save.
  Does nothing if `savehist-mode' is off."
!   (condition-case nil (when savehist-mode (savehist-save t)) (error nil)))
  
  (defun savehist-trim-history (value)
    "Retain only the first `history-length' items in VALUE.
***************
*** 376,381 ****
--- 376,391 ----
  	;; The attempt failed: the object is not printable.
  	(error nil))))))
  
+ (defun savehist-prin1-readable (value)
+   "Print VALUE in the current buffer, if it's readable.
+ Return non-nil if it was printed."
+   (let ((opoint (point)))
+     (condition-case nil
+ 	(progn (when (stringp value) (setq value (format "%s" value)))
+                (prin1 value (current-buffer))
+                t)
+       (error (delete-region opoint (point)) nil))))
+ 
  (defun savehist-minibuffer-hook ()
    (unless (or (eq minibuffer-history-variable t)
  	      ;; XEmacs sets minibuffer-history-variable to t to mean "no

[-- Attachment #3: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: 23.0.50; savehist save invalid syntax

[Andreas Schwab]

"Drew Adams" <drew.adams@oracle.com> writes:

> 1. The most common problem I've run into here is that strings with text
> properties are printed so that they cannot be read - for example: #("foobar"
> 0 6 (face font-lock-comment-face)).

That string can be read back just fine.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

RE: 23.0.50; savehist save invalid syntax

[Drew Adams]

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

> > 1. The most common problem I've run into here is that
> > strings with text properties are printed so that they
> > cannot be read - for example:
> > #("foobar" 0 6 (face font-lock-comment-face)).
>
> That string can be read back just fine.

Hmm, you're right. It must have been something else with # that was
sometimes getting into my saved histories and provoking a load error. But
what's odd is that deleting those strings seemed to remove the error. Oh
well, I must have missed something along the way.

I looked a bit closer at the rest as well, and I think that
`savehist-prin1-readable' needs to actually read what it prints (like it did
before Davis's patch). It should raise an error (hence remove the written
entry and return nil) if either the write or the read fails.

Emacs 20 serves as a good test for this, because it has a bug: If you do
`M-x cancel-debug-on-entry RET', then Emacs 20 inserts this invalid entry in
the `command-history': (cancel-debug-on-entry ') - note the quote mark
before the right paren. This provoked a read error at load time, but the
attached patch correctly does not include that invalid entry in the saved
`command-history'.

[savehist.el does not actually work with Emacs 20, for other reasons (e.g.
md5), but I have a version that does work, and I used that to test
`savehist-prin1-readable'.]

The attached patch fixes this and the other problems that I noted earlier
(and it does not remove text properties).

HTH - Drew

[-- Attachment #2: savehist-2007-09-09b.patch --]
[-- Type: application/octet-stream, Size: 7662 bytes --]

*** savehist-CVS-2007-09-09a.el	Sun Sep  9 09:32:10 2007
--- savehist-patched-2007-09-09b.el	Sun Sep  9 19:49:06 2007
***************
*** 79,86 ****
  
  (defcustom savehist-additional-variables ()
    "*List of additional variables to save.
! Each element is a symbol whose value will be persisted across Emacs
! sessions that use savehist.  The contents of variables should be
  printable with the Lisp printer.  You don't need to add minibuffer
  history variables to this list, all minibuffer histories will be
  saved automatically as long as `savehist-save-minibuffer-history' is
--- 79,86 ----
  
  (defcustom savehist-additional-variables ()
    "*List of additional variables to save.
! Each element is a symbol whose value is persisted across Emacs
! sessions that use `savehist'.  The contents of variables should be
  printable with the Lisp printer.  You don't need to add minibuffer
  history variables to this list, all minibuffer histories will be
  saved automatically as long as `savehist-save-minibuffer-history' is
***************
*** 118,124 ****
    :type 'file
    :group 'savehist)
  
! (defcustom savehist-file-modes #o600
    "*Default permissions of the history file.
  This is decimal, not octal.  The default is 384 (0600 in octal).
  Set to nil to use the default permissions that Emacs uses, typically
--- 118,124 ----
    :type 'file
    :group 'savehist)
  
! (defcustom savehist-file-modes 384      ; Octal: #o600
    "*Default permissions of the history file.
  This is decimal, not octal.  The default is 384 (0600 in octal).
  Set to nil to use the default permissions that Emacs uses, typically
***************
*** 237,243 ****
  is deducted from the contents of the file."
    (savehist-mode 1)
    ;; Old versions of savehist distributed with XEmacs didn't save
!   ;; savehist-minibuffer-history-variables.  If that variable is nil
    ;; after loading the file, try to intuit the intended value.
    (when (null savehist-minibuffer-history-variables)
      (setq savehist-minibuffer-history-variables
--- 237,243 ----
  is deducted from the contents of the file."
    (savehist-mode 1)
    ;; Old versions of savehist distributed with XEmacs didn't save
!   ;; `savehist-minibuffer-history-variables'.  If that variable is nil
    ;; after loading the file, try to intuit the intended value.
    (when (null savehist-minibuffer-history-variables)
      (setq savehist-minibuffer-history-variables
***************
*** 299,327 ****
  	  (print-string-length nil)
  	  (print-level nil)
  	  (print-readably t)
! 	  (print-quoted t))
        ;; Save the minibuffer histories, along with the value of
        ;; savehist-minibuffer-history-variables itself.
        (when savehist-save-minibuffer-history
  	(prin1 `(setq savehist-minibuffer-history-variables
! 		      ',savehist-minibuffer-history-variables)
! 	       (current-buffer))
  	(insert ?\n)
  	(dolist (symbol savehist-minibuffer-history-variables)
  	  (when (boundp symbol)
  	    (let ((value (savehist-trim-history (symbol-value symbol))))
! 	      (when value		; don't save empty histories
! 		(prin1 `(setq ,symbol ',value) (current-buffer))
! 		(insert ?\n))))))
        ;; Save the additional variables.
        (dolist (symbol savehist-additional-variables)
  	(when (boundp symbol)
! 	  (let ((value (symbol-value symbol)))
! 	    (when (savehist-printable value)
! 	      (prin1 `(setq ,symbol ',value) (current-buffer))
! 	      (insert ?\n))))))
!     ;; If autosaving, avoid writing if nothing has changed since the
!     ;; last write.
      (let ((checksum (md5 (current-buffer) nil nil savehist-no-conversion)))
        (unless (and auto-save (equal checksum savehist-last-checksum))
  	;; Set file-precious-flag when saving the buffer because we
--- 299,328 ----
  	  (print-string-length nil)
  	  (print-level nil)
  	  (print-readably t)
! 	  (print-quoted t)
!           (print-unreadable-function t)
!           (standard-output (current-buffer)))
        ;; Save the minibuffer histories, along with the value of
        ;; savehist-minibuffer-history-variables itself.
        (when savehist-save-minibuffer-history
  	(prin1 `(setq savehist-minibuffer-history-variables
!                  ',savehist-minibuffer-history-variables))
  	(insert ?\n)
  	(dolist (symbol savehist-minibuffer-history-variables)
  	  (when (boundp symbol)
  	    (let ((value (savehist-trim-history (symbol-value symbol))))
!               (when value		; Don't save empty histories.
!  		(insert "(setq ") (prin1 symbol) (insert " '(")
!  		(while value
!  		  (savehist-prin1-readable (car value))
!                   (setq value (cdr value)) (insert " "))
!  		(insert "))\n"))))))
        ;; Save the additional variables.
        (dolist (symbol savehist-additional-variables)
  	(when (boundp symbol)
!           (savehist-prin1-readable `(setq ,symbol ',(symbol-value symbol)))
!           (insert ?\n))))
!     ;; If autosaving, avoid writing if nothing has changed since the last write.
      (let ((checksum (md5 (current-buffer) nil nil savehist-no-conversion)))
        (unless (and auto-save (equal checksum savehist-last-checksum))
  	;; Set file-precious-flag when saving the buffer because we
***************
*** 340,347 ****
  (defun savehist-autosave ()
    "Save the minibuffer history if it has been modified since the last save.
  Does nothing if `savehist-mode' is off."
!   (when savehist-mode
!     (savehist-save t)))
  
  (defun savehist-trim-history (value)
    "Retain only the first `history-length' items in VALUE.
--- 341,347 ----
  (defun savehist-autosave ()
    "Save the minibuffer history if it has been modified since the last save.
  Does nothing if `savehist-mode' is off."
!   (condition-case nil (when savehist-mode (savehist-save t)) (error nil)))
  
  (defun savehist-trim-history (value)
    "Retain only the first `history-length' items in VALUE.
***************
*** 355,380 ****
        (loop repeat history-length collect (pop value))
      value))
  
! (defun savehist-printable (value)
!   "Return non-nil if VALUE is printable."
!   (cond
!    ;; Quick response for oft-encountered types known to be printable.
!    ((stringp value))
!    ((numberp value))
!    ((symbolp value))
!    (t
!     ;; For others, check explicitly.
!     (with-temp-buffer
        (condition-case nil
! 	  (let ((print-readably t) (print-level nil))
! 	  ;; Print the value into a buffer...
  	  (prin1 value (current-buffer))
! 	  ;; ...and attempt to read it.
! 	  (read (point-min-marker))
! 	  ;; The attempt worked: the object is printable.
  	  t)
! 	;; The attempt failed: the object is not printable.
! 	(error nil))))))
  
  (defun savehist-minibuffer-hook ()
    (unless (or (eq minibuffer-history-variable t)
--- 355,372 ----
        (loop repeat history-length collect (pop value))
      value))
  
! (defun savehist-prin1-readable (value)
!   "Print VALUE in the current buffer, but only if it's also readable.
! Return non-nil if it was printed."
!   (let ((opoint (copy-marker (point)))
!         (opoint-cpy (copy-marker (point))))
      (condition-case nil
! 	(progn ;;$$$ (when (stringp value) (setq value (format "%s" value)))
                 (prin1 value (current-buffer))
!                (read opoint)
                 t)
!       (error (delete-region opoint-cpy (point))
!              nil))))
  
  (defun savehist-minibuffer-hook ()
    (unless (or (eq minibuffer-history-variable t)

Diff finished at Sun Sep 09 19:50:32

[-- Attachment #3: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

RE: 23.0.50; savehist save invalid syntax

[Drew Adams]

Oops, you'll want to remove this comment when applying the patch:

;;$$ (when (stringp value) (setq value (format "%s" value)))

Sorry about that.

RE: 23.0.50; savehist save invalid syntax

[Davis Herring]

> 1. The most common problem I've run into here is that strings with text
> properties are printed so that they cannot be read - for example:
> #("foobar"
> 0 6 (face font-lock-comment-face)).

As Andreas said, no problem here.

> 2. I think your patch was missing a right paren here:
>
> (savehist-prin1-readable `(setq ,symbol ',(symbol-value symbol))))))
>
> 3. I think your patch was missing inserting the second right paren and the
> newline character here:
>
> (insert "))\n")
>
> 4. I think your patch was missing inserting the space, quote, and left
> paren
> here (and a newline is not needed here):
>
> (insert "(setq ") (prin1 symbol) (insert " '(")

You're right, of course; I fixed some parentheses, but obviously didn't
find all of the problems.  But there was a reason for the newline: all
elements but the first will start at the beginning of a line, so I thought
it best that the first do so as well.  I've adjusted my own copy
appropriately, for when I find the time to write up the patch
documentation.

> 5. I don't see how the condition-case in `savehist-prin1-readable' can
> work.
> How would an `invalid-read-syntax' error ever be raised here? Isn't it
> only
> the Lisp reader that raises that error? I think the error type should be
> just `error'.

The applicability of the error symbol may be questionable, but there is no
bug: with my patch to print.c (and `print-unreadable-function' bound to
t), the Lisp printer raises that error if the Lisp reader -would- raise
it.

> 6. In keeping with the doc string, I replaced octal 600 with decimal 384
> as
> the default value of `savehist-file-modes'.

The doc string has to explain the variable's appearance to the user; in
the code it's probably best to use the octal constant since that shows
what's meant.  I question why the docstring should mention its own default
value, except perhaps to say that 384==0600 in case that happens to be its
value.

> 7. I wrapped a condition-case around the body of `savehist-autosave'. I
> added this long ago to my version, but I cannot recall exactly why it was
> needed.

I can't see why that function should be particularly likely to throw,
unless `savehist-minibuffer-history-variables' contained unprintable
objects (and it should contain only symbols).  If it does, it is
presumably a bug to fix, yes?

> 8. I left in your binding of `print-unreadable-function', but I did not
> test
> that part (I did not build Emacs from C sources).

As stated just above, this is of course critical.

Davis

-- 
This product is sold by volume, not by mass.  If it appears too dense or
too sparse, it is because mass-energy conversion has occurred during
shipping.

RE: 23.0.50; savehist save invalid syntax

[Davis Herring]

> I looked a bit closer at the rest as well, and I think that
> `savehist-prin1-readable' needs to actually read what it prints (like it
> did
> before Davis's patch). It should raise an error (hence remove the written
> entry and return nil) if either the write or the read fails.

I'm not sure if you're saying it should try reading because of the Emacs
20 bug you're about to describe, or because you don't know (having written
this before my reply to your previous mail) that printing now raises an
error if the read would fail.

> Emacs 20 serves as a good test for this, because it has a bug: If you do
> `M-x cancel-debug-on-entry RET', then Emacs 20 inserts this invalid entry
> in
> the `command-history': (cancel-debug-on-entry ') - note the quote mark
> before the right paren. This provoked a read error at load time, but the
> attached patch correctly does not include that invalid entry in the saved
> `command-history'.

I happen to have a copy of Emacs 20 handy: it seems that that expression
is (cancel-debug-on-entry (quote)).  In Emacs 20, `pp' printed that
erroneously, but `prin1' and friends printed that bogus expression
correctly.  So there are two bugs (whatever put that disembodied quote
into `command-history', and `pp'), but I don't see the relevance to
savehist, especially since the signaling print on which I made it rely
will never exist in Emacs 20.

> [savehist.el does not actually work with Emacs 20, for other reasons (e.g.
> md5), but I have a version that does work, and I used that to test
> `savehist-prin1-readable'.]

What I just said about signaling prints should explain your test results,
although I don't know how you got savehist to attempt to read output from
`pp'.  Feel free to keep the print/read trick for Emacs 20, of course!

Davis

-- 
This product is sold by volume, not by mass.  If it appears too dense or
too sparse, it is because mass-energy conversion has occurred during
shipping.

RE: 23.0.50; savehist save invalid syntax

[Drew Adams]

> You're right, of course; I fixed some parentheses, but obviously didn't
> find all of the problems.  But there was a reason for the newline: all
> elements but the first will start at the beginning of a line, so I thought
> it best that the first do so as well.  I've adjusted my own copy
> appropriately, for when I find the time to write up the patch
> documentation.

The newlines are not necessary, unless you're thinking of someone viewing
.emacs-history. That is, any whitespace will do. I used a space, but a
newline is OK too.

> > 5. I don't see how the condition-case in `savehist-prin1-readable' can
> > work.
> > How would an `invalid-read-syntax' error ever be raised here? Isn't it
> > only
> > the Lisp reader that raises that error? I think the error type should be
> > just `error'.
>
> The applicability of the error symbol may be questionable, but there is no
> bug: with my patch to print.c (and `print-unreadable-function' bound to
> t), the Lisp printer raises that error if the Lisp reader -would- raise
it.

I see. I didn't look at your C-source change, and I didn't build Emacs just
to test the Lisp change. Sorry for not understanding the C change.

In that case, you might want to comment the Lisp code to that effect. It's
not very obvious that a Lisp reader error could be raised by `prin1'. And I
think it's probably still better to leave the general `error' handler.

I have a (very slightly) modified version of savehist.el (savehist-20+.el)
that works also with older Emacs versions (20, 21, 22). I will keep the
Lisp-level `read' in that version. If, as you say, it is superfluous now for
new Emacs builds, so much the better. In that case, I'll test something in
savehist-20+.el and skip the `read' for the Emacs versions that have your C
change.

What Lisp value can I test for that? It's obviously not
`emacs-major-version' >= 22. Is there a featurep or fboundp or boundp I can
test? If not, is there a minor version I can test? If I can't find something
to test, then I'll have to leave the `read' in for new Emacs versions also,
which is obviously a waste.

> > 6. In keeping with the doc string, I replaced octal 600 with decimal 384
> > as the default value of `savehist-file-modes'.
>
> The doc string has to explain the variable's appearance to the user; in
> the code it's probably best to use the octal constant since that shows
> what's meant.  I question why the docstring should mention its own default
> value, except perhaps to say that 384==0600 in case that happens to be its
> value.

Do as you think best. The doc string suggests that a decimal value is used,
so I used one. Also, I think decimal is what will be used by most users in
Customize. That is, even if one can enter #o600 in the Customize editable
field, I doubt that most users will think to do that. To me, the doc string
helps in this regard, and a decimal default value helps. I don't see a real
benefit in using octal here, but that's just my opinion.

[FWIW, I also use decimal as the value in my own version of the library
because that is required for older Emacs versions.]

> > 7. I wrapped a condition-case around the body of `savehist-autosave'. I
> > added this long ago to my version, but I cannot recall exactly
> why it was
> > needed.
>
> I can't see why that function should be particularly likely to throw,
> unless `savehist-minibuffer-history-variables' contained unprintable
> objects (and it should contain only symbols).  If it does, it is
> presumably a bug to fix, yes?

OK. As I say, I don't recall the specific need. I do recall that it was
preventing one from quitting Emacs, because `savehist-autosave' is in
`kill-emacs-hook' (as well as on a timer). That is, if, for any reason, it
has a problem, then it gets in the way of exiting. That was what was
happening, but perhaps that problem will never arise in the future ;-).

RE: 23.0.50; savehist save invalid syntax

[Drew Adams]

> I'm not sure if you're saying it should try reading because of the Emacs
> 20 bug you're about to describe, or because you don't know (having written
> this before my reply to your previous mail) that printing now raises an
> error if the read would fail.

The latter. But the fix I made to the Lisp has nothing to do with Emacs 20
per se. Without your C change, it's also needed for Emacs 22. If your C
change ensures that everything written is readable, then reading is not
needed in Lisp, of course. I wasn't clear on your C change, and I thought
you had just forgotten the read that was there before (for Emacs 22). Sorry
about that.

> So there are two [Emacs 20] bugs...but I don't see the relevance to
> savehist, especially since the signaling print on which I made it rely
> will never exist in Emacs 20.

The Emacs 20 bug only served to help me test. Otherwise, it's irrelevant
here.

Re: 23.0.50; savehist save invalid syntax

[Richard Stallman]

    I looked a bit closer at the rest as well, and I think that
    `savehist-prin1-readable' needs to actually read what it prints (like it did
    before Davis's patch). It should raise an error (hence remove the written
    entry and return nil) if either the write or the read fails.

Why is this necessary, if `prin1' signals an error when printing anything
unreadable?

RE: 23.0.50; savehist save invalid syntax

[Davis Herring]

> The newlines are not necessary, unless you're thinking of someone viewing
> .emacs-history. That is, any whitespace will do. I used a space, but a
> newline is OK too.

If we don't care what the file looks like, lots of things become optional:
(setq foo'bar) works, for instance.  I thought I'd err on the side of
readability.

> In that case, you might want to comment the Lisp code to that effect. It's
> not very obvious that a Lisp reader error could be raised by `prin1'. And
> I
> think it's probably still better to leave the general `error' handler.

I think the documentation for the (new) variable
`print-unreadable-function' may be sufficient, but I could add one.  I
specifically don't want to catch all errors there, because any other error
ought to be noticed, not suppressed.

> What Lisp value can I test for [the new print feature]? It's obviously not
> `emacs-major-version' >= 22. Is there a featurep or fboundp or boundp I
> can test? If not, is there a minor version I can test? If I can't find
> something to test, then I'll have to leave the `read' in for new Emacs
> versions also, which is obviously a waste.

You can, outside the `let', test (boundp 'print-unreadable-function).  You
can also test >= 22.2, if it's added there, or >= 23 otherwise (CVS has
such a version).

> Do as you think best. The doc string suggests that a decimal value is
> used, so I used one. Also, I think decimal is what will be used by most
> users in Customize. That is, even if one can enter #o600 in the Customize
> editable field, I doubt that most users will think to do that. To me, the
> doc string helps in this regard, and a decimal default value helps. I
> don't see a real benefit in using octal here, but that's just my opinion.

Once read, there is no difference between #o600 and 384; there are no
"octal integers", only octal integer literals.  The user never sees it,
then, and it has no effect on Customize.  The doc string is simply
reminding you that the value is the true mode value, and not some
decimal-interpreted-as-octal trick where six hundred sixty means rw-rw----
(and eighty has no meaning at all).

> OK. As I say, I don't recall the specific need. I do recall that it was
> preventing one from quitting Emacs, because `savehist-autosave' is in
> `kill-emacs-hook' (as well as on a timer). That is, if, for any reason, it
> has a problem, then it gets in the way of exiting. That was what was
> happening, but perhaps that problem will never arise in the future ;-).

I wonder why `kill-emacs-hook' is allowed to stop Emacs exiting; if a
function on it signals, shouldn't one just proceed to the next function or
to the actual exit?  It's not supposed to be able to veto exiting; that's
what `kill-emacs-query-functions' is for.

Davis

-- 
This product is sold by volume, not by mass.  If it appears too dense or
too sparse, it is because mass-energy conversion has occurred during
shipping.

Re: 23.0.50; savehist save invalid syntax

[Stefan Monnier]

> I think the documentation for the (new) variable
> `print-unreadable-function' may be sufficient, but I could add one.
> I specifically don't want to catch all errors there, because any other
> error ought to be noticed, not suppressed.

If you bind print-unreadable-function to your own function (instead of just
t), then the code will be easier to understand (and you'll be able to
use whichever error you feel like, you can even use catch/throw if its
more convenient).

I think this special case of print-unreadable-function being bound to t is
rather unnecessary.


        Stefan

RE: 23.0.50; savehist save invalid syntax

[Drew Adams]

> > The newlines are not necessary, unless you're thinking of
> > someone viewing .emacs-history. That is, any whitespace will do.
> I used a space, but a newline is OK too.
>
> If we don't care what the file looks like, lots of things become optional:
> (setq foo'bar) works, for instance.  I thought I'd err on the side of
> readability.

OK, it makes no difference to me. An argument in the other direction, FWIW,
is that something _very_ readable might give the impression that hand
editing is OK (in spite of the comment to the contrary).

> > In that case, you might want to comment the Lisp code to that
> > effect. It's not very obvious that a Lisp reader error could be
> > raised by `prin1'. And I think it's probably still better to
> > leave the general `error' handler.
>
> I think the documentation for the (new) variable
> `print-unreadable-function' may be sufficient, but I could add one.  I
> specifically don't want to catch all errors there, because any other error
> ought to be noticed, not suppressed.

By "there", I assume that you mean in the C code. I think that the savehist
code should catch all errors here. This is run on kill-emacs-hook. I doubt
we want to kick error messages back up to the user here. But do as you think
right. I have no strong opinion about this.

> > What Lisp value can I test for [the new print feature]? It's
> > obviously not `emacs-major-version' >= 22. Is there a featurep
> > or fboundp or boundp I can test? If not, is there a minor
> > version I can test? If I can't find something to test, then
> > I'll have to leave the `read' in for new Emacs
> > versions also, which is obviously a waste.
>
> You can, outside the `let', test (boundp 'print-unreadable-function).  You
> can also test >= 22.2, if it's added there, or >= 23 otherwise (CVS has
> such a version).

The first option is preferable. Thanks.

> > Do as you think best. The doc string suggests that a decimal value is
> > used, so I used one. Also, I think decimal is what will be used by most
> > users in Customize. That is, even if one can enter #o600 in the
> > Customize editable field, I doubt that most users will think to do that.
> > To me, the doc string helps in this regard, and a decimal default value
> > helps. I don't see a real benefit in using octal here, but that's just
> > my opinion.
>
> Once read, there is no difference between #o600 and 384; there are no
> "octal integers", only octal integer literals.  The user never sees it,
> then, and it has no effect on Customize.

Of course. I was explaining why the doc string speaks of a decimal value and
explains the equivalence. It is what users will see in Customize. It could
also be what they see if they examine the source code. Or not ;-).

> > OK. As I say, I don't recall the specific need. I do recall that it was
> > preventing one from quitting Emacs, because `savehist-autosave' is in
> > `kill-emacs-hook' (as well as on a timer). That is, if, for any
> > reason, it has a problem, then it gets in the way of exiting.
> > That was what was happening, but perhaps that problem will never
> > arise in the future ;-).
>
> I wonder why `kill-emacs-hook' is allowed to stop Emacs exiting; if a
> function on it signals, shouldn't one just proceed to the next function or
> to the actual exit?  It's not supposed to be able to veto exiting; that's
> what `kill-emacs-query-functions' is for.

I honestly do not remember the details. It may have had to do with this in
combination with using a quit confirmation (query); I don't recall. I've
explained why I added the condition-case, and I don't remember any more
details. You are free to take it out.

Re: 23.0.50; savehist save invalid syntax

[Davis Herring]

>     I looked a bit closer at the rest as well, and I think that
>     `savehist-prin1-readable' needs to actually read what it prints (like
> it did
>     before Davis's patch). It should raise an error (hence remove the
> written
>     entry and return nil) if either the write or the read fails.
>
> Why is this necessary, if `prin1' signals an error when printing anything
> unreadable?

It's not; it was a misunderstanding, and is already corrected.

Davis

-- 
This product is sold by volume, not by mass.  If it appears too dense or
too sparse, it is because mass-energy conversion has occurred during
shipping.

Re: [Released] Re: 23.0.50; savehist save invalid syntax

[Davis Herring]

>> I think the documentation for the (new) variable
>> `print-unreadable-function' may be sufficient, but I could add one.
>> I specifically don't want to catch all errors there, because any other
>> error ought to be noticed, not suppressed.
>
> If you bind print-unreadable-function to your own function (instead of
> just
> t), then the code will be easier to understand (and you'll be able to
> use whichever error you feel like, you can even use catch/throw if its
> more convenient).
>
> I think this special case of print-unreadable-function being bound to t is
> rather unnecessary.

It seemed to me that without that option, each package that used the
feature would quickly end up writing the same function:

(defun package-unreadable-function (obj)
  "Signal that OBJ would be unreadable."
  (error "%S can't be printed readably" obj))

(This of course loses the specific error symbol.)  I know when I was
testing it (before it supported t) I immediately felt silly writing a
function that just redirected to `error'.  It seems to me that "print this
readably, and if you can't, fail" is a reasonable thing to expect people
to want; "print this readably but do that instead if you can't" is also
reasonable, so support it we shall.

Of course, while thinking about this it occurs to me that while (4 5 ...)
(produced with a finite `print-length' or `print-level') is actually
readable, it certainly does not read as what was written.  Should this
trigger the unreadable-handling as well?

Davis

-- 
This product is sold by volume, not by mass.  If it appears too dense or
too sparse, it is because mass-energy conversion has occurred during
shipping.

Re: [Released] Re: 23.0.50; savehist save invalid syntax

[Stefan Monnier]

>> I think this special case of print-unreadable-function being bound to t is
>> rather unnecessary.

> It seemed to me that without that option, each package that used the
> feature would quickly end up writing the same function:

> (defun package-unreadable-function (obj)
>   "Signal that OBJ would be unreadable."
>   (error "%S can't be printed readably" obj))

That's OK.  We can even provide this function in subr.el so they can replace

     (let ((print-unreadable-function t))
with
     (let ((print-unreadable-function 'signal-print-unreadable-error))

[ But I do think that signalling `error' would be wrong: it needs to be more
  precise.  I'm sure you agree with me since that's already what t does
  right now in your code IIUC.  ]

> (This of course loses the specific error symbol.)  I know when I was
> testing it (before it supported t) I immediately felt silly writing a
> function that just redirected to `error'.  It seems to me that "print this

I'd personally write

   (let ((print-unreadable-function (lambda (obj) (signal 'the-error obj))))
or
   (let ((print-unreadable-function (lambda (obj) (throw 'the-error obj))))

and I don't feel silly about it at all.

> Of course, while thinking about this it occurs to me that while (4 5 ...)
> (produced with a finite `print-length' or `print-level') is actually
> readable, it certainly does not read as what was written.  Should this
> trigger the unreadable-handling as well?

I'd prefer that setting print-unreadable-function automatically overrides
any print-length or print-level limit.


        Stefan

Re: [Released] Re: 23.0.50; savehist save invalid syntax

[Richard Stallman]

    Of course, while thinking about this it occurs to me that while (4 5 ...)
    (produced with a finite `print-length' or `print-level') is actually
    readable, it certainly does not read as what was written.  Should this
    trigger the unreadable-handling as well?

I don't think so, because if you don't want the length and depth
limits, you simply don't enable them.

Re: 23.0.50; savehist save invalid syntax

[Leo]

On 2007-09-02 11:38 +0100, Leo wrote:
[...]
> Please describe exactly what actions triggered the bug
> and the precise symptoms of the bug:
>
> `savehist' saves something with invalid syntax and prevents emacs to
> start up next time. The error is:
[...]

This bug still exists in my GNU Emacs 23.0.50.16 (i686-pc-linux-gnu,
GTK+ Version 2.10.14) of 2007-10-10.

-- 
.:  Leo  :.  [ sdl.web AT gmail.com ]  .:  [ GPG Key: 9283AA3F ]  :.

       Use the most powerful email client -- http://gnus.org/

Re: 23.0.50; savehist save invalid syntax

[Leo]

On 2007-10-18 22:08 +0100, Leo wrote:
> On 2007-09-02 11:38 +0100, Leo wrote:
> [...]
>> Please describe exactly what actions triggered the bug
>> and the precise symptoms of the bug:
>>
>> `savehist' saves something with invalid syntax and prevents emacs to
>> start up next time. The error is:
> [...]
>
> This bug still exists in my GNU Emacs 23.0.50.16 (i686-pc-linux-gnu,
> GTK+ Version 2.10.14) of 2007-10-10.

I tried to work around the problem by setting:

(setq savehist-ignored-variables '(command-history))

However, command-history is still saved. Is this also a bug?

Best,
-- 
.:  Leo  :.  [ sdl.web AT gmail.com ]  .:  [ GPG Key: 9283AA3F ]  :.

       Use the most powerful email client -- http://gnus.org/

Re: 23.0.50; savehist save invalid syntax

[Stefan Monnier]

> I tried to work around the problem by setting:

> (setq savehist-ignored-variables '(command-history))

> However, command-history is still saved. Is this also a bug?

Sounds like one,


        Stefan