unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#54346: persist-save doesn't persist variables when the value is set to the default
@ 2022-03-12  1:15 Gulshan Singh
  2022-03-12 17:44 ` Lars Ingebrigtsen
  0 siblings, 1 reply; 10+ messages in thread
From: Gulshan Singh @ 2022-03-12  1:15 UTC (permalink / raw)
  To: 54346

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

Calling (persist-save 'myvar) should persist the value of myvar to a
file. However, it doesn't do this if the value of myvar is the same as
the default value. See lines 135 and 136 of persist.el:
https://git.savannah.gnu.org/cgit/emacs/elpa.git/tree/persist.el?h=externals/persist&id=2a2f83b4d63734ed48603008c813cfacd9e99404#n135

The intention here could have been that there's no need to persist the
variable if the user doesn't change it from the default. But it leads
to a bug in the following case:

(require 'persist)
(persist-defvar myvar nil "docstring") ; #1
(persist-save 'myvar) ; #2
(setq myvar "foo")
(persist-save 'myvar) ; #3
(setq myvar nil)
(persist-save 'myvar) ; #4

At #1, the value of myvar is not persisted, which is fine. If myvar
was persisted in the past, that value would be loaded, otherwise (like
in my case), it will be set to nil.

At #2, explicitly calling persist-save does not persist myvar. This is
unexpected, but still not a bug: the in-memory value of myvar (nil) is
still equal to the default, and it is not persisted, so the next time
the code runs the default will be loaded again.

At #3, myvar is persisted. The value is now "foo", which is different
from the default value of "nil".

#4 is where the bug happens. The persisted value of myvar is "foo",
but the in-memory value of myvar is nil. We *should* persist the new
value here, so that on the next load of the symbol we get the latest
value. However, because the current in-memory value is equal to the
default value (nil), it's not persisted. The next time this code runs,
it will load "foo" as the value of myvar instead of nil.

One fix could be to remove the check to see if the value is set to the
default. Another could be that if the value is set to the default,
remove the persist file.

System information from report-emacs-bug:

In GNU Emacs 27.2 (build 1, x86_64-apple-darwin18.7.0, NS appkit-1671.60
Version 10.14.6 (Build 18G95))
 of 2021-11-18 built on builder10-14.lan
Windowing system distributor 'Apple', version 10.3.2113
System Description:  macOS 12.2.1

Configured using:
 'configure --with-ns '--enable-locallisppath=/Library/Application
 Support/Emacs/${version}/site-lisp:/Library/Application
 Support/Emacs/site-lisp' --with-modules'

Configured features:
NOTIFY KQUEUE ACL GNUTLS LIBXML2 ZLIB TOOLKIT_SCROLL_BARS NS MODULES
THREADS JSON PDUMPER GMP

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc puny dired dired-loaddefs
format-spec rfc822 mml mml-sec password-cache epa derived epg epg-config
gnus-util rmail rmail-loaddefs text-property-search seq byte-opt gv
bytecomp byte-compile cconv mm-decode mm-bodies mm-encode mail-parse
rfc2231 mailabbrev gmm-utils mailheader sendmail rfc2047 rfc2045
ietf-drums mm-util mail-prsvr mail-utils persist ispell help-fns
radix-tree cl-print debug backtrace help-mode easymenu find-func
time-date subr-x cl-loaddefs cl-lib tooltip eldoc electric uniquify
ediff-hook vc-hooks lisp-float-type mwheel term/ns-win ns-win
ucs-normalize mule-util term/common-win tool-bar dnd fontset image
regexp-opt fringe tabulated-list replace newcomment text-mode elisp-mode
lisp-mode prog-mode register page tab-bar menu-bar rfn-eshadow isearch
timer select scroll-bar mouse jit-lock font-lock syntax facemenu
font-core term/tty-colors frame minibuffer cl-generic cham georgian
utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao korean
japanese eucjp-ms cp51932 hebrew greek romanian slovak czech european
ethiopic indian cyrillic chinese composite charscript charprop
case-table epa-hook jka-cmpr-hook help simple abbrev obarray
cl-preloaded nadvice loaddefs button faces cus-face macroexp files
text-properties overlay sha1 md5 base64 format env code-pages mule
custom widget hashtable-print-readable backquote threads kqueue cocoa ns
multi-tty make-network-process emacs)

Memory information:
((conses 16 52205 6991)
 (symbols 48 6523 1)
 (strings 32 17870 1009)
 (string-bytes 1 596696)
 (vectors 16 10502)
 (vector-slots 8 132509 11576)
 (floats 8 27 44)
 (intervals 56 378 4)
 (buffers 1000 13))

[-- Attachment #2: Type: text/html, Size: 5335 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* bug#54346: persist-save doesn't persist variables when the value is set to the default
  2022-03-12  1:15 bug#54346: persist-save doesn't persist variables when the value is set to the default Gulshan Singh
@ 2022-03-12 17:44 ` Lars Ingebrigtsen
  2022-04-09 21:45   ` Gulshan Singh
  0 siblings, 1 reply; 10+ messages in thread
From: Lars Ingebrigtsen @ 2022-03-12 17:44 UTC (permalink / raw)
  To: Gulshan Singh; +Cc: 54346, Phillip Lord

Gulshan Singh <gsingh2011@gmail.com> writes:

> One fix could be to remove the check to see if the value is set to the
> default. Another could be that if the value is set to the default,
> remove the persist file.

I've added Phillip to the CCs, perhaps he has some comments.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no





^ permalink raw reply	[flat|nested] 10+ messages in thread

* bug#54346: persist-save doesn't persist variables when the value is set to the default
  2022-03-12 17:44 ` Lars Ingebrigtsen
