Auto merge of #4298 - njam:preserve-path, r=indirect

Refactor path/environment preserver, make it always preserve the PATH

Closes #4251

@segiddins @indirect @RochesterinNYC wdyt?

12 files changed, 176 insertions, 142 deletions

diff --git a/lib/bundler.rb b/lib/bundler.rb
index b0f166040f..ca896094e0 100644
--- a/lib/bundler.rb
+++ b/lib/bundler.rb
@@ -3,7 +3,7 @@ require "fileutils"
 require "pathname"
 require "rbconfig"
 require "thread"
-require "bundler/path_preserver"
+require "bundler/environment_preserver"
 require "bundler/gem_remote_fetcher"
 require "bundler/rubygems_ext"
 require "bundler/rubygems_integration"
@@ -13,9 +13,9 @@ require "bundler/current_ruby"
 require "bundler/errors"
 
 module Bundler
-  PathPreserver.preserve_path_in_environment("PATH", ENV)
-  PathPreserver.preserve_path_in_environment("GEM_PATH", ENV)
-  ORIGINAL_ENV = ENV.to_hash
+  environment_preserver = EnvironmentPreserver.new(ENV, %w(PATH GEM_PATH))
+  ORIGINAL_ENV = environment_preserver.restore
+  ENV.replace(environment_preserver.backup)
   SUDO_MUTEX = Mutex.new
 
   autoload :Definition,             "bundler/definition"
diff --git a/lib/bundler/environment_preserver.rb b/lib/bundler/environment_preserver.rb
new file mode 100644
index 0000000000..0dd2c1ed61
--- /dev/null
+++ b/lib/bundler/environment_preserver.rb
@@ -0,0 +1,35 @@
+# frozen_string_literal: true
+module Bundler
+  class EnvironmentPreserver
+    # @param env [ENV]
+    # @param keys [Array<String>]
+    def initialize(env, keys)
+      @original = env.to_hash
+      @keys = keys
+      @prefix = "BUNDLE_ORIG_"
+    end
+
+    # @return [Hash]
+    def backup
+      env = restore.clone
+      @keys.each do |key|
+        value = env[key]
+        env[@prefix + key] = value unless value.nil? || value.empty?
+      end
+      env
+    end
+
+    # @return [Hash]
+    def restore
+      env = @original.clone
+      @keys.each do |key|
+        value_original = env[@prefix + key]
+        unless value_original.nil? || value_original.empty?
+          env[key] = value_original
+          env.delete(@prefix + key)
+        end
+      end
+      env
+    end
+  end
+end
diff --git a/lib/bundler/path_preserver.rb b/lib/bundler/path_preserver.rb
deleted file mode 100644
index f5701cd67b..0000000000
--- a/lib/bundler/path_preserver.rb
+++ /dev/null
@@ -1,12 +0,0 @@
-# frozen_string_literal: true
-module Bundler
-  module PathPreserver
-    def self.preserve_path_in_environment(env_var, env)
-      original_env_var      = "_ORIGINAL_#{env_var}"
-      original_path         = env[original_env_var]
-      path                  = env[env_var]
-      env[original_env_var] = path if original_path.nil? || original_path.empty?
-      env[env_var]          = original_path if path.nil? || path.empty?
-    end
-  end
-end
diff --git a/spec/bundler/cli_spec.rb b/spec/bundler/cli_spec.rb
index b48af7c81a..19c6f8277b 100644
--- a/spec/bundler/cli_spec.rb
+++ b/spec/bundler/cli_spec.rb
@@ -18,7 +18,7 @@ describe "bundle executable" do
       f.puts "#!/usr/bin/env ruby\nputs 'Hello, world'\n"
     end
 
-    with_path_as(tmp) do
+    with_path_added(tmp) do
       bundle "testtasks"
     end
 
