[PATCH] Fixing a regression in org-persists ability to handle non-list container arguments.

[PATCH] Fixing a regression in org-persists ability to handle non-list container arguments.

[lra]

Hello org-mode mailing list

Long time, first time. This is just a tiny patch fixing a regression
in org-persist--find-index that was missed in the 95f77669e0 bugfix. It affects
calls to org-persist-read that don't use a list of containers,
e.g. those in ox-latex's precompile function.

I was having some issues getting the org-latex-preview branch to run
smoothly when the xref-find-def trail led me to realize that it wasn't
just my setup causing trouble. In the process of compiling a bug
report I ended up fixing it on my end and thought I might as well send
it your way and save sombody else the trouble. I took the liberty of
clarifying a related comment that confused me a bit while working out
the logic of the function.

(This is just a quick fix of the specific regression, a more proper
solution would probably move all the logic concerning container lists
further up in the chain. However, actually doing something like
rewriting o-p--normalize-container to achieve that is above my
pay-grade. My experience writing elisp basically comes from writing a
few advice-hacks to get stuff to do what I want, so feel free to
reject and forget if you think this deserves a full treatment.)

MWE running with -Q and org compiled from git (taken from the
examples in org-persist documentation):
  (let ((info1 "test")
        (info2 "test 2"))
    (org-persist-register
     `("Named data" (elisp info1 local) (elisp info2 local))
     nil :write-immediately t))
  (org-persist-read
   "Named data"
   nil nil nil :read-related t)

Expected result: org-persist-read will return ("Named data" "test" "test 2")
Current result: it returns nil

Kind regards,
LRA

P.S. Thank you all for all the hard work you put into this
wonderful tool. It does not go unappreciated!

Re: [PATCH] Fixing a regression in org-persists ability to handle non-list container arguments.

[lra]

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


And it is *with* the attachment.

Kind regards,
LRA


Sep 18, 2024, 02:26 by lra@phdk.org:

> Hello org-mode mailing list
>
> Long time, first time. This is just a tiny patch fixing a regression
> in org-persist--find-index that was missed in the 95f77669e0 bugfix. It affects
> calls to org-persist-read that don't use a list of containers,
> e.g. those in ox-latex's precompile function.
>
> I was having some issues getting the org-latex-preview branch to run
> smoothly when the xref-find-def trail led me to realize that it wasn't
> just my setup causing trouble. In the process of compiling a bug
> report I ended up fixing it on my end and thought I might as well send
> it your way and save sombody else the trouble. I took the liberty of
> clarifying a related comment that confused me a bit while working out
> the logic of the function.
>
> (This is just a quick fix of the specific regression, a more proper
> solution would probably move all the logic concerning container lists
> further up in the chain. However, actually doing something like
> rewriting o-p--normalize-container to achieve that is above my
> pay-grade. My experience writing elisp basically comes from writing a
> few advice-hacks to get stuff to do what I want, so feel free to
> reject and forget if you think this deserves a full treatment.)
>
> MWE running with -Q and org compiled from git (taken from the
> examples in org-persist documentation):
>   (let ((info1 "test")
>         (info2 "test 2"))
>     (org-persist-register
>      `("Named data" (elisp info1 local) (elisp info2 local))
>      nil :write-immediately t))
>   (org-persist-read
>    "Named data"
>    nil nil nil :read-related t)
>
> Expected result: org-persist-read will return ("Named data" "test" "test 2")
> Current result: it returns nil
>
> Kind regards,
> LRA
>
> P.S. Thank you all for all the hard work you put into this
> wonderful tool. It does not go unappreciated!
>
>


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

