[BUG] ob-shell: :shebang changes interpretation of :cmdline

[BUG] ob-shell: :shebang changes interpretation of :cmdline

[Max Nikulin]

Hi,

Trying to figure out the origin of the confusion with
"bash -c bash /path/to/file-containing-the-source-code.sh"
I have faced an inconsistency with :cmdline treatment in ob-shell.el. I 
expect same results in the following cases:

#+begin_src bash :cmdline 1 2 3
   printf "%s\n" "$1"
#+end_src

#+RESULTS:
: 1

#+begin_src bash :cmdline 1 2 3 :shebang #!/bin/bash
   printf "%s\n" "$1"
#+end_src

#+RESULTS:
: 1 2 3

Emacs-28, Org is the current git HEAD.

Re: [BUG] ob-shell: :shebang changes interpretation of :cmdline

[Matt]

[-- Attachment #1: Type: text/plain, Size: 857 bytes --]


 ---- On Sat, 18 Nov 2023 16:54:39 +0100  Max Nikulin  wrote --- 

 > I have faced an inconsistency with :cmdline treatment in ob-shell.el. I 
 > expect same results in the following cases:
 > 
 > #+begin_src bash :cmdline 1 2 3
 >    printf "%s\n" "$1"
 > #+end_src
 > 
 > #+RESULTS:
 > : 1
 > 
 > #+begin_src bash :cmdline 1 2 3 :shebang #!/bin/bash
 >    printf "%s\n" "$1"
 > #+end_src
 > 
 > #+RESULTS:
 > : 1 2 3

Thank you!  This makes a good test case.

Ihor or Bastion, Craig and my employer reached agreement on the disclaimer language.  I've sent a signed copy to Craig and haven't heard back yet with the go-ahead to continue contributing.  I sent the paperwork only a week or so ago, so I'm sure he'll get back to me in time.  Because of this, I'm including patches rather than pushing (which assumes I still have access (I haven't checked)).

[-- Attachment #2: test-cmdline-alone-and-with-shebang-have-same-result.patch --]
[-- Type: application/octet-stream, Size: 971 bytes --]

diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el
index 879555af0..8f6180153 100644
--- a/testing/lisp/test-ob-shell.el
+++ b/testing/lisp/test-ob-shell.el
@@ -436,6 +436,26 @@ exiting with a non-zero return code multiple times."
                          (buffer-string)))))
       (kill-buffer "*Org-Babel Error Output*")))
 
+\f
+;;; cmdline
+
+(ert-deftest test-cmdline-alone-and-with-shebang-have-same-result ()
+  "Executing a block with the same shebang as the shell language
+should produce the same output."
+  (should (equal
+           (org-test-with-temp-text
+               "#+begin_src sh :cmdline 1 2 3
+echo \"$1\"
+<point>
+#+end_src"
+             (org-babel-execute-src-block))
+           (org-test-with-temp-text
+               "#+begin_src sh :cmdline \"1\" :shebang #!/bin/bash
+echo \"$1\"
+<point>
+#+end_src"
+             (org-babel-execute-src-block)))))
+
 (provide 'test-ob-shell)
 
 ;;; test-ob-shell.el ends here

[BUG] ob-shell: :cmdline fails with single argument (was Re: [BUG] ob-shell: :shebang changes interpretation of :cmdline)

[Matt]

[-- Attachment #1: Type: text/plain, Size: 951 bytes --]


 ---- On Sat, 18 Nov 2023 16:54:39 +0100  Max Nikulin  wrote --- 

 > I have faced an inconsistency with :cmdline treatment in ob-shell.el. 

These are sadly easy to find.  

If you run:

#+begin_src bash :cmdline 1 
echo "$1"
#+end_src

Then it fails with 

list: Wrong type argument: sequencep, 1

However, running this works:

#+begin_src bash :cmdline "1" 
echo "$1"
#+end_src

I didn't dig too much into it, but it looks like :cmdline expects a sequence.  When multiple arguments are passed, such as :cmdline 1 2 3, then "1 2 3" is passed into process-file.  (sequencep "1 2 3") is t.  (sequencep 1) is nil.  So, to work around this a single :cmdline argument must be surrounded by quotes to make it a sequence.  (sequencep "1") is t.  Obviously, this should be fixed.

Attached is a patch to test for this whenever we're ready to tackle making execution mutually consistent.  I'm still reviewing the library and am not quite ready for that yet.

[-- Attachment #2: test-cmdline-with-single-argument-shouldnt-require-quotes.patch --]
[-- Type: application/octet-stream, Size: 861 bytes --]

diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el
index 879555af0..d41387e57 100644
--- a/testing/lisp/test-ob-shell.el
+++ b/testing/lisp/test-ob-shell.el
@@ -436,6 +436,19 @@ exiting with a non-zero return code multiple times."
                          (buffer-string)))))
       (kill-buffer "*Org-Babel Error Output*")))
 