diff --git a/spec/bundler/environment_preserver_spec.rb b/spec/bundler/environment_preserver_spec.rb
new file mode 100644
index 0000000000..b3e7019a75
--- /dev/null
+++ b/spec/bundler/environment_preserver_spec.rb
@@ -0,0 +1,68 @@
+# frozen_string_literal: true
+require "spec_helper"
+
+describe Bundler::EnvironmentPreserver do
+  let(:preserver) { described_class.new(env, ["foo"]) }
+
+  describe "#backup" do
+    let(:env) { { "foo" => "my-foo", "bar" => "my-bar" } }
+    subject { preserver.backup }
+
+    it "should create backup entries" do
+      expect(subject["BUNDLE_ORIG_foo"]).to eq("my-foo")
+    end
+
+    it "should keep the original entry" do
+      expect(subject["foo"]).to eq("my-foo")
+    end
+
+    it "should not create backup entries for unspecified keys" do
+      expect(subject.key?("BUNDLE_ORIG_bar")).to eq(false)
+    end
+
+    it "should not affect the original env" do
+      subject
+      expect(env.keys.sort).to eq(%w(bar foo))
+    end
+
+    context "when a key is empty" do
+      let(:env) { { "foo" => "" } }
+
+      it "should not create backup entries" do
+        expect(subject.key?("BUNDLE_ORIG_foo")).to eq(false)
+      end
+    end
+  end
+
+  describe "#restore" do
+    subject { preserver.restore }
+
+    context "when an original key is set" do
+      let(:env) { { "foo" => "my-foo", "BUNDLE_ORIG_foo" => "orig-foo" } }
+
+      it "should restore the original value" do
+        expect(subject["foo"]).to eq("orig-foo")
+      end
+
+      it "should delete the backup value" do
+        expect(subject.key?("BUNDLE_ORIG_foo")).to eq(false)
+      end
+    end
+
+    context "when no original key is set" do
+      let(:env) { { "foo" => "my-foo" } }
+
+      it "should keep the current value" do
+        expect(subject["foo"]).to eq("my-foo")
+      end
+    end
+
+    context "when the original key is empty" do
+      let(:env) { { "foo" => "my-foo", "BUNDLE_ORIG_foo" => "" } }
+
+      it "should keep the current value" do
+        expect(subject["foo"]).to eq("my-foo")
+      end
+    end
+  end
+end
diff --git a/spec/bundler/path_preserver_spec.rb b/spec/bundler/path_preserver_spec.rb
deleted file mode 100644
index 78fda9aa40..0000000000
--- a/spec/bundler/path_preserver_spec.rb
+++ /dev/null
@@ -1,54 +0,0 @@
-# frozen_string_literal: true
-require "spec_helper"
-
-describe Bundler::PathPreserver do
-  describe "#preserve_path_in_environment" do
-    subject { described_class.preserve_path_in_environment(env_var, env) }
-
-    context "env_var is PATH" do
-      let(:env_var)       { "PATH" }
-      let(:path)          { "/path/here" }
-      let(:original_path) { "/original/path/here" }
-
-      context "when _ORIGINAL_PATH in env is nil" do
-        let(:env)  { { "ORIGINAL_PATH" => nil, "PATH" => path } }
-
-        it "should set _ORIGINAL_PATH of env to value of PATH from env" do
-          expect(env["_ORIGINAL_PATH"]).to be_nil
-          subject
-          expect(env["_ORIGINAL_PATH"]).to eq("/path/here")
-        end
-      end
-
-      context "when original_path in env is empty" do
-        let(:env)  { { "_ORIGINAL_PATH" => "", "PATH" => path } }
-
-        it "should set _ORIGINAL_PATH of env to value of PATH from env" do
-          expect(env["_ORIGINAL_PATH"]).to be_empty
-          subject
-          expect(env["_ORIGINAL_PATH"]).to eq("/path/here")
-        end
-      end
-
-      context "when path in env is nil" do
-        let(:env)  { { "_ORIGINAL_PATH" => original_path, "PATH" => nil } }
-
-        it "should set PATH of env to value of _ORIGINAL_PATH from env" do
-          expect(env["PATH"]).to be_nil
-          subject
-          expect(env["PATH"]).to eq("/original/path/here")
-        end
-      end
-
-      context "when path in env is empty" do
-        let(:env)  { { "_ORIGINAL_PATH" => original_path, "PATH" => "" } }
-
-        it "should set PATH of env to value of _ORIGINAL_PATH from env" do
-          expect(env["PATH"]).to be_empty
-          subject
-          expect(env["PATH"]).to eq("/original/path/here")
-        end
-      end
-    end
-  end
-end
diff --git a/spec/commands/exec_spec.rb b/spec/commands/exec_spec.rb
index ee766cdb5f..16e853bb1d 100644
--- a/spec/commands/exec_spec.rb
+++ b/spec/commands/exec_spec.rb
@@ -103,8 +103,9 @@ describe "bundle exec" do
       f.puts "echo foobar"
     end
     File.chmod(0744, "--verbose")
-    ENV["PATH"] = "."
-    bundle "exec -- --verbose"
+    with_path_as(".") do
+      bundle "exec -- --verbose"
+    end
     expect(out).to eq("foobar")
   end
 
@@ -358,7 +359,7 @@ describe "bundle exec" do
       Bundler.rubygems.extend(Monkey)
       G
       bundle "install --deployment"
-      bundle "exec ruby -e '`bundler -v`; puts $?.success?'"
+      bundle "exec ruby -e '`../../exe/bundler -v`; puts $?.success?'"
       expect(out).to match("true")
     end
   end
