Hopefully fixing #355. I could not actually reproduce the specific problem, but I found a couple of issues around the problem and they are all gone now.

git-svn-id: https://reductivelabs.com/svn/puppet/trunk@1970 980ebf18-57e1-0310-9a29-db15c13687c0

3 files changed, 197 insertions, 143 deletions

diff --git a/lib/puppet/type/pfile.rb b/lib/puppet/type/pfile.rb
index cc3950743..0bb67d5bb 100644
--- a/lib/puppet/type/pfile.rb
+++ b/lib/puppet/type/pfile.rb
@@ -870,7 +870,7 @@ module Puppet
             method = :stat
 
             # Files are the only types that support links
-            if self.class.name == :file and self[:links] != :follow
+            if (self.class.name == :file and self[:links] != :follow) or self.class.name == :tidy
                 method = :lstat
             end
             path = self[:path]
diff --git a/lib/puppet/type/tidy.rb b/lib/puppet/type/tidy.rb
index cebbde74a..cc3258766 100755
--- a/lib/puppet/type/tidy.rb
+++ b/lib/puppet/type/tidy.rb
@@ -6,7 +6,7 @@ require 'puppet/type/pfile'
 module Puppet
     newtype(:tidy, Puppet.type(:file)) do
         @doc = "Remove unwanted files based on specific criteria.  Multiple
-            criteria or OR'd together, so a file that is too large but is not
+            criteria are OR'd together, so a file that is too large but is not
             old enough will still get tidied."
 
         newparam(:path) do
@@ -16,10 +16,108 @@ module Puppet
         end
 
         copyparam(Puppet.type(:file), :backup)
+        
+        newstate(:ensure) do
+            desc "An internal attribute used to determine which files should be removed."
+            require 'etc'
+
+            @nodoc = true
+            
+            TATTRS = [:age, :size]
+            
+            defaultto :anything # just so we always get this state
+
+            def change_to_s
+                start = "Tidying"
+                if @out.include?(:age)
+                    start += ", older than %s seconds" % @parent.should(:age)
+                end
+                if @out.include?(:size)
+                    start += ", larger than %s bytes" % @parent.should(:size)
+                end
+
+                start
+            end
 
-        newparam(:age) do
+            def insync?
+                if @is.is_a?(Symbol)
+                    if [:absent, :notidy].include?(@is)
+                        return true
+                    else
+                        return false
+                    end
+                else
+                    @out = []
+                    TATTRS.each do |param|
+                        if state = @parent.state(param)
+                            unless state.insync?
+                                @out << param
+                            end
+                        end
+                    end
+                    if @out.length > 0
+                        return false
+                    else
+                        return true
+                    end
+                end
+            end
+
+            def retrieve
+                stat = nil
+                unless stat = @parent.stat
+                    @is = :absent
+                    return
+                end
+                
+                if stat.ftype == "directory" and ! @parent[:rmdirs]
+                    @is = :notidy
+                    return
+                end
+
+                TATTRS.each { |param|
+                    if state = @parent.state(param)
+                        state.is = state.assess(stat)
+                    end
+                }
+            end
+
+            def sync
+                file = @parent[:path]
+                case File.lstat(file).ftype
+                when "directory":
+                    if @parent[:rmdirs]
+                        subs = Dir.entries(@parent[:path]).reject { |d|
+                            d == "." or d == ".."
+                        }.length
+                        if subs > 0
+                            self.info "%s has %s children; not tidying" %
+                                [@parent[:path], subs]
+                            self.info Dir.entries(@parent[:path]).inspect
+                        else
+                            Dir.rmdir(@parent[:path])
+                        end
+                    else
+                        self.debug "Not tidying directories"
+                        return nil
+                    end
+                when "file":
+                    @parent.handlebackup(file)
+                    File.unlink(file)
+                when "link":
+                    File.unlink(file)
+                else
+                    self.fail "Cannot tidy files of type %s" %
+                        File.lstat(file).ftype
+                end
+
+                return :file_tidied
+            end
+        end
+
+        newstate(:age) do
             desc "Tidy files whose age is equal to or greater than
-                the specified number of days.  You can choose seconds, minutes,
+                the specified time.  You can choose seconds, minutes,
                 hours, days, or weeks by specifying the first letter of any
                 of those words (e.g., '1w')."
 
@@ -32,6 +130,18 @@ module Puppet
             @@ageconvertors[:d] = @@ageconvertors[:h] * 24
             @@ageconvertors[:w] = @@ageconvertors[:d] * 7
 
+            def assess(stat)
+                type = nil
+                if stat.ftype == "directory"
+                    type = :mtime
+                else
+                    type = @parent[:type] || :atime
+                end
+                
+                #return Integer(Time.now - stat.send(type))
+                return stat.send(type).to_i
+            end
+
             def convert(unit, multi)
                 if num = @@ageconvertors[unit]
                     return num * multi
@@ -40,6 +150,14 @@ module Puppet
                 end
             end
 
