From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jack Kamm Subject: Re: correct remote path handling Date: Sat, 29 Feb 2020 08:22:37 -0800 Message-ID: <87v9nptlbm.fsf@gmail.com> References: <1873694.uLqJpLN6Zz@linux-q1p8> <8736ayy85n.fsf@gmail.com> <87zhd6fpj6.fsf@gnu.org> <1993557.tO6IXrBGSL@lenovo-x200> Mime-Version: 1.0 Content-Type: text/plain Return-path: Received: from eggs.gnu.org ([2001:470:142:3::10]:53828) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1j84uC-0007h4-3t for emacs-orgmode@gnu.org; Sat, 29 Feb 2020 11:23:57 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1j84uB-00073g-04 for emacs-orgmode@gnu.org; Sat, 29 Feb 2020 11:23:56 -0500 In-Reply-To: <1993557.tO6IXrBGSL@lenovo-x200> List-Id: "General discussions about Org-mode." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-orgmode-bounces+geo-emacs-orgmode=m.gmane-mx.org@gnu.org Sender: "Emacs-orgmode" To: Felipe Lema , Bastien Cc: emacs-orgmode@gnu.org Hi Felipe, It looks like you've made some quite substantial changes to ob-shell. I think it would be a good idea to split this up into 2 patches, and to start a new thread for the ob-shell patch. I'm not as familiar with ob-shell, and it's also had some work lately. So it'd be good to get some more visibility for these changes with a new thread for it. Also, I think it'd be a good idea to provide some explanation of what you've changed in ob-shell, the reason for these changes, and some examples of how the behavior has changed. If possible, it'd also be good to split up the ob-shell changes into a few smaller patches, each addressing a specific issue. Before starting a new thread, I would recommend waiting until Org 9.4 is released, which should be any day now. Below are some specific feedback for your updated patch: > diff --git a/lisp/ob-python.el b/lisp/ob-python.el > index dbcfac08d..1afea9cb3 100644 > --- a/lisp/ob-python.el > +++ b/lisp/ob-python.el > @@ -327,7 +327,8 @@ last statement in BODY, as elisp." > "python-"))) > (with-temp-file tmp-src-file (insert body)) > (format org-babel-python--exec-tmpfile > - tmp-src-file)) > + (org-babel-local-file-name > + tmp-src-file))) You should use `org-babel-process-file-name' here, not `org-babel-local-file-name'. > - tmp-src-file)))) > - (org-babel-comint-with-output > - (session org-babel-python-eoe-indicator nil body) > + (org-babel-local-file-name > + tmp-src-file))))) > + (org-babel-comint-with-output > + (session org-babel-python-eoe-indicator nil body) Same comment as above. > diff --git a/lisp/ob-shell.el b/lisp/ob-shell.el > index 347ffedd1..66cdfb94c 100644 > --- a/lisp/ob-shell.el > +++ b/lisp/ob-shell.el > @@ -82,12 +82,17 @@ This function is called by `org-babel-execute-src-block'." > (value-is-exit-status > (member "value" (cdr (assq :result-params params)))) > (cmdline (cdr (assq :cmdline params))) > + (shebang (cdr (assq :shebang params))) > + (padline (not (equal "no" (cdr (assq :padline params))))) > + (result-params (cdr (assq :result-params params))) > (full-body (concat > (org-babel-expand-body:generic > body params (org-babel-variable-assignments:shell params)) > (when value-is-exit-status "\necho $?")))) > (org-babel-reassemble-table > - (org-babel-sh-evaluate session full-body params stdin cmdline) > + (org-babel-sh-evaluate session full-body > + stdin cmdline shebang value-is-exit-status padline > + result-params) These variables used to be let-bound inside `org-babel-sh-evaluate', but now you've changed the signature of that function to pass them in from the outside. I don't see a good reason for this change, since as far as I can tell, those variables aren't used outside `org-babel-sh-evaluate'. > -(defun org-babel-sh-evaluate (session body &optional params stdin cmdline) > - "Pass BODY to the Shell process in BUFFER. > -If RESULT-TYPE equals `output' then return a list of the outputs > -of the statements in BODY, if RESULT-TYPE equals `value' then > -return the value of the last statement in BODY." It looks like you've changed the indentation of this function, which causes a hard-to-read diff. Please stick to the original indentation so we can more easily review the changes here. > + (let* ((block-output-lines > + (let ((fun-body > + (format "%s(){\n%s\n}\n%s" > + org-babel-sh-block-function-name > + ;; function block > + (concat > + body > + "\n" > + org-babel-sh-eoe-indicator) ;; mark eoe > + ;; mark "function has been input" > + org-babel-sh-eoe-indicator))) This is a very interesting idea, to wrap the shell block inside a function. I think it's a good idea and would fix some issues with leaky prompts, among other things. As an aside, I've been working on developing an async evaluation feature for org-babel blocks, but we were struggling with ob-shell. This idea to wrap the block in a function might be the solution. Anyways, it would be good to discuss this change further with motivation and examples.