Fixing #2273 - file purging works more intuitively

It now correctly purges files whether we're recursing
locally or remotely.

*Please* test various scenarios you can think of with
this.  I've tested:

* Local recursion with no remote source
* Remote recursion with a source
* Recursion with an extra locally managed file

Signed-off-by: Luke Kanies <luke@madstop.com>

4 files changed, 44 insertions, 31 deletions

diff --git a/lib/puppet/transaction.rb b/lib/puppet/transaction.rb
index 37f51b2e5..54163c521 100644
--- a/lib/puppet/transaction.rb
+++ b/lib/puppet/transaction.rb
@@ -349,6 +349,7 @@ class Transaction
                     make_parent_child_relationship(resource, [r])
                 end
             rescue Puppet::Resource::Catalog::DuplicateResourceError
+                made.delete(res)
                 next
             end
         end
diff --git a/lib/puppet/type/file.rb b/lib/puppet/type/file.rb
index f22b19fc3..99f5b96c1 100644
--- a/lib/puppet/type/file.rb
+++ b/lib/puppet/type/file.rb
@@ -513,6 +513,14 @@ module Puppet
 
             @stat = nil
         end
+
+        # Configure discovered resources to be purged.
+        def mark_children_for_purging(children)
+            children.each do |name, child|
+                next if child[:source]
+                child[:ensure] = :absent
+            end
+        end
         
         # Create a new file or directory object as a child to the current
         # object.
@@ -580,6 +588,10 @@ module Puppet
                 recurse_remote(children)
             end
 
+            # If we're purging resources, then delete any resource that isn't on the
+            # remote system.
+            mark_children_for_purging(children) if self.purge?
+
             return children.values.sort { |a, b| a[:path] <=> b[:path] }
         end
 
@@ -661,20 +673,6 @@ module Puppet
                 children[meta.relative_path].parameter(:source).metadata = meta
             end
 
-            # If we're purging resources, then delete any resource that isn't on the
-            # remote system.
-            if self.purge?
-                # Make a hash of all of the resources we found remotely -- all we need is the
-                # fast lookup, the values don't matter.
-                remotes = total.inject({}) { |hash, meta| hash[meta.relative_path] = true; hash }
-
-                children.each do |name, child|
-                    unless remotes.include?(name)
-                        child[:ensure] = :absent
-                    end
-                end
-            end
-
             children
         end
 
diff --git a/spec/unit/transaction.rb b/spec/unit/transaction.rb
index 86ee02ce5..2589f7fc9 100755
--- a/spec/unit/transaction.rb
+++ b/spec/unit/transaction.rb
@@ -48,7 +48,7 @@ describe Puppet::Transaction do
 
             resource.expects(:finish).never
 
-            @transaction.generate_additional_resources(generator, :generate)
+            @transaction.generate_additional_resources(generator, :generate).should be_empty
         end
     end
 end
diff --git a/spec/unit/type/file.rb b/spec/unit/type/file.rb
index 1c6976440..582cc1f81 100755
--- a/spec/unit/type/file.rb
+++ b/spec/unit/type/file.rb
@@ -467,22 +467,6 @@ describe Puppet::Type.type(:file) do
             @file.recurse_remote("first" => @resource)
         end
 
-        describe "and purging is enabled" do
-            before do
-                @file[:purge] = true
-            end
-
-            it "should configure each file not on the remote system to be removed" do
-                @file.stubs(:perform_recursion).returns [@second]
-
-                @resource.expects(:[]=).with(:ensure, :absent)
-
-                @file.expects(:newchild).returns stub('secondfile', :[]= => nil, :parameter => @parameter)
-
-                @file.recurse_remote("first" => @resource)
-            end
-        end
-
         describe "and multiple sources are provided" do
             describe "and :sourceselect is set to :first" do
                 it "should create file instances for the results for the first source to return any values" do
@@ -608,6 +592,36 @@ describe Puppet::Type.type(:file) do
             @file.recurse.should == [one, two, three]
         end
 
+        describe "and purging is enabled" do
+            before do
+                @file[:purge] = true
+            end
+
+            it "should configure each file to be removed" do
+                local = stub 'local'
+                local.stubs(:[]).with(:source).returns nil # Thus, a local file
+                local.stubs(:[]).with(:path).returns "foo"
+                @file.expects(:recurse_local).returns("local" => local)
+                local.expects(:[]=).with(:ensure, :absent)
+
+                @file.recurse
+            end
+
+            it "should not remove files that exist in the remote repository" do
+                @file["source"] = "/my/file"
+                @file.expects(:recurse_local).returns({})
+
+                remote = stub 'remote'
+                remote.stubs(:[]).with(:source).returns "/whatever" # Thus, a remote file
+                remote.stubs(:[]).with(:path).returns "foo"
+
+                @file.expects(:recurse_remote).with { |hash| hash["remote"] = remote }
+                remote.expects(:[]=).with(:ensure, :absent).never
+
+                @file.recurse
+            end
+        end
+
         describe "and making a new child resource" do
             it "should not copy the parent resource's parent" do
                 Puppet::Type.type(:file).expects(:new).with { |options| ! options.include?(:parent) }