The new std.process is ready for review

[Lars T. Kyllingstad]

On Monday, 25 February 2013 at 00:15:21 UTC, Vladimir Panteleev 
wrote:
> On Sunday, 24 February 2013 at 14:43:50 UTC, Lars T. 
> Kyllingstad wrote:
>> [snip]
>
> Hi Lars,
>
> First of all, about environment. I think the old behavior makes 
> more sense.
>
> I think you had a good point about making it behave like an 
> associative array. I would expect using opIndex with an 
> inexisting key to throw. Subtle deviations of behavior for 
> types that generally behave like well-known types can introduce 
> latent bugs.
>
> The danger is even more potent in the case of environment 
> variables, as those are often used for constructing 
> command-lines and such. If attempting to get the value of an 
> inexisting variable now returns null, which is used to build a 
> command line, unexpected things can happen.
>
> For example, let's say that you're writing a program for 
> analyzing malware, which expects $BINDUMP to be set to the path 
> of some analysis tool. So it runs environment["BINDUMP"] ~ 
> args[1] - however, if BINDUMP is unset, the program runs the 
> malware executable itself.
>
> For another example, here's this classic catastrophic bug in 
> shell scripts:
>
> rm -rf $FOO/$BAR
>
> What happens if $FOO and $BAR are unset?
>
> One thing that I think is missing from the environment object 
> is opIn_r. Implementing opIn_r would allow users to more safely 
> explicitly check if a variable is set or not, and is more 
> readable than environment.get("FOO", null).
>
> And of course, there's the issue of people migrating code from 
> the old module version to the new one: if they relied on the 
> old behavior, the code can break in unexpected ways after the 
> migration.
>
> What are your specific reasons for changing environment's 
> behavior?

My reasons were what I said in my other post:  In the time I have 
been using the 'environment' API -- that is, for 2 1/2 years (I 
checked) -- I don't think there is a *single* time when I've 
chosen environment[var] over environment.get(var, null).

The thing about the process environment, as opposed to an 
associative array inside your own program, is that you can never 
be certain which variables are defined and which aren't.  This 
means that you will almost *always* have to check whether a 
variable exists before using it, thus rendering opIndex() pretty 
much useless for most cases.

Furthermore, I really don't think it is too much to expect that a 
user of a systems language such as D checks the return values of 
functions that may return a 'null' value.

However, I also think that quick'n dirty scripting is an 
extremely compelling use case for D, and in that case, your point 
is well taken.  (I also get your arguments about backwards 
compatibility and not deviating from the AA interface, but that 
was what did it for me.)

I am now on the fence about this.


> Speaking of shells, I noticed you hardcode cmd.exe in 
> std.process2. That's another bug, it should look at the COMSPEC 
> variable.

Thanks, I didn't know that.  On POSIX, the -c switch is standard, 
and works on most, if not all, shells.  Can we assume that /C is 
equally standardised on Windows shells?


> Also, about the shell function, I noticed this recent bug 
> report:
> http://d.puremagic.com/issues/show_bug.cgi?id=9444
>
> Maybe it somehow makes the transition to the new function 
> easier? :)

Hehe. :)


> If not, since you're adamant about not changing the name, can 
> we overload the function (e.g. make the new one return some 
> results in "out" parameters), and deprecate the original 
> overload?
>
> Finally, I'd just like to sum up that we seem to have two 
> decisions on the scales: somehow solving the API 
> incompatibilities, or introducing the new version as an 
> entirely new module. The latter is a mess we really don't want 
> to get into, so it'd need to be justified, and IMHO the 
> incompatibilities don't seem to be as severe and unresolvable 
> to warrant that mess.

Let us see where the discussion about command quoting ends up.  
It is going to have an impact on most of the API, shell() 
included.

Lars