+(ert-deftest test-cmdline-with-single-argument-shouldnt-require-quotes ()
+  "The original implementation of :cmdline required a single element
+to be surrounded by goes to work."
+  (should
+   (equal 1
+          (org-test-with-temp-text
+              ;; "#+begin_src sh :cmdline \"1\" :shebang #!/bin/bash
+              "#+begin_src sh :cmdline 1 #!/bin/bash
+echo \"$1\"
+<point>
+#+end_src"
+            (org-babel-execute-src-block)))))
+
 (provide 'test-ob-shell)
 
 ;;; test-ob-shell.el ends here

Re: [BUG] ob-shell: :cmdline fails with single argument (was Re: [BUG] ob-shell: :shebang changes interpretation of :cmdline)

[Max Nikulin]

On 19/11/2023 01:20, Matt wrote:
> #+begin_src bash :cmdline 1
> echo "$1"
> #+end_src
> 
> Then it fails with
> 
> list: Wrong type argument: sequencep, 1

I would say that :cmdline is treated in a different way in comparison to 
:var:

#+header: :results verbatim
#+begin_src bash :var arr='(1 2 3) :cmdline '(97 98 99)
   printf '$1:%s\n' "$1"
   declare -p arr
#+end_src

#+RESULTS:
: $1:abc
: declare -a arr=([0]="1" [1]="2" [2]="3")

I would expect more consistent results since script arguments is an 
array (positional arguments). $1=97, $2=98, $3=99

Re: [BUG] ob-shell: :cmdline fails with single argument (was Re: [BUG] ob-shell: :shebang changes interpretation of :cmdline)

[Matt]

 ---- On Sun, 19 Nov 2023 07:57:26 +0100  Max Nikulin  wrote --- 

 > I would say that :cmdline is treated in a different way in comparison to 
 > :var:

It most definitely is.  It's a completely separate process from :var.

Re: [BUG] ob-shell: :shebang changes interpretation of :cmdline

[Ihor Radchenko]

Matt <matt@excalamus.com> writes:

>  > #+begin_src bash :cmdline 1 2 3
>  >    printf "%s\n" "$1"
>  > #+end_src
>  > 
>  > #+RESULTS:
>  > : 1
>  > 
>  > #+begin_src bash :cmdline 1 2 3 :shebang #!/bin/bash
>  >    printf "%s\n" "$1"
>  > #+end_src
>  > 
>  > #+RESULTS:
>  > : 1 2 3
>
> Thank you!  This makes a good test case.

I am confused. Isn't it a bug?

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [BUG] ob-shell: :shebang changes interpretation of :cmdline

[Matt]

 ---- On Mon, 04 Dec 2023 14:55:58 +0100  Ihor Radchenko  wrote --- 
 > Matt matt@excalamus.com> writes:
 > 
 > >  > #+begin_src bash :cmdline 1 2 3
 > >  >    printf "%s\n" "$1"
 > >  > #+end_src
 > >  > 
 > >  > #+RESULTS:
 > >  > : 1
 > >  > 
 > >  > #+begin_src bash :cmdline 1 2 3 :shebang #!/bin/bash
 > >  >    printf "%s\n" "$1"
 > >  > #+end_src
 > >  > 
 > >  > #+RESULTS:
 > >  > : 1 2 3
 > >
 > > Thank you!  This makes a good test case.
 > 
 > I am confused. Isn't it a bug?

Yes, it's a bug.  And the test was wrong.

Here is a corrected version.  When the bug is fixed, the following should pass.  It currently fails because the first block returns 1 and the second block returns 1 2 3 (as seen in the quote).  Both blocks should return 1.

(ert-deftest test-cmdline-alone-and-with-shebang-have-same-result ()
  "Pass arguments to a block.  Don't use shebang.  Then use
shebang set to the same language as the block.  The result should
be the same."                                    
  (should (equal                                                    
           (org-test-with-temp-text                                 
               "#+begin_src bash :cmdline 1 2 3                       
echo \"$1\"                                                         
<point>                                                             
#+end_src"                                                          
             (org-babel-execute-src-block))                         
           (org-test-with-temp-text                                 
               "#+begin_src bash :cmdline 1 2 3 :shebang #!/bin/bash  
echo \"$1\"                                                         
<point>                                                             
#+end_src"                                                          
             (org-babel-execute-src-block)))))                      

[PATCH] Re: [BUG] ob-shell: :shebang changes interpretation of :cmdline

[Matt]

[-- Attachment #1: Type: text/plain, Size: 7954 bytes --]


 ---- On Sat, 18 Nov 2023 16:54:39 +0100  Max Nikulin  wrote --- 
 > Hi,
 > 
 > Trying to figure out the origin of the confusion with
 > "bash -c bash /path/to/file-containing-the-source-code.sh"
 > I have faced an inconsistency with :cmdline treatment in ob-shell.el. I 
 > expect same results in the following cases:
 > 
 > #+begin_src bash :cmdline 1 2 3
 >    printf "%s\n" "$1"
 > #+end_src
 > 
 > #+RESULTS:
 > : 1
 > 
 > #+begin_src bash :cmdline 1 2 3 :shebang #!/bin/bash
 >    printf "%s\n" "$1"
 > #+end_src
 > 
 > #+RESULTS:
 > : 1 2 3
 > 
 > Emacs-28, Org is the current git HEAD.

AFAIU, the inconsistency is due to how the characters following :cmdline are interpreted when the subprocess call is made.

Consider the following, when only :cmdline is used:

# Evaluates like:
#
#     bash -c "./sh-script-8GJzdG 1 2 3"
#
#+begin_src bash :cmdline 1 2 3
echo \"$1\"
#+end_src

#+RESULTS:
: 1

# Evaluates like:
#
#     bash -c "./sh-script-8GJzdG \"1 2\" 3"
#
#+begin_src bash :cmdline "1 2" 3
echo \"$1\"
#+end_src

#+RESULTS:
: 1 2

For :cmdline alone, the characters following :cmdline are passed as though each is quoted.  That is, separate arguments are delimited by one or more spaces.  The first example is equivalent to the following:

# Evaluates like:
#
#     bash -c "./sh-script-8GJzdG \"1\" \"2\" \"3\""
#
#+begin_src bash :cmdline 1 2 3
echo \"$1\"
#+end_src

#+RESULTS:
: 1

How would you expect :cmdline "1 2 3" to be evaluated?

#+begin_src bash :cmdline "1 2 3"
echo \"$1\"
#+end_src

My expectation would be that it evaluates like:

  bash -c "./sh-script-8GJzdG \"1 2 3\""

It turns out, however, that it's evaluated exactly like :cmdline 1 2 3, or :cmdline "1" "2" "3".  The result is "1".

To make the block evaluate as expected requires an extra set of parentheses:

# Evaluates like:
#
#     bash -c "./sh-script-8GJzdG \"1 2 3\""
#
#+begin_src bash :cmdline "\"1 2 3\""
echo \"$1\"
#+end_src

#+RESULTS:
: 1 2 3

This, however, appears to be separate from the reported issue[fn:1].

Now, consider :cmdline paired with :shebang, called with the same values as above.

# Evaluates like:
#
#     /tmp/babel-Xd6rGS/sh-script-61jvMa "1 2 3"
#
#+begin_src bash :cmdline 1 2 3 :shebang #!/usr/bin/env bash
echo \"$1\"
#+end_src

#+RESULTS:
: 1 2 3

# Evaluates like:
#
#     /tmp/babel-Xd6rGS/sh-script-61jvMa "\"1 2\" 3"
#
#+begin_src bash :cmdline "1 2" 3 :shebang #!/usr/bin/env bash
echo \"$1\"
#+end_src

#+RESULTS:
: 1 2" 3"

# Evaluates like:
#
#     /tmp/babel-Xd6rGS/sh-script-61jvMa "1 2 3"
#
#+begin_src bash :cmdline "1 2 3" :shebang #!/usr/bin/env bash
echo \"$1\"
#+end_src

#+RESULTS:
: 1 2 3

# Evaluates like:
#
#     /tmp/babel-Xd6rGS/sh-script-61jvMa "\"1 2 3\""
#
#+begin_src bash :cmdline "\"1 2 3\"" :shebang #!/usr/bin/env bash
echo \"$1\"
#+end_src

#+RESULTS:
: 1 2 3""

# Evaluates like:
#
#     /tmp/babel-Xd6rGS/sh-script-61jvMa "\"1\" \"2\" \"3\""
#
#+begin_src bash :cmdline "1" "2" "3" :shebang #!/usr/bin/env bash
echo \"$1\"
#+end_src

#+RESULTS:
: 1" "2" "3""

An immediate observation is that the output results don't format correctly.  If you change the results type to "raw", however, you'll see that the Org results match those from a terminal, like xfce4-terminal.  The fact that raw output matches output from the terminal means that the formatting issue is (also) separate from the bug we're trying to fix.  That is, the bug we're trying to fix occurs in how the subprocess call is made, not in how the result is formatted.

In ob-shell, the subprocess call is made with 'process-file'.  Arguments are determined casewise:

1. shebang+cmdline
2. cmdline

The characters following :cmdline are received by the 'cmdline' argument to 'org-babel-sh-evaluate' as a string.  Both cases put this string into a list for the ARGS of 'process-file':

| header               | 'org-babel-sh-evaluate' | process-file ARGS     |
|                      | cmdline variable value  | shebang+cmdline       |
|----------------------+-------------------------+-----------------------|
| :cmdline 1 2 3       | "1 2 3"                 | ("1 2 3")             |
| :cmdline "1 2" 3"    | "\"1 2\" 3"             | ("\"1 2\" 3")         |
| :cmdline "1" "2" "3" | "\"1\" \"2\" \"3\""     | ("\"1\" \"2\" \"3\"") |

| header               | 'org-babel-sh-evaluate' | process-file ARGS     |
|                      | cmdline variable value  | cmdline               |
|----------------------+-------------------------+-----------------------|
| :cmdline 1 2 3       | "1 2 3"                 | ("1 2 3")             |
| :cmdline "1 2" 3"    | "\"1 2\" 3"             | ("\"1 2\" 3")         |
| :cmdline "1" "2" "3" | "\"1\" \"2\" \"3\""     | ("\"1\" \"2\" \"3\"") |

Notice that the ARGS passed to 'process-file' are the same for both cases.  The problem is that the "block equivalent shell calls" are *not* the same.  If we arrange the equivalent shell calls from the blocks given above into a table, we see that the forms are different:

| header               | cmdline variable value | shebang+cmdline call                                   |
|----------------------+------------------------+--------------------------------------------------------|
| :cmdline 1 2 3       | "1 2 3"                | /tmp/babel-Xd6rGS/sh-script-61jvMa "1 2 3"             |
| :cmdline "1 2" 3"    | "\"1 2\" 3"            | /tmp/babel-Xd6rGS/sh-script-61jvMa "\"1 2\" 3"         |
| :cmdline "1" "2" "3" | "\"1\" \"2\" \"3\""    | /tmp/babel-Xd6rGS/sh-script-61jvMa "\"1\" \"2\" \"3\"" |

| header               | cmdline variable value | cmdline call                                   |
|----------------------+------------------------+------------------------------------------------|
| :cmdline 1 2 3       | "1 2 3"                | bash -c "./sh-script-8GJzdG 1 2 3"             |
| :cmdline "1 2" 3"    | "\"1 2\" 3"            | bash -c "./sh-script-8GJzdG \"1 2\" 3"         |
| :cmdline "1" "2" "3" | "\"1\" \"2\" \"3\""    | bash -c "./sh-script-8GJzdG \"1\" \"2\" \"3\"" |

The reported bug exists because shebang+cmdline interprets the characters following :cmdline as a *single* string.  Without :shebang, a lone :cmdline interprets them as space delimited.

One possible solution is to reformat the 'process-file' ARGS for the shebang+cmdline case so that characters following :cmdline are interpreted as space delimited.  This is possible using 'split-string-and-unquote':

(split-string-and-unquote "1 2 3")             -> ("1" "2" "3")
(split-string-and-unquote "\"1 2\" 3")         -> ("1 2" "3")
(split-string-and-unquote "\"1\" \"2\" \"3\"") -> ("1" "2" "3")

Whether this is a solution, in part, depends on the perennial problem of shell blocks: knowing what's wrong means knowing what's right.

The proposed solution assumes we intend to parse the characters following :cmdline as space delimited and grouped by quotes.  However, AFAICT, the parsing issue makes this solution ambiguous.

Thoughts?

--
Matt Trzcinski
Emacs Org contributor (ob-shell)
Learn more about Org mode at https://orgmode.org
Support Org development at https://liberapay.com/org-mode

[fn:1] AFAICT, it's due to how headers are parsed by 'org-babel-parse-header-arguments' using 'org-babel-read'.  The cell "\"1 2 3\"" (corresponding to :cmdline "1 2 3") is reduced through 'string-match' to "1 2 3".  The cell "1 2 3" (corresponding to :cmdline 1 2 3), on the other hand, passes through.  The result is that :cmdline "1 2 3" and :cmdline 1 2 3 become indistinguishable.  I mention this because it's easy to get confused by this issue which, AFAICT, is independent of the one we're trying to fix.  The reported issue appears only to be related to how the result of :cmdline header parsing is passed to the subprocess.

[-- Attachment #2: 0001-testing-lisp-test-ob-shell.el-Test-shebang-cmdline-r.patch --]
[-- Type: application/octet-stream, Size: 2070 bytes --]

From acf27407c43ba48be21b8f75a1c791d2c45db57a Mon Sep 17 00:00:00 2001
From: Matthew Trzcinski <matt@excalamus.com>
Date: Sun, 21 Apr 2024 16:02:40 +0200
Subject: [PATCH 1/3] testing/lisp/test-ob-shell.el: Test shebang/cmdline
 results

* test-ob-shell.el
(test-cmdline-alone-and-with-shebang-have-same-result): Test that
results when using the cmdline header alone are the same as when
paired with the shebang header.
---
 testing/lisp/test-ob-shell.el | 37 +++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el
index 8cebd8467..b0c15b07d 100644
--- a/testing/lisp/test-ob-shell.el
+++ b/testing/lisp/test-ob-shell.el
@@ -362,6 +362,43 @@ echo 3
       "- 1\n- 2\n- 3\n"
       (buffer-substring-no-properties (point) (point-max))))))
 
