[BUG] org-attach-id-ts-folder-format fails on customized IDs [9.6 (9.6-??-2e9999783)]

[BUG] org-attach-id-ts-folder-format fails on customized IDs [9.6 (9.6-??-2e9999783)]

[Janek F]

When setting org-id-method to 'ts or 'org,
org-attach seems to use org-attach-id-ts-folder-format
to create its hierarchy.

However I tend to customize IDs for important files by hand,
causing any attempt to use org-attach on that file to fail
if the ID is shorter than six characters:

    org-attach-id-ts-folder-format: Args out of range: "ftt", 0, 6

This method should be adjusted to handle non-ts-ids just as well,
as org-id-method does not dictate the format of existing ids.

Regards,
Janek

[PATCH] Re: [BUG] org-attach-id-ts-folder-format fails on customized IDs [9.6 (9.6-??-2e9999783)]

[Ihor Radchenko]

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

Janek F <xerusx@pm.me> writes:

> When setting org-id-method to 'ts or 'org,
> org-attach seems to use org-attach-id-ts-folder-format
> to create its hierarchy.
>
> However I tend to customize IDs for important files by hand,
> causing any attempt to use org-attach on that file to fail
> if the ID is shorter than six characters:
>
>     org-attach-id-ts-folder-format: Args out of range: "ftt", 0, 6
>
> This method should be adjusted to handle non-ts-ids just as well,
> as org-id-method does not dictate the format of existing ids.

Thanks for reporting!
Tentative patch is attached.

I have added some fallbacks for the default attach folder formatters, so
that they can work when the ID does not conform to specific format.

This change is somewhat opinionated, so feel free to suggest alternative
solutions/fallback options.

Best,
Ihor


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-org-attach-dir-from-id-Do-not-rely-on-ID-being-over-.patch --]
[-- Type: text/x-patch, Size: 2341 bytes --]

From e004752ba39f0d328645a1f6053ad3ce3b06ac28 Mon Sep 17 00:00:00 2001
Message-Id: <e004752ba39f0d328645a1f6053ad3ce3b06ac28.1658553404.git.yantar92@gmail.com>
From: Ihor Radchenko <yantar92@gmail.com>
Date: Sat, 23 Jul 2022 13:13:24 +0800
Subject: [PATCH] org-attach-dir-from-id: Do not rely on ID being over 6 chars
 long

* lisp/org-attach.el (org-attach-id-uuid-folder-format): Fall back to
using ID md5 hash when the ID contains 2 chars or less and cannot be
split into the "xy/z...." path.
(org-attach-id-ts-folder-format): Fall back to "unknown/ID" path
format when the ID contains less than 7 chars and cannot be split into
the "YYYYMM/rest" path.

Fixes https://orgmode.org/list/KC8PcypJapBpJQtJxM0kX5N7Z0THL2Lq6EQjBMzpw1-vgQf72egZ2JOIlTbPYiqAVD4MdSBhrhBZr2Ykf5DN1mocm1ANvvuKKZShlkgzKYM=@pm.me
---
 lisp/org-attach.el | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/lisp/org-attach.el b/lisp/org-attach.el
index 36c21b702..7c72fd7ee 100644
--- a/lisp/org-attach.el
+++ b/lisp/org-attach.el
@@ -159,19 +159,28 @@ (defcustom org-attach-archive-delete nil
 (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)))
