[BUG] Cannot tangle src block in capture buffer [9.7.6]

[BUG] Cannot tangle src block in capture buffer [9.7.6]

[Dilip]

[-- Attachment #1: Type: text/plain, Size: 752 bytes --]

You get ~Wrong type argument: stringp, nil~ error when you
=org-babel-tangle= on a source block in capture buffer.

To reproduce:

1. set a template
(setq org-capture-templates
'(("t" "Test" entry (file "/tmp/debug.org")
"* TODO %?\n %i\n %a")))

2. Create a source block (C-c C-, s) add :tangle header
3. Tangle it. (C-u C-c C-v C-t)

In fact,at the same time with capture buffer active if you go the file
(/tmp/debug.org) and do tangle on so far written src block it tangles
properly.

Emacs : GNU Emacs 31.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.42, cairo version 1.18.0)
Package: Org mode version 9.7.6 (9.7.6-7a4527 @ /nix/store/w3f4jzqljh96z8axnp7fc814vg47879b-emacs-packages-deps/share/emacs/site-lisp/elpa/org-9.7.6/)

Best,
Dilip

[-- Attachment #2: Type: text/html, Size: 1445 bytes --]

Re: [BUG] Cannot tangle src block in capture buffer [9.7.6]

[Ihor Radchenko]

Dilip <iDLip@protonmail.com> writes:

> You get ~Wrong type argument: stringp, nil~ error when you
> =org-babel-tangle= on a source block in capture buffer.
>
> To reproduce:
>
> 1. set a template
> (setq org-capture-templates
> '(("t" "Test" entry (file "/tmp/debug.org")
> "* TODO %?\n %i\n %a")))
>
> 2. Create a source block (C-c C-, s) add :tangle header
> 3. Tangle it. (C-u C-c C-v C-t)
>
> In fact,at the same time with capture buffer active if you go the file
> (/tmp/debug.org) and do tangle on so far written src block it tangles
> properly.

That's because capture buffer is an indirect buffer and indirect buffers
are technically not associated with any file.

We should probably fix handling indirect buffers across Org mode.

Confirmed.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [BUG] Cannot tangle src block in capture buffer [9.7.6]

[Derek Chen-Becker]

