Replace output_of_command with shell_out! in subversion provider

output_of_command doesn't work properly on Windows (see chef/chef#1533)

4 files changed, 72 insertions, 38 deletions

diff --git a/lib/chef/provider/subversion.rb b/lib/chef/provider/subversion.rb
index 5f36483c32..e3e3d5158a 100644
--- a/lib/chef/provider/subversion.rb
+++ b/lib/chef/provider/subversion.rb
@@ -130,8 +130,8 @@ class Chef
             @new_resource.revision
           else
             command = scm(:info, @new_resource.repository, @new_resource.svn_info_args, authentication, "-r#{@new_resource.revision}")
-            status, svn_info, error_message = output_of_command(command, run_options)
-            handle_command_failures(status, "STDOUT: #{svn_info}\nSTDERR: #{error_message}")
+            svn_info = shell_out!(command, run_options(:cwd => cwd, :returns => [0,1])).stdout
+
             extract_revision_info(svn_info)
           end
         end
@@ -142,11 +142,8 @@ class Chef
       def find_current_revision
         return nil unless ::File.exist?(::File.join(@new_resource.destination, ".svn"))
         command = scm(:info)
-        status, svn_info, error_message = output_of_command(command, run_options(:cwd => cwd))
+        svn_info = shell_out!(command, run_options(:cwd => cwd, :returns => [0,1])).stdout
 
-        unless [0,1].include?(status.exitstatus)
-          handle_command_failures(status, "STDOUT: #{svn_info}\nSTDERR: #{error_message}")
-        end
         extract_revision_info(svn_info)
       end
 
@@ -180,6 +177,7 @@ class Chef
           attrs
         end
         rev = (repo_attrs['Last Changed Rev'] || repo_attrs['Revision'])
+        rev.strip! if rev
         raise "Could not parse `svn info` data: #{svn_info}" if repo_attrs.empty?
         Chef::Log.debug "#{@new_resource} resolved revision #{@new_resource.revision} to #{rev}"
         rev
@@ -197,12 +195,20 @@ class Chef
       end
 
       def scm(*args)
-        ['svn', *args].compact.join(" ")
+        binary = svn_binary
+        binary = "\"#{binary}\"" if binary =~ /\s/
+        [binary, *args].compact.join(" ")
       end
 
       def target_dir_non_existent_or_empty?
         !::File.exist?(@new_resource.destination) || Dir.entries(@new_resource.destination).sort == ['.','..']
       end
+
+      def svn_binary
+        @new_resource.svn_binary ||
+          (Chef::Platform.windows? ? 'svn.exe' : 'svn')
+      end
+
       def assert_target_directory_valid!
         target_parent_directory = ::File.dirname(@new_resource.destination)
         unless ::File.directory?(target_parent_directory)
diff --git a/lib/chef/resource/subversion.rb b/lib/chef/resource/subversion.rb
index ae6a37caa2..a6f4cb4897 100644
--- a/lib/chef/resource/subversion.rb
+++ b/lib/chef/resource/subversion.rb
@@ -28,12 +28,17 @@ class Chef
         super
         @svn_arguments = '--no-auth-cache'
         @svn_info_args = '--no-auth-cache'
+        @svn_binary = nil
       end
 
       # Override exception to strip password if any, so it won't appear in logs and different Chef notifications
       def custom_exception_message(e)
         "#{self} (#{defined_at}) had an error: #{e.class.name}: #{svn_password ? e.message.gsub(svn_password, "[hidden_password]") : e.message}"
       end
+
+      def svn_binary(arg=nil)
+        set_or_return(:svn_binary, arg, :kind_of => [String])
+      end
     end
   end
 end
diff --git a/spec/unit/provider/subversion_spec.rb b/spec/unit/provider/subversion_spec.rb
index 9ca11b8d82..9d4a8bd218 100644
--- a/spec/unit/provider/subversion_spec.rb
+++ b/spec/unit/provider/subversion_spec.rb
@@ -27,6 +27,7 @@ describe Chef::Provider::Subversion do
     @resource.revision "12345"
     @resource.svn_arguments(false)
     @resource.svn_info_args(false)