+(ert-deftest test-cmdline-alone-and-with-shebang-have-same-result ()
+  "Pass arguments to a block.  Don't use shebang.  Then use
+shebang set to the same language as the block.  The result should
+be the same."
+  (should (equal
+           (org-test-with-temp-text
+               "#+begin_src bash :cmdline 1 2 3
+echo \"$1\"
+<point>
+#+end_src"
+             (org-babel-execute-src-block))
+           (org-test-with-temp-text
+               "#+begin_src bash :cmdline 1 2 3 :shebang #!/usr/bin/env bash
+echo \"$1\"
+<point>
+#+end_src"
+             (org-babel-execute-src-block)))))
+
+(ert-deftest test-cmdline-alone-and-with-shebang-have-same-result-2 ()
+  "Pass arguments to a block.  Don't use shebang.  Then use
+shebang set to the same language as the block.  The result should
+be the same."
+  (should (equal
+           (org-test-with-temp-text
+               "#+begin_src bash :cmdline \"1 2\" 3
+echo \"$1\"
+<point>
+#+end_src"
+             (org-babel-execute-src-block))
+           (org-test-with-temp-text
+               "#+begin_src bash :cmdline \"1 2\" 3 :shebang #!/usr/bin/env bash
+echo \"$1\"
+<point>
+#+end_src"
+             (org-babel-execute-src-block)))))
+
+
 \f
 ;;; Standard output
 
-- 
2.41.0


