Set environment option's 'LC_ALL' to nil if not present in option.

3 files changed, 70 insertions, 130 deletions

diff --git a/lib/chef/mixin/shell_out.rb b/lib/chef/mixin/shell_out.rb
index cbdfffeeb2..4e462a17be 100644
--- a/lib/chef/mixin/shell_out.rb
+++ b/lib/chef/mixin/shell_out.rb
@@ -44,29 +44,26 @@ class Chef
         cmd
       end
 
+      # environment['LC_ALL'] should be nil or what the user specified
       def shell_out_with_systems_locale(*command_args)
         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
+        if args.last.is_a?(Hash)
+          options = args.last
+          if options.has_key?(:environment)
+            options_env = options[:environment]
+          elsif options.has_key?(:env)
+            options_env = options[:env]
           else
-            # Add the environment option
-            args << { :environment => { 'LC_ALL' => ENV['LC_ALL'] } }
+            # No environment option provided, make one
+            options[:environment] = {}
+            options_env = options[:environment]
           end
+
+          unless options_env.has_key?('LC_ALL')
+            options_env['LC_ALL'] = nil
+          end
+        else
+          args << { :environment => { 'LC_ALL' => nil } }
         end
 
         shell_out(*args)
diff --git a/spec/functional/mixin/shell_out_spec.rb b/spec/functional/mixin/shell_out_spec.rb
index 7e57de2a0a..55cd3c7c61 100644
--- a/spec/functional/mixin/shell_out_spec.rb
+++ b/spec/functional/mixin/shell_out_spec.rb
@@ -29,15 +29,7 @@ describe Chef::Mixin::ShellOut do
           shell_out_with_systems_locale('echo $LC_ALL')
         end
 
-        # From mixlib-shellout/lib/mixlib/shell_out.rb:
-        #
-        # * +environment+: a Hash of environment variables to set before the command
-        # is run. By default, the environment will *always* be set to 'LC_ALL' => 'C'
-        # to prevent issues with multibyte characters in Ruby 1.8. To avoid this,
-        # use :environment => nil for *no* extra environment settings, or
-        # :environment => {'LC_ALL'=>nil, ...} to set other environment settings
-        # without changing the locale.
-        cmd.stdout.chomp.should eq 'C'
+        cmd.stdout.chomp.should eq ENV['LC_ALL'].to_s
       end
     end
 
diff --git a/spec/unit/mixin/shell_out_spec.rb b/spec/unit/mixin/shell_out_spec.rb
index 552face932..1f85ec6bf1 100644
--- a/spec/unit/mixin/shell_out_spec.rb
+++ b/spec/unit/mixin/shell_out_spec.rb
@@ -121,130 +121,81 @@ describe Chef::Mixin::ShellOut do
     let(:cmd) { "echo '#{rand(1000)}'" }
 
     describe "when the last argument is a Hash" do
-      describe "when ENV['LC_ALL'] is nil" do
-        let(:options) { { :environment => { 'HOME' => '/Users/morty' },
-                          :user => 'morty' } }
+      describe "and environment is an option" do
+        it "should not change environment['LC_ALL'] when set to nil" do
+          options = { :environment => { 'LC_ALL' => nil } }
+          shell_out.should_receive(:shell_out).with(cmd, options).and_return(true)
+          shell_out.shell_out_with_systems_locale(cmd, options)
+        end
+
+        it "should not change environment['LC_ALL'] when set to non-nil" do
+          options = { :environment => { 'LC_ALL' => 'en_US.UTF-8' } }
+          shell_out.should_receive(:shell_out).with(cmd, options).and_return(true)
+          shell_out.shell_out_with_systems_locale(cmd, options)
+        end
 
-        it "should not modify options" do
+        it "should set environment['LC_ALL'] to nil when 'LC_ALL' not present" do
+          options = { :environment => { 'HOME' => '/Users/morty' } }
           shell_out.should_receive(:shell_out).with(
             cmd,
-            { :environment => { 'HOME' => '/Users/morty' },
-              :user => 'morty'
+            { :environment => {
+                'HOME' => '/Users/morty',
+                'LC_ALL' => nil }
             }
           ).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 "when environment is present" do
-          let(:options) { { :environment => environment } }
-
-          describe "when environment is set to nil" do
-            let(:environment) { nil }
-
-            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
+      describe "and env is an option" do
+        it "should not change env when set to nil" do
+          options = { :env => { 'LC_ALL' => nil } }
+          shell_out.should_receive(:shell_out).with(cmd, options).and_return(true)
+          shell_out.shell_out_with_systems_locale(cmd, options)
         end
 
-        describe "when environment is not present" do
-          let(:options) { { :user => 'morty' } }
-
-          it "should set environment['LC_ALL'] to ENV['LC_ALL']" do
-            shell_out.should_receive(:shell_out).with(
-              cmd,
-              { :user => 'morty',
-                :environment => { 'LC_ALL' => ENV['LC_ALL'] }
-              }
-            ).and_return(true)
-
-            shell_out.shell_out_with_systems_locale(cmd, options)
-          end
+        it "should not change env when set to non-nil" do
+          options = { :env => { 'LC_ALL' => 'en_US.UTF-8'}}
+          shell_out.should_receive(:shell_out).with(cmd, options).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
-      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)
+        it "should set env['LC_ALL'] to nil when 'LC_ALL' not present" do
+          options = { :env => { 'HOME' => '/Users/morty' } }
+          shell_out.should_receive(:shell_out).with(
+            cmd,
+            { :env => {
+                'HOME' => '/Users/morty',
+                'LC_ALL' => nil }
+            }
+          ).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
-
-        it "should add the environment option with environment['LC_ALL']" do
+      describe "and no env/environment option is present" do
+        it "should add environment option and set environment['LC_ALL'] to nil" do
+          options = { :user => 'morty' }
           shell_out.should_receive(:shell_out).with(
             cmd,
-            { :environment => { 'LC_ALL' => ENV['LC_ALL'] } }
+            { :environment => { 'LC_ALL' => nil },
+              :user => 'morty'
+            }
           ).and_return(true)
-
-          shell_out.shell_out_with_systems_locale(cmd)
+          shell_out.shell_out_with_systems_locale(cmd, options)
         end
       end
     end
+
+    describe "when the last argument is not a Hash" do
+      it "should add environment options and set environment['LC_ALL'] to nil" do
+        shell_out.should_receive(:shell_out).with(
+          cmd,
+          { :environment => { 'LC_ALL' => nil } }
+        ).and_return(true)
+        shell_out.shell_out_with_systems_locale(cmd)
+      end
+    end
   end
 
 end