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

[Pádraig Brady]

On 24/04/18 15:57, Assaf Gordon wrote:
> 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 ==

Better, but a pity it's inconsistent.

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

Well according to the FreeBSD docs it should output 'xAAAx ==' in both cases.
I'd have a slight preference for being consistent there rather than being
bug for bug compat with FreeBSD here.

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

Sure, but the main point is that for shebang lines you only get
one argument, so if you typed the first coreutils form in a shebang
you would get effectively:

 $ src/env '-v -S A=B uname'
 src/env: invalid option -- ' '

That subtlety would be documented/avoided by explicitly
specifying it in the separated form below:

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

I still think the above two forms should be used in the synopsis
for the env man page (propagated from --help).
It also makes it clear that the position of -S is important.

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

I was thinking that the explanation of -S in usage() would say something like:

  -S, --split-string=S  process and split S into separate arguments
                          used to pass multiple arguments on shebang lines

cheers,
Pádraig