Merge pull request #9726 from chef/lcg/chef-16-git

Refactor scm, git and subversion resources & fix longstanding git issues

16 files changed, 499 insertions, 301 deletions

diff --git a/lib/chef/dsl/declare_resource.rb b/lib/chef/dsl/declare_resource.rb
index f032a76bc7..02ad64c77a 100644
--- a/lib/chef/dsl/declare_resource.rb
+++ b/lib/chef/dsl/declare_resource.rb
@@ -267,10 +267,10 @@ class Chef
       #     action :delete
       #   end
       #
-      def declare_resource(type, name, created_at: nil, run_context: self.run_context, &resource_attrs_block)
+      def declare_resource(type, name, created_at: nil, run_context: self.run_context, enclosing_provider: nil, &resource_attrs_block)
         created_at ||= caller[0]
 
-        resource = build_resource(type, name, created_at: created_at, &resource_attrs_block)
+        resource = build_resource(type, name, created_at: created_at, enclosing_provider: enclosing_provider, &resource_attrs_block)
 
         run_context.resource_collection.insert(resource, resource_type: resource.declared_type, instance_name: resource.name)
         resource
@@ -297,13 +297,15 @@ class Chef
       #     action :delete
       #   end
       #
-      def build_resource(type, name, created_at: nil, run_context: self.run_context, &resource_attrs_block)
+      def build_resource(type, name, created_at: nil, run_context: self.run_context, enclosing_provider: nil, &resource_attrs_block)
         created_at ||= caller[0]
 
         # this needs to be lazy in order to avoid circular dependencies since ResourceBuilder
         # will requires the entire provider+resolver universe
         require_relative "../resource_builder" unless defined?(Chef::ResourceBuilder)
 
+        enclosing_provider ||= self if is_a?(Chef::Provider)
+
         Chef::ResourceBuilder.new(
           type:                type,
           name:                name,
@@ -312,7 +314,7 @@ class Chef
           run_context:         run_context,
           cookbook_name:       cookbook_name,
           recipe_name:         recipe_name,
-          enclosing_provider:  is_a?(Chef::Provider) ? self : nil
+          enclosing_provider:  enclosing_provider
         ).build(&resource_attrs_block)
       end
 
diff --git a/lib/chef/provider/git.rb b/lib/chef/provider/git.rb
index 9a86f26f60..c8b48f5602 100644
--- a/lib/chef/provider/git.rb
+++ b/lib/chef/provider/git.rb
@@ -61,7 +61,7 @@ class Chef
           a.assertion { ::File.directory?(dirname) }
           a.whyrun("Directory #{dirname} does not exist, this run will fail unless it has been previously created. Assuming it would have been created.")
           a.failure_message(Chef::Exceptions::MissingParentDirectory,
-            "Cannot clone #{new_resource} to #{cwd}, the enclosing directory #{dirname} does not exist")
+                            "Cannot clone #{new_resource} to #{cwd}, the enclosing directory #{dirname} does not exist")
         end
 
         requirements.assert(:all_actions) do |a|
@@ -182,13 +182,24 @@ class Chef
       end
 
       def checkout
-        sha_ref = target_revision
-
-        converge_by("checkout ref #{sha_ref} branch #{new_resource.revision}") do
+        converge_by("checkout ref #{target_revision} branch #{new_resource.revision}") do
           # checkout into a local branch rather than a detached HEAD
-          git("branch", "-f", new_resource.checkout_branch, sha_ref, cwd: cwd)
-          git("checkout", new_resource.checkout_branch, cwd: cwd)
-          logger.info "#{new_resource} checked out branch: #{new_resource.revision} onto: #{new_resource.checkout_branch} reference: #{sha_ref}"
+          if new_resource.checkout_branch
+            # check out to a local branch
+            git("branch", "-f", new_resource.checkout_branch, target_revision, cwd: cwd)
+            git("checkout", new_resource.checkout_branch, cwd: cwd)
+            logger.info "#{new_resource} checked out branch: #{new_resource.revision} onto: #{new_resource.checkout_branch} reference: #{target_revision}"
+          elsif sha_hash?(new_resource.revision) || !is_branch?
+            # detached head
+            git("checkout", target_revision, cwd: cwd)
+            logger.info "#{new_resource} checked out reference: #{target_revision}"
+          else
+            # need a branch with a tracking branch
+            git("branch", "-f", new_resource.revision, target_revision, cwd: cwd)
+            git("checkout", new_resource.revision, cwd: cwd)
+            git("branch", "-u", "#{new_resource.remote}/#{new_resource.revision}", cwd: cwd)
+            logger.info "#{new_resource} checked out branch: #{new_resource.revision} reference: #{target_revision}"
+          end
         end
       end
 
@@ -211,7 +222,19 @@ class Chef
           logger.trace "Fetching updates from #{new_resource.remote} and resetting to revision #{target_revision}"
           git("fetch", "--prune", new_resource.remote, cwd: cwd)
           git("fetch", new_resource.remote, "--tags", cwd: cwd)
-          git("reset", "--hard", target_revision, cwd: cwd)
+          if new_resource.checkout_branch
+            # check out to a local branch
+            git("branch", "-f", new_resource.checkout_branch, target_revision, cwd: cwd)
+            git("checkout", new_resource.checkout_branch, cwd: cwd)
+          elsif sha_hash?(new_resource.revision) || is_tag?
+            # detached head
+            git("reset", "--hard", target_revision, cwd: cwd)
+          else
+            # need a branch with a tracking branch
+            git("branch", "-f", new_resource.revision, target_revision, cwd: cwd)
+            git("checkout", new_resource.revision, cwd: cwd)
+            git("branch", "-u", "#{new_resource.remote}/#{new_resource.revision}", cwd: cwd)
+          end
         end
       end
 