+attachments.  Useful if ID is generated with UUID.
+
+When ID is too short (less than 3 chars), use its md5 hash to create
+the path."
+  (if (< (length id) 3)
+      (org-attach-id-uuid-folder-format (md5 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)))
+year-month, the rest.
+
+When ID is too short (less than 7 chars), return \"unknown/ID\"."
+  (if (< (length id) 7)
+      (format "unknown/%s" id)
+    (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)
-- 
2.35.1

Re: [PATCH] Re: [BUG] org-attach-id-ts-folder-format fails on customized IDs [9.6 (9.6-??-2e9999783)]

[Janek F]

Just now had a closer look at your patch,
and I don't like the hashing.
My whole idea customizing ids is to make the file structure useful by itself, too.
This is how I patched it for me:

  (defun xf/org-attach-id-folder-format (id)
    "Translate any ID into a folder-path."
    (format "%s/%s"
            (substring id 0 2)
            (if (> (seq-length id) 2) (substring id 2) id))
    )
  (setq org-attach-id-to-path-function-list '(xf/org-attach-id-folder-format))


------- Original Message -------
Ihor Radchenko <yantar92@gmail.com> schrieb am Samstag, 23. Juli 2022 um 07:21:


> Janek F xerusx@pm.me writes:
>
> > When setting org-id-method to 'ts or 'org,
> > org-attach seems to use org-attach-id-ts-folder-format
> > to create its hierarchy.
> >
> > However I tend to customize IDs for important files by hand,
> > causing any attempt to use org-attach on that file to fail
> > if the ID is shorter than six characters:
> >
> > org-attach-id-ts-folder-format: Args out of range: "ftt", 0, 6
> >
> > This method should be adjusted to handle non-ts-ids just as well,
> > as org-id-method does not dictate the format of existing ids.
>
>
> Thanks for reporting!
> Tentative patch is attached.
>
> I have added some fallbacks for the default attach folder formatters, so
> that they can work when the ID does not conform to specific format.
>
> This change is somewhat opinionated, so feel free to suggest alternative
> solutions/fallback options.
>
> Best,
> Ihor

Re: [PATCH] Re: [BUG] org-attach-id-ts-folder-format fails on customized IDs [9.6 (9.6-??-2e9999783)]

[Max Nikulin]

On 23/07/2022 12:22, Ihor Radchenko wrote:
> Tentative patch is attached.

> +  (if (< (length id) 3)
> +      (org-attach-id-uuid-folder-format (md5 id))

Ihor, I am afraid of collisions due to short input to md5. Long hash 
value gives false impression of high entropy but actually there are not 
so many variants for 1 or 2 characters strings. Either there is no point 
in making long name from short id or random string of appropriate length 
should be used instead.

Re: [PATCH] Re: [BUG] org-attach-id-ts-folder-format fails on customized IDs [9.6 (9.6-??-2e9999783)]

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

> On 23/07/2022 12:22, Ihor Radchenko wrote:
>> Tentative patch is attached.
>
>> +  (if (< (length id) 3)
>> +      (org-attach-id-uuid-folder-format (md5 id))
>
> Ihor, I am afraid of collisions due to short input to md5. Long hash 
> value gives false impression of high entropy but actually there are not 
> so many variants for 1 or 2 characters strings. Either there is no point 
> in making long name from short id or random string of appropriate length 
> should be used instead.

Random is not an option. If we use random string, how will we get the
right attachment path on a second call? This function must return the
same value given the same input.

Having said that, I do agree (taking into account the other comment)
that md5 may not be a very good idea. Maybe simply something like
"__/id". Ideally, we should be able to recover the original id from the
path to attachment.

Best,
Ihor

[PATCH v2] Re: [BUG] org-attach-id-ts-folder-format fails on customized IDs [9.6 (9.6-??-2e9999783)]

[Ihor Radchenko]

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

Ihor Radchenko <yantar92@gmail.com> writes:

>> Ihor, I am afraid of collisions due to short input to md5. Long hash 
>> value gives false impression of high entropy but actually there are not 
>> so many variants for 1 or 2 characters strings. Either there is no point 
>> in making long name from short id or random string of appropriate length 
>> should be used instead.
>
> Random is not an option. If we use random string, how will we get the
> right attachment path on a second call? This function must return the
> same value given the same input.
>
> Having said that, I do agree (taking into account the other comment)
> that md5 may not be a very good idea. Maybe simply something like
> "__/id". Ideally, we should be able to recover the original id from the
> path to attachment.

I have updated the patch to use "__/id" when id is too short.
Any objections?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: v2-0001-org-attach-dir-from-id-Do-not-rely-on-ID-being-ov.patch --]
[-- Type: text/x-patch, Size: 2310 bytes --]

From 7ca5fe66c53a1399943a2e2d4a1675ca8cd14194 Mon Sep 17 00:00:00 2001
Message-Id: <7ca5fe66c53a1399943a2e2d4a1675ca8cd14194.1660131747.git.yantar92@gmail.com>
From: Ihor Radchenko <yantar92@gmail.com>
Date: Sat, 23 Jul 2022 13:13:24 +0800
Subject: [PATCH v2] org-attach-dir-from-id: Do not rely on ID being over 6
 chars long

* lisp/org-attach.el (org-attach-id-uuid-folder-format): Fall back to
"--/ID" when the ID contains 2 chars or less and cannot be split into
the "xy/z...." path.
(org-attach-id-ts-folder-format): Fall back to "unknown/ID" path
format when the ID contains less than 7 chars and cannot be split into
the "YYYYMM/rest" path.

Fixes https://orgmode.org/list/KC8PcypJapBpJQtJxM0kX5N7Z0THL2Lq6EQjBMzpw1-vgQf72egZ2JOIlTbPYiqAVD4MdSBhrhBZr2Ykf5DN1mocm1ANvvuKKZShlkgzKYM=@pm.me
---
 lisp/org-attach.el | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/lisp/org-attach.el b/lisp/org-attach.el
index fe49af6f3..bb7c787f7 100644
--- a/lisp/org-attach.el
+++ b/lisp/org-attach.el
@@ -159,19 +159,28 @@ (defcustom org-attach-archive-delete nil
 (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)))
+attachments.  Useful if ID is generated with UUID.
+
+When ID is too short (less than 3 chars), use its md5 hash to create
+the path."
+  (if (< (length id) 3)
+      (format "--/%s" 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)))
+year-month, the rest.
+
+When ID is too short (less than 7 chars), return \"unknown/ID\"."
+  (if (< (length id) 7)
+      (format "unknown/%s" id)
+    (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)
-- 
2.35.1


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



-- 
Ihor Radchenko,
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

Re: [PATCH v2] Re: [BUG] org-attach-id-ts-folder-format fails on customized IDs [9.6 (9.6-??-2e9999783)]

[Max Nikulin]

On 10/08/2022 18:43, Ihor Radchenko wrote:
> Ihor Radchenko writes:
> 
> I have updated the patch to use "__/id" when id is too short.
> Any objections?
> 
> +When ID is too short (less than 3 chars), use its md5 hash to create

Misleading docs, you do not use md5.

> +the path."
> +  (if (< (length id) 3)
> +      (format "--/%s" id)

Please, do not use path components starting with dash, it is terrible 
for CLI tools. By the way, you promised underscores, not dashes.

> +    (format "%s/%s"
> +	    (substring id 0 2)
> +	    (substring id 2))))

Ihor, I have not look into the code around, so my suggestions may have 
no sense.

Is it possible to pass empty string as ID to these functions? Should it 
be explicitly checked?

What if ID contains "/" that can not be used in file name? Windows has 
more forbidden characters.

Do you expect any problem if here (and for timestamp-based ids) 
directory component is just padded with some character (unsure what can 
better than underscore) to required length?

"x" -> "_x/x" or "______x/x"
"xy" -> "xy/xy" or "_____xy/xy"
etc.

 From my point of view it might be a bit better than "__" and "unknown".

[PATCH v3] Re: [BUG] org-attach-id-ts-folder-format fails on customized IDs [9.6 (9.6-??-2e9999783)]

[Ihor Radchenko]

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

Max Nikulin <manikulin@gmail.com> writes:

> On 10/08/2022 18:43, Ihor Radchenko wrote:
>> Ihor Radchenko writes:
>> 
>> I have updated the patch to use "__/id" when id is too short.
>> Any objections?
>> 
>> +When ID is too short (less than 3 chars), use its md5 hash to create
>
> Misleading docs, you do not use md5.

Oops. Will fix.

>> +the path."
>> +  (if (< (length id) 3)
>> +      (format "--/%s" id)
>
> Please, do not use path components starting with dash, it is terrible 
> for CLI tools. By the way, you promised underscores, not dashes.

Why?

I have no opinion about the possible dummy folder name, except that it
should fit the general pattern we already have "xx/..." or "YYYYMM/...".

>> +    (format "%s/%s"
>> +	    (substring id 0 2)
>> +	    (substring id 2))))
>
> Ihor, I have not look into the code around, so my suggestions may have 
> no sense.
>
> Is it possible to pass empty string as ID to these functions? Should it 
> be explicitly checked?

It should not be possible under normal operation.

> What if ID contains "/" that can not be used in file name? Windows has 
> more forbidden characters.

Then, creating the attachment dir will fail in front of the user. I am
not sure if we really need to do much about this. At least, not until we
get a bug report.

If we really need to solve the edge cases with attach dir generation, we
may need to change the overall design in org-attach/org-id to something
more restrictive. I am not sure if it is worth the effort compared to
other directions where to improve Org.

> Do you expect any problem if here (and for timestamp-based ids) 
> directory component is just padded with some character (unsure what can 
> better than underscore) to required length?
>
> "x" -> "_x/x" or "______x/x"
> "xy" -> "xy/xy" or "_____xy/xy"
> etc.
>
>  From my point of view it might be a bit better than "__" and "unknown".

Does it make sense?
Timestamp-based IDs are like 20220810T210121.478800 by default.
Then, the top-level folders will be like 202208/...
If the actual format is different, we cannot possibly expect the
timestamp to follow the same pattern, which is why I chose "unknown"
referring to unknown date.

On the other hand, if an ID has more than 6 characters, it will generate
nonsense top-level folder with or without the patch. We could go further
and match the ID against [1-9][0-9]\{5\} and put the folder into
"unknown/ID" instead, but that would be a breaking change.

In summary, I am more of less neutral towards this fallback format,
except that it would be nice to be able to recover the ID from the
directory path. Padding should be recoverable.

