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.