Subject: Re: bug#15214: 24.3.50; `Revert This Session's Customization' does not do what it says

From: Mauro Aranda <[email-masked]>
Date: Wed, 1 Jan 2020 11:47:45 -0300

Eli Zaretskii <[email-masked]> writes:

>> From: Mauro Aranda <[email-masked]>
>> Date: Mon, 30 Dec 2019 18:30:31 -0300
>>
>> > Click State and Revert This Session's Customization.
>
> You should instead click "Set to Backup Value".  The Revert button is
> in general for variables you have saved for future sessions.

For user options that are of boolean type, that works well.  But for
other kinds of user options, there is a risk it won't work:

1. emacs -Q
2. M-x customize-variable dired-kept-versions
3. Edit the field to 3 and then Set for current session.
4. Now edit it to 4, and then Set for current session.

Now "Set to Backup Value" won't ever take you back to the state before
your session's customizations.

>> The function that runs for that option is custom-variable-reset-saved,
>> and the doc string of custom-variable-reset-saved says something similar.
>
> The doc string is ambiguous, and the code definitely does NOT intend
> to reset the value, just to remove the recorded setting, so that it
> won't be saved in the custom file.  That code was installed in

What part of the doc string do you find ambiguous? Personally, I was
baffled about the (themed or standard) part, because if there is a
theme value (other than the user theme), that value counts as
the saved value too.

As for the intentions of the code, a symbol with no 'saved-value property
set would be enough for the customization to not be saved in the custom
file, AFAICS.  It's true that it does not intend to reset the value, but
at least my reading of the doc string makes me think it does, the Emacs
manual says it does, and even though the use case is rare, I think it's
better to do it.

> response to a very similar bug report (bug#9509, except that it
> complained that Emacs signals an error for a variable that was never
> saved).  I very much doubt that Chong, who made that change, omitted
> setting the value by mistake.

In Bug#9509 the suggestion was to revert those settings to the
un-customized value.  I can't tell whether Chong was inclined to
take that suggestion or not, but I do think that we should take it.

> In general, there's a feature creep here: this menu item was
> originally only for customized options that were saved during this
> session.  That was lifted back then, and now we want also to change
> the value, although another menu item exists to do just that.

See the first part of my reply.  I don't think there is another menu
item in an emacs -Q session able to take one back to the uncustomized
value, reliably.

>> The attached patch fixes custom-variable-reset-saved to do what it says
>> it does when the variable has no previous saved value.
>
> I won't object too much to such a change, if you still think it's TRT
> here after reading the above, but I wonder whether it would be cleaner
> and safer to set just the value, and move the funcall out of the
> if-else form, so that we'd have only one funcall, and it will be
> inside ignore-errors.

I still think it would be a good addition to the code.  In case you are
OK with it, I attach a patch with moves the funcall outside of the
if-else form.

Best regards,
Mauro.
From 0f1307239d29f4a06878c95840d9a43318f857b5 Mon Sep 17 00:00:00 2001
From: Mauro Aranda <[email-masked]>
Date: Mon, 30 Dec 2019 18:10:28 -0300
Subject: [PATCH] Reset to the standard value when reverting session's
 customizations

* lisp/cus-edit.el (custom-variable-reset-saved): When there is no
previous saved value, reset to the standard value.  (Bug#15214)
---
 lisp/cus-edit.el | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/lisp/cus-edit.el b/lisp/cus-edit.el
index 439667a..8a5fc8c 100644
--- a/lisp/cus-edit.el
+++ b/lisp/cus-edit.el
@@ -3035,17 +3035,18 @@ custom-variable-reset-saved
 before this operation becomes the backup value."
   (let* ((symbol (widget-value widget))
 	 (saved-value (get symbol 'saved-value))
-	 (comment (get symbol 'saved-variable-comment)))
+	 (comment (get symbol 'saved-variable-comment))
+         value)
     (custom-variable-backup-value widget)
     (if (not (or saved-value comment))
-	;; If there is no saved value, remove the setting.
-	(custom-push-theme 'theme-value symbol 'user 'reset)
-      ;; Otherwise, apply the saved value.
-      (put symbol 'variable-comment comment)
-      (custom-push-theme 'theme-value symbol 'user 'set (car-safe saved-value))
-      (ignore-errors
-	(funcall (or (get symbol 'custom-set) 'set-default)
-		 symbol (eval (car saved-value)))))
+        ;; If there is no saved value, remove the setting.
+        (custom-push-theme 'theme-value symbol 'user 'reset)
+      (setq value (car-safe saved-value))
+      (custom-push-theme 'theme-value symbol 'user 'set value)
+      (put symbol 'variable-comment comment))
+    (ignore-errors
+      (funcall (or (get symbol 'custom-set) #'set-default) symbol
+               (eval (or value (car (get symbol 'standard-value))))))
     (put symbol 'customized-value nil)
     (put symbol 'customized-variable-comment nil)
     (widget-put widget :custom-state 'unknown)
-- 
2.7.4