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 ;-).