[PATCH] lisp/ob-core.el: pass expanded body to org-confirm-babel-evaluate

[PATCH] lisp/ob-core.el: pass expanded body to org-confirm-babel-evaluate

[Tom Gillespie]

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

Hi all,
   This is a patch to improve the behavior of
org-babel-check-confirm-evaluate and the usefulness of
org-confirm-babel-evaluate when a function is provided. It is
currently made against master, but it might also be possible to make
it against maint. The rest is in the commit message. Best!
Tom

[-- Attachment #2: 0001-lisp-ob-core.el-pass-expanded-body-to-org-confirm-ba.patch --]
[-- Type: text/x-patch, Size: 3983 bytes --]

From 6d069f9532f44ee9fbc1a0ebdaadcc2eb807f8ec Mon Sep 17 00:00:00 2001
From: Tom Gillespie <tgbugs@gmail.com>
Date: Mon, 13 Jul 2020 12:04:18 -0700
Subject: [PATCH] lisp/ob-core.el: pass expanded body to
 org-confirm-babel-evaluate

* lisp/ob-core.el (org-babel-get-body-to-eval): New function extracted
from org-babel-execute-src-block so that it can be reused.
(org-babel-execute-src-block): Use org-babel-get-body-to-eval
(org-babel-check-confirm-evaluate): Use org-babel-get-body-to-eval to
pass the expanded body of the code that will actually be evaluated to
org-confirm-babel-evaluate if it is a function.

This commit changes the behavior of org-babel-check-confirm-evaluate
so that org-confirm-babel-evaluate receives the fully expanded code to
be evaluated. This is important because there is no easy way to expand
noweb references inside org-confirm-babel-evaluate without calling
org-babel-get-src-block-info a second time. It is also important
because the current behavior makes it possible for code lurking behind
noweb references to change without the user being able to detect those
changes if they trust org-confirm-babel-evaluate is receiving the
actual body of the code that will be evalulated. I was bitten by this
when trying to hash the body passed to org-confirm-babel-evaluate and
could not figure out why the hash was not changing when a nowebbed
block changed.

These changes were made in such a way as to minimize changes to the
existing functions, however they come at the cost of making two calls
to org-babel-get-body-to-eval since passing the expanded body through
to org-confirm-babel-evaluate would either require appending it to the
info list or changing the function signatures of
org-babel-confirm-evaluate and org-babel-check-confirm-evaluate which
is undesireable. Furthermore, to compute the expanded body only once
would require switching from using cond in org-babel-execute-src-block
to using a series of nested ifs. This change can be made, but starting
from the current implementation will provide a working reference for
comparison rather than trying to make all the changes at the same time.
---
 lisp/ob-core.el | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/lisp/ob-core.el b/lisp/ob-core.el
index e798595bd..4f2413579 100644
--- a/lisp/ob-core.el
+++ b/lisp/ob-core.el
@@ -238,7 +238,7 @@ should be asked whether to allow evaluation."
 		    (if (functionp org-confirm-babel-evaluate)
 			(funcall org-confirm-babel-evaluate
 				 ;; Language, code block body.
-				 (nth 0 info) (nth 1 info))
+				 (nth 0 info) (org-babel-get-body-to-eval info))
 		      org-confirm-babel-evaluate))))
     (cond
      (noeval nil)
@@ -632,6 +632,17 @@ a list with the following pattern:
 	(setf (nth 2 info) (org-babel-generate-file-param name (nth 2 info)))
 	info))))
 
