Fix for #2910 -- Tidy/matches is too tricky to use

The semantic interaction of tidy/matches and tidy/recurse is tricky to get
right; it only makes sense to use matches with recursion (a fixed path will
either statically match or it won't, no need for a run-time check) but there
was nothing to warn users of this fact.  To compound matters, the example
in the matches parameter doc string even made this mistake.

This patch: 1) fixes the doc string; 2) prohibits the use of match without a
value of recurse capable of generating files to match, 3) fixes tests that
were passing for the wrong reason and adds tests on the prohibition added
in (2).

2 files changed, 59 insertions, 30 deletions

diff --git a/lib/puppet/type/tidy.rb b/lib/puppet/type/tidy.rb
index 3d7190c27..a4b87d5fa 100755
--- a/lib/puppet/type/tidy.rb
+++ b/lib/puppet/type/tidy.rb
@@ -19,6 +19,28 @@ Puppet::Type.newtype(:tidy) do
         isnamevar
     end
 
+    newparam(:recurse) do
+        desc "If target is a directory, recursively descend
+            into the directory looking for files to tidy."
+
+        newvalues(:true, :false, :inf, /^[0-9]+$/)
+
+        # Replace the validation so that we allow numbers in
+        # addition to string representations of them.
+        validate { |arg| }
+        munge do |value|
+            newval = super(value)
+            case newval
+            when :true, :inf; true
+            when :false; false
+            when Integer, Fixnum, Bignum; value
+            when /^\d+$/; Integer(value)
+            else
+                raise ArgumentError, "Invalid recurse value %s" % value.inspect
+            end
+        end
+    end
+
     newparam(:matches) do
         desc "One or more (shell type) file glob patterns, which restrict
             the list of files to be tidied to those whose basenames match
@@ -29,22 +51,28 @@ Puppet::Type.newtype(:tidy) do
 
                     tidy { \"/tmp\":
                         age => \"1w\",
-                        recurse => false,
+                        recurse => 1,
                         matches => [ \"[0-9]pub*.tmp\", \"*.temp\", \"tmpfile?\" ]
                     }
 
             This removes files from \/tmp if they are one week old or older,
             are not in a subdirectory and match one of the shell globs given.
 
-            Note that the patterns are matched against the
-            basename of each file -- that is, your glob patterns should not
-            have any '/' characters in them, since you are only specifying
-            against the last bit of the file."
+            Note that the patterns are matched against the basename of each 
+            file -- that is, your glob patterns should not have any '/' 
+            characters in them, since you are only specifying against the last 
+            bit of the file.
+            
+            Finally, note that you must now specify a non-zero/non-false value 
+            for recurse if matches is used, as matches only apply to files found
+            by recursion (there's no reason to use static patterns match against 
+            a statically determined path).  Requiering explicit recursion clears
+            up a common source of confusion."
 
         # Make sure we convert to an array.
         munge do |value|
-            value = [value] unless value.is_a?(Array)
-            value
+            fail "Tidy can't use matches with recurse 0, false, or undef" if "#{@resource[:recurse]}" =~ /^(0|false|)$/
+            [value].flatten
         end
 
         # Does a given path match our glob patterns, if any?  Return true
@@ -170,28 +198,6 @@ Puppet::Type.newtype(:tidy) do
         defaultto :atime
     end
 
-    newparam(:recurse) do
-        desc "If target is a directory, recursively descend
-            into the directory looking for files to tidy."
-
-        newvalues(:true, :false, :inf, /^[0-9]+$/)
-
-        # Replace the validation so that we allow numbers in
-        # addition to string representations of them.
-        validate { |arg| }
-        munge do |value|
-            newval = super(value)
-            case newval
-            when :true, :inf; true
-            when :false; false
-            when Integer, Fixnum, Bignum; value
-            when /^\d+$/; Integer(value)
-            else
-                raise ArgumentError, "Invalid recurse value %s" % value.inspect
-            end
-        end
-    end
-
     newparam(:rmdirs, :boolean => true) do
         desc "Tidy directories in addition to files; that is, remove
             directories whose age is older than the specified criteria.
diff --git a/spec/unit/type/tidy.rb b/spec/unit/type/tidy.rb
index ccec9ed7c..6eac6a286 100755
--- a/spec/unit/type/tidy.rb
+++ b/spec/unit/type/tidy.rb
@@ -60,6 +60,28 @@ describe tidy do
                 lambda { @tidy[:recurse] = "whatever" }.should raise_error
             end
         end
+
+        describe "for 'matches'" do
+            before do
+                @tidy = Puppet::Type.type(:tidy).new :path => "/tmp", :age => "100d"
+            end
+
+            it "should object if matches is given with recurse is not specified" do
+                lambda { @tidy[:matches] = '*.doh' }.should raise_error
+            end
+            it "should object if matches is given and recurse is 0" do
+                lambda { @tidy[:recurse] = 0; @tidy[:matches] = '*.doh' }.should raise_error
+            end
+            it "should object if matches is given and recurse is false" do
+                lambda { @tidy[:recurse] = false; @tidy[:matches] = '*.doh' }.should raise_error
+            end
+            it "should not object if matches is given and recurse is > 0" do
+                lambda { @tidy[:recurse] = 1; @tidy[:matches] = '*.doh' }.should_not raise_error
+            end
+            it "should not object if matches is given and recurse is true" do
+                lambda { @tidy[:recurse] = true; @tidy[:matches] = '*.doh' }.should_not raise_error
+            end
+        end
     end
 
     describe "when matching files by age" do
@@ -199,7 +221,7 @@ describe tidy do
 
         describe "and determining whether a file matches provided glob patterns" do
             before do
-                @tidy = Puppet::Type.type(:tidy).new :path => "/what/ever"
+                @tidy = Puppet::Type.type(:tidy).new :path => "/what/ever", :recurse => 1
                 @tidy[:matches] = %w{*foo* *bar*}
 
                 @stat = mock 'stat'
@@ -316,6 +338,7 @@ describe tidy do
             end
 
             it "should return false if it does not match any provided globs" do
+                @tidy[:recurse] = 1
                 @tidy[:matches] = "globs"
 
                 matches = @tidy.parameter(:matches)