[-- Attachment #3: 0002-lisp-ob-shell.el-Add-comments-to-apply-call.patch --]
[-- Type: application/octet-stream, Size: 1664 bytes --]

From e3e71a3478666af4a4a9aa92402cd438aec52b46 Mon Sep 17 00:00:00 2001
From: Matthew Trzcinski <matt@excalamus.com>
Date: Sun, 21 Apr 2024 16:16:59 +0200
Subject: [PATCH 2/3] lisp/ob-shell.el: Add comments to `apply' call

* lisp/ob-shell.el (org-babel-sh-evaluate): Comment the arguments to
`apply'.  These correspond to the arguments of the function it calls.
Eldoc is only able to document the parameters to `apply' and not the
function.
---
 lisp/ob-shell.el | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/lisp/ob-shell.el b/lisp/ob-shell.el
index 35d9e9376..307360e3c 100644
--- a/lisp/ob-shell.el
+++ b/lisp/ob-shell.el
@@ -322,11 +322,11 @@ return the value of the last statement in BODY."
 	      (with-temp-buffer
                 (with-connection-local-variables
                  (apply #'process-file
-                        (if shebang (file-local-name script-file)
-                          shell-file-name)
-		        stdin-file
-                        (current-buffer)
-                        nil
+                        (if shebang (file-local-name script-file) shell-file-name) ; PROGRAM
+		        stdin-file                                                 ; INFILE
+                        (current-buffer)                                           ; BUFFER
+                        nil                                                        ; DISPLAY
+                        ;; ARGS
                         (if shebang (when cmdline (list cmdline))
                           (list shell-command-switch
                                 (concat (file-local-name script-file)  " " cmdline)))))
-- 
2.41.0


[-- Attachment #4: 0003-lisp-ob-shell.el-Fix-cmdline-shebang-inconsistencies.patch --]
[-- Type: application/octet-stream, Size: 1590 bytes --]

From 39e2f5ea53f8f6d86b1e4e07725a9a25ac7326ce Mon Sep 17 00:00:00 2001
From: Matthew Trzcinski <matt@excalamus.com>
Date: Sun, 21 Apr 2024 16:27:25 +0200
Subject: [PATCH 3/3] lisp/ob-shell.el: Fix :cmdline/:shebang inconsistencies

* ob-shell.el (org-babel-sh-evaluate): The reported bug exists because
shebang+cmdline interprets the characters following :cmdline as a
single string (single argument).  Without :shebang, a lone :cmdline
considers the characters as being space delimited (separate
arguments).  This change updates the shebang+cmdline condition so that
results are the same as a lone :cmdline header.

Reported-by: "Max Nikulin" <manikulin@gmail.com>
Link: https://list.orgmode.org/orgmode/ujamo1$5t9$1@ciao.gmane.io/
---
 lisp/ob-shell.el | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lisp/ob-shell.el b/lisp/ob-shell.el
index 307360e3c..d379acdc6 100644
--- a/lisp/ob-shell.el
+++ b/lisp/ob-shell.el
@@ -327,7 +327,8 @@ return the value of the last statement in BODY."
                         (current-buffer)                                           ; BUFFER
                         nil                                                        ; DISPLAY
                         ;; ARGS
-                        (if shebang (when cmdline (list cmdline))
+                        (if shebang
+                            (when cmdline (split-string-and-unquote cmdline))
                           (list shell-command-switch
                                 (concat (file-local-name script-file)  " " cmdline)))))
 		(buffer-string))))
-- 
2.41.0

Re: [PATCH] Re: [BUG] ob-shell: :shebang changes interpretation of :cmdline

[Ihor Radchenko]

[-- Attachment #1: Type: text/plain, Size: 721 bytes --]

Matt <matt@excalamus.com> writes:

> Whether this is a solution, in part, depends on the perennial problem of shell blocks: knowing what's wrong means knowing what's right.
>
> The proposed solution assumes we intend to parse the characters following :cmdline as space delimited and grouped by quotes.  However, AFAICT, the parsing issue makes this solution ambiguous.
>
> Thoughts?

Manually parsing the shell arguments is calling for trouble.
Especially when the arguments involve shell-specific escapes like
:cmdline 1\ 2\ 3

Since escape characters may vary from shell to shell, it is not a good
idea to parse the arguments on Elisp side. We should better leave this
job to the shell.

I propose the attached patch.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-ob-shell-Pass-cmdline-arguments-consistently-regardl.patch --]
[-- Type: text/x-patch, Size: 1953 bytes --]

From e0cf4161b4af05c513ba402ee9625851853c9465 Mon Sep 17 00:00:00 2001
Message-ID: <e0cf4161b4af05c513ba402ee9625851853c9465.1713867979.git.yantar92@posteo.net>
From: Ihor Radchenko <yantar92@posteo.net>
Date: Tue, 23 Apr 2024 13:22:22 +0300
Subject: [PATCH] ob-shell: Pass :cmdline arguments consistently regardless of
 :shebang

* lisp/ob-shell.el (org-babel-sh-evaluate): When invoking script file
generated from the code block, consistently use
<shell-name> -c <script-file> <cmdline-args> command line, even when
:shebang is header argument is provided.  The previous approach with
<script-file> <cmdline-args> call caused differences in how shell
parsed the provided command line arguments.

Reported-by: Max Nikulin <manikulin@gmail.com>
Link: https://orgmode.org/list/18f01342a2f.124ad27612732529.8693431365849276517@excalamus.com
---
 lisp/ob-shell.el | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/lisp/ob-shell.el b/lisp/ob-shell.el
index 35d9e9376..30b3ea322 100644
--- a/lisp/ob-shell.el
+++ b/lisp/ob-shell.el
@@ -322,14 +322,12 @@ (defun org-babel-sh-evaluate (session body &optional params stdin cmdline)
 	      (with-temp-buffer
                 (with-connection-local-variables
                  (apply #'process-file
-                        (if shebang (file-local-name script-file)
-                          shell-file-name)
+                        shell-file-name
 		        stdin-file
                         (current-buffer)
                         nil
-                        (if shebang (when cmdline (list cmdline))
-                          (list shell-command-switch
-                                (concat (file-local-name script-file)  " " cmdline)))))
+                        (list shell-command-switch
+                              (concat (file-local-name script-file)  " " cmdline))))
 		(buffer-string))))
 	   (session			; session evaluation
             (if async
-- 
2.44.0


[-- Attachment #3: Type: text/plain, Size: 224 bytes --]


-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [PATCH] Re: [BUG] ob-shell: :shebang changes interpretation of :cmdline

[Max Nikulin]

On 21/04/2024 22:09, Matt wrote:
> The proposed solution assumes we intend to parse the characters
> following :cmdline as space delimited and grouped by quotes.  However,
> AFAICT, the parsing issue makes this solution ambiguous.

Matt, I am sorry, but I do not agree with your proposal. I do not think 
that `split-string-and-unquote' will solve all issues.

Certainly issues with formatting of output should be treated separately.

I figured out there is at least one more issue. Consider

#+property: header-args:bash :results verbatim
#+begin_src bash :cmdline $LANG :shebang #!/bin/bash
   printf '"%s" ' "$0" "$@"
   printf '\n'
   tr '\000' '\n' </proc/$$/cmdline
#+end_src

#+RESULTS:
: "/tmp/babel-Is56Ki/sh-script-8FluOx" "$LANG"
: /bin/bash
: /tmp/babel-Is56Ki/sh-script-8FluOx
: $LANG

#+begin_src bash :cmdline $LANG
   printf '"%s" ' "$0" "$@"
   printf '\n'
   tr '\000' '\n' </proc/$$/cmdline
#+end_src

#+RESULTS:
: "/tmp/babel-Is56Ki/sh-script-g0cQ7R" "en_GB.UTF-8"
: /usr/bin/bash
: -c
: /tmp/babel-Is56Ki/sh-script-g0cQ7R $LANG

First line is argv as it is represented for a script, next lines are 
exec arguments at lower level (actual executable may be obtained from
"readlink /proc/$$/exe")

Notice that in the former case "$LANG" is passed literally, but in the 
latter it is expanded. I am in favor of dropping `shell-command-switch' 
in the latter case to pass arguments literally in both cases.

I think, it would be more consistent with :var to specify multiple 
arguments using elisp lists
#+header: :cmdline '("first 1" "second 2")

However looking wider, I do not like that :cmdline for ob-shell has 
different meaning than for other languages, see e.g. ob-sql. Only for 
shell this parameter is treated as arguments of a *script*. In other 
cases :cmdline is used to specify arguments of *interpreter* and I think 
ob-shell should follow this convention.

Actually script arguments (and :stdin) might be applied to python and at 
least some other languages, so support of this feature should be moved 
from ob-shell to common org-babel code.

My point:
- header arguments should have as close as possible meaning across 
various languages.
- withing ob-shell variation of ways to execute a script should be 
minimized either some parameters (:cmdline, :shebang, :stdin) are 
specified or not.

Finally a note on tests
> +(ert-deftest test-cmdline-alone-and-with-shebang-have-same-result ()
> +  "Pass arguments to a block.  Don't use shebang.  Then use
> +shebang set to the same language as the block.  The result should
> +be the same."
> +  (should (equal
> +           (org-test-with-temp-text
> +               "#+begin_src bash :cmdline 1 2 3
> +echo \"$1\"
> +<point>
> +#+end_src"
> +             (org-babel-execute-src-block))
> +           (org-test-with-temp-text
> +               "#+begin_src bash :cmdline 1 2 3 :shebang #!/usr/bin/env bash
> +echo \"$1\"
> +<point>
> +#+end_src"
> +             (org-babel-execute-src-block)))))
I believe, that starting point of the discussion is that the results 
should be same and decision what is more correct is the result of the 
discussion. Unit tests should check both cases independently and should 
fix particular treatment of arguments.

Re: [PATCH] Re: [BUG] ob-shell: :shebang changes interpretation of :cmdline

[Max Nikulin]

On 23/04/2024 17:51, Max Nikulin wrote:
> I am in favor of dropping `shell-command-switch' in the latter case to 
> pass arguments literally in both cases.

Dropping "-c" may have side effects. Instead of :shebang, a source block 
may have shebang in the body

#+begin_src bash
#!/bin/bash -e
echo first; false; echo second
#+end_src

This shebang is ignored if the script is executed as
     bash /tmp/script
and respected in the case of
     bash -c /tmp/script
Shebang in the script body may be detected to run it as
     /tmp/script
or
     /bin/bash -e /tmp/script

To avoid interpretation of shell specials in script arguments when "-c" 
is used, it is possible to use a trick
     bash -c /tmp/script ob-shell arg1 arg2 arg3
The -c option adds extra execve() call in comparison to
     /tmp/script arg1 arg2 arg3
Perhaps it may be neglected.

It may be more tricky on Windows where shebangs are likely ignored even 
by bash. However I do not thing ob-shell is working on windows since 
`shell-command-switch' should be /c there instead of -c since default 
shell is cmd.exe.

Re: [PATCH] Re: [BUG] ob-shell: :shebang changes interpretation of :cmdline

[Max Nikulin]

On 23/04/2024 17:28, Ihor Radchenko wrote:
> 
> I propose the attached patch.

> +++ b/lisp/ob-shell.el
> @@ -322,14 +322,12 @@ (defun org-babel-sh-evaluate (session body &optional params stdin cmdline)
>  	      (with-temp-buffer
>                  (with-connection-local-variables
>                   (apply #'process-file
> -                        (if shebang (file-local-name script-file)
> -                          shell-file-name)
> +                        shell-file-name
>  		        stdin-file
>                          (current-buffer)
>                          nil
> -                        (if shebang (when cmdline (list cmdline))
> -                          (list shell-command-switch
> -                                (concat (file-local-name script-file)  " " cmdline)))))
> +                        (list shell-command-switch
> +                              (concat (file-local-name script-file)  " " cmdline))))

Using `shell-command-switch' unconditionally may lead to executing 
/bin/sh instead of shell specified by `shell-file-name' for script files 
having no shebang, see

https://superuser.com/questions/502984/writing-shell-scripts-that-will-run-on-any-shell-using-multiple-shebang-lines
> The kernel refuses to execute such scripts and returns ENOEXEC, so the
> exact behavior depends on the program you run such a script /from/.
> 
> - bash 4.2.39 -- uses itself
> - busybox-ash 1.20.2 -- uses itself
> - dash 0.5.7 -- runs /bin/sh
> - fish 1.23.1 -- complains about ENOEXEC, then blames the wrong file
> - AT&T ksh 93u+2012.08.01 -- uses itself
> - mksh R40f -- runs /bin/sh
> - pdksh 5.2.14 -- runs /bin/sh
> - sh-heirloom 050706 -- uses itself
> - tcsh 6.18.01 -- runs /bin/sh
> - zsh 5.0.0 -- runs /bin/sh
> - cmd.exe 5.1.2600 -- looks at you funny
I am not going to spend time testing current versions.

I believe, multiple arguments should be specified as '(1 a "b c").

With shebang (as header arg or as part of the body) command should be
     /path/to/script [ARGUMENT]...
when there is no shebang
     /shell/executable /path/to/script [ARGUMENT]...

Re: [PATCH] Re: [BUG] ob-shell: :shebang changes interpretation of :cmdline

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

>> +                        shell-file-name
> ...
>> +                        (list shell-command-switch
>> +                              (concat (file-local-name script-file)  " " cmdline))))
>
> Using `shell-command-switch' unconditionally may lead to executing 
> /bin/sh instead of shell specified by `shell-file-name' for script files 
> having no shebang, see
>
> https://superuser.com/questions/502984/writing-shell-scripts-that-will-run-on-any-shell-using-multiple-shebang-lines

Good point.

> I believe, multiple arguments should be specified as '(1 a "b c").

Yes, but we do not, in general, know how to split them.

> With shebang (as header arg or as part of the body) command should be
>      /path/to/script [ARGUMENT]...
> when there is no shebang
>      /shell/executable /path/to/script [ARGUMENT]...

Maybe instead of `process-file' we can simply use `shell-command'?

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [PATCH] Re: [BUG] ob-shell: :shebang changes interpretation of :cmdline

[Max Nikulin]

On 24/04/2024 19:52, Ihor Radchenko wrote:
> Max Nikulin writes:
>> I believe, multiple arguments should be specified as '(1 a "b c").
> 
> Yes, but we do not, in general, know how to split them.

Something should be changed anyway since current behavior is 
inconsistent and so is buggy.

The only difference of script arguments from :var is that just a string 
should be converted to a list having single value. It should be possible 
to specify list of script argument as a reference to a named element 
similar to

#+name: shvar
#+header: :var a='(1 abc "def ghi") :results verbatim
#+begin_src bash
   printf '%s\n' "${a[@]}"
#+end_src

#+name: varval
- 1
- bcd
- list items

#+call: shvar(a=varval)

>> With shebang (as header arg or as part of the body) command should be
>>       /path/to/script [ARGUMENT]...
>> when there is no shebang
>>       /shell/executable /path/to/script [ARGUMENT]...
> 
> Maybe instead of `process-file' we can simply use `shell-command'?

Doesn't `shell-command` call `process-file` with `shell-file-name` and 
`shell-command-switch' under the hood like in your patch?

Re: [PATCH] Re: [BUG] ob-shell: :shebang changes interpretation of :cmdline

[Ihor Radchenko]

Ihor Radchenko <yantar92@posteo.net> writes:

> Max Nikulin <manikulin@gmail.com> writes:
>
>>> +                        shell-file-name
>> ...
>>> +                        (list shell-command-switch
>>> +                              (concat (file-local-name script-file)  " " cmdline))))
>>
>> Using `shell-command-switch' unconditionally may lead to executing 
>> /bin/sh instead of shell specified by `shell-file-name' for script files 
>> having no shebang, see
>>
>> https://superuser.com/questions/502984/writing-shell-scripts-that-will-run-on-any-shell-using-multiple-shebang-lines
>
> Good point.

I am looking at this again, and notice that the problem with
`shell-command-switch' is tangent to the original bug report we are
discussing.

Currently, ob-shell uses two ways to execute the source block:

1. When :shebang header argument is provided,
   <script-file> '<cmdline>'

2. When :shebang is not provided
   <shell-file-name> <shell-command-switch> '<script-file> <cmdline>'

The problem you linked to only manifests for calling script files
_without_ shebang, while the original bug report is about the case when
:shebang is given.

I conclude that your concern, while being valid, is a _different_ bug.
Thus, I do not see it as a blocker for my patch - my patch will fix the
*original bug reported on top of this thread*.

As for the problem with <shell-file-name> <shell-command-switch> <script-file>
when <script-file> does not contain shebang, we can trivially at that
shebang like

(with-temp-file script-file
		(if shebang (insert shebang "\n")
                  (insert "#!" shell-file-name "\n"))
		(when padline (insert "\n"))
		(insert body))

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

[DISCUSSION] The meaning of :cmdline header argument across babel backends (was: [PATCH] Re: [BUG] ob-shell: :shebang changes interpretation of :cmdline)

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

> However looking wider, I do not like that :cmdline for ob-shell has 
> different meaning than for other languages, see e.g. ob-sql. Only for 
> shell this parameter is treated as arguments of a *script*. In other 
> cases :cmdline is used to specify arguments of *interpreter* and I think 
> ob-shell should follow this convention.

Alas, we already have the current state of affairs documented in
https://orgmode.org/worg/org-contrib/babel/languages/ob-doc-shell.html#orge70bc7b

So, no breaking changes.

And shell scripts are not like SQL queries - they often do need to check
arguments. So, the current behaviour is justified, IMHO.

For some languages, only switches to the interpreter make sense (ditaa,
lilypond, plantuml). For others, (bash, python, C++, etc), we may want
to pass switches to the script itself more often.

What might be done is introducing _two_ different header arguments - one
for interpreter switches, and another for script/program switches.

Say, :interpreter-cmdline and :script-cmdline.
Then, we can call the current :cmdline behaviour "dwim" and allow users
to be more explicit if necessary.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [PATCH] Re: [BUG] ob-shell: :shebang changes interpretation of :cmdline

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

> Actually script arguments (and :stdin) might be applied to python and at 
> least some other languages, so support of this feature should be moved 
> from ob-shell to common org-babel code.
>
> My point:
> - header arguments should have as close as possible meaning across 
> various languages.
> - withing ob-shell variation of ways to execute a script should be 
> minimized either some parameters (:cmdline, :shebang, :stdin) are 
> specified or not.

+1
ob-eval is a good candidate to implement such unified handling.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [PATCH] Re: [BUG] ob-shell: :shebang changes interpretation of :cmdline

[Matt]

Thank you both for your thoughtful replies.  

There's a lot to process in this bug.  We've also uncovered, at least, related four bugs.  I think it would help to submit separate bug reports for each of the related issues so that we may discuss them separately, if possible.  Is Woof! the way to do that?  I recall there being some issues with Woof!.  Is there something not in https://tracker.orgmode.org/howto that I should know?

--
Matt Trzcinski
Emacs Org contributor (ob-shell)
Learn more about Org mode at https://orgmode.org
Support Org development at https://liberapay.com/org-mode

Re: [PATCH] Re: [BUG] ob-shell: :shebang changes interpretation of :cmdline

[Ihor Radchenko]

Matt <matt@excalamus.com> writes:

> There's a lot to process in this bug.  We've also uncovered, at least, related four bugs.  I think it would help to submit separate bug reports for each of the related issues so that we may discuss them separately, if possible.  Is Woof! the way to do that?  I recall there being some issues with Woof!.  Is there something not in https://tracker.orgmode.org/howto that I should know?

You can just send emails with [BUG] in them, as usual. Or change email
subject to branch out a new thread, as I did for :cmdline argument discussion.

What you need to know about Woof! now is that it is not working. We
need to wait for Bastien to investigate. (Bastien does not have much
time though)

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [PATCH] Re: [BUG] ob-shell: :shebang changes interpretation of :cmdline

[Max Nikulin]

On 26/04/2024 18:49, Ihor Radchenko wrote:
>>
>>>> +                        shell-file-name
>>> ...
>>>> +                        (list shell-command-switch
>>>> +                              (concat (file-local-name script-file)  " " cmdline))))