I slightly dislike the "___xx" compared to "______" because it will
create a proliferation of top-level folders as opposed to cramping the
non-standard IDs into a single "______" folder.

Attaching the updated patch.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: v3-0001-org-attach-dir-from-id-Do-not-rely-on-ID-being-ov.patch --]
[-- Type: text/x-patch, Size: 2286 bytes --]

From 50d0f9de0acdf5d67b797476816cbeb40b19f554 Mon Sep 17 00:00:00 2001
Message-Id: <50d0f9de0acdf5d67b797476816cbeb40b19f554.1660137585.git.yantar92@gmail.com>
From: Ihor Radchenko <yantar92@gmail.com>
Date: Sat, 23 Jul 2022 13:13:24 +0800
Subject: [PATCH v3] org-attach-dir-from-id: Do not rely on ID being over 6
 chars long

* lisp/org-attach.el (org-attach-id-uuid-folder-format): Fall back to
"__/ID" when the ID contains 2 chars or less and cannot be split into
the "xy/z...." path.
(org-attach-id-ts-folder-format): Fall back to "______/ID" path format
when the ID contains less than 7 chars and cannot be split into the
"YYYYMM/rest" path.

Fixes https://orgmode.org/list/KC8PcypJapBpJQtJxM0kX5N7Z0THL2Lq6EQjBMzpw1-vgQf72egZ2JOIlTbPYiqAVD4MdSBhrhBZr2Ykf5DN1mocm1ANvvuKKZShlkgzKYM=@pm.me
---
 lisp/org-attach.el | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/lisp/org-attach.el b/lisp/org-attach.el
index fe49af6f3..0f5d5af82 100644
--- a/lisp/org-attach.el
+++ b/lisp/org-attach.el
@@ -159,19 +159,27 @@ (defcustom org-attach-archive-delete nil
 (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)))