+            def insync?
+                if (Time.now.to_i - @is) > self.should
+                    return false
+                end
+
+                true
+            end
+
             munge do |age|
                 unit = multi = nil
                 case age
@@ -57,7 +175,7 @@ module Puppet
             end
         end
 
-        newparam(:size) do
+        newstate(:size) do
             desc "Tidy files whose size is equal to or greater than
                 the specified size.  Unqualified values are in kilobytes, but
                 *b*, *k*, and *m* can be appended to specify *bytes*, *kilobytes*,
@@ -71,6 +189,11 @@ module Puppet
                 :g => 3
             }
 
+            # Retrieve the size from a File::Stat object
+            def assess(stat)
+                return stat.size
+            end
+
             def convert(unit, multi)
                 if num = @@sizeconvertors[unit]
                     result = multi
@@ -81,6 +204,14 @@ module Puppet
                 end
             end
             
+            def insync?
+                if @is > self.should
+                    return false
+                end
+
+                true
+            end
+            
             munge do |size|
                 case size
                 when /^([0-9]+)(\w)\w*$/:
@@ -116,119 +247,7 @@ module Puppet
                 This will only remove empty directories, so all contained
                 files must also be tidied before a directory gets removed."
         end
-
-        newstate(:tidyup) do
-            require 'etc'
-
-            @nodoc = true
-            @name = :tidyup
-
-            def age(stat)
-                type = nil
-                if stat.ftype == "directory"
-                    type = :mtime
-                else
-                    type = @parent[:type] || :atime
-                end
-                
-                #return Integer(Time.now - stat.send(type))
-                return stat.send(type).to_i
-            end
-
-            def change_to_s
-                start = "Tidying"
-                unless insync_age?
-                    start += ", older than %s seconds" % @parent[:age]
-                end
-                unless insync_size?
-                    start += ", larger than %s bytes" % @parent[:size]
-                end
-
-                start
-            end
-
-            def insync_age?
-                if num = @parent[:age] and @is[0]
-                    if (Time.now.to_i - @is[0]) > num
-                        return false
-                    end
-                end
-
-                true
-            end
-
-            def insync_size?
-                if num = @parent[:size] and @is[1]
-                    if @is[1] > num
-                        return false
-                    end
-                end
-
-                true
-            end
-
-            def insync?
-                if @is.is_a?(Symbol)
-                    if @is == :absent
-                        return true
-                    else
-                        return false
-                    end
-                else
-                    insync_age? and insync_size?
-                end
-            end
-
-            def retrieve
-                stat = nil
-                unless stat = @parent.stat
-                    @is = :unknown
-                    return
-                end
-
-                @is = [:age, :size].collect { |param|
-                    if @parent[param]
-                        self.send(param, stat)
-                    end
-                }.reject { |p| p == false or p.nil? }
-            end
-
-            def size(stat)
-                return stat.size
-            end
-
-            def sync
-                file = @parent[:path]
-                case File.lstat(file).ftype
-                when "directory":
-                    if @parent[:rmdirs]
-                        subs = Dir.entries(@parent[:path]).reject { |d|
-                            d == "." or d == ".."
-                        }.length
-                        if subs > 0
-                            self.info "%s has %s children; not tidying" %
-                                [@parent[:path], subs]
-                            self.info Dir.entries(@parent[:path]).inspect
-                        else
-                            Dir.rmdir(@parent[:path])
-                        end
-                    else
-                        self.debug "Not tidying directories"
-                        return nil
-                    end
-                when "file":
-                    @parent.handlebackup(file)
-                    File.unlink(file)
-                when "link":     File.unlink(file)
-                else
-                    self.fail "Cannot tidy files of type %s" %
-                        File.lstat(file).ftype
-                end
-
-                return :file_tidied
-            end
-        end
-
+        
         # Erase PFile's validate method
         validate do
         end
@@ -241,10 +260,9 @@ module Puppet
 
         def initialize(hash)
             super
-            #self.setdefaults
 
-            unless  @parameters.include?(:age) or
-                    @parameters.include?(:size)
+            unless  @states.include?(:age) or
+                    @states.include?(:size)
                 unless FileTest.directory?(self[:path])
                     # don't do size comparisons for directories
                     self.fail "Tidy must specify size, age, or both"
@@ -255,11 +273,17 @@ module Puppet
             unless self[:backup].is_a? Puppet::Client::Dipper
                 self[:backup] = false
             end
-            self[:tidyup] = [:age, :size].collect { |param|
-                self[param]
-            }.reject { |p| p == false }
         end
-
+        
+        def retrieve
+            # Our ensure state knows how to retrieve everything for us.
+            @states[:ensure].retrieve
+        end
+        
+        # Hack things a bit so we only ever check the ensure state.
+        def states
+            []
+        end
     end
 end
 
