Properly modify environment option based on ENV

2 files changed, 138 insertions, 37 deletions

diff --git a/lib/chef/mixin/shell_out.rb b/lib/chef/mixin/shell_out.rb
index 3f8b37a199..cbdfffeeb2 100644
--- a/lib/chef/mixin/shell_out.rb
+++ b/lib/chef/mixin/shell_out.rb
@@ -45,13 +45,31 @@ class Chef
       end
 
       def shell_out_with_systems_locale(*command_args)
-        if command_args.last.is_a?(Hash)
-          command_args.last[:environment] ||= {}
-          command_args.last[:environment]['LC_ALL'] ||= ENV['LC_ALL']
-          shell_out(*command_args)
-        else
-          shell_out(*command_args, :environment => {'LC_ALL' => ENV['LC_ALL']})
+        args = command_args.dup
+        unless ENV['LC_ALL'].nil?
+          if args.last.is_a?(Hash)
+            options = args.last
+            # Get the environment option and set environment['LC_ALL'] if not
+            # present.
+            if options.has_key?(:environment)
+              options_env = options[:environment]
+            elsif options.has_key?(:env)
+              options_env = options[:env]
+            else
+              options[:environment] = {}
+              options_env = options[:environment]
+            end
+
+            unless options_env.nil? || options_env.has_key?('LC_ALL')
+              options_env['LC_ALL'] = ENV['LC_ALL']
+            end
+          else
+            # Add the environment option
+            args << { :environment => { 'LC_ALL' => ENV['LC_ALL'] } }
+          end
         end
+
+        shell_out(*args)
       end
 
       DEPRECATED_OPTIONS =
diff --git a/spec/unit/mixin/shell_out_spec.rb b/spec/unit/mixin/shell_out_spec.rb
index 00725b235c..552face932 100644
--- a/spec/unit/mixin/shell_out_spec.rb
+++ b/spec/unit/mixin/shell_out_spec.rb
@@ -107,61 +107,144 @@ describe Chef::Mixin::ShellOut do
   end
 
   describe "#shell_out_with_systems_locale" do
+    before(:each) do
+      @original_env = ENV.to_hash
+      ENV.clear
+    end
+
+    after(:each) do
+      ENV.clear
+      ENV.update(@original_env)
+    end
+
     let(:shell_out) { Chef::Mixin::ShellOut }
+    let(:cmd) { "echo '#{rand(1000)}'" }
 
     describe "when the last argument is a Hash" do
-      let(:cmd) { "echo '#{rand(1000)}'" }
+      describe "when ENV['LC_ALL'] is nil" do
+        let(:options) { { :environment => { 'HOME' => '/Users/morty' },
+                          :user => 'morty' } }
+
+        it "should not modify options" do
+          shell_out.should_receive(:shell_out).with(
+            cmd,
+            { :environment => { 'HOME' => '/Users/morty' },
+              :user => 'morty'
+            }
+          ).and_return(true)
+
+          shell_out.shell_out_with_systems_locale(cmd, options)
+        end
+      end
+
+      describe "when ENV['LC_ALL'] is not nil" do
+        before do
+          ENV['LC_ALL'] = 'C'
+        end
 
-      describe "and environment is a key" do
-        let(:options) { { :environment => environment } }
+        describe "when environment is present" do
+          let(:options) { { :environment => environment } }
 
-        describe "and 'LC_ALL' is an :environment key" do
-          let(:environment) { { 'LC_ALL' => 'C' } }
+          describe "when environment is set to nil" do
+            let(:environment) { nil }
 
