From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mp1.migadu.com ([2001:41d0:403:4876::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by ms13.migadu.com with LMTPS id yKVsEQXc/maxFAAA62LTzQ:P1 (envelope-from ) for ; Thu, 03 Oct 2024 18:01:41 +0000 Received: from aspmx1.migadu.com ([2001:41d0:403:4876::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by mp1.migadu.com with LMTPS id yKVsEQXc/maxFAAA62LTzQ (envelope-from ) for ; Thu, 03 Oct 2024 20:01:41 +0200 X-Envelope-To: larch@yhetil.org Authentication-Results: aspmx1.migadu.com; dkim=none; dmarc=none; spf=pass (aspmx1.migadu.com: domain of "emacs-orgmode-bounces+larch=yhetil.org@gnu.org" designates 209.51.188.17 as permitted sender) smtp.mailfrom="emacs-orgmode-bounces+larch=yhetil.org@gnu.org" ARC-Seal: i=1; s=key1; d=yhetil.org; t=1727978501; a=rsa-sha256; cv=none; b=ATPI9Q0ML7XCSFugYnX9Y2E/lGIjkaKiwivPproLLSor+y0EA9xLbujF3Y7qP7RuTSgNFa kN9bx0oD1PI055Hdek4SAk6nsxlR8un6t84Wrx7WqtGl44m/7yF+3Ycq9mx9fXhEqK2ONp FjDoxY+NJu5R4kqsR3wwLQ4OpDADMqO5LF94ABNaQ4a952tRLuaPWNGWN5nc1PFPOFaWEU FQYPmfk1l2BveVP2LX9mnzoRuagFkMJZTYcATZOW4tJ6NkNQzBv0k+0PZaGEUp3gUEfO/a 8BuAJWskoGF/OMRKZyxux6AtmuHtHV1NOHRHgLdaRdaqcRFk2eYvR8PX0y+DUA== ARC-Authentication-Results: i=1; aspmx1.migadu.com; dkim=none; dmarc=none; spf=pass (aspmx1.migadu.com: domain of "emacs-orgmode-bounces+larch=yhetil.org@gnu.org" designates 209.51.188.17 as permitted sender) smtp.mailfrom="emacs-orgmode-bounces+larch=yhetil.org@gnu.org" ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=yhetil.org; s=key1; t=1727978501; h=from:from:sender:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:list-id:list-help: list-unsubscribe:list-subscribe:list-post; bh=QmLz6+VjqM9QSWOHfbX1myQlgHPDgVbXIGiW3S9B3ms=; b=UEyXOpRAzAzrdGFaIJwCSGxWcynIpSgn20ockQozk0dKRqQLSkGsa9CUWJ+V2oRlXW6iYN fh8nLNgVHINcnOQhh1rlygsqPBGo6ns9qFF6qKe/8tRGHQ1pijrhxbDYlBzUZFjhT4+Ady GgKBnPzNGfU+9wqXymvjJFOgrRfB1hvWD2ADXoJ5O5gv8yM/DA2l6tH8J5ztu4qHc6PJ60 g6ahvY0lJhrq6ldc703/mdI7QjJE09A8gOwZM7E4g0nydtPDVV7kPXo9YLL1WFYuIPeNXj vf5kIeK01tWUFKBtNFDvtAvT+pwH8r3BktnRsOZVCHhq9ReTFzHvYrc6DI6T6w== Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by aspmx1.migadu.com (Postfix) with ESMTPS id D83577CFC7 for ; Thu, 03 Oct 2024 20:01:40 +0200 (CEST) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1swQ8O-0001KA-3k; Thu, 03 Oct 2024 14:01:04 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1swQ8A-0001JM-Js for emacs-orgmode@gnu.org; Thu, 03 Oct 2024 14:01:01 -0400 Received: from mail.w14.tutanota.de ([185.205.69.214]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1swQ88-0004Pk-9T for emacs-orgmode@gnu.org; Thu, 03 Oct 2024 14:00:50 -0400 Received: from tutadb.w10.tutanota.de (w10.api.tuta.com [IPv6:fd:ac::d:10]) by mail.w14.tutanota.de (Postfix) with ESMTP id 67CBF28A4292; Thu, 3 Oct 2024 20:00:40 +0200 (CEST) Date: Thu, 3 Oct 2024 20:00:40 +0200 (CEST) From: lra@phdk.org To: Ihor Radchenko Cc: Karthik Chikmagalur , Emacs Orgmode Message-ID: In-Reply-To: <87ikuaqugu.fsf@localhost> References: <87zfo0qchb.fsf@localhost> <87r09bwary.fsf@gmail.com> <87ikuaqugu.fsf@localhost> Subject: Re: [PATCH] Fixing a regression in org-persists ability to handle non-list container arguments. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Received-SPF: pass client-ip=185.205.69.214; envelope-from=lra@phdk.org; helo=mail.w14.tutanota.de X-Spam_score_int: -18 X-Spam_score: -1.9 X-Spam_bar: - X-Spam_report: (-1.9 / 5.0 requ) BAYES_00=-1.9, RCVD_IN_MSPIKE_H4=0.001, RCVD_IN_MSPIKE_WL=0.001, RCVD_IN_VALIDITY_RPBL_BLOCKED=0.001, RCVD_IN_VALIDITY_SAFE_BLOCKED=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: emacs-orgmode@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "General discussions about Org-mode." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-orgmode-bounces+larch=yhetil.org@gnu.org Sender: emacs-orgmode-bounces+larch=yhetil.org@gnu.org X-Migadu-Country: US X-Migadu-Flow: FLOW_IN X-Migadu-Spam-Score: -3.76 X-Spam-Score: -3.76 X-Migadu-Queue-Id: D83577CFC7 X-Migadu-Scanner: mx10.migadu.com X-TUID: JaGt80gNmyeW 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. =C2=A0=C2=A0 I noticed when testing on upstream that there are a ton of =C2=A0=C2=A0 redundant calls to `o-p--normalize-container' throughout the =C2=A0=C2=A0 package. I saw that you've already dealt with the redundancy i= n =C2=A0=C2=A0 your branch Karthik, so maybe it's unimportant but I think the =C2=A0=C2=A0 redundant calls also just hurt readability somewhat. A couple =C2=A0=C2=A0 cases of #+begin_example elisp (defun somefun (container ... ) =C2=A0 (let ((container (org-persist--normalize-container container)) =C2=A0=C2=A0=C2=A0 ... ))) ;; And then every call will have =C2=A0 ... =C2=A0 (somefun (org-persist--normalize-container container)) =C2=A0 ... #+end_example =C2=A0=C2=A0=C2=A0 2. =C2=A0=C2=A0 The bits handling the shape of container in `o-p--add-to-index= ' =C2=A0=C2=A0 and `o-p--remove-from-index' are both redundant, as all calls =C2=A0=C2=A0 to these functions use containers read from the index or from =C2=A0=C2=A0 `o-p--get-collection', in both cases there's already logic ens= uring =C2=A0=C2=A0 it's a list of lists. This change worked fine in my limited te= sting: Replacing =3Dlisp/org-persist.el:L576-9=3D and =3DL592-5=3D #+begin_example elisp =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (dolist (cont (cons container co= ntainer)) #+end_example 3. =C2=A0=C2=A0 Maybe I'm just an idiot, but the use (or maybe just the name) = of =C2=A0=C2=A0 `o-p--get-collection' was a bit confusing to me, as it's used = both =C2=A0=C2=A0 to find and /create/ indices. E.g. this bit of logic in =C2=A0=C2=A0 `o-p-register' was particularly confusing where it's used in a= let =C2=A0=C2=A0 /before/ a conditional that determines whether the assignment = is =C2=A0=C2=A0 actually used for anything. =C2=A0=C2=A0=C2=A0 =3Dlisp/org-persist.el:L1006-8=3D #+begin_example elisp =C2=A0=C2=A0=C2=A0 (let ((collection (org-persist--get-collection container= associated misc))) =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (when (and expiry (not inherit)) =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (when expiry (plist-put collecti= on :expiry expiry)))) #+end_example =C2=A0=C2=A0 With the redundant inner `when', it reads a bit like the let w= as =C2=A0=C2=A0 supposed to be between the two, which I think makes sense if t= he =C2=A0=C2=A0 first condition is an `or' instead of `and'? Although I'm not = sure =C2=A0=C2=A0 I actually figured out how this function or the inherit keywor= d =C2=A0=C2=A0 works. 4.=C2=A0 > Ihor, `org-persist--find-index' is singularly confusing.=C2=A0 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'.=C2=A0 Could i= t be > simplified in some way? =C2=A0=C2=A0 I had the exact same confusion motivating my attempt at reword= ing =C2=A0=C2=A0 the comment. On my machine and in my limited testing, dropping= the =C2=A0=C2=A0 second use of the macro completely worked fine: =C2=A0=C2=A0 Replacing =3Dlisp/org-persist.el:L552-4=3D #+begin_example elisp =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (lamb= da (cont) =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 (member cont (plist-get r :container))) #+end_example 5. =C2=A0=C2=A0 Another way to simplify `o-p--find-index' (and I think also ge= t =C2=A0=C2=A0 pretty close to achieving full consistency on the container sh= ape), =C2=A0=C2=A0 would be to put this bit from the start of `o-p--get-collectio= n' at =C2=A0=C2=A0 the start of `-read', `-write-all' and `-unregister' (or maybe= in a =C2=A0=C2=A0 (if (not inner) .. ) at the end of `o-p--normalize-container'?= ): #+begin_example elisp (unless (and (listp container) (listp (car container))) =C2=A0=C2=A0=C2=A0 (setq container (list container))) #+end_example =C2=A0=C2=A0 This would cover all calls to`o-p--find-index' and allow you t= o =C2=A0=C2=A0 revert the changes in both my patch and 7fd80991c3 I think. Th= is is =C2=A0=C2=A0 sort-of what I was thinking of wrt. rewriting =C2=A0=C2=A0 `o-p--normalize-container' in my original e-mail. -----