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

[Tom Gillespie <tgbugs@gmail.com>]

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