Merge pull request #173 from hunner/mod_660

(MODULES-660) Correct detached HEAD on latest

4 files changed, 209 insertions, 148 deletions

diff --git a/lib/puppet/provider/vcsrepo/git.rb b/lib/puppet/provider/vcsrepo/git.rb
index 5c5dd07..0a142bb 100644
--- a/lib/puppet/provider/vcsrepo/git.rb
+++ b/lib/puppet/provider/vcsrepo/git.rb
@@ -31,69 +31,49 @@ Puppet::Type.type(:vcsrepo).provide(:git, :parent => Puppet::Provider::Vcsrepo)
     FileUtils.rm_rf(@resource.value(:path))
   end
 
+  # Checks to see if the current revision is equal to the revision on the
+  # remote (whether on a branch, tag, or reference)
+  #
+  # @return [Boolean] Returns true if the repo is on the latest revision
   def latest?
-    at_path do
-      return self.revision == self.latest
-    end
+    return revision == latest_revision
   end
 
+  # Just gives the `should` value that we should be setting the repo to if
+  # latest? returns false
+  #
+  # @return [String] Returns the target sha/tag/branch
   def latest
-    branch = on_branch?
-    if !branch
-      return get_revision('HEAD')
-    else
-      return get_revision("#{@resource.value(:remote)}/#{branch}")
-    end
+    @resource.value(:revision)
   end
 
+  # Get the current revision of the repo (tag/branch/sha)
+  #
+  # @return [String] Returns the branch/tag if the current sha matches the
+  #                  remote; otherwise returns the current sha.
   def revision
-    update_references
-    current = at_path { git_with_identity('rev-parse', 'HEAD').chomp }
-    return current unless @resource.value(:revision)
-
-    if tag_revision?(@resource.value(:revision))
-      # git-rev-parse will give you the hash of the tag object itself rather than the commit it points to by default.
-      # Using tag^0 will return the actual commit.
-      canonical = at_path { git_with_identity('rev-parse', "#{@resource.value(:revision)}^0").chomp }
-    else
-      # if it's not a tag, look for it as a local ref
-      canonical = at_path { git_with_identity('rev-parse', '--revs-only', @resource.value(:revision)).chomp }
-      if canonical.empty?
-        # git rev-parse executed properly but didn't find the ref;
-        # look for it in the remote
-        remote_ref = at_path { git_with_identity('ls-remote', '--heads', '--tags', @resource.value(:remote), @resource.value(:revision)).chomp }
-        if remote_ref.empty?
-          fail("#{@resource.value(:revision)} is not a local or remote ref")
-        end
-
-        # $ git ls-remote --heads --tags origin feature/cvs
-        # 7d4244b35e72904e30130cad6d2258f901c16f1a	refs/heads/feature/cvs
-        canonical = remote_ref.split.first
-      end
-    end
-
-    if current == canonical
-      @resource.value(:revision)
-    else
-      current
-    end
+    #HEAD is the default, but lets just be explicit here.
+    get_revision('HEAD')
   end
 
+  # Is passed the desired reference, whether a tag, rev, or branch. Should
+  # handle transitions from a rev/branch/tag to a rev/branch/tag. Detached
+  # heads should be treated like bare revisions.
+  #
+  # @param [String] desired The desired revision to which the repo should be
+  #                         set.
   def revision=(desired)
+    #just checkout tags and shas; fetch has already happened so they should be updated.
     checkout(desired)
-    if local_branch_revision?
-      # reset instead of pull to avoid merge conflicts. assuming remote is
-      # authoritative.
-      # might be worthwhile to have an allow_local_changes param to decide
-      # whether to reset or pull when we're ensuring latest.
-      at_path {
-        git_with_identity('reset', '--hard', "#{@resource.value(:remote)}/#{desired}")
-        if detached?
-          git_with_identity('checkout', "#{@resource.value(:revision)}")
-          git_with_identity('pull')
-        end
-      }
+    #branches require more work.
+    if local_branch_revision?(desired)
+      #reset instead of pull to avoid merge conflicts. assuming remote is
+      #updated and authoritative.
+      #TODO might be worthwhile to have an allow_local_changes param to decide
+      #whether to reset or pull when we're ensuring latest.
+      at_path { git_with_identity('reset', '--hard', "#{@resource.value(:remote)}/#{desired}") }
     end
