Re: env: add -S option (split string for shebang lines in scripts)

[Assaf Gordon]

Hello Pádraig,

Thank you for the quick review.

On Mon, Apr 23, 2018 at 08:38:27PM -0700, Pádraig Brady wrote:
> On 22/04/18 03:25, Assaf Gordon wrote:
> > 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).
> > 
> 
> Please add an extra space after "--split-string=S " [...] 
> There is no need to specify "inline" on functions generally [..]
> The error() for --debug may need translations [...]
> In scan_varname(), I think we should we restrict to: [...]

Will fix these and send update soon.

> I noticed some tweaks for the test which are attached.

Thank you, I'll add them as well.
 
> ------ 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?

Good catch!
This slipped by me, as I did test that "-i"
and "-C" do work the same inside and outside "-S".

> 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.

It is a bug, but slightly more intricate then first seems.

Here's what FreeBSD does:

  $ TERM=AAA env -uTERM -S'sh -c "echo x${TERM}x =\$TERM="'
  xx ==
  $ TERM=AAA env -S'-uTERM sh -c "echo x${TERM}x =\$TERM="'
  xAAAx ==

Note that "${TERM}" is expanded by 'env' *before* executing sh,
while '\$TERM' is passed to 'sh' and expanded by it.

This is the current (wrong) implementation:

  $ TERM=AAA ./src/env -uTERM -S'sh -c "echo x${TERM}x =\$TERM="'
  xAAAx =AAA=
  $ TERM=AAA ./src/env -S'-uTERM sh -c "echo x${TERM}x =\$TERM="'
  xAAAx ==

I am going to assume that FreeBSD's behaviour is the "correct" one,
whether or not it makes sense, because it has been so since FreeBSD 6 (ca. 
2005).

And so I will update the patch to behave like FreeBSD, unless there
are objections or different opinions.

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

In this case it's actually the same as FreeBSD:
"-v" affects only what comes after it:

FreeBSD (needs multiple -v to get max verbosity):

  $ env -vvv -S"A=B uname"
  #env verbosity now at 2
  #env verbosity now at 3
  #env  split -S: 'A=B uname'
  #env      into: 'A=B'
  #env          & 'uname'
  #env setenv:    A=B
  #env executing: uname
  #env    arg[0]= 'uname'
  FreeBSD

  $ env -S"-vvv A=B uname"
  #env verbosity now at 2
  #env verbosity now at 3
  #env setenv:    A=B
  #env executing: uname
  #env    arg[0]= 'uname'
  FreeBSD


Coreutils:

  $ ./src/env -v -S"A=B uname"
  ./src/env: split -S:  ‘A=B uname’
  ./src/env:  into:    ‘A=B’
  ./src/env:      &    ‘uname’
  ./src/env: setenv:   ‘A=B’
  ./src/env: executing: ‘uname’
  ./src/env:    arg[0]= ‘uname’
  Linux

  $ ./src/env -S"-v A=B uname"
  ./src/env: setenv:   ‘A=B’
  ./src/env: executing: ‘uname’
  ./src/env:    arg[0]= ‘uname’
  Linux


In both implementation, if "-v" appears before "-S",
the debug messages will include the "-S". Otherwise,
they will include only what's after it.

> 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]...]'

Based on the above, do we still want to explicitly show it in usage(),
or is it sufficient to mention the subtlely in the documentation?

> 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).

Agreed. Should this go in usage(), or man-page, or only the texinfo ?


regards,
 - assaf