RE: 23.0.50; savehist save invalid syntax

["Drew Adams" <drew.adams@oracle.com>]

[-- 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