From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mp12.migadu.com ([2001:41d0:2:4a6f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by ms5.migadu.com with LMTPS id 864CAoL8cmPqWwAAbAwnHQ (envelope-from ) for ; Tue, 15 Nov 2022 03:42:10 +0100 Received: from aspmx1.migadu.com ([2001:41d0:2:4a6f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by mp12.migadu.com with LMTPS id QF4uAYL8cmPQJgEAauVa8A (envelope-from ) for ; Tue, 15 Nov 2022 03:42:10 +0100 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 128C43DA17 for ; Tue, 15 Nov 2022 03:42:08 +0100 (CET) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1oulsT-0002xS-BB; Mon, 14 Nov 2022 21:40:46 -0500 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 1oulsS-0002xK-8n for emacs-orgmode@gnu.org; Mon, 14 Nov 2022 21:40:44 -0500 Received: from mout01.posteo.de ([185.67.36.65]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1oulsH-0005yJ-0W for emacs-orgmode@gnu.org; Mon, 14 Nov 2022 21:40:43 -0500 Received: from submission (posteo.de [185.67.36.169]) by mout01.posteo.de (Postfix) with ESMTPS id 662C1240026 for ; Tue, 15 Nov 2022 03:40:28 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.net; s=2017; t=1668480028; bh=45tB4RyVlX8a2VY3u8PsvwVgtbYvlx9ZpBtEfEqe4mY=; h=From:To:Cc:Subject:Date:From; b=KbuiUubnPOPFoIMaoPBtiCMPPfH4fHSyUBfaLStoPmb5sBTvgPdI7TBd29CddaGyO SGmXfaFo8Irlb1UvzynGAJlFSFwi6Z3cB4Sigj4pd5qYw7bHymCA+mdwoDm5GI/rpw he34Y3pG1s+csMf5kxa+V1RZIXnZtpczN13Ro6suyLA/CfnlEF55cStv6j2SfCjvdO x5VM8SVVlHDVjqat1lKUKrTgEoTdYNufkt7lGqykzjn0uw1bTY47N8zHIl5jPRuNop oUfsmHc2TBqgQZPioQXSVlBejb367AUYZ6CKxf0IUpG3tw0Y8n/R1fR0akOmat1zOu UuwBgrhVxmf4A== Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4NB9QL3kbQz6tmq; Tue, 15 Nov 2022 03:40:26 +0100 (CET) From: Ihor Radchenko To: Max Nikulin Cc: emacs-orgmode@gnu.org Subject: Re: [PATCH v2] org-attach.el: ID to path functions may return nil In-Reply-To: <3326940f-f7a8-37f6-2dc2-f1f035f4c491@gmail.com> References: <871qtxhsm6.fsf@localhost> <87a68ce32u.fsf@localhost> <871qtodygs.fsf@localhost> <87v8qz9zui.fsf@localhost> <35cbf452-c3ed-d97f-db96-dcae57463eff@gmail.com> <87wnbc7ltm.fsf@localhost> <87bksn1nl1.fsf@localhost> <87sfk6zixs.fsf@localhost> <874jwiakpn.fsf@localhost> <87y1sm11aj.fsf@localhost> <87a64zjmz1.fsf@localhost> <87sfimfc2p.fsf@localhost> <3326940f-f7a8-37f6-2dc2-f1f035f4c491@gmail.com> Date: Tue, 15 Nov 2022 02:41:07 +0000 Message-ID: <878rkddjng.fsf@localhost> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" Received-SPF: pass client-ip=185.67.36.65; envelope-from=yantar92@posteo.net; helo=mout01.posteo.de X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, 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-Flow: FLOW_IN X-Migadu-Country: US ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=yhetil.org; s=key1; t=1668480129; 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:in-reply-to:in-reply-to: references:references:list-id:list-help:list-unsubscribe: list-subscribe:list-post:dkim-signature; bh=KRUFSGEfhzF8U2I9+9r4z20KJaUgMaMyq8kHM13fnRc=; b=L6HJ6Fr0JWNYkA3xV2lZmtcuevcWA61dZBx/lNsXbxZT/BioHLuN7jZRi05v1XI7lDKsyw KqfnVyRZkzOZjh8NWzp9q/R71XZYySYSXY/14WbzghBSrnD7B8rqHQ1Q6kyzAXurncZ8t/ 744uWAfXLuky/QU4eHIMKYSB/YOyrXXnNt2yOx7q1d3sGd11EqJadTbtmB2npba727x6IJ i5XjOfZhg9n6jHWiqJOqNXM9bJkYiNop5TfA/fyRH53uGWUOfFUZsHRqUKKSp+T4n7axrg zZvRbN6fWr6ou1sPP22p0ce+vNnXdKneNrAys26r9fDmEPidI+oijLweRRd1Og== ARC-Seal: i=1; s=key1; d=yhetil.org; t=1668480129; a=rsa-sha256; cv=none; b=eK1De5buotIQTffYjYfGe1YcpiglfR9STVa9AoxNWXzAQ6muRBCgHhNXoGcLzTz4qlsTjR iXGC8oU/sGBWFRgI/wHtySgdcGSYPQpPvTfywGpUnVtrWXopQ4sEhG939ka3OPfwf8KYDd MjjZI8mwgN4rqV6U3X7CLGAt22FRzZIBml4UFyNDy31GHnsPLRyDNKv09ltJPYDYcRcoCw 3+nSHJnfuqsuAvJHgo+tnYU3cFyWYhFXwoVygSRTqwh03uHcRJ5Yi6MyFqLV58GpEyLJw+ 2OnTJLvAVMThCjzPEXSZQVMYYf5Gz2YtJyWRGzK6t18Qt5+3IOGymnHUujPzNA== ARC-Authentication-Results: i=1; aspmx1.migadu.com; dkim=pass header.d=posteo.net header.s=2017 header.b=KbuiUubn; dmarc=pass (policy=none) header.from=posteo.net; 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" X-Migadu-Spam-Score: -8.54 Authentication-Results: aspmx1.migadu.com; dkim=pass header.d=posteo.net header.s=2017 header.b=KbuiUubn; dmarc=pass (policy=none) header.from=posteo.net; 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" X-Migadu-Queue-Id: 128C43DA17 X-Spam-Score: -8.54 X-Migadu-Scanner: scn0.migadu.com X-TUID: Hgi4wbJHd0a/ --=-=-= Content-Type: text/plain Max Nikulin writes: > On 14/11/2022 10:29, Ihor Radchenko wrote: >> >> I went through the patch and tried to clarify the wording. >> Especially in the defcustom docstring. > > I do not mind in general. > > Please, remove a stray space in the defcustom. Hmm. Done. I just have a habit to add space in the first item in a list because it helps auto-indentation. Leading space indicates that the list is not a function call and should not be indented as (function args...). It is not necessary in here though. >> I also added the dumb fallback to the default value. >> I feel that otherwise the description of too confusing. > > I am unsure concerning adding it to the default value. From my point of > view, it is better to ask user to clarify their intention. I believe > that the obscure error message is the real bug, not that Org can not > handle too short ID by default. We have fixed the error message in this patch as well. Handling too short IDs is a different issue indeed, but why not to fix it as well? > I think, for new users default value should include just strict variants > of `org-attach-id-uuid-folder-format' and > `org-attach-id-ts-folder-format' that checks ID format as in the example > in ORG-NEWS. Changing of `org-id-method' will not cause non-optimal > subdir layout for the same value of 'org-attach-id-to-path-function-list'. > > It will cause a problem for users who changed `org-id-method' in the > past, so they have either XX/timestamp or YYYYMM/uuid subdirectories. It > may be solved by adding original variants of > `org-attach-id-uuid-folder-format' and `org-attach-id-ts-folder-format', > but perhaps it is better to add predicates for "wrong" style (UUID or > Org for timestamp and vice versa) just for backward compatibility. >> + org-attach-id-ts-folder-format >> + org-attach-id-fallback-folder-format) > > If strict variants of functions were used above then non-standard IDs > would be isolated in the directory returned by the next entry Good point. What about using the value you provided in the NEWS as an actual default? > It should be responsibility of the user to setup subdirs layout for > non-standard ID format. The dumb fallback is intended as an example and > a variant for those who do not have a lot of attachments and do not care > where they are stored. I'd say that the probability to get too many attachments with short IDs is pretty low. We will help more users if we make Org handle short IDs by default. >> +#+begin_src emacs-lisp >> +(setq org-attach-id-to-path-function-list >> + '(;; When ID looks like an UUIDs or Org internal ID, use >> + ;; `org-attach-id-uuid-folder-format'. >> + (lambda (id) >> + (and (or (org-uuidgen-p id) >> + (string-match-p "[0-9a-z]\\{12\\}" id)) >> + (org-attach-id-uuid-folder-format id))) >> + ;; When ID looks like a timestap-based ID. Group by year-month >> + ;; folders. >> + (lambda (id) >> + (and (string-match-p "[0-9]\\{8\\}T[0-9]\\{6\\}\.[0-9]\\{6\\}" id) >> + (org-attach-id-ts-folder-format id))) >> + ;; Any other ID goes into "important" folder. >> + (lambda (id) (format "important/%s/%s" (substring id 0 1) id)))) > > `org-attach-id-uuid-folder-format' and `org-attach-id-ts-folder-format' > here were added for users changed `org-id-method' in the past and so > having mixed subdirs layout with UUIDs in 6 character prefix directories > or timestamps in two characters folders. Agree. Fixed. See the attached updated patch. --=-=-= Content-Type: text/x-patch; charset=utf-8 Content-Disposition: inline; filename=v5-0001-org-attach.el-ID-to-path-functions-may-return-nil.patch Content-Transfer-Encoding: quoted-printable >From 0be519670eeacdeab198a4160e48601d1fb22a49 Mon Sep 17 00:00:00 2001 Message-Id: <0be519670eeacdeab198a4160e48601d1fb22a49.1668479703.git.yantar= 92@posteo.net> From: Max Nikulin Date: Mon, 7 Nov 2022 23:48:02 +0700 Subject: [PATCH v5] org-attach.el: ID to path functions may return nil * lisp/org-attach.el (org-attach-dir-from-id): Ignore nil values returned by entries from `org-attach-id-to-path-function-list'. (org-attach-dir-get-create): Signal an error suggesting customization of `org-attach-id-to-path-function-list' if all ID-to-path functions return nil. (org-attach-id-to-path-function-list): Add to the docstring examples how to handle unusual IDs. (org-attach-id-uuid-folder-format, org-attach-id-ts-folder-format): Return nil if ID is too short. (org-attach-id-fallback-folder-format): New function that may be added as the last element of `org-attach-id-path-function-list' to handle unexpectedly short IDs. * etc/ORG-NEWS: Advertise the change. Earlier an obscure error like 'org-attach-id-ts-folder-format: Args out of range: "ftt", 0, 6' was signalled in the case of unexpectedly short ID. Reported-by: Janek F Link: https://list.orgmode.org/KC8PcypJapBpJQtJxM0kX5N7Z0THL2Lq6EQjBMzpw1-v= gQf72egZ2JOIlTbPYiqAVD4MdSBhrhBZr2Ykf5DN1mocm1ANvvuKKZShlkgzKYM=3D@pm.me --- etc/ORG-NEWS | 44 +++++++++++++++++++ lisp/org-attach.el | 105 +++++++++++++++++++++++++++++---------------- 2 files changed, 112 insertions(+), 37 deletions(-) diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS index 04b5be64a..1effc2e52 100644 --- a/etc/ORG-NEWS +++ b/etc/ORG-NEWS @@ -604,6 +604,50 @@ With the new customization option ~org-texinfo-with-la= tex~ set to (its default value) ~'detect~, if the system runs Texinfo 6.8 (3 July 2021) or newer, Org will export all LaTeX fragments and environments using Texinfo ~@math~ and ~@displaymath~ commands respectively. +*** More flexible ~org-attach-id-to-path-function-list~ + +List entries may return nil if they are unable to handle passed ID. +So responsibility is passed to the next item in the list. Default +entries ~org-attach-id-uuid-folder-format~ and +~org-attach-id-ts-folder-format~ now return nil in the case of too +short ID. Earlier it caused an obscure error. + +After the change, error text suggests to adjust +~org-attach-id-to-path-function-list~ value. The +~org-attach-dir-from-id~ function is adapted to ignore nil values and +to take first non-nil value instead of the value returned by first +~org-attach-id-to-path-function-list~ item. + +New policy allows to mix different ID styles while keeping subfolder layout +suited best for each one. For example, one can use the following +snippet to allow multiple different ID formats in Org files. + +#+begin_src emacs-lisp +(setq org-attach-id-to-path-function-list + '(;; When ID looks like an UUIDs or Org internal ID, use + ;; `org-attach-id-uuid-folder-format'. + (lambda (id) + (and (or (org-uuidgen-p id) + (string-match-p "[0-9a-z]\\{12\\}" id)) + (org-attach-id-uuid-folder-format id))) + ;; When ID looks like a timestap-based ID. Group by year-month + ;; folders. + (lambda (id) + (and (string-match-p "[0-9]\\{8\\}T[0-9]\\{6\\}\.[0-9]\\{6\\}" i= d) + (org-attach-id-ts-folder-format id))) + ;; Fallback to handle previous defaults. + org-attach-id-uuid-folder-format + org-attach-id-ts-folder-format + ;; Any other ID goes into "important" folder. + (lambda (id) (format "important/%s/%s" (substring id 0 1) id)))) +#+end_src + +If you have 1 or 2 characters long IDs and you do not care what +subdirs are created for longer IDs (that are neither UUIDs nor +timestamp-based) then you may just append the +~org-attach-id-fallback-folder-format~ to +~org-attach-id-to-path-function-list~. It directs attachments to +=3D__=3D=E2=80=8B/X/ID directory, where X is the first character in the ID= string. =20 * Version 9.5 =20 diff --git a/lisp/org-attach.el b/lisp/org-attach.el index ef183474e..f85811dc7 100644 --- a/lisp/org-attach.el +++ b/lisp/org-attach.el @@ -166,28 +166,57 @@ (defun org-attach-id-uuid-folder-format (id) "Translate an UUID ID into a folder-path. Default format for how Org translates ID properties to a path for attachments. Useful if ID is generated with UUID." - (format "%s/%s" - (substring id 0 2) - (substring id 2))) + (and (< 2 (length id)) + (format "%s/%s" + (substring id 0 2) + (substring id 2)))) =20 (defun org-attach-id-ts-folder-format (id) "Translate an ID based on a timestamp to a folder-path. Useful way of translation if ID is generated based on ISO8601 timestamp. Splits the attachment folder hierarchy into year-month, the rest." - (format "%s/%s" - (substring id 0 6) - (substring id 6))) - -(defcustom org-attach-id-to-path-function-list '(org-attach-id-uuid-folder= -format - org-attach-id-ts-folder-format) - "List of functions parsing an ID string into a folder-path. -The first function in this list defines the preferred function -which will be used when creating new attachment folders. All -functions of this list will be tried when looking for existing -attachment folders based on ID." + (and (< 6 (length id)) + (format "%s/%s" + (substring id 0 6) + (substring id 6)))) + +(defun org-attach-id-fallback-folder-format (id) + "Return \"__/X/ID\" folder path as a dumb fallback. +X is the first character in the ID string. + +This function may be appended to `org-attach-id-path-function-list' to +provide a fallback for non-standard ID values that other functions in +`org-attach-id-path-function-list' are unable to handle. For example, +when the ID is too short for `org-attach-id-ts-folder-format'. + +However, we recommend to define a more specific function spreading +entries over multiple folders. This function may create a large +number of entries in a single folder, which may cause issues on some +systems." + (format "__/%s/%s" (substring id 0 1) id)) + +(defcustom org-attach-id-to-path-function-list + '(org-attach-id-uuid-folder-format + org-attach-id-ts-folder-format + org-attach-id-fallback-folder-format) + "List of functions used to derive attachment path from an ID string. +The functions are called with a single ID argument until the return +value is an existing folder. If no folder has been created yet for +the given ID, then the first non-nil value defines the attachment +dir to be created. + +Usually, the ID format passed to the functions is defined by +`org-id-method'. It is advised that the first function in the list do +not generate all the attachment dirs inside the same parent dir. Some +file systems may have performance issues in such scenario. + +Care should be taken when customizing this variable. Previously +created attachment folders might not be correctly mapped upon removing +functions from the list. Then, Org will not be able to detect the +existing attachments." :group 'org-attach - :package-version '(Org . "9.3") + :package-version '(Org . "9.6") :type '(repeat (function :tag "Function with ID as input"))) =20 (defvar org-attach-after-change-hook nil @@ -360,7 +389,7 @@ (defun org-attach-dir (&optional create-if-not-exists-p= no-fs-check) (org-attach-check-absolute-path attach-dir)) ((setq id (org-entry-get nil "ID" org-attach-use-inheritance)) (org-attach-check-absolute-path nil) - (setq attach-dir (org-attach-dir-from-id id 'try-all)))) + (setq attach-dir (org-attach-dir-from-id id 'existing)))) (if no-fs-check attach-dir (when (and attach-dir (file-directory-p attach-dir)) @@ -381,7 +410,11 @@ (defun org-attach-dir-get-create () (setq answer (read-char-exclusive))) (cond ((or (eq org-attach-preferred-new-method 'id) (eq answer ?1)) - (setq attach-dir (org-attach-dir-from-id (org-id-get nil t)))) + (let ((id (org-id-get nil t))) + (or (setq attach-dir (org-attach-dir-from-id id)) + (error "Failed to get folder for id %s, \ +adjust `org-attach-id-to-path-function-list'" + id)))) ((or (eq org-attach-preferred-new-method 'dir) (eq answer ?2)) (setq attach-dir (org-attach-set-directory))) ((eq org-attach-preferred-new-method 'nil) @@ -392,27 +425,25 @@ (defun org-attach-dir-get-create () (make-directory attach-dir t)) attach-dir)) =20 -(defun org-attach-dir-from-id (id &optional try-all) +(defun org-attach-dir-from-id (id &optional existing) "Return a folder path based on `org-attach-id-dir' and ID. -If TRY-ALL is non-nil, try all id-to-path functions in -`org-attach-id-to-path-function-list' and return the first path -that exist in the filesystem, or the first one if none exist. -Otherwise only use the first function in that list." - (let ((attach-dir-preferred (expand-file-name - (funcall (car org-attach-id-to-path-function-list) id) - (expand-file-name org-attach-id-dir)))) - (if try-all - (let ((attach-dir attach-dir-preferred) - (fun-list (cdr org-attach-id-to-path-function-list))) - (while (and fun-list (not (file-directory-p attach-dir))) - (setq attach-dir (expand-file-name - (funcall (car fun-list) id) - (expand-file-name org-attach-id-dir))) - (setq fun-list (cdr fun-list))) - (if (file-directory-p attach-dir) - attach-dir - attach-dir-preferred)) - attach-dir-preferred))) +Try id-to-path functions in `org-attach-id-to-path-function-list' +ignoring nils. If EXISTING is non-nil, then return the first path +found in the filesystem. Otherwise return the first non-nil value." + (let ((fun-list org-attach-id-to-path-function-list) + (base-dir (expand-file-name org-attach-id-dir)) + preferred first) + (while (and fun-list + (not preferred)) + (let* ((name (funcall (car fun-list) id)) + (candidate (and name (expand-file-name name base-dir)))) + (setq fun-list (cdr fun-list)) + (when candidate + (if (or (not existing) (file-directory-p candidate)) + (setq preferred candidate) + (unless first + (setq first candidate)))))) + (or preferred first))) =20 (defun org-attach-check-absolute-path (dir) "Check if we have enough information to root the attachment directory. --=20 2.35.1 --=-=-= Content-Type: text/plain -- Ihor Radchenko // yantar92, Org mode contributor, Learn more about Org mode at . Support Org development at , or support my work at --=-=-=--