* [PATCH] Fix persist-reset
@ 2024-06-13 1:49 Joseph Turner
2024-06-14 2:47 ` Adam Porter
2024-06-15 13:23 ` Philip Kaludercic
0 siblings, 2 replies; 7+ messages in thread
From: Joseph Turner @ 2024-06-13 1:49 UTC (permalink / raw)
To: Emacs Devel Mailing List; +Cc: Adam Porter
[-- Attachment #1: Type: text/plain, Size: 245 bytes --]
Previously, persist-reset set the value of SYM to its default without
copying it, which caused subsequent modifications to the value of SYM
to erroneously modify the default value.
Thanks to Adam Porter for helping resolve this issue!
Joseph
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Copy-default-when-resetting-with-persist-reset.patch --]
[-- Type: text/x-diff, Size: 1886 bytes --]
From 39b1a0a1d1ae25130a270a115ae8241af4ed6d75 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Wed, 12 Jun 2024 18:45:32 -0700
Subject: [PATCH 1/2] Copy default when resetting with persist-reset
Previously, persist-reset set the value of SYM to its default without
copying it, which caused subsequent modifications to the value of SYM
to erroneously modify the default value.
Co-authored-by: Adam Porter <adam@alphapapa.net>
---
persist.el | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/persist.el b/persist.el
index 6cc94b4db3..df7f3836c5 100644
--- a/persist.el
+++ b/persist.el
@@ -122,9 +122,7 @@ (defun persist-symbol (symbol &optional initvalue)
(let ((initvalue (or initvalue (symbol-value symbol))))
(add-to-list 'persist--symbols symbol)
(put symbol 'persist t)
- (if (hash-table-p initvalue)
- (put symbol 'persist-default (copy-hash-table initvalue))
- (put symbol 'persist-default (persist-copy-tree initvalue t)))))
+ (put symbol 'persist-default (persist-copy initvalue))))
(defun persist--persistant-p (symbol)
"Return non-nil if SYMBOL is a persistent variable."
@@ -164,8 +162,8 @@ (defun persist-default (symbol)
(get symbol 'persist-default))
(defun persist-reset (symbol)
- "Reset the value of SYMBOL to the default."
- (set symbol (persist-default symbol)))
+ "Set the value of SYMBOL to a copy of the default."
+ (set symbol (persist-copy (persist-default symbol))))
(defun persist-load (symbol)
"Load the saved value of SYMBOL."
@@ -241,5 +239,11 @@ (defun persist-copy-tree (tree &optional vectors-and-records)
tree)
tree)))
+(defun persist-copy (obj)
+ "Return copy of OBJ."
+ (if (hash-table-p obj)
+ (copy-hash-table obj)
+ (persist-copy-tree obj t)))
+
(provide 'persist)
;;; persist.el ends here
--
2.41.0
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Test-persist-reset.patch --]
[-- Type: text/x-diff, Size: 1103 bytes --]
From 9423ecd164e5eb94bb91fbacfa2e3c821bdb2972 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Wed, 12 Jun 2024 18:45:36 -0700
Subject: [PATCH 2/2] Test persist-reset
---
test/persist-tests.el | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/test/persist-tests.el b/test/persist-tests.el
index 62d8501493..d571325fae 100644
--- a/test/persist-tests.el
+++ b/test/persist-tests.el
@@ -134,3 +134,13 @@ (ert-deftest test-persist-location ()
(should-error
(persist-save 'fred)))
(delete-directory "./persist-defined-location" t)))
+
+(ert-deftest test-persist-reset ()
+ "Symbol should be reset to a copy of the default."
+ (with-local-temp-persist
+ (persist-defvar persist--test-reset-variable (make-hash-table) "docstring")
+ (should-not (eq persist--test-reset-variable
+ (persist-default 'persist--test-reset-variable)))
+ (persist-reset 'persist--test-reset-variable)
+ (should-not (eq persist--test-reset-variable
+ (persist-default 'persist--test-reset-variable)))))
--
2.41.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] Fix persist-reset
2024-06-13 1:49 [PATCH] Fix persist-reset Joseph Turner
@ 2024-06-14 2:47 ` Adam Porter
2024-06-14 20:04 ` Joseph Turner
2024-06-15 13:23 ` Philip Kaludercic
1 sibling, 1 reply; 7+ messages in thread
From: Adam Porter @ 2024-06-14 2:47 UTC (permalink / raw)
To: Joseph Turner, Emacs Devel Mailing List
Hi Joseph,
On 6/12/24 20:49, Joseph Turner wrote:
> Previously, persist-reset set the value of SYM to its default without
> copying it, which caused subsequent modifications to the value of SYM
> to erroneously modify the default value.
The only suggestion I can think of would be, in the test, to perhaps
bind to a variable the initial hash table set as the default value, so
that you could also compare against that with EQ to ensure that the
original table is not being reused.
> Thanks to Adam Porter for helping resolve this issue!
And to you for maintaining this useful library!
--Adam
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix persist-reset
2024-06-14 2:47 ` Adam Porter
@ 2024-06-14 20:04 ` Joseph Turner
0 siblings, 0 replies; 7+ messages in thread
From: Joseph Turner @ 2024-06-14 20:04 UTC (permalink / raw)
To: Adam Porter; +Cc: Emacs Devel Mailing List
[-- Attachment #1: Type: text/plain, Size: 581 bytes --]
Adam Porter <adam@alphapapa.net> writes:
> Hi Joseph,
>
> On 6/12/24 20:49, Joseph Turner wrote:
>> Previously, persist-reset set the value of SYM to its default without
>> copying it, which caused subsequent modifications to the value of SYM
>> to erroneously modify the default value.
>
> The only suggestion I can think of would be, in the test, to perhaps
> bind to a variable the initial hash table set as the default value, so
> that you could also compare against that with EQ to ensure that the
> original table is not being reused.
Good idea! Please see new patchset.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Copy-default-when-resetting-with-persist-reset.patch --]
[-- Type: text/x-diff, Size: 1886 bytes --]
From 39b1a0a1d1ae25130a270a115ae8241af4ed6d75 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Wed, 12 Jun 2024 18:45:32 -0700
Subject: [PATCH 1/2] Copy default when resetting with persist-reset
Previously, persist-reset set the value of SYM to its default without
copying it, which caused subsequent modifications to the value of SYM
to erroneously modify the default value.
Co-authored-by: Adam Porter <adam@alphapapa.net>
---
persist.el | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/persist.el b/persist.el
index 6cc94b4db3..df7f3836c5 100644
--- a/persist.el
+++ b/persist.el
@@ -122,9 +122,7 @@ (defun persist-symbol (symbol &optional initvalue)
(let ((initvalue (or initvalue (symbol-value symbol))))
(add-to-list 'persist--symbols symbol)
(put symbol 'persist t)
- (if (hash-table-p initvalue)
- (put symbol 'persist-default (copy-hash-table initvalue))
- (put symbol 'persist-default (persist-copy-tree initvalue t)))))
+ (put symbol 'persist-default (persist-copy initvalue))))
(defun persist--persistant-p (symbol)
"Return non-nil if SYMBOL is a persistent variable."
@@ -164,8 +162,8 @@ (defun persist-default (symbol)
(get symbol 'persist-default))
(defun persist-reset (symbol)
- "Reset the value of SYMBOL to the default."
- (set symbol (persist-default symbol)))
+ "Set the value of SYMBOL to a copy of the default."
+ (set symbol (persist-copy (persist-default symbol))))
(defun persist-load (symbol)
"Load the saved value of SYMBOL."
@@ -241,5 +239,11 @@ (defun persist-copy-tree (tree &optional vectors-and-records)
tree)
tree)))
+(defun persist-copy (obj)
+ "Return copy of OBJ."
+ (if (hash-table-p obj)
+ (copy-hash-table obj)
+ (persist-copy-tree obj t)))
+
(provide 'persist)
;;; persist.el ends here
--
2.41.0
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Test-persist-reset.patch --]
[-- Type: text/x-diff, Size: 1564 bytes --]
From b30f854d149d7cbe550716d3cfdbeb170a6ca1ab Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Wed, 12 Jun 2024 18:45:36 -0700
Subject: [PATCH 2/2] Test persist-reset
Co-authored-by: Adam Porter <adam@alphapapa.net>
---
test/persist-tests.el | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/test/persist-tests.el b/test/persist-tests.el
index 62d8501493..4439fa3e07 100644
--- a/test/persist-tests.el
+++ b/test/persist-tests.el
@@ -134,3 +134,20 @@ (ert-deftest test-persist-location ()
(should-error
(persist-save 'fred)))
(delete-directory "./persist-defined-location" t)))
+
+(ert-deftest test-persist-reset ()
+ "Symbol should be reset to a copy of the default."
+ (let ((initial-value (make-hash-table)))
+ (with-local-temp-persist
+ (persist-defvar persist--test-reset-variable initial-value "docstring")
+ (should-not (eq persist--test-reset-variable
+ (persist-default 'persist--test-reset-variable)))
+ (should-not (eq persist--test-reset-variable initial-value))
+ (should-not (eq initial-value
+ (persist-default 'persist--test-reset-variable)))
+ (persist-reset 'persist--test-reset-variable)
+ (should-not (eq persist--test-reset-variable
+ (persist-default 'persist--test-reset-variable)))
+ (should-not (eq persist--test-reset-variable initial-value))
+ (should-not (eq initial-value
+ (persist-default 'persist--test-reset-variable))))))
--
2.41.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix persist-reset
2024-06-13 1:49 [PATCH] Fix persist-reset Joseph Turner
2024-06-14 2:47 ` Adam Porter
@ 2024-06-15 13:23 ` Philip Kaludercic
2024-06-15 18:28 ` Joseph Turner
1 sibling, 1 reply; 7+ messages in thread
From: Philip Kaludercic @ 2024-06-15 13:23 UTC (permalink / raw)
To: Joseph Turner; +Cc: Emacs Devel Mailing List, Adam Porter
Joseph Turner <joseph@ushin.org> writes:
> Previously, persist-reset set the value of SYM to its default without
> copying it, which caused subsequent modifications to the value of SYM
> to erroneously modify the default value.
LGTM, and seeing as you are the maintainer and there is nothing funny
going on I'll apply the patches to elpa.git.
> Thanks to Adam Porter for helping resolve this issue!
>
> Joseph
>
> From 39b1a0a1d1ae25130a270a115ae8241af4ed6d75 Mon Sep 17 00:00:00 2001
> From: Joseph Turner <joseph@breatheoutbreathe.in>
> Date: Wed, 12 Jun 2024 18:45:32 -0700
> Subject: [PATCH 1/2] Copy default when resetting with persist-reset
>
> Previously, persist-reset set the value of SYM to its default without
> copying it, which caused subsequent modifications to the value of SYM
> to erroneously modify the default value.
>
> Co-authored-by: Adam Porter <adam@alphapapa.net>
> ---
> persist.el | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/persist.el b/persist.el
> index 6cc94b4db3..df7f3836c5 100644
> --- a/persist.el
> +++ b/persist.el
> @@ -122,9 +122,7 @@ (defun persist-symbol (symbol &optional initvalue)
> (let ((initvalue (or initvalue (symbol-value symbol))))
> (add-to-list 'persist--symbols symbol)
> (put symbol 'persist t)
> - (if (hash-table-p initvalue)
> - (put symbol 'persist-default (copy-hash-table initvalue))
> - (put symbol 'persist-default (persist-copy-tree initvalue t)))))
> + (put symbol 'persist-default (persist-copy initvalue))))
>
> (defun persist--persistant-p (symbol)
> "Return non-nil if SYMBOL is a persistent variable."
> @@ -164,8 +162,8 @@ (defun persist-default (symbol)
> (get symbol 'persist-default))
>
> (defun persist-reset (symbol)
> - "Reset the value of SYMBOL to the default."
> - (set symbol (persist-default symbol)))
> + "Set the value of SYMBOL to a copy of the default."
> + (set symbol (persist-copy (persist-default symbol))))
>
> (defun persist-load (symbol)
> "Load the saved value of SYMBOL."
> @@ -241,5 +239,11 @@ (defun persist-copy-tree (tree &optional vectors-and-records)
> tree)
> tree)))
>
> +(defun persist-copy (obj)
> + "Return copy of OBJ."
> + (if (hash-table-p obj)
> + (copy-hash-table obj)
> + (persist-copy-tree obj t)))
> +
> (provide 'persist)
> ;;; persist.el ends here
> --
> 2.41.0
>
>
> From 9423ecd164e5eb94bb91fbacfa2e3c821bdb2972 Mon Sep 17 00:00:00 2001
> From: Joseph Turner <joseph@breatheoutbreathe.in>
> Date: Wed, 12 Jun 2024 18:45:36 -0700
> Subject: [PATCH 2/2] Test persist-reset
>
> ---
> test/persist-tests.el | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/test/persist-tests.el b/test/persist-tests.el
> index 62d8501493..d571325fae 100644
> --- a/test/persist-tests.el
> +++ b/test/persist-tests.el
> @@ -134,3 +134,13 @@ (ert-deftest test-persist-location ()
> (should-error
> (persist-save 'fred)))
> (delete-directory "./persist-defined-location" t)))
> +
> +(ert-deftest test-persist-reset ()
> + "Symbol should be reset to a copy of the default."
> + (with-local-temp-persist
> + (persist-defvar persist--test-reset-variable (make-hash-table) "docstring")
> + (should-not (eq persist--test-reset-variable
> + (persist-default 'persist--test-reset-variable)))
> + (persist-reset 'persist--test-reset-variable)
> + (should-not (eq persist--test-reset-variable
> + (persist-default 'persist--test-reset-variable)))))
--
Philip Kaludercic on peregrine
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix persist-reset
2024-06-15 13:23 ` Philip Kaludercic
@ 2024-06-15 18:28 ` Joseph Turner
2024-06-15 19:07 ` Philip Kaludercic
0 siblings, 1 reply; 7+ messages in thread
From: Joseph Turner @ 2024-06-15 18:28 UTC (permalink / raw)
To: Philip Kaludercic; +Cc: Emacs Devel Mailing List, Adam Porter
[-- Attachment #1: Type: text/plain, Size: 3827 bytes --]
Philip Kaludercic <philipk@posteo.net> writes:
> Joseph Turner <joseph@ushin.org> writes:
>
>> Previously, persist-reset set the value of SYM to its default without
>> copying it, which caused subsequent modifications to the value of SYM
>> to erroneously modify the default value.
>
> LGTM, and seeing as you are the maintainer and there is nothing funny
> going on I'll apply the patches to elpa.git.
Thanks! Would you please apply the second patchset with the improved
tests? I have rebased and created new patches, attached.
>> Thanks to Adam Porter for helping resolve this issue!
>>
>> Joseph
>>
>> From 39b1a0a1d1ae25130a270a115ae8241af4ed6d75 Mon Sep 17 00:00:00 2001
>> From: Joseph Turner <joseph@breatheoutbreathe.in>
>> Date: Wed, 12 Jun 2024 18:45:32 -0700
>> Subject: [PATCH 1/2] Copy default when resetting with persist-reset
>>
>> Previously, persist-reset set the value of SYM to its default without
>> copying it, which caused subsequent modifications to the value of SYM
>> to erroneously modify the default value.
>>
>> Co-authored-by: Adam Porter <adam@alphapapa.net>
>> ---
>> persist.el | 14 +++++++++-----
>> 1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/persist.el b/persist.el
>> index 6cc94b4db3..df7f3836c5 100644
>> --- a/persist.el
>> +++ b/persist.el
>> @@ -122,9 +122,7 @@ (defun persist-symbol (symbol &optional initvalue)
>> (let ((initvalue (or initvalue (symbol-value symbol))))
>> (add-to-list 'persist--symbols symbol)
>> (put symbol 'persist t)
>> - (if (hash-table-p initvalue)
>> - (put symbol 'persist-default (copy-hash-table initvalue))
>> - (put symbol 'persist-default (persist-copy-tree initvalue t)))))
>> + (put symbol 'persist-default (persist-copy initvalue))))
>>
>> (defun persist--persistant-p (symbol)
>> "Return non-nil if SYMBOL is a persistent variable."
>> @@ -164,8 +162,8 @@ (defun persist-default (symbol)
>> (get symbol 'persist-default))
>>
>> (defun persist-reset (symbol)
>> - "Reset the value of SYMBOL to the default."
>> - (set symbol (persist-default symbol)))
>> + "Set the value of SYMBOL to a copy of the default."
>> + (set symbol (persist-copy (persist-default symbol))))
>>
>> (defun persist-load (symbol)
>> "Load the saved value of SYMBOL."
>> @@ -241,5 +239,11 @@ (defun persist-copy-tree (tree &optional vectors-and-records)
>> tree)
>> tree)))
>>
>> +(defun persist-copy (obj)
>> + "Return copy of OBJ."
>> + (if (hash-table-p obj)
>> + (copy-hash-table obj)
>> + (persist-copy-tree obj t)))
>> +
>> (provide 'persist)
>> ;;; persist.el ends here
>> --
>> 2.41.0
>>
>>
>> From 9423ecd164e5eb94bb91fbacfa2e3c821bdb2972 Mon Sep 17 00:00:00 2001
>> From: Joseph Turner <joseph@breatheoutbreathe.in>
>> Date: Wed, 12 Jun 2024 18:45:36 -0700
>> Subject: [PATCH 2/2] Test persist-reset
>>
>> ---
>> test/persist-tests.el | 10 ++++++++++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git a/test/persist-tests.el b/test/persist-tests.el
>> index 62d8501493..d571325fae 100644
>> --- a/test/persist-tests.el
>> +++ b/test/persist-tests.el
>> @@ -134,3 +134,13 @@ (ert-deftest test-persist-location ()
>> (should-error
>> (persist-save 'fred)))
>> (delete-directory "./persist-defined-location" t)))
>> +
>> +(ert-deftest test-persist-reset ()
>> + "Symbol should be reset to a copy of the default."
>> + (with-local-temp-persist
>> + (persist-defvar persist--test-reset-variable (make-hash-table) "docstring")
>> + (should-not (eq persist--test-reset-variable
>> + (persist-default 'persist--test-reset-variable)))
>> + (persist-reset 'persist--test-reset-variable)
>> + (should-not (eq persist--test-reset-variable
>> + (persist-default 'persist--test-reset-variable)))))
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Also-test-that-persist-reset-does-not-reset-to-initi.patch --]
[-- Type: text/x-diff, Size: 1859 bytes --]
From 4b5c7687bf72af7f253f8b38a175f8e2c5f18cd3 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Sat, 15 Jun 2024 11:26:49 -0700
Subject: [PATCH] Also test that persist-reset does not reset to initial value
---
test/persist-tests.el | 21 ++++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/test/persist-tests.el b/test/persist-tests.el
index d571325fae..4439fa3e07 100644
--- a/test/persist-tests.el
+++ b/test/persist-tests.el
@@ -137,10 +137,17 @@ (ert-deftest test-persist-location ()
(ert-deftest test-persist-reset ()
"Symbol should be reset to a copy of the default."
- (with-local-temp-persist
- (persist-defvar persist--test-reset-variable (make-hash-table) "docstring")
- (should-not (eq persist--test-reset-variable
- (persist-default 'persist--test-reset-variable)))
- (persist-reset 'persist--test-reset-variable)
- (should-not (eq persist--test-reset-variable
- (persist-default 'persist--test-reset-variable)))))
+ (let ((initial-value (make-hash-table)))
+ (with-local-temp-persist
+ (persist-defvar persist--test-reset-variable initial-value "docstring")
+ (should-not (eq persist--test-reset-variable
+ (persist-default 'persist--test-reset-variable)))
+ (should-not (eq persist--test-reset-variable initial-value))
+ (should-not (eq initial-value
+ (persist-default 'persist--test-reset-variable)))
+ (persist-reset 'persist--test-reset-variable)
+ (should-not (eq persist--test-reset-variable
+ (persist-default 'persist--test-reset-variable)))
+ (should-not (eq persist--test-reset-variable initial-value))
+ (should-not (eq initial-value
+ (persist-default 'persist--test-reset-variable))))))
--
2.41.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix persist-reset
2024-06-15 18:28 ` Joseph Turner
@ 2024-06-15 19:07 ` Philip Kaludercic
2024-06-15 21:09 ` Joseph Turner
0 siblings, 1 reply; 7+ messages in thread
From: Philip Kaludercic @ 2024-06-15 19:07 UTC (permalink / raw)
To: Joseph Turner; +Cc: Emacs Devel Mailing List, Adam Porter
Joseph Turner <joseph@ushin.org> writes:
> Philip Kaludercic <philipk@posteo.net> writes:
>
>> Joseph Turner <joseph@ushin.org> writes:
>>
>>> Previously, persist-reset set the value of SYM to its default without
>>> copying it, which caused subsequent modifications to the value of SYM
>>> to erroneously modify the default value.
>>
>> LGTM, and seeing as you are the maintainer and there is nothing funny
>> going on I'll apply the patches to elpa.git.
>
> Thanks! Would you please apply the second patchset with the improved
> tests? I have rebased and created new patches, attached.
OK done, but this doesn't warrant a new release, right? I bumped the version
tag earlier, as my understanding was that the bug you fixed was
functional.
>>> Thanks to Adam Porter for helping resolve this issue!
>>>
>>> Joseph
>>>
>>> From 39b1a0a1d1ae25130a270a115ae8241af4ed6d75 Mon Sep 17 00:00:00 2001
>>> From: Joseph Turner <joseph@breatheoutbreathe.in>
>>> Date: Wed, 12 Jun 2024 18:45:32 -0700
>>> Subject: [PATCH 1/2] Copy default when resetting with persist-reset
>>>
>>> Previously, persist-reset set the value of SYM to its default without
>>> copying it, which caused subsequent modifications to the value of SYM
>>> to erroneously modify the default value.
>>>
>>> Co-authored-by: Adam Porter <adam@alphapapa.net>
>>> ---
>>> persist.el | 14 +++++++++-----
>>> 1 file changed, 9 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/persist.el b/persist.el
>>> index 6cc94b4db3..df7f3836c5 100644
>>> --- a/persist.el
>>> +++ b/persist.el
>>> @@ -122,9 +122,7 @@ (defun persist-symbol (symbol &optional initvalue)
>>> (let ((initvalue (or initvalue (symbol-value symbol))))
>>> (add-to-list 'persist--symbols symbol)
>>> (put symbol 'persist t)
>>> - (if (hash-table-p initvalue)
>>> - (put symbol 'persist-default (copy-hash-table initvalue))
>>> - (put symbol 'persist-default (persist-copy-tree initvalue t)))))
>>> + (put symbol 'persist-default (persist-copy initvalue))))
>>>
>>> (defun persist--persistant-p (symbol)
>>> "Return non-nil if SYMBOL is a persistent variable."
>>> @@ -164,8 +162,8 @@ (defun persist-default (symbol)
>>> (get symbol 'persist-default))
>>>
>>> (defun persist-reset (symbol)
>>> - "Reset the value of SYMBOL to the default."
>>> - (set symbol (persist-default symbol)))
>>> + "Set the value of SYMBOL to a copy of the default."
>>> + (set symbol (persist-copy (persist-default symbol))))
>>>
>>> (defun persist-load (symbol)
>>> "Load the saved value of SYMBOL."
>>> @@ -241,5 +239,11 @@ (defun persist-copy-tree (tree &optional vectors-and-records)
>>> tree)
>>> tree)))
>>>
>>> +(defun persist-copy (obj)
>>> + "Return copy of OBJ."
>>> + (if (hash-table-p obj)
>>> + (copy-hash-table obj)
>>> + (persist-copy-tree obj t)))
>>> +
>>> (provide 'persist)
>>> ;;; persist.el ends here
>>> --
>>> 2.41.0
>>>
>>>
>>> From 9423ecd164e5eb94bb91fbacfa2e3c821bdb2972 Mon Sep 17 00:00:00 2001
>>> From: Joseph Turner <joseph@breatheoutbreathe.in>
>>> Date: Wed, 12 Jun 2024 18:45:36 -0700
>>> Subject: [PATCH 2/2] Test persist-reset
>>>
>>> ---
>>> test/persist-tests.el | 10 ++++++++++
>>> 1 file changed, 10 insertions(+)
>>>
>>> diff --git a/test/persist-tests.el b/test/persist-tests.el
>>> index 62d8501493..d571325fae 100644
>>> --- a/test/persist-tests.el
>>> +++ b/test/persist-tests.el
>>> @@ -134,3 +134,13 @@ (ert-deftest test-persist-location ()
>>> (should-error
>>> (persist-save 'fred)))
>>> (delete-directory "./persist-defined-location" t)))
>>> +
>>> +(ert-deftest test-persist-reset ()
>>> + "Symbol should be reset to a copy of the default."
>>> + (with-local-temp-persist
>>> + (persist-defvar persist--test-reset-variable (make-hash-table) "docstring")
>>> + (should-not (eq persist--test-reset-variable
>>> + (persist-default 'persist--test-reset-variable)))
>>> + (persist-reset 'persist--test-reset-variable)
>>> + (should-not (eq persist--test-reset-variable
>>> + (persist-default 'persist--test-reset-variable)))))
>
> From 4b5c7687bf72af7f253f8b38a175f8e2c5f18cd3 Mon Sep 17 00:00:00 2001
> From: Joseph Turner <joseph@breatheoutbreathe.in>
> Date: Sat, 15 Jun 2024 11:26:49 -0700
> Subject: [PATCH] Also test that persist-reset does not reset to initial value
>
> ---
> test/persist-tests.el | 21 ++++++++++++++-------
> 1 file changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/test/persist-tests.el b/test/persist-tests.el
> index d571325fae..4439fa3e07 100644
> --- a/test/persist-tests.el
> +++ b/test/persist-tests.el
> @@ -137,10 +137,17 @@ (ert-deftest test-persist-location ()
>
> (ert-deftest test-persist-reset ()
> "Symbol should be reset to a copy of the default."
> - (with-local-temp-persist
> - (persist-defvar persist--test-reset-variable (make-hash-table) "docstring")
> - (should-not (eq persist--test-reset-variable
> - (persist-default 'persist--test-reset-variable)))
> - (persist-reset 'persist--test-reset-variable)
> - (should-not (eq persist--test-reset-variable
> - (persist-default 'persist--test-reset-variable)))))
> + (let ((initial-value (make-hash-table)))
> + (with-local-temp-persist
> + (persist-defvar persist--test-reset-variable initial-value "docstring")
> + (should-not (eq persist--test-reset-variable
> + (persist-default 'persist--test-reset-variable)))
> + (should-not (eq persist--test-reset-variable initial-value))
> + (should-not (eq initial-value
> + (persist-default 'persist--test-reset-variable)))
> + (persist-reset 'persist--test-reset-variable)
> + (should-not (eq persist--test-reset-variable
> + (persist-default 'persist--test-reset-variable)))
> + (should-not (eq persist--test-reset-variable initial-value))
> + (should-not (eq initial-value
> + (persist-default 'persist--test-reset-variable))))))
--
Philip Kaludercic on peregrine
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix persist-reset
2024-06-15 19:07 ` Philip Kaludercic
@ 2024-06-15 21:09 ` Joseph Turner
0 siblings, 0 replies; 7+ messages in thread
From: Joseph Turner @ 2024-06-15 21:09 UTC (permalink / raw)
To: Philip Kaludercic; +Cc: Emacs Devel Mailing List, Adam Porter
Philip Kaludercic <philipk@posteo.net> writes:
> Joseph Turner <joseph@ushin.org> writes:
>
>> Philip Kaludercic <philipk@posteo.net> writes:
>>
>>> Joseph Turner <joseph@ushin.org> writes:
>>>
>>>> Previously, persist-reset set the value of SYM to its default without
>>>> copying it, which caused subsequent modifications to the value of SYM
>>>> to erroneously modify the default value.
>>>
>>> LGTM, and seeing as you are the maintainer and there is nothing funny
>>> going on I'll apply the patches to elpa.git.
>>
>> Thanks! Would you please apply the second patchset with the improved
>> tests? I have rebased and created new patches, attached.
>
> OK done, but this doesn't warrant a new release, right? I bumped the version
> tag earlier, as my understanding was that the bug you fixed was
> functional.
Thanks! I agree, I don't think a new release is necessary.
>>>> Thanks to Adam Porter for helping resolve this issue!
>>>>
>>>> Joseph
>>>>
>>>> From 39b1a0a1d1ae25130a270a115ae8241af4ed6d75 Mon Sep 17 00:00:00 2001
>>>> From: Joseph Turner <joseph@breatheoutbreathe.in>
>>>> Date: Wed, 12 Jun 2024 18:45:32 -0700
>>>> Subject: [PATCH 1/2] Copy default when resetting with persist-reset
>>>>
>>>> Previously, persist-reset set the value of SYM to its default without
>>>> copying it, which caused subsequent modifications to the value of SYM
>>>> to erroneously modify the default value.
>>>>
>>>> Co-authored-by: Adam Porter <adam@alphapapa.net>
>>>> ---
>>>> persist.el | 14 +++++++++-----
>>>> 1 file changed, 9 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/persist.el b/persist.el
>>>> index 6cc94b4db3..df7f3836c5 100644
>>>> --- a/persist.el
>>>> +++ b/persist.el
>>>> @@ -122,9 +122,7 @@ (defun persist-symbol (symbol &optional initvalue)
>>>> (let ((initvalue (or initvalue (symbol-value symbol))))
>>>> (add-to-list 'persist--symbols symbol)
>>>> (put symbol 'persist t)
>>>> - (if (hash-table-p initvalue)
>>>> - (put symbol 'persist-default (copy-hash-table initvalue))
>>>> - (put symbol 'persist-default (persist-copy-tree initvalue t)))))
>>>> + (put symbol 'persist-default (persist-copy initvalue))))
>>>>
>>>> (defun persist--persistant-p (symbol)
>>>> "Return non-nil if SYMBOL is a persistent variable."
>>>> @@ -164,8 +162,8 @@ (defun persist-default (symbol)
>>>> (get symbol 'persist-default))
>>>>
>>>> (defun persist-reset (symbol)
>>>> - "Reset the value of SYMBOL to the default."
>>>> - (set symbol (persist-default symbol)))
>>>> + "Set the value of SYMBOL to a copy of the default."
>>>> + (set symbol (persist-copy (persist-default symbol))))
>>>>
>>>> (defun persist-load (symbol)
>>>> "Load the saved value of SYMBOL."
>>>> @@ -241,5 +239,11 @@ (defun persist-copy-tree (tree &optional vectors-and-records)
>>>> tree)
>>>> tree)))
>>>>
>>>> +(defun persist-copy (obj)
>>>> + "Return copy of OBJ."
>>>> + (if (hash-table-p obj)
>>>> + (copy-hash-table obj)
>>>> + (persist-copy-tree obj t)))
>>>> +
>>>> (provide 'persist)
>>>> ;;; persist.el ends here
>>>> --
>>>> 2.41.0
>>>>
>>>>
>>>> From 9423ecd164e5eb94bb91fbacfa2e3c821bdb2972 Mon Sep 17 00:00:00 2001
>>>> From: Joseph Turner <joseph@breatheoutbreathe.in>
>>>> Date: Wed, 12 Jun 2024 18:45:36 -0700
>>>> Subject: [PATCH 2/2] Test persist-reset
>>>>
>>>> ---
>>>> test/persist-tests.el | 10 ++++++++++
>>>> 1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/test/persist-tests.el b/test/persist-tests.el
>>>> index 62d8501493..d571325fae 100644
>>>> --- a/test/persist-tests.el
>>>> +++ b/test/persist-tests.el
>>>> @@ -134,3 +134,13 @@ (ert-deftest test-persist-location ()
>>>> (should-error
>>>> (persist-save 'fred)))
>>>> (delete-directory "./persist-defined-location" t)))
>>>> +
>>>> +(ert-deftest test-persist-reset ()
>>>> + "Symbol should be reset to a copy of the default."
>>>> + (with-local-temp-persist
>>>> + (persist-defvar persist--test-reset-variable (make-hash-table) "docstring")
>>>> + (should-not (eq persist--test-reset-variable
>>>> + (persist-default 'persist--test-reset-variable)))
>>>> + (persist-reset 'persist--test-reset-variable)
>>>> + (should-not (eq persist--test-reset-variable
>>>> + (persist-default 'persist--test-reset-variable)))))
>>
>> From 4b5c7687bf72af7f253f8b38a175f8e2c5f18cd3 Mon Sep 17 00:00:00 2001
>> From: Joseph Turner <joseph@breatheoutbreathe.in>
>> Date: Sat, 15 Jun 2024 11:26:49 -0700
>> Subject: [PATCH] Also test that persist-reset does not reset to initial value
>>
>> ---
>> test/persist-tests.el | 21 ++++++++++++++-------
>> 1 file changed, 14 insertions(+), 7 deletions(-)
>>
>> diff --git a/test/persist-tests.el b/test/persist-tests.el
>> index d571325fae..4439fa3e07 100644
>> --- a/test/persist-tests.el
>> +++ b/test/persist-tests.el
>> @@ -137,10 +137,17 @@ (ert-deftest test-persist-location ()
>>
>> (ert-deftest test-persist-reset ()
>> "Symbol should be reset to a copy of the default."
>> - (with-local-temp-persist
>> - (persist-defvar persist--test-reset-variable (make-hash-table) "docstring")
>> - (should-not (eq persist--test-reset-variable
>> - (persist-default 'persist--test-reset-variable)))
>> - (persist-reset 'persist--test-reset-variable)
>> - (should-not (eq persist--test-reset-variable
>> - (persist-default 'persist--test-reset-variable)))))
>> + (let ((initial-value (make-hash-table)))
>> + (with-local-temp-persist
>> + (persist-defvar persist--test-reset-variable initial-value "docstring")
>> + (should-not (eq persist--test-reset-variable
>> + (persist-default 'persist--test-reset-variable)))
>> + (should-not (eq persist--test-reset-variable initial-value))
>> + (should-not (eq initial-value
>> + (persist-default 'persist--test-reset-variable)))
>> + (persist-reset 'persist--test-reset-variable)
>> + (should-not (eq persist--test-reset-variable
>> + (persist-default 'persist--test-reset-variable)))
>> + (should-not (eq persist--test-reset-variable initial-value))
>> + (should-not (eq initial-value
>> + (persist-default 'persist--test-reset-variable))))))
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-06-15 21:09 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-06-13 1:49 [PATCH] Fix persist-reset Joseph Turner
2024-06-14 2:47 ` Adam Porter
2024-06-14 20:04 ` Joseph Turner
2024-06-15 13:23 ` Philip Kaludercic
2024-06-15 18:28 ` Joseph Turner
2024-06-15 19:07 ` Philip Kaludercic
2024-06-15 21:09 ` Joseph Turner
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).