Merge pull request #4873 from joshburt/jb_git_hwrp_shell_out_cleanup

Consolidate shell_out usage when calling git within it's HWRP.

2 files changed, 66 insertions, 45 deletions

diff --git a/lib/chef/provider/git.rb b/lib/chef/provider/git.rb
index 82f5ca2ba5..9516011c3f 100644
--- a/lib/chef/provider/git.rb
+++ b/lib/chef/provider/git.rb
@@ -105,7 +105,7 @@ class Chef
       end
 
       def git_minor_version
-        @git_minor_version ||= Gem::Version.new(shell_out!("git --version", run_options).stdout.split.last)
+        @git_minor_version ||= Gem::Version.new( git("--version").stdout.split.last )
       end
 
       def existing_git_clone?
@@ -120,7 +120,7 @@ class Chef
         Chef::Log.debug("#{@new_resource} finding current git revision")
         if ::File.exist?(::File.join(cwd, ".git"))
           # 128 is returned when we're not in a git repo. this is fine
-          result = shell_out!("git rev-parse HEAD", :cwd => cwd, :returns => [0, 128]).stdout.strip
+          result = git("rev-parse", "HEAD", cwd: cwd, returns: [0, 128]).stdout.strip
         end
         sha_hash?(result) ? result : nil
       end
@@ -140,15 +140,15 @@ class Chef
         converge_by("clone from #{@new_resource.repository} into #{@new_resource.destination}") do
           remote = @new_resource.remote
 
-          args = []
-          args << "-o #{remote}" unless remote == "origin"
-          args << "--depth #{@new_resource.depth}" if @new_resource.depth
-          args << "--no-single-branch" if @new_resource.depth && git_minor_version >= Gem::Version.new("1.7.10")
+          clone_cmd = ["clone"]
+          clone_cmd << "-o #{remote}" unless remote == "origin"
+          clone_cmd << "--depth #{@new_resource.depth}" if @new_resource.depth
+          clone_cmd << "--no-single-branch" if @new_resource.depth && git_minor_version >= Gem::Version.new("1.7.10")
+          clone_cmd << "\"#{@new_resource.repository}\""
+          clone_cmd << "\"#{@new_resource.destination}\""
 
           Chef::Log.info "#{@new_resource} cloning repo #{@new_resource.repository} to #{@new_resource.destination}"
-
-          clone_cmd = "git clone #{args.join(' ')} \"#{@new_resource.repository}\" \"#{@new_resource.destination}\""
-          shell_out!(clone_cmd, run_options)
+          git clone_cmd
         end
       end
 
@@ -157,8 +157,8 @@ class Chef
 
         converge_by("checkout ref #{sha_ref} branch #{@new_resource.revision}") do
           # checkout into a local branch rather than a detached HEAD
-          shell_out!("git branch -f #{@new_resource.checkout_branch} #{sha_ref}", run_options(:cwd => @new_resource.destination))
-          shell_out!("git checkout #{@new_resource.checkout_branch}", run_options(:cwd => @new_resource.destination))
+          git("branch", "-f", @new_resource.checkout_branch, sha_ref, cwd: cwd)
+          git("checkout", @new_resource.checkout_branch, cwd: cwd)
           Chef::Log.info "#{@new_resource} checked out branch: #{@new_resource.revision} onto: #{@new_resource.checkout_branch} reference: #{sha_ref}"
         end
       end
@@ -167,12 +167,10 @@ class Chef
         if @new_resource.enable_submodules
           converge_by("enable git submodules for #{@new_resource}") do
             Chef::Log.info "#{@new_resource} synchronizing git submodules"
-            command = "git submodule sync"
-            shell_out!(command, run_options(:cwd => @new_resource.destination))
+            git("submodule", "sync", cwd: cwd)
             Chef::Log.info "#{@new_resource} enabling git submodules"
             # the --recursive flag means we require git 1.6.5+ now, see CHEF-1827
-            command = "git submodule update --init --recursive"
-            shell_out!(command, run_options(:cwd => @new_resource.destination))
+            git("submodule", "update", "--init", "--recursive", cwd: cwd)
           end
         end
       end