+    #TODO Would this ever reach here if it is bare?
     if @resource.value(:ensure) != :bare
       update_submodules
     end
@@ -132,10 +112,12 @@ Puppet::Type.type(:vcsrepo).provide(:git, :parent => Puppet::Provider::Vcsrepo)
 
   private
 
+  # @!visibility private
   def bare_git_config_exists?
     File.exist?(File.join(@resource.value(:path), 'config'))
   end
 
+  # @!visibility private
   def clone_repository(source, path)
     check_force
     args = ['clone']
@@ -158,6 +140,7 @@ Puppet::Type.type(:vcsrepo).provide(:git, :parent => Puppet::Provider::Vcsrepo)
     end
   end
 
+  # @!visibility private
   def check_force
     if path_exists? and not path_empty?
       if @resource.value(:force)
@@ -169,6 +152,7 @@ Puppet::Type.type(:vcsrepo).provide(:git, :parent => Puppet::Provider::Vcsrepo)
     end
   end
 
+  # @!visibility private
   def init_repository(path)
     check_force
     if @resource.value(:ensure) == :bare && working_copy_exists?
@@ -195,6 +179,7 @@ Puppet::Type.type(:vcsrepo).provide(:git, :parent => Puppet::Provider::Vcsrepo)
   #   <path>/.git
   # to:
   #   <path>/
+  # @!visibility private
   def convert_working_copy_to_bare
     notice "Converting working copy repository to bare repository"
     FileUtils.mv(File.join(@resource.value(:path), '.git'), tempdir)
@@ -208,6 +193,7 @@ Puppet::Type.type(:vcsrepo).provide(:git, :parent => Puppet::Provider::Vcsrepo)
   #   <path>/
   # to:
   #   <path>/.git
+  # @!visibility private
   def convert_bare_to_working_copy
     notice "Converting bare repository to working copy repository"
     FileUtils.mv(@resource.value(:path), tempdir)
@@ -220,6 +206,7 @@ Puppet::Type.type(:vcsrepo).provide(:git, :parent => Puppet::Provider::Vcsrepo)
     end
   end
 
+  # @!visibility private
   def commits_in?(dot_git)
     Dir.glob(File.join(dot_git, 'objects/info/*'), File::FNM_DOTMATCH) do |e|
       return true unless %w(. ..).include?(File::basename(e))
@@ -227,26 +214,35 @@ Puppet::Type.type(:vcsrepo).provide(:git, :parent => Puppet::Provider::Vcsrepo)
     false
   end
 
+  # Will checkout a rev/branch/tag using the locally cached versions. Does not
+  # handle upstream branch changes
+  # @!visibility private
   def checkout(revision = @resource.value(:revision))
     if !local_branch_revision? && remote_branch_revision?
-      at_path { git_with_identity('checkout', '-b', revision, '--track', "#{@resource.value(:remote)}/#{revision}") }
+      #non-locally existant branches (perhaps switching to a branch that has never been checked out)
+      at_path { git_with_identity('checkout', '--force', '-b', revision, '--track', "#{@resource.value(:remote)}/#{revision}") }
     else
+      #tags, locally existant branches (perhaps outdated), and shas
       at_path { git_with_identity('checkout', '--force', revision) }
     end
   end
 
+  # @!visibility private
   def reset(desired)
     at_path do
       git_with_identity('reset', '--hard', desired)
     end
   end
 
+  # @!visibility private
   def update_submodules
     at_path do
       git_with_identity('submodule', 'update', '--init', '--recursive')
     end
   end
 
+  # Determins if the branch exists at the upstream but has not yet been locally committed
+  # @!visibility private
   def remote_branch_revision?(revision = @resource.value(:revision))
     # git < 1.6 returns '#{@resource.value(:remote)}/#{revision}'
     # git 1.6+ returns 'remotes/#{@resource.value(:remote)}/#{revision}'
@@ -254,64 +250,93 @@ Puppet::Type.type(:vcsrepo).provide(:git, :parent => Puppet::Provider::Vcsrepo)
     branch unless branch.empty?
   end
 
