* [PATCH] Fixing a regression in org-persists ability to handle non-list container arguments.
@ 2024-09-18 0:24 lra
[not found] ` <O71W8io--B-9@phdk.org-O71WWax----9>
0 siblings, 1 reply; 7+ messages in thread
From: lra @ 2024-09-18 0:24 UTC (permalink / raw)
To: Emacs Orgmode
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!
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fixing a regression in org-persists ability to handle non-list container arguments.
[not found] ` <O71W8io--B-9@phdk.org-O71WWax----9>
@ 2024-09-18 0:29 ` lra
2024-09-22 9:11 ` Ihor Radchenko
0 siblings, 1 reply; 7+ messages in thread
From: lra @ 2024-09-18 0:29 UTC (permalink / raw)
To: Lra; +Cc: Emacs Orgmode
[-- 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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] Fixing a regression in org-persists ability to handle non-list container arguments.
2024-09-18 0:29 ` lra
@ 2024-09-22 9:11 ` Ihor Radchenko
2024-09-22 23:04 ` Karthik Chikmagalur
0 siblings, 1 reply; 7+ messages in thread
From: Ihor Radchenko @ 2024-09-22 9:11 UTC (permalink / raw)
To: lra; +Cc: Emacs Orgmode
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>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fixing a regression in org-persists ability to handle non-list container arguments.
2024-09-22 9:11 ` Ihor Radchenko
@ 2024-09-22 23:04 ` Karthik Chikmagalur
2024-10-02 17:34 ` Ihor Radchenko
0 siblings, 1 reply; 7+ messages in thread
From: Karthik Chikmagalur @ 2024-09-22 23:04 UTC (permalink / raw)
To: Ihor Radchenko, lra; +Cc: Emacs Orgmode
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fixing a regression in org-persists ability to handle non-list container arguments.
2024-09-22 23:04 ` Karthik Chikmagalur
@ 2024-10-02 17:34 ` Ihor Radchenko
2024-10-03 18:00 ` lra
0 siblings, 1 reply; 7+ messages in thread
From: Ihor Radchenko @ 2024-10-02 17:34 UTC (permalink / raw)
To: Karthik Chikmagalur; +Cc: lra, Emacs Orgmode
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>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fixing a regression in org-persists ability to handle non-list container arguments.
2024-10-02 17:34 ` Ihor Radchenko
@ 2024-10-03 18:00 ` lra
2024-10-06 12:35 ` Ihor Radchenko
0 siblings, 1 reply; 7+ messages in thread
From: lra @ 2024-10-03 18:00 UTC (permalink / raw)
To: Ihor Radchenko; +Cc: Karthik Chikmagalur, Emacs Orgmode
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.
-----
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fixing a regression in org-persists ability to handle non-list container arguments.
2024-10-03 18:00 ` lra
@ 2024-10-06 12:35 ` Ihor Radchenko
0 siblings, 0 replies; 7+ messages in thread
From: Ihor Radchenko @ 2024-10-06 12:35 UTC (permalink / raw)
To: lra; +Cc: Karthik Chikmagalur, Emacs Orgmode
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>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-10-06 12:34 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-09-18 0:24 [PATCH] Fixing a regression in org-persists ability to handle non-list container arguments lra
[not found] ` <O71W8io--B-9@phdk.org-O71WWax----9>
2024-09-18 0:29 ` lra
2024-09-22 9:11 ` Ihor Radchenko
2024-09-22 23:04 ` Karthik Chikmagalur
2024-10-02 17:34 ` Ihor Radchenko
2024-10-03 18:00 ` lra
2024-10-06 12:35 ` Ihor Radchenko
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs/org-mode.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).