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.