Re: [PATCH v2] org-attach.el: ID to path functions may return nil

[Ihor Radchenko <yantar92@posteo.net>]

[-- Attachment #1: Type: text/plain, Size: 3641 bytes --]

Max Nikulin <manikulin@gmail.com> 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.


[-- Attachment #2: v5-0001-org-attach.el-ID-to-path-functions-may-return-nil.patch --]
[-- Type: text/x-patch, Size: 10656 bytes --]

From 0be519670eeacdeab198a4160e48601d1fb22a49 Mon Sep 17 00:00:00 2001
Message-Id: <0be519670eeacdeab198a4160e48601d1fb22a49.1668479703.git.yantar92@posteo.net>
From: Max Nikulin <manikulin@gmail.com>
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 <xerusx@pm.me>
Link: https://list.orgmode.org/KC8PcypJapBpJQtJxM0kX5N7Z0THL2Lq6EQjBMzpw1-vgQf72egZ2JOIlTbPYiqAVD4MdSBhrhBZr2Ykf5DN1mocm1ANvvuKKZShlkgzKYM=@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-latex~ 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\\}" id)
+               (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
+=__=​/X/ID directory, where X is the first character in the ID string.
 
 * Version 9.5
 
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))))
 
 (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")))
 
 (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))
 
-(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)))
 
 (defun org-attach-check-absolute-path (dir)
   "Check if we have enough information to root the attachment directory.
-- 
2.35.1


[-- Attachment #3: Type: text/plain, Size: 224 bytes --]


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