>> Max Nikulin writes:
>>> Using `shell-command-switch' unconditionally may lead to executing
>>> /bin/sh instead of shell specified by `shell-file-name' for script files
>>> having no shebang, see
>>>
>>> https://superuser.com/questions/502984/writing-shell-scripts-that-will-run-on-any-shell-using-multiple-shebang-lines
> 
> I conclude that your concern, while being valid, is a _different_ bug.
> Thus, I do not see it as a blocker for my patch - my patch will fix the
> *original bug reported on top of this thread*.

My concern is that your patch trying to fix one bug (I am not convinced 
it is an improvement despite it is a step toward consistency) introduces 
another one that is not currently present in the code.

> (with-temp-file script-file
> 		(if shebang (insert shebang "\n")
>                    (insert "#!" shell-file-name "\n"))
> 		(when padline (insert "\n"))
> 		(insert body))

This code has an issue. Interpretation of relative file names in 
shebangs varies across shells.

If you insist on parsing :cmdline by shell (I do not like it) then you 
may try

<shell-file-name> <shell-command-switch> '<shell-file-name> 
<script-file> <cmdline>'

Re: [DISCUSSION] The meaning of :cmdline header argument across babel backends

[Max Nikulin]

On 26/04/2024 20:09, Ihor Radchenko wrote:
> Max Nikulin writes:
> 
>> However looking wider, I do not like that :cmdline for ob-shell has
>> different meaning than for other languages, see e.g. ob-sql. Only for
>> shell this parameter is treated as arguments of a *script*. In other
>> cases :cmdline is used to specify arguments of *interpreter* and I think
>> ob-shell should follow this convention.
> 
> Alas, we already have the current state of affairs documented in
> https://orgmode.org/worg/org-contrib/babel/languages/ob-doc-shell.html#orge70bc7b
> 
> So, no breaking changes.