@@ -181,17 +179,18 @@ class Chef
         setup_remote_tracking_branches(@new_resource.remote, @new_resource.repository)
         converge_by("fetch updates for #{@new_resource.remote}") do
           # since we're in a local branch already, just reset to specified revision rather than merge
-          fetch_command = "git fetch #{@new_resource.remote} && git fetch #{@new_resource.remote} --tags && git reset --hard #{target_revision}"
           Chef::Log.debug "Fetching updates from #{new_resource.remote} and resetting to revision #{target_revision}"
-          shell_out!(fetch_command, run_options(:cwd => @new_resource.destination))
+          git("fetch", @new_resource.remote, cwd: cwd)
+          git("fetch", @new_resource.remote, "--tags", cwd: cwd)
+          git("reset", "--hard", target_revision, cwd: cwd)
         end
       end
 
       def setup_remote_tracking_branches(remote_name, remote_url)
         converge_by("set up remote tracking branches for #{remote_url} at #{remote_name}") do
           Chef::Log.debug "#{@new_resource} configuring remote tracking branches for repository #{remote_url} " + "at remote #{remote_name}"
-          check_remote_command = "git config --get remote.#{remote_name}.url"
-          remote_status = shell_out!(check_remote_command, run_options(:cwd => @new_resource.destination, :returns => [0, 1, 2]))
+          check_remote_command = ["config", "--get", "remote.#{remote_name}.url"]
+          remote_status = git(check_remote_command, cwd: cwd, returns: [0, 1, 2])
           case remote_status.exitstatus
           when 0, 2
             # * Status 0 means that we already have a remote with this name, so we should update the url
@@ -200,12 +199,10 @@ class Chef
             #   which we can fix by replacing them all with our target url (hence the --replace-all option)
 
             if multiple_remotes?(remote_status) || !remote_matches?(remote_url, remote_status)
-              update_remote_url_command = "git config --replace-all remote.#{remote_name}.url #{remote_url}"
-              shell_out!(update_remote_url_command, run_options(:cwd => @new_resource.destination))
+              git("config", "--replace-all", "remote.#{remote_name}.url", remote_url, cwd: cwd)
             end
           when 1
-            add_remote_command = "git remote add #{remote_name} #{remote_url}"
-            shell_out!(add_remote_command, run_options(:cwd => @new_resource.destination))
+            git("remote", "add", remote_name, remote_url, cwd: cwd)
           end
         end
       end
@@ -282,8 +279,7 @@ class Chef
       end
 
       def git_ls_remote(rev_pattern)
-        command = git(%Q{ls-remote "#{@new_resource.repository}" "#{rev_pattern}"})
-        shell_out!(command, run_options).stdout
+        git("ls-remote", "\"#{@new_resource.repository}\"", "\"#{rev_pattern}\"").stdout
       end
 
       def refs_search(refs, pattern)
@@ -319,8 +315,10 @@ class Chef
         @new_resource.destination
       end
 
-      def git(*args)
-        ["git", *args].compact.join(" ")
+      def git(*args, **run_opts)
+        git_command = ["git", args].compact.join(" ")
+        Chef::Log.debug "running #{git_command}"
+        shell_out!(git_command, run_options(run_opts))
       end
 
       def sha_hash?(string)
diff --git a/spec/unit/provider/git_spec.rb b/spec/unit/provider/git_spec.rb
index 97f04a5a77..36c33d313c 100644
--- a/spec/unit/provider/git_spec.rb
+++ b/spec/unit/provider/git_spec.rb
@@ -58,7 +58,7 @@ describe Chef::Provider::Git do
     it "determines the current revision when there is one" do
       expect(::File).to receive(:exist?).with("/my/deploy/dir/.git").and_return(true)
       @stdout = "9b4d8dc38dd471246e7cfb1c3c1ad14b0f2bee13\n"
-      expect(@provider).to receive(:shell_out!).with("git rev-parse HEAD", { :cwd => "/my/deploy/dir", :returns => [0, 128] }).and_return(double("ShellOut result", :stdout => @stdout))
+      expect(@provider).to receive(:shell_out!).with("git rev-parse HEAD", { :cwd => "/my/deploy/dir", :returns => [0, 128], :log_tag => "git[web2.0 app]" }).and_return(double("ShellOut result", :stdout => @stdout))
       expect(@provider.find_current_revision).to eql("9b4d8dc38dd471246e7cfb1c3c1ad14b0f2bee13")
     end
 
