On 10/11/2022 14:19, Ihor Radchenko wrote:
Max Nikulin writes:
P.S. At first I believed that you have some objections concerning
changed role of the first function in the list, not just how it is
documented.
I had. Most importantly, because we are changing the existing meaning of
`org-attach-id-to-path-function-list'. However, the only scenario where
it actually matters is the bug report at hand. So, there will be no
regression.
Since the change relaxes requirements for
`org-attach-id-to-path-function-list' members, it should not be a
breaking change. I have realized that it allows to mix e.g. UUID and
timestamp IDs in the single file and still follow storage policy for
each one. Earlier change of `org-id-method' caused subdirs for
timestamps created like for UUID or vice versa. Actually with a
cumbersome function it was possible earlier as well, but now particular
ID to subdir functions may be unaware of other ID methods.
However, please add NEWS entry detailing the change in
`org-attach-id-to-path-function-list' to the next version of the patch.
See the attachment.
From 8a71e6262149f351ddb9a52d886d592de04736ae Mon Sep 17 00:00:00 2001
From: Max Nikulin <maniku...@gmail.com>
Date: Mon, 7 Nov 2022 23:48:02 +0700
Subject: [PATCH v3] 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 <xer...@pm.me>
https://list.orgmode.org/KC8PcypJapBpJQtJxM0kX5N7Z0THL2Lq6EQjBMzpw1-vgQf72egZ2JOIlTbPYiqAVD4MdSBhrhBZr2Ykf5DN1mocm1ANvvuKKZShlkgzKYM=@pm.me
---
etc/ORG-NEWS | 40 +++++++++++++++++++
lisp/org-attach.el | 98 ++++++++++++++++++++++++++++++----------------
2 files changed, 105 insertions(+), 33 deletions(-)
diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
index 04b5be64a..e0cf3f2ce 100644
--- a/etc/ORG-NEWS
+++ b/etc/ORG-NEWS
@@ -604,6 +604,46 @@ 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 of 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. It requires customization however:
+
+#+begin_example
+'(
+ ;; Spread UUIDs and Org internal IDs over folders
+ ;; splitting first two ID characters.
+ (lambda (id)
+ (and (or (org-uuidgen-p id)
+ (string-match-p "[0-9a-z]\\{12\\}" id))
+ (org-attach-id-uuid-folder-format id)))
+ ;; Group timestap-based IDs by year-month.
+ (lambda (id)
+ (and (string-match-p "[0-9]\\{8\\}T[0-9]\\{6\\}\.[0-9]\\{6\\}" id))
+ (org-attach-id-ts-folder-format id))
+ ;; Handle handcrafted IDs.
+ (lambda (id) (format "important/%s/%s" (substring id 0 1) id))
+ ;; Subfolder layouts may be mixed for earlier created attachments
+ ;; if `org-id-method' was changed.
+ org-attach-id-uuid-folder-format
+ org-attach-id-ts-folder-format)
+#+end_example
+
+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~
+new function to ~org-attach-id-to-path-function-list~. It directs attachment
+to the ID subfolder of the =__= directory.
* Version 9.5
diff --git a/lisp/org-attach.el b/lisp/org-attach.el
index ef183474e..6332233cd 100644
--- a/lisp/org-attach.el
+++ b/lisp/org-attach.el
@@ -166,26 +166,56 @@ (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)))
+ (and (< 6 (length id))
+ (format "%s/%s"
+ (substring id 0 6)
+ (substring id 6))))
+
+(defun org-attach-id-fallback-folder-format (id)
+ "Use \"__/ID\" folder path as a dumb fallback.
+May be appended to `org-attach-id-path-function-list'
+to avoid errors due to customized ID values that other functions
+are unable to handle. An example is too short ID for
+`org-attach-id-ts-folder-format'. However it is better to define
+a more specific function spreading entries over subfolders."
+ (format "__/%s" id))
(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."
+ "List of functions tried to get a folder path for an ID string.
+Functions are called in order till some of them returns an existing
+folder. If no folder has been created yet for the given ID
+then first value defines folder name that will be created
+to store an attachment. Nil values are skipped.
+
+Preferred storage policy depends on `org-id-method' and should be chosen
+to avoid excessive number of entries in a single directory. List
+of functions allows to handle the case of mixed ID styles after
+change of ID generator.
+
+If format of some ID strings differs from `org-id-method' you may
+add `org-attach-id-fallback-folder-format' to the end of the list.
+A better option is to keep attachments inside a dedicated folder.
+A function like the following one may be added as first element
+of the list.
+
+ (defun my/attach-id-custom-folder-format (id)
+ (let ((case-fold-search t))
+ (unless
+ (or (org-uuidgen-p id)
+ (string-match-p \"[0-9a-z]\\\\=\\{12\\\\}\" id)
+ (string-match-p \"[0-9]\\\\=\\{8\\\\}T[0-9]\\\\=\\{6\\\\}\\.[0-9]\\\\=\\{6\\\\}\" id))
+ (format \"important/%s/%s\" (substring id 0 1) id))))"
:group 'org-attach
:package-version '(Org . "9.3")
:type '(repeat (function :tag "Function with ID as input")))
@@ -360,7 +390,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 +411,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 +426,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-nill 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.25.1
