Subject: Re: bug#21355: 24.4; Loading a theme causes session customizations to be saved

From: Mauro Aranda <[email-masked]>
Date: Mon, 7 Sep 2020 16:50:39 -0300

Greg Lucas <[email-masked]> writes:

> Loading a theme has the unexpected side of effect of making
> customizations in the current session saved to the custom-file the next
> time it is updated.
>
> I reproduced this problem by starting Emacs with an new empty user home
> directory (since using -q would prevent saving customizations at all).
>
> Then:
>
> M-x customize-variable sentence-end-double-space -- toggle the value,
> and Set for Current Session.
> M-x load-theme deeper-blue
> M-x customize-variable user-full-name -- set a value and Save for Future
> Sessions.
>
> I would expect only the user-full-name to be saved, but the resulting
> .emacs file contains:
>
> (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.
>  '(sentence-end-double-space nil)
>  '(user-full-name "Greg Lucas"))
> (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.
>  )
>
>
>
> From testing this I've found that at the time I call `load-theme` all
> customizations made for the current session will get marked to be saved.
> Changes made after load-theme behave as expected and do not get saved
> unless I explicitly choose to save them.

This one is really tricky.

When saving the variables to the custom-file with
custom-save-variables, we are interested in the symbols that have a
non-nil saved-value property and that don't have any theme applied:
(get symbol 'theme-value) ==> nil
or have the user theme preference applied:
(caar (get symbol 'theme-value)) ==> 'user

So we shouldn't let those combinations happen by accident.

The offending code is in custom-theme-recalc-variable: when there is a
custom theme that specifies a value for the variable, it puts that into
the 'saved-value property.  And custom-theme-recalc-variable is called
when enabling or disabling a theme.  So what happens in the recipe is:
- The user customizes some variable, so now:
(get VARIABLE 'theme-value) ==> ((user SETTING))
(get VARIABLE 'saved-value) ==> nil

- Then the user loads a theme with M-x load-theme.  So we enable it, and
then we give the priority to the user, so we re-enable the user theme.
When we enable the user theme, we end up with:
(get VARIABLE 'theme-value) ==> ((user SETTING))
(get VARIABLE 'saved-value) ==> (SETTING)

- If later in the session `custom-save-all' runs, then we save that
setting by mistake.

The code that depends on that side-effect of
custom-theme-recalc-variable is the custom-initialize-* functions (at
least in custom.el).  Those functions need to know if there is one value
stashed for the variable, to set it accordingly when initializing it.
And we stash the value when we load the custom-file or when loading a
theme.  So we are using the saved-value property for stashing this
value, which seems unfortunate to me.

Furthermore, there are some other scenarios where the bug happens:
1. Suppose we load a theme that has a setting for a bound variable.
Then we have:
(get VARIABLE 'theme-value) ==> ((THEME THEME-SETTING))
(get VARIABLE 'saved-value) ==> (THEME-SETTING)

Since THEME is not the user theme, we are not saving it in
`custom-save-all'.  But, when we disable the theme we have:
(get VARIABLE 'theme-value) ==> nil
(get VARIABLE 'saved-value) ==> (THEME-SETTING)

So if later custom-save-all runs, we lose again.

2. Now say we load a theme that has a setting for a variable that is
initially void in our session.
(get VARIABLE 'theme-value) ==> ((THEME THEME-SETTING))
(get VARIABLE 'saved-value) ==> (THEME-SETTING)

And then Emacs finds the option.  Then:
VARIABLE ==> THEME-SETTING
(get VARIABLE 'theme-value) ==> ((THEME THEME-SETTING))
(get VARIABLE 'saved-value) ==> (THEME-SETTING)

So, that shows that stashing the value worked.  But say we customize the
variable for the session:
VARIABLE ==> OUR-SETTING
(get VARIABLE 'theme-value) ==> ((user OUR-SETTING) (THEME THEME-SETTING))
(get VARIABLE 'saved-value) ==> (THEME-SETTING)

And we lose again.

I think that if we want to keep stashing the value under the
saved-value property, we could try to stash the value only if we need
to.  That is, if default-toplevel-value errors out, which would mean
custom-initialize-* will need to know if there is a saved-value.  But
then we have to make some tricks to reset the saved-value if we need
to.

I hope this analysis is helpful, I tried to keep it as short as I
could.