From 24a895f54d2dc6333c34cca69e600c494e697a26 Mon Sep 17 00:00:00 2001 From: Mauro Aranda Date: Fri, 6 Nov 2020 08:59:32 -0300 Subject: [PATCH] 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. * test/lisp/custom--tests.el (custom--test-bug-21355-before): New user option for testing. (custom-test-no-saved-value-after-enabling-theme) (custom-test-no-saved-value-after-enabling-theme-2) (custom-test-no-saved-value-after-customizing-option): New tests. --- lisp/custom.el | 40 ++++++- .../custom-resources/custom--test-theme.el | 4 +- test/lisp/custom-tests.el | 104 ++++++++++++++++++ 3 files changed, 142 insertions(+), 6 deletions(-) diff --git a/lisp/custom.el b/lisp/custom.el index cee4589543..0b23726361 100644 --- a/lisp/custom.el +++ b/lisp/custom.el @@ -205,7 +205,22 @@ custom-declare-variable (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))))) (run-hooks 'custom-define-hook) symbol) @@ -1458,7 +1473,15 @@ disable-theme (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. @@ -1507,9 +1530,16 @@ custom-variable-theme-value (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) - (setq valspec (get variable 'standard-value))) + ;; 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)))) + (or valspec (setq valspec (get variable 'standard-value))) (if (and valspec (or (get variable 'force-value) (default-boundp variable))) diff --git a/test/lisp/custom-resources/custom--test-theme.el b/test/lisp/custom-resources/custom--test-theme.el index 4ced98a50b..6b1cca3c15 100644 --- a/test/lisp/custom-resources/custom--test-theme.el +++ b/test/lisp/custom-resources/custom--test-theme.el @@ -6,6 +6,8 @@ custom--test (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 a1451cf0ce..15661fb78f 100644 --- a/test/lisp/custom-tests.el +++ b/test/lisp/custom-tests.el @@ -156,4 +156,108 @@ check-for-wrong-custom-types (load custom-test-admin-cus-test) (should (null (cus-test-opts t)))) +;; 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 -- 2.29.2