bug#8720: 24.0.50; load-theme in .emacs makes it easy to inadvertently delete custom-set variables

bug#8720: 24.0.50; load-theme in .emacs makes it easy to inadvertently delete custom-set variables

[David Engster]

This is with Emacs revno 104332 on GNU/Linux.

Here's a recipe:

* Create a minimal .emacs file, containing the following:

-------------------
;; Load a theme - it doesn't matter which.
(load-theme 'sometheme)

(custom-set-variables
 ;; custom-set-variables was added by Custom.
 ;; If you edit it by hand, you could mess it up, so be careful.
 ;; Your init file should contain only one such instance.
 ;; If there is more than one, they won't work right.
 '(user-mail-address "deng@randomsample.de"))
-------------------

* Start Emacs with that init file. Answer both questions "Really load?"
  and "Treat this theme as safe in future sessions?" with 'y'.

* Exit Emacs.

* The .emacs file will now look like this:

-------------------
;; Load a theme - it doesn't matter which
(load-theme 'zenburn)

(custom-set-variables
 ;; custom-set-variables was added by Custom.
 ;; If you edit it by hand, you could mess it up, so be careful.
 ;; Your init file should contain only one such instance.
 ;; If there is more than one, they won't work right.
 '(custom-safe-themes (quote ("79debafda48631fbb02f822cab5bb8d50df6e3f6" default))))
(custom-set-faces
 ;; custom-set-faces was added by Custom.
 ;; If you edit it by hand, you could mess it up, so be careful.
 ;; Your init file should contain only one such instance.
 ;; If there is more than one, they won't work right.
 )
--------------------

As you can see, the existing customization for user-mail-address was
deleted; in fact, every existing entry there will be deleted.  I know
this can be fixed by putting the `load-theme' at the end of the .emacs
file, but this isn't mentioned in the doc-string and I'd still consider
this at least as unexpected behavior.

-David

bug#8720: 24.0.50; load-theme in .emacs makes it easy to inadvertently delete custom-set variables

[Chong Yidong]

David Engster <deng@randomsample.de> writes:

> the existing customization for user-mail-address was deleted; in fact,
> every existing entry there will be deleted.  I know this can be fixed
> by putting the `load-theme' at the end of the .emacs file, but this
> isn't mentioned in the doc-string and I'd still consider this at least
> as unexpected behavior.

This is certainly a problem, but I don't know a good fix.

It's not limited to themes; another way to trigger it is to add a line
(find-file "foo") in your init file, where "foo" contains an unsafe
local variable; then tell Emacs to mark the variable as safe for future
visits.  The customizations in your init file are similarly wiped out.

One possibility is to do something like the following patch.  If Emacs
needs to save a variable in a function that could be called during
startup, it calls a new function customize-save-list-variable-safely
instead of customize-save-variables.  That function records the stuff to
be saved into a list, which is acted on after initialization.


=== modified file 'lisp/cus-edit.el'
*** lisp/cus-edit.el	2011-04-19 13:44:55 +0000
--- lisp/cus-edit.el	2011-06-04 23:50:48 +0000
***************
*** 1037,1042 ****
--- 1037,1059 ----
    value)
  
  ;;;###autoload
+ (defun customize-save-list-variable-safely (list-var entries)
+   "Add ENTRIES to LIST-VAR, saving the customization safely.
+ Each element in ENTRIES is added to LIST-VAR using `add-to-list'.
+ If Emacs is already initialized, call `customize-save-variable'
+ to save the resulting list value.
+ Otherwise, record args into `custom-save-after-load-list-vars',
+ so that this function is called again after initialization."
+   (dolist (elt entries)
+     (add-to-list list-var elt))
+   (cond
+    ((null after-init-time)
+     (push (list list-var entries) custom-save-after-load-list-vars))
+    ((or custom-file user-init-file)
+     (let ((coding-system-for-read nil))
+       (customize-save-variable list-var (eval list-var))))))
+ 
+ ;;;###autoload
  (defun customize ()
    "Select a customization buffer which you can use to set user options.
  User options are structured into \"groups\".

=== modified file 'lisp/files.el'
*** lisp/files.el	2011-05-28 19:26:25 +0000
--- lisp/files.el	2011-06-04 23:43:46 +0000
***************
*** 3031,3046 ****
  	    (setq char nil)))
  	(kill-buffer buf)
  	(when (and offer-save (= char ?!) unsafe-vars)
! 	  (dolist (elt unsafe-vars)
! 	    (add-to-list 'safe-local-variable-values elt))
! 	  ;; When this is called from desktop-restore-file-buffer,
! 	  ;; coding-system-for-read may be non-nil.  Reset it before
! 	  ;; writing to .emacs.
! 	  (if (or custom-file user-init-file)
! 	      (let ((coding-system-for-read nil))
! 		(customize-save-variable
! 		 'safe-local-variable-values
! 		 safe-local-variable-values))))
  	(memq char '(?! ?\s ?y))))))
  
  (defun hack-local-variables-prop-line (&optional mode-only)
--- 3031,3038 ----
  	    (setq char nil)))
  	(kill-buffer buf)
  	(when (and offer-save (= char ?!) unsafe-vars)
! 	  (customize-save-list-variable-safely 'safe-local-variable-values
! 					       unsafe-vars))
  	(memq char '(?! ?\s ?y))))))
  
  (defun hack-local-variables-prop-line (&optional mode-only)

=== modified file 'lisp/startup.el'
*** lisp/startup.el	2011-05-28 23:30:17 +0000
--- lisp/startup.el	2011-06-04 23:51:44 +0000
***************
*** 305,310 ****
--- 305,315 ----
  Setting `init-file-user' does not prevent Emacs from loading
  `site-start.el'.  The only way to do that is to use `--no-site-file'.")
  
+ (defvar custom-save-after-load-list-vars nil
+   "List of delayed customization save data.
+ After initialization, call `customize-save-list-variable-safely'
+ with each element as the list of arguments.")
+ 
  (defcustom site-run-file (purecopy "site-start")
    "File containing site-wide run-time initializations.
  This file is loaded at run-time before `~/.emacs'.  It contains inits
***************
*** 1212,1217 ****
--- 1217,1224 ----
         (package-initialize))
  
    (setq after-init-time (current-time))
+   (dolist (args custom-save-after-load-list-vars)
+     (apply 'customize-save-list-variable-safely args))
    (run-hooks 'after-init-hook)
  
    ;; Decode all default-directory.

=== modified file 'lisp/custom.el'
*** lisp/custom.el	2011-04-25 16:52:51 +0000
--- lisp/custom.el	2011-06-04 23:56:08 +0000
***************
*** 1211,1220 ****
  	  ;; Offer to save to `custom-safe-themes'.
  	  (and (or custom-file user-init-file)
  	       (y-or-n-p "Treat this theme as safe in future sessions? ")
! 	       (let ((coding-system-for-read nil))
! 		 (push hash custom-safe-themes)
! 		 (customize-save-variable 'custom-safe-themes
! 					  custom-safe-themes)))
  	  t)))))
  
  (defun custom-theme-name-valid-p (name)
--- 1211,1218 ----
  	  ;; Offer to save to `custom-safe-themes'.
  	  (and (or custom-file user-init-file)
  	       (y-or-n-p "Treat this theme as safe in future sessions? ")
! 	       (customize-save-list-variable-safely 'custom-safe-themes
! 						    (list hash)))
  	  t)))))
  
  (defun custom-theme-name-valid-p (name)

bug#8720: 24.0.50; load-theme in .emacs makes it easy to inadvertently delete custom-set variables

[David Engster]

Chong Yidong writes:
> David Engster <deng@randomsample.de> writes:
>
>> the existing customization for user-mail-address was deleted; in fact,
>> every existing entry there will be deleted.  I know this can be fixed
>> by putting the `load-theme' at the end of the .emacs file, but this
>> isn't mentioned in the doc-string and I'd still consider this at least
>> as unexpected behavior.
>
> This is certainly a problem, but I don't know a good fix.
>
> It's not limited to themes; another way to trigger it is to add a line
> (find-file "foo") in your init file, where "foo" contains an unsafe
> local variable; then tell Emacs to mark the variable as safe for future
> visits.  The customizations in your init file are similarly wiped out.

I assumed this was a general problem with customize, but I wasn't sure.

> One possibility is to do something like the following patch.  If Emacs
> needs to save a variable in a function that could be called during
> startup, it calls a new function customize-save-list-variable-safely
> instead of customize-save-variables.  That function records the stuff to
> be saved into a list, which is acted on after initialization.

Well, I figure almost any function can possibly be called during
startup. :-) But leaving that aside, I'm afraid even with your patch the
actual feature will still not work, meaning you'll be asked if you want
to "treat this theme as safe" every time you start Emacs, because the
customize section simply hasn't been read yet.

I guess to reliably use customize during startup, it would have to
automatically read the custom sections as soon as it is loaded.  Is that
somehow possible?

-David

bug#8720: 24.0.50; load-theme in .emacs makes it easy to inadvertently delete custom-set variables

[Chong Yidong]

David Engster <deng@randomsample.de> writes:

> I'm afraid even with your patch the actual feature will still not
> work, meaning you'll be asked if you want to "treat this theme as
> safe" every time you start Emacs, because the customize section simply
> hasn't been read yet.

Yes, you're right.

> I guess to reliably use customize during startup, it would have to
> automatically read the custom sections as soon as it is loaded.  Is that
> somehow possible?

Not without making Emacs startup way more complicated than it is.

Since there appears to be no way to really solve this problem, I suggest
simply mitigating it, by adding an optional argument to `load-theme'
skips the confirmation.  Users who wish to call `load-theme' in the init
file can then use this argument.

If no one has a better suggestion, I will go ahead and implement this,
and update the relevant docstrings to mention this pitfall.

bug#8720: 24.0.50; load-theme in .emacs makes it easy to inadvertently delete custom-set variables

[Stefan Monnier]

> One possibility is to do something like the following patch.  If Emacs
> needs to save a variable in a function that could be called during
> startup,

Rather than "startup" we could prefer checking "have custom settings
been fully loaded?".  It's mostly equivalent (except for things
like -Q), but it's more obviously the correct thing to do.


        Stefan