+    @resource.svn_binary "svn"
     @node = Chef::Node.new
     @events = Chef::EventDispatch::Dispatcher.new
     @run_context = Chef::RunContext.new(@node, {}, @events)
@@ -63,28 +64,18 @@ describe Chef::Provider::Subversion do
                           "Last Changed Rev: 11410\n" + # Last Changed Rev is preferred to Revision
                           "Last Changed Date: 2009-03-25 06:09:56 -0600 (Wed, 25 Mar 2009)\n\n"
       expect(::File).to receive(:exist?).at_least(1).times.with("/my/deploy/dir/.svn").and_return(true)
-      expect(::File).to receive(:directory?).with("/my/deploy/dir").and_return(true)
-      expect(::Dir).to receive(:chdir).with("/my/deploy/dir").and_yield
-      allow(@stdout).to receive(:string).and_return(example_svn_info)
-      allow(@stderr).to receive(:string).and_return("")
-      allow(@exitstatus).to receive(:exitstatus).and_return(0)
-      expected_command = ["svn info", {:cwd=>"/my/deploy/dir"}]
-      expect(@provider).to receive(:popen4).with(*expected_command).
-                                        and_yield("no-pid", "no-stdin", @stdout,@stderr).
-                                        and_return(@exitstatus)
+      expected_command = ["svn info", {:cwd => '/my/deploy/dir', :returns => [0,1]}]
+      expect(@provider).to receive(:shell_out!).with(*expected_command).
+                             and_return(double("ShellOut result", :stdout => example_svn_info, :stderr => ""))
       expect(@provider.find_current_revision).to eql("11410")
     end
 
     it "gives nil as the current revision if the deploy dir isn't a SVN working copy" do
       example_svn_info = "svn: '/tmp/deploydir' is not a working copy\n"
       expect(::File).to receive(:exist?).with("/my/deploy/dir/.svn").and_return(true)
-      expect(::File).to receive(:directory?).with("/my/deploy/dir").and_return(true)
-      expect(::Dir).to receive(:chdir).with("/my/deploy/dir").and_yield
-      allow(@stdout).to receive(:string).and_return(example_svn_info)
-      allow(@stderr).to receive(:string).and_return("")
-      allow(@exitstatus).to receive(:exitstatus).and_return(1)
-      expect(@provider).to receive(:popen4).and_yield("no-pid", "no-stdin", @stdout,@stderr).
-                                        and_return(@exitstatus)
+      expected_command = ["svn info", {:cwd => '/my/deploy/dir', :returns => [0,1]}]
+      expect(@provider).to receive(:shell_out!).with(*expected_command).
+                             and_return(double("ShellOut result", :stdout => example_svn_info, :stderr => ""))
       expect(@provider.find_current_revision).to be_nil
     end
 
@@ -127,28 +118,20 @@ describe Chef::Provider::Subversion do
                           "Last Changed Author: codeninja\n" +
                           "Last Changed Rev: 11410\n" + # Last Changed Rev is preferred to Revision
                           "Last Changed Date: 2009-03-25 06:09:56 -0600 (Wed, 25 Mar 2009)\n\n"
-      exitstatus = double("exitstatus")
-      allow(exitstatus).to receive(:exitstatus).and_return(0)
       @resource.revision "HEAD"
-      allow(@stdout).to receive(:string).and_return(example_svn_info)
-      allow(@stderr).to receive(:string).and_return("")
-      expected_command = ["svn info http://svn.example.org/trunk/ --no-auth-cache  -rHEAD", {:cwd=>Dir.tmpdir}]
-      expect(@provider).to receive(:popen4).with(*expected_command).
-                                        and_yield("no-pid","no-stdin",@stdout,@stderr).
-                                        and_return(exitstatus)
+      expected_command = ["svn info http://svn.example.org/trunk/ --no-auth-cache  -rHEAD", {:cwd => '/my/deploy/dir', :returns => [0,1]}]
+      expect(@provider).to receive(:shell_out!).with(*expected_command).
+                                        and_return(double("ShellOut result", :stdout => example_svn_info, :stderr => ""))
       expect(@provider.revision_int).to eql("11410")
     end
 
     it "returns a helpful message if data from `svn info` can't be parsed" do
       example_svn_info =  "some random text from an error message\n"
