Re: correct remote path handling

[Jack Kamm <jackkamm@gmail.com>]

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.