emacs-orgmode@gnu.org archives
 help / color / mirror / code / Atom feed
From: Max Nikulin <manikulin@gmail.com>
To: emacs-orgmode@gnu.org
Subject: Re: [PATCH v2] org-attach.el: ID to path functions may return nil
Date: Sun, 13 Nov 2022 23:26:49 +0700	[thread overview]
Message-ID: <tkr5sb$gd2$1@ciao.gmane.io> (raw)
In-Reply-To: <87a64zjmz1.fsf@localhost>

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

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.

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

From 8a71e6262149f351ddb9a52d886d592de04736ae Mon Sep 17 00:00:00 2001
From: Max Nikulin <manikulin@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 <xerusx@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


  reply	other threads:[~2022-11-13 16:27 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-07-20 19:12 [BUG] org-attach-id-ts-folder-format fails on customized IDs [9.6 (9.6-??-2e9999783)] Janek F
2022-07-23  5:22 ` [PATCH] " Ihor Radchenko
2022-08-02 22:26   ` Janek F
2022-08-03 16:03   ` Max Nikulin
2022-08-03 22:25     ` Ihor Radchenko
2022-08-10 11:43       ` [PATCH v2] " Ihor Radchenko
2022-08-10 12:17         ` Max Nikulin
2022-08-10 13:23           ` [PATCH v3] " Ihor Radchenko
2022-08-10 15:18             ` Max Nikulin
2022-08-11  4:19               ` Ihor Radchenko
2022-08-11 14:43                 ` Max Nikulin
2022-08-13  5:29                   ` Ihor Radchenko
2022-08-13 16:25                     ` Max Nikulin
2022-08-14  4:00                       ` Ihor Radchenko
2022-10-02  9:14                         ` [PATCH v4] " Ihor Radchenko
2022-10-04 15:27                           ` Max Nikulin
2022-10-05  5:44                             ` Ihor Radchenko
2022-11-06  7:43                               ` Ihor Radchenko
2022-11-07 17:05                               ` [PATCH] org-attach.el: ID to path functions may return nil Max Nikulin
2022-11-08  5:08                                 ` Ihor Radchenko
2022-11-09 16:53                                   ` [PATCH v2] " Max Nikulin
2022-11-10  7:19                                     ` Ihor Radchenko
2022-11-13 16:26                                       ` Max Nikulin [this message]
2022-11-14  3:29                                         ` Ihor Radchenko
2022-11-14 16:59                                           ` Max Nikulin
2022-11-15  2:41                                             ` Ihor Radchenko
2022-11-15 16:41                                               ` Max Nikulin
2022-11-16  1:54                                                 ` Ihor Radchenko
2022-08-12 15:35 ` [BUG] org-attach-id-ts-folder-format fails on customized IDs [9.6 (9.6-??-2e9999783)] Max Nikulin
2022-08-12 16:08   ` Janek F
2022-08-13  5:17     ` Ihor Radchenko
2022-08-13 15:59     ` Max Nikulin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://www.orgmode.org/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='tkr5sb$gd2$1@ciao.gmane.io' \
    --to=manikulin@gmail.com \
    --cc=emacs-orgmode@gnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).