[CHEF-2994][WINDOWS] Fixed Utils.which()

- Factored out Utils.should_run_under_cmd? (simple case)

3 files changed, 58 insertions, 10 deletions

diff --git a/lib/mixlib/shellout/windows.rb b/lib/mixlib/shellout/windows.rb
index 942f377..1703236 100644
--- a/lib/mixlib/shellout/windows.rb
+++ b/lib/mixlib/shellout/windows.rb
@@ -165,6 +165,8 @@ module Mixlib
       IS_BATCH_FILE = /\.bat"?$|\.cmd"?$/i
 
       def command_to_run(command)
+        return _run_under_cmd(command) if Utils.should_run_under_cmd?(command)
+
         candidate = candidate_executable_for_command(command)
 
         # Don't do searching for empty commands.  Let it fail when it runs.
@@ -174,18 +176,27 @@ module Mixlib
         exe = Utils.find_executable(candidate)
         exe = Utils.which(unquoted_executable_path(command)) if exe.nil? && exe !~ /[\\\/]/
 
+        # Batch files MUST use cmd; and if we couldn't find the command we're looking for,
+        # we assume it must be a cmd builtin.
         if exe.nil? || exe =~ IS_BATCH_FILE
-          # Batch files MUST use cmd; and if we couldn't find the command we're looking for, we assume it must be a cmd builtin.
-          #
-          # cmd does not parse multiple quotes well unless the whole thing is wrapped up in quotes.
-          # https://github.com/opscode/mixlib-shellout/pull/2#issuecomment-4837859
-          # http://ss64.com/nt/syntax-esc.html
-          [ ENV['COMSPEC'], "cmd /c \"#{command}\"" ]
+          _run_under_cmd(command)
         else
-          [ exe, command ]
+          _run_directly(command, exe)
         end
       end
 
+
+      # cmd does not parse multiple quotes well unless the whole thing is wrapped up in quotes.
+      # https://github.com/opscode/mixlib-shellout/pull/2#issuecomment-4837859
+      # http://ss64.com/nt/syntax-esc.html
+      def _run_under_cmd(command)
+        [ ENV['COMSPEC'], "cmd /c \"#{command}\"" ]
+      end
+
+      def _run_directly(command, exe)
+        [ exe, command ]
+      end
+
       def unquoted_executable_path(command)
         command[0,command.index(/\s/) || command.length]
       end
@@ -217,6 +228,13 @@ module Mixlib
       end
 
       module Utils
+        # api: semi-private
+        # If there are special characters parsable by cmd.exe (such as file redirection), then
+        # this method should return true.
+        def self.should_run_under_cmd?(command)
+          !!(command =~ /[%&|<>{}@^]/)
+        end
+
         def self.pathext
           @pathext ||= ENV['PATHEXT'] ? ENV['PATHEXT'].split(';') + [''] : ['']
         end
@@ -224,10 +242,8 @@ module Mixlib
         # which() mimicks the Unix which command
         # FIXME: it is not working
         def self.which(cmd)
-          return nil # noop
-
           ENV['PATH'].split(File::PATH_SEPARATOR).each do |path|
-            exe = find_executable("#{path}/${cmd}") # This looks like it got disabled
+            exe = find_executable("#{path}/#{cmd}")
             return exe if exe
           end
           return nil
diff --git a/spec/mixlib/shellout/windows_spec.rb b/spec/mixlib/shellout/windows_spec.rb
index 1cc6ff3..4e6b83f 100644
--- a/spec/mixlib/shellout/windows_spec.rb
+++ b/spec/mixlib/shellout/windows_spec.rb
@@ -2,6 +2,36 @@ require 'spec_helper'
 
 describe 'Mixlib::ShellOut::Windows', :windows_only => true do
 
+  describe 'Utils' do
+    describe '.should_run_under_cmd?' do
+      subject { Mixlib::ShellOut::Windows::Utils.should_run_under_cmd?(command) }
+
+      def self.with_command(_command, &example)
+        context "with command: #{_command}" do
+          let(:command) { _command }
+          it(&example)
+        end
+      end
+
+      with_command(%q{ruby -e 'prints "foobar"'}) { should_not be_true }
+
+      # https://github.com/opscode/mixlib-shellout/pull/2#issuecomment-4825574
+      with_command(%q{"C:\Program Files (x86)\Microsoft SDKs\Windows\v7.0A\Bin\NETFX 4.0 Tools\gacutil.exe" /i "C:\Program Files (x86)\NUnit 2.6\bin\framework\nunit.framework.dll"}) { should_not be_true }
+
+      with_command(%q{ruby -e 'exit 1' | ruby -e 'exit 0'}) { should be_true }
+      with_command(%q{ruby -e 'exit 1' > out.txt}) { should be_true }
+      with_command(%q{ruby -e 'exit 1' > out.txt 2>&1}) { should be_true }
+      with_command(%q{ruby -e 'exit 1' < in.txt}) { should be_true }
+      with_command(%q{ruby -e 'exit 1' || ruby -e 'exit 0'}) { should be_true }
+      with_command(%q{ruby -e 'exit 1' && ruby -e 'exit 0'}) { should be_true }
+      with_command(%q{@echo TRUE}) { should be_true }
+      with_command(%q{echo %PATH%}) { should be_true }
+
+      # TODO: It would be awesome if it can detect quoted special characters
+      with_command(%q{echo "ruby -e 'exit 1' || ruby -e 'exit 0'"}) { should be_true }
+    end
+  end
+
   # Caveat: Private API methods are subject to change without notice.
   # Monkeypatch at your own risk.
   context 'for private API methods' do
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index 6f400d9..cb6d017 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -8,6 +8,8 @@ require 'timeout'
 
 require 'ap'
 
+WATCH = lambda { |x| ap x } unless defined?(WATCH)
+
 # Load everything from spec/support
 # Do not change the gsub.
 Dir["spec/support/**/*.rb"].map { |f| f.gsub(%r{.rb$}, '') }.each { |f| require f }