Converting the file 'source' property to a parameter.

This makes a lot of sense because source was always
more of a metaparameter than a property -- it affected
the 'should' values of other properties, but it shouldn't
have done any other work.

It will hopefully make everything else much cleaner.

This is such a large commit mostly because of the need
to fix a lot of tests.

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

6 files changed, 105 insertions, 127 deletions

diff --git a/lib/puppet/parameter.rb b/lib/puppet/parameter.rb
index 40bb32ff6..f82438903 100644
--- a/lib/puppet/parameter.rb
+++ b/lib/puppet/parameter.rb
@@ -487,15 +487,6 @@ class Puppet::Parameter
         @value = munge(value)
     end
 
-    def inspect
-        s = "Parameter(%s = %s" % [self.name, self.value || "nil"]
-        if defined? @resource
-            s += ", @resource = %s)" % @resource
-        else
-            s += ")"
-        end
-    end
-
     # Retrieve the resource's provider.  Some types don't have providers, in which
     # case we return the resource object itself.
     def provider
diff --git a/lib/puppet/property.rb b/lib/puppet/property.rb
index 96f6ff1bf..b8dbd6e77 100644
--- a/lib/puppet/property.rb
+++ b/lib/puppet/property.rb
@@ -188,16 +188,6 @@ class Puppet::Property < Puppet::Parameter
         end
     end
 
-    def inspect
-        str = "Property('%s', " % self.name
-
-        if defined? @should and @should
-            str += "@should = '%s')" % @should.join(", ")
-        else
-            str += "@should = nil)"
-        end
-    end
-
     # Determine whether the property is in-sync or not.  If @should is
     # not defined or is set to a non-true value, then we do not have
     # a valid value for it and thus consider the property to be in-sync
diff --git a/lib/puppet/type/file.rb b/lib/puppet/type/file.rb
index 6038aa50b..dfb909f3f 100644
--- a/lib/puppet/type/file.rb
+++ b/lib/puppet/type/file.rb
@@ -240,6 +240,9 @@ module Puppet
             CREATORS.each do |param|
                 count += 1 if self.should(param)
             end
+            if @parameters.include?(:source)
+                count += 1
+            end
             if count > 1
                 self.fail "You cannot specify more than one of %s" % CREATORS.collect { |p| p.to_s}.join(", ")
             end
@@ -293,6 +296,12 @@ module Puppet
             return asuser
         end
 
+        # Does the file currently exist?  Just checks for whether
+        # we have a stat
+        def exist?
+            stat ? true : false
+        end
+
         # We have to do some extra finishing, to retrieve our bucket if
         # there is one.
         def finish
@@ -478,6 +487,7 @@ module Puppet
             end
 
             return self.class.create(options)
+            #return catalog.create_implicit_resource(:file, options)
         end
 
         # Files handle paths specially, because they just lengthen their
@@ -576,6 +586,7 @@ module Puppet
 
             total = self[:source].collect do |source|
                 next unless result = perform_recursion(source)
+                return if top = result.find { |r| r.relative_path == "." } and top.ftype != "directory"
                 result.each { |data| data.source = "%s/%s" % [source, data.relative_path] }
                 break result if result and ! result.empty? and sourceselect == :first
                 result
@@ -593,14 +604,14 @@ module Puppet
 
             total.each do |meta|
                 if meta.relative_path == "."
-                    property(:source).metadata = meta
+                    parameter(:source).metadata = meta
                     next
                 end
                 children[meta.relative_path] ||= newchild(meta.relative_path)
                 children[meta.relative_path][:source] = meta.source
                 children[meta.relative_path][:checksum] = :md5 if meta.ftype == "file"
 
-                children[meta.relative_path].property(:source).metadata = meta
+                children[meta.relative_path].parameter(:source).metadata = meta
             end
 
             # If we're purging resources, then delete any resource that isn't on the