@@ -287,9 +310,18 @@ class Chef
 
       def find_revision(refs, revision, suffix = "")
         found = refs_search(refs, rev_match_pattern("refs/tags/", revision) + suffix)
-        found = refs_search(refs, rev_match_pattern("refs/heads/", revision) + suffix) if found.empty?
-        found = refs_search(refs, revision + suffix) if found.empty?
-        found
+        if !found.empty?
+          @is_tag = true
+          found
+        else
+          found = refs_search(refs, rev_match_pattern("refs/heads/", revision) + suffix)
+          if !found.empty?
+            @is_branch = true
+            found
+          else
+            refs_search(refs, revision + suffix)
+          end
+        end
       end
 
       def rev_match_pattern(prefix, revision)
@@ -320,6 +352,14 @@ class Chef
 
       private
 
+      def is_branch?
+        !!@is_branch
+      end
+
+      def is_tag?
+        !!@is_tag
+      end
+
       def run_options(run_opts = {})
         env = {}
         if new_resource.user
@@ -341,7 +381,7 @@ class Chef
       def git(*args, **run_opts)
         git_command = ["git", args].compact.join(" ")
         logger.trace "running #{git_command}"
-        shell_out!(git_command, run_options(run_opts))
+        shell_out!(git_command, **run_options(run_opts))
       end
 
       def sha_hash?(string)
diff --git a/lib/chef/provider/subversion.rb b/lib/chef/provider/subversion.rb
index 486bb38e5d..270f7457fa 100644
--- a/lib/chef/provider/subversion.rb
+++ b/lib/chef/provider/subversion.rb
@@ -149,9 +149,15 @@ class Chef
       end
 
       def run_options(run_opts = {})
-        run_opts[:user] = new_resource.user if new_resource.user
+        env = {}
+        if new_resource.user
+          run_opts[:user] = new_resource.user
+          env["HOME"] = get_homedir(new_resource.user)
+        end
         run_opts[:group] = new_resource.group if new_resource.group
         run_opts[:timeout] = new_resource.timeout if new_resource.timeout
+        env.merge!(new_resource.environment) if new_resource.environment
+        run_opts[:environment] = env unless env.empty?
         run_opts
       end
 
@@ -225,6 +231,20 @@ class Chef
           raise Chef::Exceptions::MissingParentDirectory, msg
         end
       end
+
+      # Returns the home directory of the user
+      # @param [String] user must be a string.
+      # @return [String] the home directory of the user.
+      #
+      def get_homedir(user)
+        require "etc" unless defined?(Etc)
+        case user
+        when Integer
+          Etc.getpwuid(user).dir
+        else
+          Etc.getpwnam(user.to_s).dir
+        end
+      end
     end
   end
 end
diff --git a/lib/chef/resource/scm.rb b/lib/chef/resource/scm.rb
deleted file mode 100644
index a09168dc11..0000000000
--- a/lib/chef/resource/scm.rb
+++ /dev/null
@@ -1,78 +0,0 @@
-#
-# Author:: Daniel DeLeo (<dan@kallistec.com>)
-# Copyright:: Copyright (c) Chef Software Inc.
-# License:: Apache License, Version 2.0
-#
-# Licensed under the Apache License, Version 2.0 (the "License");
-# you may not use this file except in compliance with the License.
-# You may obtain a copy of the License at
-#
-#     http://www.apache.org/licenses/LICENSE-2.0
-#
-# Unless required by applicable law or agreed to in writing, software
-# distributed under the License is distributed on an "AS IS" BASIS,
-# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-# See the License for the specific language governing permissions and
-# limitations under the License.
-#
-
-require_relative "../resource"
-
-class Chef
-  class Resource
-    class Scm < Chef::Resource
-      unified_mode true
-
-      default_action :sync
-      allowed_actions :checkout, :export, :sync, :diff, :log
-
-      property :destination, String,
-        description: "The location path to which the source is to be cloned, checked out, or exported. Default value: the name of the resource block.",
-        name_property: true
-
-      property :repository, String
-
-      property :revision, String,
-        description: "The revision to checkout.",
-        default: "HEAD"
-
-      property :user, [String, Integer],
-        description: "The system user that is responsible for the checked-out code."
-
-      property :group, [String, Integer],
-        description: "The system group that is responsible for the checked-out code."
-
-      # Capistrano and git-deploy use ``shallow clone''
-      property :depth, Integer,
-        description: "The number of past revisions to be included in the git shallow clone. Unless specified the default behavior will do a full clone."
-
-      property :enable_submodules, [TrueClass, FalseClass],
-        description: "Perform a sub-module initialization and update.",
-        default: false
-
-      property :enable_checkout, [TrueClass, FalseClass],
-        description: "Check out a repo from master. Set to false when using the checkout_branch attribute to prevent the git resource from attempting to check out master from master.",
-        default: true
-
-      property :remote, String,
-        default: "origin"
-
-      property :ssh_wrapper, String,
-        desired_state: false
-
-      property :timeout, Integer,
-        description: "The amount of time (in seconds) to wait before timing out.",
-        desired_state: false
-
-      property :checkout_branch, String,
-        description: "Do a one-time checkout **or** use when a branch in the upstream repository is named 'deploy'. To prevent the resource from attempting to check out master from master, set 'enable_checkout' to 'false' when using the 'checkout_branch' property.",
-        default: "deploy"
-
-      property :environment, [Hash, nil],
-        description: "A Hash of environment variables in the form of ({'ENV_VARIABLE' => 'VALUE'}).",
-        default: nil
-
-      alias :env :environment
-    end
-  end
-end
diff --git a/lib/chef/resource/scm/_scm.rb b/lib/chef/resource/scm/_scm.rb
new file mode 100644
index 0000000000..ddb6619cab
--- /dev/null
+++ b/lib/chef/resource/scm/_scm.rb
@@ -0,0 +1,48 @@
+#
+# Author:: Daniel DeLeo (<dan@kallistec.com>)
+# Copyright:: Copyright (c) Chef Software Inc.
+# License:: Apache License, Version 2.0
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+unified_mode true
+
+default_action :sync
+allowed_actions :checkout, :export, :sync, :diff, :log
+
+property :destination, String,
+  description: "The location path to which the source is to be cloned, checked out, or exported. Default value: the name of the resource block.",
+  name_property: true
+
+property :repository, String
+
+property :revision, String,
+  description: "The revision to checkout.",
+  default: "HEAD"
+
+property :user, [String, Integer],
+  description: "The system user that should own the checked-out code."
+
+property :group, [String, Integer],
+  description: "The system group that should own the checked-out code."
+
+property :timeout, Integer,
+  description: "The amount of time (in seconds) to wait before timing out.",
+  desired_state: false
+
+property :environment, [Hash, nil],
+  description: "A Hash of environment variables in the form of ({'ENV_VARIABLE' => 'VALUE'}).",
+  default: nil
+
+alias :env :environment
diff --git a/lib/chef/resource/git.rb b/lib/chef/resource/scm/git.rb
index 2ae68b94af..788eafbd3b 100644
--- a/lib/chef/resource/git.rb
+++ b/lib/chef/resource/scm/git.rb
@@ -16,11 +16,13 @@
 # limitations under the License.
 #
 