[-- Attachment #2: 0001-lisp-org-persist.el-Fix-regression-missed-by-7fd8099.patch --]
[-- Type: text/x-patch, Size: 1646 bytes --]

From caf7ff050876c4e604c8d4e159e7c2d51ad7f897 Mon Sep 17 00:00:00 2001
From: Lukas Rudd Andersen <lra@phdk.org>
Date: Wed, 18 Sep 2024 00:54:27 +0200
Subject: [PATCH] lisp/org-persist.el: Fix regression missed by 7fd8099

* lisp/org-persist.el: (org-persist--find-index): Fix regression that
  makes the function return nil when container in COLLECTION is not a
  list of containers.

TINYCHANGE
---
 lisp/org-persist.el | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/lisp/org-persist.el b/lisp/org-persist.el
index 8114fd5b9..7bae09f92 100644
--- a/lisp/org-persist.el
+++ b/lisp/org-persist.el
@@ -546,13 +546,15 @@ FORMAT and ARGS are passed to `message'."
                          (and hash (gethash (cons cont (list :hash hash)) org-persist--index-hash))
                          (and key (gethash (cons cont (list :key key)) org-persist--index-hash))))
              (when (and r
-                        ;; Every element in CONTAINER matches
-                        ;; COLLECTION.
+                        ;; Every element in container group of
+                        ;; COLLECTION matches returned CONTAINER. 
                         (seq-every-p
                          (lambda (cont)
                            (org-persist-collection-let r
                              (member cont container)))
-                         container))
+                         (if (listp (car container))
+                             container
+                           (list container))))
                (throw :found r))))))))
 
 (defun org-persist--add-to-index (collection &optional hash-only)
-- 
2.45.2

Re: [PATCH] Fixing a regression in org-persists ability to handle non-list container arguments.

[Ihor Radchenko]

lra@phdk.org writes:

> And it is *with* the attachment.

Thanks!
Applied, onto main.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=a8790ed09e

I also added you to the contributor list.
https://git.sr.ht/~bzg/worg/commit/466cfcf2

P.S. We really need tests for org-persist.

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

Re: [PATCH] Fixing a regression in org-persists ability to handle non-list container arguments.

[Karthik Chikmagalur]

LRA, thanks for the fix.

Ihor, `org-persist--find-index' is singularly confusing.  I've been over
it with edebug a few times and still can't figure out what it's doing,
because `container' means different things at different points in the
function because of the macro `org-persist-collection-let'.  Could it be
simplified in some way?

Karthik

Re: [PATCH] Fixing a regression in org-persists ability to handle non-list container arguments.

[Ihor Radchenko]

Karthik Chikmagalur <karthikchikmagalur@gmail.com> writes:

> LRA, thanks for the fix.
>
> Ihor, `org-persist--find-index' is singularly confusing.  I've been over
> it with edebug a few times and still can't figure out what it's doing,
> because `container' means different things at different points in the
> function because of the macro `org-persist-collection-let'.  Could it be
> simplified in some way?

This dichotomy is indeed confusing.
I think that it will make things easier to change :container field into
:containers - always storing container list.

One needs to go through org-persist.el and change this convention
everywhere. Then, bump `org-persist--storage-version'.

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

Re: [PATCH] Fixing a regression in org-persists ability to handle non-list container arguments.

[lra]

Hi Ihor and Karthik,

> Thanks!
> Applied, onto main.

> LRA, thanks for the fix.

Glad to be of assistance!

> P.S. We really need tests for org-persist.

Yeah I did note a couple of pretty straight-forward opportunities for
clean-up wrt. redundancies and readability, but without
regression-tests and lacking confidence that I understood everything I
thought it was best to keep the patch as simple as possible. I
would've felt pretty stupid if I accidentally introduced a new
regression in my regression-fix haha. I'll include some of the stuff
from my notes below in case it's of interest to anybody working on
org-persist.

I hope it isn't just noise and that it can be of use :)

Regards,
LRA

-----

(These are all relative to the org-latex-preview dev-branch and not
main, but I think all the code in question applies to both, just the
linenums might be off.)

1.
   I noticed when testing on upstream that there are a ton of
   redundant calls to `o-p--normalize-container' throughout the
   package. I saw that you've already dealt with the redundancy in
   your branch Karthik, so maybe it's unimportant but I think the
   redundant calls also just hurt readability somewhat. A couple
   cases of
#+begin_example elisp
(defun somefun (container ... )
  (let ((container (org-persist--normalize-container container))
    ... )))

;; And then every call will have
  ...
  (somefun (org-persist--normalize-container container))
  ...
#+end_example
   
2.
   The bits handling the shape of container in `o-p--add-to-index'
   and `o-p--remove-from-index' are both redundant, as all calls
   to these functions use containers read from the index or from
   `o-p--get-collection', in both cases there's already logic ensuring
   it's a list of lists. This change worked fine in my limited testing:

Replacing =lisp/org-persist.el:L576-9= and =L592-5=
#+begin_example elisp
        (dolist (cont (cons container container))
#+end_example

3.
   Maybe I'm just an idiot, but the use (or maybe just the name) of
   `o-p--get-collection' was a bit confusing to me, as it's used both
   to find and /create/ indices. E.g. this bit of logic in
   `o-p-register' was particularly confusing where it's used in a let
   /before/ a conditional that determines whether the assignment is
   actually used for anything.
   
=lisp/org-persist.el:L1006-8=
#+begin_example elisp
    (let ((collection (org-persist--get-collection container associated misc)))
      (when (and expiry (not inherit))
        (when expiry (plist-put collection :expiry expiry))))
#+end_example

   With the redundant inner `when', it reads a bit like the let was
   supposed to be between the two, which I think makes sense if the
   first condition is an `or' instead of `and'? Although I'm not sure
   I actually figured out how this function or the inherit keyword
   works.

4. 
> Ihor, `org-persist--find-index' is singularly confusing.  I've been over
> it with edebug a few times and still can't figure out what it's doing,
> because `container' means different things at different points in the
> function because of the macro `org-persist-collection-let'.  Could it be
> simplified in some way?

   I had the exact same confusion motivating my attempt at rewording
   the comment. On my machine and in my limited testing, dropping the
   second use of the macro completely  worked fine:

   Replacing =lisp/org-persist.el:L552-4=
#+begin_example elisp
                         (lambda (cont)
                             (member cont (plist-get r :container)))
#+end_example

5.
   Another way to simplify `o-p--find-index' (and I think also get
   pretty close to achieving full consistency on the container shape),
   would be to put this bit from the start of `o-p--get-collection' at
   the start of `-read', `-write-all' and `-unregister' (or maybe in a
   (if (not inner) .. ) at the end of `o-p--normalize-container'?):

#+begin_example elisp
(unless (and (listp container) (listp (car container)))
    (setq container (list container)))
#+end_example

   This would cover all calls to`o-p--find-index' and allow you to
   revert the changes in both my patch and 7fd80991c3 I think. This is
   sort-of what I was thinking of wrt. rewriting
   `o-p--normalize-container' in my original e-mail.

-----

Re: [PATCH] Fixing a regression in org-persists ability to handle non-list container arguments.

[Ihor Radchenko]

lra@phdk.org writes:

>> P.S. We really need tests for org-persist.
>
> Yeah I did note a couple of pretty straight-forward opportunities for
> clean-up wrt. redundancies and readability, but without
> regression-tests and lacking confidence that I understood everything I
> thought it was best to keep the patch as simple as possible. I
> would've felt pretty stupid if I accidentally introduced a new
> regression in my regression-fix haha. I'll include some of the stuff
> from my notes below in case it's of interest to anybody working on
> org-persist.
> ...

It is hard to judge many of the comments without seeing the right
context. Ideally, the proposed ideas could be done in a form of patches
;)

That said, I would be similarly hesitant to merge large changes in
org-persist without adding at least some basic tests.

Would you be interested to create testing/lisp/org-persist.el file and
add one or two basic tests that replicate what the preview branch does?
That way, we can at least make sure that things are not broken as we
change the actual library.

Then, we can start looking into changing org-persist itself. (Also,
having _a_ test file will greatly reduce friction to add more tests)

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