denote-add-links doesn't use titles as descriptions inside silos

[yetanotherfossman]

Given an existing silo, let's say with the following structure:

.dir-locals.el
20240630T114629--my-target-file__example.org
20240630T115520--test__example.org

Knowing that my-target-file has the title:

#+title: My target file

When I call denote-add-links inside the test file, and as a regexp I input target, I get the following inserted in the buffer:

- [[denote:20240630T114629][my-target-file]]

While I'd expect the title as a description instead of the hyphenated expression, i.e.:

- [[denote:20240630T114629][My target file]]

It seems this is because denote-link--prepare-links uses with-temp-buffer, and thus doesn't inherit the buffer-local value of denote-directory, making the linked files not be recognized as notes. This not-so-elegant patch fixes it for me:

diff --git a/denote.el b/denote.el
index 64e729c..7bfdaac 100644
--- a/denote.el
+++ b/denote.el
@@ -4415,18 +4415,20 @@ When ID-ONLY is non-nil, use a generic link format.

 With optional NO-SORT do not try to sort the inserted lines.
 Otherwise sort lines while accounting for `denote-link-add-links-sort'."
-  (with-temp-buffer
-    (mapc
-     (lambda (file)
-       (let ((description (denote--link-get-description file)))
-         (insert
-          (format
-           denote-link--prepare-links-format
-           (denote-format-link file description current-file-type id-only)))))
-     files)
-    (unless no-sort
-      (sort-lines denote-link-add-links-sort (point-min) (point-max)))
-    (buffer-string)))
+  (let ((dir denote-directory))
+    (with-temp-buffer
+      (setq-local denote-directory dir)
+      (mapc
+       (lambda (file)
+         (let ((description (denote--link-get-description file)))
+           (insert
+            (format
+             denote-link--prepare-links-format
+             (denote-format-link file description current-file-type id-only)))))
+       files)
+      (unless no-sort
+        (sort-lines denote-link-add-links-sort (point-min) (point-max)))
+      (buffer-string))))

 (defun denote-link--insert-links (files current-file-type &optional id-only no-sort)
   "Insert at point a typographic list of links matching FILES.

Note that the bug also affects dynamic blocks, and possibly other things as well.

I'm in Denote 3.0.1 and Org 9.7.3.

Best regards, and thank you for your package!

[protesilaos]

Thank you @yetanotherfossman! I will need to review this tomorrow morning my time. Assuming your diagnosis is correct, perhaps all we need is to use the let inside of the with-temp-buffer? Like this:

(with-temp-buffer
  (let ((denote-directory default-directory)))
  ...)

[yetanotherfossman]

Tested and that works, indeed!

[protesilaos]

From: yetanotherfossman @.***> Date: Sun, 30 Jun 2024 12:57:10 -0700

Tested and that works, indeed!

Good! Though this is still problematic because we cannot know how many local variables we need to 'let' bind. I will check what is happening here, because my understanding of 'with-temp-buffer' is that it does not change the 'default-directory' (and thus all other directory-local variables).

-- Protesilaos Stavrou https://protesilaos.com

[yetanotherfossman]

Hmm... Reading the documentation, it seems that a single call to hack-dir-local-variables-non-file-buffer is all we need! That should apply all directory local variables to the temp buffer. And it works in my tests, too.

(I'm reading (elisp) Directory Local Variables)

[protesilaos]

I think I have fixed it by changing how we expand those links. Here is the patch:

From 6cd88356aa2efc58c8bc86d5d75b0a23ff4980eb Mon Sep 17 00:00:00 2001
Message-Id: <6cd88356aa2efc58c8bc86d5d75b0a23ff4980eb.1719831492.git.info@protesilaos.com>
From: Protesilaos Stavrou <info@protesilaos.com>
Date: Mon, 1 Jul 2024 13:54:46 +0300
Subject: [PATCH] Make 'denote-add-links' always read the dir-local variables

We do this by expanding the lists without using an intermediate
buffer. The previous approach was not working as intended in a silo,
because the dir-local value of 'denote-directory' was not present.

Thanks to yetanotherfossman for reporting the problem with
'denote-add-links' when used in a silo and for figuring out that the
temporary buffer was part of the problem. This was done in issue 386:
<https://github.com/protesilaos/denote/issues/386>.
---
 denote.el | 47 +++++++++++++++--------------------------------
 1 file changed, 15 insertions(+), 32 deletions(-)

diff --git a/denote.el b/denote.el
index af65334..9d52647 100644
--- a/denote.el
+++ b/denote.el
@@ -4404,10 +4404,7 @@ ;;;;; Add links matching regexp
 (defvar denote-link--prepare-links-format "- %s\n"
   "Format specifiers for `denote-link-add-links'.")