-require_relative "scm"
+require_relative "../../resource"
 
 class Chef
   class Resource
-    class Git < Chef::Resource::Scm
+    class Git < Chef::Resource
+      use "scm"
+
       unified_mode true
 
       provides :git
@@ -31,6 +33,28 @@ class Chef
         description: "A Hash of additional remotes that are added to the git repository configuration.",
         default: lazy { {} }
 
+      property :depth, Integer,
+        description: "The number of past revisions to be included in the git shallow clone. Unless specified the default behavior will do a full clone."
+
+      property :enable_submodules, [TrueClass, FalseClass],
+        description: "Perform a sub-module initialization and update.",
+        default: false
+
+      property :enable_checkout, [TrueClass, FalseClass],
+        description: "Check out a repo from master. Set to false when using the checkout_branch attribute to prevent the git resource from attempting to check out master from master.",
+        default: true
+
+      property :remote, String,
+        description: "The remote repository to use when synchronizing an existing clone.",
+        default: "origin"
+
+      property :ssh_wrapper, String,
+        desired_state: false,
+        description: "The path to the wrapper script used when running SSH with git. The `GIT_SSH` environment variable is set to this."
+
+      property :checkout_branch, String,
+        description: "Set this to use a local branch to avoid checking SHAs or tags to a detatched head state."
+
       alias :branch :revision
       alias :reference :revision
       alias :repo :repository
diff --git a/lib/chef/resource/subversion.rb b/lib/chef/resource/scm/subversion.rb
index c1edabfd8a..619df673b4 100644
--- a/lib/chef/resource/subversion.rb
+++ b/lib/chef/resource/scm/subversion.rb
@@ -17,12 +17,13 @@
 # limitations under the License.
 #
 
-require_relative "scm"
-require_relative "../dist"
+require_relative "../../dist"
 
 class Chef
   class Resource
-    class Subversion < Chef::Resource::Scm
+    class Subversion < Chef::Resource
+      use "scm"
+
       unified_mode true
 
       provides :subversion
@@ -45,10 +46,10 @@ class Chef
         description: "The location of the svn binary."
 
       property :svn_username, String,
-        description: "The username to use for interacting with subversion."
+        description: "The user name for a user that has access to the Subversion repository."
 
       property :svn_password, String,
-        description: "The password to use for interacting with subversion.",
+        description: "The password for a user that has access to the Subversion repository.",
         sensitive: true, desired_state: false
 
       # Override exception to strip password if any, so it won't appear in logs and different Chef notifications
diff --git a/lib/chef/resources.rb b/lib/chef/resources.rb
index f997d43a39..6a87960972 100644
--- a/lib/chef/resources.rb
+++ b/lib/chef/resources.rb
@@ -53,7 +53,7 @@ require_relative "resource/file"
 require_relative "resource/freebsd_package"
 require_relative "resource/ips_package"
 require_relative "resource/gem_package"
-require_relative "resource/git"
+require_relative "resource/scm/git"
 require_relative "resource/group"
 require_relative "resource/http_request"
 require_relative "resource/hostname"
@@ -108,7 +108,6 @@ require_relative "resource/solaris_package"
 require_relative "resource/route"
 require_relative "resource/ruby"
 require_relative "resource/ruby_block"
-require_relative "resource/scm"
 require_relative "resource/script"
 require_relative "resource/service"
 require_relative "resource/sudo"
@@ -117,7 +116,7 @@ require_relative "resource/swap_file"
 require_relative "resource/systemd_unit"
 require_relative "resource/ssh_known_hosts_entry"
 require_relative "resource/windows_service"
-require_relative "resource/subversion"
+require_relative "resource/scm/subversion"
 require_relative "resource/smartos_package"
 require_relative "resource/template"
 require_relative "resource/user"
diff --git a/spec/functional/resource/git_spec.rb b/spec/functional/resource/git_spec.rb
index 677cfe4bf0..0355b59233 100644
--- a/spec/functional/resource/git_spec.rb
+++ b/spec/functional/resource/git_spec.rb
@@ -17,30 +17,18 @@
 #
 
 require "spec_helper"
-require "chef/mixin/shell_out"
 require "tmpdir"
-require "shellwords"
 
 # Deploy relies heavily on symlinks, so it doesn't work on windows.
 describe Chef::Resource::Git, requires_git: true do