-      exitstatus = double("exitstatus")
-      allow(exitstatus).to receive(:exitstatus).and_return(0)
       @resource.revision "HEAD"
-      allow(@stdout).to receive(:string).and_return(example_svn_info)
-      allow(@stderr).to receive(:string).and_return("")
-      expect(@provider).to receive(:popen4).and_yield("no-pid","no-stdin",@stdout,@stderr).
-                                        and_return(exitstatus)
-      expect {@provider.revision_int}.to raise_error(RuntimeError, "Could not parse `svn info` data: some random text from an error message")
+      expected_command = ["svn info http://svn.example.org/trunk/ --no-auth-cache  -rHEAD", {:cwd => '/my/deploy/dir', :returns => [0,1]}]
+      expect(@provider).to receive(:shell_out!).with(*expected_command).
+                             and_return(double("ShellOut result", :stdout => example_svn_info, :stderr => ""))
+      expect {@provider.revision_int}.to raise_error(RuntimeError, "Could not parse `svn info` data: some random text from an error message\n")
 
     end
 
@@ -277,4 +260,40 @@ describe Chef::Provider::Subversion do
     expect(@resource).to be_updated
   end
 
+  context "selects the correct svn binary" do
+    before do
+    end
+
+    it "selects 'svn' as the binary by default" do
+      @resource.svn_binary nil
+      allow(ChefConfig).to receive(:windows?) { false }
+      expect(@provider).to receive(:svn_binary).and_return('svn')
+      expect(@provider.export_command).to eql(
+        'svn export --force -q  -r12345 http://svn.example.org/trunk/ /my/deploy/dir')
+    end
+
+    it "selects an svn binary with an exe extension on windows" do
+      @resource.svn_binary nil
+      allow(ChefConfig).to receive(:windows?) { true }
+      expect(@provider).to receive(:svn_binary).and_return('svn.exe')
+      expect(@provider.export_command).to eql(
+        'svn.exe export --force -q  -r12345 http://svn.example.org/trunk/ /my/deploy/dir')
+    end
+
+    it "uses a custom svn binary as part of the svn command" do
+      @resource.svn_binary 'teapot'
+      expect(@provider).to receive(:svn_binary).and_return('teapot')
+      expect(@provider.export_command).to eql(
+        'teapot export --force -q  -r12345 http://svn.example.org/trunk/ /my/deploy/dir')
+    end
+
+    it "wraps custom svn binary with quotes if it contains whitespace" do
+      @resource.svn_binary 'c:/program files (x86)/subversion/svn.exe'
+      expect(@provider).to receive(:svn_binary).and_return('c:/program files (x86)/subversion/svn.exe')
+      expect(@provider.export_command).to eql(
+        '"c:/program files (x86)/subversion/svn.exe" export --force -q  -r12345 http://svn.example.org/trunk/ /my/deploy/dir')
+    end
+
+  end
+
 end
diff --git a/spec/unit/resource/subversion_spec.rb b/spec/unit/resource/subversion_spec.rb
index 5cd5d0de80..aa4d1ed708 100644
--- a/spec/unit/resource/subversion_spec.rb
+++ b/spec/unit/resource/subversion_spec.rb
@@ -54,6 +54,10 @@ describe Chef::Resource::Subversion do
     expect(@svn.svn_arguments).to eq('--no-auth-cache')
   end
 
+  it "sets svn binary to nil by default" do
+    expect(@svn.svn_binary).to be_nil
+  end
+
   it "resets svn arguments to nil when given false in the setter" do
     @svn.svn_arguments(false)
     expect(@svn.svn_arguments).to be_nil