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

[Tom Gillespie <tgbugs@gmail.com>]

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