-  include Chef::Mixin::ShellOut
-  let(:file_cache_path) { Dir.mktmpdir }
+  include RecipeDSLHelper
+
   # Some versions of git complains when the deploy directory is
   # already created. Here we intentionally don't create the deploy
   # directory beforehand.
   let(:base_dir_path) { Dir.mktmpdir }
   let(:deploy_directory) { File.join(base_dir_path, make_tmpname("git_base")) }
 
-  let(:node) do
-    Chef::Node.new.tap do |n|
-      n.name "rspec-test"
-      n.consume_external_attrs(@ohai.data, {})
-    end
-  end
-
-  let(:event_dispatch) { Chef::EventDispatch::Dispatcher.new }
-  let(:run_context) { Chef::RunContext.new(node, {}, event_dispatch) }
-
   # These tests use git's bundle feature, which is a way to export an entire
   # git repo (or subset of commits) as a single file.
   #
@@ -64,33 +52,36 @@ describe Chef::Resource::Git, requires_git: true do
   let(:rev_testing) { "972d153654503bccec29f630c5dd369854a561e8" }
   let(:rev_head) { "d294fbfd05aa7709ad9a9b8ef6343b17d355bf5f" }
 
-  let(:git_user_config) do
-    <<~E
-      [user]
-        name = frodoTbaggins
-        email = frodo@shire.org
-    E
-  end
-
   before(:each) do
-    Chef::Log.level = :warn # silence git command live streams
-    @old_file_cache_path = Chef::Config[:file_cache_path]
-    shell_out!("git clone \"#{git_bundle_repo}\" example", cwd: origin_repo_dir)
-    File.open("#{origin_repo}/.git/config", "a+") { |f| f.print(git_user_config) }
-    Chef::Config[:file_cache_path] = file_cache_path
+    shell_out!("git", "clone", git_bundle_repo, "example", cwd: origin_repo_dir)
+    File.open("#{origin_repo}/.git/config", "a+") do |f|
+      f.print <<~EOF
+        [user]
+          name = frodoTbaggins
+          email = frodo@shire.org
+      EOF
+    end
   end
 
   after(:each) do
-    Chef::Config[:file_cache_path] = @old_file_cache_path
     FileUtils.remove_entry_secure deploy_directory if File.exist?(deploy_directory)
     FileUtils.remove_entry_secure base_dir_path
-    FileUtils.remove_entry_secure file_cache_path
     FileUtils.remove_entry_secure origin_repo_dir
   end
 
-  before(:all) do
-    @ohai = Ohai::System.new
-    @ohai.all_plugins(%w{platform os})
+  def expect_revision_to_be(revision, version)
+    rev_ver = shell_out!("git", "rev-parse", revision, cwd: deploy_directory).stdout.strip
+    expect(rev_ver).to eq(version)
+  end
+
+  def expect_branch_upstream_to_be(branch, upstream)
+    branch_upstream = shell_out("git", "rev-parse", "--abbrev-ref", "#{branch}@{upstream}", cwd: deploy_directory).stdout.strip
+    expect(branch_upstream).to eq(upstream)
+  end
+
+  def expect_branch_to_be(branch)
+    head_branch = shell_out!("git name-rev --name-only HEAD", cwd: deploy_directory).stdout.strip
+    expect(head_branch).to eq(branch)
   end
 
   context "working with pathes with special characters" do
@@ -102,156 +93,221 @@ describe Chef::Resource::Git, requires_git: true do
     end
 
     it "clones a repository with a space in the path" do
-      Chef::Resource::Git.new(deploy_directory, run_context).tap do |r|
-        r.repository "#{path_with_spaces}/example-repo.gitbundle"
-      end.run_action(:sync)
+      repo = "#{path_with_spaces}/example-repo.gitbundle"
+      git(deploy_directory) do
+        repository repo
+      end.should_be_updated
+      expect_revision_to_be("HEAD", rev_head)
     end
   end
 
   context "when deploying from an annotated tag" do
-    let(:basic_git_resource) do
-      Chef::Resource::Git.new(deploy_directory, run_context).tap do |r|
-        r.repository origin_repo
-        r.revision "v1.0.0"
-      end
-    end
-
-    # We create a copy of the basic_git_resource so that we can run
-    # the resource again and verify that it doesn't update.
-    let(:copy_git_resource) do
-      Chef::Resource::Git.new(deploy_directory, run_context).tap do |r|
-        r.repository origin_repo
-        r.revision "v1.0.0"
-      end
-    end
-
     it "checks out the revision pointed to by the tag commit, not the tag commit itself" do
-      basic_git_resource.run_action(:sync)
-      head_rev = shell_out!("git rev-parse HEAD", cwd: deploy_directory, returns: [0]).stdout.strip
-      expect(head_rev).to eq(v1_commit)
+      git deploy_directory do
+        repository origin_repo
+        revision "v1.0.0"
+      end.should_be_updated
+      expect_revision_to_be("HEAD", v1_commit)
+      expect_branch_to_be("tags/v1.0.0^0") # detatched
       # also verify the tag commit itself is what we expect as an extra sanity check
-      rev = shell_out!("git rev-parse v1.0.0", cwd: deploy_directory, returns: [0]).stdout.strip
-      expect(rev).to eq(v1_tag)
+      expect_revision_to_be("v1.0.0", v1_tag)
     end
 
     it "doesn't update if up-to-date" do