+  # Determins if the branch is already cached locally
+  # @!visibility private
   def local_branch_revision?(revision = @resource.value(:revision))
     at_path { branches.include?(revision) }
   end
 
+  # @!visibility private
   def tag_revision?(revision = @resource.value(:revision))
     at_path { tags.include?(revision) }
   end
 
+  # @!visibility private
   def branches
     at_path { git_with_identity('branch', '-a') }.gsub('*', ' ').split(/\n/).map { |line| line.strip }
   end
 
+  # @!visibility private
   def on_branch?
     at_path {
       matches = git_with_identity('branch', '-a').match /\*\s+(.*)/
-      matches[1] unless matches[1].match /detached/
-    }
-  end
-
-  def detached?
-    at_path {
-      git_with_identity('branch', '-a').match /\*\s+\(detached from.*\)/
+      matches[1] unless matches[1].match /(\(detached from|\(no branch)/
     }
   end
 
+  # @!visibility private
   def tags
     at_path { git_with_identity('tag', '-l') }.split(/\n/).map { |line| line.strip }
   end
 
+  # @!visibility private
   def set_excludes
     at_path { open('.git/info/exclude', 'w') { |f| @resource.value(:excludes).each { |ex| f.write(ex + "\n") }}}
   end
 
-  def get_revision(rev)
-    if @resource.value(:force) && working_copy_exists?
-      create
-    end
-    if !working_copy_exists?
-      create
-    end
-    at_path do
-      update_remote_origin_url
-      git_with_identity('fetch', @resource.value(:remote))
-      git_with_identity('fetch', '--tags', @resource.value(:remote))
+  # Finds the latest revision or sha of the current branch if on a branch, or
+  # of HEAD otherwise.
+  # @note Calls create which can forcibly destroy and re-clone the repo if
+  #       force => true
+  # @see get_revision
+  #
+  # @!visibility private
+  # @return [String] Returns the output of get_revision
+  def latest_revision
+    #TODO Why is create called here anyway?
+    create if @resource.value(:force) && working_copy_exists?
+    create if !working_copy_exists?
+
+    if branch = on_branch?
+      return get_revision("#{@resource.value(:remote)}/#{branch}")
+    else
+      return get_revision
     end
+  end
+
+  # Returns the current revision given if the revision is a tag or branch and
+  # matches the current sha. If the current sha does not match the sha of a tag
+  # or branch, then it will just return the sha (ie, is not in sync)
+  #
+  # @!visibility private
+  #
+  # @param [String] rev The revision of which to check if it is current
+  # @return [String] Returns the tag/branch of the current repo if it's up to
+  #                  date; otherwise returns the sha of the requested revision.
+  def get_revision(rev = 'HEAD')
+    update_references
     current = at_path { git_with_identity('rev-parse', rev).strip }
     if @resource.value(:revision)
-      if local_branch_revision?
+      if tag_revision?
+        # git-rev-parse will give you the hash of the tag object itself rather
+        # than the commit it points to by default. Using tag^0 will return the
+        # actual commit.
+        canonical = at_path { git_with_identity('rev-parse', "#{@resource.value(:revision)}^0").strip }
+      elsif local_branch_revision?
         canonical = at_path { git_with_identity('rev-parse', @resource.value(:revision)).strip }
       elsif remote_branch_revision?
-        canonical = at_path { git_with_identity('rev-parse', "#{@resource.value(:remote)}/" + @resource.value(:revision)).strip }
+        canonical = at_path { git_with_identity('rev-parse', "#{@resource.value(:remote)}/#{@resource.value(:revision)}").strip }
+      else
+        #look for a sha (could match invalid shas)
+        canonical = at_path { git_with_identity('rev-parse', '--revs-only', @resource.value(:revision)).strip }
       end
+      fail("#{@resource.value(:revision)} is not a local or remote ref") if canonical.nil? or canonical.empty?
       current = @resource.value(:revision) if current == canonical
     end
-    update_owner_and_excludes
     return current
   end
 
+  # @!visibility private
   def update_owner_and_excludes
     if @resource.value(:owner) or @resource.value(:group)
       set_ownership
@@ -321,6 +346,7 @@ Puppet::Type.type(:vcsrepo).provide(:git, :parent => Puppet::Provider::Vcsrepo)
     end
   end
 
+  # @!visibility private
   def git_with_identity(*args)
     if @resource.value(:identity)
       Tempfile.open('git-helper') do |f|
diff --git a/spec/acceptance/modules_660_spec.rb b/spec/acceptance/modules_660_spec.rb
new file mode 100644
index 0000000..c45aa28
--- /dev/null
+++ b/spec/acceptance/modules_660_spec.rb
@@ -0,0 +1,89 @@
+require 'spec_helper_acceptance'
+
+tmpdir = default.tmpdir('vcsrepo')
+
+describe 'MODULES-660' do
+  before(:all) do
+    # Create testrepo.git
+    my_root = File.expand_path(File.join(File.dirname(__FILE__), '..'))
+    shell("mkdir -p #{tmpdir}") # win test
+    scp_to(default, "#{my_root}/acceptance/files/create_git_repo.sh", tmpdir)
+    shell("cd #{tmpdir} && ./create_git_repo.sh")
+
+    # Configure testrepo.git as upstream of testrepo
+    pp = <<-EOS
+    vcsrepo { "#{tmpdir}/testrepo":
+      ensure   => present,
+      provider => git,
+      revision => 'a_branch',
+      source   => "file://#{tmpdir}/testrepo.git",
+    }
+    EOS
+    apply_manifest(pp, :catch_failures => true)
+  end
+
+  after(:all) do
+    shell("rm -rf #{tmpdir}/testrepo.git")
+  end
+
+  shared_examples 'switch to branch/tag/sha' do
+    it 'pulls the new branch commits' do
+      pp = <<-EOS
+      vcsrepo { "#{tmpdir}/testrepo":
+        ensure   => latest,
+        provider => git,
+        revision => 'a_branch',
+        source   => "file://#{tmpdir}/testrepo.git",
+      }
+      EOS
+      apply_manifest(pp, :expect_changes => true)
+      apply_manifest(pp, :catch_changes  => true)
+    end
+    it 'checks out the tag' do
+      pp = <<-EOS
+      vcsrepo { "#{tmpdir}/testrepo":
+        ensure   => latest,
+        provider => git,
+        revision => '0.0.3',
+        source   => "file://#{tmpdir}/testrepo.git",
+      }
+      EOS
+      apply_manifest(pp, :expect_changes => true)
+      apply_manifest(pp, :catch_changes  => true)
+    end
+    it 'checks out the sha' do
+      sha = shell("cd #{tmpdir}/testrepo && git rev-parse origin/master").stdout.chomp
+      pp = <<-EOS
+      vcsrepo { "#{tmpdir}/testrepo":
+        ensure   => latest,
+        provider => git,
+        revision => '#{sha}',
+        source   => "file://#{tmpdir}/testrepo.git",
+      }
+      EOS
+      apply_manifest(pp, :expect_changes => true)
+      apply_manifest(pp, :catch_changes  => true)
+    end
+  end
+
+  context 'on branch' do
+    before :each do
+      shell("cd #{tmpdir}/testrepo && git checkout a_branch")
+      shell("cd #{tmpdir}/testrepo && git reset --hard 0.0.2")
+    end
+    it_behaves_like 'switch to branch/tag/sha'
+  end
+  context 'on tag' do
+    before :each do
+      shell("cd #{tmpdir}/testrepo && git checkout 0.0.1")
+    end
+    it_behaves_like 'switch to branch/tag/sha'
+  end
+  context 'on detached head' do
+    before :each do
+      shell("cd #{tmpdir}/testrepo && git checkout 0.0.2")
+      shell("cd #{tmpdir}/testrepo && git checkout HEAD~1")
+    end
+    it_behaves_like 'switch to branch/tag/sha'
+  end
+end
diff --git a/spec/spec_helper_acceptance.rb b/spec/spec_helper_acceptance.rb
index e566a12..d37c169 100644
--- a/spec/spec_helper_acceptance.rb
+++ b/spec/spec_helper_acceptance.rb
@@ -1,6 +1,6 @@
 require 'beaker-rspec'
 
-unless ENV['RS_PROVISION'] == 'no'
+unless ENV['RS_PROVISION'] == 'no' or ENV['BEAKER_provision'] == 'no'
   hosts.each do |host|
     # Install Puppet
     if host.is_pe?
diff --git a/spec/unit/puppet/provider/vcsrepo/git_spec.rb b/spec/unit/puppet/provider/vcsrepo/git_spec.rb
index 2fd63f0..3f81cc8 100644
--- a/spec/unit/puppet/provider/vcsrepo/git_spec.rb
+++ b/spec/unit/puppet/provider/vcsrepo/git_spec.rb
@@ -1,7 +1,7 @@
 require 'spec_helper'
 
 describe Puppet::Type.type(:vcsrepo).provider(:git_provider) do
-  def branch_a_list(include_branch)
+  def branch_a_list(include_branch = nil?)
     <<branches
 end
 #{"*  master" unless  include_branch.nil?}
@@ -222,51 +222,41 @@ branches
       expects_chdir('/tmp/test')
       resource[:revision] = 'currentsha'
       resource.delete(:source)
-      provider.expects(:git).with('rev-parse', 'HEAD').returns('currentsha')
+      provider.stubs(:git).with('config', 'remote.origin.url').returns('')
+      provider.stubs(:git).with('fetch', 'origin') # FIXME
+      provider.stubs(:git).with('fetch', '--tags', 'origin')
+      provider.stubs(:git).with('rev-parse', 'HEAD').returns('currentsha')
+      provider.stubs(:git).with('branch', '-a').returns(branch_a_list(resource.value(:revision)))
+      provider.stubs(:git).with('tag', '-l').returns("Hello")
     end
 
     context "when its SHA is not different than the current SHA" do
       it "should return the ref" do
-        provider.expects(:git).with('config', 'remote.origin.url').returns('')
-        provider.expects(:git).with('fetch', 'origin') # FIXME
-        provider.expects(:git).with('fetch', '--tags', 'origin')
-        provider.expects(:git).with('rev-parse', '--revs-only', resource.value(:revision)).returns('currentsha')
-        provider.expects(:git).with('tag', '-l').returns("Hello")
+        provider.expects(:git).with('rev-parse', resource.value(:revision)).returns('currentsha')
         provider.revision.should == resource.value(:revision)
       end
     end
 
     context "when its SHA is different than the current SHA" do
       it "should return the current SHA" do
-        provider.expects(:git).with('config', 'remote.origin.url').returns('')
-        provider.expects(:git).with('fetch', 'origin') # FIXME
-        provider.expects(:git).with('fetch', '--tags', 'origin')
-        provider.expects(:git).with('rev-parse', '--revs-only', resource.value(:revision)).returns('othersha')
-        provider.expects(:git).with('tag', '-l').returns("Hello")
-        provider.revision.should == 'currentsha'
+        provider.expects(:git).with('rev-parse', resource.value(:revision)).returns('othersha')
+        provider.revision.should == resource.value(:revision)
       end
     end
 
     context "when its a ref to a remote head" do
       it "should return the revision" do
-        provider.expects(:git).with('config', 'remote.origin.url').returns('')
-        provider.expects(:git).with('fetch', 'origin') # FIXME
-        provider.expects(:git).with('fetch', '--tags', 'origin')
-        provider.expects(:git).with('tag', '-l').returns("Hello")
-        provider.expects(:git).with('rev-parse', '--revs-only', resource.value(:revision)).returns('')
-        provider.expects(:git).with('ls-remote', '--heads', '--tags', 'origin', resource.value(:revision)).returns("newsha refs/heads/#{resource.value(:revision)}")
-        provider.revision.should == 'currentsha'
+        provider.stubs(:git).with('branch', '-a').returns("  remotes/origin/#{resource.value(:revision)}")
+        provider.expects(:git).with('rev-parse', "origin/#{resource.value(:revision)}").returns("newsha")
+        provider.revision.should == resource.value(:revision)
       end
     end
 
     context "when its a ref to non existant remote head" do
       it "should fail" do
-        provider.expects(:git).with('config', 'remote.origin.url').returns('')
-        provider.expects(:git).with('fetch', 'origin') # FIXME
-        provider.expects(:git).with('fetch', '--tags', 'origin')
-        provider.expects(:git).with('tag', '-l').returns("Hello")
+        provider.expects(:git).with('branch', '-a').returns(branch_a_list)
         provider.expects(:git).with('rev-parse', '--revs-only', resource.value(:revision)).returns('')
-        provider.expects(:git).with('ls-remote', '--heads', '--tags', 'origin', resource.value(:revision)).returns('')
+
         expect { provider.revision }.to raise_error(Puppet::Error, /not a local or remote ref$/)
       end
     end
@@ -276,10 +266,7 @@ branches
         resource[:source] = 'git://git@foo.com/bar.git'
         provider.expects(:git).with('config', 'remote.origin.url').returns('old')
         provider.expects(:git).with('config', 'remote.origin.url', 'git://git@foo.com/bar.git')
-        provider.expects(:git).with('fetch', 'origin') # FIXME
-        provider.expects(:git).with('fetch', '--tags', 'origin')
-        provider.expects(:git).with('rev-parse', '--revs-only', resource.value(:revision)).returns('currentsha')
-        provider.expects(:git).with('tag', '-l').returns("Hello")
+        provider.expects(:git).with('rev-parse', resource.value(:revision)).returns('currentsha')
         provider.revision.should == resource.value(:revision)
       end
     end
@@ -295,7 +282,7 @@ branches
         provider.expects(:update_submodules)
         provider.expects(:git).with('branch', '-a').at_least_once.returns(branch_a_list(resource.value(:revision)))
         provider.expects(:git).with('checkout', '--force', resource.value(:revision))
-        provider.expects(:git).with('reset', '--hard', "origin/#{resource.value(:revision)}")
+        provider.expects(:git).with('reset', '--hard',  "origin/#{resource.value(:revision)}")
         provider.revision = resource.value(:revision)
       end
     end
@@ -305,7 +292,7 @@ branches
         provider.expects(:update_submodules)
         provider.expects(:git).with('branch', '-a').at_least_once.returns(resource.value(:revision))
         provider.expects(:git).with('checkout', '--force', resource.value(:revision))
-        provider.expects(:git).with('reset', '--hard', "origin/#{resource.value(:revision)}")
+        provider.expects(:git).with('reset', '--hard',  "origin/#{resource.value(:revision)}")
         provider.revision = resource.value(:revision)
       end
     end
@@ -368,64 +355,23 @@ branches
     end
   end
 
-  context "retrieving the current revision" do
-    before do
-      expects_chdir
-      provider.expects(:git).with('branch','-a').returns("* foo")
-    end
-
-    it "will strip trailing newlines" do
-      provider.expects(:get_revision).with('origin/foo')
-      provider.latest
-    end
-  end
-
   describe 'latest?' do
-    before do
-      expects_chdir('/tmp/test')
-    end
     context 'when true' do
       it do
         provider.expects(:revision).returns('testrev')
-        provider.expects(:latest).returns('testrev')
+        provider.expects(:latest_revision).returns('testrev')
         provider.latest?.should be_true
       end
     end
     context 'when false' do
       it do
         provider.expects(:revision).returns('master')
-        provider.expects(:latest).returns('testrev')
+        provider.expects(:latest_revision).returns('testrev')
         provider.latest?.should be_false
       end
     end
   end
 
-  describe 'latest' do
-    before do
-      provider.expects(:get_revision).returns('master')
-      expects_chdir
-    end
-    context 'on master' do
-      it do
-        provider.expects(:git).with('branch','-a').returns("* master")
-        provider.latest.should == 'master'
-      end
-    end
-    context 'no branch' do
-      it do
-        provider.expects(:git).with('branch','-a').returns("* master")
-
-        provider.latest.should == 'master'
-      end
-    end
-    context 'feature/bar' do
-      it do
-        provider.expects(:git).with('branch','-a').returns("* master")
-        provider.latest.should == 'master'
-      end
-    end
-  end
-
   describe 'convert_working_copy_to_bare' do
     it do
       FileUtils.expects(:mv).returns(true)