[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 --]

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

[Ihor Radchenko]

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

>> 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

It is ok to only limit the current patch to fixing the bug in question.
We can continue with more changes later, after merging the more limited patch.

-- 
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: 1854 bytes --]

Thanks for the help on this! I've reworked the changes into two patches
(one for the README and one for tangle) and I think I've covered your
concerns. I also added a unit test for the org-base-buffer-file-name
function to cover the miss on provided buffers. Please let me know if you
have any questions.

Thanks,

Derek


On Mon, Jan 13, 2025 at 10:21 AM Ihor Radchenko <yantar92@posteo.net> wrote:

> Derek Chen-Becker <derek@chen-becker.org> writes:
>
> >> 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
>
> It is ok to only limit the current patch to fixing the bug in question.
> We can continue with more changes later, after merging the more limited
> patch.
>
> --
> 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 #1.2: Type: text/html, Size: 3920 bytes --]

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

From 15c324abce9ce404f18af9a02ce57fab621e8424 Mon Sep 17 00:00:00 2001
From: Derek Chen-Becker <github@chen-becker.org>
Date: Mon, 13 Jan 2025 19:54:57 -0700
Subject: [PATCH 2/2] 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/lisp/test-org-macs.el (test-org-base-buffer-file-name): Add a
unit test to exercise the behavior of the new org-base-buffer-file-name
function, particulary around handling base vis indirect buffers.

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               |  9 +++++++++
 testing/lisp/test-ob-tangle.el | 21 +++++++++++++++++++++
 testing/lisp/test-org-macs.el  | 28 ++++++++++++++++++++++++++++
 4 files changed, 59 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..f2c555e1d 100644
--- a/lisp/org-macs.el
+++ b/lisp/org-macs.el
@@ -1821,6 +1821,15 @@ 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."
+  (if (buffer-base-buffer BUFFER)
+      (buffer-file-name (buffer-base-buffer BUFFER))
+    (buffer-file-name BUFFER)))
+
 (provide 'org-macs)
 
 ;; Local variables:
diff --git a/testing/lisp/test-ob-tangle.el b/testing/lisp/test-ob-tangle.el
index 4c953b15d..0b7428576 100644
--- a/testing/lisp/test-ob-tangle.el
+++ b/testing/lisp/test-ob-tangle.el
@@ -741,6 +741,27 @@ 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
+    (unwind-protect
+        (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)))
+      ;; Clean up the tangled file with the filename from org-test-with-temp-text-in-file
+      (delete-file (format "%s.el" file))))))
+
 (provide 'test-ob-tangle)
 
 ;;; test-ob-tangle.el ends here
diff --git a/testing/lisp/test-org-macs.el b/testing/lisp/test-org-macs.el
index 93f00a4c5..3ef62ba84 100644
--- a/testing/lisp/test-org-macs.el
+++ b/testing/lisp/test-org-macs.el
@@ -145,5 +145,33 @@
                      (butlast (decode-time (org-matcher-time "<+14h>"))
                               3))))))
 