-      # this used to fail because we didn't resolve the annotated tag
-      # properly to the pointed to commit.
-      basic_git_resource.run_action(:sync)
-      head_rev = shell_out!("git rev-parse HEAD", cwd: deploy_directory, returns: [0]).stdout.strip
-      expect(head_rev).to eq(v1_commit)
-
-      copy_git_resource.run_action(:sync)
-      expect(copy_git_resource).not_to be_updated
+      git deploy_directory do
+        repository origin_repo
+        revision "v1.0.0"
+      end.should_be_updated
+      git deploy_directory do
+        repository origin_repo
+        revision "v1.0.0"
+        expect_branch_to_be("tags/v1.0.0^0") # detatched
+      end.should_not_be_updated
     end
   end
 
   context "when deploying from a SHA revision" do
-    let(:basic_git_resource) do
-      Chef::Resource::Git.new(deploy_directory, run_context).tap do |r|
-        r.repository git_bundle_repo
-      end
-    end
-
-    # We create a copy of the basic_git_resource so that we can run
-    # the resource again and verify that it doesn't update.
-    let(:copy_git_resource) do
-      Chef::Resource::Git.new(deploy_directory, run_context).tap do |r|
-        r.repository origin_repo
-      end
+    it "checks out the expected revision ed18" do
+      git deploy_directory do
+        repository git_bundle_repo
+        revision rev_foo
+      end.should_be_updated
+      expect_revision_to_be("HEAD", rev_foo)
+      expect_branch_to_be("master~1") # detatched
     end
 
-    it "checks out the expected revision ed18" do
-      basic_git_resource.revision rev_foo
-      basic_git_resource.run_action(:sync)
-      head_rev = shell_out!("git rev-parse HEAD", cwd: deploy_directory, returns: [0]).stdout.strip
-      expect(head_rev).to eq(rev_foo)
+    it "checks out the expected revision ed18 to a local branch" do
+      git deploy_directory do
+        repository git_bundle_repo
+        revision rev_foo
+        checkout_branch "deploy"
+      end.should_be_updated
+      expect_revision_to_be("HEAD", rev_foo)
+      expect_branch_to_be("deploy") # detatched
     end
 
     it "doesn't update if up-to-date" do
-      basic_git_resource.revision rev_foo
-      basic_git_resource.run_action(:sync)
-      head_rev = shell_out!("git rev-parse HEAD", cwd: deploy_directory, returns: [0]).stdout.strip
-      expect(head_rev).to eq(rev_foo)
-
-      copy_git_resource.revision rev_foo
-      copy_git_resource.run_action(:sync)
-      expect(copy_git_resource).not_to be_updated
+      git deploy_directory do
+        repository git_bundle_repo
+        revision rev_foo
+      end.should_be_updated
+      expect_revision_to_be("HEAD", rev_foo)
+
+      git deploy_directory do
+        repository origin_repo
+        revision rev_foo
+      end.should_not_be_updated
+      expect_branch_to_be("master~1") # detatched
     end
 
     it "checks out the expected revision 972d" do
-      basic_git_resource.revision rev_testing
-      basic_git_resource.run_action(:sync)
-      head_rev = shell_out!("git rev-parse HEAD", cwd: deploy_directory, returns: [0]).stdout.strip
-      expect(head_rev).to eq(rev_testing)
+      git deploy_directory do
+        repository git_bundle_repo
+        revision rev_testing
+      end.should_be_updated
+      expect_revision_to_be("HEAD", rev_testing)
+      expect_branch_to_be("master~2") # detatched
+    end
+
+    it "checks out the expected revision 972d to a local branch" do
+      git deploy_directory do
+        repository git_bundle_repo
+        revision rev_testing
+        checkout_branch "deploy"
+      end.should_be_updated
+      expect_revision_to_be("HEAD", rev_testing)
+      expect_branch_to_be("deploy")
     end
   end
 
   context "when deploying from a revision named 'HEAD'" do
-    let(:basic_git_resource) do
-      Chef::Resource::Git.new(deploy_directory, run_context).tap do |r|
-        r.repository origin_repo
-        r.revision "HEAD"
-      end
+    it "checks out the expected revision" do
+      git deploy_directory do
+        repository origin_repo
+        revision "HEAD"
+      end.should_be_updated
+      expect_revision_to_be("HEAD", rev_head)
+      expect_branch_to_be("master")
     end
 
-    it "checks out the expected revision" do
-      basic_git_resource.run_action(:sync)
-      head_rev = shell_out!("git rev-parse HEAD", cwd: deploy_directory, returns: [0]).stdout.strip
-      expect(head_rev).to eq(rev_head)
+    it "checks out the expected revision, and is idempotent" do
+      git deploy_directory do
+        repository origin_repo
+        revision "HEAD"
+      end.should_be_updated
+      git deploy_directory do
+        repository origin_repo
+        revision "HEAD"
+      end.should_not_be_updated
+      expect_revision_to_be("HEAD", rev_head)
+      expect_branch_to_be("master")
+    end
+
+    it "checks out the expected revision to a local branch" do
+      git deploy_directory do
+        repository origin_repo
+        revision "HEAD"
+        checkout_branch "deploy"
+      end.should_be_updated
+      expect_revision_to_be("HEAD", rev_head)
+      expect_branch_to_be("deploy")
     end
   end
 
   context "when deploying from the default revision" do
-    let(:basic_git_resource) do
-      Chef::Resource::Git.new(deploy_directory, run_context).tap do |r|
-        r.repository origin_repo
-        # use default
-      end
+    it "checks out HEAD as the default revision" do
+      git deploy_directory do
+        repository origin_repo
+      end.should_be_updated
+      expect_branch_upstream_to_be("master", "origin/master")
+      expect_revision_to_be("HEAD", rev_head)
+      expect_branch_to_be("master")
     end
 
