GNU bug report logs - #54346
persist-save doesn't persist variables when the value is set to the default

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 54346 in the body.
You can then email your comments to 54346 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Gulshan Singh <gsingh2011 <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: persist-save doesn't persist variables when the value is set to the
 default
Date: Fri, 11 Mar 2022 17:15:53 -0800

[Message part 1 (text/plain, inline)]

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))

[Message part 2 (text/html, inline)]

Message #8 received at 54346 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Gulshan Singh <gsingh2011 <at> gmail.com>
Cc: Phillip Lord <phillip.lord <at> russet.org.uk>, 54346 <at> debbugs.gnu.org
Subject: Re: bug#54346: persist-save doesn't persist variables when the
 value is set to the default
Date: Sat, 12 Mar 2022 18:44:34 +0100

Gulshan Singh <gsingh2011 <at> 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

Message #11 received at 54346 <at> debbugs.gnu.org (full text, mbox):

From: Gulshan Singh <gsingh2011 <at> gmail.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: Phillip Lord <phillip.lord <at> russet.org.uk>, 54346 <at> debbugs.gnu.org
Subject: Re: bug#54346: persist-save doesn't persist variables when the value
 is set to the default
Date: Sat, 9 Apr 2022 14:45:38 -0700

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 <at> gnus.org> wrote:
>
> Gulshan Singh <gsingh2011 <at> 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

Message #14 received at 54346 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Gulshan Singh <gsingh2011 <at> gmail.com>
Cc: 54346 <at> debbugs.gnu.org, Phillip Lord <phillip.lord <at> russet.org.uk>
Subject: Re: bug#54346: persist-save doesn't persist variables when the
 value is set to the default
Date: Sun, 10 Apr 2022 14:17:26 +0200

Gulshan Singh <gsingh2011 <at> 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

Message #17 received at 54346 <at> debbugs.gnu.org (full text, mbox):

From: Gulshan Singh <gsingh2011 <at> gmail.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 54346 <at> debbugs.gnu.org, Phillip Lord <phillip.lord <at> russet.org.uk>
Subject: Re: bug#54346: persist-save doesn't persist variables when the value
 is set to the default
Date: Sun, 10 Apr 2022 12:36:54 -0700

[Message part 1 (text/plain, inline)]

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 <at> gnus.org> wrote:
>
> Gulshan Singh <gsingh2011 <at> 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

[0001-Delete-persist-file-when-symbol-is-set-to-default-va.patch (application/octet-stream, attachment)]

Message #20 received at 54346 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Gulshan Singh <gsingh2011 <at> gmail.com>
Cc: 54346 <at> debbugs.gnu.org, Phillip Lord <phillip.lord <at> russet.org.uk>
Subject: Re: bug#54346: persist-save doesn't persist variables when the
 value is set to the default
Date: Mon, 11 Apr 2022 12:18:01 +0200

Gulshan Singh <gsingh2011 <at> 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

Message #23 received at 54346 <at> debbugs.gnu.org (full text, mbox):

From: Gulshan Singh <gsingh2011 <at> gmail.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 54346 <at> debbugs.gnu.org, Phillip Lord <phillip.lord <at> russet.org.uk>
Subject: Re: bug#54346: persist-save doesn't persist variables when the value
 is set to the default
Date: Mon, 20 Jun 2022 19:14:35 -0700

[Message part 1 (text/plain, inline)]

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 <at> gnus.org> wrote:

> Gulshan Singh <gsingh2011 <at> 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
>

[Message part 2 (text/html, inline)]

Message #26 received at 54346 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Gulshan Singh <gsingh2011 <at> gmail.com>
Cc: Phillip Lord <phillip.lord <at> russet.org.uk>, 54346 <at> debbugs.gnu.org
Subject: Re: bug#54346: persist-save doesn't persist variables when the
 value is set to the default
Date: Tue, 21 Jun 2022 12:49:51 +0200

Gulshan Singh <gsingh2011 <at> 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

Message #29 received at 54346 <at> debbugs.gnu.org (full text, mbox):

From: Gulshan Singh <gsingh2011 <at> gmail.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: Phillip Lord <phillip.lord <at> russet.org.uk>, 54346 <at> debbugs.gnu.org
Subject: Re: bug#54346: persist-save doesn't persist variables when the value
 is set to the default
Date: Tue, 21 Jun 2022 17:55:42 -0700

[Message part 1 (text/plain, inline)]

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

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

> Gulshan Singh <gsingh2011 <at> 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
>

[Message part 2 (text/html, inline)]

[0001-Delete-persist-file-when-symbol-is-set-to-default-va.patch (application/octet-stream, attachment)]

Message #32 received at 54346 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Gulshan Singh <gsingh2011 <at> gmail.com>
Cc: Phillip Lord <phillip.lord <at> russet.org.uk>, 54346 <at> debbugs.gnu.org
Subject: Re: bug#54346: persist-save doesn't persist variables when the
 value is set to the default
Date: Wed, 22 Jun 2022 06:21:48 +0200

Gulshan Singh <gsingh2011 <at> 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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.