(fix)org-id: condition-case instead of unwind-protect the error handler

unwind-protect will still propagate the initial error, which isn't
desirable and can confuse both, the end-user and us when resolving
issues.

To keep the backtrace clean (in case of other errors), just
automatically detatch the advice once it ran in the error handling
branch. If the user change the file at the runtime, org-id shouldn't
fail with the error as long as it was handled at least once, and it
still will be handled by us during the next session. At the end of the
day it's a very niche edge case, which most of the users shouldn't ever
see.

Fixes #1805.
This commit is contained in:
Wetlize
2021-08-26 02:29:03 +03:00
parent 74a6fd598a
commit 5dde894a0c

View File

@ -99,35 +99,33 @@ recursion."
(advice-add #'org-id-add-location :around #'org-roam--handle-absent-org-id-locations-file-a) (advice-add #'org-id-add-location :around #'org-roam--handle-absent-org-id-locations-file-a)
(defun org-roam--handle-absent-org-id-locations-file-a (fn &rest args) (defun org-roam--handle-absent-org-id-locations-file-a (fn &rest args)
"Gracefully handle errors related to absence of `org-id-locations-file'. "Gracefully handle errors related to absence of `org-id-locations-file'.
FN is `org-id-locations-file' that comes from advice and ARGS are FN is `org-id-add-location' that comes from advice and ARGS are
passed to it." passed to it."
(let (result) (condition-case err
;; Use `unwind-protect' over `condition-case' because `org-id' can produce various other errors, but all (apply fn args)
;; of its errors are generic ones, so trapping all of them isn't a good idea and preserving the correct ;; `org-id' makes the assumption that `org-id-locations-file' will be stored in `user-emacs-directory'
;; backtrace is valuable. ;; which always exist if you have Emacs, so it uses `with-temp-file' to write to the file. However, the
(unwind-protect (setq result (apply fn args)) ;; users *do* change the path to this file and `with-temp-file' unable to create the file, if the path to
(unless result ;; it consists of directories that don't exist. We'll have to handle this ourselves.
(unless org-id-locations (error
;; Pre-allocate the hash table to avoid weird access related errors during the regeneration. (advice-remove 'org-id-add-location #'org-roam--handle-absent-org-id-locations-file-a)
(setq org-id-locations (make-hash-table :test 'equal))) (if (file-exists-p (file-truename org-id-locations-file))
;; `org-id' makes the assumption that `org-id-locations-file' will be stored in `user-emacs-directory' (signal (car err) (cdr err))
;; which always exist if you have Emacs, so it uses `with-temp-file' to write to the file. However, ;; Pre-allocate the hash table to avoid weird access related errors during the regeneration.
;; the users *do* change the path to this file and `with-temp-file' unable to create the file, if the (or org-id-locations (setq org-id-locations (make-hash-table :test 'equal)))
;; path to it consists of directories that don't exist. We'll have to handle this ourselves. ;; If permissions allow that, try to create the user specified directory path to
(unless (file-exists-p (file-truename org-id-locations-file)) ;; `org-id-locations-file' ourselves.
;; If permissions allow that, try to create the user specified directory path to (condition-case _err
;; `org-id-locations-file' ourselves. (progn (org-roam-message (concat "`org-id-locations-file' (%s) doesn't exist. "
(condition-case _err "Trying to regenerate it (this may take a while)...")
(progn (org-roam-message (concat "`org-id-locations-file' (%s) doesn't exist. " org-id-locations-file)
"Trying to regenerate it (this may take a while)...") (make-directory (file-name-directory (file-truename org-id-locations-file)))
org-id-locations-file) (org-roam-update-org-id-locations)
(make-directory (file-name-directory (file-truename org-id-locations-file))) (apply fn args))
(org-roam-update-org-id-locations) ;; In case of failure (lack of permissions), we'll patch it to at least handle the current session
(apply fn args)) ;; without errors.
;; In case of failure (lack of permissions), we'll patch it to at least handle the current session (file-error (org-roam-message "Failed to regenerate `org-id-locations-file'")
;; without errors. (lwarn 'org-roam :error "
(file-error (org-roam-message "Failed to regenerate `org-id-locations-file'")
(lwarn 'org-roam :error "
-------- --------
WARNING: `org-id-locations-file' (%s) doesn't exist! WARNING: `org-id-locations-file' (%s) doesn't exist!
Org-roam is unable to create it for you. Org-roam is unable to create it for you.
@ -151,10 +149,9 @@ allows to keep linking with \"id:\" links within the current
`org-roam-directory' to headings and files that are excluded from `org-roam-directory' to headings and files that are excluded from
identification (e.g. with \"ROAM_EXCLUDE\" property) as Org-roam identification (e.g. with \"ROAM_EXCLUDE\" property) as Org-roam
nodes." org-id-locations-file) nodes." org-id-locations-file)
(setq org-id-locations-file (setq org-id-locations-file
(expand-file-name ".orgids" (file-truename org-roam-directory))) (expand-file-name ".orgids" (file-truename org-roam-directory)))
(apply fn args))))) (apply fn args)))))))
result)))
;;; Obsolete aliases (remove after next major release) ;;; Obsolete aliases (remove after next major release)
(define-obsolete-function-alias (define-obsolete-function-alias