-    it "checks out HEAD as the default revision" do
-      basic_git_resource.run_action(:sync)
-      head_rev = shell_out!("git rev-parse HEAD", cwd: deploy_directory, returns: [0]).stdout.strip
-      expect(head_rev).to eq(rev_head)
+    it "checks out HEAD as the default revision, and is idempotent" do
+      git deploy_directory do
+        repository origin_repo
+      end.should_be_updated
+      git deploy_directory do
+        repository origin_repo
+      end.should_not_be_updated
+      expect_revision_to_be("HEAD", rev_head)
+      expect_branch_to_be("master")
+    end
+
+    it "checks out HEAD as the default revision to a local branch" do
+      git deploy_directory do
+        repository origin_repo
+        checkout_branch "deploy"
+      end.should_be_updated
+      expect_revision_to_be("HEAD", rev_head)
+      expect_branch_to_be("deploy")
     end
   end
 
   context "when dealing with a repo with a degenerate tag named 'HEAD'" do
     before do
-      shell_out!("git tag -m\"degenerate tag\" HEAD ed181b3419b6f489bedab282348162a110d6d3a1",
-        cwd: origin_repo)
+      shell_out!("git", "tag", "-m\"degenerate tag\"", "HEAD", "ed181b3419b6f489bedab282348162a110d6d3a1", cwd: origin_repo)
     end
 
-    let(:basic_git_resource) do
-      Chef::Resource::Git.new(deploy_directory, run_context).tap do |r|
-        r.repository origin_repo
-        r.revision "HEAD"
-      end
+    it "checks out the (master) HEAD revision and ignores the tag" do
+      git deploy_directory do
+        repository origin_repo
+        revision "HEAD"
+      end.should_be_updated
+      expect_revision_to_be("HEAD", rev_head)
+      expect_branch_to_be("master")
     end
 
-    let(:git_resource_default_rev) do
-      Chef::Resource::Git.new(deploy_directory, run_context).tap do |r|
-        r.repository origin_repo
-        # use default of revision
-      end
+    it "checks out the (master) HEAD revision and ignores the tag, and is idempotent" do
+      git deploy_directory do
+        repository origin_repo
+        revision "HEAD"
+      end.should_be_updated
+      git deploy_directory do
+        repository origin_repo
+        revision "HEAD"
+      end.should_not_be_updated
+      expect_revision_to_be("HEAD", rev_head)
+      expect_branch_to_be("master")
     end
 
-    it "checks out the (master) HEAD revision and ignores the tag" do
-      basic_git_resource.run_action(:sync)
-      head_rev = shell_out!("git rev-parse HEAD",
-        cwd: deploy_directory,
-        returns: [0]).stdout.strip
-      expect(head_rev).to eq(rev_head)
+    it "checks out the (master) HEAD revision and ignores the tag to a local branch" do
+      git deploy_directory do
+        repository origin_repo
+        revision "HEAD"
+        checkout_branch "deploy"
+      end.should_be_updated
+      expect_revision_to_be("HEAD", rev_head)
+      expect_branch_to_be("deploy")
     end
 
     it "checks out the (master) HEAD revision when no revision is specified (ignores tag)" do
-      git_resource_default_rev.run_action(:sync)
-      head_rev = shell_out!("git rev-parse HEAD",
-        cwd: deploy_directory,
-        returns: [0]).stdout.strip
-      expect(head_rev).to eq(rev_head)
+      git deploy_directory do
+        repository origin_repo
+      end.should_be_updated
+      expect_revision_to_be("HEAD", rev_head)
+      expect_branch_to_be("master")
+    end
+
+    it "checks out the (master) HEAD revision when no revision is specified (ignores tag), and is idempotent" do
+      git deploy_directory do
+        repository origin_repo
+      end.should_be_updated
+      git deploy_directory do
+        repository origin_repo
+      end.should_not_be_updated
+      expect_revision_to_be("HEAD", rev_head)
+      expect_branch_to_be("master")
     end
 
