Re: [PATCH] ob-tangle.el: restore :tangle closure nil behavior

[Tom Gillespie <tgbugs@gmail.com>]

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

Here is a corrected patch that fixes the fact that closures passed to the
:tangle header were not being evaluated. Details in the commit message.
Best,
Tom

On Tue, Aug 15, 2023 at 6:41 PM Tom Gillespie <tgbugs@gmail.com> wrote:
>
> After a bit more investigation don't apply this patch because the change
> is insufficient to correct another issue.
>
> Specifically org-babel-tangle-collect-blocks must check for and resolve
> any closures that are passed to :tangle _before_ testing (string=
> src-tfile "no").
> As it stands blocks that are marked :tangle (and "no") with a closure
> incorrectly make it to a call to (org-babel-get-src-block-info) which causes
> a call to org-babel-process-params when :tangle would be "no".
>
> Working on a proper fix now.

[-- Attachment #2: 0001-ob-tangle.el-restore-tangle-closure-evaluation-befor.patch --]
[-- Type: text/x-patch, Size: 6587 bytes --]

From ce56fe5c1a24ba37c5c7c2f541c48684b0cb1e0b Mon Sep 17 00:00:00 2001
From: Tom Gillespie <tgbugs@gmail.com>
Date: Tue, 15 Aug 2023 13:46:08 -0700
Subject: [PATCH] ob-tangle.el: restore :tangle closure evaluation before eval
 info

* lisp/ob-tangle.el (org-babel-tangle): check that :tangle is not no
before attempting to tangle a single block
(org-babel-tangle--unbracketed-link): new optional argument
info-was-evaled to control whether (cdr (assq :tangle params)) is
expanded via `org-babel-read'
(org-babel-tangle-collect-blocks): expand closures passed to :tangle
with `org-babel-read' so that blocks with :tangle closure that eval to
"no" will not incorrectly eval other parameters
(org-babel-tangle-single-block): src-tfile handle cases where output
of :tangle closure could be nil and conver to "no"

This patch fixes a bug where header arguments like :tangle (or "no")
were treated as if they were tangling to a file named "(or \"no\")".
As a result, org-bable would call org-babel-get-src-block-info with
'no-eval set to nil, causing parameters to be evaluated despite the
fact that when :tangle no or equivalent is set, the other parameters
should never be evaluated.

This patch also restores the original behavior of the :tangle header
argument when a closure returned nil, e.g. #+header: :tangle (or), by
using (or (cdr (assq :tangle params)) "no"). Without this the call to
`file-name-directory' in `org-babel-tangle--unbracketed-link' can fail
with a wrong-type-argument stringp nil error.
---
 lisp/ob-tangle.el | 32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/lisp/ob-tangle.el b/lisp/ob-tangle.el
index 4566e03ad..a9fb0e286 100644
--- a/lisp/ob-tangle.el
+++ b/lisp/ob-tangle.el
@@ -316,9 +316,13 @@ matching a regular expression."
 		     (write-region nil nil file-name)
 		     (mapc (lambda (mode) (set-file-modes file-name mode)) modes))
                    (push file-name path-collector))))))
