master 779c615: Avoid saving session customizations in the custom-file

[Lars Ingebrigtsen]

branch: master
commit 779c615f333a01d11ab930b030d61545fb048f3d
Author: Mauro Aranda <maurooaranda@gmail.com>
Commit: Lars Ingebrigtsen <larsi@gnus.org>

    Avoid saving session customizations in the custom-file
    
    * lisp/custom.el (custom-theme-recalc-variable): Only stash theme
    settings for void variables.
    (custom-declare-variable): After initializing a variable, unstash a
    theme setting, if present.
    (disable-theme): When disabling a theme, maybe unstash a theme
    setting.
    
    * test/lisp/custom-resources/custom--test-theme.el: Add two settings
    for testing the fix.
---
 lisp/custom.el                                   |  39 ++++++++-
 test/lisp/custom-resources/custom--test-theme.el |   4 +-
 test/lisp/custom-tests.el                        | 104 +++++++++++++++++++++++
 3 files changed, 142 insertions(+), 5 deletions(-)

diff --git a/lisp/custom.el b/lisp/custom.el
index 614f8cf..078e3a8 100644
--- a/lisp/custom.el
+++ b/lisp/custom.el
@@ -207,7 +207,22 @@ set to nil, as the value is no longer rogue."
     (put symbol 'custom-requests requests)
     ;; Do the actual initialization.
     (unless custom-dont-initialize
-      (funcall initialize symbol default))
+      (funcall initialize symbol default)
+      ;; If there is a value under saved-value that wasn't saved by the user,
+      ;; reset it: we used that property to stash the value, but we don't need
+      ;; it anymore.
+      ;; This can happen given the following:
+      ;; 1. The user loaded a theme that had a setting for an unbound
+      ;; variable, so we stashed the theme setting under the saved-value
+      ;; property in `custom-theme-recalc-variable'.
+      ;; 2. Then, Emacs evaluated the defcustom for the option
+      ;; (e.g., something required the file where the option is defined).
+      ;; If we don't reset it and the user later sets this variable via
+      ;; Customize, we might end up saving the theme setting in the 
custom-file.
+      ;; See the test `custom-test-no-saved-value-after-customizing-option'.
+      (let ((theme (caar (get symbol 'theme-value))))
+        (when (and theme (not (eq theme 'user)) (get symbol 'saved-value))
+          (put symbol 'saved-value nil))))
     (when buffer-local
       (make-variable-buffer-local symbol)))
   (run-hooks 'custom-define-hook)
@@ -1516,7 +1531,15 @@ See `custom-enabled-themes' for a list of enabled 
themes."
           (custom-push-theme prop symbol theme 'reset)
          (cond
           ((eq prop 'theme-value)
-           (custom-theme-recalc-variable symbol))
+            (custom-theme-recalc-variable symbol)
+            ;; We might have to reset the stashed value of the variable, if
+            ;; no other theme is customizing it.  Without this, loading a theme
+            ;; that has a setting for an unbound user option and then disabling
+            ;; it will leave this lingering setting for the option, and if then
+            ;; Emacs evaluates the defcustom the saved-value might be used to
+            ;; set the variable.  (Bug#20766)
+            (unless (get symbol 'theme-value)
+              (put symbol 'saved-value nil)))
           ((eq prop 'theme-face)
            ;; If the face spec specified by this theme is in the
            ;; saved-face property, reset that property.
@@ -1565,8 +1588,16 @@ This function returns nil if no custom theme specifies a 
value for VARIABLE."
 (defun custom-theme-recalc-variable (variable)
   "Set VARIABLE according to currently enabled custom themes."
   (let ((valspec (custom-variable-theme-value variable)))
-    (if valspec
-       (put variable 'saved-value valspec)
+    ;; We used to save VALSPEC under the saved-value property unconditionally,
+    ;; but that is a recipe for trouble because we might end up saving session
+    ;; customizations if the user loads a theme.  (Bug#21355)
+    ;; It's better to only use the saved-value property to stash the value only
+    ;; if we really need to stash it (i.e., VARIABLE is void).
+    (condition-case nil
+        (default-toplevel-value variable) ; See if it doesn't fail.
+      (void-variable (when valspec
+                       (put variable 'saved-value valspec))))
+    (unless valspec
       (setq valspec (get variable 'standard-value)))
     (if (and valspec
             (or (get variable 'force-value)
diff --git a/test/lisp/custom-resources/custom--test-theme.el 
b/test/lisp/custom-resources/custom--test-theme.el
index 122bd79..36424cd 100644
--- a/test/lisp/custom-resources/custom--test-theme.el
+++ b/test/lisp/custom-resources/custom--test-theme.el
@@ -6,6 +6,8 @@
 (custom-theme-set-variables
  'custom--test
  '(custom--test-user-option 'bar)
- '(custom--test-variable 'bar))
+ '(custom--test-variable 'bar)
+ '(custom--test-bug-21355-before 'before)
+ '(custom--test-bug-21355-after 'after))
 
 (provide-theme 'custom--test)
diff --git a/test/lisp/custom-tests.el b/test/lisp/custom-tests.el
index 02a9239..e93c96e 100644
--- a/test/lisp/custom-tests.el
+++ b/test/lisp/custom-tests.el
@@ -230,4 +230,108 @@ Ensure the directory is recursively deleted after the 
fact."
       (should (eq (default-value 'custom--test-local-option) 'initial))
       (should (eq (default-value 'custom--test-permanent-option) 'initial)))))
 
+;; The following three tests demonstrate Bug#21355.
+;; In this one, we set an user option for the current session and then
+;; we enable a theme that doesn't have a setting for it, ending up with
+;; a non-nil saved-value property.  Since the `caar' of the theme-value
+;; property is user (i.e., the user theme setting is active), we might
+;; save the setting to the custom-file, even though it was meant for the
+;; current session only.  So there should be a nil saved-value property
+;; for this test to pass.
+(ert-deftest custom-test-no-saved-value-after-enabling-theme ()
+  "Test that we don't record a saved-value property when we shouldn't."
+  (let ((custom-theme-load-path `(,(ert-resource-directory))))
+    (customize-option 'mark-ring-max)
+    (let* ((field (seq-find (lambda (widget)
+                              (eq mark-ring-max (widget-value widget)))
+                            widget-field-list))
+           (parent (widget-get field :parent)))
+      ;; Move to the editable widget, modify the value and save it.
+      (goto-char (widget-field-text-end field))
+      (insert "0")
+      (widget-apply parent :custom-set)
+      ;; Just setting for the current session should not store a saved-value
+      ;; property.
+      (should-not (get 'mark-ring-max 'saved-value))
+      ;; Now enable and disable the test theme.
+      (load-theme 'custom--test 'no-confirm)
+      (disable-theme 'custom--test)
+      ;; Since the user customized the option, this is OK.
+      (should (eq (caar (get 'mark-ring-max 'theme-value)) 'user))
+      ;; The saved-value property should still be nil.
+      (should-not (get 'mark-ring-max 'saved-value)))))
+
+;; In this second test, we load a theme that has a setting for the user option
+;; above.  We must check that we don't end up with a non-nil saved-value
+;; property and a user setting active in the theme-value property, which
+;; means we might inadvertently save the session setting in the custom-file.
+(defcustom custom--test-bug-21355-before 'foo
+  "User option for `custom-test-no-saved-value-after-enabling-theme-2'."
+  :type 'symbol :group 'emacs)
+
+(ert-deftest custom-test-no-saved-value-after-enabling-theme-2 ()
+  "Test that we don't record a saved-value property when we shouldn't."
+  (let ((custom-theme-load-path `(,(ert-resource-directory))))
+    (customize-option 'custom--test-bug-21355-before)
+    (let* ((field (seq-find
+                   (lambda (widget)
+                     (eq custom--test-bug-21355-before (widget-value widget)))
+                   widget-field-list))
+           (parent (widget-get field :parent)))
+      ;; Move to the editable widget, modify the value and save it.
+      (goto-char (widget-field-text-end field))
+      (insert "bar")
+      (widget-apply parent :custom-set)
+      ;; Just setting for the current session should not store a saved-value
+      ;; property.
+      (should-not (get 'custom--test-bug-21355-before 'saved-value))
+      ;; Now load our test theme, which has a setting for
+      ;; `custom--test-bug-21355-before'.
+      (load-theme 'custom--test 'no-confirm 'no-enable)
+      (enable-theme 'custom--test)
+      ;; Since the user customized the option, this is OK.
+      (should (eq (caar (get 'custom--test-bug-21355-before 'theme-value))
+                  'user))
+      ;; But the saved-value property has to be nil, since the user didn't mark
+      ;; this variable to save for future sessions.
+      (should-not (get 'custom--test-bug-21355-before 'saved-value)))))
+
+(defvar custom--test-bug-21355-after)
+
+;; In this test, we check that stashing a theme value for a not yet defined
+;; option works, but that later on if the user customizes the option for the
+;; current session, we might save the theme setting in the custom file.
+(ert-deftest custom-test-no-saved-value-after-customizing-option ()
+  "Test for a nil saved-value after setting an option for the current session."
+  (let ((custom-theme-load-path `(,(ert-resource-directory))))
+    ;; Check that we correctly stashed the value.
+    (load-theme 'custom--test 'no-confirm 'no-enable)
+    (enable-theme 'custom--test)
+    (should (and (not (boundp 'custom--test-bug-21355-after))
+                 (eq (eval
+                      (car (get 'custom--test-bug-21355-after 'saved-value)))
+                     'after)))
+    ;; Now Emacs finds the defcustom.
+    (defcustom custom--test-bug-21355-after 'initially "..."
+      :type 'symbol :group 'emacs)
+    ;; And we used the stashed value correctly.
+    (should (and (boundp 'custom--test-bug-21355-after)
+                 (eq custom--test-bug-21355-after 'after)))
+    ;; Now customize it.
+    (customize-option 'custom--test-bug-21355-after)
+    (let* ((field (seq-find (lambda (widget)
+                              (eq custom--test-bug-21355-after
+                                  (widget-value widget)))
+                            widget-field-list))
+           (parent (widget-get field :parent)))
+      ;; Move to the editable widget, modify the value and save it.
+      (goto-char (widget-field-text-end field))
+      (insert "bar")
+      (widget-apply parent :custom-set)
+      ;; The user customized the variable, so this is OK.
+      (should (eq (caar (get 'custom--test-bug-21355-after 'theme-value))
+                  'user))
+      ;; But it was only for the current session, so this should not happen.
+      (should-not (get 'custom--test-bug-21355-after 'saved-value)))))
+
 ;;; custom-tests.el ends here