Haven't checked the code, but regarding the hashing - I have had eliminated this much before from my system

;; (defun org-roam-db--file-hash (file-path)
;;   "Compute the hash of FILE-PATH."
;;   (with-temp-buffer
;;     (set-buffer-multibyte nil)
;;     (insert-file-contents-literally file-path)
;;     (secure-hash 'sha1 (current-buffer))))

(defun org-roam-db--file-hash (file-path)
  "Compute the hash of FILE-PATH concatenated with its modification time."
  (secure-hash 'sha1 (format "%s %s" file-path (nth 5 (file-attributes file-path)))))

This hasn't caused me any issues whatsoever.
Don't see any reason to check file content hash when there could be more cheap check for modification

Unless something constantly corrupts these attributes for some users!

It looks great. One suggestion:

instead of using dolist and cl-loop use mapcar. This turns the code from mutable to immutable and makes it easier to read (in my opinion) especially if you extract the mapcar larger lambdas (like in org-roam--list-recursive-subdirs)

mapcan does mutate the original list to the result list. I think that is desirable in this case. Otherwise it should be replaced with mapcar. the advantage of mapcan is that it does not create a new list. See the documentation for each. The code below uses mapcan

For example:

 (defun org-roam--process-subdir-entry (name dir deny-re follow-symlinks)
   "Process a single directory entry NAME in DIR.
 Returns a list of the directory and its subdirectories if it matches the criteria.
 DENY-RE is the exclusion regex pattern.
 FOLLOW-SYMLINKS determines whether to follow symlinks."
   (when (and (directory-name-p name)
              (not (member name '("./" "../"))))
     (let ((full-name (concat dir name)))
       (when (and (not (string-match-p deny-re full-name))
                  (or follow-symlinks (not (file-symlink-p full-name))))
         (cons full-name
               (org-roam--list-recursive-subdirs full-name follow-symlinks))))))
 
 (defun org-roam--list-recursive-subdirs (dir &optional follow-symlinks)
   "Return a list of DIR, its subdirs, sub-subdirs and so on.
 FOLLOW-SYMLINKS means to include dirs that are symlinks and search them
 for subdirs too, but can result in an infinite loop.
 
 Affected by user options:
 - `org-roam-dir-exclude-literals'
 - `org-roam-dir-exclude-regexps'"
   (let ((deny-re (org-roam--dir-deny-re)))
     (mapcan
      (lambda (name)
        (org-roam--process-subdir-entry name dir deny-re follow-symlinks))
      (file-name-all-completions "" dir))))

Thank you for your kind words. I don't think I agree on easier to read, and you'll have to explain what you mean by turning it from mutable to immutable. AFAIK these are identical in effect:

(cl-loop for x in '(1 2 3 4)
         collect (+ x 10))

(let (result)
  (dolist (x '(1 2 3 4))
    (push (+ x 10) result))
  (nreverse result))

(mapcar (lambda (x)
          (+ x 10))
        '(1 2 3 4))

I generally prefer cl-loop as my go-to tool, since it also has destructuring (the (file . attr) here):

;; return directories in current dir
(cl-loop for (file . attr) in (directory-files-and-attributes ".")
         if (eq t (file-attribute-type attr))
         collect file)

and many other things, but destructuring in particular is really annoying to lack when using mapcar and friends. Anyway, just a tangent :)

Btw, the design of org-roam--list-recursive-subdirs is essentially copy-pasted from the built-in directory-files-recursively and simplified. That one uses dolist.

Not that the built-ins are well-known for ease of reading... but that's why it's how it is.

Unrelated but I too prefer cl-loop. I don't get why some library creators insist on excluding cl - for example the first thing that is written in dash library readme is that it excludes cl and so on.
Cl-loop is very simple and nonverbose.

I think a reader will find a cl-loop much easy to reason about than something built from first principles.

you'll have to explain what you mean by turning it from mutable to immutable. AFAIK these are identical in effect:

I am of the opinion that mutating variables increases the probability of bugs (as one has to keep track of how the variable is mutated).

That is why I think it is not very good idea to define bindings equal to nil and then assign them. That means that until they are assigned, they cannot be used, which is what I mean as increasing the mental load on whoever is reading the code.

That is why I prefer do-loop and mapcar. But do-loop is a macro, so we might need to see if the macro creates garbage. mapcar is low level and very fast.

In this case, I suspect that mapcan will be slightly faster.

But I am happy with cl-loop. Much better than using do-list. cl-loop is basically for python people ;) as it mimics a comprehension.

This might be relevant to speeding up org-roam

https://mahmoodsh.com/efficiently_parsing_org_files.html

This might be relevant to speeding up org-roam

https://mahmoodsh.com/efficiently_parsing_org_files.html

As yantar92 pointed out on the Reddit discussion, that article does one thing wrong: (let ((major-mode 'org-mode))). Alas, you should actually call (org-mode) to guarantee it's sane.

But yes, some of the other tricks are good. I said last month (#2547 (comment)) that it'd make sense to rework org-roam-db-sync to not use org-roam-with-file, but rather org-roam-with-temp-buffer.

Finally, the checks pass. The tests did good.

Added debatable fifth commit that sets org-roam-list-files-commands to nil by default.

Since it is my impression these alternative code paths will be unnecessary -- they only ever existed because the elisp version was thought to be slow, right?

The bottleneck all along seems to have been org-roam-file-p, which the other code paths still apply -- and the elisp path no longer does, hence being faster.

Tests pass.

Two things I can do after this is merged:

• I can make a quick PR for simplifying org-roam-file-p, similar to cabb0ee.

• I can start work on some optional async code-flow like https://github.com/meedstrom/org-roam-async.

  • It could remain an extension, but either way, I'd want to figure out how that would best work, before I want to touch org-roam-node-read--completions and all its hairy callees.

    • Especially because it seems like there should be some kind of after-sync-hook which just runs on after-save-hook by default -- along with DB sync -- but can be deferred for later if async is enabled. Such a hook would be useful for caching completions as soon as the DB is updated.

      I don't know if caching will be necessary, but anyway, first this PR and then some thinking about async and then we'll get to org-roam-node-read--completions.