From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mp2 ([2001:41d0:2:4a6f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by ms11 with LMTPS id iIt0E1n+FV8USgAA0tVLHw (envelope-from ) for ; Mon, 20 Jul 2020 20:28:09 +0000 Received: from aspmx1.migadu.com ([2001:41d0:2:4a6f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by mp2 with LMTPS id mDJOD1n+FV8+GAAAB5/wlQ (envelope-from ) for ; Mon, 20 Jul 2020 20:28:09 +0000 Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by aspmx1.migadu.com (Postfix) with ESMTPS id 34403940A21 for ; Mon, 20 Jul 2020 20:28:08 +0000 (UTC) Received: from localhost ([::1]:43820 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jxcON-00033x-6u for larch@yhetil.org; Mon, 20 Jul 2020 16:28:07 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:32858) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1jxcO2-00033p-HG for emacs-orgmode@gnu.org; Mon, 20 Jul 2020 16:27:46 -0400 Received: from mail-wm1-x343.google.com ([2a00:1450:4864:20::343]:56194) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1jxcO0-0003Eg-Kb for emacs-orgmode@gnu.org; Mon, 20 Jul 2020 16:27:46 -0400 Received: by mail-wm1-x343.google.com with SMTP id g75so711892wme.5 for ; Mon, 20 Jul 2020 13:27:43 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=n1a3NZ2Z33L/4v55mLtoAsN+CI6s7yficLnOEHpqOLk=; b=j/1R+6vYkuU5OVTmaRdomoo0bSfggugTdGEB83mdSzfytAcKB62YgNzmjM4U0dTmC5 zgFqj6TNVPHw/94WICG0j6XWfAMqPxqMdX/EOfkjmGkCZ7KLn1pTW70IxWWcZ1F+eisj Zy8S8S9Pq52xdd2Z1vt1jdiBaEija/DyBjWaG/+6pkwhUIgQDsC2JRed6kAZ/4q6UxOn LPo5GQp0N+2Gsn7unNy5RsB7X8kvx1wsbNOxR4dp0PgjwTJWmNvf4SHBfivQj0vFQUFu +70HzoMMgn9WWkruF8kvCftiThkGEC5yjsCWyfbWJifqqiyXKcCoPfJuVRhODurTWr8I +cYA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=n1a3NZ2Z33L/4v55mLtoAsN+CI6s7yficLnOEHpqOLk=; b=lXB+7RdWRWSZ1iuIY+SLUNGghfJsmXeE4+lQrR7fJIfS72LYnptbknXqNBkTHu40Pn n9Py49k2hC5N4aIbwiecVip2ORVA23I+k8SDyrW7JiP81GSJltBp6USWYUO4UEYfk3dp F7Td3Xsgzu4XCP3xE5bcTPfUxlG5Fwc5YX2BgD3nke4GVrP78NGyiZbtryT/FB1bcevY 7LXrETXpf16YqLdDjObzxwkkN9yREDQtVhDi4Vm15cHmFDr+Ki5/Bqrk1vUyHi4y+bFm eP791VtedWwVKRx2jV1AbBTSfYpJ2UjGaZcPrIjn1qX/t96Z+VMltP0xv6e5F2olqDH1 b8gw== X-Gm-Message-State: AOAM533LzGczgiGh0FGc8Am7HSnpoobfsNsJqKY9f/IBS1+ANAmf/uLN ob46hJNwKGigNftmS/fAgzLXeYpnFzCHCqI/GY3sUk0i X-Google-Smtp-Source: ABdhPJwPkAb6e0RUALfR9KvY3q/aDtIN0mpyKSVoBq4VDMPQeIn3sFjyHoriEf+TFGy65Iylzg1ggkLilEIEjjfpJjY= X-Received: by 2002:a1c:7717:: with SMTP id t23mr875587wmi.75.1595276862584; Mon, 20 Jul 2020 13:27:42 -0700 (PDT) MIME-Version: 1.0 References: <87h7u3dx8h.fsf@kyleam.com> In-Reply-To: <87h7u3dx8h.fsf@kyleam.com> From: Tom Gillespie Date: Mon, 20 Jul 2020 13:27:46 -0700 Message-ID: Subject: Re: [PATCH] lisp/ob-core.el: pass expanded body to org-confirm-babel-evaluate To: Kyle Meyer Content-Type: text/plain; charset="UTF-8" Received-SPF: pass client-ip=2a00:1450:4864:20::343; envelope-from=tgbugs@gmail.com; helo=mail-wm1-x343.google.com X-detected-operating-system: by eggs.gnu.org: No matching host in p0f cache. That's all we know. X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, FREEMAIL_FROM=0.001, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, URIBL_BLOCKED=0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: emacs-orgmode@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "General discussions about Org-mode." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: emacs-orgmode Errors-To: emacs-orgmode-bounces+larch=yhetil.org@gnu.org Sender: "Emacs-orgmode" X-Scanner: scn0 Authentication-Results: aspmx1.migadu.com; dkim=pass header.d=gmail.com header.s=20161025 header.b=j/1R+6vY; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (aspmx1.migadu.com: domain of emacs-orgmode-bounces@gnu.org designates 209.51.188.17 as permitted sender) smtp.mailfrom=emacs-orgmode-bounces@gnu.org X-Spam-Score: -1.71 X-TUID: Ce/f1rzcGteP 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 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 <> #+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