@@ -66,7 +66,7 @@ describe Chef::Provider::Git do
       expect(::File).to receive(:exist?).with("/my/deploy/dir/.git").and_return(true)
       @stderr = "fatal: Not a git repository (or any of the parent directories): .git"
       @stdout = ""
-      expect(@provider).to receive(:shell_out!).with("git rev-parse HEAD", :cwd => "/my/deploy/dir", :returns => [0, 128]).and_return(double("ShellOut result", :stdout => "", :stderr => @stderr))
+      expect(@provider).to receive(:shell_out!).with("git rev-parse HEAD", :cwd => "/my/deploy/dir", :returns => [0, 128], :log_tag => "git[web2.0 app]" ).and_return(double("ShellOut result", :stdout => "", :stderr => @stderr))
       expect(@provider.find_current_revision).to be_nil
     end
   end
@@ -218,7 +218,7 @@ SHAS
   context "with an ssh wrapper" do
     let(:deploy_user)  { "deployNinja" }
     let(:wrapper)      { "do_it_this_way.sh" }
-    let(:expected_cmd) { 'git clone  "git://github.com/opscode/chef.git" "/my/deploy/dir"' }
+    let(:expected_cmd) { 'git clone "git://github.com/opscode/chef.git" "/my/deploy/dir"' }
     let(:default_options) do
       {
         :user => deploy_user,
@@ -272,11 +272,11 @@ SHAS
     allow(Etc).to receive(:getpwnam).and_return(double("Struct::Passwd", :name => @resource.user, :dir => "/home/deployNinja"))
     @resource.destination "/Application Support/with/space"
     @resource.ssh_wrapper "do_it_this_way.sh"
-    expected_cmd = "git clone  \"git://github.com/opscode/chef.git\" \"/Application Support/with/space\""
+    expected_cmd = "git clone \"git://github.com/opscode/chef.git\" \"/Application Support/with/space\""
     expect(@provider).to receive(:shell_out!).with(expected_cmd, :user => "deployNinja",
-                                                                 :environment => { "GIT_SSH" => "do_it_this_way.sh",
-                                                                                   "HOME" => "/home/deployNinja" },
-                                                                 :log_tag => "git[web2.0 app]")
+                                                                 :log_tag => "git[web2.0 app]",
+                                                                 :environment => { "HOME" => "/home/deployNinja",
+                                                                                   "GIT_SSH" => "do_it_this_way.sh" })
     @provider.clone
   end
 
@@ -334,8 +334,12 @@ SHAS
 
   it "runs a sync command with default options" do
     expect(@provider).to receive(:setup_remote_tracking_branches).with(@resource.remote, @resource.repository)
-    expected_cmd = "git fetch origin && git fetch origin --tags && git reset --hard d35af14d41ae22b19da05d7d03a0bafc321b244c"
-    expect(@provider).to receive(:shell_out!).with(expected_cmd, :cwd => "/my/deploy/dir", :log_tag => "git[web2.0 app]")
+    expected_cmd1 = "git fetch origin"
+    expect(@provider).to receive(:shell_out!).with(expected_cmd1, :cwd => "/my/deploy/dir", :log_tag => "git[web2.0 app]")
+    expected_cmd2 = "git fetch origin --tags"
+    expect(@provider).to receive(:shell_out!).with(expected_cmd2, :cwd => "/my/deploy/dir", :log_tag => "git[web2.0 app]")
+    expected_cmd3 = "git reset --hard d35af14d41ae22b19da05d7d03a0bafc321b244c"
+    expect(@provider).to receive(:shell_out!).with(expected_cmd3, :cwd => "/my/deploy/dir", :log_tag => "git[web2.0 app]")
     @provider.fetch_updates
   end
 
@@ -344,27 +348,46 @@ SHAS
     allow(Etc).to receive(:getpwnam).and_return(double("Struct::Passwd", :name => @resource.user, :dir => "/home/whois"))
     @resource.group("thisis")
     expect(@provider).to receive(:setup_remote_tracking_branches).with(@resource.remote, @resource.repository)
