From fa14d0b449451208ec4aed1d4a35aa993d65e398 Mon Sep 17 00:00:00 2001
From: Elliot Saba <staticfloat@gmail.com>
Date: Thu, 7 Jan 2021 08:55:50 -0800
Subject: [PATCH] `addenv()`, by default, should inherit from `ENV` (#39100)

The spirit of `addenv()` is to be non-destructive, however because a
`Cmd` object with an `env` member that is set to `nothing` (the default)
inherts from the current environment when it is run.  This means that
the following, rather surprising behavior holds true on current master:

```
julia> ENV["FOO"] = "foo"
       run(`/bin/bash -c "echo \$FOO \$BAR"`)
       run(addenv(`/bin/bash -c "echo \$FOO \$BAR"`, "BAR" => "bar"))
foo
bar
```

This PR adds an `inherit` flag to `addenv()` to allow keeping this
behavior if it is actually desired (this might make sense if you are
constructing `.env` blocks over a series of `addenv()` calls and you
don't want to have to special-case your first operation as a `setenv()`
rather than an `addenv()`.
---
 base/cmd.jl   | 21 +++++++++++++--------
 test/spawn.jl | 22 ++++++++++++++++++++++
 2 files changed, 35 insertions(+), 8 deletions(-)

diff --git a/base/cmd.jl b/base/cmd.jl
index 4fc7c65094166..e1f93413a2fbc 100644
--- a/base/cmd.jl
+++ b/base/cmd.jl
@@ -246,14 +246,19 @@ setenv(cmd::Cmd, env::Pair{<:AbstractString}...; dir="") =
 setenv(cmd::Cmd; dir="") = Cmd(cmd; dir=dir)
 
 """
-    addenv(command::Cmd, env...)
+    addenv(command::Cmd, env...; inherit::Bool = true)
 
 Merge new environment mappings into the given `Cmd` object, returning a new `Cmd` object.
-Duplicate keys are replaced.
+Duplicate keys are replaced.  If `command` does not contain any environment values set already,
+it inherits the current environment at time of `addenv()` call if `inherit` is `true`.
 """
-function addenv(cmd::Cmd, env::Dict)
+function addenv(cmd::Cmd, env::Dict; inherit::Bool = true)
     new_env = Dict{String,String}()
-    if cmd.env !== nothing
+    if cmd.env === nothing
+        if inherit
+            merge!(new_env, ENV)
+        end
+    else
         for (k, v) in split.(cmd.env, "=")
             new_env[string(k)::String] = string(v)::String
         end
@@ -264,12 +269,12 @@ function addenv(cmd::Cmd, env::Dict)
     return setenv(cmd, new_env)
 end
 
-function addenv(cmd::Cmd, pairs::Pair{<:AbstractString}...)
-    return addenv(cmd, Dict(k => v for (k, v) in pairs))
+function addenv(cmd::Cmd, pairs::Pair{<:AbstractString}...; inherit::Bool = true)
+    return addenv(cmd, Dict(k => v for (k, v) in pairs); inherit)
 end
 
-function addenv(cmd::Cmd, env::Vector{<:AbstractString})
-    return addenv(cmd, Dict(k => v for (k, v) in split.(env, "=")))
+function addenv(cmd::Cmd, env::Vector{<:AbstractString}; inherit::Bool = true)
+    return addenv(cmd, Dict(k => v for (k, v) in split.(env, "=")); inherit)
 end
 
 (&)(left::AbstractCmd, right::AbstractCmd) = AndCmds(left, right)
diff --git a/test/spawn.jl b/test/spawn.jl
index 236b9f280d799..4f826b05df93d 100644
--- a/test/spawn.jl
+++ b/test/spawn.jl
@@ -711,6 +711,28 @@ end
     @test strip(String(read(cmd))) == "bar bar"
     cmd = addenv(cmd, ["FOO=baz"])
     @test strip(String(read(cmd))) == "baz bar"
+
+    # Test that `addenv()` works properly with `inherit`
+    withenv("FOO" => "foo") do
+        cmd = Cmd(`$shcmd -c "echo \$FOO \$BAR"`)
+        @test strip(String(read(cmd))) == "foo"
+
+        cmd2 = addenv(cmd, "BAR" => "bar"; inherit=false)
+        @test strip(String(read(cmd2))) == "bar"
+
+        cmd2 = addenv(cmd, "BAR" => "bar"; inherit=true)
+        @test strip(String(read(cmd2))) == "foo bar"
+
+        # Changing the environment doesn't effect the command,
+        # because it was baked in at `addenv()` time
+        withenv("FOO" => "baz") do
+            @test strip(String(read(cmd2))) == "foo bar"
+        end
+
+        # Even with inheritance, `addenv()` dominates:
+        cmd2 = addenv(cmd, "FOO" => "foo2", "BAR" => "bar"; inherit=true)
+        @test strip(String(read(cmd2))) == "foo2 bar"
+    end
 end