diff --git a/spec/commands/help_spec.rb b/spec/commands/help_spec.rb
index 704b99a0e8..4e35ba3e6a 100644
--- a/spec/commands/help_spec.rb
+++ b/spec/commands/help_spec.rb
@@ -13,23 +13,23 @@ describe "bundle help" do
   end
 
   it "uses mann when available" do
-    fake_man!
-
-    bundle "help gemfile"
+    with_fake_man do
+      bundle "help gemfile"
+    end
     expect(out).to eq(%(["#{root}/lib/bundler/man/gemfile.5"]))
   end
 
   it "prefixes bundle commands with bundle- when finding the groff files" do
-    fake_man!
-
-    bundle "help install"
+    with_fake_man do
+      bundle "help install"
+    end
     expect(out).to eq(%(["#{root}/lib/bundler/man/bundle-install"]))
   end
 
   it "simply outputs the txt file when there is no man on the path" do
-    kill_path!
-
-    bundle "help install", :expect_err => true
+    with_path_as("") do
+      bundle "help install", :expect_err => true
+    end
     expect(out).to match(/BUNDLE-INSTALL/)
   end
 
@@ -43,7 +43,7 @@ describe "bundle help" do
       f.puts "#!/usr/bin/env ruby\nputs ARGV.join(' ')\n"
     end
 
-    with_path_as(tmp) do
+    with_path_added(tmp) do
       bundle "help testtasks"
     end
 
@@ -52,37 +52,37 @@ describe "bundle help" do
   end
 
   it "is called when the --help flag is used after the command" do
-    fake_man!
-
-    bundle "install --help"
+    with_fake_man do
+      bundle "install --help"
+    end
     expect(out).to eq(%(["#{root}/lib/bundler/man/bundle-install"]))
   end
 
   it "is called when the --help flag is used before the command" do
-    fake_man!
-
-    bundle "--help install"
+    with_fake_man do
+      bundle "--help install"
+    end
     expect(out).to eq(%(["#{root}/lib/bundler/man/bundle-install"]))
   end
 
   it "is called when the -h flag is used before the command" do
-    fake_man!
-
-    bundle "-h install"
+    with_fake_man do
+      bundle "-h install"
+    end
     expect(out).to eq(%(["#{root}/lib/bundler/man/bundle-install"]))
   end
 
   it "is called when the -h flag is used after the command" do
-    fake_man!
-
-    bundle "install -h"
+    with_fake_man do
+      bundle "install -h"
+    end
     expect(out).to eq(%(["#{root}/lib/bundler/man/bundle-install"]))
   end
 
   it "has helpful output when using --help flag for a non-existent command" do
-    fake_man!
-
-    bundle "instill -h", :expect_err => true
+    with_fake_man do
+      bundle "instill -h", :expect_err => true
+    end
     expect(err).to include('Could not find command "instill -h --no-color".')
   end
 end
diff --git a/spec/install/gemfile/git_spec.rb b/spec/install/gemfile/git_spec.rb
index ecd6462540..c5c70c2c12 100644
--- a/spec/install/gemfile/git_spec.rb
+++ b/spec/install/gemfile/git_spec.rb
@@ -977,7 +977,9 @@ describe "bundle install with git sources" do
         end
       G
 
-      bundle "update", :env => { "PATH" => "", "_ORIGINAL_PATH" => "" }
+      with_path_as("") do
+        bundle "update"
+      end
       expect(out).to include("You need to install git to be able to use gems from git repositories. For help installing git, please refer to GitHub's tutorial at https://help.github.com/articles/set-up-git")
     end
 
diff --git a/spec/runtime/with_clean_env_spec.rb b/spec/runtime/with_clean_env_spec.rb
index 92607efc76..6bb0d21e78 100644
--- a/spec/runtime/with_clean_env_spec.rb
+++ b/spec/runtime/with_clean_env_spec.rb
@@ -89,6 +89,21 @@ describe "Bundler.with_env helpers" do
         expect(`echo $BUNDLE_PATH`.strip).to eq("./Gemfile")
       end
     end
+
+    it "should preserve the PATH environment variable" do
+      gemfile ""
+      bundle "install --path vendor/bundle"
+
+      code = "Bundler.with_original_env do;" \
+             "  print ENV['PATH'];" \
+             "end"
+
+      path = `getconf PATH`.strip
+      with_path_as(path) do
+        result = bundle("exec ruby -e #{code.inspect}")
+        expect(result).to eq(path)
+      end
+    end
   end
 
   describe "Bundler.clean_system" do
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index bea836aae6..c9359012fd 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -80,13 +80,8 @@ RSpec.configure do |config|
   config.filter_run :focused => true unless ENV["CI"]
   config.run_all_when_everything_filtered = true
 