+\f
+;;; Buffers
+
+(ert-deftest test-org-base-buffer-file-name ()
+  "Test `org-base-buffer-file-name'."
+  (should
+   (org-test-with-temp-text-in-file
+    "File"
+    (and
+     ;; Confirm that we get the same answer whether we provide the buffer or use the default
+     (equal file (org-base-buffer-file-name))
+     (equal file (org-base-buffer-file-name (current-buffer))))))
+  (should
+   (org-test-with-temp-text-in-file
+    "File with capture buffer"
+    (let ((org-capture-templates '(("t" "Test" entry (here) "* Test Header\n\n"))))
+      (org-capture nil "t")
+      (and
+       ;; Confirm that we get the same answer whether we provide the buffer or use the default
+       (equal file (org-base-buffer-file-name))
+       (equal file (org-base-buffer-file-name (current-buffer)))
+       (equal file (org-base-buffer-file-name (buffer-base-buffer (current-buffer))))
+       ))))
+  (should-not
+   (org-test-with-temp-text
+    "Buffer without file"
+    (org-base-buffer-file-name))))
+
 (provide 'test-org-macs)
 ;;; test-org-macs.el ends here
-- 
2.43.0


[-- Attachment #3: 0001-testing-README-Add-details-on-use-of-TEST_NO_AUTOCLE.patch --]
[-- Type: application/octet-stream, Size: 1178 bytes --]

From 8550757aa57bd7ebd2103d39e24f6e10bf787b5e Mon Sep 17 00:00:00 2001
From: Derek Chen-Becker <github@chen-becker.org>
Date: Sun, 12 Jan 2025 15:27:59 -0700
Subject: [PATCH 1/2] testing/README: Add details on use of TEST_NO_AUTOCLEAN

* testing/README: Add a section on the use of the TEST_NO_AUTOCLEAN make variable

While doing some testing I needed this functionality to debug a test and
found this variable was helpful. Add a section to the README so that others
may benefit without having to dig through makefiles.

TINYCHANGE
---
 testing/README | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/testing/README b/testing/README
index 1c4dd9e76..72a28d75c 100644
--- a/testing/README
+++ b/testing/README
@@ -86,6 +86,15 @@ 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.
+
+#+begin_src sh :dir (expand-file-name "..") :results silent
+make TEST_NO_AUTOCLEAN=1 test-dirty
+#+end_src
+
 * Interactive testing from within Emacs
 
 To run the Org mode test suite from a current Emacs instance simply
-- 
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:

> Thanks for the help on this! I've reworked the changes into two patches
> (one for the README and one for tangle) and I think I've covered your
> concerns. I also added a unit test for the org-base-buffer-file-name
> function to cover the miss on provided buffers. Please let me know if you
> have any questions.

Thanks!
Although, I am afraid that the second set of unit tests is a bit much
- they no longer fit within TINYCHANGE (15-20 significant LOC).
We have a legal limit on how much code we can accept without requiring
copyright assignment. See https://orgmode.org/worg/org-contribute.html#copyright

More comments below.

> * lisp/ob-tangle.el (org-babel-tangle): Update to utilize the new
> org-base-buffer-file-name function.

Our convention is to quote Elisp symbols in commit messages like `this'.

> 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

Another convention is double space between sentences.

> +(defun org-base-buffer-file-name (&optional BUFFER)

Elisp convention is to use lower case for function arguments.
Upper case in the docstring is used to highlight the function arguments;
it does not mean that the arguments themselves need to be in upper case.

> +  (if (buffer-base-buffer BUFFER)
> +      (buffer-file-name (buffer-base-buffer BUFFER))
> +    (buffer-file-name BUFFER)))

Nit: Can also use `if-let*' to avoid calling `buffer-base-buffer' twice.

> +;; 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
> +    (unwind-protect
> +        (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)))
> +      ;; Clean up the tangled file with the filename from org-test-with-temp-text-in-file
> +      (delete-file (format "%s.el" file))))))

This is not the best style.
While technically `file' variable will be let-bound within the
`org-test-with-temp-text-in-file' body, it is an internal implementation
detail that we should not rely upon. Instead, it is a good idea to
examine `buffer-file-name' inside the macro body to get the file name.

Also, what if "%s.el" file does not exist? Say, tangling does happen,
but to a wrong file. Note that `delete-file' does not throw any error
when there is nothing to delete.

-- 
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: 4041 bytes --]

Thanks for the comments! This is my first real foray into non-trivial elisp
so I really appreciate your patience and help. Let me try and get these
addressed.

Cheers,

derek

On Tue, Jan 14, 2025 at 10:54 AM Ihor Radchenko <yantar92@posteo.net> wrote:

> Derek Chen-Becker <derek@chen-becker.org> writes:
>
> > Thanks for the help on this! I've reworked the changes into two patches
> > (one for the README and one for tangle) and I think I've covered your
> > concerns. I also added a unit test for the org-base-buffer-file-name
> > function to cover the miss on provided buffers. Please let me know if you
> > have any questions.
>
> Thanks!
> Although, I am afraid that the second set of unit tests is a bit much
> - they no longer fit within TINYCHANGE (15-20 significant LOC).
> We have a legal limit on how much code we can accept without requiring
> copyright assignment. See
> https://orgmode.org/worg/org-contribute.html#copyright
>
> More comments below.
>
> > * lisp/ob-tangle.el (org-babel-tangle): Update to utilize the new
> > org-base-buffer-file-name function.
>
> Our convention is to quote Elisp symbols in commit messages like `this'.
>
> > 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
>
> Another convention is double space between sentences.
>
> > +(defun org-base-buffer-file-name (&optional BUFFER)
>
> Elisp convention is to use lower case for function arguments.
> Upper case in the docstring is used to highlight the function arguments;
> it does not mean that the arguments themselves need to be in upper case.
>
> > +  (if (buffer-base-buffer BUFFER)
> > +      (buffer-file-name (buffer-base-buffer BUFFER))
> > +    (buffer-file-name BUFFER)))
>
> Nit: Can also use `if-let*' to avoid calling `buffer-base-buffer' twice.
>
> > +;; 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
> > +    (unwind-protect
> > +        (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)))
> > +      ;; Clean up the tangled file with the filename from
> org-test-with-temp-text-in-file
> > +      (delete-file (format "%s.el" file))))))
>
> This is not the best style.
> While technically `file' variable will be let-bound within the
> `org-test-with-temp-text-in-file' body, it is an internal implementation
> detail that we should not rely upon. Instead, it is a good idea to
> examine `buffer-file-name' inside the macro body to get the file name.
>
> Also, what if "%s.el" file does not exist? Say, tangling does happen,
> but to a wrong file. Note that `delete-file' does not throw any error
> when there is nothing to delete.
>
> --
> 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: 6585 bytes --]