diff --git a/test/types/tidy.rb b/test/types/tidy.rb
index 513b05319..65ffa7d82 100755
--- a/test/types/tidy.rb
+++ b/test/types/tidy.rb
@@ -35,13 +35,10 @@ class TestTidy < Test::Unit::TestCase
         tidy = Puppet.type(:tidy).create(
             :name => dir,
             :size => "1b",
-            :age => "1s",
             :rmdirs => true,
             :recurse => true
         )
 
-
-        sleep(2)
         assert_events([:file_tidied, :file_tidied], tidy)
 
         assert(!FileTest.exists?(file), "Tidied %s still exists" % file)
@@ -93,7 +90,7 @@ class TestTidy < Test::Unit::TestCase
             tidy[:age] = "2"
         end
 
-        assert_equal(2 * convertors[:day], tidy[:age],
+        assert_equal(2 * convertors[:day], tidy.should(:age),
             "Converted 2 wrong")
 
         convertors.each do |name, number|
@@ -105,7 +102,7 @@ class TestTidy < Test::Unit::TestCase
                         tidy[:age] = age
                     end
 
-                    assert_equal(multi * convertors[name], tidy[:age],
+                    assert_equal(multi * convertors[name], tidy.should(:age),
                         "Converted %s wrong" % age)
                 end
             end
@@ -127,7 +124,7 @@ class TestTidy < Test::Unit::TestCase
             tidy[:size] = "2"
         end
 
-        assert_equal(2048, tidy[:size],
+        assert_equal(2048, tidy.should(:size),
             "Converted 2 wrong")
 
         convertors.each do |name, number|
@@ -142,7 +139,7 @@ class TestTidy < Test::Unit::TestCase
                     total = multi
                     number.times do total *= 1024 end
 
-                    assert_equal(total, tidy[:size],
+                    assert_equal(total, tidy.should(:size),
                         "Converted %s wrong" % size)
                 end
             end
@@ -152,33 +149,33 @@ class TestTidy < Test::Unit::TestCase
     def test_agetest
         tidy = Puppet::Type.newtidy :path => tempfile(), :age => "1m"
 
-        state = tidy.state(:tidyup)
+        age = tidy.state(:age)
 
         # Set it to something that should be fine
-        state.is = [Time.now.to_i - 5, 50]
+        age.is = Time.now.to_i - 5
 
-        assert(state.insync?, "Tried to tidy a low age")
+        assert(age.insync?, "Tried to tidy a low age")
 
         # Now to something that should fail
-        state.is = [Time.now.to_i - 120, 50]
+        age.is = Time.now.to_i - 120
 
-        assert(! state.insync?, "Incorrectly skipped tidy")
+        assert(! age.insync?, "Incorrectly skipped tidy")
     end
 
     def test_sizetest
         tidy = Puppet::Type.newtidy :path => tempfile(), :size => "1k"
 
-        state = tidy.state(:tidyup)
+        size = tidy.state(:size)
 
         # Set it to something that should be fine
-        state.is = [5, 50]
+        size.is = 50
 
-        assert(state.insync?, "Tried to tidy a low size")
+        assert(size.insync?, "Tried to tidy a low size")
 
         # Now to something that should fail
-        state.is = [120, 2048]
+        size.is = 2048
 
-        assert(! state.insync?, "Incorrectly skipped tidy")
+        assert(! size.insync?, "Incorrectly skipped tidy")
     end
 
     # Make sure we can remove different types of files
@@ -201,11 +198,44 @@ class TestTidy < Test::Unit::TestCase
 
         # And a directory
         Dir.mkdir(path)
-        tidy.is = [:tidyup, [Time.now - 1024, 1]]
+        tidy.is = [:ensure, [Time.now - 1024, 1]]
         tidy[:rmdirs] = true
         assert_events([:file_tidied], tidy)
         assert(! FileTest.exists?(path), "File was not removed")
     end
+    
+    # Make sure we can specify either attribute and get appropriate behaviour.
+    # I found that the original implementation of this did not work unless both
+    # size and age were specified.
+    def test_one_attribute
+        path = tempfile()
+        File.open(path, "w") { |f| 10.times { f.puts "yayness " } }
+        tidy = Puppet::Type.type(:tidy).create :path => path, :size => "1b"
+        
+        assert_apply(tidy)
+        assert(! FileTest.exists?(path), "file did not get tidied")
+        
+        # Now try one with just an age attribute.
+        File.open(path, "w") { |f| 10.times { f.puts "yayness " } }
+        tidy = Puppet::Type.type(:tidy).create :path => path, :age => "5s"
+        
+        tidy.state(:age).is = "0s"
+        assert_apply(tidy)
+        assert(! FileTest.exists?(path), "file did not get tidied")
+    end
+    
+    # Testing #355.
+    def test_remove_dead_links
+        dir = tempfile()
+        link = File.join(dir, "link")
+        target = tempfile()
+        Dir.mkdir(dir)
+        File.symlink(target, link)
+        
+        tidy = Puppet::Type.newtidy :path => dir, :size => "1b", :recurse => true
+        assert_apply(tidy)
+        assert(! FileTest.symlink?(link), "link was not tidied")
+    end
 end
 
 # $Id$