bug#63513: [PATCH] Make persist-defvar work with records and hash tables

bug#63513: [PATCH] Make persist-defvar work with records and hash tables

[Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 964 bytes --]

Hello!

persist-defvar does not persist records or hash tables correctly.

Here's a minimal example with a hash table:

(progn
  (persist-defvar foo (make-hash-table :test #'equal) "docstring")
  (puthash 'bar t foo)
  (persist-save 'foo)
  (gethash 'bar (persist-default 'foo))) ;; Expected nil, got t

This patch fixes persisting records by using copy-tree. Currently,
copy-tree does not work with records (see
<https://lists.gnu.org/archive/html/bug-gnu-emacs/2023-05/msg00875.html>).
The following snippet will return the expected value only when both this
patch and the above-linked patch are applied:

(progn
  (cl-defstruct baz a)
  (persist-defvar quux (make-baz :a nil) "docstring")
  (setf (baz-a quux) t)
  (persist-save 'quux)
  (baz-a (persist-default 'quux))) ;; Expected nil, got t

Before applying this patch, the updated values in both cases are not
persisted to disk. With the patch, the updated values are persisted as
expected.

Best,

Joseph


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Make-persist-defvar-work-with-records-and-hash-table.patch --]
[-- Type: text/x-diff, Size: 1141 bytes --]

From b1512983fb82b9800ab6d0d1c9ca359e1251e94a Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Sun, 14 May 2023 22:32:16 -0700
Subject: [PATCH] Make persist-defvar work with records and hash tables

Previously, when persist-defvar received a record or hash table for an
initial value, updated values were not persisted. This was because the
value and default value shared the same structure.
---
 persist.el | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/persist.el b/persist.el
index d80943d19e..a40fc2b297 100644
--- a/persist.el
+++ b/persist.el
@@ -118,7 +118,9 @@ (defun persist-symbol (symbol &optional initvalue)
   (let ((initvalue (or initvalue (symbol-value symbol))))
     (add-to-list 'persist--symbols symbol)
     (put symbol 'persist t)
-    (put symbol 'persist-default initvalue)))
+    (if (eq 'hash-table (type-of initvalue))
+        (put symbol 'persist-default (copy-hash-table initvalue))
+      (put symbol 'persist-default (copy-tree initvalue t)))))
 
 (defun persist--persistant-p (symbol)
   "Return non-nil if SYMBOL is a persistant variable."
-- 
2.40.1

bug#63513: [PATCH] Make persist-defvar work with records and hash tables

[Eli Zaretskii]

> Cc: Adam Porter <adam@alphapapa.net>
> Date: Sun, 14 May 2023 22:56:20 -0700
> From:  Joseph Turner via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> persist-defvar does not persist records or hash tables correctly.
> 
> Here's a minimal example with a hash table:
> 
> (progn
>   (persist-defvar foo (make-hash-table :test #'equal) "docstring")
>   (puthash 'bar t foo)
>   (persist-save 'foo)
>   (gethash 'bar (persist-default 'foo))) ;; Expected nil, got t
> 
> This patch fixes persisting records by using copy-tree. Currently,
> copy-tree does not work with records (see
> <https://lists.gnu.org/archive/html/bug-gnu-emacs/2023-05/msg00875.html>).
> The following snippet will return the expected value only when both this
> patch and the above-linked patch are applied:
> 
> (progn
>   (cl-defstruct baz a)
>   (persist-defvar quux (make-baz :a nil) "docstring")
>   (setf (baz-a quux) t)
>   (persist-save 'quux)
>   (baz-a (persist-default 'quux))) ;; Expected nil, got t
> 
> Before applying this patch, the updated values in both cases are not
> persisted to disk. With the patch, the updated values are persisted as
> expected.

Philip, any comments?

bug#63513: [PATCH] Make persist-defvar work with records and hash tables

[Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 258 bytes --]

The patch should now work on Emacs versions before Emacs 30. I also
added tests for persisting records and hash tables.

Instead of copying the updated behavior of copy-tree into persist.el,
would it be more appropriate to require compat.el?

Best,

Joseph


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-test-persist-save-macro.patch --]
[-- Type: text/x-diff, Size: 3379 bytes --]

From 7907521e72e2e99b883912f250b7afb14cbf5e80 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Tue, 23 May 2023 12:57:02 -0700
Subject: [PATCH 1/5] Add test-persist-save macro

---
 test/persist-tests.el | 76 +++++++++++++++++++------------------------
 1 file changed, 33 insertions(+), 43 deletions(-)

diff --git a/test/persist-tests.el b/test/persist-tests.el
index b6645a9297..0a85b78767 100644
--- a/test/persist-tests.el
+++ b/test/persist-tests.el
@@ -25,51 +25,41 @@ (ert-deftest test-persist-save-only-persistant ()
    :type 'error
    :exclude-subtypes t))
 
-(ert-deftest test-persist-save ()
-  (with-local-temp-persist
-   (let ((sym (cl-gensym)))
-     ;; precondition
-   (should-not (file-exists-p (persist--file-location sym)))
-     (set sym 10)
-     (persist-symbol sym 10)
-     (persist-save sym)
-     (should t)
-     (should-not (file-exists-p (persist--file-location sym)))
-     (set sym 20)
-     (persist-save sym)
-     (should (file-exists-p (persist--file-location sym)))
-     (should
-      (string-match-p
-       "20"
-       (with-temp-buffer
-         (insert-file-contents (persist--file-location sym))
-         (buffer-string))))
-     (set sym 10)
-     (persist-save sym)
-     (should-not (file-exists-p (persist--file-location sym)))
-     (should-error
-      (persist-save 'fred)))))
+(defmacro test-persist-save (init default change printed-changed)
+  "Test persisting symbols.
+- symbol is not persisted when value is set to INIT and default
+  value is set to DEFAULT.
+- symbol is persisted when value is changed according to CHANGE.
+- persisted file contents match PRINTED-CHANGED.
+- symbol is not persisted after value is set back to DEFAULT."
+  `(with-local-temp-persist
+    (let ((sym (cl-gensym)))
+      (should-not (file-exists-p (persist--file-location sym)))
+      (set sym ,init)
+      (persist-symbol sym ,default)
+      (persist-save sym)
+      (should t)
+      (should-not (file-exists-p (persist--file-location sym)))
+      ,change
+      (persist-save sym)
+      (should (file-exists-p (persist--file-location sym)))
+      (should
+       (string-match-p
+        ,printed-changed
+        (with-temp-buffer
+          (insert-file-contents (persist--file-location sym))
+          (buffer-string))))
+      (set sym ,default)
+      (persist-save sym)
+      (should-not (file-exists-p (persist--file-location sym))))))
 
-(ert-deftest test-persist-save-non-number ()
-  "Test saving something that is not a number.
+(ert-deftest test-persist-save-number ()
+  "Test saving number."
+  (test-persist-save 1 1 (set sym 2) "2"))
 
-`test-persist-save' missed "
-  (with-local-temp-persist
-   (let ((sym (cl-gensym)))
-     (set sym "fred")
-     (persist-symbol sym "fred")
-     (persist-save sym)
-     (should t)
-     (should-not (file-exists-p (persist--file-location sym)))
-     (set sym "george")
-     (persist-save sym)
-     (should (file-exists-p (persist--file-location sym)))
-     (should
-      (string-match-p
-       "george"
-       (with-temp-buffer
-         (insert-file-contents (persist--file-location sym))
-         (buffer-string)))))))
+(ert-deftest test-persist-save-string ()
+  "Test saving string."
+  (test-persist-save "foo" "foo" (set sym "bar") "bar"))
 
 (ert-deftest test-persist-load ()
   (with-local-temp-persist
-- 
2.40.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Add-persist-hash-equal.patch --]
[-- Type: text/x-diff, Size: 1460 bytes --]

From 41f90ac59a26018382d1bb9153af08ce4b9423ff Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Tue, 23 May 2023 12:52:24 -0700
Subject: [PATCH 2/5] Add persist-hash-equal

See bug#63671.
---
 persist.el | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/persist.el b/persist.el
index d80943d19e..0069273ca2 100644
--- a/persist.el
+++ b/persist.el
@@ -187,5 +187,22 @@ (defun persist--save-all ()
 (add-hook 'kill-emacs-hook
           'persist--save-all)
 
+(defun persist-hash-equal (hash1 hash2)
+  "Return non-nil when the contents of HASH1 and HASH2 are equal.
+Table values are compared using `equal' unless they are both hash
+tables themselves, in which case `hash-equal' is used.
+Does not compare equality predicates."
+  (and (= (hash-table-count hash1)
+          (hash-table-count hash2))
+       (catch 'flag (maphash (lambda (key hash1-value)
+                               (let ((hash2-value (gethash key hash2)))
+                                 (or (if (and (hash-table-p hash1-value)
+                                              (hash-table-p hash2-value))
+                                         (hash-equal hash1-value hash2-value)
+                                       (equal hash1-value hash2-value))
+                                     (throw 'flag nil))))
+                             hash1)
+              t)))
+
 (provide 'persist)
 ;;; persist.el ends here
-- 
2.40.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-Make-persist-defvar-work-with-hash-tables.patch --]
[-- Type: text/x-diff, Size: 2988 bytes --]

From 53e166cb0c7c2624a5a54edafe8828d7b7edc612 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Tue, 23 May 2023 13:09:29 -0700
Subject: [PATCH 3/5] Make persist-defvar work with hash tables

Previously, when persist-defvar received a hash table for an initial
value, updated values were not persisted.
---
 persist.el            | 16 +++++++++++-----
 test/persist-tests.el |  8 ++++++++
 2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/persist.el b/persist.el
index 0069273ca2..725f2f71cf 100644
--- a/persist.el
+++ b/persist.el
@@ -118,7 +118,9 @@ (defun persist-symbol (symbol &optional initvalue)
   (let ((initvalue (or initvalue (symbol-value symbol))))
     (add-to-list 'persist--symbols symbol)
     (put symbol 'persist t)
-    (put symbol 'persist-default initvalue)))
+    (if (hash-table-p initvalue)
+        (put symbol 'persist-default (copy-hash-table initvalue))
+      (put symbol 'persist-default initvalue))))
 
 (defun persist--persistant-p (symbol)
   "Return non-nil if SYMBOL is a persistant variable."
@@ -132,9 +134,13 @@ (defun persist-save (symbol)
   (unless (persist--persistant-p symbol)
     (error (format
             "Symbol %s is not persistant" symbol)))
-  (let ((symbol-file-loc (persist--file-location symbol)))
-    (if (equal (symbol-value symbol)
-               (persist-default symbol))
+  (let ((symbol-file-loc (persist--file-location symbol))
+        (value (symbol-value symbol))
+        (default (persist-default symbol)))
+    (if (if (and (hash-table-p value)
+                 (hash-table-p default))
+            (persist-hash-equal value default)
+          (equal value default))
         (when (file-exists-p symbol-file-loc)
           (delete-file symbol-file-loc))
       (let ((dir-loc
@@ -148,7 +154,7 @@ (defun persist-save (symbol)
                 (print-escape-control-characters t)
                 (print-escape-nonascii t)
                 (print-circle t))
-            (print (symbol-value symbol) (current-buffer)))
+            (print value (current-buffer)))
           (write-region (point-min) (point-max)
                         symbol-file-loc
                         nil 'quiet))))))
diff --git a/test/persist-tests.el b/test/persist-tests.el
index 0a85b78767..8a30a24e23 100644
--- a/test/persist-tests.el
+++ b/test/persist-tests.el
@@ -61,6 +61,14 @@ (ert-deftest test-persist-save-string ()
   "Test saving string."
   (test-persist-save "foo" "foo" (set sym "bar") "bar"))
 
+(ert-deftest test-persist-save-hash ()
+  "Test saving hash table."
+  (let* ((hash (make-hash-table))
+         (default (copy-hash-table hash)))
+    (test-persist-save hash default
+                       (puthash 'foo "bar" (symbol-value sym))
+                       "#s(hash-table size 65 test eql rehash-size 1.5 rehash-threshold 0.8125 data (foo \"bar\"))")))
+
 (ert-deftest test-persist-load ()
   (with-local-temp-persist
    (let ((sym (cl-gensym)))
-- 
2.40.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: 0004-Add-persist-copy-tree.patch --]
[-- Type: text/x-diff, Size: 1912 bytes --]

From ccc1be590b6c7b71aaa4ee3dd3bf25184ea7c122 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Tue, 23 May 2023 13:43:02 -0700
Subject: [PATCH 4/5] Add persist-copy-tree

The behavior of copy-tree was changed in Emacs 30. This function will
ensure that persist works correctly for previous Emacs versions.
---
 persist.el | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/persist.el b/persist.el
index 725f2f71cf..eb408b5dca 100644
--- a/persist.el
+++ b/persist.el
@@ -210,5 +210,34 @@ (defun persist-hash-equal (hash1 hash2)
                              hash1)
               t)))
 
+
+(defun persist-copy-tree (tree &optional vectors-and-records)
+  "Make a copy of TREE.
+If TREE is a cons cell, this recursively copies both its car and its cdr.
+Contrast to `copy-sequence', which copies only along the cdrs.
+With the second argument VECTORS-AND-RECORDS non-nil, this
+traverses and copies vectors and records as well as conses."
+  (declare (side-effect-free error-free))
+  (if (consp tree)
+      (let (result)
+	(while (consp tree)
+	  (let ((newcar (car tree)))
+	    (if (or (consp (car tree))
+                    (and vectors-and-records
+                         (or (vectorp (car tree)) (recordp (car tree)))))
+		(setq newcar (copy-tree (car tree) vectors-and-records)))
+	    (push newcar result))
+	  (setq tree (cdr tree)))
+	(nconc (nreverse result)
+               (if (and vectors-and-records (or (vectorp tree) (recordp tree)))
+                   (copy-tree tree vectors-and-records)
+                 tree)))
+    (if (and vectors-and-records (or (vectorp tree) (recordp tree)))
+	(let ((i (length (setq tree (copy-sequence tree)))))
+	  (while (>= (setq i (1- i)) 0)
+	    (aset tree i (copy-tree (aref tree i) vectors-and-records)))
+	  tree)
+      tree)))
+
 (provide 'persist)
 ;;; persist.el ends here
-- 
2.40.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #6: 0005-Make-persist-defvar-work-with-records.patch --]
[-- Type: text/x-diff, Size: 1763 bytes --]

From 4658bde147253d2f070b14c6f54300f640da063e Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Tue, 23 May 2023 13:44:40 -0700
Subject: [PATCH 5/5] Make persist-defvar work with records

Previously, when persist-defvar received a record for an initial
value, updated values were not persisted.
---
 persist.el            | 2 +-
 test/persist-tests.el | 8 ++++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/persist.el b/persist.el
index eb408b5dca..39ce54bbf0 100644
--- a/persist.el
+++ b/persist.el
@@ -120,7 +120,7 @@ (defun persist-symbol (symbol &optional initvalue)
     (put symbol 'persist t)
     (if (hash-table-p initvalue)
         (put symbol 'persist-default (copy-hash-table initvalue))
-      (put symbol 'persist-default initvalue))))
+      (put symbol 'persist-default (persist-copy-tree initvalue t)))))
 
 (defun persist--persistant-p (symbol)
   "Return non-nil if SYMBOL is a persistant variable."
diff --git a/test/persist-tests.el b/test/persist-tests.el
index 8a30a24e23..18b8af2b89 100644
--- a/test/persist-tests.el
+++ b/test/persist-tests.el
@@ -69,6 +69,14 @@ (ert-deftest test-persist-save-hash ()
                        (puthash 'foo "bar" (symbol-value sym))
                        "#s(hash-table size 65 test eql rehash-size 1.5 rehash-threshold 0.8125 data (foo \"bar\"))")))
 
+(ert-deftest test-persist-save-record ()
+  "Test saving record."
+  (let* ((rec (record 'foo 'a 'b))
+         (default (copy-sequence rec)))
+    (test-persist-save rec default
+                       (setf (aref (symbol-value sym) 2) 'quux)
+                       "#s(foo a quux)")))
+
 (ert-deftest test-persist-load ()
   (with-local-temp-persist
    (let ((sym (cl-gensym)))
-- 
2.40.1

bug#63513: [PATCH] Make persist-defvar work with records and hash tables

[Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 60 bytes --]

I fixed the definition of persist-hash-equal, and rebased.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-test-persist-save-macro.patch --]
[-- Type: text/x-diff, Size: 3379 bytes --]

From 6ec3955a521266e0661bbd0b2a6d1984875ae1a1 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Tue, 23 May 2023 12:57:02 -0700
Subject: [PATCH 1/5] Add test-persist-save macro

---
 test/persist-tests.el | 76 +++++++++++++++++++------------------------
 1 file changed, 33 insertions(+), 43 deletions(-)

diff --git a/test/persist-tests.el b/test/persist-tests.el
index b6645a9297..0a85b78767 100644
--- a/test/persist-tests.el
+++ b/test/persist-tests.el
@@ -25,51 +25,41 @@ (ert-deftest test-persist-save-only-persistant ()
    :type 'error
    :exclude-subtypes t))
 
-(ert-deftest test-persist-save ()
-  (with-local-temp-persist
-   (let ((sym (cl-gensym)))
-     ;; precondition
-   (should-not (file-exists-p (persist--file-location sym)))
-     (set sym 10)
-     (persist-symbol sym 10)
-     (persist-save sym)
-     (should t)
-     (should-not (file-exists-p (persist--file-location sym)))
-     (set sym 20)
-     (persist-save sym)
-     (should (file-exists-p (persist--file-location sym)))
-     (should
-      (string-match-p
-       "20"
-       (with-temp-buffer
-         (insert-file-contents (persist--file-location sym))
-         (buffer-string))))
-     (set sym 10)
-     (persist-save sym)
-     (should-not (file-exists-p (persist--file-location sym)))
-     (should-error
-      (persist-save 'fred)))))
+(defmacro test-persist-save (init default change printed-changed)
+  "Test persisting symbols.
+- symbol is not persisted when value is set to INIT and default
+  value is set to DEFAULT.
+- symbol is persisted when value is changed according to CHANGE.
+- persisted file contents match PRINTED-CHANGED.
+- symbol is not persisted after value is set back to DEFAULT."
+  `(with-local-temp-persist
+    (let ((sym (cl-gensym)))
+      (should-not (file-exists-p (persist--file-location sym)))
+      (set sym ,init)
+      (persist-symbol sym ,default)
+      (persist-save sym)
+      (should t)
+      (should-not (file-exists-p (persist--file-location sym)))
+      ,change
+      (persist-save sym)
+      (should (file-exists-p (persist--file-location sym)))
+      (should
+       (string-match-p
+        ,printed-changed
+        (with-temp-buffer
+          (insert-file-contents (persist--file-location sym))
+          (buffer-string))))
+      (set sym ,default)
+      (persist-save sym)
+      (should-not (file-exists-p (persist--file-location sym))))))
 
-(ert-deftest test-persist-save-non-number ()
-  "Test saving something that is not a number.
+(ert-deftest test-persist-save-number ()
+  "Test saving number."
+  (test-persist-save 1 1 (set sym 2) "2"))
 
-`test-persist-save' missed "
-  (with-local-temp-persist
-   (let ((sym (cl-gensym)))
-     (set sym "fred")
-     (persist-symbol sym "fred")
-     (persist-save sym)
-     (should t)
-     (should-not (file-exists-p (persist--file-location sym)))
-     (set sym "george")
-     (persist-save sym)
-     (should (file-exists-p (persist--file-location sym)))
-     (should
-      (string-match-p
-       "george"
-       (with-temp-buffer
-         (insert-file-contents (persist--file-location sym))
-         (buffer-string)))))))
+(ert-deftest test-persist-save-string ()
+  "Test saving string."
+  (test-persist-save "foo" "foo" (set sym "bar") "bar"))
 
 (ert-deftest test-persist-load ()
   (with-local-temp-persist
-- 
2.41.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Add-persist-hash-equal.patch --]
[-- Type: text/x-diff, Size: 1475 bytes --]

From 1f6fae3a9799dbb58b742095480917c2e3b99a7f Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Sat, 2 Sep 2023 16:52:31 -0700
Subject: [PATCH 2/5] Add persist-hash-equal

See bug#63671.
---
 persist.el | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/persist.el b/persist.el
index d80943d19e..fd0d750161 100644
--- a/persist.el
+++ b/persist.el
@@ -187,5 +187,22 @@ (defun persist--save-all ()
 (add-hook 'kill-emacs-hook
           'persist--save-all)
 
+(defun persist-hash-equal (hash1 hash2)
+  "Return non-nil when the contents of HASH1 and HASH2 are equal.
+Table values are compared using `equal' unless they are both hash
+tables themselves, in which case `persist-hash-equal' is used.
+Does not compare equality predicates."
+  (and (= (hash-table-count hash1)
+          (hash-table-count hash2))
+       (catch 'flag (maphash (lambda (key hash1-value)
+                               (let ((hash2-value (gethash key hash2)))
+                                 (or (if (and (hash-table-p hash1-value)
+                                              (hash-table-p hash2-value))
+                                         (persist-hash-equal hash1-value hash2-value)
+                                       (equal hash1-value hash2-value))
+                                     (throw 'flag nil))))
+                             hash1)
+              t)))
+
 (provide 'persist)
 ;;; persist.el ends here
-- 
2.41.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-Make-persist-defvar-work-with-hash-tables.patch --]
[-- Type: text/x-diff, Size: 3100 bytes --]

From ae35177b30bf1fabbc56ebf315b25a6074102201 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Tue, 23 May 2023 13:09:29 -0700
Subject: [PATCH 3/5] Make persist-defvar work with hash tables

Previously, when persist-defvar received a hash table for an initial
value, updated values were not persisted.
---
 persist.el            | 17 ++++++++++++-----
 test/persist-tests.el |  8 ++++++++
 2 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/persist.el b/persist.el
index fd0d750161..9a0ee33a65 100644
--- a/persist.el
+++ b/persist.el
@@ -118,7 +118,9 @@ (defun persist-symbol (symbol &optional initvalue)
   (let ((initvalue (or initvalue (symbol-value symbol))))
     (add-to-list 'persist--symbols symbol)
     (put symbol 'persist t)
-    (put symbol 'persist-default initvalue)))
+    (if (hash-table-p initvalue)
+        (put symbol 'persist-default (copy-hash-table initvalue))
+      (put symbol 'persist-default initvalue))))
 
 (defun persist--persistant-p (symbol)
   "Return non-nil if SYMBOL is a persistant variable."
@@ -132,9 +134,14 @@ (defun persist-save (symbol)
   (unless (persist--persistant-p symbol)
     (error (format
             "Symbol %s is not persistant" symbol)))
-  (let ((symbol-file-loc (persist--file-location symbol)))
-    (if (equal (symbol-value symbol)
-               (persist-default symbol))
+  (let* ((symbol-file-loc (persist--file-location symbol))
+         (value (symbol-value symbol))
+         (default (persist-default symbol))
+         (value-unchanged-p (if (and (hash-table-p value)
+                                     (hash-table-p default))
+                                (persist-hash-equal value default)
+                              (equal value default))))
+    (if value-unchanged-p
         (when (file-exists-p symbol-file-loc)
           (delete-file symbol-file-loc))
       (let ((dir-loc
@@ -148,7 +155,7 @@ (defun persist-save (symbol)
                 (print-escape-control-characters t)
                 (print-escape-nonascii t)
                 (print-circle t))
-            (print (symbol-value symbol) (current-buffer)))
+            (print value (current-buffer)))
           (write-region (point-min) (point-max)
                         symbol-file-loc
                         nil 'quiet))))))
diff --git a/test/persist-tests.el b/test/persist-tests.el
index 0a85b78767..8a30a24e23 100644
--- a/test/persist-tests.el
+++ b/test/persist-tests.el
@@ -61,6 +61,14 @@ (ert-deftest test-persist-save-string ()
   "Test saving string."
   (test-persist-save "foo" "foo" (set sym "bar") "bar"))
 
+(ert-deftest test-persist-save-hash ()
+  "Test saving hash table."
+  (let* ((hash (make-hash-table))
+         (default (copy-hash-table hash)))
+    (test-persist-save hash default
+                       (puthash 'foo "bar" (symbol-value sym))
+                       "#s(hash-table size 65 test eql rehash-size 1.5 rehash-threshold 0.8125 data (foo \"bar\"))")))
+
 (ert-deftest test-persist-load ()
   (with-local-temp-persist
    (let ((sym (cl-gensym)))
-- 
2.41.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: 0004-Add-persist-copy-tree.patch --]
[-- Type: text/x-diff, Size: 1909 bytes --]

From 1b06fc7efb7339c390170e2bc26e298153221f2e Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Tue, 23 May 2023 13:43:02 -0700
Subject: [PATCH 4/5] Add persist-copy-tree

The behavior of copy-tree was changed in Emacs 30. This function will
ensure that persist works correctly for previous Emacs versions.
---
 persist.el | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/persist.el b/persist.el
index 9a0ee33a65..8eb0e7ba51 100644
--- a/persist.el
+++ b/persist.el
@@ -211,5 +211,33 @@ (defun persist-hash-equal (hash1 hash2)
                              hash1)
               t)))
 
+(defun persist-copy-tree (tree &optional vectors-and-records)
+  "Make a copy of TREE.
+If TREE is a cons cell, this recursively copies both its car and its cdr.
+Contrast to `copy-sequence', which copies only along the cdrs.
+With the second argument VECTORS-AND-RECORDS non-nil, this
+traverses and copies vectors and records as well as conses."
+  (declare (side-effect-free error-free))
+  (if (consp tree)
+      (let (result)
+	(while (consp tree)
+	  (let ((newcar (car tree)))
+	    (if (or (consp (car tree))
+                    (and vectors-and-records
+                         (or (vectorp (car tree)) (recordp (car tree)))))
+		(setq newcar (copy-tree (car tree) vectors-and-records)))
+	    (push newcar result))
+	  (setq tree (cdr tree)))
+	(nconc (nreverse result)
+               (if (and vectors-and-records (or (vectorp tree) (recordp tree)))
+                   (copy-tree tree vectors-and-records)
+                 tree)))
+    (if (and vectors-and-records (or (vectorp tree) (recordp tree)))
+	(let ((i (length (setq tree (copy-sequence tree)))))
+	  (while (>= (setq i (1- i)) 0)
+	    (aset tree i (copy-tree (aref tree i) vectors-and-records)))
+	  tree)
+      tree)))
+
 (provide 'persist)
 ;;; persist.el ends here
-- 
2.41.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #6: 0005-Make-persist-defvar-work-with-records.patch --]
[-- Type: text/x-diff, Size: 1763 bytes --]

From afd6ea405efb1cba0ad1b1601f4af8efc796c1d7 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Tue, 23 May 2023 13:44:40 -0700
Subject: [PATCH 5/5] Make persist-defvar work with records

Previously, when persist-defvar received a record for an initial
value, updated values were not persisted.
---
 persist.el            | 2 +-
 test/persist-tests.el | 8 ++++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/persist.el b/persist.el
index 8eb0e7ba51..2f3885f80e 100644
--- a/persist.el
+++ b/persist.el
@@ -120,7 +120,7 @@ (defun persist-symbol (symbol &optional initvalue)
     (put symbol 'persist t)
     (if (hash-table-p initvalue)
         (put symbol 'persist-default (copy-hash-table initvalue))
-      (put symbol 'persist-default initvalue))))
+      (put symbol 'persist-default (persist-copy-tree initvalue t)))))
 
 (defun persist--persistant-p (symbol)
   "Return non-nil if SYMBOL is a persistant variable."
diff --git a/test/persist-tests.el b/test/persist-tests.el
index 8a30a24e23..18b8af2b89 100644
--- a/test/persist-tests.el
+++ b/test/persist-tests.el
@@ -69,6 +69,14 @@ (ert-deftest test-persist-save-hash ()
                        (puthash 'foo "bar" (symbol-value sym))
                        "#s(hash-table size 65 test eql rehash-size 1.5 rehash-threshold 0.8125 data (foo \"bar\"))")))
 
+(ert-deftest test-persist-save-record ()
+  "Test saving record."
+  (let* ((rec (record 'foo 'a 'b))
+         (default (copy-sequence rec)))
+    (test-persist-save rec default
+                       (setf (aref (symbol-value sym) 2) 'quux)
+                       "#s(foo a quux)")))
+
 (ert-deftest test-persist-load ()
   (with-local-temp-persist
    (let ((sym (cl-gensym)))
-- 
2.41.0

bug#63513: [PATCH] Make persist-defvar work with records and hash tables

[Eli Zaretskii]

> From: Joseph Turner <joseph@breatheoutbreathe.in>
> Date: Sat, 02 Sep 2023 16:54:00 -0700
> 
> I fixed the definition of persist-hash-equal, and rebased.

Thanks, but this lacks the commit log messages (see CONTRIBUTE).

Also, I think the new features warrant a NEWS entry.

bug#63513: [PATCH] Make persist-defvar work with records and hash tables

[Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 366 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

> Thanks, but this lacks the commit log messages (see CONTRIBUTE).

I have added commit log messages. See attached patches.

> Also, I think the new features warrant a NEWS entry.

Should that go in the NEWS file in the main Emacs repo? I don't see a
NEWS file in either of the main or externals/persist ELPA branches.

Joseph


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-test-persist-save-macro.patch --]
[-- Type: text/x-diff, Size: 3629 bytes --]

From 2ca778a44c10f11059f16ef5922cf1eff9118105 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Tue, 23 May 2023 12:57:02 -0700
Subject: [PATCH 1/5] Add test-persist-save macro

* test/persist-tests.el (persist-test-persist-save):
Consolidate logic of test-persist-save and
test-persist-save-non-number.
(test-persist-save-number test-persist-save-string): Simple wrappers
over persist-test-persist-save
---
 test/persist-tests.el | 76 +++++++++++++++++++------------------------
 1 file changed, 33 insertions(+), 43 deletions(-)

diff --git a/test/persist-tests.el b/test/persist-tests.el
index b6645a9297..f2b04769ef 100644
--- a/test/persist-tests.el
+++ b/test/persist-tests.el
@@ -25,51 +25,41 @@ (ert-deftest test-persist-save-only-persistant ()
    :type 'error
    :exclude-subtypes t))
 
-(ert-deftest test-persist-save ()
-  (with-local-temp-persist
-   (let ((sym (cl-gensym)))
-     ;; precondition
-   (should-not (file-exists-p (persist--file-location sym)))
-     (set sym 10)
-     (persist-symbol sym 10)
-     (persist-save sym)
-     (should t)
-     (should-not (file-exists-p (persist--file-location sym)))
-     (set sym 20)
-     (persist-save sym)
-     (should (file-exists-p (persist--file-location sym)))
-     (should
-      (string-match-p
-       "20"
-       (with-temp-buffer
-         (insert-file-contents (persist--file-location sym))
-         (buffer-string))))
-     (set sym 10)
-     (persist-save sym)
-     (should-not (file-exists-p (persist--file-location sym)))
-     (should-error
-      (persist-save 'fred)))))
+(defmacro persist-test-persist-save (init default change printed-changed)
+  "Test persisting symbols.
+- symbol is not persisted when value is set to INIT and default
+  value is set to DEFAULT.
+- symbol is persisted when value is changed according to CHANGE.
+- persisted file contents match PRINTED-CHANGED.
+- symbol is not persisted after value is set back to DEFAULT."
+  `(with-local-temp-persist
+    (let ((sym (cl-gensym)))
+      (should-not (file-exists-p (persist--file-location sym)))
+      (set sym ,init)
+      (persist-symbol sym ,default)
+      (persist-save sym)
+      (should t)
+      (should-not (file-exists-p (persist--file-location sym)))
+      ,change
+      (persist-save sym)
+      (should (file-exists-p (persist--file-location sym)))
+      (should
+       (string-match-p
+        ,printed-changed
+        (with-temp-buffer
+          (insert-file-contents (persist--file-location sym))
+          (buffer-string))))
+      (set sym ,default)
+      (persist-save sym)
+      (should-not (file-exists-p (persist--file-location sym))))))
 
-(ert-deftest test-persist-save-non-number ()
-  "Test saving something that is not a number.
+(ert-deftest test-persist-save-number ()
+  "Test saving number."
+  (persist-test-persist-save 1 1 (set sym 2) "2"))
 
-`test-persist-save' missed "
-  (with-local-temp-persist
-   (let ((sym (cl-gensym)))
-     (set sym "fred")
-     (persist-symbol sym "fred")
-     (persist-save sym)
-     (should t)
-     (should-not (file-exists-p (persist--file-location sym)))
-     (set sym "george")
-     (persist-save sym)
-     (should (file-exists-p (persist--file-location sym)))
-     (should
-      (string-match-p
-       "george"
-       (with-temp-buffer
-         (insert-file-contents (persist--file-location sym))
-         (buffer-string)))))))
+(ert-deftest test-persist-save-string ()
+  "Test saving string."
+  (persist-test-persist-save "foo" "foo" (set sym "bar") "bar"))
 
 (ert-deftest test-persist-load ()
   (with-local-temp-persist
-- 
2.41.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Add-persist-equal.patch --]
[-- Type: text/x-diff, Size: 1394 bytes --]

From b379d8d1779e0190541ef7f8adf39dfe4d4c551a Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Sat, 2 Sep 2023 16:52:31 -0700
Subject: [PATCH 2/5] Add persist-equal

* persist.el (persist-hash-equal): Like equal, but compares hash
tables also.
See bug#63671 for a discussion of built-in hash table comparison.
---
 persist.el | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/persist.el b/persist.el
index d80943d19e..a707d038cd 100644
--- a/persist.el
+++ b/persist.el
@@ -187,5 +187,25 @@ (defun persist--save-all ()
 (add-hook 'kill-emacs-hook
           'persist--save-all)
 
+(defun persist-equal (a b)
+  "Return non-nil when the values of A and B are equal.
+A and B are compared using `equal' unless they are both hash
+tables. In that case, the following are compared:
+
+- hash table count
+- hash table predicate
+- values, using `persist-equal'"
+  (if (and (hash-table-p a) (hash-table-p b))
+      (and (= (hash-table-count a) (hash-table-count b))
+           (eq (hash-table-test a) (hash-table-test b))
+           (catch 'done
+             (maphash
+              (lambda (key a-value)
+                (unless (persist-equal a-value (gethash key b (not a-value)))
+                  (throw 'done nil)))
+              a)
+             t))
+    (equal a b)))
+
 (provide 'persist)
 ;;; persist.el ends here
-- 
2.41.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-Make-persist-defvar-work-with-hash-tables.patch --]
[-- Type: text/x-diff, Size: 2645 bytes --]

From 11194569423dcdf8778a55f59dbca8f49e8b7b37 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Sun, 3 Sep 2023 17:10:38 -0700
Subject: [PATCH 3/5] Make persist-defvar work with hash tables

Previously, when persist-defvar received a hash table for an initial
value, updated values were not persisted.

* persist.el (persist-symbol): Use hash table copy as default so that
the original table can be modified without modifying the default value.
(persist-save): Use persist-equal to ensure that hash tables are
correctly compared
* test/persist-tests.el (test-persist-save-hash): Test hash tables
persistence.
---
 persist.el            | 8 +++++---
 test/persist-tests.el | 8 ++++++++
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/persist.el b/persist.el
index a707d038cd..7b2ab491d7 100644
--- a/persist.el
+++ b/persist.el
@@ -118,7 +118,9 @@ (defun persist-symbol (symbol &optional initvalue)
   (let ((initvalue (or initvalue (symbol-value symbol))))
     (add-to-list 'persist--symbols symbol)
     (put symbol 'persist t)
-    (put symbol 'persist-default initvalue)))
+    (if (hash-table-p initvalue)
+        (put symbol 'persist-default (copy-hash-table initvalue))
+      (put symbol 'persist-default initvalue))))
 
 (defun persist--persistant-p (symbol)
   "Return non-nil if SYMBOL is a persistant variable."
@@ -133,8 +135,8 @@ (defun persist-save (symbol)
     (error (format
             "Symbol %s is not persistant" symbol)))
   (let ((symbol-file-loc (persist--file-location symbol)))
-    (if (equal (symbol-value symbol)
-               (persist-default symbol))
+    (if (persist-equal (symbol-value symbol)
+                       (persist-default symbol))
         (when (file-exists-p symbol-file-loc)
           (delete-file symbol-file-loc))
       (let ((dir-loc
diff --git a/test/persist-tests.el b/test/persist-tests.el
index f2b04769ef..90adf1c6d6 100644
--- a/test/persist-tests.el
+++ b/test/persist-tests.el
@@ -61,6 +61,14 @@ (ert-deftest test-persist-save-string ()
   "Test saving string."
   (persist-test-persist-save "foo" "foo" (set sym "bar") "bar"))
 
+(ert-deftest test-persist-save-hash ()
+  "Test saving hash table."
+  (let* ((hash (make-hash-table))
+         (default (copy-hash-table hash)))
+    (persist-test-persist-save hash default
+                               (puthash 'foo "bar" (symbol-value sym))
+                               "#s(hash-table size 65 test eql rehash-size 1.5 rehash-threshold 0.8125 data (foo \"bar\"))")))
+
 (ert-deftest test-persist-load ()
   (with-local-temp-persist
    (let ((sym (cl-gensym)))
-- 
2.41.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: 0004-Add-persist-copy-tree.patch --]
[-- Type: text/x-diff, Size: 1983 bytes --]

From f360f7ae53125a847c2a8d5762ca5f08d16445b9 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Tue, 23 May 2023 13:43:02 -0700
Subject: [PATCH 4/5] Add persist-copy-tree

The behavior of copy-tree was changed in Emacs 30. This function will
ensure that persist works correctly for previous Emacs versions.

* persist.el (persist-copy-tree): Add copy-tree, so that records can
be compared.
---
 persist.el | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/persist.el b/persist.el
index 7b2ab491d7..93444995f2 100644
--- a/persist.el
+++ b/persist.el
@@ -209,5 +209,33 @@ (defun persist-equal (a b)
              t))
     (equal a b)))
 
+(defun persist-copy-tree (tree &optional vectors-and-records)
+  "Make a copy of TREE.
+If TREE is a cons cell, this recursively copies both its car and its cdr.
+Contrast to `copy-sequence', which copies only along the cdrs.
+With the second argument VECTORS-AND-RECORDS non-nil, this
+traverses and copies vectors and records as well as conses."
+  (declare (side-effect-free error-free))
+  (if (consp tree)
+      (let (result)
+	(while (consp tree)
+	  (let ((newcar (car tree)))
+	    (if (or (consp (car tree))
+                    (and vectors-and-records
+                         (or (vectorp (car tree)) (recordp (car tree)))))
+		(setq newcar (persist-copy-tree (car tree) vectors-and-records)))
+	    (push newcar result))
+	  (setq tree (cdr tree)))
+	(nconc (nreverse result)
+               (if (and vectors-and-records (or (vectorp tree) (recordp tree)))
+                   (persist-copy-tree tree vectors-and-records)
+                 tree)))
+    (if (and vectors-and-records (or (vectorp tree) (recordp tree)))
+	(let ((i (length (setq tree (copy-sequence tree)))))
+	  (while (>= (setq i (1- i)) 0)
+	    (aset tree i (persist-copy-tree (aref tree i) vectors-and-records)))
+	  tree)
+      tree)))
+
 (provide 'persist)
 ;;; persist.el ends here
-- 
2.41.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #6: 0005-Make-persist-defvar-work-with-records.patch --]
[-- Type: text/x-diff, Size: 2019 bytes --]

From ea57e6205b10678b9c26f7dcf3704e2a7acb25a7 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Tue, 23 May 2023 13:44:40 -0700
Subject: [PATCH 5/5] Make persist-defvar work with records

Previously, when persist-defvar received a record for an initial
value, updated values were not persisted.

* persist.el (persist-symbol): Set default to a copy of initvalue so
when initvalue is a record, the original can be modified without
modifying the default.
* test/persist-tests.el: Test persist-save with a record.
---
 persist.el            | 2 +-
 test/persist-tests.el | 8 ++++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/persist.el b/persist.el
index 93444995f2..e43171459e 100644
--- a/persist.el
+++ b/persist.el
@@ -120,7 +120,7 @@ (defun persist-symbol (symbol &optional initvalue)
     (put symbol 'persist t)
     (if (hash-table-p initvalue)
         (put symbol 'persist-default (copy-hash-table initvalue))
-      (put symbol 'persist-default initvalue))))
+      (put symbol 'persist-default (persist-copy-tree initvalue t)))))
 
 (defun persist--persistant-p (symbol)
   "Return non-nil if SYMBOL is a persistant variable."
diff --git a/test/persist-tests.el b/test/persist-tests.el
index 90adf1c6d6..62d8501493 100644
--- a/test/persist-tests.el
+++ b/test/persist-tests.el
@@ -69,6 +69,14 @@ (ert-deftest test-persist-save-hash ()
                                (puthash 'foo "bar" (symbol-value sym))
                                "#s(hash-table size 65 test eql rehash-size 1.5 rehash-threshold 0.8125 data (foo \"bar\"))")))
 
+(ert-deftest test-persist-save-record ()
+  "Test saving record."
+  (let* ((rec (record 'foo 'a 'b))
+         (default (copy-sequence rec)))
+    (persist-test-persist-save rec default
+                               (setf (aref (symbol-value sym) 2) 'quux)
+                               "#s(foo a quux)")))
+
 (ert-deftest test-persist-load ()
   (with-local-temp-persist
    (let ((sym (cl-gensym)))
-- 
2.41.0

bug#63513: [PATCH] Make persist-defvar work with records and hash tables

[Eli Zaretskii]

> From: Joseph Turner <joseph@breatheoutbreathe.in>
> Cc: phillip.lord@russet.org.uk, monnier@iro.umontreal.ca,
>  63513@debbugs.gnu.org, Adam Porter <adam.porter@47ap.net>
> Date: Sun, 03 Sep 2023 17:29:22 -0700
> 
> > Also, I think the new features warrant a NEWS entry.
> 
> Should that go in the NEWS file in the main Emacs repo? I don't see a
> NEWS file in either of the main or externals/persist ELPA branches.

I'm terribly sorry: I haven't realized this is for ELPA, I thought it
was for emacs.git.  My bad Please ignore what I wrote in my previous
message.

bug#63513: [PATCH] Make persist-defvar work with records and hash tables

[Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

On September 4, 2023 4:33:55 AM PDT, Eli Zaretskii <eliz@gnu.org> wrote:
>> From: Joseph Turner <joseph@breatheoutbreathe.in>
>> Cc: phillip.lord@russet.org.uk, monnier@iro.umontreal.ca,
>>  63513@debbugs.gnu.org, Adam Porter <adam.porter@47ap.net>
>> Date: Sun, 03 Sep 2023 17:29:22 -0700
>> 
>> > Also, I think the new features warrant a NEWS entry.
>> 
>> Should that go in the NEWS file in the main Emacs repo? I don't see a
>> NEWS file in either of the main or externals/persist ELPA branches.
>
>I'm terribly sorry: I haven't realized this is for ELPA, I thought it
>was for emacs.git.  My bad Please ignore what I wrote in my previous
>message.

No problem! It gave me a chance to do a final editing pass. What's the next step? I would be glad for Phillip's review, if possible.

bug#63513: [PATCH] Make persist-defvar work with records and hash tables

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Joseph Turner [2023-09-04 08:57:13] wrote:
> On September 4, 2023 4:33:55 AM PDT, Eli Zaretskii <eliz@gnu.org> wrote:
>>> From: Joseph Turner <joseph@breatheoutbreathe.in>
>>> Cc: phillip.lord@russet.org.uk, monnier@iro.umontreal.ca,
>>>  63513@debbugs.gnu.org, Adam Porter <adam.porter@47ap.net>
>>> Date: Sun, 03 Sep 2023 17:29:22 -0700
>>> 
>>> > Also, I think the new features warrant a NEWS entry.
>>> 
>>> Should that go in the NEWS file in the main Emacs repo? I don't see a
>>> NEWS file in either of the main or externals/persist ELPA branches.
>>
>>I'm terribly sorry: I haven't realized this is for ELPA, I thought it
>>was for emacs.git.  My bad Please ignore what I wrote in my previous
>>message.
>
> No problem! It gave me a chance to do a final editing pass. What's the next
> step? I would be glad for Phillip's review, if possible.

I was about to ping Phil over on https://gitlab.com/phillord/persist/
but was reminded along the way that Phil gave me write access to
that repository.

Could you send me the result of your final editing pass?


        Stefan

bug#63513: [PATCH] Make persist-defvar work with records and hash tables

[Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 285 bytes --]


Stefan Monnier <monnier@iro.umontreal.ca> writes:
> I was about to ping Phil over on https://gitlab.com/phillord/persist/
> but was reminded along the way that Phil gave me write access to
> that repository.
>
> Could you send me the result of your final editing pass?

Here you go!


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-test-persist-save-macro.patch --]
[-- Type: text/x-diff, Size: 3629 bytes --]

From 2ca778a44c10f11059f16ef5922cf1eff9118105 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Tue, 23 May 2023 12:57:02 -0700
Subject: [PATCH 1/5] Add test-persist-save macro

* test/persist-tests.el (persist-test-persist-save):
Consolidate logic of test-persist-save and
test-persist-save-non-number.
(test-persist-save-number test-persist-save-string): Simple wrappers
over persist-test-persist-save
---
 test/persist-tests.el | 76 +++++++++++++++++++------------------------
 1 file changed, 33 insertions(+), 43 deletions(-)

diff --git a/test/persist-tests.el b/test/persist-tests.el
index b6645a9297..f2b04769ef 100644
--- a/test/persist-tests.el
+++ b/test/persist-tests.el
@@ -25,51 +25,41 @@ (ert-deftest test-persist-save-only-persistant ()
    :type 'error
    :exclude-subtypes t))
 
-(ert-deftest test-persist-save ()
-  (with-local-temp-persist
-   (let ((sym (cl-gensym)))
-     ;; precondition
-   (should-not (file-exists-p (persist--file-location sym)))
-     (set sym 10)
-     (persist-symbol sym 10)
-     (persist-save sym)
-     (should t)
-     (should-not (file-exists-p (persist--file-location sym)))
-     (set sym 20)
-     (persist-save sym)
-     (should (file-exists-p (persist--file-location sym)))
-     (should
-      (string-match-p
-       "20"
-       (with-temp-buffer
-         (insert-file-contents (persist--file-location sym))
-         (buffer-string))))
-     (set sym 10)
-     (persist-save sym)
-     (should-not (file-exists-p (persist--file-location sym)))
-     (should-error
-      (persist-save 'fred)))))
+(defmacro persist-test-persist-save (init default change printed-changed)
+  "Test persisting symbols.
+- symbol is not persisted when value is set to INIT and default
+  value is set to DEFAULT.
+- symbol is persisted when value is changed according to CHANGE.
+- persisted file contents match PRINTED-CHANGED.
+- symbol is not persisted after value is set back to DEFAULT."
+  `(with-local-temp-persist
+    (let ((sym (cl-gensym)))
+      (should-not (file-exists-p (persist--file-location sym)))
+      (set sym ,init)
+      (persist-symbol sym ,default)
+      (persist-save sym)
+      (should t)
+      (should-not (file-exists-p (persist--file-location sym)))
+      ,change
+      (persist-save sym)
+      (should (file-exists-p (persist--file-location sym)))
+      (should
+       (string-match-p
+        ,printed-changed
+        (with-temp-buffer
+          (insert-file-contents (persist--file-location sym))
+          (buffer-string))))
+      (set sym ,default)
+      (persist-save sym)
+      (should-not (file-exists-p (persist--file-location sym))))))
 
-(ert-deftest test-persist-save-non-number ()
-  "Test saving something that is not a number.
+(ert-deftest test-persist-save-number ()
+  "Test saving number."
+  (persist-test-persist-save 1 1 (set sym 2) "2"))
 
-`test-persist-save' missed "
-  (with-local-temp-persist
-   (let ((sym (cl-gensym)))
-     (set sym "fred")
-     (persist-symbol sym "fred")
-     (persist-save sym)
-     (should t)
-     (should-not (file-exists-p (persist--file-location sym)))
-     (set sym "george")
-     (persist-save sym)
-     (should (file-exists-p (persist--file-location sym)))
-     (should
-      (string-match-p
-       "george"
-       (with-temp-buffer
-         (insert-file-contents (persist--file-location sym))
-         (buffer-string)))))))
+(ert-deftest test-persist-save-string ()
+  "Test saving string."
+  (persist-test-persist-save "foo" "foo" (set sym "bar") "bar"))
 
 (ert-deftest test-persist-load ()
   (with-local-temp-persist
-- 
2.41.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Add-persist-equal.patch --]
[-- Type: text/x-diff, Size: 1394 bytes --]

From b379d8d1779e0190541ef7f8adf39dfe4d4c551a Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Sat, 2 Sep 2023 16:52:31 -0700
Subject: [PATCH 2/5] Add persist-equal

* persist.el (persist-hash-equal): Like equal, but compares hash
tables also.
See bug#63671 for a discussion of built-in hash table comparison.
---
 persist.el | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/persist.el b/persist.el
index d80943d19e..a707d038cd 100644
--- a/persist.el
+++ b/persist.el
@@ -187,5 +187,25 @@ (defun persist--save-all ()
 (add-hook 'kill-emacs-hook
           'persist--save-all)
 
+(defun persist-equal (a b)
+  "Return non-nil when the values of A and B are equal.
+A and B are compared using `equal' unless they are both hash
+tables. In that case, the following are compared:
+
+- hash table count
+- hash table predicate
+- values, using `persist-equal'"
+  (if (and (hash-table-p a) (hash-table-p b))
+      (and (= (hash-table-count a) (hash-table-count b))
+           (eq (hash-table-test a) (hash-table-test b))
+           (catch 'done
+             (maphash
+              (lambda (key a-value)
+                (unless (persist-equal a-value (gethash key b (not a-value)))
+                  (throw 'done nil)))
+              a)
+             t))
+    (equal a b)))
+
 (provide 'persist)
 ;;; persist.el ends here
-- 
2.41.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-Make-persist-defvar-work-with-hash-tables.patch --]
[-- Type: text/x-diff, Size: 2645 bytes --]

From 11194569423dcdf8778a55f59dbca8f49e8b7b37 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Sun, 3 Sep 2023 17:10:38 -0700
Subject: [PATCH 3/5] Make persist-defvar work with hash tables

Previously, when persist-defvar received a hash table for an initial
value, updated values were not persisted.

* persist.el (persist-symbol): Use hash table copy as default so that
the original table can be modified without modifying the default value.
(persist-save): Use persist-equal to ensure that hash tables are
correctly compared
* test/persist-tests.el (test-persist-save-hash): Test hash tables
persistence.
---
 persist.el            | 8 +++++---
 test/persist-tests.el | 8 ++++++++
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/persist.el b/persist.el
index a707d038cd..7b2ab491d7 100644
--- a/persist.el
+++ b/persist.el
@@ -118,7 +118,9 @@ (defun persist-symbol (symbol &optional initvalue)
   (let ((initvalue (or initvalue (symbol-value symbol))))
     (add-to-list 'persist--symbols symbol)
     (put symbol 'persist t)
-    (put symbol 'persist-default initvalue)))
+    (if (hash-table-p initvalue)
+        (put symbol 'persist-default (copy-hash-table initvalue))
+      (put symbol 'persist-default initvalue))))
 
 (defun persist--persistant-p (symbol)
   "Return non-nil if SYMBOL is a persistant variable."
@@ -133,8 +135,8 @@ (defun persist-save (symbol)
     (error (format
             "Symbol %s is not persistant" symbol)))
   (let ((symbol-file-loc (persist--file-location symbol)))
-    (if (equal (symbol-value symbol)
-               (persist-default symbol))
+    (if (persist-equal (symbol-value symbol)
+                       (persist-default symbol))
         (when (file-exists-p symbol-file-loc)
           (delete-file symbol-file-loc))
       (let ((dir-loc
diff --git a/test/persist-tests.el b/test/persist-tests.el
index f2b04769ef..90adf1c6d6 100644
--- a/test/persist-tests.el
+++ b/test/persist-tests.el
@@ -61,6 +61,14 @@ (ert-deftest test-persist-save-string ()
   "Test saving string."
   (persist-test-persist-save "foo" "foo" (set sym "bar") "bar"))
 
+(ert-deftest test-persist-save-hash ()
+  "Test saving hash table."
+  (let* ((hash (make-hash-table))
+         (default (copy-hash-table hash)))
+    (persist-test-persist-save hash default
+                               (puthash 'foo "bar" (symbol-value sym))
+                               "#s(hash-table size 65 test eql rehash-size 1.5 rehash-threshold 0.8125 data (foo \"bar\"))")))
+
 (ert-deftest test-persist-load ()
   (with-local-temp-persist
    (let ((sym (cl-gensym)))
-- 
2.41.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: 0004-Add-persist-copy-tree.patch --]
[-- Type: text/x-diff, Size: 1983 bytes --]

From f360f7ae53125a847c2a8d5762ca5f08d16445b9 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Tue, 23 May 2023 13:43:02 -0700
Subject: [PATCH 4/5] Add persist-copy-tree

The behavior of copy-tree was changed in Emacs 30. This function will
ensure that persist works correctly for previous Emacs versions.

* persist.el (persist-copy-tree): Add copy-tree, so that records can
be compared.
---
 persist.el | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/persist.el b/persist.el
index 7b2ab491d7..93444995f2 100644
--- a/persist.el
+++ b/persist.el
@@ -209,5 +209,33 @@ (defun persist-equal (a b)
              t))
     (equal a b)))
 
+(defun persist-copy-tree (tree &optional vectors-and-records)
+  "Make a copy of TREE.
+If TREE is a cons cell, this recursively copies both its car and its cdr.
+Contrast to `copy-sequence', which copies only along the cdrs.
+With the second argument VECTORS-AND-RECORDS non-nil, this
+traverses and copies vectors and records as well as conses."
+  (declare (side-effect-free error-free))
+  (if (consp tree)
+      (let (result)
+	(while (consp tree)
+	  (let ((newcar (car tree)))
+	    (if (or (consp (car tree))
+                    (and vectors-and-records
+                         (or (vectorp (car tree)) (recordp (car tree)))))
+		(setq newcar (persist-copy-tree (car tree) vectors-and-records)))
+	    (push newcar result))
+	  (setq tree (cdr tree)))
+	(nconc (nreverse result)
+               (if (and vectors-and-records (or (vectorp tree) (recordp tree)))
+                   (persist-copy-tree tree vectors-and-records)
+                 tree)))
+    (if (and vectors-and-records (or (vectorp tree) (recordp tree)))
+	(let ((i (length (setq tree (copy-sequence tree)))))
+	  (while (>= (setq i (1- i)) 0)
+	    (aset tree i (persist-copy-tree (aref tree i) vectors-and-records)))
+	  tree)
+      tree)))
+
 (provide 'persist)
 ;;; persist.el ends here
-- 
2.41.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #6: 0005-Make-persist-defvar-work-with-records.patch --]
[-- Type: text/x-diff, Size: 2019 bytes --]

From ea57e6205b10678b9c26f7dcf3704e2a7acb25a7 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Tue, 23 May 2023 13:44:40 -0700
Subject: [PATCH 5/5] Make persist-defvar work with records

Previously, when persist-defvar received a record for an initial
value, updated values were not persisted.

* persist.el (persist-symbol): Set default to a copy of initvalue so
when initvalue is a record, the original can be modified without
modifying the default.
* test/persist-tests.el: Test persist-save with a record.
---
 persist.el            | 2 +-
 test/persist-tests.el | 8 ++++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/persist.el b/persist.el
index 93444995f2..e43171459e 100644
--- a/persist.el
+++ b/persist.el
@@ -120,7 +120,7 @@ (defun persist-symbol (symbol &optional initvalue)
     (put symbol 'persist t)
     (if (hash-table-p initvalue)
         (put symbol 'persist-default (copy-hash-table initvalue))
-      (put symbol 'persist-default initvalue))))
+      (put symbol 'persist-default (persist-copy-tree initvalue t)))))
 
 (defun persist--persistant-p (symbol)
   "Return non-nil if SYMBOL is a persistant variable."
diff --git a/test/persist-tests.el b/test/persist-tests.el
index 90adf1c6d6..62d8501493 100644
--- a/test/persist-tests.el
+++ b/test/persist-tests.el
@@ -69,6 +69,14 @@ (ert-deftest test-persist-save-hash ()
                                (puthash 'foo "bar" (symbol-value sym))
                                "#s(hash-table size 65 test eql rehash-size 1.5 rehash-threshold 0.8125 data (foo \"bar\"))")))
 
+(ert-deftest test-persist-save-record ()
+  "Test saving record."
+  (let* ((rec (record 'foo 'a 'b))
+         (default (copy-sequence rec)))
+    (persist-test-persist-save rec default
+                               (setf (aref (symbol-value sym) 2) 'quux)
+                               "#s(foo a quux)")))
+
 (ert-deftest test-persist-load ()
   (with-local-temp-persist
    (let ((sym (cl-gensym)))
-- 
2.41.0

bug#63513: [PATCH] Make persist-defvar work with records and hash tables

[phillip.lord]

On 2023-09-04 18:05, Stefan Monnier wrote:
> Joseph Turner [2023-09-04 08:57:13] wrote:
>> On September 4, 2023 4:33:55 AM PDT, Eli Zaretskii <eliz@gnu.org> 
>> wrote:
>>>> From: Joseph Turner <joseph@breatheoutbreathe.in>
>>>> Cc: phillip.lord@russet.org.uk, monnier@iro.umontreal.ca,
>>>>  63513@debbugs.gnu.org, Adam Porter <adam.porter@47ap.net>
>>>> Date: Sun, 03 Sep 2023 17:29:22 -0700
>>>> 
>>>> > Also, I think the new features warrant a NEWS entry.
>>>> 
>>>> Should that go in the NEWS file in the main Emacs repo? I don't see 
>>>> a
>>>> NEWS file in either of the main or externals/persist ELPA branches.
>>> 
>>> I'm terribly sorry: I haven't realized this is for ELPA, I thought it
>>> was for emacs.git.  My bad Please ignore what I wrote in my previous
>>> message.
>> 
>> No problem! It gave me a chance to do a final editing pass. What's the 
>> next
>> step? I would be glad for Phillip's review, if possible.
> 
> I was about to ping Phil over on https://gitlab.com/phillord/persist/
> but was reminded along the way that Phil gave me write access to
> that repository.
> 
> Could you send me the result of your final editing pass?


Indeed! I am afraid I am fairly unresponsive these days ("real life" 
intrudes), but I am more than happy for you to update there.

Phil

bug#63513: [PATCH] Make persist-defvar work with records and hash tables

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> Here you go!

Thanks, pushed!


        Stefan

bug#63513: [PATCH] Make persist-defvar work with records and hash tables

[Ihor Radchenko]

Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of
text editors" <bug-gnu-emacs@gnu.org> writes:

>> Here you go!
>
> Thanks, pushed!

May it be possible to promote `persist-hash-equal' and
`persist-copy-tree' to common subr.el functions?

The topic of comparing hash tables has been previously discussed in
https://yhetil.org/emacs-devel/871qvz4kdw.fsf@localhost/

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#63513: [PATCH] Make persist-defvar work with records and hash tables

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: Joseph Turner <joseph@breatheoutbreathe.in>, Adam Porter
>  <adam@alphapapa.net>, Eli Zaretskii <eliz@gnu.org>,
>  phillip.lord@russet.org.uk, 63513-done@debbugs.gnu.org
> Date: Fri, 08 Sep 2023 11:30:52 +0000
> 
> May it be possible to promote `persist-hash-equal' and
> `persist-copy-tree' to common subr.el functions?

Why do we need them in subr.el, i.e. preloaded?  Why cannot these
functions be autoloaded instead?

bug#63513: [PATCH] Make persist-defvar work with records and hash tables

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

>> May it be possible to promote `persist-hash-equal' and
>> `persist-copy-tree' to common subr.el functions?
>
> Why do we need them in subr.el, i.e. preloaded?  Why cannot these
> functions be autoloaded instead?

Similar functions - `string-equal-ignore-case' and `copy-tree' - are
already in subr.el. I do not see why the new functions discussed here
should be any different.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#63513: [PATCH] Make persist-defvar work with records and hash tables

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: monnier@iro.umontreal.ca, joseph@breatheoutbreathe.in,
>  adam@alphapapa.net, phillip.lord@russet.org.uk, 63513-done@debbugs.gnu.org
> Date: Fri, 08 Sep 2023 12:06:06 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> May it be possible to promote `persist-hash-equal' and
> >> `persist-copy-tree' to common subr.el functions?
> >
> > Why do we need them in subr.el, i.e. preloaded?  Why cannot these
> > functions be autoloaded instead?
> 
> Similar functions - `string-equal-ignore-case' and `copy-tree' - are
> already in subr.el. I do not see why the new functions discussed here
> should be any different.

We make the decision whether a function must be preloaded in a case by
case basis, to avoid making the memory footprint of an Emacs session
larger than it needs to be.  So arguments "by similarity" are not
useful in this case; you need to explain why you think these functions
are needed right from the startup.  Valid reasons include, among
others:

  . function is used by the startup code or during dumping
  . function is used by all or many important configurations
    immediately after startup

bug#63513: [PATCH] Make persist-defvar work with records and hash tables

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

>> > Why do we need them in subr.el, i.e. preloaded?  Why cannot these
>> > functions be autoloaded instead?
>> 
>> Similar functions - `string-equal-ignore-case' and `copy-tree' - are
>> already in subr.el. I do not see why the new functions discussed here
>> should be any different.
>
> We make the decision whether a function must be preloaded in a case by
> case basis, to avoid making the memory footprint of an Emacs session
> larger than it needs to be.  So arguments "by similarity" are not
> useful in this case; you need to explain why you think these functions
> are needed right from the startup.
> ...

Got it. Then, I do not insist on subr.el specifically. Autoloading is
fine.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#63513: [PATCH] Make persist-defvar work with records and hash tables

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>>> Here you go!
>> Thanks, pushed!
> May it be possible to promote `persist-hash-equal' and
> `persist-copy-tree' to common subr.el functions?
> The topic of comparing hash tables has been previously discussed in
> https://yhetil.org/emacs-devel/871qvz4kdw.fsf@localhost/

Hmm... AFAICT `persist-copy-tree` is a copy of our current `copy-tree`,
so there's nothing to do there :-)

As for `persist-hash-equal` (which is actually called `persist-equal`,
AFAICT), I think if we want to move it out of `persist.el` we need to
improve it so it also looks inside hash-tables that occur within
vectors, conses, etc...

Maybe, like Sam Steingold suggested, we should simply consider the
current treatment of `equal` on hash-tables to be a bug.


        Stefan

bug#63513: [PATCH] Make persist-defvar work with records and hash tables

[Ihor Radchenko]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>>> Here you go!
>>> Thanks, pushed!
>> May it be possible to promote `persist-hash-equal' and
>> `persist-copy-tree' to common subr.el functions?
>> The topic of comparing hash tables has been previously discussed in
>> https://yhetil.org/emacs-devel/871qvz4kdw.fsf@localhost/
>
> Hmm... AFAICT `persist-copy-tree` is a copy of our current `copy-tree`,
> so there's nothing to do there :-)

Err...
Then, wouldn't it be better to contribute this function to compat.el and
use it from there?

> As for `persist-hash-equal` (which is actually called `persist-equal`,
> AFAICT), I think if we want to move it out of `persist.el` we need to
> improve it so it also looks inside hash-tables that occur within
> vectors, conses, etc...
>
> Maybe, like Sam Steingold suggested, we should simply consider the
> current treatment of `equal` on hash-tables to be a bug.

That thread had no progress for quite a while.
I thought that having `hash-equal' function could at least be an
improvement (clearly, it is being used 🙂).

But if you have an idea how to move the original discussion forward,
please share it. AFAIU, no actionable conclusion have been reached in
the linked thread.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#63513: [PATCH] Make persist-defvar work with records and hash tables

[Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Ihor Radchenko <yantar92@posteo.net> writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>>>>> Here you go!
>>>> Thanks, pushed!
>>> May it be possible to promote `persist-hash-equal' and
>>> `persist-copy-tree' to common subr.el functions?
>>> The topic of comparing hash tables has been previously discussed in
>>> https://yhetil.org/emacs-devel/871qvz4kdw.fsf@localhost/
>>
>> Hmm... AFAICT `persist-copy-tree` is a copy of our current `copy-tree`,
>> so there's nothing to do there :-)
>
> Err...
> Then, wouldn't it be better to contribute this function to compat.el and
> use it from there?

The new behavior of copy-tree has already been added to compat.el:

https://github.com/emacs-compat/compat/pull/25

However, that change currently only exists in compat's emacs-30 branch.

I did not know if it was acceptable for persist.el to require compat
when I wrote these patches. If we agree that it is acceptable, then I'm
happy to submit a patch to replace (persist-copy-tree ...) with
(compat-call copy-tree ...), but we'll have to wait to apply the patch
until after the compat.el emacs-30 branch is merged into master.

bug#63513: [PATCH] Make persist-defvar work with records and hash tables

[Ihor Radchenko]

Joseph Turner <joseph@breatheoutbreathe.in> writes:

>> Then, wouldn't it be better to contribute this function to compat.el and
>> use it from there?
>
> The new behavior of copy-tree has already been added to compat.el:
>
> https://github.com/emacs-compat/compat/pull/25
>
> However, that change currently only exists in compat's emacs-30 branch.

I see.

> I did not know if it was acceptable for persist.el to require compat
> when I wrote these patches. If we agree that it is acceptable, then I'm
> happy to submit a patch to replace (persist-copy-tree ...) with
> (compat-call copy-tree ...), but we'll have to wait to apply the patch
> until after the compat.el emacs-30 branch is merged into master.

AFAIU, the recommended way to implement compat function definitions that
are not yet added to compat.el is using `compat-defun' + `compat-call'.
Then, one can simply drop `compat-defun' after the function is finally
release with compat.el without touching the rest of the code.

CCing Daniel.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#63513: [PATCH] Make persist-defvar work with records and hash tables

[Daniel Mendler]

On 9/9/23 12:01, Ihor Radchenko wrote:
> Joseph Turner <joseph@breatheoutbreathe.in> writes:
> 
>>> Then, wouldn't it be better to contribute this function to compat.el and
>>> use it from there?
>>
>> The new behavior of copy-tree has already been added to compat.el:
>>
>> https://github.com/emacs-compat/compat/pull/25
>>
>> However, that change currently only exists in compat's emacs-30 branch.
> 
> I see.
>
>> I did not know if it was acceptable for persist.el to require compat
>> when I wrote these patches. If we agree that it is acceptable, then I'm
>> happy to submit a patch to replace (persist-copy-tree ...) with
>> (compat-call copy-tree ...), but we'll have to wait to apply the patch
>> until after the compat.el emacs-30 branch is merged into master.
> 
> AFAIU, the recommended way to implement compat function definitions that
> are not yet added to compat.el is using `compat-defun' + `compat-call'.
> Then, one can simply drop `compat-defun' after the function is finally
> release with compat.el without touching the rest of the code.

`compat-call` is meant to be used to call "extended functions", for
example functions with additional arguments. See the Compat manual for
details.

The macros from compat-macs.el (`compat-defun` etc.) are internal as
documented in the file compat-macs.el. These macros must not be used
outside Compat.

So using Compat here has to wait until compat-30.x is released.

Daniel

bug#63513: [PATCH] Make persist-defvar work with records and hash tables

[Ihor Radchenko]

Daniel Mendler <mail@daniel-mendler.de> writes:

>> AFAIU, the recommended way to implement compat function definitions that
>> are not yet added to compat.el is using `compat-defun' + `compat-call'.
>> Then, one can simply drop `compat-defun' after the function is finally
>> release with compat.el without touching the rest of the code.
>
> `compat-call` is meant to be used to call "extended functions", for
> example functions with additional arguments. See the Compat manual for
> details.
>
> The macros from compat-macs.el (`compat-defun` etc.) are internal as
> documented in the file compat-macs.el. These macros must not be used
> outside Compat.
>
> So using Compat here has to wait until compat-30.x is released.

And do I understand correctly that compat-30 will only be released after
Emacs 30 is released? If so, it is awkward for :core packages.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#63513: [PATCH] Make persist-defvar work with records and hash tables

[Daniel Mendler]

On 9/9/23 13:35, Ihor Radchenko wrote:
>> So using Compat here has to wait until compat-30.x is released.
> 
> And do I understand correctly that compat-30 will only be released after
> Emacs 30 is released? If so, it is awkward for :core packages.

compat-30 can be released as soon as Emacs 30 has been reasonably
stabilized, e.g., when the emacs-30 branch has been frozen, or a bit
before that. We cannot release much earlier since APIs may still change
and it is probably undesired to release unfinished APIs to the public
too early. For reference, I've created the compat-29.1.1 release around
the day that Eli announced the emacs-29 branch freeze.

Daniel