Here are several patches to fix things in and around org-babel. They're each independent of the others (and hopefully all apply cleanly, without depending on other members of the series). Here's a little summary of each: Aaron Ecay (10): Fix org-babel-R-initiate-session -> An obvious bugfix Clean up org-babel-expand-body: functions for awk and picolisp -> Makes these functions consistent with other babel languages, though I don't use these languages so can't test Clean up various org-babel-*-maybe commands -> Code simplification and avoids an expensive operation under some circumstances Add 'light argument to some uses of org-babel-get-src-block-info -> Avoids an expensive operation Remove info arg from several org-babel functions -> Code cleanup. Could break third-party code. Use prefix arg in org-edit-special -> Makes the function consistent with its docstring, although the new behavior is somewhat odd (C-u C-c ' becomes basically the same as C-c C-v C-z, AFAICT) Simplify org-babel-execute-src-block -> Makes data flow cleaner through this function Fix testing/lisp/test-ob-emacs-lisp.el -> Obvious bugfix Remove org-babel-check-confirm-evaluate macro -> Refactoring. Of all the patches, I am least sure of this one. It is a complicated operation however you slice it, but I find the approach where the complexity is local easier to understand. Deserves careful review, since it touches code which decides whether to evaluate source blocks. Read: has security implications. Document how :var introduces code block dependencies. -> Documentation. doc/org.texi | 9 +++ lisp/ob-R.el | 20 +++-- lisp/ob-awk.el | 2 +- lisp/ob-core.el | 156 +++++++++++++++++-------------------- lisp/ob-picolisp.el | 2 +- lisp/ob-tangle.el | 2 +- lisp/org.el | 7 +- testing/lisp/test-ob-emacs-lisp.el | 19 +++-- 8 files changed, 112 insertions(+), 105 deletions(-) -- 1.8.2
* lisp/ob-R.el (org-babel-R-initiate-session): handle case where the session buffer exists, but does not have a live process If the session buffer exists, but the user has exited the R process manually, then the (R) command will create a new buffer, then try to rename it over the old buffer, causing an error. The right thing to do is to start R within the existing buffer. --- lisp/ob-R.el | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/lisp/ob-R.el b/lisp/ob-R.el index 9875f81..de9ec5b 100644 --- a/lisp/ob-R.el +++ b/lisp/ob-R.el @@ -209,14 +209,18 @@ This function is called by `org-babel-execute-src-block'." (if (org-babel-comint-buffer-livep session) session (save-window-excursion - (require 'ess) (R) - (rename-buffer - (if (bufferp session) - (buffer-name session) - (if (stringp session) - session - (buffer-name)))) - (current-buffer)))))) + (save-excursion + (when (get-buffer session) + ;; Session buffer exists, but with dead process + (set-buffer session)) + (require 'ess) (R) + (rename-buffer + (if (bufferp session) + (buffer-name session) + (if (stringp session) + session + (buffer-name)))) + (current-buffer))))))) (defun org-babel-R-associate-session (session) "Associate R code buffer with an R session. -- 1.8.2
* lisp/ob-awk.el (org-babel-expand-body:awk), lisp/ob-picolisp.el (org-babel-expand-body:picolisp): remove optional arg from these functions The optional argument is apparently never passed by org-babel code. Maybe this is a relic of an earlier calling convention? --- lisp/ob-awk.el | 2 +- lisp/ob-picolisp.el | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lisp/ob-awk.el b/lisp/ob-awk.el index f717fec..373d5fd 100644 --- a/lisp/ob-awk.el +++ b/lisp/ob-awk.el @@ -44,7 +44,7 @@ (defvar org-babel-awk-command "awk" "Name of the awk executable command.") -(defun org-babel-expand-body:awk (body params &optional processed-params) +(defun org-babel-expand-body:awk (body params) "Expand BODY according to PARAMS, return the expanded body." (dolist (pair (mapcar #'cdr (org-babel-get-header params :var))) (setf body (replace-regexp-in-string diff --git a/lisp/ob-picolisp.el b/lisp/ob-picolisp.el index e785366..1d17919 100644 --- a/lisp/ob-picolisp.el +++ b/lisp/ob-picolisp.el @@ -78,7 +78,7 @@ :version "24.1" :type 'string) -(defun org-babel-expand-body:picolisp (body params &optional processed-params) +(defun org-babel-expand-body:picolisp (body params) "Expand BODY according to PARAMS, return the expanded body." (let ((vars (mapcar #'cdr (org-babel-get-header params :var))) (result-params (cdr (assoc :result-params params))) -- 1.8.2
* lisp/ob-core.el (org-babel-if-in-src-block): New macro (org-babel-execute-src-block-maybe), (org-babel-expand-src-block-maybe), (org-babel-load-in-session-maybe), (org-babel-pop-to-session-maybe): Use it org-babel-get-src-block-info is a potentially expensive operation, which is why its ‘light’ argument exists. But in any case, it is overkill to query the whole info, if all that is needed is whether point is in a block or not. Factor the simplified common code out into a macro. --- lisp/ob-core.el | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/lisp/ob-core.el b/lisp/ob-core.el index 723aa9d..283628e 100644 --- a/lisp/ob-core.el +++ b/lisp/ob-core.el @@ -365,15 +365,22 @@ of potentially harmful code." (or (org-babel-execute-src-block-maybe) (org-babel-lob-execute-maybe))) +(defmacro org-babel-when-in-src-block (&rest body) + `(if (or (org-babel-where-is-src-block-head) + (org-babel-get-inline-src-block-matches)) + (progn + ,@body + t) + nil)) + (defun org-babel-execute-src-block-maybe () "Conditionally execute a source block. Detect if this is context for a Babel src-block and if so then run `org-babel-execute-src-block'." (interactive) - (let ((info (org-babel-get-src-block-info))) - (if info - (progn (org-babel-eval-wipe-error-buffer) - (org-babel-execute-src-block current-prefix-arg info) t) nil))) + (org-babel-when-in-src-block + (org-babel-eval-wipe-error-buffer) + (org-babel-execute-src-block current-prefix-arg))) ;;;###autoload (defun org-babel-view-src-block-info () @@ -409,10 +416,8 @@ a window into the `org-babel-get-src-block-info' function." Detect if this is context for a org-babel src-block and if so then run `org-babel-expand-src-block'." (interactive) - (let ((info (org-babel-get-src-block-info))) - (if info - (progn (org-babel-expand-src-block current-prefix-arg info) t) - nil))) + (org-babel-when-in-src-block + (org-babel-expand-src-block current-prefix-arg))) ;;;###autoload (defun org-babel-load-in-session-maybe () @@ -420,10 +425,8 @@ then run `org-babel-expand-src-block'." Detect if this is context for a org-babel src-block and if so then run `org-babel-load-in-session'." (interactive) - (let ((info (org-babel-get-src-block-info))) - (if info - (progn (org-babel-load-in-session current-prefix-arg info) t) - nil))) + (org-babel-when-in-src-block + (org-babel-load-in-session current-prefix-arg))) (add-hook 'org-metaup-hook 'org-babel-load-in-session-maybe) @@ -433,8 +436,8 @@ then run `org-babel-load-in-session'." Detect if this is context for a org-babel src-block and if so then run `org-babel-pop-to-session'." (interactive) - (let ((info (org-babel-get-src-block-info))) - (if info (progn (org-babel-pop-to-session current-prefix-arg info) t) nil))) + (org-babel-when-in-src-block + (org-babel-pop-to-session current-prefix-arg))) (add-hook 'org-metadown-hook 'org-babel-pop-to-session-maybe) -- 1.8.2
* lisp/ob-core.el (org-babel-do-key-sequence-in-edit-buffer), (org-babel-expand-noweb-references), * lisp/ob-tangle.el (org-babel-tangle): Use 'light argument to `org-babel-get-src-block-info'. --- lisp/ob-core.el | 4 ++-- lisp/ob-tangle.el | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lisp/ob-core.el b/lisp/ob-core.el index 283628e..0aae998 100644 --- a/lisp/ob-core.el +++ b/lisp/ob-core.el @@ -912,7 +912,7 @@ source code block, otherwise return nil. With optional prefix argument RE-RUN the source-code block is evaluated even if results already exist." (interactive "P") - (let ((info (org-babel-get-src-block-info))) + (let ((info (org-babel-get-src-block-info 'light))) (when info (save-excursion ;; go to the results, if there aren't any then run the block @@ -2358,7 +2358,7 @@ would set the value of argument \"a\" equal to \"9\". Note that these arguments are not evaluated in the current source-code block but are passed literally to the \"example-block\"." (let* ((parent-buffer (or parent-buffer (current-buffer))) - (info (or info (org-babel-get-src-block-info))) + (info (or info (org-babel-get-src-block-info 'light))) (lang (nth 0 info)) (body (nth 1 info)) (ob-nww-start org-babel-noweb-wrap-start) diff --git a/lisp/ob-tangle.el b/lisp/ob-tangle.el index 4bcb2e3..4c6eb5c 100644 --- a/lisp/ob-tangle.el +++ b/lisp/ob-tangle.el @@ -211,7 +211,7 @@ used to limit the exported source code blocks by language." org-babel-default-header-args)) (tangle-file (when (equal arg '(16)) - (or (cdr (assoc :tangle (nth 2 (org-babel-get-src-block-info)))) + (or (cdr (assoc :tangle (nth 2 (org-babel-get-src-block-info 'light)))) (user-error "Point is not in a source code block")))) path-collector) (mapc ;; map over all languages -- 1.8.2
* lisp/ob-core.el (org-babel-load-in-session), (org-babel-initiate-session), (org-babel-switch-to-session) (org-babel-switch-to-session-with-code): Remove info optional arg The info arg is threaded through this code, but never used by callers (at least in org code). --- lisp/ob-core.el | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/lisp/ob-core.el b/lisp/ob-core.el index 0aae998..c69b736 100644 --- a/lisp/ob-core.el +++ b/lisp/ob-core.el @@ -794,13 +794,13 @@ arguments and pop open the results in a preview buffer." (add-hook 'org-tab-first-hook 'org-babel-header-arg-expand) ;;;###autoload -(defun org-babel-load-in-session (&optional arg info) +(defun org-babel-load-in-session (&optional arg) "Load the body of the current source-code block. Evaluate the header arguments for the source block before entering the session. After loading the body this pops open the session." (interactive) - (let* ((info (or info (org-babel-get-src-block-info))) + (let* ((info (org-babel-get-src-block-info)) (lang (nth 0 info)) (params (nth 2 info)) (body (if (not info) @@ -820,13 +820,13 @@ session." (end-of-line 1))) ;;;###autoload -(defun org-babel-initiate-session (&optional arg info) +(defun org-babel-initiate-session (&optional arg) "Initiate session for current code block. If called with a prefix argument then resolve any variable references in the header arguments and assign these variables in the session. Copy the body of the code block to the kill ring." (interactive "P") - (let* ((info (or info (org-babel-get-src-block-info (not arg)))) + (let* ((info (org-babel-get-src-block-info (not arg))) (lang (nth 0 info)) (body (nth 1 info)) (params (nth 2 info)) @@ -849,19 +849,19 @@ the session. Copy the body of the code block to the kill ring." (funcall init-cmd session params))) ;;;###autoload -(defun org-babel-switch-to-session (&optional arg info) +(defun org-babel-switch-to-session (&optional arg) "Switch to the session of the current code block. Uses `org-babel-initiate-session' to start the session. If called with a prefix argument then this is passed on to `org-babel-initiate-session'." (interactive "P") - (pop-to-buffer (org-babel-initiate-session arg info)) + (pop-to-buffer (org-babel-initiate-session arg)) (end-of-line 1)) (defalias 'org-babel-pop-to-session 'org-babel-switch-to-session) ;;;###autoload -(defun org-babel-switch-to-session-with-code (&optional arg info) +(defun org-babel-switch-to-session-with-code (&optional arg) "Switch to code buffer and display session." (interactive "P") (let ((swap-windows @@ -870,10 +870,9 @@ with a prefix argument then this is passed on to (set-window-buffer (next-window) (current-buffer)) (set-window-buffer (selected-window) other-window-buffer)) (other-window 1))) - (info (org-babel-get-src-block-info)) (org-src-window-setup 'reorganize-frame)) (save-excursion - (org-babel-switch-to-session arg info)) + (org-babel-switch-to-session arg)) (org-edit-src-code) (funcall swap-windows))) -- 1.8.2
* lisp/org.el (org-edit-special): Use prefix arg, as docstring says we do Only makes a difference for src-block editing. --- lisp/org.el | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lisp/org.el b/lisp/org.el index 04ce386..1edfbc4 100644 --- a/lisp/org.el +++ b/lisp/org.el @@ -19943,7 +19943,7 @@ When in a fixed-width region, call `org-edit-fixed-width-region'. When at an #+INCLUDE keyword, visit the included file. On a link, call `ffap' to visit the link at point. Otherwise, return a user error." - (interactive) + (interactive "P") (let ((element (org-element-at-point))) (assert (not buffer-read-only) nil "Buffer is read-only: %s" (buffer-name)) @@ -19958,8 +19958,9 @@ Otherwise, return a user error." ;; At a src-block with a session and function called with ;; an ARG: switch to the buffer related to the inferior ;; process. - (funcall (intern (concat "org-babel-prep-session:" lang)) - session params))))) + (switch-to-buffer + (funcall (intern (concat "org-babel-prep-session:" lang)) + session params)))))) (keyword (if (member (org-element-property :key element) '("INCLUDE" "SETUPFILE")) (find-file -- 1.8.2
* lisp/ob-core.el (org-babel-execute-src-block): Simplify control flow Avoid potential duplication of org-babel-process-params call. Also makes the code simpler. --- lisp/ob-core.el | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/lisp/ob-core.el b/lisp/ob-core.el index c69b736..d8c11ee 100644 --- a/lisp/ob-core.el +++ b/lisp/ob-core.el @@ -557,13 +557,11 @@ Optionally supply a value for PARAMS which will be merged with the header arguments specified at the front of the source code block." (interactive) - (let* ((info (or info (org-babel-get-src-block-info))) - (merged-params (org-babel-merge-params (nth 2 info) params))) - (when (org-babel-check-evaluate - (let ((i info)) (setf (nth 2 i) merged-params) i)) - (let* ((params (if params - (org-babel-process-params merged-params) - (nth 2 info))) + (let* ((info (or info (org-babel-get-src-block-info 'light)))) + (setf (nth 2 info) (org-babel-merge-params (nth 2 info) params)) + (when (org-babel-check-evaluate info) + (setf (nth 2 info) (org-babel-process-params (nth 2 info))) + (let* ((params (nth 2 info)) (cachep (and (not arg) (cdr (assoc :cache params)) (string= "yes" (cdr (assoc :cache params))))) (new-hash (when cachep (org-babel-sha1-hash info))) @@ -578,8 +576,7 @@ block." (let ((result (org-babel-read-result))) (message (replace-regexp-in-string "%" "%%" (format "%S" result))) result))) - ((org-babel-confirm-evaluate - (let ((i info)) (setf (nth 2 i) merged-params) i)) + ((org-babel-confirm-evaluate info) (let* ((lang (nth 0 info)) (result-params (cdr (assoc :result-params params))) (body (setf (nth 1 info) -- 1.8.2
* testing/lisp/test-ob-emacs-lisp.el: Move stray test inside ert-deftest --- testing/lisp/test-ob-emacs-lisp.el | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/testing/lisp/test-ob-emacs-lisp.el b/testing/lisp/test-ob-emacs-lisp.el index 94092e4..d03f048 100644 --- a/testing/lisp/test-ob-emacs-lisp.el +++ b/testing/lisp/test-ob-emacs-lisp.el @@ -62,17 +62,20 @@ (should (string= "" (buffer-substring-no-properties (point-at-bol) (point-at-eol)))))) -(org-test-with-temp-text-in-file " + +(ert-deftest ob-emacs-lisp/commented-last-block-line () + (org-test-with-temp-text-in-file " #+begin_src emacs-lisp :var a=2 2;; #+end_src" - (org-babel-next-src-block) - (org-ctrl-c-ctrl-c) - (re-search-forward "results" nil t) - (forward-line) - (should (string= - ": 2" - (buffer-substring-no-properties (point-at-bol) (point-at-eol))))) + (org-babel-next-src-block) + (org-ctrl-c-ctrl-c) + (re-search-forward "results" nil t) + (forward-line) + (should (string= + ": 2" + (buffer-substring-no-properties (point-at-bol) (point-at-eol)))))) + (provide 'test-ob-emacs-lisp) ;;; test-ob-emacs-lisp.el ends here -- 1.8.2
* lisp/ob-core.el (org-babel-check-confirm-evaluate): remove (org-babel-check-evaluate), (org-babel-confirm-evaluate): move logic here This macro is used in only two places, and has two almost-independent complex logics coded into it. So, suppress the macro and move the logic into the respective functions. --- lisp/ob-core.el | 89 ++++++++++++++++++++++++++------------------------------- 1 file changed, 40 insertions(+), 49 deletions(-) diff --git a/lisp/ob-core.el b/lisp/ob-core.el index d8c11ee..65c5a0b 100644 --- a/lisp/ob-core.el +++ b/lisp/ob-core.el @@ -284,49 +284,26 @@ Returns a list (setf (nth 2 info) (org-babel-process-params (nth 2 info)))) (when info (append info (list name indent))))) -(defvar org-current-export-file) ; dynamically bound -(defmacro org-babel-check-confirm-evaluate (info &rest body) - "Evaluate BODY with special execution confirmation variables set. - -Specifically; NOEVAL will indicate if evaluation is allowed, -QUERY will indicate if a user query is required, CODE-BLOCK will -hold the language of the code block, and BLOCK-NAME will hold the -name of the code block." - (declare (indent defun)) - (org-with-gensyms - (lang block-body headers name eval eval-no export eval-no-export) - `(let* ((,lang (nth 0 ,info)) - (,block-body (nth 1 ,info)) - (,headers (nth 2 ,info)) - (,name (nth 4 ,info)) - (,eval (or (cdr (assoc :eval ,headers)) - (when (assoc :noeval ,headers) "no"))) - (,eval-no (or (equal ,eval "no") - (equal ,eval "never"))) - (,export (org-bound-and-true-p org-current-export-file)) - (,eval-no-export (and ,export (or (equal ,eval "no-export") - (equal ,eval "never-export")))) - (noeval (or ,eval-no ,eval-no-export)) - (query (or (equal ,eval "query") - (and ,export (equal ,eval "query-export")) - (when (functionp org-confirm-babel-evaluate) - (funcall org-confirm-babel-evaluate - ,lang ,block-body)) - org-confirm-babel-evaluate)) - (code-block (if ,info (format " %s " ,lang) " ")) - (block-name (if ,name (format " (%s) " ,name) " "))) - ,@body))) +;; dynamically bound during export +(defvar org-current-export-file) +;; dynamically bound during asynchronous export +(defvar org-babel-confirm-evaluate-answer-no) (defsubst org-babel-check-evaluate (info) "Check if code block INFO should be evaluated. Do not query the user." - (org-babel-check-confirm-evaluate info - (not (when noeval - (message (format "Evaluation of this%scode-block%sis disabled." - code-block block-name)))))) - - ;; dynamically scoped for asynchroneous export -(defvar org-babel-confirm-evaluate-answer-no) + (let* ((params (nth 2 info)) + (name (nth 4 info)) + (eval (cdr (assq :eval params))) + (can-eval (not (or (member eval '("never" "no")) + (assq :noeval params) + (and (org-bound-and-true-p org-current-export-file) + (member eval '("no-export" "never-export"))))))) + (when (not can-eval) + (message (format "Evaluation of this %s code-block (%s) is disabled." + (nth 0 info) + (if name (concat " (" name ") ") "")))) + can-eval)) (defsubst org-babel-confirm-evaluate (info) "Confirm evaluation of the code block INFO. @@ -341,16 +318,30 @@ confirmation from the user. Note disabling confirmation may result in accidental evaluation of potentially harmful code." - (org-babel-check-confirm-evaluate info - (not (when query - (unless - (and (not (org-bound-and-true-p - org-babel-confirm-evaluate-answer-no)) - (yes-or-no-p - (format "Evaluate this%scode block%son your system? " - code-block block-name))) - (message (format "Evaluation of this%scode-block%sis aborted." - code-block block-name))))))) + + (let* ((params (nth 2 info)) + (name (if (nth 4 info) (concat " (" (nth 4 info) ") ") " ")) + (eval (cdr (assq :eval params))) + (should-query (or (equal eval "query") + (and (org-bound-and-true-p org-current-export-file) + (equal eval "query-export")) + (and (functionp org-confirm-babel-evaluate) + (funcall org-confirm-babel-evaluate + (nth 0 info) + (nth 1 info))) + org-confirm-babel-evaluate)) + (result (or (not should-query) + (not (org-bound-and-true-p + org-babel-confirm-evaluate-answer-no)) + (yes-or-no-p + (format "Evaluate this %s code block%son your system? " + (nth 0 info) + name))))) + (when (not result) + (message (format "Evaluation of this %s code-block%sis aborted." + (nth 0 info) + name))) + result)) ;;;###autoload (defun org-babel-execute-safely-maybe () -- 1.8.2
* doc/org.texi: Document how :var introduces code block dependencies. --- doc/org.texi | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/doc/org.texi b/doc/org.texi index 6791570..dda922e 100644 --- a/doc/org.texi +++ b/doc/org.texi @@ -13211,6 +13211,15 @@ include anything in the Org mode file that takes a @code{#+NAME:}, @code{#+BEGIN_EXAMPLE} blocks, other code blocks, and the results of other code blocks. +When a reference is made to another code block, the referenced block will be +evaluated whenever needed, in order to supply its value to the referencing +block. If the referenced block is cached (see @ref{cache}), its value will +be reused if possible, instead of being re-calculated. If the referring code +block is cached, its hash value will depend on the value of all the code +blocks it references. This system can thus be used to create a system of +as-needed re-evaluation among code blocks similar to that provided by +@uref{http://yihui.name/knitr/, knitr} or Sweave. + Argument values can be indexed in a manner similar to arrays (see @ref{var, Indexable variable values}). -- 1.8.2
Aaron Ecay writes: > * lisp/ob-core.el (org-babel-if-in-src-block): New macro […] > +(defmacro org-babel-when-in-src-block (&rest body) > + `(if (or (org-babel-where-is-src-block-head) > + (org-babel-get-inline-src-block-matches)) > + (progn > + ,@body > + t) > + nil)) Commit message and patch disagree about the name. Regards, Achim. -- +<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+ Factory and User Sound Singles for Waldorf rackAttack: http://Synth.Stromeko.net/Downloads.html#WaldorfSounds
Aaron Ecay writes: > * lisp/ob-core.el (org-babel-execute-src-block): Simplify control flow > > Avoid potential duplication of org-babel-process-params call. Also > makes the code simpler. You may be changing semantics here. I'm not entirely certain if the current way of dealing with the the unmerged and merged parameters and the info block is necessary, but I'd be wary of such a change. Regards, Achim. -- +<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+ Wavetables for the Waldorf Blofeld: http://Synth.Stromeko.net/Downloads.html#BlofeldUserWavetables
Aaron Ecay writes: > * lisp/ob-core.el (org-babel-check-confirm-evaluate): remove > (org-babel-check-evaluate), > (org-babel-confirm-evaluate): move logic here > > This macro is used in only two places, and has two almost-independent > complex logics coded into it. So, suppress the macro and move the logic > into the respective functions. I have recently introduced that macro because no amount of documentation can guarantee that the two functions using these values compute them the same way when somebody makes further changes down the road. That is, however, mandatory for these functions to work properly and safely. I haven't checked if the logic hasn't changed with that patch, but I don't think it's any easier to understand than before. Regards, Achim. -- +<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+ Wavetables for the Terratec KOMPLEXER: http://Synth.Stromeko.net/Downloads.html#KomplexerWaves
Aaron Ecay <aaronecay@gmail.com> writes: > Here are several patches to fix things in and around org-babel. > They're each independent of the others (and hopefully all apply > cleanly, without depending on other members of the series). Here's a > little summary of each: > Thanks for these patches, many look like obvious wins, some less so. I likely won't have time to review them (or do much of anything) in the near term, but at some point I will make time to review them and reply here. -- Eric Schulte http://cs.unm.edu/~eschulte
Aaron Ecay <aaronecay@gmail.com> writes: > * lisp/org.el (org-edit-special): Use prefix arg, as docstring says we > do > This is beyond my ken. I'll leave review of this patch to Bastien. > > Only makes a difference for src-block editing. > --- > lisp/org.el | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/lisp/org.el b/lisp/org.el > index 04ce386..1edfbc4 100644 > --- a/lisp/org.el > +++ b/lisp/org.el > @@ -19943,7 +19943,7 @@ When in a fixed-width region, call `org-edit-fixed-width-region'. > When at an #+INCLUDE keyword, visit the included file. > On a link, call `ffap' to visit the link at point. > Otherwise, return a user error." > - (interactive) > + (interactive "P") > (let ((element (org-element-at-point))) > (assert (not buffer-read-only) nil > "Buffer is read-only: %s" (buffer-name)) > @@ -19958,8 +19958,9 @@ Otherwise, return a user error." > ;; At a src-block with a session and function called with > ;; an ARG: switch to the buffer related to the inferior > ;; process. > - (funcall (intern (concat "org-babel-prep-session:" lang)) > - session params))))) > + (switch-to-buffer > + (funcall (intern (concat "org-babel-prep-session:" lang)) > + session params)))))) > (keyword > (if (member (org-element-property :key element) '("INCLUDE" "SETUPFILE")) > (find-file -- Eric Schulte http://cs.unm.edu/~eschulte
Aaron Ecay <aaronecay@gmail.com> writes: > * lisp/ob-R.el (org-babel-R-initiate-session): handle case where the > session buffer exists, but does not have a live process > Applied, but I removed an unnecessary save-excursion nested inside of a save-window-excursion. Thanks! -- Eric Schulte http://cs.unm.edu/~eschulte
Aaron Ecay <aaronecay@gmail.com> writes: > * testing/lisp/test-ob-emacs-lisp.el: Move stray test inside > ert-deftest Applied, Thanks! -- Eric Schulte http://cs.unm.edu/~eschulte
Achim Gratz <Stromeko@nexgo.de> writes: > Aaron Ecay writes: >> * lisp/ob-core.el (org-babel-execute-src-block): Simplify control flow >> >> Avoid potential duplication of org-babel-process-params call. Also >> makes the code simpler. > > You may be changing semantics here. I'm not entirely certain if the > current way of dealing with the the unmerged and merged parameters and > the info block is necessary, but I'd be wary of such a change. > Can you check if this change causes any of the existing tests to fail? Thanks, -- Eric Schulte http://cs.unm.edu/~eschulte
Aaron Ecay <aaronecay@gmail.com> writes: > * lisp/ob-awk.el (org-babel-expand-body:awk), > lisp/ob-picolisp.el (org-babel-expand-body:picolisp): remove optional > arg from these functions > > The optional argument is apparently never passed by org-babel code. > Applied, Thanks! > > Maybe this is a relic of an earlier calling convention? > Yes, that is exactly the case. -- Eric Schulte http://cs.unm.edu/~eschulte
Aaron Ecay <aaronecay@gmail.com> writes: > * lisp/ob-core.el (org-babel-load-in-session), > (org-babel-initiate-session), > (org-babel-switch-to-session) > (org-babel-switch-to-session-with-code): Remove info optional arg > > The info arg is threaded through this code, but never used by > callers (at least in org code). The rgrep command disagrees with this last statement. ./ob-core.el:411: (progn (org-babel-load-in-session current-prefix-arg info) t) -- Eric Schulte http://cs.unm.edu/~eschulte
Aaron Ecay <aaronecay@gmail.com> writes: > * doc/org.texi: Document how :var introduces code block dependencies. I've committed this patch, although I then simplified your discussion of code block re-execution. Thanks! -- Eric Schulte http://cs.unm.edu/~eschulte
Achim Gratz <Stromeko@nexgo.de> writes: > Aaron Ecay writes: >> * lisp/ob-core.el (org-babel-check-confirm-evaluate): remove >> (org-babel-check-evaluate), >> (org-babel-confirm-evaluate): move logic here >> >> This macro is used in only two places, and has two almost-independent >> complex logics coded into it. So, suppress the macro and move the logic >> into the respective functions. > > I have recently introduced that macro because no amount of documentation > can guarantee that the two functions using these values compute them the > same way when somebody makes further changes down the road. That is, > however, mandatory for these functions to work properly and safely. > > I haven't checked if the logic hasn't changed with that patch, but I > don't think it's any easier to understand than before. > I agree with Achim, I think we should retain the macro. Best, -- Eric Schulte http://cs.unm.edu/~eschulte
Aaron Ecay <aaronecay@gmail.com> writes: > * lisp/ob-core.el (org-babel-do-key-sequence-in-edit-buffer), > (org-babel-expand-noweb-references), > * lisp/ob-tangle.el (org-babel-tangle): > Use 'light argument to `org-babel-get-src-block-info'. I'd like to apply this patch, however tracing the effects of these changes can be tricky and `org-babel-expand-noweb-references' and `org-babel-tangle' are both core functions. Have you run the test suite to confirm that these changes don't break any existing tests? Thanks, -- Eric Schulte http://cs.unm.edu/~eschulte
Eric Schulte <schulte.eric@gmail.com> writes: > Aaron Ecay <aaronecay@gmail.com> writes: > >> Here are several patches to fix things in and around org-babel. >> They're each independent of the others (and hopefully all apply >> cleanly, without depending on other members of the series). Here's a >> little summary of each: >> > > Thanks for these patches, many look like obvious wins, some less so. I > likely won't have time to review them (or do much of anything) in the > near term, but at some point I will make time to review them and reply > here. I've reviewed these patches and applied most of them. Let me express a heartfelt THANKS. It is always good to have more eyes on code (as these patches indicate), and I'm very happy you're learning the internals of Org mode's code block support. Although these patches are mainly refactoring I'm excited to have your help in bug-fixing and feature enhancements in the future. Cheers, -- Eric Schulte http://cs.unm.edu/~eschulte
Hi Aaron, Aaron Ecay <aaronecay@gmail.com> writes: > * lisp/ob-core.el (org-babel-if-in-src-block): New macro > (org-babel-execute-src-block-maybe), > (org-babel-expand-src-block-maybe), > (org-babel-load-in-session-maybe), > (org-babel-pop-to-session-maybe): Use it A slightly enhanced version: * lisp/ob-core.el (org-babel-if-in-src-block): New macro. (org-babel-execute-src-block-maybe) (org-babel-expand-src-block-maybe) (org-babel-load-in-session-maybe, org-babel-pop-to-session-maybe): Use it. In a nutshell: 1. No commas outside parentheses; 2. A full-stop at the end of sentences... C-x 4 a and M-q should be all what you need. > org-babel-get-src-block-info is a potentially expensive operation, which > is why its ‘light’ argument exists. But in any case, it is overkill to > query the whole info, if all that is needed is whether point is in a > block or not. Factor the simplified common code out into a macro. The let-bound info variable is not only used to check if we are within a src block, it is also passed as an argument to functions, see the ^^ marks below. > --- > lisp/ob-core.el | 31 +++++++++++++++++-------------- > 1 file changed, 17 insertions(+), 14 deletions(-) > > diff --git a/lisp/ob-core.el b/lisp/ob-core.el > index 723aa9d..283628e 100644 > --- a/lisp/ob-core.el > +++ b/lisp/ob-core.el > @@ -365,15 +365,22 @@ of potentially harmful code." > (or (org-babel-execute-src-block-maybe) > (org-babel-lob-execute-maybe))) > > +(defmacro org-babel-when-in-src-block (&rest body) > + `(if (or (org-babel-where-is-src-block-head) > + (org-babel-get-inline-src-block-matches)) > + (progn > + ,@body > + t) > + nil)) (Please always add a docstring of defuns and defmacros) > (defun org-babel-execute-src-block-maybe () > "Conditionally execute a source block. > Detect if this is context for a Babel src-block and if so > then run `org-babel-execute-src-block'." > (interactive) > - (let ((info (org-babel-get-src-block-info))) > - (if info > - (progn (org-babel-eval-wipe-error-buffer) > - (org-babel-execute-src-block current-prefix-arg info) t) nil))) ^^^^ > + (org-babel-when-in-src-block > + (org-babel-eval-wipe-error-buffer) > + (org-babel-execute-src-block current-prefix-arg))) > > ;;;###autoload > (defun org-babel-view-src-block-info () > @@ -409,10 +416,8 @@ a window into the `org-babel-get-src-block-info' function." > Detect if this is context for a org-babel src-block and if so > then run `org-babel-expand-src-block'." > (interactive) > - (let ((info (org-babel-get-src-block-info))) > - (if info > - (progn (org-babel-expand-src-block current-prefix-arg info) t) ^^^^ > - nil))) > + (org-babel-when-in-src-block > + (org-babel-expand-src-block current-prefix-arg))) > > ;;;###autoload > (defun org-babel-load-in-session-maybe () > @@ -420,10 +425,8 @@ then run `org-babel-expand-src-block'." > Detect if this is context for a org-babel src-block and if so > then run `org-babel-load-in-session'." > (interactive) > - (let ((info (org-babel-get-src-block-info))) > - (if info > - (progn (org-babel-load-in-session current-prefix-arg info) t) ^^^^ > - nil))) > + (org-babel-when-in-src-block > + (org-babel-load-in-session current-prefix-arg))) > > (add-hook 'org-metaup-hook 'org-babel-load-in-session-maybe) > > @@ -433,8 +436,8 @@ then run `org-babel-load-in-session'." > Detect if this is context for a org-babel src-block and if so > then run `org-babel-pop-to-session'." > (interactive) > - (let ((info (org-babel-get-src-block-info))) > - (if info (progn (org-babel-pop-to-session current-prefix-arg info) t) nil))) ^^^^ > + (org-babel-when-in-src-block > + (org-babel-pop-to-session current-prefix-arg))) (Let's use the current name `org-babel-switch-to-session' instead of the obsolete alias.) Maybe we don't always need to pass the info as an argument, but at least for this last example it is needed. Thanks, -- Bastien
Eric Schulte <schulte.eric@gmail.com> writes:
> I've reviewed these patches and applied most of them.
>
> Let me express a heartfelt THANKS. It is always good to have more eyes
> on code (as these patches indicate), and I'm very happy you're learning
> the internals of Org mode's code block support. Although these patches
> are mainly refactoring I'm excited to have your help in bug-fixing and
> feature enhancements in the future.
Yes, thanks a lot to both of you!
--
Bastien
Eric Schulte <schulte.eric@gmail.com> writes:
> Aaron Ecay <aaronecay@gmail.com> writes:
>
>> * lisp/org.el (org-edit-special): Use prefix arg, as docstring says we
>> do
>>
>
> This is beyond my ken. I'll leave review of this patch to Bastien.
Applied, thanks.
--
Bastien
Eric Schulte writes: > Can you check if this change causes any of the existing tests to fail? I don't think there is a test for that, at least I don't remember anything in that direction. However when implementing my earlier change w.r.t. confirmation I noticed that merging the parameters early has potential for triggering execution of source blocks that would otherwise lay dormant until the execution of the current block was already confirmed. As I said, I have no idea if this behaviour is intended, but that was reason enough for me not to try to "optimize" this away. The behaviour Aaron tries to implement is maybe more sane, but it does alter some corner cases and I can't tell how practically relevant this is. But if we want to change it then I agree that the time is now. Regards, Achim. -- +<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+ Wavetables for the Waldorf Blofeld: http://Synth.Stromeko.net/Downloads.html#BlofeldUserWavetables
Achim Gratz <Stromeko@nexgo.de> writes: > Eric Schulte writes: >> Can you check if this change causes any of the existing tests to fail? > > I don't think there is a test for that, at least I don't remember > anything in that direction. However when implementing my earlier change > w.r.t. confirmation I noticed that merging the parameters early has > potential for triggering execution of source blocks that would otherwise > lay dormant until the execution of the current block was already > confirmed. As I said, I have no idea if this behaviour is intended, but > that was reason enough for me not to try to "optimize" this away. The > behaviour Aaron tries to implement is maybe more sane, but it does alter > some corner cases and I can't tell how practically relevant this is. > But if we want to change it then I agree that the time is now. > I'm happy with the current implementation, even if it is a couple of lines longer, it has the benefit of having been used in production for a time and proven itself (sufficiently) bug free. So lets discard this patch and stick with the current for now. Thanks, > > > Regards, > Achim. -- Eric Schulte http://cs.unm.edu/~eschulte
Hi Bastien, Thanks for your comments. 2013ko apirilak 3an, Bastien-ek idatzi zuen: [...] >> org-babel-get-src-block-info is a potentially expensive operation, which >> is why its ‘light’ argument exists. But in any case, it is overkill to >> query the whole info, if all that is needed is whether point is in a >> block or not. Factor the simplified common code out into a macro. > > The let-bound info variable is not only used to check if we are within > a src block, it is also passed as an argument to functions, see the ^^ > marks below. All of these functions will re-calculate the info if it is not passed, using org-babel-get-src-block-info. So not passing it does no harm. > >> --- >> lisp/ob-core.el | 31 +++++++++++++++++-------------- >> 1 file changed, 17 insertions(+), 14 deletions(-) >> >> diff --git a/lisp/ob-core.el b/lisp/ob-core.el >> index 723aa9d..283628e 100644 >> --- a/lisp/ob-core.el >> +++ b/lisp/ob-core.el >> @@ -365,15 +365,22 @@ of potentially harmful code." >> (or (org-babel-execute-src-block-maybe) >> (org-babel-lob-execute-maybe))) >> >> +(defmacro org-babel-when-in-src-block (&rest body) >> + `(if (or (org-babel-where-is-src-block-head) >> + (org-babel-get-inline-src-block-matches)) >> + (progn >> + ,@body >> + t) >> + nil)) > > (Please always add a docstring of defuns and defmacros) I’ll resend the patch with a docstring and fixing the commit message problems you and Achim pointed out. [...] >> @@ -433,8 +436,8 @@ then run `org-babel-load-in-session'." >> Detect if this is context for a org-babel src-block and if so >> then run `org-babel-pop-to-session'." >> (interactive) >> - (let ((info (org-babel-get-src-block-info))) >> - (if info (progn (org-babel-pop-to-session current-prefix-arg info) t) nil))) > ^^^^ >> + (org-babel-when-in-src-block >> + (org-babel-pop-to-session current-prefix-arg))) > > (Let's use the current name `org-babel-switch-to-session' instead of > the obsolete alias.) OK. > > Maybe we don't always need to pass the info as an argument, but at > least for this last example it is needed. o-b-switch-to-session does nothing with the info argument but pass it along to o-b-initiate-session, which will recalculate it if it is missing. So it takes 2 hops in contrast to the 1 in the other cases, but it still gets recalculated. -- Aaron Ecay
Hi Eric,
Thanks for your comments on this and all the patches.
2013ko apirilak 3an, Eric Schulte-ek idatzi zuen:
>
> Aaron Ecay <aaronecay@gmail.com> writes:
>
>> * lisp/ob-core.el (org-babel-load-in-session),
>> (org-babel-initiate-session),
>> (org-babel-switch-to-session)
>> (org-babel-switch-to-session-with-code): Remove info optional arg
>>
>> The info arg is threaded through this code, but never used by
>> callers (at least in org code).
>
> The rgrep command disagrees with this last statement.
>
> ./ob-core.el:411: (progn (org-babel-load-in-session current-prefix-arg info) t)
This was removed in patch #3 of the series. I tried to make all the
patches independent of each other, but this dependency did slip in. I
can squash the two patches if you’d prefer.
--
Aaron Ecay
Hi Eric,
2013ko apirilak 3an, Eric Schulte-ek idatzi zuen:
>
> Aaron Ecay <aaronecay@gmail.com> writes:
>
>> * lisp/ob-core.el (org-babel-do-key-sequence-in-edit-buffer),
>> (org-babel-expand-noweb-references),
>> * lisp/ob-tangle.el (org-babel-tangle):
>> Use 'light argument to `org-babel-get-src-block-info'.
>
> I'd like to apply this patch, however tracing the effects of these
> changes can be tricky and `org-babel-expand-noweb-references' and
> `org-babel-tangle' are both core functions.
>
> Have you run the test suite to confirm that these changes don't break
> any existing tests?
The test suite gives no failures with this patch.
--
Aaron Ecay
* lisp/ob-core.el (org-babel-when-in-src-block): New macro. (org-babel-execute-src-block-maybe) (org-babel-expand-src-block-maybe) (org-babel-load-in-session-maybe, org-babel-pop-to-session-maybe): Use it. org-babel-get-src-block-info is a potentially expensive operation, which is why its ‘light’ argument exists. But in any case, it is overkill to query the whole info, if all that is needed is whether point is in a block or not. Factor the simplified common code out into a macro. --- lisp/ob-core.el | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/lisp/ob-core.el b/lisp/ob-core.el index b8d93f2..e44fc02 100644 --- a/lisp/ob-core.el +++ b/lisp/ob-core.el @@ -351,15 +351,25 @@ of potentially harmful code." (or (org-babel-execute-src-block-maybe) (org-babel-lob-execute-maybe))) +(defmacro org-babel-when-in-src-block (&rest body) + "Execute BODY if point is in a source block and return t. + +Otherwise do nothing and return nil." + `(if (or (org-babel-where-is-src-block-head) + (org-babel-get-inline-src-block-matches)) + (progn + ,@body + t) + nil)) + (defun org-babel-execute-src-block-maybe () "Conditionally execute a source block. Detect if this is context for a Babel src-block and if so then run `org-babel-execute-src-block'." (interactive) - (let ((info (org-babel-get-src-block-info))) - (if info - (progn (org-babel-eval-wipe-error-buffer) - (org-babel-execute-src-block current-prefix-arg info) t) nil))) + (org-babel-when-in-src-block + (org-babel-eval-wipe-error-buffer) + (org-babel-execute-src-block current-prefix-arg))) ;;;###autoload (defun org-babel-view-src-block-info () @@ -395,10 +405,8 @@ a window into the `org-babel-get-src-block-info' function." Detect if this is context for a org-babel src-block and if so then run `org-babel-expand-src-block'." (interactive) - (let ((info (org-babel-get-src-block-info))) - (if info - (progn (org-babel-expand-src-block current-prefix-arg info) t) - nil))) + (org-babel-when-in-src-block + (org-babel-expand-src-block current-prefix-arg))) ;;;###autoload (defun org-babel-load-in-session-maybe () @@ -406,10 +414,8 @@ then run `org-babel-expand-src-block'." Detect if this is context for a org-babel src-block and if so then run `org-babel-load-in-session'." (interactive) - (let ((info (org-babel-get-src-block-info))) - (if info - (progn (org-babel-load-in-session current-prefix-arg info) t) - nil))) + (org-babel-when-in-src-block + (org-babel-load-in-session current-prefix-arg))) (add-hook 'org-metaup-hook 'org-babel-load-in-session-maybe) @@ -419,8 +425,8 @@ then run `org-babel-load-in-session'." Detect if this is context for a org-babel src-block and if so then run `org-babel-pop-to-session'." (interactive) - (let ((info (org-babel-get-src-block-info))) - (if info (progn (org-babel-pop-to-session current-prefix-arg info) t) nil))) + (org-babel-when-in-src-block + (org-babel-switch-to-session current-prefix-arg))) (add-hook 'org-metadown-hook 'org-babel-pop-to-session-maybe) -- 1.8.2.1
Aaron Ecay <aaronecay@gmail.com> writes:
> * lisp/ob-core.el (org-babel-when-in-src-block): New macro.
> (org-babel-execute-src-block-maybe)
> (org-babel-expand-src-block-maybe)
> (org-babel-load-in-session-maybe, org-babel-pop-to-session-maybe):
> Use it.
Applied, thanks.
--
Bastien
Aaron Ecay <aaronecay@gmail.com> writes: > Hi Eric, > > 2013ko apirilak 3an, Eric Schulte-ek idatzi zuen: >> >> Aaron Ecay <aaronecay@gmail.com> writes: >> >>> * lisp/ob-core.el (org-babel-do-key-sequence-in-edit-buffer), >>> (org-babel-expand-noweb-references), >>> * lisp/ob-tangle.el (org-babel-tangle): >>> Use 'light argument to `org-babel-get-src-block-info'. >> >> I'd like to apply this patch, however tracing the effects of these >> changes can be tricky and `org-babel-expand-noweb-references' and >> `org-babel-tangle' are both core functions. >> >> Have you run the test suite to confirm that these changes don't break >> any existing tests? > > The test suite gives no failures with this patch. Then please go ahead and apply this patch (or re-send it to me and I can apply it). Cheers, -- Eric Schulte http://cs.unm.edu/~eschulte
Aaron Ecay <aaronecay@gmail.com> writes: > Hi Bastien, > > Thanks for your comments. > > 2013ko apirilak 3an, Bastien-ek idatzi zuen: > > [...] > >>> org-babel-get-src-block-info is a potentially expensive operation, which >>> is why its ‘light’ argument exists. But in any case, it is overkill to >>> query the whole info, if all that is needed is whether point is in a >>> block or not. Factor the simplified common code out into a macro. >> >> The let-bound info variable is not only used to check if we are within >> a src block, it is also passed as an argument to functions, see the ^^ >> marks below. > > All of these functions will re-calculate the info if it is not > passed, using org-babel-get-src-block-info. So not passing it does no > harm. > Could re-calculating the info cause referenced blocks to be executed more than once? If so then we should continue passing the info and *not* simply re-calculate it later on. Cheers, > >> >>> --- >>> lisp/ob-core.el | 31 +++++++++++++++++-------------- >>> 1 file changed, 17 insertions(+), 14 deletions(-) >>> >>> diff --git a/lisp/ob-core.el b/lisp/ob-core.el >>> index 723aa9d..283628e 100644 >>> --- a/lisp/ob-core.el >>> +++ b/lisp/ob-core.el >>> @@ -365,15 +365,22 @@ of potentially harmful code." >>> (or (org-babel-execute-src-block-maybe) >>> (org-babel-lob-execute-maybe))) >>> >>> +(defmacro org-babel-when-in-src-block (&rest body) >>> + `(if (or (org-babel-where-is-src-block-head) >>> + (org-babel-get-inline-src-block-matches)) >>> + (progn >>> + ,@body >>> + t) >>> + nil)) >> >> (Please always add a docstring of defuns and defmacros) > > I’ll resend the patch with a docstring and fixing the commit message > problems you and Achim pointed out. > > > [...] > > >>> @@ -433,8 +436,8 @@ then run `org-babel-load-in-session'." >>> Detect if this is context for a org-babel src-block and if so >>> then run `org-babel-pop-to-session'." >>> (interactive) >>> - (let ((info (org-babel-get-src-block-info))) >>> - (if info (progn (org-babel-pop-to-session current-prefix-arg info) t) nil))) >> ^^^^ >>> + (org-babel-when-in-src-block >>> + (org-babel-pop-to-session current-prefix-arg))) >> >> (Let's use the current name `org-babel-switch-to-session' instead of >> the obsolete alias.) > > OK. > >> >> Maybe we don't always need to pass the info as an argument, but at >> least for this last example it is needed. > > o-b-switch-to-session does nothing with the info argument but pass it > along to o-b-initiate-session, which will recalculate it if it is > missing. So it takes 2 hops in contrast to the 1 in the other cases, > but it still gets recalculated. -- Eric Schulte http://cs.unm.edu/~eschulte
Hi Eric,
2013ko apirilak 20an, Eric Schulte-ek idatzi zuen:
>
> Then please go ahead and apply this patch (or re-send it to me and I can
> apply it).
Done. :)
--
Aaron Ecay
Hi Eric,
2013ko apirilak 20an, Eric Schulte-ek idatzi zuen:
> Could re-calculating the info cause referenced blocks to be executed
> more than once?
>
> If so then we should continue passing the info and *not* simply
> re-calculate it later on.
This is a very good question. I will look into it and send an update
when I have found out the answer.
Thanks,
--
Aaron Ecay