Hi Ihor, Thank you for the detailed feedback. I've addressed all the points you raised:
1. Elisp symbol formatting: Updated the commit message to quote Elisp symbols with backticks throughout (e.g., `org-babel-expand-noweb-references'). 2. ORG-NEWS entry* Added an entry documenting the removal of noweb expansion from `org-babel-lob-ingest'. I placed it under "Important announcements and breaking changes" and noted that it's unlikely to affect most users but may be visible to code accessing `org-babel-library-of-babel' directly. 3. Docstring clarification: Updated the docstring for `org-babel-expand-noweb-references' to explicitly state that CONTEXT does not affect whether the top-level block is expanded - that's determined by the caller and the block's own :noweb setting. The CONTEXT parameter only determines which noweb header arguments are honored when recursively expanding nested references within referenced blocks. Regarding the 2017 issue you mentioned (https://lists.gnu.org/archive/html/emacs-orgmode/2017-09/msg00361.html): I wanted to confirm what specific behavior I should verify to ensure my changes don't reintroduce that problem. Could you clarify what aspect of that issue I should test against, or point me to the commit that fixed it so I can review the solution? I'm attaching the updated patch. Please let me know if there's anything else that needs adjustment. Thanks again for your guidance! Best regards, Dominic On Sat, Oct 11, 2025 at 7:33 AM Ihor Radchenko <[email protected]> wrote: > > dominic meiser <[email protected]> writes: > > >> > * lisp/ob-lob.el (org-babel-lob-ingest): Pass :eval context when > >> > ingesting named blocks into the Library of Babel. > >> > >> We should probably drop calling noweb expansion altogether here. > >> But we need to make sure that > >> https://lists.gnu.org/archive/html/emacs-orgmode/2017-09/msg00361.html > >> does not re-appear. > > > > Is there any additional due diligence I need to do regarding the issue > > reference here? > > Sorry, I do not understand the question. Could you please elaborate? > > > In other news, I've submitted the assignment form to fsf and my > > understanding is that the process is almost complete. > > Great! Remember that you can always ask me to push them, if the process > is stuck for any reason. > > > Subject: [PATCH] ob-core: Fix noweb tangle recursive expansion > > > > * lisp/ob-tangle.el (org-babel-tangle-single-block): Pass :tangle as > > CONTEXT to org-babel-expand-noweb-references. > > Please quote Elisp symbols like `org-babel-expand-noweb-references'. See > https://orgmode.org/worg/org-contribute.html#org76b803f > > > * lisp/ob-lob.el (org-babel-lob-ingest): Remove premature noweb > > expansion. Both org-babel-execute-src-block and > > org-babel-exp-do-export handle noweb expansion with appropriate > > context when the block is actually used, making expansion during > > ingest redundant and preventing context-specific behavior. > > Please document this breaking change in etc/ORG-NEWS. > This is actually not very breaking as we give no promises about what > exactly is inside library of babel, but still a good idea to warn users > just in case if some code is using `org-babel-library-of-babel' directly. > > > -(defun org-babel-expand-noweb-references (&optional info parent-buffer) > > +(defun org-babel-expand-noweb-references (&optional info parent-buffer > > context) > > "Expand Noweb references in the body of the current source code block. > > > > When optional argument INFO is non-nil, use the block defined by INFO > > @@ -3146,6 +3154,20 @@ instead. > > The block is assumed to be located in PARENT-BUFFER or current buffer > > \(when PARENT-BUFFER is nil). > > > > +CONTEXT specifies the context of expansion and can be one of :tangle, > > +:export, or :eval. When CONTEXT is nil, it defaults to :eval. > > + > > +The context determines which noweb header arguments are honored when > > +recursively expanding nested references: > > +- :tangle context: expands blocks with :noweb tangle, :noweb yes, etc. > > +- :export context: expands blocks with :noweb export, :noweb yes, etc. > > +- :eval context: expands blocks with :noweb eval, :noweb yes, etc. > > + > > +This is important for recursive expansion: when a block with :noweb tangle > > +references another block that also contains noweb references, those nested > > +references should only be expanded if the referenced block's :noweb setting > > +permits expansion in the tangle context. > > One potentially confusing thing here is that CONTEXT does not apply to > top-level expansion. We may need to highlight this fact in the docstring. > > -- > 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>
From 0f649ef9e54881d236bde17d4b19b60f7df2c4ab Mon Sep 17 00:00:00 2001 From: Dominic Meiser <[email protected]> Date: Tue, 30 Sep 2025 07:33:10 -0600 Subject: [PATCH] ob-core: Fix noweb tangle recursive expansion * lisp/ob-core.el (org-babel-expand-noweb-references): Add optional CONTEXT parameter (third parameter, after INFO and PARENT-BUFFER) specifying the expansion context (:tangle, :export, or :eval). Default to :eval when not provided for backward compatibility. Use the provided context when recursively expanding nested noweb references. Update documentation to explain the context parameter and its role in recursive expansion. (org-babel--expand-body): Add documentation explaining why :eval context is used for code evaluation, including during export and tangling when results need to be generated. (org-babel-check-confirm-evaluate): Pass :eval as CONTEXT. (org-babel-expand-src-block): Pass :eval as CONTEXT. * lisp/ob-tangle.el (org-babel-tangle-single-block): Pass :tangle as CONTEXT to `org-babel-expand-noweb-references'. * lisp/ob-exp.el (org-babel-exp-process-buffer): Pass :export as CONTEXT when expanding inline source blocks during export. (org-babel-exp-code): Pass :export as CONTEXT when expanding code blocks for export. (org-babel-exp-results): Pass :eval as CONTEXT when evaluating blocks during export to generate results. * lisp/ob-lob.el (org-babel-lob-ingest): Remove premature noweb expansion. Both `org-babel-execute-src-block' and `org-babel-exp-do-export' handle noweb expansion with appropriate context when the block is actually used, making expansion during ingest redundant and preventing context-specific behavior. * etc/ORG-NEWS: Document removal of noweb expansion from `org-babel-lob-ingest'. * testing/lisp/test-ob-tangle.el (ob-tangle/expand-headers-as-noweb-references): Pass :tangle as CONTEXT to match tangle expansion behavior. (ob-tangle/noweb-tangle-recursive-expansion): New test verifying that :noweb tangle recursively expands nested noweb references during tangling. (ob-tangle/noweb-tangle-vs-export-contexts): New test verifying that noweb expansion respects different contexts. * testing/lisp/test-ob.el (test-ob/noweb-expansion): Pass :eval as CONTEXT in all test invocations to explicitly specify evaluation context. Previously, noweb expansion during tangling would fail to recursively expand nested references in blocks marked with :noweb tangle. The expansion context was hardcoded to :eval during recursive calls, meaning nested blocks would only expand if their :noweb setting permitted expansion in the :eval context. For example, when tangling a block containing <<ref>> where the referenced block has :noweb tangle and itself contains noweb references, those nested references would not expand because the recursive call used :eval context instead of :tangle. The context parameter determines which :noweb header argument values trigger expansion: - :tangle context: "tangle", "yes", "no-export", "strip-export", etc. - :export context: "yes", "strip-tangle" - :eval context: "eval", "yes", "no-export", "strip-export", etc. The CONTEXT parameter is optional and placed after INFO and PARENT-BUFFER to maintain backward compatibility with third-party code that may call `org-babel-expand-noweb-references' directly. Reported-by: Dominic Meiser <[email protected]> Link: https://list.orgmode.org/[email protected]/ --- etc/ORG-NEWS | 13 ++++++ lisp/ob-core.el | 46 ++++++++++++++++---- lisp/ob-exp.el | 6 +-- lisp/ob-lob.el | 4 -- lisp/ob-tangle.el | 3 +- testing/lisp/test-ob-tangle.el | 77 +++++++++++++++++++++++++++++++++- testing/lisp/test-ob.el | 16 +++---- 7 files changed, 139 insertions(+), 26 deletions(-) diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS index 87fbf8f06..2cbdfb02c 100644 --- a/etc/ORG-NEWS +++ b/etc/ORG-NEWS @@ -18,6 +18,19 @@ Please send Org bug reports to mailto:[email protected]. # require user action for most Org mode users. # Sorted from most important to least important. +*** ~org-babel-lob-ingest~ no longer performs noweb expansion when ingesting blocks + +Previously, ~org-babel-lob-ingest~ would expand noweb references when +adding source blocks to the Library of Babel. Now, blocks are stored +with unexpanded noweb references. + +Noweb expansion is handled appropriately when blocks are actually used +via ~org-babel-execute-src-block~ or ~org-babel-exp-do-export~, with +the correct context (~:tangle~, ~:export~, or ~:eval~). + +This change is unlikely to affect most users, but code that directly +accesses ~org-babel-library-of-babel~ may observe the difference. + *** ~org-store-link~ no longer asks to select store function when called noninteractively Previously, when multiple store functions are available to store link diff --git a/lisp/ob-core.el b/lisp/ob-core.el index 1402827e4..7495eee40 100644 --- a/lisp/ob-core.el +++ b/lisp/ob-core.el @@ -765,11 +765,19 @@ Otherwise, return a list with the following pattern: (defun org-babel--expand-body (info) "Expand noweb references in src block and remove any coderefs. The src block is defined by its INFO, as returned by -`org-babel-get-src-block-info'." +`org-babel-get-src-block-info'. + +This function uses the :eval context for noweb expansion because it is +called when code needs to be evaluated, either by `org-babel-execute-src-block' +or `org-babel-confirm-evaluate'. The :eval context is appropriate even during +export or tangling when the code block needs to be evaluated to generate results. +It is distinct from the :export and :tangle contexts which are used when +generating the source code body for display in exported documents or tangled +files." (let ((coderef (nth 6 info)) (expand (if (org-babel-noweb-p (nth 2 info) :eval) - (org-babel-expand-noweb-references info) + (org-babel-expand-noweb-references info nil :eval) (nth 1 info)))) (if (not coderef) expand (replace-regexp-in-string @@ -1003,7 +1011,7 @@ arguments and pop open the results in a preview buffer." (symbol-name (car el2))))))) (body (setf (nth 1 info) (if (org-babel-noweb-p params :eval) - (org-babel-expand-noweb-references info) (nth 1 info)))) + (org-babel-expand-noweb-references info nil :eval) (nth 1 info)))) (expand-cmd (intern (concat "org-babel-expand-body:" lang))) (assignments-cmd (intern (concat "org-babel-variable-assignments:" lang))) @@ -1143,7 +1151,7 @@ session." (user-error "No src code block at point") (setf (nth 1 info) (if (org-babel-noweb-p params :eval) - (org-babel-expand-noweb-references info) + (org-babel-expand-noweb-references info nil :eval) (nth 1 info))))) (session (cdr (assq :session params))) (dir (cdr (assq :dir params))) @@ -1513,7 +1521,7 @@ CONTEXT specifies the context of evaluation. It can be `:eval', (lang (nth 0 info)) (params (nth 2 info)) (body (if (org-babel-noweb-p params context) - (org-babel-expand-noweb-references info) + (org-babel-expand-noweb-references info nil context) (nth 1 info))) (expand-cmd (intern (concat "org-babel-expand-body:" lang))) (assignments-cmd (intern (concat "org-babel-variable-assignments:" @@ -3137,7 +3145,7 @@ CONTEXT may be one of :tangle, :export or :eval." (defvar org-babel-expand-noweb-references--cache-buffer nil "Cons (BUFFER . MODIFIED-TICK) for cached noweb references. See `org-babel-expand-noweb-references--cache'.") -(defun org-babel-expand-noweb-references (&optional info parent-buffer) +(defun org-babel-expand-noweb-references (&optional info parent-buffer context) "Expand Noweb references in the body of the current source code block. When optional argument INFO is non-nil, use the block defined by INFO @@ -3146,6 +3154,24 @@ instead. The block is assumed to be located in PARENT-BUFFER or current buffer \(when PARENT-BUFFER is nil). +CONTEXT specifies the context of expansion and can be one of :tangle, +:export, or :eval. When CONTEXT is nil, it defaults to :eval. + +Note: CONTEXT does not affect whether the top-level block is expanded - +that is determined by the caller and the block's own :noweb setting. +The context only determines which noweb header arguments are honored when +recursively expanding nested references within referenced blocks. + +For recursive expansion: +- :tangle context: expands blocks with :noweb tangle, :noweb yes, etc. +- :export context: expands blocks with :noweb export, :noweb yes, etc. +- :eval context: expands blocks with :noweb eval, :noweb yes, etc. + +This is important for recursive expansion: when a block with :noweb tangle +references another block that also contains noweb references, those nested +references should only be expanded if the referenced block's :noweb setting +permits expansion in the tangle context. + For example the following reference would be replaced with the body of the source-code block named `example-block'. @@ -3173,7 +3199,8 @@ defined by `org-babel-lob'. For example would set the value of argument \"a\" equal to \"9\". Note that these arguments are not evaluated in the current source-code block but are passed literally to the \"example-block\"." - (let* ((parent-buffer (or parent-buffer (current-buffer))) + (let* ((context (or context :eval)) + (parent-buffer (or parent-buffer (current-buffer))) (info (or info (org-babel-get-src-block-info 'no-eval))) (lang (nth 0 info)) (body (nth 1 info)) @@ -3207,8 +3234,9 @@ block but are passed literally to the \"example-block\"." (expand-body (i) ;; Expand body of code represented by block info I. - `(let ((b (if (org-babel-noweb-p (nth 2 ,i) :eval) - (org-babel-expand-noweb-references ,i) + `(let ((b (if (org-babel-noweb-p (nth 2 ,i) context) + (org-babel-expand-noweb-references + ,i parent-buffer context) (nth 1 ,i)))) (if (not comment) b (let ((cs (org-babel-tangle-comment-links ,i))) diff --git a/lisp/ob-exp.el b/lisp/ob-exp.el index ab99541ab..f2c85ca43 100644 --- a/lisp/ob-exp.el +++ b/lisp/ob-exp.el @@ -215,7 +215,7 @@ this template." (string= "yes" (cdr (assq :noweb params)))) (org-babel-expand-noweb-references - info org-babel-exp-reference-buffer) + info org-babel-exp-reference-buffer :export) (nth 1 info))) (goto-char begin) (let ((replacement @@ -423,7 +423,7 @@ replaced with its value." (org-babel-noweb-wrap) "" (nth 1 info)) (if (org-babel-noweb-p (nth 2 info) :export) (org-babel-expand-noweb-references - info org-babel-exp-reference-buffer) + info org-babel-exp-reference-buffer :export) (nth 1 info)))) (org-fill-template (if (eq type 'inline) @@ -462,7 +462,7 @@ inhibit insertion of results into the buffer." (let ((lang (nth 0 info)) (body (if (org-babel-noweb-p (nth 2 info) :eval) (org-babel-expand-noweb-references - info org-babel-exp-reference-buffer) + info org-babel-exp-reference-buffer :eval) (nth 1 info))) (info (copy-sequence info)) (org-babel-current-src-block-location (point-marker))) diff --git a/lisp/ob-lob.el b/lisp/ob-lob.el index 472f9eda1..b0b2cb309 100644 --- a/lisp/ob-lob.el +++ b/lisp/ob-lob.el @@ -59,10 +59,6 @@ should not be inherited from a source block.") (let* ((info (org-babel-get-src-block-info 'no-eval)) (source-name (nth 4 info))) (when source-name - (setf (nth 1 info) - (if (org-babel-noweb-p (nth 2 info) :eval) - (org-babel-expand-noweb-references info) - (nth 1 info))) (let ((source (intern source-name))) (setq org-babel-library-of-babel (cons (cons source info) diff --git a/lisp/ob-tangle.el b/lisp/ob-tangle.el index 4c224743b..24c8f876b 100644 --- a/lisp/ob-tangle.el +++ b/lisp/ob-tangle.el @@ -581,7 +581,8 @@ non-nil, return the full association list to be used by (let ((body (if (org-babel-noweb-p params :tangle) (if (string= "strip-tangle" (cdr (assq :noweb (nth 2 info)))) (replace-regexp-in-string (org-babel-noweb-wrap) "" (nth 1 info)) - (org-babel-expand-noweb-references info)) + (org-babel-expand-noweb-references + info nil :tangle)) (nth 1 info)))) (with-temp-buffer (insert diff --git a/testing/lisp/test-ob-tangle.el b/testing/lisp/test-ob-tangle.el index cd6876370..e7b970c70 100644 --- a/testing/lisp/test-ob-tangle.el +++ b/testing/lisp/test-ob-tangle.el @@ -78,7 +78,7 @@ "Test that references to headers are expanded during noweb expansion." (org-test-at-id "2409e8ba-7b5f-4678-8888-e48aa02d8cb4" (org-babel-next-src-block 2) - (let ((expanded (org-babel-expand-noweb-references))) + (let ((expanded (org-babel-expand-noweb-references nil nil :tangle))) (should (string-match (regexp-quote "simple") expanded)) (should (string-match (regexp-quote "length 14") expanded))))) @@ -764,6 +764,81 @@ This is to ensure that we properly resolve the buffer name." ;; Clean up the tangled file with the filename from org-test-with-temp-text-in-file (delete-file tangle-filename))))) +(ert-deftest ob-tangle/noweb-tangle-recursive-expansion () + "Test that :noweb tangle recursively expands nested noweb references." + (let ((file (make-temp-file "org-tangle-"))) + (unwind-protect + (progn + (org-test-with-temp-text-in-file + (format " +#+begin_src c :tangle %s :noweb tangle +// some code +<<noweb-ref1>> +<<noweb-ref2>> +#+end_src + +#+begin_src c :noweb-ref noweb-ref1 +// code from source block A +#+end_src + +#+begin_src c :noweb-ref noweb-ref2 :noweb tangle +// code from source block B +<<noweb-ref3>> +#+end_src + +#+begin_src c :noweb-ref noweb-ref3 +// code from source block C +#+end_src +" file) + (let ((org-babel-noweb-error-all-langs nil) + (org-babel-noweb-error-langs nil)) + (org-babel-tangle))) + (let ((tangled-content (with-temp-buffer + (insert-file-contents file) + (buffer-string)))) + ;; The tangled output should contain the content from block C + ;; (not the unexpanded <<noweb-ref3>> reference) + (should (string-match-p "// code from source block C" tangled-content)) + ;; The tangled output should NOT contain the unexpanded reference + (should-not (string-match-p "<<noweb-ref3>>" tangled-content)))) + (delete-file file)))) + +(ert-deftest ob-tangle/noweb-tangle-vs-export-contexts () + "Test that noweb expansion respects different contexts during tangle vs export." + (let ((tangle-file (make-temp-file "org-tangle-"))) + (unwind-protect + (progn + (org-test-with-temp-text-in-file + (format " +#+begin_src c :tangle %s :noweb yes +// tangled code +<<tangle-only>> +<<no-export>> +#+end_src + +#+begin_src c :noweb-ref tangle-only :noweb tangle +// visible during tangle +#+end_src + +#+begin_src c :noweb-ref no-export :noweb no-export +// visible during eval but not export +#+end_src +" tangle-file) + ;; Test tangling + (let ((org-babel-noweb-error-all-langs nil) + (org-babel-noweb-error-langs nil)) + (org-babel-tangle))) + + ;; Check tangled content + (let ((tangled-content (with-temp-buffer + (insert-file-contents tangle-file) + (buffer-string)))) + ;; Should have tangle-only content + (should (string-match-p "// visible during tangle" tangled-content)) + ;; Should have no-export content since :noweb no-export allows tangle context + (should (string-match-p "// visible during eval but not export" tangled-content)))) + (delete-file tangle-file)))) + (provide 'test-ob-tangle) ;;; test-ob-tangle.el ends here diff --git a/testing/lisp/test-ob.el b/testing/lisp/test-ob.el index b786798d6..618b6e0be 100644 --- a/testing/lisp/test-ob.el +++ b/testing/lisp/test-ob.el @@ -829,7 +829,7 @@ x #+begin_src sh bar #+end_src" - (org-babel-expand-noweb-references)))) + (org-babel-expand-noweb-references nil nil :eval)))) ;; Handle :noweb-sep. (should (string= @@ -845,7 +845,7 @@ x #+begin_src sh :noweb-ref foo :noweb-sep \"\" baz #+end_src" - (org-babel-expand-noweb-references)))) + (org-babel-expand-noweb-references nil nil :eval)))) ;; :noweb-ref is extracted from definition, not point of call. (should (string= @@ -869,7 +869,7 @@ x #+begin_src sh :noweb-sep \"\" (+ 1 1) #+end_src" - (org-babel-expand-noweb-references)))) + (org-babel-expand-noweb-references nil nil :eval)))) ;; Handle recursive expansion. (should (equal "baz" @@ -887,7 +887,7 @@ x #+begin_src emacs-lisp baz #+end_src" - (org-babel-expand-noweb-references)))) + (org-babel-expand-noweb-references nil nil :eval)))) ;; During recursive expansion, obey to `:noweb' property. (should (equal "<<bar>>" @@ -905,7 +905,7 @@ x #+begin_src emacs-lisp baz #+end_src" - (org-babel-expand-noweb-references)))) + (org-babel-expand-noweb-references nil nil :eval)))) ;; Respect COMMENT headlines (should (equal "C" @@ -929,7 +929,7 @@ x #+begin_src emacs-lisp :noweb-ref foo C #+end_src" - (org-babel-expand-noweb-references)))) + (org-babel-expand-noweb-references nil nil :eval)))) ;; Preserve case when replacing Noweb reference. (should (equal "(ignore)" @@ -941,7 +941,7 @@ x #+begin_src emacs-lisp :noweb yes<point> <<AA>> #+end_src" - (org-babel-expand-noweb-references)))) + (org-babel-expand-noweb-references nil nil :eval)))) ;; Test :noweb-ref expansion. (should (equal "(message \"!! %s\" \"Running confpkg-test-setup\") @@ -986,7 +986,7 @@ x " (goto-char (point-min)) (search-forward "begin_src") - (org-babel-expand-noweb-references))))) + (org-babel-expand-noweb-references nil nil :eval))))) (ert-deftest test-ob/splitting-variable-lists-in-references () (org-test-with-temp-text "" -- 2.51.0