+(defun org-babel-get-body-to-eval (info)
+  "Expand noweb references in BODY and remove any coderef."
+  (let ((coderef (nth 6 info))
+	(expand
+	 (if (org-babel-noweb-p (nth 2 info) :eval)
+	     (org-babel-expand-noweb-references info)
+	   (nth 1 info))))
+    (if (not coderef) expand
+      (replace-regexp-in-string
+       (org-src-coderef-regexp coderef) "" expand nil nil 1))))
+
 ;;;###autoload
 (defun org-babel-execute-src-block (&optional arg info params)
   "Execute the current source code block.
@@ -679,15 +690,7 @@ block."
 		 (result-params (cdr (assq :result-params params)))
 		 ;; Expand noweb references in BODY and remove any
 		 ;; coderef.
-		 (body
-		  (let ((coderef (nth 6 info))
-			(expand
-			 (if (org-babel-noweb-p params :eval)
-			     (org-babel-expand-noweb-references info)
-			   (nth 1 info))))
-		    (if (not coderef) expand
-		      (replace-regexp-in-string
-		       (org-src-coderef-regexp coderef) "" expand nil nil 1))))
+		 (body (org-babel-get-body-to-eval info))
 		 (dir (cdr (assq :dir params)))
 		 (mkdirp (cdr (assq :mkdirp params)))
 		 (default-directory
-- 
2.26.2

Re: [PATCH] lisp/ob-core.el: pass expanded body to org-confirm-babel-evaluate

[Kyle Meyer]

Tom Gillespie writes:

> This is a patch to improve the behavior of
> org-babel-check-confirm-evaluate and the usefulness of
> org-confirm-babel-evaluate when a function is provided.

Thank you.

> This commit changes the behavior of org-babel-check-confirm-evaluate
> so that org-confirm-babel-evaluate receives the fully expanded code to
> be evaluated. This is important because there is no easy way to expand
> noweb references inside org-confirm-babel-evaluate without calling
> org-babel-get-src-block-info a second time. It is also important
> because the current behavior makes it possible for code lurking behind
> noweb references to change without the user being able to detect those
> changes if they trust org-confirm-babel-evaluate is receiving the
> actual body of the code that will be evalulated.

I find that convincing.  I'd guess that a org-confirm-babel-evaluate
function would always want to see expanded noweb references.

This doesn't mention coderefs, though, and the case there seems less
clear.  If there's not a strong reason to strip coderefs, then the new
function wouldn't be necessary, and -check-confirm-evaluate could
instead go with the pattern used elsewhere:

    (if (org-babel-noweb-p (nth 2 info) :eval)
        (org-babel-expand-noweb-references info)
      (nth 1 info))

(Perhaps it'd be worth adding a -maybe-expand variant of
-expand-noweb-references since there are a good number of spots that do
the same params check before calling -expand-noweb-references.)

> These changes were made in such a way as to minimize changes to the
> existing functions, however they come at the cost of making two calls
> to org-babel-get-body-to-eval [...]

Or potentially three calls to -get-body-to-eval: one call with the
-check-evaluate call in -execute-src-block, one with the
-confirm-evaluate in -execute-src-block, and then the direct call to
-get-body-to-eval in -execute-src-block.

> [...] since passing the expanded body through to
> org-confirm-babel-evaluate would either require appending it to the
> info list or changing the function signatures of
> org-babel-confirm-evaluate and org-babel-check-confirm-evaluate which
> is undesireable.  Furthermore, to compute the expanded body only once
> would require switching from using cond in org-babel-execute-src-block
> to using a series of nested ifs. This change can be made, but starting
> from the current implementation will provide a working reference for
> comparison rather than trying to make all the changes at the same
> time.

An option not mentioned above is to replace (nth 1 info) with the
expanded body upstream of (when (org-babel-check-evaluate info) ...).
Modifying the body in INFO is admittedly not pretty, but it's in line
with what is done elsewhere (e.g., -expand-src-block,
-exp-process-buffer, -load-in-session), as well as with how other INFO
elements in -execute-src-block are handled.

In fact, -execute-src-block did modify (nth 1 info) before 3b3fc520a
(Fix coderef handling in source blocks, 2016-08-28), though too late to
affect calls to a org-confirm-babel-evaluate function.  Quickly checking
the tests added in that commit as well as the example in the message [0]
that prompted that commit, modifying (nth 1 info) didn't seem to break
the fix there.

[0] https://orgmode.org/list/87mvjyoja2.fsf@dell-desktop.WORKGROUP

Re: [PATCH] lisp/ob-core.el: pass expanded body to org-confirm-babel-evaluate

[Tom Gillespie]

Hi Kyle,
   Thank you for the feedback. In short if modifying (nth 1 info) in place won't
cause a problem then I think it is the way to go. Details below. Best,
Tom

On Sun, Jul 19, 2020 at 2:13 PM Kyle Meyer <kyle@kyleam.com> wrote:
>
> Tom Gillespie writes:
>
> > This is a patch to improve the behavior of
> > org-babel-check-confirm-evaluate and the usefulness of
> > org-confirm-babel-evaluate when a function is provided.
>
> Thank you.
>
> > This commit changes the behavior of org-babel-check-confirm-evaluate
> > so that org-confirm-babel-evaluate receives the fully expanded code to
> > be evaluated. This is important because there is no easy way to expand
> > noweb references inside org-confirm-babel-evaluate without calling
> > org-babel-get-src-block-info a second time. It is also important
> > because the current behavior makes it possible for code lurking behind
> > noweb references to change without the user being able to detect those
> > changes if they trust org-confirm-babel-evaluate is receiving the
> > actual body of the code that will be evalulated.
>
> I find that convincing.  I'd guess that a org-confirm-babel-evaluate
> function would always want to see expanded noweb references.
>
> This doesn't mention coderefs, though, and the case there seems less
> clear.  If there's not a strong reason to strip coderefs, then the new
> function wouldn't be necessary, and -check-confirm-evaluate could
> instead go with the pattern used elsewhere:
>
>     (if (org-babel-noweb-p (nth 2 info) :eval)
>         (org-babel-expand-noweb-references info)
>       (nth 1 info))

Yes, this is in line with my thinking below about what
-get-src-block-info should
be returning by default, specifically that (nth 1 info) should
probably always be
expanded unless the caller explicitly asks for it not to be.

> (Perhaps it'd be worth adding a -maybe-expand variant of
> -expand-noweb-references since there are a good number of spots that do
> the same params check before calling -expand-noweb-references.)

Does the suggestion below to do this inside of -get-src-block-info cover those
other cases?

> > These changes were made in such a way as to minimize changes to the
> > existing functions, however they come at the cost of making two calls
> > to org-babel-get-body-to-eval [...]
>
> Or potentially three calls to -get-body-to-eval: one call with the
> -check-evaluate call in -execute-src-block, one with the
> -confirm-evaluate in -execute-src-block, and then the direct call to
> -get-body-to-eval in -execute-src-block.

Ah yep, I missed that -check-evaluate would also hit that path, but it is
safer that way.

> > [...] since passing the expanded body through to
> > org-confirm-babel-evaluate would either require appending it to the
> > info list or changing the function signatures of
> > org-babel-confirm-evaluate and org-babel-check-confirm-evaluate which
> > is undesireable.  Furthermore, to compute the expanded body only once
> > would require switching from using cond in org-babel-execute-src-block
> > to using a series of nested ifs. This change can be made, but starting
> > from the current implementation will provide a working reference for
> > comparison rather than trying to make all the changes at the same
> > time.
>
> An option not mentioned above is to replace (nth 1 info) with the
> expanded body upstream of (when (org-babel-check-evaluate info) ...).
> Modifying the body in INFO is admittedly not pretty, but it's in line
> with what is done elsewhere (e.g., -expand-src-block,
> -exp-process-buffer, -load-in-session), as well as with how other INFO
> elements in -execute-src-block are handled.

I considered this as well, and in fact I assumed that this is how it worked
before I looked to see the code was actually doing. I didn't know what the
consequences of making it work that way would be and tend to err on the
side of not kobbering data that some other function might expect to be in
an unmodified state. This would certainly be the most expedient solution.

There would need to be a way to indicate that info should not expand noweb
references so that :noweb no-export can get the unexpanded form. Maybe
and optional argument =expand= that defaults to t?
(defun org-babel-get-src-block-info (&optional light datum expand) ... )
It might not even be needed if all the required information can be derived from
info itself using just
(when (org-babel-noweb-p params :eval) (org-babel-expand-noweb-references info))
at the end of -get-src-block-info with the accompanying expansion of params.

In a sense I think it is also advantageous to do this because it will make
the noweb expansion orthogonal to execution. In principle this means that
-get-src-block-info should return in a state such that (nth 1 info) is always
already expanded if :noweb is set to anything but no. The no-export case
is orthogonal here as well because if a block needs to be executed during
export, it still must be expanded independent of how the block itself will
be rendered.

e.g.
#+name: other-block
#+begin_src elisp
(+ 1 2)
#+end_src

#+name: block
#+begin_src elisp :noweb no-export
<<other-block>>
#+end_src

#+name: print-3
#+begin_src elisp :var three=block() :exports both
(print three)
#+end_src

> In fact, -execute-src-block did modify (nth 1 info) before 3b3fc520a
> (Fix coderef handling in source blocks, 2016-08-28), though too late to
> affect calls to a org-confirm-babel-evaluate function.  Quickly checking
> the tests added in that commit as well as the example in the message [0]
> that prompted that commit, modifying (nth 1 info) didn't seem to break
> the fix there.
>
> [0] https://orgmode.org/list/87mvjyoja2.fsf@dell-desktop.WORKGROUP

Re: [PATCH] lisp/ob-core.el: pass expanded body to org-confirm-babel-evaluate

[Kyle Meyer]

Tom Gillespie writes:

> On Sun, Jul 19, 2020 at 2:13 PM Kyle Meyer <kyle@kyleam.com> wrote:

>> An option not mentioned above is to replace (nth 1 info) with the
>> expanded body upstream of (when (org-babel-check-evaluate info) ...).
>> Modifying the body in INFO is admittedly not pretty, but it's in line
>> with what is done elsewhere (e.g., -expand-src-block,
>> -exp-process-buffer, -load-in-session), as well as with how other INFO
>> elements in -execute-src-block are handled.
>
> I considered this as well, and in fact I assumed that this is how it worked
> before I looked to see the code was actually doing. I didn't know what the
> consequences of making it work that way would be and tend to err on the
> side of not kobbering data that some other function might expect to be in
> an unmodified state. This would certainly be the most expedient solution.
>
> There would need to be a way to indicate that info should not expand noweb
> references so that :noweb no-export can get the unexpanded form. Maybe
> and optional argument =expand= that defaults to t?
> (defun org-babel-get-src-block-info (&optional light datum expand)
> ... )

Hmm, it looks like you're thinking of a different spot than I was
referring to.  I was talking about modifying (nth 1 info) in
-execute-src-block, before the (when (org-babel-check-evaluate ...).

Prior to commit 3b3fc520a, (nth 1 info) was modified in
-execute-src-block but after org-babel-check-evaluate and
org-babel-confirm-evaluate.  That makes me think it'd probably be safe
to do again, just a bit upstream in the same function.  And I don't see
a spot where modifying that element would be problematic.  On the
incoming end, INFO is passed to copy-tree if it is provided as an
argument, and I don't think any of the functions that -execute-src-block
feeds INFO to would be affected.  But I of course could be overlooking
something.

On the other hand, if org-babel-get-src-block-info did the expansion,
I'd be a bit more worried about downstream effects (though I haven't
tried to trace things too closely).  Also, -execute-src-block takes a
PARAMS argument, and I think that needs to be merged with (nth 2 info)
before considering whether to expand, so it seems like expanding in
-get-src-block-info would be too soon.

Regardless of whether modifying (nth 1 info) would work, I also think
it'd be fine to instead go ahead with something along the lines of your
initial patch.  In that case, the main question is whether coderefs
should be removed (as they currently are in your patch).  If not,
perhaps something like below (untested) would suffice.

diff --git a/lisp/ob-core.el b/lisp/ob-core.el
index e798595bd..230706b7f 100644
--- a/lisp/ob-core.el
+++ b/lisp/ob-core.el
@@ -238,7 +238,10 @@ (defun org-babel-check-confirm-evaluate (info)
 		    (if (functionp org-confirm-babel-evaluate)
 			(funcall org-confirm-babel-evaluate
 				 ;; Language, code block body.
-				 (nth 0 info) (nth 1 info))
+				 (nth 0 info)
+				 (if (org-babel-noweb-p headers :eval)
+				     (org-babel-expand-noweb-references info)
+				   (nth 1 info)))
 		      org-confirm-babel-evaluate))))
     (cond
      (noeval nil)

Re: [PATCH] lisp/ob-core.el: pass expanded body to org-confirm-babel-evaluate

[Tom Gillespie]

Hi Kyle,
   Sorry for the slow turnaround time on this one. Having now tested
it, I think that your solution is a much better one for the time
being, so please go ahead and apply it. From this discussion there are
a number of good options for improvements in the future, but my
priority would be to get the expanded version of the body passed to
org-confirm-babel-evaluate asap with as few disturbances to the rest
of the code base as possible. Best!
Tom

On Tue, Jul 21, 2020 at 9:20 PM Kyle Meyer <kyle@kyleam.com> wrote:
>
> Tom Gillespie writes:
>
> > On Sun, Jul 19, 2020 at 2:13 PM Kyle Meyer <kyle@kyleam.com> wrote:
>
> >> An option not mentioned above is to replace (nth 1 info) with the
> >> expanded body upstream of (when (org-babel-check-evaluate info) ...).
> >> Modifying the body in INFO is admittedly not pretty, but it's in line
> >> with what is done elsewhere (e.g., -expand-src-block,
> >> -exp-process-buffer, -load-in-session), as well as with how other INFO
> >> elements in -execute-src-block are handled.
> >
> > I considered this as well, and in fact I assumed that this is how it worked
> > before I looked to see the code was actually doing. I didn't know what the
> > consequences of making it work that way would be and tend to err on the
> > side of not kobbering data that some other function might expect to be in
> > an unmodified state. This would certainly be the most expedient solution.
> >
> > There would need to be a way to indicate that info should not expand noweb
> > references so that :noweb no-export can get the unexpanded form. Maybe
> > and optional argument =expand= that defaults to t?
> > (defun org-babel-get-src-block-info (&optional light datum expand)
> > ... )
>
> Hmm, it looks like you're thinking of a different spot than I was
> referring to.  I was talking about modifying (nth 1 info) in
> -execute-src-block, before the (when (org-babel-check-evaluate ...).
>
> Prior to commit 3b3fc520a, (nth 1 info) was modified in
> -execute-src-block but after org-babel-check-evaluate and
> org-babel-confirm-evaluate.  That makes me think it'd probably be safe
> to do again, just a bit upstream in the same function.  And I don't see
> a spot where modifying that element would be problematic.  On the
> incoming end, INFO is passed to copy-tree if it is provided as an
> argument, and I don't think any of the functions that -execute-src-block
> feeds INFO to would be affected.  But I of course could be overlooking
> something.
>
> On the other hand, if org-babel-get-src-block-info did the expansion,
> I'd be a bit more worried about downstream effects (though I haven't
> tried to trace things too closely).  Also, -execute-src-block takes a
> PARAMS argument, and I think that needs to be merged with (nth 2 info)
> before considering whether to expand, so it seems like expanding in
> -get-src-block-info would be too soon.
>
> Regardless of whether modifying (nth 1 info) would work, I also think
> it'd be fine to instead go ahead with something along the lines of your
> initial patch.  In that case, the main question is whether coderefs
> should be removed (as they currently are in your patch).  If not,
> perhaps something like below (untested) would suffice.
>
> diff --git a/lisp/ob-core.el b/lisp/ob-core.el
> index e798595bd..230706b7f 100644
> --- a/lisp/ob-core.el
> +++ b/lisp/ob-core.el
> @@ -238,7 +238,10 @@ (defun org-babel-check-confirm-evaluate (info)
>                     (if (functionp org-confirm-babel-evaluate)
>                         (funcall org-confirm-babel-evaluate
>                                  ;; Language, code block body.
> -                                (nth 0 info) (nth 1 info))
> +                                (nth 0 info)
> +                                (if (org-babel-noweb-p headers :eval)
> +                                    (org-babel-expand-noweb-references info)
> +                                  (nth 1 info)))
>                       org-confirm-babel-evaluate))))
>      (cond
>       (noeval nil)

Re: [PATCH] lisp/ob-core.el: pass expanded body to org-confirm-babel-evaluate

[Kyle Meyer]

Tom Gillespie writes:

>    Sorry for the slow turnaround time on this one. Having now tested
> it, I think that your solution is a much better one for the time
> being, so please go ahead and apply it. From this discussion there are
> a number of good options for improvements in the future, but my
> priority would be to get the expanded version of the body passed to
> org-confirm-babel-evaluate asap with as few disturbances to the rest
> of the code base as possible. Best!

All right, sounds good.  Applied in df5a83637.

Thanks.

Re: [PATCH] lisp/ob-core.el: pass expanded body to org-confirm-babel-evaluate

[Tom Gillespie]

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

Hi Kyle,
    Following up in this thread having investigated the impact of coderefs.
My conclusion is that coderefs need to be stripped out before they are
passed to org-confirm-babel-evaluate. They are not present in the
executed code and removing them is not something that a definition
of org-confirm-babel-evaluate should have to know anything about.
Right now I work around them by suggesting that users comment
out their coderefs. This works because my use case is restricted to
elisp code and I strip the comments using read, but other languages
would not have such an easy solution.

I have included a patch against maint that reuses the let block
from org-babel-execute-src-block to accomplish this.

Best!
Tom

[-- Attachment #2: 0001-lisp-ob-core.el-org-babel-check-confirm-evaluate-str.patch --]
[-- Type: application/x-patch, Size: 1320 bytes --]

Re: [PATCH] lisp/ob-core.el: pass expanded body to org-confirm-babel-evaluate

[Kyle Meyer]

Tom Gillespie writes:

> Hi Kyle,
>     Following up in this thread having investigated the impact of coderefs.
> My conclusion is that coderefs need to be stripped out before they are
> passed to org-confirm-babel-evaluate. They are not present in the
> executed code and removing them is not something that a definition
> of org-confirm-babel-evaluate should have to know anything about.
> Right now I work around them by suggesting that users comment
> out their coderefs. This works because my use case is restricted to
> elisp code and I strip the comments using read, but other languages
> would not have such an easy solution.

Thanks for revisiting this.  This change (df5a83637) hasn't made it into
a release yet, so it'd be good to make this move now.

> I have included a patch against maint that reuses the let block
> from org-babel-execute-src-block to accomplish this.

> diff --git a/lisp/ob-core.el b/lisp/ob-core.el
> index cd876da0f..44b02feb9 100644
> --- a/lisp/ob-core.el
> +++ b/lisp/ob-core.el
> @@ -240,9 +240,14 @@ should be asked whether to allow evaluation."
>  			(funcall org-confirm-babel-evaluate
>  				 ;; Language, code block body.
>  				 (nth 0 info)
> -				 (if (org-babel-noweb-p headers :eval)
> -				     (org-babel-expand-noweb-references info)
> -				   (nth 1 info)))
> +				 (let ((coderef (nth 6 info))
> +				       (expand
> +					(if (org-babel-noweb-p params :eval)

params is undefined here.  I've s/params/headers/ when applying.

> +					    (org-babel-expand-noweb-references info)
> +					  (nth 1 info))))
> +				   (if (not coderef) expand
> +				     (replace-regexp-in-string
> +				      (org-src-coderef-regexp coderef) "" expand nil nil 1))))
>  		      org-confirm-babel-evaluate))))
>      (cond
>       (noeval nil)

Okay, so this is equivalent to your original patch, though your initial
approach avoided duplicating the logic, which I think is worth doing.
I'd like to make sure this gets in before a release, so I've applied
this message's patch (3e1c0b0f4) and then followed it up with a patch
that adds a test, and another that extracts the duplicated logic out to
a helper (as in your original patch).

Re: [PATCH] lisp/ob-core.el: pass expanded body to org-confirm-babel-evaluate

[Tom Gillespie]

Hi Kyle,
    Great. That sounds like the best solution given that correctness
is more important than performance at the moment. We have a good idea
of how to improve the performance going forward so providing the
correct behavior asap seems like the right thing to do. Thank you very
much for getting this in! Best,
Tom

On Sat, Sep 5, 2020 at 8:45 PM Kyle Meyer <kyle@kyleam.com> wrote:
>
> Tom Gillespie writes:
>
> > Hi Kyle,
> >     Following up in this thread having investigated the impact of coderefs.
> > My conclusion is that coderefs need to be stripped out before they are
> > passed to org-confirm-babel-evaluate. They are not present in the
> > executed code and removing them is not something that a definition
> > of org-confirm-babel-evaluate should have to know anything about.
> > Right now I work around them by suggesting that users comment
> > out their coderefs. This works because my use case is restricted to
> > elisp code and I strip the comments using read, but other languages
> > would not have such an easy solution.
>
> Thanks for revisiting this.  This change (df5a83637) hasn't made it into
> a release yet, so it'd be good to make this move now.
>
> > I have included a patch against maint that reuses the let block
> > from org-babel-execute-src-block to accomplish this.
>
> > diff --git a/lisp/ob-core.el b/lisp/ob-core.el
> > index cd876da0f..44b02feb9 100644
> > --- a/lisp/ob-core.el
> > +++ b/lisp/ob-core.el
> > @@ -240,9 +240,14 @@ should be asked whether to allow evaluation."
> >                       (funcall org-confirm-babel-evaluate
> >                                ;; Language, code block body.
> >                                (nth 0 info)
> > -                              (if (org-babel-noweb-p headers :eval)
> > -                                  (org-babel-expand-noweb-references info)
> > -                                (nth 1 info)))
> > +                              (let ((coderef (nth 6 info))
> > +                                    (expand
> > +                                     (if (org-babel-noweb-p params :eval)
>
> params is undefined here.  I've s/params/headers/ when applying.
>
> > +                                         (org-babel-expand-noweb-references info)
> > +                                       (nth 1 info))))
> > +                                (if (not coderef) expand
> > +                                  (replace-regexp-in-string
> > +                                   (org-src-coderef-regexp coderef) "" expand nil nil 1))))
> >                     org-confirm-babel-evaluate))))
> >      (cond
> >       (noeval nil)
>
> Okay, so this is equivalent to your original patch, though your initial
> approach avoided duplicating the logic, which I think is worth doing.
> I'd like to make sure this gets in before a release, so I've applied
> this message's patch (3e1c0b0f4) and then followed it up with a patch
> that adds a test, and another that extracts the duplicated logic out to
> a helper (as in your original patch).