* [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; 6+ 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] 6+ messages in thread
[parent not found: <O71W8io--B-9@phdk.org-O71WWax----9>]
* 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; 6+ 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] 6+ 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; 6+ 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] 6+ 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; 6+ 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] 6+ 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; 6+ 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] 6+ 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 0 siblings, 0 replies; 6+ 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] 6+ messages in thread
end of thread, other threads:[~2024-10-03 18:01 UTC | newest] Thread overview: 6+ 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
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).