It is documented as
" :cmdline <arg_1> ... [arg_n]

Use the :cmdline header arg to pass arguments to a shell command."

However current implementation allows code injection through args, 
including a trivial one

#+header-arg: :results verbatim
#+begin_src sh :cmdline 1 ; touch /tmp/not-an-arg
   printf '%s\n' "$@"
#+end_src

#+RESULTS:
: 1

"touch ..." *are not arguments of the script*. So users should be 
careful to get documented behavior.

> And shell scripts are not like SQL queries - they often do need to check
> arguments. So, the current behaviour is justified, IMHO.

stackoverflow is full of suggestion how to pass arguments to a SQL 
script executed by mysql. Unfortunately it is unsafe and allows 
injection of code. psql (PostgreSQL) allows to pass parameters, however 
it is more like :var than script arguments. So it is true that CLI 
clients for SQL databases do not implement positional parameters.

ARGV is treated in a quite specific way by awk. It may contain file 
names, variable assignments, and might be overwritten in BEGIN block. 
However a close ob-awk header argument is :cmd-line, not :cmdline, so 
inconsistency is even greater.

> What might be done is introducing _two_ different header arguments - one
> for interpreter switches, and another for script/program switches.
> 
> Say, :interpreter-cmdline and :script-cmdline.
> Then, we can call the current :cmdline behaviour "dwim" and allow users
> to be more explicit if necessary.

It is too easy to confuse org-babel, so "dwim" works in simple cases 
only. Independent header arguments make things more clear, I would 
prefer :script-args. The question is whether they should be interpreted 
by shell (flexibility and shooting feet) or more strict syntax `("hello 
world" 1 a) should be used.

Re: [PATCH] Re: [BUG] ob-shell: :shebang changes interpretation of :cmdline

[Max Nikulin]

On 27/04/2024 17:31, Max Nikulin wrote:
> On 26/04/2024 18:49, Ihor Radchenko wrote:
>>>
>>>>> +                        shell-file-name
>>>> ...
>>>>> +                        (list shell-command-switch
>>>>> +                              (concat (file-local-name 
>>>>> script-file)  " " cmdline))))
> 
>>> Max Nikulin writes:
>>>> Using `shell-command-switch' unconditionally may lead to executing
>>>> /bin/sh instead of shell specified by `shell-file-name' for script 
>>>> files
>>>> having no shebang, see
>>>>
>>>> https://superuser.com/questions/502984/writing-shell-scripts-that-will-run-on-any-shell-using-multiple-shebang-lines
>>
>> I conclude that your concern, while being valid, is a _different_ bug.
>> Thus, I do not see it as a blocker for my patch - my patch will fix the
>> *original bug reported on top of this thread*.
> 
> My concern is that your patch trying to fix one bug (I am not convinced 
> it is an improvement despite it is a step toward consistency) introduces 
> another one that is not currently present in the code.

I read current variant of code once more. -c is unconditionally added, 
so your variant just makes the bug apparent. I still do not like behavior of

:cmdline 1 ; touch /tmp/not-an-arg

as I stated in

Re: [DISCUSSION] The meaning of :cmdline header argument across babel 
backends. Sat, 27 Apr 2024 17:53:25 +0700.
https://list.orgmode.org/v0ilf6$34l$1@ciao.gmane.io

Re: [PATCH] Re: [BUG] ob-shell: :shebang changes interpretation of :cmdline

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

>> I conclude that your concern, while being valid, is a _different_ bug.
>> Thus, I do not see it as a blocker for my patch - my patch will fix the
>> *original bug reported on top of this thread*.
>
> My concern is that your patch trying to fix one bug (I am not convinced 
> it is an improvement despite it is a step toward consistency) introduces 
> another one that is not currently present in the code.

May you explain which bug you are referring to?

The bug with bash -c ./script-with-no-shebang described in
https://superuser.com/questions/502984/writing-shell-scripts-that-will-run-on-any-shell-using-multiple-shebang-lines
is already present, and my patch does not change anything about it.

>> (with-temp-file script-file
>> 		(if shebang (insert shebang "\n")
>>                    (insert "#!" shell-file-name "\n"))
>> 		(when padline (insert "\n"))
>> 		(insert body))
>
> This code has an issue. Interpretation of relative file names in 
> shebangs varies across shells.

May you elaborate? Are you concerned that `shell-file-name' may not be
an absolute path?