@ 2022-04-09 21:45   ` Gulshan Singh
  2022-04-10 12:17     ` Lars Ingebrigtsen
  0 siblings, 1 reply; 10+ messages in thread
From: Gulshan Singh @ 2022-04-09 21:45 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: 54346, Phillip Lord

If either of these solutions make sense, or if Phillip has any other
suggestions, I'd be happy to make the fix myself if that would be
easier.

On Sat, Mar 12, 2022 at 9:44 AM Lars Ingebrigtsen <larsi@gnus.org> wrote:
>
> Gulshan Singh <gsingh2011@gmail.com> writes:
>
> > One fix could be to remove the check to see if the value is set to the
> > default. Another could be that if the value is set to the default,
> > remove the persist file.
>
> I've added Phillip to the CCs, perhaps he has some comments.
>
> --
> (domestic pets only, the antidote for overdose, milk.)
>    bloggy blog: http://lars.ingebrigtsen.no





^ permalink raw reply	[flat|nested] 10+ messages in thread

* bug#54346: persist-save doesn't persist variables when the value is set to the default
  2022-04-09 21:45   ` Gulshan Singh
@ 2022-04-10 12:17     ` Lars Ingebrigtsen
  2022-04-10 19:36       ` Gulshan Singh
  0 siblings, 1 reply; 10+ messages in thread
From: Lars Ingebrigtsen @ 2022-04-10 12:17 UTC (permalink / raw)
  To: Gulshan Singh; +Cc: Phillip Lord, 54346

Gulshan Singh <gsingh2011@gmail.com> writes:

> If either of these solutions make sense, or if Phillip has any other
> suggestions, I'd be happy to make the fix myself if that would be
> easier.

Yes, that'd be great (and post the patch here).

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no





^ permalink raw reply	[flat|nested] 10+ messages in thread

* bug#54346: persist-save doesn't persist variables when the value is set to the default
  2022-04-10 12:17     ` Lars Ingebrigtsen
@ 2022-04-10 19:36       ` Gulshan Singh
  2022-04-11 10:18         ` Lars Ingebrigtsen
  0 siblings, 1 reply; 10+ messages in thread
From: Gulshan Singh @ 2022-04-10 19:36 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: Phillip Lord, 54346

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

Hi,

I cloned the upstream project [1] and created a merge request [2]
there. The fix deletes the persist file when it gets set back to the
default value, and it modifies a test to verify this behavior. I've
attached the same patch here (created with `git format-patch HEAD^`, I
haven't submitted a patch here before so let me know if this is the
correct way to do this).

I'm a little confused though, I see that on the externals/persist
branch [3] there is a commit that does not exist on the upstream
GitLab project. Why is this the case? Should I actually be making a
patch off of the externals/persist branch?

