MODULES-1596 - Repository repeatedly destroyed/created with force

The `retrieve` method was calling `create` and `destroy` on every run
with `force => true`. Retrieve should not be making any changes to the
system, so removed that code, and updated `working_copy_exists` to make
sure that the directory not only contains a `.git` directory, but also
if `source` is specified it also matches `#{path}/.git/config` so that
it will overwrite a git repo with a different source.

Updated tests to not check for the old broken behavior. Added a regression test.

4 files changed, 74 insertions, 27 deletions

diff --git a/spec/acceptance/beaker/git/clone/negative/clone_over_different_exiting_repo.rb b/spec/acceptance/beaker/git/clone/negative/clone_over_different_exiting_repo.rb
index 6826673..1e3b4bb 100644
--- a/spec/acceptance/beaker/git/clone/negative/clone_over_different_exiting_repo.rb
+++ b/spec/acceptance/beaker/git/clone/negative/clone_over_different_exiting_repo.rb
@@ -35,8 +35,7 @@ hosts.each do |host|
     }
     EOS
 
-    apply_manifest_on(host, pp, :catch_failures => true)
-    apply_manifest_on(host, pp, :catch_changes  => true)
+    apply_manifest_on(host, pp, :expect_failures => true)
   end
 
   step 'verify original repo was not replaced' do
diff --git a/spec/acceptance/clone_repo_spec.rb b/spec/acceptance/clone_repo_spec.rb
index f3e77db..4e9293b 100644
--- a/spec/acceptance/clone_repo_spec.rb
+++ b/spec/acceptance/clone_repo_spec.rb
@@ -350,9 +350,7 @@ describe 'clones a remote repo' do
         }
         EOS
 
-        apply_manifest(pp, :catch_changes => true) do |r|
-          expect(r.stdout).to match(/Noop Mode/)
-        end
+        apply_manifest(pp, :catch_changes => true)
       end
     end
   end
diff --git a/spec/acceptance/modules_1596_spec.rb b/spec/acceptance/modules_1596_spec.rb
new file mode 100644
index 0000000..fa36285
--- /dev/null
+++ b/spec/acceptance/modules_1596_spec.rb
@@ -0,0 +1,72 @@
+require 'spec_helper_acceptance'
+
+tmpdir = default.tmpdir('vcsrepo')
+
+describe 'clones a remote repo' do
+  before(:all) do
+    my_root = File.expand_path(File.join(File.dirname(__FILE__), '..'))
+    shell("mkdir -p #{tmpdir}") # win test
+  end
+
+  after(:all) do
+    shell("rm -rf #{tmpdir}/vcsrepo")
+  end
+
+  context 'force with a remote' do
+    it 'clones from remote' do
+      pp = <<-EOS
+      vcsrepo { "#{tmpdir}/vcsrepo":
+        ensure   => present,
+        provider => git,
+        source   => 'https://github.com/puppetlabs/puppetlabs-vcsrepo',
+        force    => true,
+      }
+      EOS
+
+      # Run it twice to test for idempotency
+      apply_manifest(pp, :catch_failures => true)
+      # need to create a file to make sure we aren't destroying the repo
+      # because fun fact, if you call destroy/create in 'retrieve' puppet won't
+      # register that any changes happen, because that method isn't supposed to
+      # be making any changes.
+      shell("touch #{tmpdir}/vcsrepo/foo")
+      apply_manifest(pp, :catch_changes  => true)
+    end
+
+    describe file("#{tmpdir}/vcsrepo/foo") do
+      it { is_expected.to be_file }
+    end
+  end
+
+  context 'force over an existing repo' do
+    it 'clones from remote' do
+      pp = <<-EOS
+      vcsrepo { "#{tmpdir}/vcsrepo":
+        ensure   => present,
+        provider => git,
+        source   => 'https://github.com/puppetlabs/puppetlabs-vcsrepo',
+        force    => true,
+      }
+      EOS
+
+      pp2 = <<-EOS
+      vcsrepo { "#{tmpdir}/vcsrepo":
+        ensure   => present,
+        provider => git,
+        source   => 'https://github.com/puppetlabs/puppetlabs-stdlib',
+        force    => true,
+      }
+      EOS
+
+
+      apply_manifest(pp, :catch_failures => true)
+      # create a file to make sure we're destroying the repo
+      shell("touch #{tmpdir}/vcsrepo/foo")
+      apply_manifest(pp2, :catch_failures  => true)
+    end
+
+    describe file("#{tmpdir}/vcsrepo/foo") do
+      it { is_expected.to_not be_file }
+    end
+  end
+end
diff --git a/spec/unit/puppet/provider/vcsrepo/git_spec.rb b/spec/unit/puppet/provider/vcsrepo/git_spec.rb
index 122eb62..edc7202 100644
--- a/spec/unit/puppet/provider/vcsrepo/git_spec.rb
+++ b/spec/unit/puppet/provider/vcsrepo/git_spec.rb
@@ -175,28 +175,6 @@ branches
         provider.expects(:git).with('checkout', '--force', resource.value(:revision))
         provider.create
       end
-      it "should warn about destroying it using force and noop attribute" do
-        resource[:force] = true
-        resource[:noop] = true
-        resource.delete(:revision)
-        provider.expects(:working_copy_exists?).returns(true)
-
-        provider.expects(:destroy).never
-        provider.expects(:create).never
-        Puppet::Type::Vcsrepo::Ensure.any_instance.expects(:send_log).with(:notice, "Noop Mode - Would have deleted repository and re-created from latest")
-        provider.resource.retrieve
-      end
-      it "should warn about destroying it using force and global noop" do
-        resource[:force] = true
-        Puppet[:noop] = true
-        resource.delete(:revision)
-        provider.expects(:working_copy_exists?).returns(true)
-
-        provider.expects(:destroy).never
-        provider.expects(:create).never
-        Puppet::Type::Vcsrepo::Ensure.any_instance.expects(:send_log).with(:notice, "Noop Mode - Would have deleted repository and re-created from latest")
-        provider.resource.retrieve
-      end
     end
 
     context "when the path is not empty and not a repository" do