Re: correct remote path handling

[Jack Kamm]

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.