@@ -681,22 +692,10 @@ module Puppet
 
         # a wrapper method to make sure the file exists before doing anything
         def retrieve
-            unless stat = self.stat(true)
-
-                propertyvalues = properties().inject({}) { |hash, property|
-                                     hash[property] = :absent
-                                     hash
-                                  }
-
-                # If the file doesn't exist but we have a source, then call
-                # set our 'should' values based on the source file.
-                if @parameters.include?(:source)
-                    @parameters[:source].copy_source_values
-                end
-                return propertyvalues
+            if source = parameter(:source)
+                source.copy_source_values
             end
-
-            currentpropvalues()
+            super
         end
 
         # Set the checksum, from another property.  There are multiple
@@ -715,6 +714,28 @@ module Puppet
             end
         end
 
+        # Should this thing be a normal file?  This is a relatively complex
+        # way of determining whether we're trying to create a normal file,
+        # and it's here so that the logic isn't visible in the content property.
+        def should_be_file?
+            return true if self[:ensure] == :file
+
+            # I.e., it's set to something like "directory"
+            return false if e = self[:ensure] and e != :present
+
+            # The user doesn't really care, apparently
+            if self[:ensure] == :present
+                return true unless s = stat
+                return true if s.ftype == "file"
+                return false
+            end
+
+            # If we've gotten here, then :ensure isn't set
+            return true if self[:content]
+            return true if stat and stat.ftype == "file"
+            return false
+        end
+
         # Stat our file.  Depending on the value of the 'links' attribute, we
         # use either 'stat' or 'lstat', and we expect the properties to use the
         # resulting stat object accordingly (mostly by testing the 'ftype'
diff --git a/lib/puppet/type/file/content.rb b/lib/puppet/type/file/content.rb
index 1eb1423aa..169ba9078 100755
--- a/lib/puppet/type/file/content.rb
+++ b/lib/puppet/type/file/content.rb
@@ -1,6 +1,9 @@
+require 'puppet/util/checksums'
+
 module Puppet
     Puppet::Type.type(:file).newproperty(:content) do
         include Puppet::Util::Diff
