RE: 23.0.50; savehist save invalid syntax

[Drew Adams]

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

savehist-2007-09-09.patch
Description: Binary data