[1] https://gitlab.com/phillord/persist/
[2] https://gitlab.com/phillord/persist/-/merge_requests/1
[3] https://git.savannah.gnu.org/cgit/emacs/elpa.git/log/?h=externals/persist

On Sun, Apr 10, 2022 at 5:17 AM Lars Ingebrigtsen <larsi@gnus.org> wrote:
>
> Gulshan Singh <gsingh2011@gmail.com> writes:
>
> > If either of these solutions make sense, or if Phillip has any other
> > suggestions, I'd be happy to make the fix myself if that would be
> > easier.
>
> Yes, that'd be great (and post the patch here).
>
> --
> (domestic pets only, the antidote for overdose, milk.)
>    bloggy blog: http://lars.ingebrigtsen.no

[-- Attachment #2: 0001-Delete-persist-file-when-symbol-is-set-to-default-va.patch --]
[-- Type: application/octet-stream, Size: 2265 bytes --]

From b0496a5a8848d11233f241f6b78f00f1536410d6 Mon Sep 17 00:00:00 2001
From: Gulshan Singh <gsingh2011@gmail.com>
Date: Sun, 10 Apr 2022 12:13:31 -0700
Subject: [PATCH] Delete persist file when symbol is set to default value

---
 persist.el            | 26 ++++++++++++++------------
 test/persist-tests.el |  3 +++
 2 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/persist.el b/persist.el
index 091e428..8b645ea 100644
--- a/persist.el
+++ b/persist.el
@@ -132,18 +132,20 @@ variables persist automatically when Emacs exits."
   (unless (persist--persistant-p symbol)
     (error (format
             "Symbol %s is not persistant" symbol)))
-  (unless (equal (symbol-value symbol)
-                 (persist-default symbol))
-    (let ((dir-loc
-           (file-name-directory
-            (persist--file-location symbol))))
-      (unless (file-exists-p dir-loc)
-        (mkdir dir-loc)))
-    (with-temp-buffer
-      (print (symbol-value symbol) (current-buffer))
-      (write-region (point-min) (point-max)
-                    (persist--file-location symbol)
-                    nil 'quiet))))
+  (let ((symbol-file-loc (persist--file-location symbol)))
+    (if (equal (symbol-value symbol)
+               (persist-default symbol))
+        (when (file-exists-p symbol-file-loc)
+          (delete-file symbol-file-loc))
+      (let ((dir-loc
+             (file-name-directory symbol-file-loc)))
+        (unless (file-exists-p dir-loc)
+          (mkdir dir-loc))
+        (with-temp-buffer
+          (print (symbol-value symbol) (current-buffer))
+          (write-region (point-min) (point-max)
+                        symbol-file-loc
+                        nil 'quiet))))))
 
 (defun persist-default (symbol)
   "Return the default value for SYMBOL."
diff --git a/test/persist-tests.el b/test/persist-tests.el
index 9fa406f..b6645a9 100644
--- a/test/persist-tests.el
+++ b/test/persist-tests.el
@@ -44,6 +44,9 @@
        (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)))))
 
-- 
2.35.1.1178.g4f1659d476-goog


^ permalink raw reply	[flat|nested] 10+ messages in thread

* bug#54346: persist-save doesn't persist variables when the value is set to the default
  2022-04-10 19:36       ` Gulshan Singh
@ 2022-04-11 10:18         ` Lars Ingebrigtsen
  2022-06-21  2:14           ` Gulshan Singh
  0 siblings, 1 reply; 10+ messages in thread
From: Lars Ingebrigtsen @ 2022-04-11 10:18 UTC (permalink / raw)
  To: Gulshan Singh; +Cc: Phillip Lord, 54346

Gulshan Singh <gsingh2011@gmail.com> writes:

> I cloned the upstream project [1] and created a merge request [2]
> there. The fix deletes the persist file when it gets set back to the
> default value, and it modifies a test to verify this behavior. I've
> attached the same patch here (created with `git format-patch HEAD^`, I
> haven't submitted a patch here before so let me know if this is the
> correct way to do this).

Yup; looks good.

> I'm a little confused though, I see that on the externals/persist
> branch [3] there is a commit that does not exist on the upstream
> GitLab project. Why is this the case? Should I actually be making a
> patch off of the externals/persist branch?

That's my error -- I didn't check whether it persist was maintained
externally before making that change.  So it should be merged upstream.

Phillip?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no





^ permalink raw reply	[flat|nested] 10+ messages in thread

* bug#54346: persist-save doesn't persist variables when the value is set to the default
  2022-04-11 10:18         ` Lars Ingebrigtsen
@ 2022-06-21  2:14           ` Gulshan Singh
  2022-06-21 10:49             ` Lars Ingebrigtsen
  0 siblings, 1 reply; 10+ messages in thread
From: Gulshan Singh @ 2022-06-21  2:14 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: 54346, Phillip Lord

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

Hi,

It's been a few months since the last message here. I also directly sent
Phillip an email last week asking if he could take a look at my PR, but I
haven't received a response.

Do you know what's normally done in cases like this? Even if persist.el is
maintained externally, should we just merge my patch and close this bug if
we can't get a response from the maintainer?

On Mon, Apr 11, 2022 at 3:18 AM Lars Ingebrigtsen <larsi@gnus.org> wrote:

> Gulshan Singh <gsingh2011@gmail.com> writes:
>
> > I cloned the upstream project [1] and created a merge request [2]
> > there. The fix deletes the persist file when it gets set back to the
> > default value, and it modifies a test to verify this behavior. I've
> > attached the same patch here (created with `git format-patch HEAD^`, I
> > haven't submitted a patch here before so let me know if this is the
> > correct way to do this).
>
> Yup; looks good.
>
> > I'm a little confused though, I see that on the externals/persist
> > branch [3] there is a commit that does not exist on the upstream
> > GitLab project. Why is this the case? Should I actually be making a
> > patch off of the externals/persist branch?
>
> That's my error -- I didn't check whether it persist was maintained
> externally before making that change.  So it should be merged upstream.
>
> Phillip?
>
> --
> (domestic pets only, the antidote for overdose, milk.)
>    bloggy blog: http://lars.ingebrigtsen.no
>

[-- Attachment #2: Type: text/html, Size: 2034 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* bug#54346: persist-save doesn't persist variables when the value is set to the default
  2022-06-21  2:14           ` Gulshan Singh
@ 2022-06-21 10:49             ` Lars Ingebrigtsen
  2022-06-22  0:55               ` Gulshan Singh
  0 siblings, 1 reply; 10+ messages in thread
From: Lars Ingebrigtsen @ 2022-06-21 10:49 UTC (permalink / raw)
  To: Gulshan Singh; +Cc: Phillip Lord, 54346

Gulshan Singh <gsingh2011@gmail.com> writes:

> Do you know what's normally done in cases like this? Even if
> persist.el is maintained externally, should we just merge my patch and
> close this bug if we can't get a response from the maintainer?

Yes, we can merge the patch.  However, it doesn't seem to apply to the
version we have in ELPA -- the upstream and the ELPA version have
already diverged, apparently.

Can you respin the patch against the version in GNU ELPA?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no





^ permalink raw reply	[flat|nested] 10+ messages in thread

* bug#54346: persist-save doesn't persist variables when the value is set to the default
  2022-06-21 10:49             ` Lars Ingebrigtsen
@ 2022-06-22  0:55               ` Gulshan Singh
  2022-06-22  4:21                 ` Lars Ingebrigtsen
  0 siblings, 1 reply; 10+ messages in thread
From: Gulshan Singh @ 2022-06-22  0:55 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: Phillip Lord, 54346


[-- Attachment #1.1: Type: text/plain, Size: 739 bytes --]

Sure, the attached patch should work on the ELPA version.

On Tue, Jun 21, 2022 at 3:50 AM Lars Ingebrigtsen <larsi@gnus.org> wrote:

> Gulshan Singh <gsingh2011@gmail.com> writes:
>
> > Do you know what's normally done in cases like this? Even if
> > persist.el is maintained externally, should we just merge my patch and
> > close this bug if we can't get a response from the maintainer?
>
> Yes, we can merge the patch.  However, it doesn't seem to apply to the
> version we have in ELPA -- the upstream and the ELPA version have
> already diverged, apparently.
>
> Can you respin the patch against the version in GNU ELPA?
>
> --
> (domestic pets only, the antidote for overdose, milk.)
>    bloggy blog: http://lars.ingebrigtsen.no
>

[-- Attachment #1.2: Type: text/html, Size: 1224 bytes --]

[-- Attachment #2: 0001-Delete-persist-file-when-symbol-is-set-to-default-va.patch --]
[-- Type: application/octet-stream, Size: 2688 bytes --]

From 4f27733c326c5f24768fb09fd8be4742fd19e3bc Mon Sep 17 00:00:00 2001
From: Gulshan Singh <gsingh2011@gmail.com>
Date: Tue, 21 Jun 2022 17:38:58 -0700
Subject: [PATCH] Delete persist file when symbol is set to default value

---
 persist.el            | 38 ++++++++++++++++++++------------------
 test/persist-tests.el |  3 +++
 2 files changed, 23 insertions(+), 18 deletions(-)

diff --git a/persist.el b/persist.el
index 3d8d1af..950de3a 100644
--- a/persist.el
+++ b/persist.el
@@ -132,24 +132,26 @@ variables persist automatically when Emacs exits."
   (unless (persist--persistant-p symbol)
     (error (format
             "Symbol %s is not persistant" symbol)))
-  (unless (equal (symbol-value symbol)
-                 (persist-default symbol))
-    (let ((dir-loc
-           (file-name-directory
-            (persist--file-location symbol))))
-      (unless (file-exists-p dir-loc)
-        (mkdir dir-loc)))
-    (with-temp-buffer
-      (let (print-level
-	    print-length
-            print-quoted
-            (print-escape-control-characters t)
-            (print-escape-nonascii t)
-            (print-circle t))
-	(print (symbol-value symbol) (current-buffer)))
-      (write-region (point-min) (point-max)
-                    (persist--file-location symbol)
-                    nil 'quiet))))
+  (let ((symbol-file-loc (persist--file-location symbol)))
+    (if (equal (symbol-value symbol)
+               (persist-default symbol))
+        (when (file-exists-p symbol-file-loc)
+          (delete-file symbol-file-loc))
+      (let ((dir-loc
+             (file-name-directory symbol-file-loc)))
+        (unless (file-exists-p dir-loc)
+          (mkdir dir-loc))
+        (with-temp-buffer
+          (let (print-level
+                print-length
+                print-quoted
+                (print-escape-control-characters t)
+                (print-escape-nonascii t)
+                (print-circle t))
+            (print (symbol-value symbol) (current-buffer)))
+          (write-region (point-min) (point-max)
+                        symbol-file-loc
+                        nil 'quiet))))))
 
 (defun persist-default (symbol)
   "Return the default value for SYMBOL."
diff --git a/test/persist-tests.el b/test/persist-tests.el
index 9fa406f..b6645a9 100644
--- a/test/persist-tests.el
+++ b/test/persist-tests.el
@@ -44,6 +44,9 @@
        (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)))))
 
-- 
2.37.0.rc0.104.g0611611a94-goog


^ permalink raw reply	[flat|nested] 10+ messages in thread

* bug#54346: persist-save doesn't persist variables when the value is set to the default
  2022-06-22  0:55               ` Gulshan Singh
@ 2022-06-22  4:21                 ` Lars Ingebrigtsen
  0 siblings, 0 replies; 10+ messages in thread
From: Lars Ingebrigtsen @ 2022-06-22  4:21 UTC (permalink / raw)
  To: Gulshan Singh; +Cc: Phillip Lord, 54346

Gulshan Singh <gsingh2011@gmail.com> writes:

> Sure, the attached patch should work on the ELPA version.

Thanks; now pushed to GNU ELPA.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no





^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2022-06-22  4:21 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-12  1:15 bug#54346: persist-save doesn't persist variables when the value is set to the default Gulshan Singh
2022-03-12 17:44 ` Lars Ingebrigtsen
2022-04-09 21:45   ` Gulshan Singh
2022-04-10 12:17     ` Lars Ingebrigtsen
2022-04-10 19:36       ` Gulshan Singh
2022-04-11 10:18         ` Lars Ingebrigtsen
2022-06-21  2:14           ` Gulshan Singh
2022-06-21 10:49             ` Lars Ingebrigtsen
2022-06-22  0:55               ` Gulshan Singh
2022-06-22  4:21                 ` Lars Ingebrigtsen

Code repositories for project(s) associated with this 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).