-    expected_cmd = "git fetch origin && git fetch origin --tags && git reset --hard d35af14d41ae22b19da05d7d03a0bafc321b244c"
-    expect(@provider).to receive(:shell_out!).with(expected_cmd, :cwd => "/my/deploy/dir",
-                                                                 :user => "whois", :group => "thisis",
-                                                                 :log_tag => "git[web2.0 app]",
-                                                                 :environment => { "HOME" => "/home/whois" })
+
+    expected_cmd1 = "git fetch origin"
+    expect(@provider).to receive(:shell_out!).with(expected_cmd1, :cwd => "/my/deploy/dir",
+                                                                  :user => "whois", :group => "thisis",
+                                                                  :log_tag => "git[web2.0 app]",
+                                                                  :environment => { "HOME" => "/home/whois" })
+    expected_cmd2 = "git fetch origin --tags"
+    expect(@provider).to receive(:shell_out!).with(expected_cmd2, :cwd => "/my/deploy/dir",
+                                                                  :user => "whois", :group => "thisis",
+                                                                  :log_tag => "git[web2.0 app]",
+                                                                  :environment => { "HOME" => "/home/whois" })
+    expected_cmd3 = "git reset --hard d35af14d41ae22b19da05d7d03a0bafc321b244c"
+    expect(@provider).to receive(:shell_out!).with(expected_cmd3, :cwd => "/my/deploy/dir",
+                                                                  :user => "whois", :group => "thisis",
+                                                                  :log_tag => "git[web2.0 app]",
+                                                                  :environment => { "HOME" => "/home/whois" })
     @provider.fetch_updates
   end
 
   it "configures remote tracking branches when remote is ``origin''" do
     @resource.remote "origin"
     expect(@provider).to receive(:setup_remote_tracking_branches).with(@resource.remote, @resource.repository)
-    fetch_command = "git fetch origin && git fetch origin --tags && git reset --hard d35af14d41ae22b19da05d7d03a0bafc321b244c"
-    expect(@provider).to receive(:shell_out!).with(fetch_command, :cwd => "/my/deploy/dir", :log_tag => "git[web2.0 app]")
+    fetch_command1 = "git fetch origin"
+    expect(@provider).to receive(:shell_out!).with(fetch_command1, :cwd => "/my/deploy/dir", :log_tag => "git[web2.0 app]")
+    fetch_command2 = "git fetch origin --tags"
+    expect(@provider).to receive(:shell_out!).with(fetch_command2, :cwd => "/my/deploy/dir", :log_tag => "git[web2.0 app]")
+    fetch_command3 = "git reset --hard d35af14d41ae22b19da05d7d03a0bafc321b244c"
+    expect(@provider).to receive(:shell_out!).with(fetch_command3, :cwd => "/my/deploy/dir", :log_tag => "git[web2.0 app]")
     @provider.fetch_updates
   end
 
   it "configures remote tracking branches when remote is not ``origin''" do
     @resource.remote "opscode"
     expect(@provider).to receive(:setup_remote_tracking_branches).with(@resource.remote, @resource.repository)
-    fetch_command = "git fetch opscode && git fetch opscode --tags && git reset --hard d35af14d41ae22b19da05d7d03a0bafc321b244c"
-    expect(@provider).to receive(:shell_out!).with(fetch_command, :cwd => "/my/deploy/dir", :log_tag => "git[web2.0 app]")
+    fetch_command1 = "git fetch opscode"
+    expect(@provider).to receive(:shell_out!).with(fetch_command1, :cwd => "/my/deploy/dir", :log_tag => "git[web2.0 app]")
+    fetch_command2 = "git fetch opscode --tags"
+    expect(@provider).to receive(:shell_out!).with(fetch_command2, :cwd => "/my/deploy/dir", :log_tag => "git[web2.0 app]")
+    fetch_command3 = "git reset --hard d35af14d41ae22b19da05d7d03a0bafc321b244c"
+    expect(@provider).to receive(:shell_out!).with(fetch_command3, :cwd => "/my/deploy/dir", :log_tag => "git[web2.0 app]")
     @provider.fetch_updates
   end