On 22/04/18 03:25, Assaf Gordon wrote:
> Hello,
> 
> Following recent discussion about shebang lines [1][2],
> attached is a patch to add "env -S", with the same syntax as freebsd's 
> env(1).
> 
> [1] https://lists.gnu.org/r/coreutils/2017-05/msg00020.html
> [2] https://lists.gnu.org/r/bug-coreutils/2018-04/msg00031.html
> 
> Also included a small patch adding "--debug/-v" option.
> 
> It includes tests with close to %100 coverage, but no documentation yet.
> If you think this is a good addition, I'll update the patch with 
> documentation.

This is quite useful functionality to have in a standard tool.
Thanks a lot for implementing it.

I'd rather be consistent with other utils and just use the long --debug form.
Oh I see, the FreeBSD version supports -v. So in that case I agree with
supporting the short option also.

Please add an extra space after "--split-string=S " so that the man page
will be formatted appropriately.

There is no need to specify "inline" on functions generally
(unless defining in headers). Best let the compiler sort it out.

The error() for --debug may need translations or use
a debug() wrapper to avoid the syntax check

In scan_varname(), I think we should we restrict to:
s/isalpha/c_isalpha/
s/isalnum/c_isalnum/

I see the test script identifies the case where the OS does split
and handles spaces in the src dir. Excellent.

I noticed some tweaks for the test which are attached.

------ some general notes -----

I see that if one wants to use other options in combo with -S
they need to be part of the -S option. I.E. after -S.
It would be good to illustrate that in the docs,
or perhaps issue a warning if -S comes after other options?

For example it looks buggy that -u TERM
is just ignored in this example:

  src/env -v -u TERM -S sh -c "echo \$TERM"

We should at least warn about this case, but maybe error out.

Another gotcha is that portably specifying -v
with -S (i.e. with only one option) requires:
  src/env '-vS sh -c "echo \$TERM"'

So I would document two forms in the man page/--help like:

  env        [OPTION]... [-] [NAME=VALUE]... [COMMAND [ARG]...]
  env -[v]S '[OPTION]... [-] [NAME=VALUE]... [COMMAND [ARG]...]'

and make it clear usage() that -S is only needed with shebang lines.
The quotes above are important to document too,
as otherwise consistent processing (like interpolation etc.) wouldn't
be done on operating systems that did split shebangs
(or when used in a standard command invocation).

thanks!
Pádraig
diff --git a/tests/misc/env-S-script.sh b/tests/misc/env-S-script.sh
old mode 100644
new mode 100755
index 0377191..d164be5
--- a/tests/misc/env-S-script.sh
+++ b/tests/misc/env-S-script.sh
@@ -37,12 +37,11 @@ chmod a+x env_test || framework_failure_
 # support multiple arguments on the shebang line.
 # Ignoring the absolute paths, the script is:
 #     #!env printf x%sx\n A B
-printf "#!$dir/env $dir/printf "'x%%sx\\n A B\n' > env_bad || framework_failure_
+printf "#!$dir/env $dir/printf "'x%%sx\\n A B\n' > env_bad ||
+  framework_failure_
 chmod a+x env_bad || framework_failure_
-returns_ 127 ./env_bad \
-    || warn_ "WARNING: OS natively accepts multiple arguments on shebang line"
-
-
+returns_ 127 ./env_bad ||
+  warn_ 'Note: OS natively accepts multiple arguments on shebang line'
 
 # env should execute 'printf' with 7 parameters:
 # 'x%sx\n', 'A', 'B' from the "-S" argument,
@@ -51,7 +50,7 @@ returns_ 127 ./env_bad \
 #     #!env -S printf x%sx\n A B
 printf "#!$dir/env -S $dir/printf "'x%%sx\\n A B\n' > env1 || framework_failure_
 chmod a+x env1 || framework_failure_
-cat<<EOF>exp1 || framework_failure_
+cat<<\EOF>exp1 || framework_failure_
 xAx
 xBx
 x./env1x
@@ -67,10 +66,10 @@ compare exp1 out1 || fail=1
 # 'A B' and not two paramaters 'A','B'.
 # Ignoring the absolute paths, the script is:
 #     #!env -S printf x%sx\n "A B"
-printf "#!$dir/env -S $dir/printf "'x%%sx\\n "A B"\n' > env2 \
-    || framework_failure_
+printf "#!$dir/env -S $dir/printf "'x%%sx\\n "A B"\n' > env2 ||
+  framework_failure_
 chmod a+x env2 || framework_failure_
-cat<<EOF>exp2 || framework_failure_
+cat<<\EOF>exp2 || framework_failure_
 xA Bx
 x./env2x
 EOF
@@ -81,9 +80,10 @@ compare exp2 out2 || fail=1
 # backslash-underscore instead of spaces.
 # Ignoring the absolute paths, the script is:
 #     #!env -Sprintf\_x%sx\n\_Y
-printf "#!$dir/env -S$dir/printf"'\\_x%%sx\\n\\_Y\n' > env3 || framework_failure_
+printf "#!$dir/env -S$dir/printf"'\\_x%%sx\\n\\_Y\n' > env3 ||
+  framework_failure_
 chmod a+x env3 || framework_failure_
-cat<<EOF>exp3 || framework_failure_
+cat<<\EOF>exp3 || framework_failure_
 xYx
 x./env3x
 xWx
@@ -99,7 +99,7 @@ compare exp3 out3 || fail=1
 printf "#!$dir/env -S$dir/printf"' x%%sx\\n A#B #C D\n' > env4 \
     || framework_failure_
 chmod a+x env4 || framework_failure_
-cat<<EOF>exp4 || framework_failure_
+cat<<\EOF>exp4 || framework_failure_
 xA#Bx
 x./env4x
 xZx
@@ -116,7 +116,7 @@ compare exp4 out4 || fail=1
 { printf "#!$dir/env -S perl -w -T\n" ;
   printf 'print "hello\\n";\n' ; } > env5 || framework_failure_
 chmod a+x env5 || framework_failure_
-cat<<EOF>exp5 || framework_failure_
+cat<<\EOF>exp5 || framework_failure_
 hello
 EOF
 ./env5 > out5 || fail=1
@@ -138,7 +138,4 @@ printf "env6" > exp6 || framework_failure_
 compare exp6 out6 || fail=1
 
 
-
-
-
 Exit $fail

Reply via email to