-          it "should not modify :environment['LC_ALL']" do
-            shell_out.should_receive(:shell_out).with(cmd, options).and_return(true)
-            shell_out.shell_out_with_systems_locale(cmd, options)
+            it "should not modify the environment option" do
+              shell_out.should_receive(:shell_out).with(
+                cmd,
+                { :environment => nil }
+              ).and_return(true)
+
+              shell_out.shell_out_with_systems_locale(cmd, options)
+            end
+          end
+
+          describe "when environment['LC_ALL'] is present" do
+            let(:environment) { { 'LC_ALL' => lc_all } }
+
+            describe "when set to nil" do
+              let(:lc_all) { nil }
+
+              it "should not be modified" do
+                shell_out.should_receive(:shell_out).with(
+                  cmd,
+                  { :environment => { 'LC_ALL' => nil } }
+                ).and_return(true)
+
+                shell_out.shell_out_with_systems_locale(cmd, options)
+              end
+            end
+
+            describe "when set to non-nil" do
+              let(:lc_all) { 'POSIX' }
+
+              it "should not be modified" do
+                shell_out.should_receive(:shell_out).with(
+                  cmd,
+                  { :environment => { 'LC_ALL' => 'POSIX' } }
+                ).and_return(true)
+
+                shell_out.shell_out_with_systems_locale(cmd, options)
+              end
+            end
+          end
+
+          describe "when environment['LC_ALL'] is not present" do
+            let(:environment) { { 'HOME' => '/Users/morty' } }
+
+            it "should set environment['LC_ALL'] to ENV['LC_ALL']" do
+              shell_out.should_receive(:shell_out).with(
+                cmd,
+                { :environment => {
+                  'LC_ALL' => ENV['LC_ALL'],
+                  'HOME' => '/Users/morty' }
+                }
+              ).and_return(true)
+
+              shell_out.shell_out_with_systems_locale(cmd, options)
+            end
           end
         end
 
-        describe "and 'LC_ALL' is not an :environment key" do
-          let(:environment) { { 'USER' => 'morty' } }
+        describe "when environment is not present" do
+          let(:options) { { :user => 'morty' } }
 
-          it "should set :environment['LC_ALL'] => nil" do
+          it "should set environment['LC_ALL'] to ENV['LC_ALL']" do
             shell_out.should_receive(:shell_out).with(
               cmd,
-              { :environment => { 'USER'   => 'morty',
-                                  'LC_ALL' => ENV['LC_ALL'] } }
+              { :user => 'morty',
+                :environment => { 'LC_ALL' => ENV['LC_ALL'] }
+              }
             ).and_return(true)
+
             shell_out.shell_out_with_systems_locale(cmd, options)
           end
         end
       end
+    end
 
-      describe "and :environment is not a key" do
-        let(:options) { { :user => 'morty' } }
+    describe "when the last argument is not a Hash" do
+      describe "when ENV['LC_ALL'] is nil" do
+        it "should not add options" do
+          shell_out.should_receive(:shell_out).with(cmd).and_return(true)
+          shell_out.shell_out_with_systems_locale(cmd)
+        end
+      end
+
+      describe "when ENV['LC_ALL'] is not nil" do
+        before do
+          ENV['LC_ALL'] = 'C'
+        end
 
-        it "should add :environment => { 'LC_ALL' => nil }" do
+        it "should add the environment option with environment['LC_ALL']" do
           shell_out.should_receive(:shell_out).with(
             cmd,
-            { :user => 'morty',
-              :environment => { 'LC_ALL' => ENV['LC_ALL'] } }
+            { :environment => { 'LC_ALL' => ENV['LC_ALL'] } }
           ).and_return(true)
-          shell_out.shell_out_with_systems_locale(cmd, options)
-        end
-      end
-    end
 
-    describe "when the last argument is not a Hash" do
-      let(:cmd) { "echo '#{rand(1000)}'" }
-
-      it "should add :environment => {'LC_ALL' => nil} to the command args" do
-        shell_out.should_receive(:shell_out).with(
-          cmd,
-          { :environment => { 'LC_ALL' => ENV['LC_ALL'] } }
-        ).and_return(true)
-        shell_out.shell_out_with_systems_locale(cmd)
+          shell_out.shell_out_with_systems_locale(cmd)
+        end
       end
     end
   end
+
 end