> If you insist on parsing :cmdline by shell (I do not like it) then you 
> may try
>
> <shell-file-name> <shell-command-switch> '<shell-file-name> 
> <script-file> <cmdline>'

May you explain how it is better?

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [DISCUSSION] The meaning of :cmdline header argument across babel backends

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

> It is documented as
> " :cmdline <arg_1> ... [arg_n]
>
> Use the :cmdline header arg to pass arguments to a shell command."
>
> However current implementation allows code injection through args, 
> including a trivial one
>
> #+header-arg: :results verbatim
> #+begin_src sh :cmdline 1 ; touch /tmp/not-an-arg
>    printf '%s\n' "$@"
> #+end_src
>
> #+RESULTS:
> : 1
>
> "touch ..." *are not arguments of the script*. So users should be 
> careful to get documented behavior.

I do not see any way to address this concern without introducing feature
regression. So, let's keep things as they are and maybe document that
:cmdline ... is passed verbatim as shell command.

>> What might be done is introducing _two_ different header arguments - one
>> for interpreter switches, and another for script/program switches.
>> 
>> Say, :interpreter-cmdline and :script-cmdline.
>> Then, we can call the current :cmdline behaviour "dwim" and allow users
>> to be more explicit if necessary.
>
> It is too easy to confuse org-babel, so "dwim" works in simple cases 
> only. Independent header arguments make things more clear, I would 
> prefer :script-args. The question is whether they should be interpreted 
> by shell (flexibility and shooting feet) or more strict syntax `("hello 
> world" 1 a) should be used.

I like :script-args.
The counterpart should then be :interpreter-args?

The point of "dwim" is mostly to keep backwards-compatibility. We may
discourage :cmdline for non-trivial cases.

More strict syntax with '(<arga> <argb> <argc> ...) is possible for the
new header arguments, not for the old :cmdline where the existing
backends may not be able to understand the list format.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [DISCUSSION] The meaning of :cmdline header argument across babel backends

[Matt]

 ---- On Sat, 27 Apr 2024 12:53:25 +0200  Max Nikulin  wrote --- 
 > On 26/04/2024 20:09, Ihor Radchenko wrote:
 > > Max Nikulin writes:
 > > 
 > >> However looking wider, I do not like that :cmdline for ob-shell has
 > >> different meaning than for other languages, see e.g. ob-sql. Only for
 > >> shell this parameter is treated as arguments of a *script*. In other
 > >> cases :cmdline is used to specify arguments of *interpreter* and I think
 > >> ob-shell should follow this convention.
 > > 
 > > Alas, we already have the current state of affairs documented in
 > > https://orgmode.org/worg/org-contrib/babel/languages/ob-doc-shell.html#orge70bc7b
 > > 
 > > So, no breaking changes.

Both points are valid.

I disagree with one aspect: we shouldn't use Worg as a source of truth.  The argument holds based on historical behavior of :cmdline.  AFAIU, Worg is a wiki which is open, more or less, to anyone.  Worg contents, AFAIU, have not always undergone review.  The manual should be the final authority.  Fortunately, there's nothing in the manual about :cmdline.

 ---- On Mon, 29 Apr 2024 15:33:38 +0200  Ihor Radchenko  wrote --- 

 > I like :script-args.

+ 1

 > The counterpart should then be :interpreter-args?

Are we thinking of implementing these for other languages, beyond ob-shell?  If not, I believe we should consider it.   That may be a path to resolve the issues of consistency and backwards compatibility.

If we're looking at these as general headers, then I don't think "arg" is the correct term here since a switch may not take a value.   For example, the "-r" option for Bash (IIUC).

Quick name ideas that aren't good yet may inspire better ones by inspiring disgust--:switches, :flags, :options, (using an "i" prefix for "interpreter") :iswitches, :iflags, :ioptions

--
Matt Trzcinski
Emacs Org contributor (ob-shell)
Learn more about Org mode at https://orgmode.org
Support Org development at https://liberapay.com/org-mode

Re: [DISCUSSION] The meaning of :cmdline header argument across babel backends

[Ihor Radchenko]

Matt <matt@excalamus.com> writes:

> I disagree with one aspect: we shouldn't use Worg as a source of
> truth. The argument holds based on historical behavior of :cmdline.
> AFAIU, Worg is a wiki which is open, more or less, to anyone. Worg
> contents, AFAIU, have not always undergone review. The manual should
> be the final authority. Fortunately, there's nothing in the manual
> about :cmdline.

For babel backends specifically, WORG is _the_ documentation for the
built-in backends. It is what we will eventually move to the official
manual and it is what we point users to from the manual for now.

The main reasons why relevant WORG pages are not yet in the manual are
(1) not all the backends yet fully support the common parameters we
introduce in the manual (see
https://orgmode.org/worg/org-contrib/babel/languages/lang-compat.html);
(2) nobody got around to actually move things to the manual.

>  ---- On Mon, 29 Apr 2024 15:33:38 +0200  Ihor Radchenko  wrote --- 
>
>  > I like :script-args.
> ...
>  > The counterpart should then be :interpreter-args?
>
> Are we thinking of implementing these for other languages, beyond
> ob-shell?

Yes. The title of this thread has "across babel backends" :)

> If we're looking at these as general headers, then I don't think "arg"
> is the correct term here since a switch may not take a value. For
> example, the "-r" option for Bash (IIUC).

> Quick name ideas that aren't good yet may inspire better ones by
> inspiring disgust--:switches, :flags, :options, (using an "i" prefix
> for "interpreter") :iswitches, :iflags, :ioptions

Emm... but "command line arguments". No?

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [DISCUSSION] The meaning of :cmdline header argument across babel backends

[Matt]

 ---- On Wed, 01 May 2024 20:01:03 +0200  Ihor Radchenko  wrote --- 
 > Matt matt@excalamus.com> writes:
 > 
 > > I disagree with one aspect: we shouldn't use Worg as a source of
 > > truth. The argument holds based on historical behavior of :cmdline.
 > > AFAIU, Worg is a wiki which is open, more or less, to anyone. Worg
 > > contents, AFAIU, have not always undergone review. The manual should
 > > be the final authority. Fortunately, there's nothing in the manual
 > > about :cmdline.
 > 
 > For babel backends specifically, WORG is _the_ documentation for the
 > built-in backends. It is what we will eventually move to the official
 > manual and it is what we point users to from the manual for now.

Okay, I didn't know we made an explicit reference.

  > (2) nobody got around to actually move things to the manual.

I'd be happy to help with this.  Discussion for another thread, though.

 > > Are we thinking of implementing these for other languages, beyond
 > > ob-shell?
 > 
 > Yes. The title of this thread has "across babel backends" :)

:)

 > > If we're looking at these as general headers, then I don't think "arg"
 > > is the correct term here since a switch may not take a value. For
 > > example, the "-r" option for Bash (IIUC).
 > 
 > > Quick name ideas that aren't good yet may inspire better ones by
 > > inspiring disgust--:switches, :flags, :options, (using an "i" prefix
 > > for "interpreter") :iswitches, :iflags, :ioptions
 > 
 > Emm... but "command line arguments". No?
 
I was thinking it'd be strange to have an "argument to an argument."  However, the Bash man says things like "non-option arguments".  It wasn't my intent to have an argument argument argument.

Since we're considering this for all babel backends, "interpreter-args" wouldn't describe gcc or other compilers.  What about :command-args, :command-args, :cmd-args?  "Command" is the only thing I can think of that describes all of the languages, other than maybe "binary" or "executable".  Maybe we could simply use ":args"?  Maybe ":meta-args" since gcc or bash is meta to the script/artifact?

--
Matt Trzcinski
Emacs Org contributor (ob-shell)
Learn more about Org mode at https://orgmode.org
Support Org development at https://liberapay.com/org-mode

Re: [DISCUSSION] The meaning of :cmdline header argument across babel backends

[Ihor Radchenko]

Matt <matt@excalamus.com> writes:

> Since we're considering this for all babel backends, "interpreter-args" wouldn't describe gcc or other compilers.  What about :command-args, :command-args, :cmd-args?  "Command" is the only thing I can think of that describes all of the languages, other than maybe "binary" or "executable".  Maybe we could simply use ":args"?  Maybe ":meta-args" since gcc or bash is meta to the script/artifact?

I am not a big fan. It might be easily confused with :script-args.

Maybe instead use two different extra arguments:
:interpreter-args / :compiler-args ?

Then, also :script-args / :program-args

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [DISCUSSION] The meaning of :cmdline header argument across babel backends

[Matt]

 ---- On Fri, 03 May 2024 14:11:56 +0200  Ihor Radchenko  wrote --- 
  > Maybe instead use two different extra arguments:
 > :interpreter-args / :compiler-args ?
 > 
 > Then, also :script-args / :program-args
 
I'm okay with these.  I can start on a patch for :script-args and :program-args within ob-shell.

I anticipate :interpreter-args will be more involved since it will interact with 'shell-command-switch' and it'd be nice to have it also work with sessions.
 
--
Matt Trzcinski
Emacs Org contributor (ob-shell)
Learn more about Org mode at https://orgmode.org
Support Org development at https://liberapay.com/org-mode

Re: [DISCUSSION] The meaning of :cmdline header argument across babel backends

[Max Nikulin]

On 21/05/2024 01:01, Matt wrote:
> I'm okay with these.  I can start on a patch for :script-args and
> :program-args within ob-shell.

Frankly speaking your plan is not clear for me. My special concern is 
DWIM behavior
https://list.orgmode.org/874jbkcmyg.fsf@localhost
(Ihor Radchenko Mon, 29 Apr 2024 13:33:59 +0000)
and

      #+begin_src sh :script-args 1 ; touch /tmp/not-an-arg

if you are going to pass it literally to "sh -c" then it is 
:script-cmdline rather than :script-args.

I expect a way to explicitly specify if it is a single argument or 
multiple ones

     #+begin_src sh :script-args '("a b c")

vs.

     #+begin_src sh :script-args '("a" "b" "c")

preferably passed as separate arguments of process-files or at least 
properly escaped.

As to literal command line, taking into account stripped outer quotes 
issue, I do not like requirement to quote characters for shells. Even 
splitting string into arguments using `read' might be better, but there 
are still enough issues.