-	 (if (equal arg '(4))
-	     (org-babel-tangle-single-block 1 t)
-	   (org-babel-tangle-collect-blocks lang-re tangle-file)))
+         (if (equal arg '(4))
+             (let* ((info (org-babel-get-src-block-info 'no-eval))
+                    (params (nth 2 info))
+                    (src-tfile (or (org-babel-read (cdr (assq :tangle params))) "no")))
+               (unless (string= src-tfile "no")
+                 (org-babel-tangle-single-block 1 t)))
+           (org-babel-tangle-collect-blocks lang-re tangle-file)))
 	(message "Tangled %d code block%s from %s" block-counter
 		 (if (= block-counter 1) "" "s")
 		 (file-name-nondirectory
@@ -473,14 +477,14 @@ code blocks by target file."
 		  (org-in-archived-heading-p))
 	(let* ((info (org-babel-get-src-block-info 'no-eval))
 	       (src-lang (nth 0 info))
-	       (src-tfile (cdr (assq :tangle (nth 2 info)))))
+	       (src-tfile (or (org-babel-read (cdr (assq :tangle (nth 2 info)))) "no")))
 	  (unless (or (string= src-tfile "no")
 		      (and tangle-file (not (equal tangle-file src-tfile)))
 		      (and lang-re (not (string-match-p lang-re src-lang))))
 	    ;; Add the spec for this block to blocks under its tangled
 	    ;; file name.
 	    (let* ((block (org-babel-tangle-single-block counter))
-                   (src-tfile (cdr (assq :tangle (nth 4 block))))
+                   (src-tfile (or (cdr (assq :tangle (nth 4 block))) "no"))
 		   (file-name (org-babel-effective-tangled-filename
                                buffer-fn src-lang src-tfile))
 		   (by-fn (assoc file-name blocks)))
@@ -490,7 +494,7 @@ code blocks by target file."
     (mapcar (lambda (b) (cons (car b) (nreverse (cdr b))))
 	    (nreverse blocks))))
 
-(defun org-babel-tangle--unbracketed-link (params)
+(defun org-babel-tangle--unbracketed-link (params &optional info-was-evaled)
   "Get a raw link to the src block at point, without brackets.
 
 The PARAMS are the 3rd element of the info for the same src block."
@@ -510,7 +514,10 @@ The PARAMS are the 3rd element of the info for the same src block."
               (concat "file:"
                       (file-relative-name (substring bare (match-end 0))
                                           (file-name-directory
-                                           (cdr (assq :tangle params)))))
+                                           (or (if info-was-evaled
+                                                   (cdr (assq :tangle params))
+                                                 (org-babel-read (cdr (assq :tangle params))))
+                                               "no"))))
             bare))))))
 
 (defvar org-outline-regexp) ; defined in lisp/org.el
@@ -520,6 +527,8 @@ Return the list of block attributes needed by
 `org-babel-tangle-collect-blocks'.  When ONLY-THIS-BLOCK is
 non-nil, return the full association list to be used by
 `org-babel-tangle' directly."
+  ;; this should only ever be called when :tangle has already been determined
+  ;; to be non-nil and not string= "no"
   (let* ((info (org-babel-get-src-block-info))
 	 (start-line
 	  (save-restriction (widen)
@@ -530,7 +539,7 @@ non-nil, return the full association list to be used by
 	 (extra (nth 3 info))
          (coderef (nth 6 info))
 	 (cref-regexp (org-src-coderef-regexp coderef))
-	 (link (org-babel-tangle--unbracketed-link params))
+	 (link (org-babel-tangle--unbracketed-link params 'info-was-evaled))
 	 (source-name
 	  (or (nth 4 info)
 	      (format "%s:%d"
@@ -580,7 +589,7 @@ non-nil, return the full association list to be used by
 			 (match-end 0)
 		       (point-min))))
 	      (point)))))
-         (src-tfile (cdr (assq :tangle params)))
+         (src-tfile (or (cdr (assq :tangle params)) "no"))
 	 (result
 	  (list start-line
 		(if org-babel-tangle-use-relative-file-links
@@ -602,6 +611,11 @@ non-nil, return the full association list to be used by
   "Return a list of begin and end link comments for the code block at point.
 INFO, when non nil, is the source block information, as returned
 by `org-babel-get-src-block-info'."
+  ;; currently all call sites for this function pass info that came from a
+  ;; 'no-eval call to `org-babel-get-src-block-info', specifically they come
+  ;; info populated into `org-babel-expand-noweb-references--cache', as such
+  ;; we DO NOT pass the indicator that 'info-was-evaled to
+  ;; `org-babel-tangle--unbracketed-link' below
   (let ((link-data (pcase (or info (org-babel-get-src-block-info 'no-eval))
 		     (`(,_ ,_ ,params ,_ ,name ,start ,_)
 		      `(("start-line" . ,(org-with-point-at start
-- 
2.41.0