-;; NOTE 2022-06-16: There is no need to overwhelm the user with options,
-;; though I expect someone to want to change the sort order.
-(defvar denote-link-add-links-sort nil
-  "When t, add REVERSE to `sort-lines' of `denote-link-add-links'.")
+(make-obsolete-variable 'denote-link-add-links-sort nil "3.1.0")

 (defun denote-link--prepare-links (files current-file-type id-only &optional no-sort)
   "Prepare links to FILES from CURRENT-FILE-TYPE.
@@ -4415,32 +4412,17 @@ (defun denote-link--prepare-links (files current-file-type id-only &optional no-

 With optional NO-SORT do not try to sort the inserted lines.
 Otherwise sort lines while accounting for `denote-link-add-links-sort'."
-  (with-temp-buffer
-    (mapc
-     (lambda (file)
-       (let ((description (denote--link-get-description file)))
-         (insert
-          (format
-           denote-link--prepare-links-format
-           (denote-format-link file description current-file-type id-only)))))
-     files)
-    (unless no-sort
-      (sort-lines denote-link-add-links-sort (point-min) (point-max)))
-    (buffer-string)))
-
-(defun denote-link--insert-links (files current-file-type &optional id-only no-sort)
-  "Insert at point a typographic list of links matching FILES.
-
-With CURRENT-FILE-TYPE as a symbol among those specified in
-`denote-file-type' (or the `car' of each element in
-`denote-file-types'), format the link accordingly.  With a nil or
-unknown non-nil value, default to the Org notation.
-
-With ID-ONLY as a non-nil value, produce links that consist only
-of the identifier, thus deviating from CURRENT-FILE-TYPE.
-
-Optional NO-SORT is passed to `denote-link--prepare-links'."
-  (insert (denote-link--prepare-links files current-file-type id-only no-sort)))
+  (let ((links))
+    (dolist (file files)
+      (let ((description (denote--link-get-description file)))
+         (push
+          (format denote-link--prepare-links-format (denote-format-link file description current-file-type id-only))
+          links)))
+    (if no-sort
+        links
+      (sort links #'string-collate-lessp))))
+
+(make-obsolete 'denote-link--insert-links nil "3.1.0")

 ;;;###autoload
 (defun denote-add-links (regexp &optional id-only)
@@ -4460,8 +4442,9 @@ (defun denote-add-links (regexp &optional id-only)
     (user-error "The current file type is not recognized by Denote"))
   (let ((file-type (denote-filetype-heuristics (buffer-file-name))))
     (if-let ((files (denote-directory-files regexp :omit-current))
-             (beg (point)))
-        (denote-link--insert-links files file-type id-only)
+             (links (denote-link--prepare-links files file-type id-only)))
+        (dolist (link links)
+          (insert link))
       (message "No links matching `%s'" regexp))))

 (defalias 'denote-link-insert-links-matching-regexp 'denote-add-links
-- 
2.39.2

[yetanotherfossman]

Works for me! Though, of course, this as is breaks Org dynamic blocks, given that they still use denote-link--insert-links. Maybe a small macro for inserting links would be useful, so as to not write a (dolist (link links) (insert link)) every time?

[protesilaos]

Thank you! This was sloppy on my end, as I did not check if the helper function was used elsewhere. Anyway, I made some other tweaks as well. Here is the latest version:

From d00fb4cd822ea2af1fc4e049eb342b56829f64a7 Mon Sep 17 00:00:00 2001
Message-Id: <d00fb4cd822ea2af1fc4e049eb342b56829f64a7.1719844641.git.info@protesilaos.com>
From: Protesilaos Stavrou <info@protesilaos.com>
Date: Mon, 1 Jul 2024 13:54:46 +0300
Subject: [PATCH] Make 'denote-add-links' always read the dir-local variables

We do this by expanding the lists without using an intermediate
buffer. The previous approach was not working as intended in a silo,
because the dir-local value of 'denote-directory' was not present.

Thanks to yetanotherfossman for reporting the problem with
'denote-add-links' when used in a silo and for figuring out that the
temporary buffer was part of the problem. This was done in issue 386:
<https://github.com/protesilaos/denote/issues/386>.
---
 denote.el | 33 ++++++++++++++-------------------
 1 file changed, 14 insertions(+), 19 deletions(-)

diff --git a/denote.el b/denote.el
index af65334..b0d14f7 100644
--- a/denote.el
+++ b/denote.el
@@ -4404,10 +4404,7 @@ ;;;;; Add links matching regexp
 (defvar denote-link--prepare-links-format "- %s\n"
   "Format specifiers for `denote-link-add-links'.")

-;; NOTE 2022-06-16: There is no need to overwhelm the user with options,
-;; though I expect someone to want to change the sort order.
-(defvar denote-link-add-links-sort nil
-  "When t, add REVERSE to `sort-lines' of `denote-link-add-links'.")
+(make-obsolete-variable 'denote-link-add-links-sort nil "3.1.0")

 (defun denote-link--prepare-links (files current-file-type id-only &optional no-sort)
   "Prepare links to FILES from CURRENT-FILE-TYPE.
@@ -4415,18 +4412,15 @@ (defun denote-link--prepare-links (files current-file-type id-only &optional no-

 With optional NO-SORT do not try to sort the inserted lines.
 Otherwise sort lines while accounting for `denote-link-add-links-sort'."
-  (with-temp-buffer
-    (mapc
-     (lambda (file)
-       (let ((description (denote--link-get-description file)))
-         (insert
-          (format
-           denote-link--prepare-links-format
-           (denote-format-link file description current-file-type id-only)))))
-     files)
-    (unless no-sort
-      (sort-lines denote-link-add-links-sort (point-min) (point-max)))
-    (buffer-string)))
+  (let ((links))
+    (dolist (file files)
+      (let* ((description (denote--link-get-description file))
+             (link (denote-format-link file description current-file-type id-only))
+             (link-as-list-item (format denote-link--prepare-links-format link)))
+         (push link-as-list-item links)))
+    (if no-sort
+        (nreverse links)
+      (sort links #'string-collate-lessp))))

 (defun denote-link--insert-links (files current-file-type &optional id-only no-sort)
   "Insert at point a typographic list of links matching FILES.
@@ -4440,7 +4434,9 @@ (defun denote-link--insert-links (files current-file-type &optional id-only no-s
 of the identifier, thus deviating from CURRENT-FILE-TYPE.

 Optional NO-SORT is passed to `denote-link--prepare-links'."
-  (insert (denote-link--prepare-links files current-file-type id-only no-sort)))
+  (when-let ((links (denote-link--prepare-links files current-file-type id-only no-sort)))
+    (dolist (link links)
+      (insert link))))

 ;;;###autoload
 (defun denote-add-links (regexp &optional id-only)
@@ -4459,8 +4455,7 @@ (defun denote-add-links (regexp &optional id-only)
               (and buffer-file-name (denote-file-has-supported-extension-p buffer-file-name)))
     (user-error "The current file type is not recognized by Denote"))
   (let ((file-type (denote-filetype-heuristics (buffer-file-name))))
-    (if-let ((files (denote-directory-files regexp :omit-current))
-             (beg (point)))
+    (if-let ((files (denote-directory-files regexp :omit-current)))
         (denote-link--insert-links files file-type id-only)
       (message "No links matching `%s'" regexp))))

-- 
2.39.2

[yetanotherfossman]

Working as intended now. Thanks!

[protesilaos]

From: yetanotherfossman @.***> Date: Mon, 1 Jul 2024 08:00:08 -0700

Working as intended now. Thanks!

Great! I just pushed it. Thank you!

-- Protesilaos Stavrou https://protesilaos.com