Besides interpreters, there is may be a stack of "launchers" like 
toolbox in the case of applications installed as isolated flatpak/snap 
packages:

Florin Boariu to emacs-orgmode. org-ditaa woes. Thu, 19 Oct 2023 
12:59:59 +0200.
https://list.orgmode.org/ZTEML8zWrB6kQflk@toolbox

Re: [DISCUSSION] The meaning of :cmdline header argument across babel backends

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

> Frankly speaking your plan is not clear for me. My special concern is 
> DWIM behavior
> https://list.orgmode.org/874jbkcmyg.fsf@localhost
> (Ihor Radchenko Mon, 29 Apr 2024 13:33:59 +0000)
> and
>
>       #+begin_src sh :script-args 1 ; touch /tmp/not-an-arg
>
> if you are going to pass it literally to "sh -c" then it is 
> :script-cmdline rather than :script-args.

Your pathological example is not how we encourage these header arguments
to be used. Their intended purpose is serving as arguments, not to
complete the cmdline. Anything else is implementation details we may or
may not change in future. It need not be reflected in the header
argument names. So, I stand on the :*-args names.

> I expect a way to explicitly specify if it is a single argument or 
> multiple ones
>
>      #+begin_src sh :script-args '("a b c")
>
> vs.
>
>      #+begin_src sh :script-args '("a" "b" "c")

Max, I believe that we discussed this. This problem has nothing to do
with introducing new header arguments. It is just a question of how we
pass these header arguments to scripts. Your concerns must not stop Matt
from working on the proposed patch.

> As to literal command line, taking into account stripped outer quotes 
> issue, I do not like requirement to quote characters for shells. Even 
> splitting string into arguments using `read' might be better, but there 
> are still enough issues.

I fail to see how this relates to new header arguments.

> Besides interpreters, there is may be a stack of "launchers" like 
> toolbox in the case of applications installed as isolated flatpak/snap 
> packages:
>
> Florin Boariu to emacs-orgmode. org-ditaa woes. Thu, 19 Oct 2023 
> 12:59:59 +0200.
> https://list.orgmode.org/ZTEML8zWrB6kQflk@toolbox

This is also out of scope of the discussed header arguments. Using
custom interpreter (flapak-spawn ... bash instead of default bash) has
nothing to do with script/interpreter arguments. We may (and should)
discuss that question separately.

Let's not raise all the possible related concerns at once, unless they
cannot be resolved in future because of the discussed patch. Otherwise,
it is almost impossible to make _incremental_ progress.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [PATCH] Re: [BUG] ob-shell: :shebang changes interpretation of :cmdline

[Ihor Radchenko]

Ihor Radchenko <yantar92@posteo.net> writes:

> I am looking at this again, and notice that the problem with
> `shell-command-switch' is tangent to the original bug report we are
> discussing.
> ...
> I conclude that your concern, while being valid, is a _different_ bug.
> Thus, I do not see it as a blocker for my patch - my patch will fix the
> *original bug reported on top of this thread*.

Applied.
Fixed, on main.
This is the original bug report and reproducer that is fixed.
Other bugs discussed in this thread have to be handled separately.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=1c7b3ed26

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

[BUG] ob-shell may use /bin/sh instead of the specified shell when :cmdline is provided (was: [PATCH] Re: [BUG] ob-shell: :shebang changes interpretation of :cmdline)

[Ihor Radchenko]

Ihor Radchenko <yantar92@posteo.net> writes:

> As for the problem with <shell-file-name> <shell-command-switch> <script-file>
> when <script-file> does not contain shebang, we can trivially at that
> shebang like
>
> (with-temp-file script-file
> 		(if shebang (insert shebang "\n")
>                   (insert "#!" shell-file-name "\n"))
> 		(when padline (insert "\n"))
> 		(insert body))

I am branching off this bug into a new thread.

Reproducer:

#+begin_src dash
  echo This must be empty in dash: "$RANDOM"
#+end_src

#+RESULTS:
: This must be empty in dash:



#+begin_src dash :cmdline 1 2 3
  echo This must be empty in dash: "$RANDOM"
#+end_src

#+RESULTS:
: This must be empty in dash: 25561

The second code block uses shell as evidenced by $RANDOM variable being
initialized. This only happens when :cmdline header argument is
provided.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [BUG] ob-shell may use /bin/sh instead of the specified shell when :cmdline is provided (was: [PATCH] Re: [BUG] ob-shell: :shebang changes interpretation of :cmdline)

[Ihor Radchenko]

Ihor Radchenko <yantar92@posteo.net> writes:

> Reproducer:
> ...

Fixed, on main.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=b64dbd838

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [DISCUSSION] The meaning of :cmdline header argument across babel backends

[Ihor Radchenko]

Ihor Radchenko <yantar92@posteo.net> writes:

>> #+header-arg: :results verbatim
>> #+begin_src sh :cmdline 1 ; touch /tmp/not-an-arg
>>    printf '%s\n' "$@"
>> #+end_src
>>
>> #+RESULTS:
>> : 1
>>
>> "touch ..." *are not arguments of the script*. So users should be 
>> careful to get documented behavior.
>
> I do not see any way to address this concern without introducing feature
> regression. So, let's keep things as they are and maybe document that
> :cmdline ... is passed verbatim as shell command.

https://git.sr.ht/~bzg/worg/commit/1c6390fc

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>