+    it "checks out the (master) HEAD revision when no revision is specified (ignores tag) to a local branch" do
+      git deploy_directory do
+        repository origin_repo
+        checkout_branch "deploy"
+      end.should_be_updated
+      expect_revision_to_be("HEAD", rev_head)
+      expect_branch_to_be("deploy")
+    end
   end
 end
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index 127af0470f..e2e967ba9c 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -74,6 +74,8 @@ require "spec/support/local_gems.rb" if File.exists?(File.join(File.dirname(__FI
 require "spec/support/platform_helpers"
 require "spec/support/shared/unit/mock_shellout"
 
+require "spec/support/recipe_dsl_helper"
+
 # Autoloads support files
 # Excludes support/platforms by default
 # Do not change the gsub.
diff --git a/spec/support/recipe_dsl_helper.rb b/spec/support/recipe_dsl_helper.rb
new file mode 100644
index 0000000000..2542345ed4
--- /dev/null
+++ b/spec/support/recipe_dsl_helper.rb
@@ -0,0 +1,83 @@
+#
+# This is a helper for functional tests to embed the recipe DSL directly into the rspec example blocks using
+# unified mode.
+#
+# If you wind up wanting to stub/expect on internal details of the resource/provider you are not testing the
+# public API and are trying to write a unit test, which this is not designed for.
+#
+# If you want to start writing full recipes and testing them, doing notifies/subscribes/etc then you are writing
+# an integration test, and not a functional single-resource test, which this is not designed for.
+#
+# Examples:
+#
+# it "creates a file" do
+#   FileUtils.rm_f("/tmp/foo.xyz")
+#   file "/tmp/foo.xyz" do           # please use proper tmpdir though
+#     content "whatever"
+#   end.should_be_updated
+#   expect(IO.read("/tmp/foo.xyz").to eql("content")
+# end
+#
+# it "is idempotent" do
+#   FileUtils.rm_f("/tmp/foo.xyz")
+#   file "/tmp/foo.xyz" do           # please use proper tmpdir though
+#     content "whatever"
+#   end.should_be_updated
+#   file "/tmp/foo.xyz" do           # please use proper tmpdir though
+#     content "whatever"
+#   end.should_not_be_updated
+#   expect(IO.read("/tmp/foo.xyz").to eql("content")
+# end
+#
+# it "has a failure" do
+#   FileUtils.rm_f("/tmp/foo.xyz")
+#   expect { file "/tmp/lksjdflksjdf/foo.xyz" do
+#     content "whatever"
+#   end }.to raise_error(Chef::Exception::EnclosingDirectoryDoesNotExist)
+# end
+#
+module RecipeDSLHelper
+  include Chef::DSL::Recipe
+  def event_dispatch
+    @event_dispatch ||= Chef::EventDispatch::Dispatcher.new
+  end
+
+  def node
+    @node ||= Chef::Node.new.tap do |n|
+      # clone the global ohai data to keep tests fast but reasonably isolated
+      n.consume_external_attrs(OHAI_SYSTEM.data.dup, {})
+    end
+  end
+
+  def run_context
+    @run_context ||= Chef::RunContext.new(node, {}, event_dispatch).tap do |rc|
+      rc.resource_collection.unified_mode = true
+      Chef::Runner.new(rc)
+    end
+  end
+
+  def cookbook_name
+    "rspec"
+  end
+
+  def recipe_name
+    "default"
+  end
+
+  def declare_resource(type, name, created_at: nil, run_context: self.run_context, &resource_attrs_block)
+    created_at = caller[0]
+    rspec_context = self
+    # we slightly abuse the "enclosing_provider" method_missing magic to send methods to the rspec example block so that
+    # rspec `let` methods work as arguments to resource properties
+    resource = super(type, name, created_at: created_at, run_context: run_context, enclosing_provider: rspec_context, &resource_attrs_block)
+    # we also inject these methods to make terse expression of checking the updated status (so it is more readiable and
+    # therefore should get used more -- even though it is "should" vs. "expect")
+    resource.define_singleton_method(:should_be_updated) do
+      rspec_context.expect(self).to be_updated
+    end
+    resource.define_singleton_method(:should_not_be_updated) do
+      rspec_context.expect(self).not_to be_updated
+    end
+    resource
+  end
+end
diff --git a/spec/unit/provider/git_spec.rb b/spec/unit/provider/git_spec.rb
index b98745b3ca..2f253358f1 100644
--- a/spec/unit/provider/git_spec.rb
+++ b/spec/unit/provider/git_spec.rb
@@ -400,7 +400,8 @@ describe Chef::Provider::Git do
     @provider.clone
   end
 
-  it "runs a checkout command with default options" do
+  it "runs a checkout command when the local branch is set" do
+    @resource.checkout_branch "deploy"
     expect(@provider).to receive(:shell_out!).with("git branch -f deploy d35af14d41ae22b19da05d7d03a0bafc321b244c", cwd: "/my/deploy/dir",
                                                                                                                     log_tag: "git[web2.0 app]").ordered
     expect(@provider).to receive(:shell_out!).with("git checkout deploy", cwd: "/my/deploy/dir",
@@ -607,7 +608,7 @@ describe Chef::Provider::Git do
 
     it "does not raise an error if user exists" do
       allow(@provider).to receive(:get_homedir).with(@resource.user).and_return("/home/test")
-      expect { @provider.run_action(:sync) }.not_to raise_error(ArgumentError)
+      expect { @provider.run_action(:sync) }.not_to raise_error
     end
   end
 
@@ -622,8 +623,10 @@ describe Chef::Provider::Git do
     end
 
     it "does not raise an error if user exists" do
+      allow(@provider).to receive(:action_sync) # stub the entire action
+      allow(::File).to receive(:directory?).with("/my/deploy").and_return(true)
       allow(@provider).to receive(:get_homedir).with(@resource.user).and_return("/home/test")
-      expect { @provider.run_action(:sync) }.not_to raise_error(Chef::Exceptions::User)
+      expect { @provider.run_action(:sync) }.not_to raise_error
     end
   end
 
diff --git a/spec/unit/provider/subversion_spec.rb b/spec/unit/provider/subversion_spec.rb
index f0393f6b40..f3d8404841 100644
--- a/spec/unit/provider/subversion_spec.rb
+++ b/spec/unit/provider/subversion_spec.rb
@@ -46,7 +46,8 @@ describe Chef::Provider::Subversion do
   it "converts resource properties to options for shell_out" do
     expect(@provider.run_options).to eq({})
     @resource.user "deployninja"
-    expect(@provider.run_options).to eq({ user: "deployninja" })
+    expect(@provider).to receive(:get_homedir).and_return("/home/deployninja")
+    expect(@provider.run_options).to eq({ user: "deployninja", environment: { "HOME" => "/home/deployninja" } })
   end
 
   context "determining the revision of the currently deployed code" do
@@ -221,7 +222,8 @@ describe Chef::Provider::Subversion do
     @resource.user "whois"
     @resource.group "thisis"
     expected_cmd = "svn checkout -q   -r12345 http://svn.example.org/trunk/ /my/deploy/dir"
-    expect(@provider).to receive(:shell_out!).with(expected_cmd, { user: "whois", group: "thisis" })
+    expect(@provider).to receive(:get_homedir).and_return("/home/whois")
+    expect(@provider).to receive(:shell_out!).with(expected_cmd, { user: "whois", group: "thisis", environment: { "HOME" => "/home/whois" } })
     @provider.run_action(:checkout)
     expect(@resource).to be_updated
   end
diff --git a/spec/unit/resource/git_spec.rb b/spec/unit/resource/scm/git_spec.rb
index 740af48e5f..706b7c370b 100644
--- a/spec/unit/resource/git_spec.rb
+++ b/spec/unit/resource/scm/git_spec.rb
@@ -17,6 +17,7 @@
 #
 
 require "spec_helper"
+require_relative "scm"
 
 describe Chef::Resource::Git do
 
@@ -29,8 +30,55 @@ describe Chef::Resource::Git do
 
   let(:resource) { Chef::Resource::Git.new("fakey_fakerton") }
 
-  it "is a subclass of Chef::Resource::Scm" do
-    expect(resource).to be_a_kind_of(Chef::Resource::Scm)
+  it_behaves_like "an SCM resource"
+
+  it "takes the depth as an integer for shallow clones" do
+    resource.depth 5
+    expect(resource.depth).to eq(5)
+    expect { resource.depth "five" }.to raise_error(ArgumentError)
+  end
+
+  it "defaults to nil depth for a full clone" do
+    expect(resource.depth).to be_nil
+  end
+
+  it "takes a boolean for #enable_submodules" do
+    resource.enable_submodules true
+    expect(resource.enable_submodules).to be_truthy
+    expect { resource.enable_submodules "lolz" }.to raise_error(ArgumentError)
+  end
+
+  it "defaults to not enabling submodules" do
+    expect(resource.enable_submodules).to be_falsey
+  end
+
+  it "takes a boolean for #enable_checkout" do
+    resource.enable_checkout true
+    expect(resource.enable_checkout).to be_truthy
+    expect { resource.enable_checkout "lolz" }.to raise_error(ArgumentError)
+  end
+
+  it "defaults to enabling checkout" do
+    expect(resource.enable_checkout).to be_truthy
+  end
+
+  it "takes a string for the remote" do
+    resource.remote "opscode"
+    expect(resource.remote).to eql("opscode")
+    expect { resource.remote 1337 }.to raise_error(ArgumentError)
+  end
+
+  it "defaults to ``origin'' for the remote" do
+    expect(resource.remote).to eq("origin")
+  end
+
+  it "takes a string for the ssh wrapper" do
+    resource.ssh_wrapper "with_ssh_fu"
+    expect(resource.ssh_wrapper).to eql("with_ssh_fu")
+  end
+
+  it "defaults to nil for the ssh wrapper" do
+    expect(resource.ssh_wrapper).to be_nil
   end
 
   it "uses aliases revision as branch" do
diff --git a/spec/unit/resource/scm_spec.rb b/spec/unit/resource/scm/scm.rb
index 6f282c1350..28c3f73136 100644
--- a/spec/unit/resource/scm_spec.rb
+++ b/spec/unit/resource/scm/scm.rb
@@ -19,9 +19,7 @@
 
 require "spec_helper"
 
-describe Chef::Resource::Scm do
-  let(:resource) { Chef::Resource::Scm.new("fakey_fakerton") }
-
+shared_examples_for "an SCM resource" do
   it "the destination property is the name_property" do
     expect(resource.destination).to eql("fakey_fakerton")
   end
@@ -78,55 +76,6 @@ describe Chef::Resource::Scm do
     expect(resource.group).to eq(23)
   end
 
-  it "takes the depth as an integer for shallow clones" do
-    resource.depth 5
-    expect(resource.depth).to eq(5)
-    expect { resource.depth "five" }.to raise_error(ArgumentError)
-  end
-
-  it "defaults to nil depth for a full clone" do
-    expect(resource.depth).to be_nil
-  end
-
-  it "takes a boolean for #enable_submodules" do
-    resource.enable_submodules true
-    expect(resource.enable_submodules).to be_truthy
-    expect { resource.enable_submodules "lolz" }.to raise_error(ArgumentError)
-  end
-
-  it "defaults to not enabling submodules" do
-    expect(resource.enable_submodules).to be_falsey
-  end
-
-  it "takes a boolean for #enable_checkout" do
-    resource.enable_checkout true
-    expect(resource.enable_checkout).to be_truthy
-    expect { resource.enable_checkout "lolz" }.to raise_error(ArgumentError)
-  end
-
-  it "defaults to enabling checkout" do
-    expect(resource.enable_checkout).to be_truthy
-  end
-
-  it "takes a string for the remote" do
-    resource.remote "opscode"
-    expect(resource.remote).to eql("opscode")
-    expect { resource.remote 1337 }.to raise_error(ArgumentError)
-  end
-
-  it "defaults to ``origin'' for the remote" do
-    expect(resource.remote).to eq("origin")
-  end
-
-  it "takes a string for the ssh wrapper" do
-    resource.ssh_wrapper "with_ssh_fu"
-    expect(resource.ssh_wrapper).to eql("with_ssh_fu")
-  end
-
-  it "defaults to nil for the ssh wrapper" do
-    expect(resource.ssh_wrapper).to be_nil
-  end
-
   it "defaults to nil for the environment" do
     expect(resource.environment).to be_nil
   end
diff --git a/spec/unit/resource/subversion_spec.rb b/spec/unit/resource/scm/subversion_spec.rb
index 48cc7fea42..394ed6be9f 100644
--- a/spec/unit/resource/subversion_spec.rb
+++ b/spec/unit/resource/scm/subversion_spec.rb
@@ -17,6 +17,7 @@
 #
 
 require "spec_helper"
+require_relative "scm"
 
 describe Chef::Resource::Subversion do
   static_provider_resolution(
@@ -28,9 +29,7 @@ describe Chef::Resource::Subversion do
 
   let(:resource) { Chef::Resource::Subversion.new("fakey_fakerton") }
 
-  it "is a subclass of Resource::Scm" do
-    expect(resource).to be_a_kind_of(Chef::Resource::Scm)
-  end
+  it_behaves_like "an SCM resource"
 
   it "the destination property is the name_property" do
     expect(resource.destination).to eql("fakey_fakerton")