+        include Puppet::Util::Checksums
 
         desc "Specify the contents of a file as a string.  Newlines, tabs, and
             spaces can be specified using the escaped syntax (e.g., \\n for a
@@ -23,7 +26,11 @@ module Puppet
             `PuppetTemplating templating`:trac:."
 
         def change_to_s(currentvalue, newvalue)
-            newvalue = "{md5}" + Digest::MD5.hexdigest(newvalue)
+            if source = resource.parameter(:source)
+                newvalue = source.metadata.checksum
+            else
+                newvalue = "{md5}" + Digest::MD5.hexdigest(newvalue)
+            end
             if currentvalue == :absent
                 return "created file with contents %s" % newvalue
             else
@@ -31,18 +38,40 @@ module Puppet
                 return "changed file contents from %s to %s" % [currentvalue, newvalue]
             end
         end
+        
+        def content
+            self.should || (s = resource.parameter(:source) and s.content)
+        end
 
         # Override this method to provide diffs if asked for.
         # Also, fix #872: when content is used, and replace is true, the file
         # should be insync when it exists
         def insync?(is)
-            if ! @resource.replace? and File.exists?(@resource[:path])
+            if resource.should_be_file?
+                return false if is == :absent
+            else
+                return true
+            end
+
+            return true if ! @resource.replace?
+
+            if self.should
+                return super
+            elsif source = resource.parameter(:source)
+                unless sum_method = sumtype(source.checksum)
+                    fail "Could not extract checksum type from source checksum '%s'" % source.checksum
+                end
+
+                newsum = "{%s}" % sum_method + send(sum_method, is)
+                result = (newsum == source.checksum)
+            else
+                # We've got no content specified, and no source from which to
+                # get content.
                 return true
             end
 
-            result = super
-            if ! result and Puppet[:show_diff] and File.exists?(@resource[:path])
-                string_file_diff(@resource[:path], self.should)
+            if ! result and Puppet[:show_diff]
+                string_file_diff(@resource[:path], content)
             end
             return result
         end
@@ -50,31 +79,31 @@ module Puppet
         def retrieve
             return :absent unless stat = @resource.stat
 
-            return self.should if stat.ftype == "link" and @resource[:links] == :ignore
-
             # Don't even try to manage the content on directories
             return nil if stat.ftype == "directory"
 
             begin
-                currentvalue = File.read(@resource[:path])
-                return currentvalue
+                return File.read(@resource[:path])
             rescue => detail
-                raise Puppet::Error, "Could not read %s: %s" %
-                    [@resource.title, detail]
+                raise Puppet::Error, "Could not read %s: %s" % [@resource.title, detail]
             end
         end
 
         # Make sure we're also managing the checksum property.
         def should=(value)
             super
-            @resource.newattr(:checksum) unless @resource.property(:checksum)
+            @resource.newattr(:checksum) unless @resource.parameter(:checksum)
         end
 
         # Just write our content out to disk.
         def sync
             return_event = @resource.stat ? :file_changed : :file_created
             
-            @resource.write(self.should, :content)
+            # We're safe not testing for the 'source' if there's no 'should'
+            # because we wouldn't have gotten this far if there weren't at least
+            # one valid value somewhere.
+            content = self.should || resource.parameter(:source).content
+            @resource.write(content, :content)
 
             return return_event
         end
diff --git a/lib/puppet/type/file/ensure.rb b/lib/puppet/type/file/ensure.rb
index a9ddc2dba..ea89a7d83 100755
--- a/lib/puppet/type/file/ensure.rb
+++ b/lib/puppet/type/file/ensure.rb
@@ -111,10 +111,8 @@ module Puppet
         end
 
         def change_to_s(currentvalue, newvalue)
-            if property = (@resource.property(:content) || @resource.property(:source)) and ! property.insync?(currentvalue)
-                currentvalue = property.retrieve
-                
-                return property.change_to_s(property.retrieve, property.should)
+            if property = @resource.property(:content) and ! property.insync?(currentvalue)
+                return property.change_to_s(currentvalue, property.should)
             else
                 super(currentvalue, newvalue)
             end
diff --git a/lib/puppet/type/file/source.rb b/lib/puppet/type/file/source.rb
index 04a6931f9..ba5fcbd0e 100755
--- a/lib/puppet/type/file/source.rb
+++ b/lib/puppet/type/file/source.rb
@@ -8,7 +8,7 @@ module Puppet
     # the file down.  If the remote file is a dir or a link or whatever, then
     # this state, during retrieval, modifies the appropriate other states
     # so that things get taken care of appropriately.
-    Puppet.type(:file).newproperty(:source) do
+    Puppet.type(:file).newparam(:source) do
         include Puppet::Util::Diff
 
         attr_accessor :source, :local
@@ -63,27 +63,24 @@ module Puppet
             to ``follow`` if any remote sources are links.
             "
 
-        uncheckable
-        
-        validate do |source|
-            begin
-                uri = URI.parse(URI.escape(source))
-            rescue => detail
-                self.fail "Could not understand source %s: %s" % [source, detail.to_s]
-            end
+        validate do |sources|
+            sources = [sources] unless sources.is_a?(Array)
+            sources.each do |source|
+                begin
+                    uri = URI.parse(URI.escape(source))
+                rescue => detail
+                    self.fail "Could not understand source %s: %s" % [source, detail.to_s]
+                end
 
-            unless uri.scheme.nil? or %w{file puppet}.include?(uri.scheme)
-                self.fail "Cannot use URLs of type '%s' as source for fileserving" % [uri.scheme]
+                unless uri.scheme.nil? or %w{file puppet}.include?(uri.scheme)
+                    self.fail "Cannot use URLs of type '%s' as source for fileserving" % [uri.scheme]
+                end
             end
         end
             
-        munge do |source|
-            # if source.is_a? Symbol
-            #     return source
-            # end
-
-            # Remove any trailing slashes
-            source.sub(/\/$/, '')
+        munge do |sources|
+            sources = [sources] unless sources.is_a?(Array)
+            sources.collect { |source| source.sub(/\/$/, '') }
         end
         
         def change_to_s(currentvalue, newvalue)
@@ -124,6 +121,7 @@ module Puppet
             # if a value has not already been provided.
             [:owner, :mode, :group, :checksum].each do |param|
                 next if param == :owner and Puppet::Util::SUIDManager.uid != 0
+                next if param == :checksum and metadata.ftype == "directory"
                 unless value = @resource[param] and value != :absent
                     @resource[param] = metadata.send(param)
                 end
@@ -143,34 +141,8 @@ module Puppet
             @content = nil
         end
 
-        # Use the remote metadata to see if we're in sync.
-        # LAK:NOTE This method should still get refactored.
-        def insync?(currentvalue)
-            # the only thing this actual state can do is copy files around.  Therefore,
-            # only pay attention if the remote is a file.
-            return true unless metadata.ftype == "file" 
-            
-            # The file is not in sync if it doesn't even exist.
-            return false unless resource.stat
-            
-            # The file is considered in sync if it exists and 'replace' is false.
-            return true unless resource.replace?
-
-            # Now, we just check to see if the checksums are the same
-            parentchecksum = @resource.property(:checksum).retrieve
-            result = (!parentchecksum.nil? and (parentchecksum == metadata.checksum))
-
-            # Diff the contents if they ask it.  This is quite annoying -- we need to do this in
-            # 'insync?' because they might be in noop mode, but we don't want to do the file
-            # retrieval twice, so we cache the value.
-            if ! result and Puppet[:show_diff] and File.exists?(@resource[:path])
-                string_file_diff(@resource[:path], content)
-            end
-            return result
-        end
-
         def pinparams
-            [:mode, :type, :owner, :group]
+            [:mode, :type, :owner, :group, :content]
         end
 
         def found?
@@ -183,8 +155,8 @@ module Puppet
         attr_writer :metadata
         def metadata
             unless defined?(@metadata) and @metadata
-                return @metadata = nil unless should
-                should.each do |source|
+                return @metadata = nil unless value
+                value.each do |source|
                     begin
                         if data = Puppet::FileServing::Metadata.find(source)
                             @metadata = data
@@ -195,43 +167,20 @@ module Puppet
                         fail detail, "Could not retrieve file metadata for %s: %s" % [source, detail]
                     end
                 end
-                fail "Could not retrieve information from source(s) %s" % @should.join(", ") unless @metadata
+                fail "Could not retrieve information from source(s) %s" % value.join(", ") unless @metadata
             end
             return @metadata
         end
 
-        # Just call out to our copy method.  Hopefully we'll refactor 'source' to
-        # be a parameter soon, in which case 'retrieve' is unnecessary.
-        def retrieve
-            copy_source_values
-        end
-        
-        # Return the whole array, rather than the first item.
-        def should
-            @should
-        end
-        
         # Make sure we're also checking the checksum
-        def should=(value)
+        def value=(value)
             super
 
             checks = (pinparams + [:ensure])
             checks.delete(:checksum)
             
-            @resource[:check] = checks
-            @resource[:checksum] = :md5 unless @resource.property(:checksum)
-        end
-
-        def sync
-            exists = FileTest.exist?(@resource[:path])
-
-            @resource.write(content, :source, @metadata.checksum)
-
-            if exists
-                return :file_changed
-            else
-                return :file_created
-            end
+            resource[:check] = checks
+            resource[:checksum] = :md5 unless resource.property(:checksum)
         end
     end
 end