-  original_wd            = Dir.pwd
-  original_path          = ENV["PATH"]
-  original_gem_home      = ENV["GEM_HOME"]
-  original_ruby_opt      = ENV["RUBYOPT"]
-  original_ruby_lib      = ENV["RUBYLIB"]
-  original_git_dir       = ENV["GIT_DIR"]
-  original_git_work_tree = ENV["GIT_WORK_TREE"]
+  original_wd  = Dir.pwd
+  original_env = ENV.to_hash
 
   def pending_jruby_shebang_fix
     pending "JRuby executables do not have a proper shebang" if RUBY_PLATFORM == "java"
@@ -110,20 +105,6 @@ RSpec.configure do |config|
     puts @out if defined?(@out) && example.exception
 
     Dir.chdir(original_wd)
-    # Reset ENV
-    ENV["PATH"]                  = original_path
-    ENV["GEM_HOME"]              = original_gem_home
-    ENV["GEM_PATH"]              = original_gem_home
-    ENV["RUBYOPT"]               = original_ruby_opt
-    ENV["RUBYLIB"]               = original_ruby_lib
-    ENV["GIT_DIR"]               = original_git_dir
-    ENV["GIT_WORK_TREE"]         = original_git_work_tree
-    ENV["BUNDLE_PATH"]           = nil
-    ENV["BUNDLE_GEMFILE"]        = nil
-    ENV["BUNDLE_FROZEN"]         = nil
-    ENV["BUNDLE_APP_CONFIG"]     = nil
-    ENV["BUNDLER_TEST"]          = nil
-    ENV["BUNDLER_SPEC_PLATFORM"] = nil
-    ENV["BUNDLER_SPEC_VERSION"]  = nil
+    ENV.replace(original_env)
   end
 end
diff --git a/spec/support/helpers.rb b/spec/support/helpers.rb
index 8bcae2a45a..60c1c6f42f 100644
--- a/spec/support/helpers.rb
+++ b/spec/support/helpers.rb
@@ -230,22 +230,28 @@ module Spec
     alias_method :install_gem, :install_gems
 
     def with_gem_path_as(path)
-      gem_home = ENV["GEM_HOME"]
-      gem_path = ENV["GEM_PATH"]
+      backup = ENV.to_hash
       ENV["GEM_HOME"] = path.to_s
       ENV["GEM_PATH"] = path.to_s
+      ENV["BUNDLE_ORIG_GEM_PATH"] = nil
       yield
     ensure
-      ENV["GEM_HOME"] = gem_home
-      ENV["GEM_PATH"] = gem_path
+      ENV.replace(backup)
     end
 
     def with_path_as(path)
-      old_path = ENV["PATH"]
-      ENV["PATH"] = "#{path}:#{ENV["PATH"]}"
+      backup = ENV.to_hash
+      ENV["PATH"] = path.to_s
+      ENV["BUNDLE_ORIG_PATH"] = nil
       yield
     ensure
-      ENV["PATH"] = old_path
+      ENV.replace(backup)
+    end
+
+    def with_path_added(path)
+      with_path_as(path.to_s + ":" + ENV["PATH"]) do
+        yield
+      end
     end
 
     def break_git!
@@ -257,17 +263,12 @@ module Spec
       ENV["PATH"] = "#{tmp("broken_path")}:#{ENV["PATH"]}"
     end
 
-    def fake_man!
+    def with_fake_man
       FileUtils.mkdir_p(tmp("fake_man"))
       File.open(tmp("fake_man/man"), "w", 0755) do |f|
         f.puts "#!/usr/bin/env ruby\nputs ARGV.inspect\n"
       end
-
-      ENV["PATH"] = "#{tmp("fake_man")}:#{ENV["PATH"]}"
-    end
-
-    def kill_path!
-      ENV["PATH"] = ""
+      with_path_added(tmp("fake_man")) { yield }
     end
 
     def system_gems(*gems)
@@ -278,20 +279,17 @@ module Spec
 
       Gem.clear_paths
 
-      gem_home = ENV["GEM_HOME"]
-      gem_path = ENV["GEM_PATH"]
-      path = ENV["PATH"]
+      env_backup = ENV.to_hash
       ENV["GEM_HOME"] = system_gem_path.to_s
       ENV["GEM_PATH"] = system_gem_path.to_s
+      ENV["BUNDLE_ORIG_GEM_PATH"] = nil
 
       install_gems(*gems)
       return unless block_given?
       begin
         yield
       ensure
-        ENV["GEM_HOME"] = gem_home
-        ENV["GEM_PATH"] = gem_path
-        ENV["PATH"] = path
+        ENV.replace(env_backup)
       end
     end