+attachments.  Useful if ID is generated with UUID.
+
+When ID is too short (less than 3 chars), return \"__/ID\"."
+  (if (< (length id) 3)
+      (format "__/%s" 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)))
+year-month, the rest.
+
+When ID is too short (less than 7 chars), return \"______/ID\"."
+  (if (< (length id) 7)
+      (format "______/%s" id)
+    (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)
-- 
2.35.1


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


-- 
Ihor Radchenko,
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

Re: [PATCH v3] Re: [BUG] org-attach-id-ts-folder-format fails on customized IDs [9.6 (9.6-??-2e9999783)]

[Max Nikulin]

On 10/08/2022 20:23, Ihor Radchenko wrote:
> Max Nikulin writes:
> 
>>> +the path."
>>> +  (if (< (length id) 3)
>>> +      (format "--/%s" id)
>>
>> Please, do not use path components starting with dash, it is terrible
>> for CLI tools. By the way, you promised underscores, not dashes.
> 
> Why?

It is inconvenient in interactive sessions (in scripts appropriate 
measures must be taken anyway), the following does not list content of 
the "--" directory:

     ls -l --

> I slightly dislike the "___xx" compared to "______" because it will
> create a proliferation of top-level folders as opposed to cramping the
> non-standard IDs into a single "______" folder.

I believed that proliferation of folders is for purpose. Intermediate 
directories allows to avoid excessive number of files in single 
directory. ext4 with directory tree index usually is not the case, but 
other filesystems may have rather poor performance when too much files 
are stuffed into single folder. Some applications become really slow for 
huge directories.

On the other hand there are not so many distinct file names that may 
fall into "__" directory, so perhaps it is OK. Concerning "unknown" I am 
less sure.

I do not have strong opinion how to properly deal with short IDs.

Re: [PATCH v3] Re: [BUG] org-attach-id-ts-folder-format fails on customized IDs [9.6 (9.6-??-2e9999783)]

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

>>> Please, do not use path components starting with dash, it is terrible
>>> for CLI tools. By the way, you promised underscores, not dashes.
>> 
>> Why?
>
> It is inconvenient in interactive sessions (in scripts appropriate 
> measures must be taken anyway), the following does not list content of 
> the "--" directory:
>
>      ls -l --

Thanks for the clarification.

>> I slightly dislike the "___xx" compared to "______" because it will
>> create a proliferation of top-level folders as opposed to cramping the
>> non-standard IDs into a single "______" folder.
>
> I believed that proliferation of folders is for purpose. Intermediate 
> directories allows to avoid excessive number of files in single 
> directory. ext4 with directory tree index usually is not the case, but 
> other filesystems may have rather poor performance when too much files 
> are stuffed into single folder. Some applications become really slow for 
> huge directories.

I was referring to TS style timestamp resolver here. It is designed to
group directories by creation time, not to distribute them homogeneously.

> On the other hand there are not so many distinct file names that may 
> fall into "__" directory, so perhaps it is OK.

Yes. Such short IDs are not supposed to be common and even if they are,
the maximum possible number of such IDs is relatively small. 


-- 
Ihor Radchenko,
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

Re: [PATCH v3] Re: [BUG] org-attach-id-ts-folder-format fails on customized IDs [9.6 (9.6-??-2e9999783)]

[Max Nikulin]

On 11/08/2022 11:19, Ihor Radchenko wrote:
> Max Nikulin writes:
> 
>>> I slightly dislike the "___xx" compared to "______" because it will
>>> create a proliferation of top-level folders as opposed to cramping the
>>> non-standard IDs into a single "______" folder.
>>
>> I believed that proliferation of folders is for purpose. Intermediate
>> directories allows to avoid excessive number of files in single
>> directory. ext4 with directory tree index usually is not the case, but
>> other filesystems may have rather poor performance when too much files
>> are stuffed into single folder. Some applications become really slow for
>> huge directories.
> 
> I was referring to TS style timestamp resolver here. It is designed to
> group directories by creation time, not to distribute them homogeneously.

My bad, I have realizes that my idea of mapping

     "x" -> "______x/x"
     "xy" -> "_____xy/xy"

was a really bad one. It leads to a separate directory for each short 
ID. However I still believe that the purpose of 
`org-attach-id-ts-folder-format' is avoid concentration of huge number 
of file in a single directory. Distribution of attachments over 
subdirectories is not perfectly even but it still works reasonably well 
for long-lasting projects while IDs follow assumed format and month is 
included into directory names.

Back to the original problem. First of all, I believe that if a user 
decided to use non-standard IDs then it is their responsibility to 
customize `org-attach-id-to-path-function-list' and to provide a 
function that implements alternative layout of attachment files. 
Depending on expected amount, they may be put into single directory, 
spread using some hash function, or accordingly to project structure. So 
today I am against default fallback directories especially in 
`org-attach-id-ts-folder-format'.

I believe that interaction between `org-attach-dir-from-id' and members 
of `org-attach-id-to-path-function-list' could be improved. If a too 
short ID is passed to `org-attach-id-uuid-folder-format', 
`org-attach-id-ts-folder-format', or a user-defined function, they may 
return nil and `org-attach-dir-from-id' should try next element from the 
list. If nothing succeeds then a user error should be signaled demanding 
to implement a strategy to properly deal with peculiar IDs.

There should be no problem to put single character IDs into a common 
directory (UUID case), but there are enough variants for 5 characters 
long names to create delayed performance problem. The worst case is when 
a user decides to use some common prefix, e.g. "id-" before UUID and all 
files go to the same directory. On the other hand I do not think that 
code should detect such cases.

Re: [BUG] org-attach-id-ts-folder-format fails on customized IDs [9.6 (9.6-??-2e9999783)]

[Max Nikulin]

On 21/07/2022 02:12, Janek F wrote:
> 
> However I tend to customize IDs for important files by hand,
> causing any attempt to use org-attach on that file to fail
> if the ID is shorter than six characters:
> 
>      org-attach-id-ts-folder-format: Args out of range: "ftt", 0, 6
> 
> This method should be adjusted to handle non-ts-ids just as well,
> as org-id-method does not dictate the format of existing ids.

There are should be no such restriction for CUSTOM_ID links (referenced 
using #anchor). IDs used for attachments assume particular format and 
consistent layout of files. Janek, could you, please, provide some 
details concerning your use case. I am interested why you decided to 
avoid CUSTOM_ID for headings important to you.

As I wrote yesterday, you can customize Org to use directories for 
attachments you like.

Re: [BUG] org-attach-id-ts-folder-format fails on customized IDs [9.6 (9.6-??-2e9999783)]

[Janek F]

I use org-roam which creates links between org-files using the ID, and I would like to be able to manually add such links to common pages even if autocompletion doesn't work smoothly.

Considering the other messages, let's not get lost in details, the default function does not need to consider all edge cases. It simply should not error out on ids in a different format.

------- Original Message -------
Max Nikulin <manikulin@gmail.com> schrieb am Freitag, 12. August 2022 um 17:35:


> On 21/07/2022 02:12, Janek F wrote:
>
> > However I tend to customize IDs for important files by hand,
> > causing any attempt to use org-attach on that file to fail
> > if the ID is shorter than six characters:
> >
> > org-attach-id-ts-folder-format: Args out of range: "ftt", 0, 6
> >
> > This method should be adjusted to handle non-ts-ids just as well,
> > as org-id-method does not dictate the format of existing ids.
>
>
> There are should be no such restriction for CUSTOM_ID links (referenced
> using #anchor). IDs used for attachments assume particular format and
> consistent layout of files. Janek, could you, please, provide some
> details concerning your use case. I am interested why you decided to
> avoid CUSTOM_ID for headings important to you.
>
> As I wrote yesterday, you can customize Org to use directories for
> attachments you like.

Re: [BUG] org-attach-id-ts-folder-format fails on customized IDs [9.6 (9.6-??-2e9999783)]

[Ihor Radchenko]

Janek F <xerusx@pm.me> writes:

> Considering the other messages, let's not get lost in details, the default function does not need to consider all edge cases. It simply should not error out on ids in a different format.

You are indeed right, but we may use multiple different approaches to
achieve this. Some are more simplistic, some are less. We are trying to
find a balance between less hassle for users and not overthinking
things.

The simplest approach would be wrapping the function calls into
(ignore-errors ...), which would avoid errors when we just try to check
the existence of the attachment dir. However, it may raise unexpected errors
for non-standard/copy-pasted IDs for users who did not customize this
area.

The approach I am proposing is handling unexpected IDs inside the
default functions and bringing errors in user-defined functions to the
attention of user - I feel that it will be more useful than ignoring
errors.

We could use yet another approach and use a global fallback if no
available function can generate an ID, but we may run into tricky cases
because the first function in the list has a special meaning - it _must_
generate a path from ID or the user may run into unexpected breakage in
some cases when custom IDs are being used.

-- 
Ihor Radchenko,
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

Re: [PATCH v3] Re: [BUG] org-attach-id-ts-folder-format fails on customized IDs [9.6 (9.6-??-2e9999783)]

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

> was a really bad one. It leads to a separate directory for each short 
> ID. However I still believe that the purpose of 
> `org-attach-id-ts-folder-format' is avoid concentration of huge number 
> of file in a single directory. Distribution of attachments over 
> subdirectories is not perfectly even but it still works reasonably well 
> for long-lasting projects while IDs follow assumed format and month is 
> included into directory names.

This is only true for uuid-style. Also, this is likely not the problem
we need to worry about. To be a problem, the number of attach dirs
should be really large, which is only relevant when thousands of attach
dirs are cramped into a single parent directory. Thousands of attach
dirs would probably correspond to tens of thousands of headings, which
is not something we commonly have and, honestly, Org will have much more
serious issues when the number of headings is that large.

If someone ends up complaining about too many attach dirs, we can always
ask to adjust the ID naming scheme.

The previous paragraph appears to be consistent with

> Back to the original problem. First of all, I believe that if a user 
> decided to use non-standard IDs then it is their responsibility to 
> customize `org-attach-id-to-path-function-list' and to provide a 
> function that implements alternative layout of attachment files.
> Depending on expected amount, they may be put into single directory, 
> spread using some hash function, or accordingly to project structure. So 
> today I am against default fallback directories especially in 
> `org-attach-id-ts-folder-format'.

---

> I believe that interaction between `org-attach-dir-from-id' and members 
> of `org-attach-id-to-path-function-list' could be improved. If a too 
> short ID is passed to `org-attach-id-uuid-folder-format', 
> `org-attach-id-ts-folder-format', or a user-defined function, they may 
> return nil and `org-attach-dir-from-id' should try next element from the 
> list. If nothing succeeds then a user error should be signaled demanding 
> to implement a strategy to properly deal with peculiar IDs.

I do not think that it is a good idea. We currently promise to use the
first function in the list to generate the IDs. Other functions are
mostly a fallback to catch the existing attach dirs created in the past
using different ID scheme.

I guess that one option could be demanding a non-nil return value when
generating a new attach dir (without TRY-ALL argument) and wrapping
things into (ignore-errors ...) otherwise.

> There should be no problem to put single character IDs into a common 
> directory (UUID case), but there are enough variants for 5 characters 
> long names to create delayed performance problem. The worst case is when 
> a user decides to use some common prefix, e.g. "id-" before UUID and all 
> files go to the same directory. On the other hand I do not think that 
> code should detect such cases.

We should not worry about this. It will be the user responsibility to
create the relevant custom function.

-- 
Ihor Radchenko,
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

Re: [BUG] org-attach-id-ts-folder-format fails on customized IDs [9.6 (9.6-??-2e9999783)]

[Max Nikulin]

On 12/08/2022 23:08, Janek F wrote:
> I use org-roam which creates links between org-files using the ID, and I
> would like to be able to manually add such links to common pages even if
> autocompletion doesn't work smoothly.

Unfortunately I have not tried org-roam yet. My suggestion is to add 
CUSTOM_ID property in addition to ID. I admit that CUSTOM_ID may be 
inconvenient if a heading is moved from one file to another. Are there 
more problems with CUSTOM_ID links?

> Considering the other messages, let's not get lost in details, the
> default function does not need to consider all edge cases. It simply
> should not error out on ids in a different format.

Janek, what is your expectation concerning directory structure for 
attachments in the case of customized ID values?

You have not provided details concerning your configuration, but perhaps 
your issue may be solved by customizing 
`org-attach-id-to-path-function-list': keep 
`org-attach-id-uuid-folder-format' and drop 
`org-attach-id-ts-folder-format'. It should help unless you have single 
character IDs.

I do not think Org behavior is ideal in your case. On the other hand the 
code was written with assumption that either UUID or timestamp-based IDs 
are used. It is violated by handcrafted IDs. I do not know the full 
story, but I guess, `org-attach-id-ts-folder-format' is present in 
default configuration for backward compatibility reasons.

org-attach-id-ts-folder-format: Args out of range: "ftt", 0, 6

is an obscure error. However Org needs some hint from user how to handle 
such case.

Re: [PATCH v3] Re: [BUG] org-attach-id-ts-folder-format fails on customized IDs [9.6 (9.6-??-2e9999783)]

[Max Nikulin]

On 13/08/2022 12:29, Ihor Radchenko wrote:
> Max Nikulin writes:
> 
>> I believe that interaction between `org-attach-dir-from-id' and members
>> of `org-attach-id-to-path-function-list' could be improved. If a too
>> short ID is passed to `org-attach-id-uuid-folder-format',
>> `org-attach-id-ts-folder-format', or a user-defined function, they may
>> return nil and `org-attach-dir-from-id' should try next element from the
>> list. If nothing succeeds then a user error should be signaled demanding
>> to implement a strategy to properly deal with peculiar IDs.
> 
> I do not think that it is a good idea. We currently promise to use the
> first function in the list to generate the IDs. Other functions are
> mostly a fallback to catch the existing attach dirs created in the past
> using different ID scheme.

My idea is to add an extra check that filters out nil values. I do not 
mind to signal a user error if first entry from 
`org-attach-id-to-path-function-list' returns nil and paths returned by 
other function do not exist.

> I guess that one option could be demanding a non-nil return value when
> generating a new attach dir (without TRY-ALL argument) and wrapping
> things into (ignore-errors ...) otherwise.

I am against `ignore-errors' because it makes harder bug hunting using 
debug on errors.

I would prefer to force users to justify strategy for non-standard IDs 
as early as possible. The goal is to avoid manual actions to change 
directory structure when users have hundreds of folders. My experience 
that users may easily face performance issues with ~1000 files per 
directory. The reason is either lack of optimization for such case in 
particular application or a bug introduced in a new version.

Re: [PATCH v3] Re: [BUG] org-attach-id-ts-folder-format fails on customized IDs [9.6 (9.6-??-2e9999783)]

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

>> I do not think that it is a good idea. We currently promise to use the
>> first function in the list to generate the IDs. Other functions are
>> mostly a fallback to catch the existing attach dirs created in the past
>> using different ID scheme.
>
> My idea is to add an extra check that filters out nil values. I do not 
> mind to signal a user error if first entry from 
> `org-attach-id-to-path-function-list' returns nil and paths returned by 
> other function do not exist.

This would be possible, in addition to the patch above.
Or do you mean the default ts/uuid format functions to return nil to
short IDs as well?

>> I guess that one option could be demanding a non-nil return value when
>> generating a new attach dir (without TRY-ALL argument) and wrapping
>> things into (ignore-errors ...) otherwise.
>
> I am against `ignore-errors' because it makes harder bug hunting using 
> debug on errors.

Agreed.

> I would prefer to force users to justify strategy for non-standard IDs 
> as early as possible. The goal is to avoid manual actions to change 
> directory structure when users have hundreds of folders. My experience 
> that users may easily face performance issues with ~1000 files per 
> directory. The reason is either lack of optimization for such case in 
> particular application or a bug introduced in a new version.

I am not sure if we can solve this kind of issue reliably. The only idea
is enforcing [1-9][0-9]\{5\} for ts IDs. And we cannot do much about IDs
prefixed with the same constant string.

-- 
Ihor Radchenko,
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

[PATCH v4] [BUG] org-attach-id-ts-folder-format fails on customized IDs [9.6 (9.6-??-2e9999783)]

[Ihor Radchenko]

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

Ihor Radchenko <yantar92@gmail.com> writes:

> Max Nikulin <manikulin@gmail.com> writes:
>
>>> I do not think that it is a good idea. We currently promise to use the
>>> first function in the list to generate the IDs. Other functions are
>>> mostly a fallback to catch the existing attach dirs created in the past
>>> using different ID scheme.
>>
>> My idea is to add an extra check that filters out nil values. I do not 
>> mind to signal a user error if first entry from 
>> `org-attach-id-to-path-function-list' returns nil and paths returned by 
>> other function do not exist.
>
> This would be possible, in addition to the patch above.
> Or do you mean the default ts/uuid format functions to return nil to
> short IDs as well?

The new version of the patch allows all but first function return nil.
See the attached.

Let me know if there are any other objections.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: v4-0001-org-attach-dir-from-id-Do-not-rely-on-ID-being-ov.patch --]
[-- Type: text/x-patch, Size: 4023 bytes --]

From ac88b8ac1117007eca49a0b229d8cee6cba8bc87 Mon Sep 17 00:00:00 2001
Message-Id: <ac88b8ac1117007eca49a0b229d8cee6cba8bc87.1664701946.git.yantar92@gmail.com>
From: Ihor Radchenko <yantar92@gmail.com>
Date: Sat, 23 Jul 2022 13:13:24 +0800
Subject: [PATCH v4] org-attach-dir-from-id: Do not rely on ID being over 6
 chars long

* lisp/org-attach.el (org-attach-id-uuid-folder-format): Fall back to
"__/ID" when the ID contains 2 chars or less and cannot be split into
the "xy/z...." path.
(org-attach-id-ts-folder-format): Fall back to "______/ID" path format
when the ID contains less than 7 chars and cannot be split into the
"YYYYMM/rest" path.

(org-attach-dir-from-id): Allow all but first function return nil and
be ignored.
(org-attach-id-to-path-function-list): Document that all but first
function can return nil.

Fixes https://orgmode.org/list/KC8PcypJapBpJQtJxM0kX5N7Z0THL2Lq6EQjBMzpw1-vgQf72egZ2JOIlTbPYiqAVD4MdSBhrhBZr2Ykf5DN1mocm1ANvvuKKZShlkgzKYM=@pm.me
---
 lisp/org-attach.el | 43 ++++++++++++++++++++++++++++++-------------
 1 file changed, 30 insertions(+), 13 deletions(-)

diff --git a/lisp/org-attach.el b/lisp/org-attach.el
index 8509a6564..e4511794f 100644
--- a/lisp/org-attach.el
+++ b/lisp/org-attach.el
@@ -162,19 +162,27 @@ (defcustom org-attach-archive-delete nil
 (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)))
+attachments.  Useful if ID is generated with UUID.
+
+When ID is too short (less than 3 chars), return \"__/ID\"."
+  (if (< (length id) 3)
+      (format "__/%s" 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)))
+year-month, the rest.
+
+When ID is too short (less than 7 chars), return \"______/ID\"."
+  (if (< (length id) 7)
+      (format "______/%s" id)
+    (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)
@@ -182,7 +190,12 @@ (defcustom org-attach-id-to-path-function-list '(org-attach-id-uuid-folder-forma
 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."
+attachment folders based on ID.
+
+The functions must either return string or nil.  If a function returns
+nil, its return value will be ignored when looking for existing
+attachment folders.  The first function in the list must always return
+string."
   :group 'org-attach
   :package-version '(Org . "9.3")
   :type '(repeat (function :tag "Function with ID as input")))
@@ -400,11 +413,15 @@ (defun org-attach-dir-from-id (id  &optional try-all)
 			       (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)))
+	      (fun-list (cdr org-attach-id-to-path-function-list))
+              rtn)
 	  (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 rtn (funcall (car fun-list) id))
+            ;; Skip nil return values.
+            (when rtn
+	      (setq attach-dir (expand-file-name
+			        rtn
+			        (expand-file-name org-attach-id-dir))))
 	    (setq fun-list (cdr fun-list)))
 	  (if (file-directory-p attach-dir)
 	      attach-dir
-- 
2.35.1


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


-- 
Ihor Radchenko,
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

Re: [PATCH v4] [BUG] org-attach-id-ts-folder-format fails on customized IDs [9.6 (9.6-??-2e9999783)]

[Max Nikulin]

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

On 02/10/2022 16:14, Ihor Radchenko wrote:
> Ihor Radchenko writes:
>> Max Nikulin writes:
>>
> The new version of the patch allows all but first function return nil.
> See the attached.
> 
> Let me know if there are any other objections.

Ihor, sorry that I left your earlier questions with no response. I hope, 
the attached diff may illustrate my ideas better. I have not tried to 
run it though, so it can be full of stupid mistakes. I suggest to ignore 
nil values completely, but maybe I missed some use case.

Actually I suspect the if the `org-attach-dir-from-id' function returned 
both first and existing directories, the code of callers would be clearer.

[-- Attachment #2: alt-org-attach-id-dir.diff --]
[-- Type: text/x-patch, Size: 4740 bytes --]

diff --git a/lisp/org-attach.el b/lisp/org-attach.el
index 8509a6564..c4d043fb9 100644
--- a/lisp/org-attach.el
+++ b/lisp/org-attach.el
@@ -163,18 +163,27 @@ When set to `query', ask the user instead."
   "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)
+  "May be added last to `org-attach-id-path-function-list'.
+A fallback as \"__/ID\" directory for the case when user
+changed ID value to a string too short for `org-attach-id-ts-folder-format'
+and other functions."
+  (format "__/%s" id))
 
 (defcustom org-attach-id-to-path-function-list '(org-attach-id-uuid-folder-format
 						 org-attach-id-ts-folder-format)
@@ -182,7 +191,18 @@ year-month, the rest."
 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."
+attachment folders based on ID.
+
+Add `org-attach-id-fallback-folder-format' to the end if you edit
+IDs to short value, but you can not implement a better variant of
+layout.  Consider something like the following as first element instead
+
+    (defun my/attach-id-custom-folder-format (id)
+     (unless
+      (or (string-match-p \"[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}\" id)
+          (string-match-p \"[0-9]{8}T[0-9]{6}\\.[0-9]{6}\" id))
+      (format \"important/%s/%s\" (substring id 0 1) (substring id 1))))
+"
   :group 'org-attach
   :package-version '(Org . "9.3")
   :type '(repeat (function :tag "Function with ID as input")))
@@ -384,7 +404,8 @@ If the attachment by some reason cannot be created an error will be raised."
 	 ((eq org-attach-preferred-new-method 'nil)
 	  (error "No existing directory.  DIR or ID property has to be explicitly created")))))
     (unless attach-dir
-      (error "No attachment directory is associated with the current node"))
+      (error "No attachment directory is associated with the current node, \
+adjust `org-attach-id-to-path-function-list'"))
     (unless (file-directory-p attach-dir)
       (make-directory attach-dir t))
     attach-dir))
@@ -393,23 +414,22 @@ If the attachment by some reason cannot be created an error will be raised."
   "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)))
+that exist in the filesystem, otherwise independently of TRY-ALL
+value 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 try-all) (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.

Re: [PATCH v4] [BUG] org-attach-id-ts-folder-format fails on customized IDs [9.6 (9.6-??-2e9999783)]

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

>> The new version of the patch allows all but first function return nil.
>> See the attached.
>> 
>> Let me know if there are any other objections.
>
> Ihor, sorry that I left your earlier questions with no response. I hope, 
> the attached diff may illustrate my ideas better. I have not tried to 
> run it though, so it can be full of stupid mistakes. I suggest to ignore 
> nil values completely, but maybe I missed some use case.

AFAIU, your version of `org-attach-dir-from-id' may sometimes return
nil, which is neither documented nor expected by the callers.

In particular, `org-attach-dir-get-create' will throw unhelpful "No
attachment directory is associated with the current node" error.

`org-attach-dir' appears to handle nil return value fine.

I will be happy to merge a proper patch upon addressing possible
pitfalls. I do not feel strongly about my variant vs. yours.

> Actually I suspect the if the `org-attach-dir-from-id' function returned 
> both first and existing directories, the code of callers would be
> clearer.

`org-attach-dir-from-id' is a public function, and we cannot change its
return signature drastically without breaking backwards compatibility.
If we do want to change the return value, the current function name
should be obsoleted and an alternative function should be introduced.

I do not feel like the benefits are strong enough to justify such a
drastic measure. `org-attach-dir-from-id' is only used in two places in
Org code - I see no clear benefit of returning multiple paths.

-- 
Ihor Radchenko,
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

Re: [PATCH v4] [BUG] org-attach-id-ts-folder-format fails on customized IDs [9.6 (9.6-??-2e9999783)]

[Ihor Radchenko]

Ihor Radchenko <yantar92@gmail.com> writes:

> I will be happy to merge a proper patch upon addressing possible
> pitfalls. I do not feel strongly about my variant vs. yours.

Max, do you have any update here? If not, I will merge my variant as it
should not break anything.

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

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

[Max Nikulin]

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

Ihor, I played with my patch for some time and the only issue I have 
noticed is missed backslashes in a docstring.

On 05/10/2022 12:44, Ihor Radchenko wrote:
> 
> AFAIU, your version of `org-attach-dir-from-id' may sometimes return
> nil, which is neither documented

"otherwise independently of TRY-ALL
value return the first non nil value."

indirectly assumes nil. It may be made more prominent.

> nor expected by the callers.

I see 2 direct callers and I am not aware of any problem with them.

> In particular, `org-attach-dir-get-create' will throw unhelpful "No
> attachment directory is associated with the current node" error.

* H short
:PROPERTIES:
:ID:       ec
:END:

if: No attachment directory is associated with the current node, adjust 
‘org-attach-id-to-path-function-list’

I do not think this message is unhelpful.

> `org-attach-dir' appears to handle nil return value fine.

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

From 19e51a0112353a377deef64a83c02c5ee393a15d Mon Sep 17 00:00:00 2001
From: Max Nikulin <manikulin@gmail.com>
Date: Mon, 7 Nov 2022 23:48:02 +0700
Subject: [PATCH] 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): Update error message to suggest
customization of `org-attach-id-to-path-function-list'.
(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.

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
---
 lisp/org-attach.el | 69 +++++++++++++++++++++++++++++-----------------
 1 file changed, 44 insertions(+), 25 deletions(-)

diff --git a/lisp/org-attach.el b/lisp/org-attach.el
index ef183474e..f615e757a 100644
--- a/lisp/org-attach.el
+++ b/lisp/org-attach.el
@@ -166,18 +166,27 @@ When set to `query', ask the user instead."
   "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)
+  "May be added last to `org-attach-id-path-function-list'.
+A fallback as \"__/ID\" directory for the case when user
+changed ID value to a string too short for `org-attach-id-ts-folder-format'
+and other functions."
+  (format "__/%s" id))
 
 (defcustom org-attach-id-to-path-function-list '(org-attach-id-uuid-folder-format
 						 org-attach-id-ts-folder-format)
@@ -185,7 +194,17 @@ year-month, the rest."
 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."
+attachment folders based on ID.
+
+Add `org-attach-id-fallback-folder-format' to the end if you edit
+IDs to short value, but you can not implement a better variant of
+layout.  Consider something like the following as first element instead
+
+    (defun my/attach-id-custom-folder-format (id)
+     (unless
+      (or (string-match-p \"[0-9a-f]\\\\=\\{8\\\\}\\\\(?:-[0-9a-f]{4}\\\\)\\\\=\\{3\\\\}-[0-9a-f]\\\\=\\{12\\\\}\" id)
+          (string-match-p \"[0-9]\\\\=\\{8\\\\}T[0-9]\\\\=\\{6\\\\}\\.[0-9]\\\\=\\{6\\\\}\" id))
+      (format \"important/%s/%s\" (substring id 0 1) (substring id 1))))"
   :group 'org-attach
   :package-version '(Org . "9.3")
   :type '(repeat (function :tag "Function with ID as input")))
@@ -387,7 +406,8 @@ If the attachment by some reason cannot be created an error will be raised."
 	 ((eq org-attach-preferred-new-method 'nil)
 	  (error "No existing directory.  DIR or ID property has to be explicitly created")))))
     (unless attach-dir
-      (error "No attachment directory is associated with the current node"))
+      (error "No attachment directory is associated with the current node, \
+adjust `org-attach-id-to-path-function-list'"))
     (unless (file-directory-p attach-dir)
       (make-directory attach-dir t))
     attach-dir))
@@ -396,23 +416,22 @@ If the attachment by some reason cannot be created an error will be raised."
   "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)))
+that exist in the filesystem, otherwise independently of TRY-ALL
+value 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 try-all) (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

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

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

>> AFAIU, your version of `org-attach-dir-from-id' may sometimes return
>> nil, which is neither documented
>
> "otherwise independently of TRY-ALL
> value return the first non nil value."
>
> indirectly assumes nil. It may be made more prominent.

I feel like this makes the docstring confusing.

Note that `org-attach-id-to-path-function-list':

    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.

The docstring implies that only the first function will be used to
create new attachment folders. With your patch, it will no longer be the
case and may surprise users.

You at least need to update the docstring.

> 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, otherwise independently of TRY-ALL
> value return the first non nil value.

This is a very long sentence and I cannot clearly parse which sub-clause
is referenced by "otherwise".

>> nor expected by the callers.
>
> I see 2 direct callers and I am not aware of any problem with them.
>
>> In particular, `org-attach-dir-get-create' will throw unhelpful "No
>> attachment directory is associated with the current node" error.
>
> * H short
> :PROPERTIES:
> :ID:       ec
> :END:
>
> if: No attachment directory is associated with the current node, adjust 
> ‘org-attach-id-to-path-function-list’
>
> I do not think this message is unhelpful.

With your patch, such message will be displayed even when
`org-attach-preferred-new-method' is set to something other than 'id.
And the message will be wrong then.

> +(defun org-attach-id-fallback-folder-format (id)
> +  "May be added last to `org-attach-id-path-function-list'.

This is not a proper first line in a function docstring. First line must
describe what the function does.

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

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

[Max Nikulin]

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

On 08/11/2022 12:08, Ihor Radchenko wrote:
> 
> I feel like this makes the docstring confusing.
> 
> Note that `org-attach-id-to-path-function-list':

I have tried to update the docstrings.

>> if: No attachment directory is associated with the current node, adjust
>> ‘org-attach-id-to-path-function-list’
>>
>> I do not think this message is unhelpful.
> 
> With your patch, such message will be displayed even when
> `org-attach-preferred-new-method' is set to something other than 'id.
> And the message will be wrong then.

I have moved `error' call despite I have not figured out how to trigger 
the error for other options.

>> +(defun org-attach-id-fallback-folder-format (id)
>> +  "May be added last to `org-attach-id-path-function-list'.
> 
> This is not a proper first line in a function docstring. First line must
> describe what the function does.

I am still unsure that in this case effect is more important than 
purpose. The function is too specific.

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.

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

From dfc35f46f59d5938f1a520b860c3eda36f49a9d5 Mon Sep 17 00:00:00 2001
From: Max Nikulin <manikulin@gmail.com>
Date: Mon, 7 Nov 2022 23:48:02 +0700
Subject: [PATCH v2] 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): Update error message to suggest
customization of `org-attach-id-to-path-function-list'.
(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.

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
---
 lisp/org-attach.el | 98 ++++++++++++++++++++++++++++++----------------
 1 file changed, 65 insertions(+), 33 deletions(-)

diff --git a/lisp/org-attach.el b/lisp/org-attach.el
index ef183474e..bd3034992 100644
--- a/lisp/org-attach.el
+++ b/lisp/org-attach.el
@@ -166,26 +166,56 @@ When set to `query', ask the user instead."
   "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 added as the last element of `org-attach-id-path-function-list'
+to avoid errors for 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 @@ If no attachment directory can be derived, return nil."
       (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 @@ If the attachment by some reason cannot be created an error will be raised."
 	  (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 @@ If the attachment by some reason cannot be created an error will be raised."
       (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

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

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

> On 08/11/2022 12:08, Ihor Radchenko wrote:
>> 
>> I feel like this makes the docstring confusing.
>> 
>> Note that `org-attach-id-to-path-function-list':
>
> I have tried to update the docstrings.

Thanks!

>>> if: No attachment directory is associated with the current node, adjust
>>> ‘org-attach-id-to-path-function-list’
>>>
>>> I do not think this message is unhelpful.
>> 
>> With your patch, such message will be displayed even when
>> `org-attach-preferred-new-method' is set to something other than 'id.
>> And the message will be wrong then.
>
> I have moved `error' call despite I have not figured out how to trigger 
> the error for other options.

The main reason is code readability.

Also, one can trigger the other error for non-standard values of
`org-attach-preferred-new-method' .

>>> +(defun org-attach-id-fallback-folder-format (id)
>>> +  "May be added last to `org-attach-id-path-function-list'.
>> 
>> This is not a proper first line in a function docstring. First line must
>> describe what the function does.
>
> I am still unsure that in this case effect is more important than 
> purpose. The function is too specific.

That's a function and should follow standards. If we do not follow
standards, it will become harder to maintain Org.

If we want to be really specific, we may allow a special symbol in the
list indicating the fallback. I'd prefer this approach a bit better. No
strong opinion though.

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

However, please add NEWS entry detailing the change in
`org-attach-id-to-path-function-list' to the next version of the patch.

I have no other comments apart from various grammar issues and typos.
But that's easy to fix before merging.

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

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

[Max Nikulin]

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

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

[Ihor Radchenko]

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

Max Nikulin <manikulin@gmail.com> writes:

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

Thanks!

I went through the patch and tried to clarify the wording.
Especially in the defcustom docstring.

I also added the dumb fallback to the default value.
I feel that otherwise the description of too confusing.

Let me know what you think about the attached new version of the patch
with my amendments.


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

From aee92ee992d9497c91a0d2bef7a1517df5a032a6 Mon Sep 17 00:00:00 2001
Message-Id: <aee92ee992d9497c91a0d2bef7a1517df5a032a6.1668396417.git.yantar92@posteo.net>
From: Max Nikulin <manikulin@gmail.com>
Date: Mon, 7 Nov 2022 23:48:02 +0700
Subject: [PATCH v4] 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       |  41 ++++++++++++++++++
 lisp/org-attach.el | 104 +++++++++++++++++++++++++++++----------------
 2 files changed, 108 insertions(+), 37 deletions(-)

diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
index 04b5be64a..851658082 100644
--- a/etc/ORG-NEWS
+++ b/etc/ORG-NEWS
@@ -604,6 +604,47 @@ 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)))
+        ;; 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
+=__=​/ID directory.
 
 * Version 9.5
 
diff --git a/lisp/org-attach.el b/lisp/org-attach.el
index ef183474e..e956cac18 100644
--- a/lisp/org-attach.el
+++ b/lisp/org-attach.el
@@ -166,28 +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)))
-
-(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 +388,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 +409,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 +424,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: 225 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>

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

[Max Nikulin]

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.

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

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.

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.

> +#+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.

> +#+end_src

> +(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))

Additional single character subdir level should be a minor improvement, 
unless `org-id-prefix' is non-nil.

> +(defcustom org-attach-id-to-path-function-list '( org-attach-id-uuid-folder-format
space ----------------------------------------------^

> +					org-attach-id-ts-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

> +                                        org-attach-id-fallback-folder-format)

So I do not have strong objections since default value of 
`org-attach-id-to-path-function-list' may be adjusted later. Maybe 
strict variants of ID to subdir functions should be added as named 
functions. Fortunately it does not conflict with the patch variant your 
suggested. Feel free to commit your version.

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

[Ihor Radchenko]

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

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

[Max Nikulin]

On 15/11/2022 09:41, Ihor Radchenko wrote:
> Max Nikulin writes:
>>
>> 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.

I had an impression that something was wrong with indentation of next 
entries.

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

> Handling too short IDs is a different issue indeed, but why not to fix
> it as well?

I still have a different opinion, but it should not prevent you from 
committing your variant. The issue is not critical.

The problem is not too short ID. Actually they are some ID having 
unknown format, so functions designed for particular ID generation 
methods may cause a mess in attachment subdirs.

>> 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 may be done by a next patch unless other issues will attract more 
attention.

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

> +(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))))

Sorry, but "important" entry should be before 
`org-attach-id-uuid-folder-format'. My idea is the following:
- If the ID was generated by 'uuid or 'org `org-id-method' then prefer 
XX/... subfolder. It is important for new attachments. Even if current 
`org-id-method' is 'ts then this entry is ignored and layout is 
determined by the next entry.
- If the ID is timestamp-based then prefer "YYYYMM/DDTIME" subfolder.
- Put new non-standard IDs to important/X/... subfolder whenever 
particular ID is a short or a long one.
- Handle the case of `org-id-method' changed in the past from timestamp 
to UUID or vice versa. Try to find attachment trying to split 2 
characters from timestamp or 6 characters from UUID. So add bare 
`org-attach-id-uuid-folder-format' and `org-attach-id-ts-folder-format'. 
These 2 entries may be skipped for new users or for users who never 
experienced change of `org-id-method'.

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

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

>>> 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.
>
> I had an impression that something was wrong with indentation of next 
> entries.

I thought that you referred to '(<space>symbol ...
Other stray spaces were there because I forgot to disable nameless-mode.

> Sorry, but "important" entry should be before 
> `org-attach-id-uuid-folder-format'. My idea is the following:
> - If the ID was generated by 'uuid or 'org `org-id-method' then prefer 
> XX/... subfolder. It is important for new attachments. Even if current 
> `org-id-method' is 'ts then this entry is ignored and layout is 
> determined by the next entry.
> - If the ID is timestamp-based then prefer "YYYYMM/DDTIME" subfolder.
> - Put new non-standard IDs to important/X/... subfolder whenever 
> particular ID is a short or a long one.
> - Handle the case of `org-id-method' changed in the past from timestamp 
> to UUID or vice versa. Try to find attachment trying to split 2 
> characters from timestamp or 6 characters from UUID. So add bare 
> `org-attach-id-uuid-folder-format' and `org-attach-id-ts-folder-format'. 
> These 2 entries may be skipped for new users or for users who never 
> experienced change of `org-id-method'.

Agree.
I now applied the updated version of the patch.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=08a8c9e67

>>>> 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.
>
>> Handling too short IDs is a different issue indeed, but why not to fix
>> it as well?
>
> I still have a different opinion, but it should not prevent you from 
> committing your variant. The issue is not critical.
>
> The problem is not too short ID. Actually they are some ID having 
> unknown format, so functions designed for particular ID generation 
> methods may cause a mess in attachment subdirs.

>>> 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 may be done by a next patch unless other issues will attract more 
> attention.

I guess it does not heart to wait a bit. Though I do not see any issue
with updating the defaults to your version.

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