[-- Attachment #1: Type: text/plain, Size: 1982 bytes --]

On Mon, Aug 05, 2024 at 02:03:50PM +0000, Ihor Radchenko wrote:
> That's because capture buffer is an indirect buffer and indirect buffers
> are technically not associated with any file.
> 
> We should probably fix handling indirect buffers across Org mode.
> 
> Confirmed.
> 

OK, after some debugging it looks like the primary culprit is the assignment of source-file from buffer-file-name. A quick
patch seems to fix it, but I can definitely see a pattern here if org functions are trying to get the filename of the current
buffer (I can submit an official patch if this looks right):

modified   lisp/ob-tangle.el
@@ -269,7 +269,7 @@ matching a regular expression."
        (or (cdr (assq :tangle (nth 2 (org-babel-get-src-block-info 'no-eval))))
    (user-error "Point is not in a source code block"))))
     path-collector
-            (source-file buffer-file-name))
+            (source-file (buffer-file-name (buffer-base-buffer))))
  (mapc ;; map over file-names
  (lambda (by-fn)
    (let ((file-name (car by-fn)))

There are 339 uses of buffer-file-name that I can find, but most are just bare (buffer-file-name). Are there any other cases
besides indirect buffers that we would need to handle? Would it be worth creating a new function "org-buffer-file-name" that
could properly handle indirect buffers and any other special cases, or is it just a search and replace throughout?

Cheers,

Derek

-- 
+---------------------------------------------------------------+
| Derek Chen-Becker                                             |
| http://chen-becker.org                                        |
|                                                               |
| GPG Key available at https://keybase.io/dchenbecker and       |
| https://pgp.mit.edu/pks/lookup?search=derek%40chen-becker.org |
| Fngrprnt: EB8A 6480 F0A3 C8EB C1E7  7F42 AFC5 AFEE 96E4 6ACC  |
+---------------------------------------------------------------+

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [BUG] Cannot tangle src block in capture buffer [9.7.6]

[Ihor Radchenko]

Derek Chen-Becker <derek@chen-becker.org> writes:

> OK, after some debugging it looks like the primary culprit is the assignment of source-file from buffer-file-name. A quick
> patch seems to fix it, but I can definitely see a pattern here if org functions are trying to get the filename of the current
> buffer (I can submit an official patch if this looks right):
>
> modified   lisp/ob-tangle.el
> @@ -269,7 +269,7 @@ matching a regular expression."
>         (or (cdr (assq :tangle (nth 2 (org-babel-get-src-block-info 'no-eval))))
>     (user-error "Point is not in a source code block"))))
>      path-collector
> -            (source-file buffer-file-name))
> +            (source-file (buffer-file-name (buffer-base-buffer))))
>   (mapc ;; map over file-names
>   (lambda (by-fn)
>     (let ((file-name (car by-fn)))

This looks right, yes.

> There are 339 uses of buffer-file-name that I can find, but most are just bare (buffer-file-name). Are there any other cases
> besides indirect buffers that we would need to handle? Would it be worth creating a new function "org-buffer-file-name" that
> could properly handle indirect buffers and any other special cases, or is it just a search and replace throughout?

I can think of two scenarios:

1. indirect Org buffer, as you pointed
2. a new Org buffer not yet associated with file. Even base buffer will
   then have buffer-file-name returning nil

May we have a special function? If it is going to be used 339 times,
definitely yes ;) Although, I'd prefer more telling name like
`org-base-buffer-file-name' (akin the existing `org-with-base-buffer' macro)

-- 
Ihor Radchenko // yantar92,
Org mode maintainer,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [BUG] Cannot tangle src block in capture buffer [9.7.6]

[Derek Chen-Becker]

[-- Attachment #1: Type: text/plain, Size: 2893 bytes --]

OK, I can start working on the new function and plumb it in. I agree that
"org-base-buffer-file-name" would be better. For the case you mentioned
about an Org buffer not yet associated with a file, that seems like a
complication that we can't solve at this level. Do we leave that as a known
issue, or are there straightforward ways to maybe deal with that? It feels
like that may be case-by-case.

Thanks,

Derek

On Mon, Dec 16, 2024 at 10:37 AM Ihor Radchenko <yantar92@posteo.net> wrote:

> Derek Chen-Becker <derek@chen-becker.org> writes:
>
> > OK, after some debugging it looks like the primary culprit is the
> assignment of source-file from buffer-file-name. A quick
> > patch seems to fix it, but I can definitely see a pattern here if org
> functions are trying to get the filename of the current
> > buffer (I can submit an official patch if this looks right):
> >
> > modified   lisp/ob-tangle.el
> > @@ -269,7 +269,7 @@ matching a regular expression."
> >         (or (cdr (assq :tangle (nth 2 (org-babel-get-src-block-info
> 'no-eval))))
> >     (user-error "Point is not in a source code block"))))
> >      path-collector
> > -            (source-file buffer-file-name))
> > +            (source-file (buffer-file-name (buffer-base-buffer))))
> >   (mapc ;; map over file-names
> >   (lambda (by-fn)
> >     (let ((file-name (car by-fn)))
>
> This looks right, yes.
>
> > There are 339 uses of buffer-file-name that I can find, but most are
> just bare (buffer-file-name). Are there any other cases
> > besides indirect buffers that we would need to handle? Would it be worth
> creating a new function "org-buffer-file-name" that
> > could properly handle indirect buffers and any other special cases, or
> is it just a search and replace throughout?
>
> I can think of two scenarios:
>
> 1. indirect Org buffer, as you pointed
> 2. a new Org buffer not yet associated with file. Even base buffer will
>    then have buffer-file-name returning nil
>
> May we have a special function? If it is going to be used 339 times,
> definitely yes ;) Although, I'd prefer more telling name like
> `org-base-buffer-file-name' (akin the existing `org-with-base-buffer'
> macro)
>
> --
> Ihor Radchenko // yantar92,
> Org mode maintainer,
> Learn more about Org mode at <https://orgmode.org/>.
> Support Org development at <https://liberapay.com/org-mode>,
> or support my work at <https://liberapay.com/yantar92>
>


-- 
+---------------------------------------------------------------+
| Derek Chen-Becker                                             |
| GPG Key available at https://keybase.io/dchenbecker and       |
| https://pgp.mit.edu/pks/lookup?search=derek%40chen-becker.org |
| Fngrprnt: EB8A 6480 F0A3 C8EB C1E7  7F42 AFC5 AFEE 96E4 6ACC  |
+---------------------------------------------------------------+

[-- Attachment #2: Type: text/html, Size: 4963 bytes --]

Re: [BUG] Cannot tangle src block in capture buffer [9.7.6]

[Ihor Radchenko]

Derek Chen-Becker <derek@chen-becker.org> writes:

> OK, I can start working on the new function and plumb it in. I agree that
> "org-base-buffer-file-name" would be better.

Thanks in advance!
Do note that we still cannot blindly replace every instance of
buffer-file-name with the new function. At least a cursory case-by-case
check is necessary to make sure that logic will not be broken.

> ... For the case you mentioned
> about an Org buffer not yet associated with a file, that seems like a
> complication that we can't solve at this level. Do we leave that as a known
> issue, or are there straightforward ways to maybe deal with that? It feels
> like that may be case-by-case.

It will certainly be case-by-case. Sometimes, the code can only be
evaluated in a file buffer. Ever. Sometimes not.
This one is rather theoretical.

-- 
Ihor Radchenko // yantar92,
Org mode maintainer,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [BUG] Cannot tangle src block in capture buffer [9.7.6]

[Derek Chen-Becker]

[-- Attachment #1: Type: text/plain, Size: 1289 bytes --]

On Thu, Dec 19, 2024 at 12:16 PM Ihor Radchenko <yantar92@posteo.net> wrote:

>
> Thanks in advance!
> Do note that we still cannot blindly replace every instance of
> buffer-file-name with the new function. At least a cursory case-by-case
> check is necessary to make sure that logic will not be broken.]


Understood, that's clear to me. I plan on making the common function
something that will resolve the buffer filename whether the buffer is
direct or indirect. For the case of a new Org buffer I'm not sure what I
can return other than nil. For functions that require a filename, I think
we'll have to evaluate those on a case-by-case basis and figure out if we
can provide a helper/wrapper that can simplify error reporting when a
filename is required. Where would be the best place to put the
`org-base-buffer-file-name` function?

Thanks,

Derek


-- 
+---------------------------------------------------------------+
| Derek Chen-Becker                                             |
| GPG Key available at https://keybase.io/dchenbecker and       |
| https://pgp.mit.edu/pks/lookup?search=derek%40chen-becker.org |
| Fngrprnt: EB8A 6480 F0A3 C8EB C1E7  7F42 AFC5 AFEE 96E4 6ACC  |
+---------------------------------------------------------------+

[-- Attachment #2: Type: text/html, Size: 3096 bytes --]

Re: [BUG] Cannot tangle src block in capture buffer [9.7.6]

[Ihor Radchenko]

Derek Chen-Becker <derek@chen-becker.org> writes:

> ... Where would be the best place to put the
> `org-base-buffer-file-name` function?

org-macs.el

-- 
Ihor Radchenko // yantar92,
Org mode maintainer,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [BUG] Cannot tangle src block in capture buffer [9.7.6]

[Derek Chen-Becker]

[-- Attachment #1: Type: text/plain, Size: 1437 bytes --]

The holidays got the better of me and I'm just coming back to this. I've
made the code changes to add the new function for resolving the base name,
but I was thinking about adding a test for it as well. I'm looking at the
examples in test-ob-tangle.el and I think I understand some of it, but is
there any documentation on writing tests? The README under testing has some
useful info for how to run the tests, but not much about how tests should
be structured or any important facilities to be aware of.

Thanks,

Derek

On Tue, Dec 24, 2024 at 2:13 AM Ihor Radchenko <yantar92@posteo.net> wrote:

> Derek Chen-Becker <derek@chen-becker.org> writes:
>
> > ... Where would be the best place to put the
> > `org-base-buffer-file-name` function?
>
> org-macs.el
>
> --
> Ihor Radchenko // yantar92,
> Org mode maintainer,
> Learn more about Org mode at <https://orgmode.org/>.
> Support Org development at <https://liberapay.com/org-mode>,
> or support my work at <https://liberapay.com/yantar92>
>


-- 
+---------------------------------------------------------------+
| Derek Chen-Becker                                             |
| GPG Key available at https://keybase.io/dchenbecker and       |
| https://pgp.mit.edu/pks/lookup?search=derek%40chen-becker.org |
| Fngrprnt: EB8A 6480 F0A3 C8EB C1E7  7F42 AFC5 AFEE 96E4 6ACC  |
+---------------------------------------------------------------+

[-- Attachment #2: Type: text/html, Size: 3348 bytes --]

Re: [BUG] Cannot tangle src block in capture buffer [9.7.6]

[Ihor Radchenko]

Derek Chen-Becker <derek@chen-becker.org> writes:

> The holidays got the better of me and I'm just coming back to this. I've
> made the code changes to add the new function for resolving the base name,
> but I was thinking about adding a test for it as well. I'm looking at the
> examples in test-ob-tangle.el and I think I understand some of it, but is
> there any documentation on writing tests? The README under testing has some
> useful info for how to run the tests, but not much about how tests should
> be structured or any important facilities to be aware of.

1. testing/org-test.el contains some helper functions and macros that
   can be used.
2. otherwise, just follow by example

-- 
Ihor Radchenko // yantar92,
Org mode maintainer,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [BUG] Cannot tangle src block in capture buffer [9.7.6]

[Derek Chen-Becker]

[-- Attachment #1.1: Type: text/plain, Size: 218 bytes --]

OK, I've attached the patch. It wasn't clear from
https://orgmode.org/worg/org-contribute.html#first-patch whether this
should be sent in the thread or as a new thread. I can start a new one if
needed.

Thanks,

Derek

[-- Attachment #1.2: Type: text/html, Size: 825 bytes --]

[-- Attachment #2: 0001-ob-tangle-Ensure-proper-tangling-within-a-capture-bu.patch --]
[-- Type: application/octet-stream, Size: 4106 bytes --]

From fbc0fe66587228401b9ce042a0b7d361c3ec2a8f Mon Sep 17 00:00:00 2001
From: Derek Chen-Becker <github@chen-becker.org>
Date: Wed, 1 Jan 2025 20:06:10 -0700
Subject: [PATCH] ob-tangle: Ensure proper tangling within a capture buffer

* lisp/org-macs.el (org-base-buffer-file-name): Add a function to properly
resolve the filename for both direct and indirect buffers
* lisp/ob-tangle.el (org-babel-tangle): Update to utilize the new
org-base-buffer-file-name function
* testing/lisp/test-ob-tangle.el (ob-tangle/tangle-from-capture-buffer): Add
a unit test to cover the use case of tangling within a capture buffer
* testing/README: Add a section on the use of the TEST_NO_AUTOCLEAN make variable

The previous use of buffer-file-name would fail inside of a capture buffer
because it is indirect and does not have an associated filename. This
appears to be a common pitfall across Org, so I've added a helper function
to resolve the filename for both indirect and direct buffers so that it can
be used in other places within the codebase.

TINYCHANGE

Reported-by: Dilip <iDLip@protonmail.com>
Link: https://list.orgmode.org/87msfxd81c.fsf@localhost/T/#t
---
 lisp/ob-tangle.el              |  2 +-
 lisp/org-macs.el               |  7 +++++++
 testing/README                 |  5 +++++
 testing/lisp/test-ob-tangle.el | 18 ++++++++++++++++++
 4 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/lisp/ob-tangle.el b/lisp/ob-tangle.el
index 38cad78ab..64bdced84 100644
--- a/lisp/ob-tangle.el
+++ b/lisp/ob-tangle.el
@@ -269,7 +269,7 @@ matching a regular expression."
 	       (or (cdr (assq :tangle (nth 2 (org-babel-get-src-block-info 'no-eval))))
 		   (user-error "Point is not in a source code block"))))
 	    path-collector
-            (source-file buffer-file-name))
+            (source-file (org-base-buffer-file-name)))
 	(mapc ;; map over file-names
 	 (lambda (by-fn)
 	   (let ((file-name (car by-fn)))
diff --git a/lisp/org-macs.el b/lisp/org-macs.el
index 044c49efb..6577bcc4b 100644
--- a/lisp/org-macs.el
+++ b/lisp/org-macs.el
@@ -1821,6 +1821,13 @@ indirectly called by the latter."
                    (eq (window-frame) (window-frame window))))
       (window--display-buffer buffer window 'reuse alist))))
 
+(defun org-base-buffer-file-name (&optional BUFFER)
+  "Resolve the base file name for the provided BUFFER.
+If BUFFER is not provided, default to the current buffer.  If
+BUFFER does not have a file name associated with it (e.g. a
+transient buffer) then return nil."
+  (buffer-file-name (buffer-base-buffer BUFFER)))
+
 (provide 'org-macs)
 
 ;; Local variables:
diff --git a/testing/README b/testing/README
index 1c4dd9e76..8bd3ae3fc 100644
--- a/testing/README
+++ b/testing/README
@@ -86,6 +86,11 @@ Ran 2 tests, 2 results as expected (2017-12-28 15:04:45+0100)
 ...
 #+end_example
 
+** Keep test artifacts
+
+Set the ~TEST_NO_AUTOCLEAN~ variable to leave the test directory
+around for inspection.
+
 * Interactive testing from within Emacs
 
 To run the Org mode test suite from a current Emacs instance simply
diff --git a/testing/lisp/test-ob-tangle.el b/testing/lisp/test-ob-tangle.el
index 4c953b15d..171ed7780 100644
--- a/testing/lisp/test-ob-tangle.el
+++ b/testing/lisp/test-ob-tangle.el
@@ -741,6 +741,24 @@ another block
                              bib))))
       (delete-file file))))
 
+;; See https://list.orgmode.org/87msfxd81c.fsf@localhost/T/#t
+(ert-deftest ob-tangle/tangle-from-capture-buffer ()
+  "Test tangling of source blocks from within a capture buffer.
+This is to ensure that we properly resolve the buffer name."
+  (org-test-with-temp-text-in-file
+   "* Header\n\nCapture after this point:\n<point>"
+   (should
+    (progn
+      (let ((org-capture-templates '(("t" "Test" entry (here) "* Test Header\n\n"))))
+        (org-capture nil "t")
+        (goto-char (point-max))
+        (insert "
+#+begin_src elisp :tangle yes :comments org
+  (message \"FOO\")
+#+end_src")
+        (search-backward "message")
+        (org-babel-tangle 4))))))
+
 (provide 'test-ob-tangle)
 
 ;;; test-ob-tangle.el ends here
-- 
2.43.0

Re: [BUG] Cannot tangle src block in capture buffer [9.7.6]

[Ihor Radchenko]

Derek Chen-Becker <derek@chen-becker.org> writes:

> OK, I've attached the patch. It wasn't clear from
> https://orgmode.org/worg/org-contribute.html#first-patch whether this
> should be sent in the thread or as a new thread. I can start a new one if
> needed.

Thanks!
The same thread is the right place - we want everything related to the
same bug report discussed in the same thread.
Also, see https://orgmode.org/worg/org-contribute.html#fixbug

> +(defun org-base-buffer-file-name (&optional BUFFER)
> +  "Resolve the base file name for the provided BUFFER.
> +If BUFFER is not provided, default to the current buffer.  If
> +BUFFER does not have a file name associated with it (e.g. a
> +transient buffer) then return nil."
> +  (buffer-file-name (buffer-base-buffer BUFFER)))

What if BUFFER is provided, but that buffer is already a base buffer?
Your function will then return file name for _current buffer_, not BUFFER.

Also, what about other places in the code that use `buffer-file-name'?
We may want to use the new function there as well.

> +** Keep test artifacts
> +
> +Set the ~TEST_NO_AUTOCLEAN~ variable to leave the test directory
> +around for inspection.

Thanks for helping to improve the documentation here, but may you (1)
sepearate it into a new patch (this change is not relevant to
`buffer-file-name' bug); (2) maybe give an example of how to set
TEST_NO_AUTOCLEAN.

> +;; See https://list.orgmode.org/87msfxd81c.fsf@localhost/T/#t
> +(ert-deftest ob-tangle/tangle-from-capture-buffer ()
> +  "Test tangling of source blocks from within a capture buffer.
> +This is to ensure that we properly resolve the buffer name."
> +  (org-test-with-temp-text-in-file
> +   "* Header\n\nCapture after this point:\n<point>"
> +   (should
> +    (progn
> +      (let ((org-capture-templates '(("t" "Test" entry (here) "* Test Header\n\n"))))
> +        (org-capture nil "t")
> +        (goto-char (point-max))
> +        (insert "
> +#+begin_src elisp :tangle yes :comments org
> +  (message \"FOO\")
> +#+end_src")
> +        (search-backward "message")
> +        (org-babel-tangle 4))))))

This will leave the tangled file lying around.
Please explicitly remove it via
(unwind-protect ...
 (delete-file <tangled-file-name>)

See how `org-test-with-temp-text-in-file' macro does it.

-- 
Ihor Radchenko // yantar92,
Org mode maintainer,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [BUG] Cannot tangle src block in capture buffer [9.7.6]

[Derek Chen-Becker]

[-- Attachment #1: Type: text/plain, Size: 1862 bytes --]

On Sun, Jan 12, 2025 at 9:43 AM Ihor Radchenko <yantar92@posteo.net> wrote:


> What if BUFFER is provided, but that buffer is already a base buffer?
> Your function will then return file name for _current buffer_, not BUFFER.
>

Hmmm, I thought I tested that but I can take a look and fix it.


>
> Also, what about other places in the code that use `buffer-file-name'?
> We may want to use the new function there as well.
>

Definitely. There are hundreds of places in the code where the same pattern
applies. Per our previous discussion in the thread, though, I thought it
would be better to start small and incrementally work through them. If you
want me to include more changes,I'm going to need to help sorting all of
these out:

❯ rg -q --stats buffer-file-name
185 matches
182 matched lines


> Thanks for helping to improve the documentation here, but may you (1)
> sepearate it into a new patch (this change is not relevant to
> `buffer-file-name' bug); (2) maybe give an example of how to set
> TEST_NO_AUTOCLEAN.
>

Sure, I'll do both.


> This will leave the tangled file lying around.
> Please explicitly remove it via
> (unwind-protect ...
>  (delete-file <tangled-file-name>)
>
> See how `org-test-with-temp-text-in-file' macro does it.
>

Yes, my apologies, I had originally done this and stashed that part of the
change while debugging the test. I'll fix that, too.

Cheers,

Derek

-- 
+---------------------------------------------------------------+
| Derek Chen-Becker                                             |
| GPG Key available at https://keybase.io/dchenbecker and       |
| https://pgp.mit.edu/pks/lookup?search=derek%40chen-becker.org |
| Fngrprnt: EB8A 6480 F0A3 C8EB C1E7  7F42 AFC5 AFEE 96E4 6ACC  |
+---------------------------------------------------------------+

[-- Attachment #2: Type: text/html, Size: 4689 bytes --]