From d1746054b8617271941e7307b2ecee0b61976818 Mon Sep 17 00:00:00 2001
From: Luke Kanies <luke@madstop.com>
Date: Thu, 21 Aug 2008 22:41:01 -0500
Subject: Fixing a test that relied on hash ordering.

Signed-off-by: Luke Kanies <luke@madstop.com>
---
 spec/unit/indirector/rest.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/spec/unit/indirector/rest.rb b/spec/unit/indirector/rest.rb
index 3bd63359d..0c88ea4df 100755
--- a/spec/unit/indirector/rest.rb
+++ b/spec/unit/indirector/rest.rb
@@ -158,7 +158,7 @@ describe Puppet::Indirector::REST do
         it "should include all options in the query string" do
             @request.stubs(:options).returns(:one => "two", :three => "four")
             should_path = "/%s/%s" % [@indirection.name.to_s, "foo"]
-            @connection.expects(:get).with { |path, args| path =~ /\?one=two&three=four$/ }.returns(@response)
+            @connection.expects(:get).with { |path, args| path =~ /\?one=two&three=four$/ or path =~ /\?three=four&one=two$/ }.returns(@response)
             @searcher.find(@request)
         end
 
@@ -220,7 +220,7 @@ describe Puppet::Indirector::REST do
             @request.stubs(:options).returns(:one => "two", :three => "four")
 
             should_path = "/%s/%s" % [@indirection.name.to_s, "foo"]
-            @connection.expects(:get).with { |path, args| path =~ /\?one=two&three=four$/ }.returns(@response)
+            @connection.expects(:get).with { |path, args| path =~ /\?one=two&three=four$/ or path =~ /\?three=four&one=two$/ }.returns(@response)
             @searcher.search(@request)
         end
 
-- 
cgit 


From a215abaef97ea1fb0187f46c5e6a880ff1d29036 Mon Sep 17 00:00:00 2001
From: Luke Kanies <luke@madstop.com>
Date: Thu, 21 Aug 2008 23:09:27 -0500
Subject: Dividing server/port configuration responsibility between the REST
 terminus and the indirection request.

Previously, the REST terminus did all of the configuration,
but it required rewriting the request key if it was a URI
because you can't have a uri in a uri (i.e., you can't use
http://host/puppet://host/dist/file).

Now the request parses the URI and sets host/port/key/protocol
appropriately, and the REST terminus has its own overrides
and defaults so that subclasses like certificate classes
can provide specific values.

Signed-off-by: Luke Kanies <luke@madstop.com>
---
 lib/puppet/indirector/request.rb | 36 +++++++++++++++++-
 lib/puppet/indirector/rest.rb    | 40 +++++++++++++-------
 spec/unit/indirector/request.rb  | 31 ++++++++++++++++
 spec/unit/indirector/rest.rb     | 80 ++++++++++++++++++++++++----------------
 4 files changed, 141 insertions(+), 46 deletions(-)

diff --git a/lib/puppet/indirector/request.rb b/lib/puppet/indirector/request.rb
index 98fa38885..f1d02cead 100644
--- a/lib/puppet/indirector/request.rb
+++ b/lib/puppet/indirector/request.rb
@@ -5,6 +5,8 @@ require 'puppet/indirector'
 class Puppet::Indirector::Request
     attr_accessor :indirection_name, :key, :method, :options, :instance, :node, :ip, :authenticated
 
+    attr_accessor :server, :port, :uri, :protocol
+
     # Is this an authenticated request?
     def authenticated?
         # Double negative, so we just get true or false
@@ -28,7 +30,15 @@ class Puppet::Indirector::Request
         end
 
         if key.is_a?(String) or key.is_a?(Symbol)
-            @key = key
+            # If the request key is a URI, then we need to treat it specially,
+            # because it rewrites the key.  We could otherwise strip server/port/etc
+            # info out in the REST class, but it seemed bad design for the REST
+            # class to rewrite the key.
+            if key.to_s =~ /^\w+:\/\// # it's a URI
+                set_uri_key(key)
+            else
+                @key = key
+            end
         else
             @instance = key
             @key = @instance.name
@@ -39,4 +49,28 @@ class Puppet::Indirector::Request
     def indirection
         Puppet::Indirector::Indirection.instance(@indirection_name)
     end
+
+    private
+
+    # Parse the key as a URI, setting attributes appropriately.
+    def set_uri_key(key)
+        @uri = key
+        begin
+            uri = URI.parse(URI.escape(key))
+        rescue => detail
+            raise ArgumentError, "Could not understand URL %s: %s" % [source, detail.to_s]
+        end
+        @server = uri.host if uri.host
+
+        # If the URI class can look up the scheme, it will provide a port,
+        # otherwise it will default to '0'.
+        if uri.port.to_i == 0 and uri.scheme == "puppet"
+            @port = Puppet.settings[:masterport].to_i
+        else
+            @port = uri.port.to_i
+        end
+
+        @protocol = uri.scheme
+        @key = uri.path.sub(/^\//, '')
+    end
 end
diff --git a/lib/puppet/indirector/rest.rb b/lib/puppet/indirector/rest.rb
index 4389dfb7e..a2bfc5d9a 100644
--- a/lib/puppet/indirector/rest.rb
+++ b/lib/puppet/indirector/rest.rb
@@ -1,8 +1,33 @@
 require 'net/http'
 require 'uri'
 
+require 'puppet/network/http_pool'
+
 # Access objects via REST
 class Puppet::Indirector::REST < Puppet::Indirector::Terminus
+
+    class << self
+        attr_reader :server_setting, :port_setting
+    end
+
+    # Specify the setting that we should use to get the server name.
+    def self.use_server_setting(setting)
+        @server_setting = setting
+    end
+
+    def self.server
+        return Puppet.settings[server_setting || :server]
+    end
+
+    # Specify the setting that we should use to get the port.
+    def self.use_port_setting(setting)
+        @port_setting = setting
+    end
+
+    def self.port
+        return Puppet.settings[port_setting || :masterport].to_i
+    end
+
     # Figure out the content type, turn that into a format, and use the format
     # to extract the body of the response.
     def deserialize(response, multiple = false)
@@ -33,20 +58,7 @@ class Puppet::Indirector::REST < Puppet::Indirector::Terminus
     end
   
     def network(request)
-        if request.key =~ /^\w+:\/\// # it looks like a URI
-            begin
-                uri = URI.parse(URI.escape(request.key))
-            rescue => detail
-                raise ArgumentError, "Could not understand URL %s: %s" % [source, detail.to_s]
-            end
-            server = uri.host || Puppet[:server]
-            port = uri.port.to_i == 0 ? Puppet[:masterport].to_i : uri.port.to_i
-        else
-            server = Puppet[:server]
-            port = Puppet[:masterport].to_i
-        end
-
-        Puppet::Network::HttpPool.http_instance(server, port)
+        Puppet::Network::HttpPool.http_instance(request.server || self.class.server, request.port || self.class.port)
     end
 
     def find(request)
diff --git a/spec/unit/indirector/request.rb b/spec/unit/indirector/request.rb
index f7702f821..2476a3d7f 100755
--- a/spec/unit/indirector/request.rb
+++ b/spec/unit/indirector/request.rb
@@ -75,6 +75,37 @@ describe Puppet::Indirector::Request do
         it "should keep its options as a hash even if another option is specified" do
             Puppet::Indirector::Request.new(:ind, :method, :key, :foo => "bar").options.should be_instance_of(Hash)
         end
+
+        describe "and the request key is a URI" do
+            it "should set the protocol to the URI scheme" do
+                Puppet::Indirector::Request.new(:ind, :method, "http://host/stuff").protocol.should == "http"
+            end
+
+            it "should set the server if a server is provided" do
+                Puppet::Indirector::Request.new(:ind, :method, "http://host/stuff").server.should == "host"
+            end
+
+            it "should set the server and port if both are provided" do
+                Puppet::Indirector::Request.new(:ind, :method, "http://host:543/stuff").port.should == 543
+            end
+
+            it "should default to the masterport if the URI scheme is 'puppet'" do
+                Puppet.settings.expects(:value).with(:masterport).returns "321"
+                Puppet::Indirector::Request.new(:ind, :method, "puppet://host/stuff").port.should == 321
+            end
+
+            it "should use the provided port if the URI scheme is not 'puppet'" do
+                Puppet::Indirector::Request.new(:ind, :method, "http://host/stuff").port.should == 80
+            end
+
+            it "should set the request key to the path from the URI" do
+                Puppet::Indirector::Request.new(:ind, :method, "http:///stuff").key.should == "stuff"
+            end
+
+            it "should set the :uri attribute to the full URI" do
+                Puppet::Indirector::Request.new(:ind, :method, "http:///stuff").uri.should == "http:///stuff"
+            end
+        end
     end
 
     it "should look use the Indirection class to return the appropriate indirection" do
diff --git a/spec/unit/indirector/rest.rb b/spec/unit/indirector/rest.rb
index 0c88ea4df..e7afbb5b2 100755
--- a/spec/unit/indirector/rest.rb
+++ b/spec/unit/indirector/rest.rb
@@ -45,6 +45,38 @@ describe Puppet::Indirector::REST do
         @searcher = @rest_class.new
     end
 
+    it "should have a method for specifying what setting a subclass should use to retrieve its server" do
+        @rest_class.should respond_to(:use_server_setting)
+    end
+
+    it "should use any specified setting to pick the server" do
+        @rest_class.expects(:server_setting).returns :servset
+        Puppet.settings.expects(:value).with(:servset).returns "myserver"
+        @rest_class.server.should == "myserver"
+    end
+
+    it "should default to :server for the server setting" do
+        @rest_class.expects(:server_setting).returns nil
+        Puppet.settings.expects(:value).with(:server).returns "myserver"
+        @rest_class.server.should == "myserver"
+    end
+
+    it "should have a method for specifying what setting a subclass should use to retrieve its port" do
+        @rest_class.should respond_to(:use_port_setting)
+    end
+
+    it "should use any specified setting to pick the port" do
+        @rest_class.expects(:port_setting).returns :servset
+        Puppet.settings.expects(:value).with(:servset).returns "321"
+        @rest_class.port.should == 321
+    end
+
+    it "should default to :port for the port setting" do
+        @rest_class.expects(:port_setting).returns nil
+        Puppet.settings.expects(:value).with(:masterport).returns "543"
+        @rest_class.port.should == 543
+    end
+
     describe "when deserializing responses" do
         it "should return nil if the response code is 404" do
             response = mock 'response'
@@ -91,40 +123,26 @@ describe Puppet::Indirector::REST do
             Puppet.settings.stubs(:value).returns("rest_testing")
         end
 
-        describe "and the request key is not a URL" do
-            it "should use the :server setting as the host and the :masterport setting (as an Integer) as the port" do
-                @request = stub 'request', :key => "foo"
-                Puppet.settings.expects(:value).with(:server).returns "myserver"
-                Puppet.settings.expects(:value).with(:masterport).returns "1234"
-                Puppet::Network::HttpPool.expects(:http_instance).with("myserver", 1234).returns "myconn"
-                @searcher.network(@request).should == "myconn"
-            end
+        it "should use the class's server and port if the indirection request provides neither" do
+            @request = stub 'request', :key => "foo", :server => nil, :port => nil
+            @searcher.class.expects(:port).returns 321
+            @searcher.class.expects(:server).returns "myserver"
+            Puppet::Network::HttpPool.expects(:http_instance).with("myserver", 321).returns "myconn"
+            @searcher.network(@request).should == "myconn"
         end
 
-        describe "and the request key is a URL" do
-            it "should use the :server setting and masterport setting, as an Integer, as the host if no values are provided" do
-                @request = stub 'request', :key => "puppet:///key"
-
-                Puppet.settings.expects(:value).with(:server).returns "myserver"
-                Puppet.settings.expects(:value).with(:masterport).returns "1234"
-                Puppet::Network::HttpPool.expects(:http_instance).with("myserver", 1234).returns "myconn"
-                @searcher.network(@request).should == "myconn"
-            end
-
-            it "should use the server if one is provided" do
-                @request.stubs(:key).returns "puppet://host/key"
-
-                Puppet.settings.expects(:value).with(:masterport).returns "1234"
-                Puppet::Network::HttpPool.expects(:http_instance).with("host", 1234).returns "myconn"
-                @searcher.network(@request).should == "myconn"
-            end
-
-            it "should use the server and port if they are provided" do
-                @request.stubs(:key).returns "puppet://host:123/key"
+        it "should use the server from the indirection request if one is present" do
+            @request = stub 'request', :key => "foo", :server => "myserver", :port => nil
+            @searcher.class.stubs(:port).returns 321
+            Puppet::Network::HttpPool.expects(:http_instance).with("myserver", 321).returns "myconn"
+            @searcher.network(@request).should == "myconn"
+        end
 
-                Puppet::Network::HttpPool.expects(:http_instance).with("host", 123).returns "myconn"
-                @searcher.network(@request).should == "myconn"
-            end
+        it "should use the port from the indirection request if one is present" do
+            @request = stub 'request', :key => "foo", :server => nil, :port => 321
+            @searcher.class.stubs(:server).returns "myserver"
+            Puppet::Network::HttpPool.expects(:http_instance).with("myserver", 321).returns "myconn"
+            @searcher.network(@request).should == "myconn"
         end
     end
 
-- 
cgit 


From f5ba99fd24ce2e7cdba7c81153c46df14811d193 Mon Sep 17 00:00:00 2001
From: Luke Kanies <luke@madstop.com>
Date: Sat, 23 Aug 2008 18:15:56 -0500
Subject: Special-casing 'file' URIs in the indirection requests.

These just get converted to full file paths, since
we know they will never pass over the wire.

Signed-off-by: Luke Kanies <luke@madstop.com>
---
 lib/puppet/indirector/request.rb |  7 +++++++
 spec/unit/indirector/request.rb  | 22 +++++++++++++++++++++-
 2 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/lib/puppet/indirector/request.rb b/lib/puppet/indirector/request.rb
index f1d02cead..194b9e031 100644
--- a/lib/puppet/indirector/request.rb
+++ b/lib/puppet/indirector/request.rb
@@ -60,6 +60,13 @@ class Puppet::Indirector::Request
         rescue => detail
             raise ArgumentError, "Could not understand URL %s: %s" % [source, detail.to_s]
         end
+
+        # Just short-circuit these to full paths
+        if uri.scheme == "file"
+            @key = uri.path
+            return
+        end
+
         @server = uri.host if uri.host
 
         # If the URI class can look up the scheme, it will provide a port,
diff --git a/spec/unit/indirector/request.rb b/spec/unit/indirector/request.rb
index 2476a3d7f..6169a09b4 100755
--- a/spec/unit/indirector/request.rb
+++ b/spec/unit/indirector/request.rb
@@ -77,6 +77,26 @@ describe Puppet::Indirector::Request do
         end
 
         describe "and the request key is a URI" do
+            describe "and the URI is a 'file' URI" do
+                before do
+                    @request = Puppet::Indirector::Request.new(:ind, :method, "file:///my/file")
+                end
+
+                it "should set the request key to the full file path" do @request.key.should == "/my/file" end
+
+                it "should not set the protocol" do
+                    @request.protocol.should be_nil
+                end
+
+                it "should not set the port" do
+                    @request.port.should be_nil
+                end
+
+                it "should not set the server" do
+                    @request.server.should be_nil
+                end
+            end
+
             it "should set the protocol to the URI scheme" do
                 Puppet::Indirector::Request.new(:ind, :method, "http://host/stuff").protocol.should == "http"
             end
@@ -98,7 +118,7 @@ describe Puppet::Indirector::Request do
                 Puppet::Indirector::Request.new(:ind, :method, "http://host/stuff").port.should == 80
             end
 
-            it "should set the request key to the path from the URI" do
+            it "should set the request key to the unqualified path from the URI" do
                 Puppet::Indirector::Request.new(:ind, :method, "http:///stuff").key.should == "stuff"
             end
 
-- 
cgit 


From 91b8252755c78dde752c2e06cf13ab0d757aad42 Mon Sep 17 00:00:00 2001
From: Luke Kanies <luke@madstop.com>
Date: Sat, 23 Aug 2008 18:16:38 -0500
Subject: Fixing the fileserving terminus selection hook.

It now uses the fact that the indirection request does
URI parsing, rather than doing the parsing on its own.

Signed-off-by: Luke Kanies <luke@madstop.com>
---
 lib/puppet/file_serving/indirection_hooks.rb |  45 ++++----
 spec/unit/file_serving/indirection_hooks.rb  | 156 +++++++++++++--------------
 2 files changed, 101 insertions(+), 100 deletions(-)

diff --git a/lib/puppet/file_serving/indirection_hooks.rb b/lib/puppet/file_serving/indirection_hooks.rb
index 66ed169dc..15564cf3d 100644
--- a/lib/puppet/file_serving/indirection_hooks.rb
+++ b/lib/puppet/file_serving/indirection_hooks.rb
@@ -9,36 +9,37 @@ require 'puppet/file_serving'
 # in file-serving indirections.  This is necessary because
 # the terminus varies based on the URI asked for.
 module Puppet::FileServing::IndirectionHooks
-    PROTOCOL_MAP = {"puppet" => :rest, "file" => :file, "puppetmounts" => :file_server}
+    PROTOCOL_MAP = {"puppet" => :rest, "file" => :file}
 
     # Pick an appropriate terminus based on the protocol.
     def select_terminus(request)
-        full_uri = request.key
-        # Short-circuit to :file if it's a fully-qualified path.
-        return PROTOCOL_MAP["file"] if full_uri =~ /^#{::File::SEPARATOR}/
-        begin
-            uri = URI.parse(URI.escape(full_uri))
-        rescue => detail
-            raise ArgumentError, "Could not understand URI %s: %s" % [full_uri, detail.to_s]
-        end
+        # We rely on the request's parsing of the URI.
 
-        terminus = PROTOCOL_MAP[uri.scheme] || raise(ArgumentError, "URI protocol '%s' is not supported for file serving" % uri.scheme)
+        # Short-circuit to :file if it's a fully-qualified path or specifies a 'file' protocol.
+        return PROTOCOL_MAP["file"] if request.key =~ /^#{::File::SEPARATOR}/
+        return PROTOCOL_MAP["file"] if request.protocol == "file"
 
-        # This provides a convenient mechanism for people to write configurations work
-        # well in both a networked and local setting.
-        if uri.host.nil? and uri.scheme == "puppet" and Puppet.settings[:name] == "puppet"
-            terminus = :file_server
+        # We're heading over the wire the protocol is 'puppet' and we've got a server name or we're not named 'puppet'
+        if request.protocol == "puppet" and (request.server or Puppet.settings[:name] != "puppet")
+            return PROTOCOL_MAP["puppet"]
+        end
+
+        if request.protocol and PROTOCOL_MAP[request.protocol].nil?
+            raise(ArgumentError, "URI protocol '%s' is not currently supported for file serving" % request.protocol)
         end
 
+        # If we're still here, we're using the file_server or modules.
+
         # This is the backward-compatible module terminus.
-        if terminus == :file_server and uri.path =~ %r{^/([^/]+)\b}
-            modname = $1
-            if modname == "modules"
-                terminus = :modules
-            elsif terminus(:modules).find_module(modname, request.options[:node])
-                Puppet.warning "DEPRECATION NOTICE: Found file '%s' in module without using the 'modules' mount; please prefix path with '/modules'" % uri.path
-                terminus = :modules
-            end
+        modname = request.key.split("/")[0]
+
+        if modname == "modules"
+            terminus = :modules
+        elsif terminus(:modules).find_module(modname, request.options[:node])
+            Puppet.warning "DEPRECATION NOTICE: Found file '%s' in module without using the 'modules' mount; please prefix path with 'modules/'" % request.key
+            terminus = :modules
+        else
+            terminus = :file_server
         end
 
         return terminus
diff --git a/spec/unit/file_serving/indirection_hooks.rb b/spec/unit/file_serving/indirection_hooks.rb
index 160e3ff0a..12993bf1b 100755
--- a/spec/unit/file_serving/indirection_hooks.rb
+++ b/spec/unit/file_serving/indirection_hooks.rb
@@ -12,113 +12,113 @@ describe Puppet::FileServing::IndirectionHooks do
         @object = Object.new
         @object.extend(Puppet::FileServing::IndirectionHooks)
 
-        @request = stub 'request', :key => "http://myhost/blah", :options => {:node => "whatever"}
+        @request = stub 'request', :key => "mymod/myfile", :options => {:node => "whatever"}, :server => nil, :protocol => nil
     end
 
     describe "when being used to select termini" do
-        it "should escape the key before parsing" do
-            uri = stub 'uri', :scheme => "puppet", :host => "blah", :path => "/something"
-            URI.expects(:escape).with("http://myhost/blah").returns("escaped_blah")
-            URI.expects(:parse).with("escaped_blah").returns(uri)
-            @object.select_terminus(@request)
+        it "should return :file if the request key is fully qualified" do
+            @request.expects(:key).returns "#{File::SEPARATOR}foo"
+            @object.select_terminus(@request).should == :file
         end
 
-        it "should use the URI class to parse the key" do
-            uri = stub 'uri', :scheme => "puppet", :host => "blah", :path => "/something"
-            URI.expects(:parse).with("http://myhost/blah").returns(uri)
-            @object.select_terminus @request
+        it "should return :file if the URI protocol is set to 'file'" do
+            @request.expects(:protocol).returns "file"
+            @object.select_terminus(@request).should == :file
         end
 
-        it "should choose :rest when the protocol is 'puppet'" do
-            @request.stubs(:key).returns "puppet://host/module/file"
-            @object.select_terminus(@request).should == :rest
+        it "should fail when a protocol other than :puppet or :file is used" do
+            @request.stubs(:protocol).returns "http"
+            proc { @object.select_terminus(@request) }.should raise_error(ArgumentError)
         end
 
-        it "should choose :file_server when the protocol is 'puppetmounts' and the mount name is not 'modules'" do
-            modules = mock 'modules'
-            @object.stubs(:terminus).with(:modules).returns(modules)
-            modules.stubs(:find_module).returns(nil)
+        describe "and the protocol is 'puppet'" do
+            before do
+                @request.stubs(:protocol).returns "puppet"
+            end
 
-            @request.stubs(:key).returns "puppetmounts://host/notmodules/file"
+            it "should choose :rest when a server is specified" do
+                @request.stubs(:protocol).returns "puppet"
+                @request.expects(:server).returns "foo"
+                @object.select_terminus(@request).should == :rest
+            end
 
-            @object.select_terminus(@request).should == :file_server
-        end
+            # This is so a given file location works when bootstrapping with no server.
+            it "should choose :rest when the Settings name isn't 'puppet'" do
+                @request.stubs(:protocol).returns "puppet"
+                @request.stubs(:server).returns "foo"
+                Puppet.settings.stubs(:value).with(:name).returns "foo"
+                @object.select_terminus(@request).should == :rest
+            end
 
-        it "should choose :file_server when no server name is provided, the process name is 'puppet', and the mount name is not 'modules'" do
-            modules = mock 'modules'
-            @object.stubs(:terminus).with(:modules).returns(modules)
-            modules.stubs(:find_module).returns(nil)
+            it "should not choose :rest when the settings name is 'puppet' and no server is specified" do
+                modules = mock 'modules'
 
-            Puppet.settings.expects(:value).with(:name).returns("puppet")
-            @request.stubs(:key).returns "puppet:///notmodules/file"
-            @object.select_terminus(@request).should == :file_server
-        end
+                @object.stubs(:terminus).with(:modules).returns(modules)
+                modules.stubs(:find_module).with("mymod", @request.options[:node]).returns nil
 
-        it "should choose :modules if it would normally choose :file_server but the mount name is 'modules'" do
-            @request.stubs(:key).returns "puppetmounts://host/modules/mymod/file"
-            @object.select_terminus(@request).should == :modules
+                @request.expects(:protocol).returns "puppet"
+                @request.expects(:server).returns nil
+                Puppet.settings.expects(:value).with(:name).returns "puppet"
+                @object.select_terminus(@request).should_not == :rest
+            end
         end
 
-        it "should choose :modules if it would normally choose :file_server but a module exists with the mount name" do
-            modules = mock 'modules'
+        describe "and the terminus is not :rest or :file" do
+            before do
+                @request.stubs(:protocol).returns nil
+            end
 
-            @object.expects(:terminus).with(:modules).returns(modules)
-            modules.expects(:find_module).with("mymod", @request.options[:node]).returns(:thing)
+            it "should choose :modules if the mount name is 'modules'" do
+                @request.stubs(:key).returns "modules/mymod/file"
+                @object.select_terminus(@request).should == :modules
+            end
 
-            @request.stubs(:key).returns "puppetmounts://host/mymod/file"
-            @object.select_terminus(@request).should == :modules
-        end
+            it "should choose :modules and provide a deprecation notice if a module exists with the mount name" do
+                modules = mock 'modules'
 
-        it "should choose :rest when no server name is provided and the process name is not 'puppet'" do
-            Puppet.settings.expects(:value).with(:name).returns("puppetd")
-            @request.stubs(:key).returns "puppet:///module/file"
-            @object.select_terminus(@request).should == :rest
-        end
+                @object.expects(:terminus).with(:modules).returns(modules)
+                modules.expects(:find_module).with("mymod", @request.options[:node]).returns(:thing)
 
-        it "should choose :file when the protocol is 'file'" do
-            @request.stubs(:key).returns "file://host/module/file"
-            @object.select_terminus(@request).should == :file
-        end
+                Puppet.expects(:warning)
 
-        it "should choose :file when the URI is a normal path name" do
-            @request.stubs(:key).returns "/module/file"
-            @object.select_terminus(@request).should == :file
-        end
+                @request.stubs(:key).returns "mymod/file"
+                @object.select_terminus(@request).should == :modules
+            end
 
-        # This is so that we only choose modules over mounts, not file
-        it "should choose :file when the protocol is 'file' and the fully qualified path starts with '/modules'" do
-            @request.stubs(:key).returns "/module/file"
-            @object.select_terminus(@request).should == :file
-        end
+            it "should choose :file_server if the mount name is not 'modules' nor matches a module name" do
+                modules = mock 'modules'
+                @object.stubs(:terminus).with(:modules).returns(modules)
+                modules.stubs(:find_module).returns(nil)
 
-        it "should fail when a protocol other than :puppet, :file, or :puppetmounts is used" do
-            @request.stubs(:key).returns "http:///module/file"
-            proc { @object.select_terminus(@request) }.should raise_error(ArgumentError)
+                @request.stubs(:key).returns "puppetmounts://host/notmodules/file"
+
+                @object.select_terminus(@request).should == :file_server
+            end
         end
-    end
 
-    describe "when looking for a module whose name matches the mount name" do
-        before do
-            @modules = mock 'modules'
-            @object.stubs(:terminus).with(:modules).returns(@modules)
+        describe "when looking for a module whose name matches the mount name" do
+            before do
+                @modules = mock 'modules'
+                @object.stubs(:terminus).with(:modules).returns(@modules)
 
-            @request.stubs(:key).returns "puppetmounts://host/mymod/file"
-        end
+                @request.stubs(:key).returns "mymod/file"
+            end
 
-        it "should use the modules terminus to look up the module" do
-            @modules.expects(:find_module).with("mymod", @request.options[:node])
-            @object.select_terminus @request
-        end
+            it "should use the modules terminus to look up the module" do
+                @modules.expects(:find_module).with("mymod", @request.options[:node])
+                @object.select_terminus @request
+            end
 
-        it "should pass the node name to the modules terminus" do
-            @modules.expects(:find_module).with("mymod", @request.options[:node])
-            @object.select_terminus @request
-        end
+            it "should pass the node name to the modules terminus" do
+                @modules.expects(:find_module).with("mymod", @request.options[:node])
+                @object.select_terminus @request
+            end
 
-        it "should log a deprecation warning if a module is found" do
-            @modules.expects(:find_module).with("mymod", @request.options[:node]).returns(:something)
-            Puppet.expects(:warning)
-            @object.select_terminus @request
+            it "should log a deprecation warning if a module is found" do
+                @modules.expects(:find_module).with("mymod", @request.options[:node]).returns(:something)
+                Puppet.expects(:warning)
+                @object.select_terminus @request
+            end
         end
     end
 end
-- 
cgit 


From 237b7b294bf90c87891964d01429bbdd42a92524 Mon Sep 17 00:00:00 2001
From: Luke Kanies <luke@madstop.com>
Date: Sat, 23 Aug 2008 18:17:08 -0500
Subject: Fixing whitespace in docs of some tests.

Signed-off-by: Luke Kanies <luke@madstop.com>
---
 spec/unit/file_serving/configuration.rb | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/spec/unit/file_serving/configuration.rb b/spec/unit/file_serving/configuration.rb
index d51fa70b2..a89fdeaae 100755
--- a/spec/unit/file_serving/configuration.rb
+++ b/spec/unit/file_serving/configuration.rb
@@ -35,7 +35,7 @@ describe Puppet::FileServing::Configuration do
         Puppet::Util::Cacher.invalidate
     end
 
-    describe Puppet::FileServing::Configuration, " when initializing" do
+    describe Puppet::FileServing::Configuration, "when initializing" do
 
         it "should work without a configuration file" do
             FileTest.stubs(:exists?).with(@path).returns(false)
@@ -55,7 +55,7 @@ describe Puppet::FileServing::Configuration do
         end
     end
 
-    describe Puppet::FileServing::Configuration, " when parsing the configuration file" do
+    describe Puppet::FileServing::Configuration, "when parsing the configuration file" do
 
         before do
             FileTest.stubs(:exists?).with(@path).returns(true)
@@ -95,7 +95,7 @@ describe Puppet::FileServing::Configuration do
         end
     end
 
-    describe Puppet::FileServing::Configuration, " when finding files" do
+    describe Puppet::FileServing::Configuration, "when finding files" do
 
         before do
             @parser = mock 'parser'
@@ -175,7 +175,7 @@ describe Puppet::FileServing::Configuration do
         end
     end
 
-    describe Puppet::FileServing::Configuration, " when checking authorization" do
+    describe Puppet::FileServing::Configuration, "when checking authorization" do
 
         before do
             @parser = mock 'parser'
-- 
cgit 


From 0a05720e6c4bd0875fc03b53263ff26c6fe14de2 Mon Sep 17 00:00:00 2001
From: Luke Kanies <luke@madstop.com>
Date: Sat, 23 Aug 2008 18:25:18 -0500
Subject: FileServing Configurations now expect unqualified files.

This fits in with the fact that the indirection requests split URIs
and set the request key to an unqualified path rather than
a fully-qualified path.

The whole system is unqualified end-to-end, now, except when you're
specifically asking for a full, local file name.

Signed-off-by: Luke Kanies <luke@madstop.com>
---
 lib/puppet/file_serving/configuration.rb       |  5 ++--
 spec/integration/file_serving/configuration.rb |  4 +--
 spec/unit/file_serving/configuration.rb        | 39 +++++++++++++++-----------
 3 files changed, 27 insertions(+), 21 deletions(-)

diff --git a/lib/puppet/file_serving/configuration.rb b/lib/puppet/file_serving/configuration.rb
index 9c38aaa19..bceecc30c 100644
--- a/lib/puppet/file_serving/configuration.rb
+++ b/lib/puppet/file_serving/configuration.rb
@@ -98,13 +98,14 @@ class Puppet::FileServing::Configuration
         # Reparse the configuration if necessary.
         readconfig
 
-        raise(ArgumentError, "Cannot find file: Invalid path '%s'" % uri) unless uri =~ %r{/([-\w]+)/?}
+        raise(ArgumentError, "Cannot find file: Invalid path '%s'" % uri) unless uri =~ %r{^([-\w]+)(/|$)}
 
         # the dir is based on one of the mounts
         # so first retrieve the mount path
         mount = path = nil
+
         # Strip off the mount name.
-        mount_name, path = uri.sub(%r{^/}, '').split(File::Separator, 2)
+        mount_name, path = uri.split(File::Separator, 2)
 
         return nil unless mount = @mounts[mount_name]
 
diff --git a/spec/integration/file_serving/configuration.rb b/spec/integration/file_serving/configuration.rb
index cb5a23d3b..dfdda402d 100755
--- a/spec/integration/file_serving/configuration.rb
+++ b/spec/integration/file_serving/configuration.rb
@@ -29,12 +29,12 @@ describe Puppet::FileServing::Configuration, " when finding files with Puppet::F
 
     it "should return nil if the file does not exist" do
         FileTest.expects(:exists?).with("/my/path/my/file").returns(false)
-        @config.file_path("/mymount/my/file").should be_nil
+        @config.file_path("mymount/my/file").should be_nil
     end
 
     it "should return the full file path if the file exists" do
         FileTest.expects(:exists?).with("/my/path/my/file").returns(true)
-        @config.file_path("/mymount/my/file").should == "/my/path/my/file"
+        @config.file_path("mymount/my/file").should == "/my/path/my/file"
     end
 
     after do
diff --git a/spec/unit/file_serving/configuration.rb b/spec/unit/file_serving/configuration.rb
index a89fdeaae..741e3bc1d 100755
--- a/spec/unit/file_serving/configuration.rb
+++ b/spec/unit/file_serving/configuration.rb
@@ -111,55 +111,60 @@ describe Puppet::FileServing::Configuration do
             @config = Puppet::FileServing::Configuration.create
         end
 
-        it "should fail if the uri does not match a leading slash followed by a valid mount name" do
+        it "should fail if the uri has a leading slash" do
             @parser.expects(:parse).returns(@mounts)
-            proc { @config.file_path("something") }.should raise_error(ArgumentError)
+            proc { @config.file_path("/something") }.should raise_error(ArgumentError)
+        end
+
+        it "should fail if the uri does not start with a valid mount name" do
+            @parser.expects(:parse).returns(@mounts)
+            proc { @config.file_path("some.thing") }.should raise_error(ArgumentError)
         end
 
         it "should use the first term after the first slash for the mount name" do
             @parser.expects(:parse).returns(@mounts)
             FileTest.stubs(:exists?).returns(true)
             @mount1.expects(:file)
-            @config.file_path("/one")
+            @config.file_path("one")
         end
 
         it "should use the remainder of the URI after the mount name as the file name" do
             @parser.expects(:parse).returns(@mounts)
             @mount1.expects(:file).with("something/else", {})
             FileTest.stubs(:exists?).returns(true)
-            @config.file_path("/one/something/else")
+            @config.file_path("one/something/else")
         end
 
         it "should treat a bare name as a mount and no relative file" do
             @parser.expects(:parse).returns(@mounts)
             @mount1.expects(:file).with(nil, {})
             FileTest.stubs(:exists?).returns(true)
-            @config.file_path("/one")
+            @config.file_path("one")
         end
 
         it "should treat a name with a trailing slash equivalently to a name with no trailing slash" do
             @parser.expects(:parse).returns(@mounts)
             @mount1.expects(:file).with(nil, {})
             FileTest.stubs(:exists?).returns(true)
-            @config.file_path("/one/")
+            @config.file_path("one/")
         end
 
         it "should return nil if the mount cannot be found" do
             @parser.expects(:changed?).returns(true)
             @parser.expects(:parse).returns({})
-            @config.file_path("/one/something").should be_nil
+            @config.file_path("one/something").should be_nil
         end
 
         it "should return nil if the mount does not contain the file" do
             @parser.expects(:parse).returns(@mounts)
             @mount1.expects(:file).with("something/else", {}).returns(nil)
-            @config.file_path("/one/something/else").should be_nil
+            @config.file_path("one/something/else").should be_nil
         end
 
         it "should return the fully qualified path if the mount exists" do
             @parser.expects(:parse).returns(@mounts)
             @mount1.expects(:file).with("something/else", {}).returns("/full/path")
-            @config.file_path("/one/something/else").should == "/full/path"
+            @config.file_path("one/something/else").should == "/full/path"
         end
 
         it "should reparse the configuration file when it has changed" do
@@ -167,11 +172,11 @@ describe Puppet::FileServing::Configuration do
             @parser.expects(:changed?).returns(true)
             @parser.expects(:parse).returns(@mounts)
             FileTest.stubs(:exists?).returns(true)
-            @config.file_path("/one/something")
+            @config.file_path("one/something")
 
             @parser.expects(:changed?).returns(true)
             @parser.expects(:parse).returns({})
-            @config.file_path("/one/something").should be_nil
+            @config.file_path("one/something").should be_nil
         end
     end
 
@@ -193,32 +198,32 @@ describe Puppet::FileServing::Configuration do
         end
 
         it "should return false if the mount cannot be found" do
-            @config.authorized?("/nope/my/file").should be_false
+            @config.authorized?("nope/my/file").should be_false
         end
 
         it "should use the mount to determine authorization" do
             @mount1.expects(:allowed?)
-            @config.authorized?("/one/my/file")
+            @config.authorized?("one/my/file")
         end
 
         it "should pass the client's name to the mount if provided" do
             @mount1.expects(:allowed?).with("myhost", nil)
-            @config.authorized?("/one/my/file", :node => "myhost")
+            @config.authorized?("one/my/file", :node => "myhost")
         end
 
         it "should pass the client's IP to the mount if provided" do
             @mount1.expects(:allowed?).with("myhost", "myip")
-            @config.authorized?("/one/my/file", :node => "myhost", :ipaddress => "myip")
+            @config.authorized?("one/my/file", :node => "myhost", :ipaddress => "myip")
         end
 
         it "should return true if the mount allows the client" do
             @mount1.expects(:allowed?).returns(true)
-            @config.authorized?("/one/my/file").should be_true
+            @config.authorized?("one/my/file").should be_true
         end
 
         it "should return false if the mount denies the client"  do
             @mount1.expects(:allowed?).returns(false)
-            @config.authorized?("/one/my/file").should be_false
+            @config.authorized?("one/my/file").should be_false
         end
     end
 end
-- 
cgit 


From bcd40fb58338e44d46452c47c0ef150a96a5f829 Mon Sep 17 00:00:00 2001
From: Luke Kanies <luke@madstop.com>
Date: Sat, 23 Aug 2008 18:36:03 -0500
Subject: Cleaning up an exception.

Only adding option information when options are present.

Signed-off-by: Luke Kanies <luke@madstop.com>
---
 lib/puppet/indirector/indirection.rb | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lib/puppet/indirector/indirection.rb b/lib/puppet/indirector/indirection.rb
index a9fff75b8..e6068a6aa 100644
--- a/lib/puppet/indirector/indirection.rb
+++ b/lib/puppet/indirector/indirection.rb
@@ -262,7 +262,11 @@ class Puppet::Indirector::Indirection
         return unless terminus.respond_to?(:authorized?)
 
         unless terminus.authorized?(request)
-            raise ArgumentError, "Not authorized to call %s on %s with %s" % [request.method, request.key, request.options.inspect]
+            msg = "Not authorized to call %s on %s" % [request.method, request.key]
+            unless request.options.empty?
+                msg += " with %s" % request.options.inspect
+            end
+            raise ArgumentError, msg
         end
     end
 
-- 
cgit 


From 2f224c9fc97cf632ee8a551180aaa08e263d77df Mon Sep 17 00:00:00 2001
From: Luke Kanies <luke@madstop.com>
Date: Sat, 23 Aug 2008 19:02:43 -0500
Subject: Spell-correcting a comment

Signed-off-by: Luke Kanies <luke@madstop.com>
---
 lib/puppet/indirector/rest.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/puppet/indirector/rest.rb b/lib/puppet/indirector/rest.rb
index a2bfc5d9a..5ac25f02d 100644
--- a/lib/puppet/indirector/rest.rb
+++ b/lib/puppet/indirector/rest.rb
@@ -89,7 +89,7 @@ class Puppet::Indirector::REST < Puppet::Indirector::Terminus
 
     private
 
-    # Create the qurey string, if options are present.
+    # Create the query string, if options are present.
     def query_string(request)
         return "" unless request.options and ! request.options.empty?
         "?" + request.options.collect { |key, value| "%s=%s" % [key, value] }.join("&")
-- 
cgit 


From 1104edb13926a53ee90685d96700cb03eaeca509 Mon Sep 17 00:00:00 2001
From: Luke Kanies <luke@madstop.com>
Date: Sat, 23 Aug 2008 21:20:21 -0500
Subject: Correcting whitespace in a test

Signed-off-by: Luke Kanies <luke@madstop.com>
---
 spec/unit/file_serving/content.rb | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/spec/unit/file_serving/content.rb b/spec/unit/file_serving/content.rb
index 89d786295..5444f4abd 100755
--- a/spec/unit/file_serving/content.rb
+++ b/spec/unit/file_serving/content.rb
@@ -18,7 +18,7 @@ describe Puppet::FileServing::Content do
     end
 end
 
-describe Puppet::FileServing::Content, " when returning the contents" do
+describe Puppet::FileServing::Content, "when returning the contents" do
     before do
         @path = "/my/base"
         @content = Puppet::FileServing::Content.new("sub/path", :links => :follow, :path => @path)
@@ -46,7 +46,7 @@ describe Puppet::FileServing::Content, " when returning the contents" do
     end
 end
 
-describe Puppet::FileServing::Content, " when converting to yaml" do
+describe Puppet::FileServing::Content, "when converting to yaml" do
     it "should fail if no path has been set" do
         @content = Puppet::FileServing::Content.new("some/key")
         proc { @content.to_yaml }.should raise_error(ArgumentError)
@@ -60,7 +60,7 @@ describe Puppet::FileServing::Content, " when converting to yaml" do
     end
 end
 
-describe Puppet::FileServing::Content, " when converting from yaml" do
+describe Puppet::FileServing::Content, "when converting from yaml" do
     # LAK:FIXME This isn't in the right place, but we need some kind of
     # control somewhere that requires that all REST connections only pull
     # from the file-server, thus guaranteeing they go through our authorization
-- 
cgit 


From 6335b143a312481aaa200f71cd25dffd4f88c8ae Mon Sep 17 00:00:00 2001
From: Luke Kanies <luke@madstop.com>
Date: Sat, 23 Aug 2008 21:59:14 -0500
Subject: Causing the Indirection to fail if a terminus selection hook does not
 return a value.

Signed-off-by: Luke Kanies <luke@madstop.com>
---
 lib/puppet/indirector/indirection.rb |  4 +++-
 spec/unit/indirector/indirection.rb  | 16 ++++++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/lib/puppet/indirector/indirection.rb b/lib/puppet/indirector/indirection.rb
index e6068a6aa..04c3aed23 100644
--- a/lib/puppet/indirector/indirection.rb
+++ b/lib/puppet/indirector/indirection.rb
@@ -274,7 +274,9 @@ class Puppet::Indirector::Indirection
     def prepare(request)
         # Pick our terminus.
         if respond_to?(:select_terminus)
-            terminus_name = select_terminus(request)
+            unless terminus_name = select_terminus(request)
+                raise ArgumentError, "Could not determine appropriate terminus for %s" % request
+            end
         else
             terminus_name = terminus_class
         end
diff --git a/spec/unit/indirector/indirection.rb b/spec/unit/indirector/indirection.rb
index e52be31e1..166065ecf 100755
--- a/spec/unit/indirector/indirection.rb
+++ b/spec/unit/indirector/indirection.rb
@@ -34,6 +34,22 @@ describe "Indirection Delegator", :shared => true do
         @indirection.send(@method, "me")
     end
 
+    it "should fail if the :select_terminus hook does not return a terminus name" do
+        # Define the method, so our respond_to? hook matches.
+        class << @indirection
+            def select_terminus(request)
+            end
+        end
+
+        request = stub 'request', :key => "me", :options => {}
+
+        @indirection.stubs(:request).returns request
+
+        @indirection.expects(:select_terminus).with(request).returns nil
+
+        lambda { @indirection.send(@method, "me") }.should raise_error(ArgumentError)
+    end
+
     it "should choose the terminus returned by the :terminus_class method if no :select_terminus method is available" do
         @indirection.expects(:terminus_class).returns :test_terminus
 
-- 
cgit 


From a0bda8532f5e1e9f5bb29eb92f389383ae0857d5 Mon Sep 17 00:00:00 2001
From: Luke Kanies <luke@madstop.com>
Date: Sat, 23 Aug 2008 22:00:19 -0500
Subject: Removing the yaml conversion code from FileContent.

Also fixing some integration tests that were failing
because of the change to the terminus selection code
for file serving.

Signed-off-by: Luke Kanies <luke@madstop.com>
---
 lib/puppet/file_serving/content.rb       |  8 --------
 spec/integration/file_serving/content.rb |  2 ++
 spec/shared_behaviours/file_serving.rb   | 23 +++++++++--------------
 spec/unit/file_serving/content.rb        | 24 ------------------------
 4 files changed, 11 insertions(+), 46 deletions(-)

diff --git a/lib/puppet/file_serving/content.rb b/lib/puppet/file_serving/content.rb
index 9398513e7..1114829f1 100644
--- a/lib/puppet/file_serving/content.rb
+++ b/lib/puppet/file_serving/content.rb
@@ -23,12 +23,4 @@ class Puppet::FileServing::Content < Puppet::FileServing::FileBase
 
         ::File.read(full_path())
     end
-
-    # Just return the file contents as the yaml.  This allows us to
-    # avoid escaping or any such thing.  LAK:NOTE Not really sure how
-    # this will behave if the file contains yaml...  I think the far
-    # side needs to understand that it's a plain string.
-    def to_yaml
-        content
-    end
 end
diff --git a/spec/integration/file_serving/content.rb b/spec/integration/file_serving/content.rb
index aee2a9f2d..af0181393 100755
--- a/spec/integration/file_serving/content.rb
+++ b/spec/integration/file_serving/content.rb
@@ -15,4 +15,6 @@ describe Puppet::FileServing::Content, " when finding files" do
         @test_class = Puppet::FileServing::Content
         @indirection = Puppet::FileServing::Content.indirection
     end
+
+    after { Puppet::Util::Cacher.invalidate }
 end
diff --git a/spec/shared_behaviours/file_serving.rb b/spec/shared_behaviours/file_serving.rb
index ba01f75a1..99994b99a 100644
--- a/spec/shared_behaviours/file_serving.rb
+++ b/spec/shared_behaviours/file_serving.rb
@@ -6,12 +6,18 @@
 describe "Puppet::FileServing::Files", :shared => true do
     it "should use the rest terminus when the 'puppet' URI scheme is used and a host name is present" do
         uri = "puppet://myhost/fakemod/my/file"
-        @indirection.terminus(:rest).expects(:find)
+
+        # It appears that the mocking somehow interferes with the caching subsystem.
+        # This mock somehow causes another terminus to get generated.
+        term = @indirection.terminus(:rest)
+        @indirection.stubs(:terminus).with(:rest).returns term
+        term.expects(:find)
         @test_class.find(uri)
     end
 
     it "should use the rest terminus when the 'puppet' URI scheme is used, no host name is present, and the process name is not 'puppet'" do
         uri = "puppet:///fakemod/my/file"
+        Puppet.settings.stubs(:value).returns "foo"
         Puppet.settings.stubs(:value).with(:name).returns("puppetd")
         Puppet.settings.stubs(:value).with(:modulepath).returns("")
         @indirection.terminus(:rest).expects(:find)
@@ -21,19 +27,9 @@ describe "Puppet::FileServing::Files", :shared => true do
     it "should use the file_server terminus when the 'puppet' URI scheme is used, no host name is present, and the process name is 'puppet'" do
         uri = "puppet:///fakemod/my/file"
         Puppet::Node::Environment.stubs(:new).returns(stub("env", :name => "testing"))
+        Puppet.settings.stubs(:value).returns ""
         Puppet.settings.stubs(:value).with(:name).returns("puppet")
-        Puppet.settings.stubs(:value).with(:modulepath, "testing").returns("")
-        Puppet.settings.stubs(:value).with(:modulepath).returns("")
-        Puppet.settings.stubs(:value).with(:libdir).returns("")
         Puppet.settings.stubs(:value).with(:fileserverconfig).returns("/whatever")
-        Puppet.settings.stubs(:value).with(:environment).returns("")
-        @indirection.terminus(:file_server).expects(:find)
-        @indirection.terminus(:file_server).stubs(:authorized?).returns(true)
-        @test_class.find(uri)
-    end
-
-    it "should use the file_server terminus when the 'puppetmounts' URI scheme is used" do
-        uri = "puppetmounts:///fakemod/my/file"
         @indirection.terminus(:file_server).expects(:find)
         @indirection.terminus(:file_server).stubs(:authorized?).returns(true)
         @test_class.find(uri)
@@ -52,7 +48,7 @@ describe "Puppet::FileServing::Files", :shared => true do
     end
 
     it "should use the configuration to test whether the request is allowed" do
-        uri = "puppetmounts:///fakemod/my/file"
+        uri = "fakemod/my/file"
         config = mock 'configuration'
         @indirection.terminus(:file_server).stubs(:configuration).returns config
 
@@ -61,4 +57,3 @@ describe "Puppet::FileServing::Files", :shared => true do
         @test_class.find(uri, :node => "foo", :ip => "bar")
     end
 end
-
diff --git a/spec/unit/file_serving/content.rb b/spec/unit/file_serving/content.rb
index 5444f4abd..e8de1d63e 100755
--- a/spec/unit/file_serving/content.rb
+++ b/spec/unit/file_serving/content.rb
@@ -45,27 +45,3 @@ describe Puppet::FileServing::Content, "when returning the contents" do
         @content.content.should == :mycontent
     end
 end
-
-describe Puppet::FileServing::Content, "when converting to yaml" do
-    it "should fail if no path has been set" do
-        @content = Puppet::FileServing::Content.new("some/key")
-        proc { @content.to_yaml }.should raise_error(ArgumentError)
-    end
-
-    it "should return the file contents" do
-        @content = Puppet::FileServing::Content.new("some/path")
-        @content.path = "/base/path"
-        @content.expects(:content).returns(:content)
-        @content.to_yaml.should == :content
-    end
-end
-
-describe Puppet::FileServing::Content, "when converting from yaml" do
-    # LAK:FIXME This isn't in the right place, but we need some kind of
-    # control somewhere that requires that all REST connections only pull
-    # from the file-server, thus guaranteeing they go through our authorization
-    # hook.
-    it "should set the URI scheme to 'puppetmounts'" do
-        pending "We need to figure out where this should be"
-    end
-end
-- 
cgit 


From 89a3738d2b0678ae8fb1e7191e01caf6c5ece1f9 Mon Sep 17 00:00:00 2001
From: Luke Kanies <luke@madstop.com>
Date: Sun, 24 Aug 2008 12:18:42 -0500
Subject: Adding suitability as a requirement for a format being supported.

Signed-off-by: Luke Kanies <luke@madstop.com>
---
 lib/puppet/network/format.rb | 3 ++-
 spec/unit/network/format.rb  | 5 +++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/lib/puppet/network/format.rb b/lib/puppet/network/format.rb
index 5f259fa49..3e0a7d8aa 100644
--- a/lib/puppet/network/format.rb
+++ b/lib/puppet/network/format.rb
@@ -55,7 +55,8 @@ class Puppet::Network::Format
     end
 
     def supported?(klass)
-        klass.respond_to?(intern_method) and
+        suitable? and
+            klass.respond_to?(intern_method) and
             klass.respond_to?(intern_multiple_method) and
             klass.respond_to?(render_multiple_method) and
             klass.instance_methods.include?(render_method)
diff --git a/spec/unit/network/format.rb b/spec/unit/network/format.rb
index b960a3f73..818ecdce3 100755
--- a/spec/unit/network/format.rb
+++ b/spec/unit/network/format.rb
@@ -74,6 +74,11 @@ describe Puppet::Network::Format do
             Puppet::Network::Format.new(:yaml).should_not be_supported(String)
         end
 
+        it "should not consider a class supported unless the format is suitable" do
+            @format.expects(:suitable?).returns false
+            @format.should_not be_supported(FormatRenderer)
+        end
+
         it "should always downcase mimetypes" do
             @format.mime = "Foo/Bar"
             @format.mime.should == "foo/bar"
-- 
cgit 


From 3101ea23e556081fe38502218034f02aafe0c5bf Mon Sep 17 00:00:00 2001
From: Luke Kanies <luke@madstop.com>
Date: Sun, 24 Aug 2008 13:02:16 -0500
Subject: Adding a hackish raw format.

As the comment in the file says, we don't really have enough
data to know what a good design would look like, and I think
this format will be a bit of a one-off, so I'm just throwing
up some barriers to keep people from doing silly things with it.

Signed-off-by: Luke Kanies <luke@madstop.com>
---
 lib/puppet/network/formats.rb | 22 +++++++++++++++++++++-
 spec/unit/network/formats.rb  | 26 ++++++++++++++++++++++++++
 2 files changed, 47 insertions(+), 1 deletion(-)

diff --git a/lib/puppet/network/formats.rb b/lib/puppet/network/formats.rb
index 8e4c59fb3..c2a8b7ab6 100644
--- a/lib/puppet/network/formats.rb
+++ b/lib/puppet/network/formats.rb
@@ -42,7 +42,7 @@ Puppet::Network::FormatHandler.create(:marshal, :mime => "text/marshal") do
         Marshal.dump(instance)
     end
 
-    # Yaml monkey-patches Array, so this works.
+    # Marshal monkey-patches Array, so this works.
     def render_multiple(instances)
         Marshal.dump(instances)
     end
@@ -54,3 +54,23 @@ Puppet::Network::FormatHandler.create(:marshal, :mime => "text/marshal") do
 end
 
 Puppet::Network::FormatHandler.create(:s, :mime => "text/plain")
+
+Puppet::Network::FormatHandler.create(:raw, :mime => "application/x-raw") do
+    def intern_multiple(klass, text)
+        raise NotImplementedError
+    end
+
+    def render_multiple(instances)
+        raise NotImplementedError
+    end
+
+    # LAK:NOTE The format system isn't currently flexible enough to handle
+    # what I need to support raw formats just for individual instances (rather
+    # than both individual and collections), but we don't yet have enough data
+    # to make a "correct" design.
+    #   So, we hack it so it works for singular but fail if someone tries it
+    # on plurals.
+    def supported?(klass)
+        true
+    end
+end
diff --git a/spec/unit/network/formats.rb b/spec/unit/network/formats.rb
index f7493360d..527fd9d79 100755
--- a/spec/unit/network/formats.rb
+++ b/spec/unit/network/formats.rb
@@ -99,4 +99,30 @@ describe "Puppet Network Format" do
             @text.mime.should == "text/plain"
         end
     end
+
+    describe Puppet::Network::FormatHandler.format(:raw) do
+        before do
+            @format = Puppet::Network::FormatHandler.format(:raw)
+        end
+
+        it "should exist" do
+            @format.should_not be_nil
+        end
+
+        it "should have its mimetype set to application/x-raw" do
+            @format.mime.should == "application/x-raw"
+        end
+
+        it "should always be supported" do
+            @format.should be_supported(String)
+        end
+
+        it "should fail if its multiple_render method is used" do
+            lambda { @format.render_multiple("foo") }.should raise_error(NotImplementedError)
+        end
+
+        it "should fail if its multiple_intern method is used" do
+            lambda { @format.intern_multiple(String, "foo") }.should raise_error(NotImplementedError)
+        end
+    end
 end
-- 
cgit 


From 5a195e0c06daa2bfa008cfd94c660e50b9d0ae56 Mon Sep 17 00:00:00 2001
From: Luke Kanies <luke@madstop.com>
Date: Sun, 24 Aug 2008 13:32:33 -0500
Subject: Renaming FileServing::FileBase to FileServing::Base.

Also fixing a set of tests I broke last night.  I'm looking
at replacing autotest with rspactor, because my FSEvents hack
to autotest means it's harder for me to rerun autotest.

Signed-off-by: Luke Kanies <luke@madstop.com>
---
 lib/puppet/file_serving/content.rb   |  17 +++--
 lib/puppet/file_serving/file_base.rb |  76 ---------------------
 lib/puppet/file_serving/metadata.rb  |   4 +-
 spec/unit/file_serving/content.rb    |  14 ++++
 spec/unit/file_serving/file_base.rb  | 124 -----------------------------------
 spec/unit/file_serving/metadata.rb   |   4 +-
 spec/unit/indirector/file_server.rb  |  30 ++++-----
 7 files changed, 45 insertions(+), 224 deletions(-)
 delete mode 100644 lib/puppet/file_serving/file_base.rb
 delete mode 100755 spec/unit/file_serving/file_base.rb

diff --git a/lib/puppet/file_serving/content.rb b/lib/puppet/file_serving/content.rb
index 1114829f1..64f000eaa 100644
--- a/lib/puppet/file_serving/content.rb
+++ b/lib/puppet/file_serving/content.rb
@@ -4,23 +4,30 @@
 
 require 'puppet/indirector'
 require 'puppet/file_serving'
-require 'puppet/file_serving/file_base'
+require 'puppet/file_serving/base'
 require 'puppet/file_serving/indirection_hooks'
 
 # A class that handles retrieving file contents.
 # It only reads the file when its content is specifically
 # asked for.
-class Puppet::FileServing::Content < Puppet::FileServing::FileBase
+class Puppet::FileServing::Content < Puppet::FileServing::Base
     extend Puppet::Indirector
     indirects :file_content, :extend => Puppet::FileServing::IndirectionHooks
 
     attr_reader :path
 
+    def collect
+    end
+
     # Read the content of our file in.
     def content
-        # This stat can raise an exception, too.
-        raise(ArgumentError, "Cannot read the contents of links unless following links") if stat().ftype == "symlink" 
+        unless defined?(@content) and @content
+            # This stat can raise an exception, too.
+            raise(ArgumentError, "Cannot read the contents of links unless following links") if stat().ftype == "symlink" 
 
-        ::File.read(full_path())
+            p full_path
+            @content = ::File.read(full_path())
+        end
+        @content
     end
 end
diff --git a/lib/puppet/file_serving/file_base.rb b/lib/puppet/file_serving/file_base.rb
deleted file mode 100644
index e87d683aa..000000000
--- a/lib/puppet/file_serving/file_base.rb
+++ /dev/null
@@ -1,76 +0,0 @@
-#
-#  Created by Luke Kanies on 2007-10-22.
-#  Copyright (c) 2007. All rights reserved.
-
-require 'puppet/file_serving'
-
-# The base class for Content and Metadata; provides common
-# functionality like the behaviour around links.
-class Puppet::FileServing::FileBase
-    attr_accessor :key
-
-    # Does our file exist?
-    def exist?
-        begin
-            stat
-            return true
-        rescue => detail
-            return false
-        end
-    end
-
-    # Return the full path to our file.  Fails if there's no path set.
-    def full_path
-        raise(ArgumentError, "You must set a path to get a file's path") unless self.path
-
-        if relative_path.nil? or relative_path == ""
-            path
-        else
-            File.join(path, relative_path)
-        end
-    end
-
-    def initialize(key, options = {})
-        @key = key
-        @links = :manage
-
-        options.each do |param, value|
-            begin
-                send param.to_s + "=", value
-            rescue NoMethodError
-                raise ArgumentError, "Invalid option %s for %s" % [param, self.class]
-            end
-        end
-    end
-
-    # Determine how we deal with links.
-    attr_reader :links
-    def links=(value)
-        value = :manage if value == :ignore
-        raise(ArgumentError, ":links can only be set to :manage or :follow") unless [:manage, :follow].include?(value) 
-        @links = value
-    end
-
-    # Set our base path.
-    attr_reader :path
-    def path=(path)
-        raise ArgumentError.new("Paths must be fully qualified") unless path =~ /^#{::File::SEPARATOR}/
-        @path = path
-    end
-
-    # Set a relative path; this is used for recursion, and sets
-    # the file's path relative to the initial recursion point.
-    attr_reader :relative_path
-    def relative_path=(path)
-        raise ArgumentError.new("Relative paths must not be fully qualified") if path =~ /^#{::File::SEPARATOR}/
-        @relative_path = path
-    end
-
-    # Stat our file, using the appropriate link-sensitive method.
-    def stat
-        unless defined?(@stat_method)
-            @stat_method = self.links == :manage ? :lstat : :stat
-        end
-        File.send(@stat_method, full_path())
-    end
-end
diff --git a/lib/puppet/file_serving/metadata.rb b/lib/puppet/file_serving/metadata.rb
index beecaef48..a1265dd8b 100644
--- a/lib/puppet/file_serving/metadata.rb
+++ b/lib/puppet/file_serving/metadata.rb
@@ -5,12 +5,12 @@
 require 'puppet'
 require 'puppet/indirector'
 require 'puppet/file_serving'
-require 'puppet/file_serving/file_base'
+require 'puppet/file_serving/base'
 require 'puppet/util/checksums'
 require 'puppet/file_serving/indirection_hooks'
 
 # A class that handles retrieving file metadata.
-class Puppet::FileServing::Metadata < Puppet::FileServing::FileBase
+class Puppet::FileServing::Metadata < Puppet::FileServing::Base
 
     include Puppet::Util::Checksums
 
diff --git a/spec/unit/file_serving/content.rb b/spec/unit/file_serving/content.rb
index e8de1d63e..b747ced78 100755
--- a/spec/unit/file_serving/content.rb
+++ b/spec/unit/file_serving/content.rb
@@ -16,6 +16,20 @@ describe Puppet::FileServing::Content do
     it "should should include the IndirectionHooks module in its indirection" do
         Puppet::FileServing::Content.indirection.metaclass.included_modules.should include(Puppet::FileServing::IndirectionHooks)
     end
+
+    it "should have a method for collecting its attributes" do
+        Puppet::FileServing::Content.new("sub/path", :path => "/base").should respond_to(:collect)
+    end
+
+    it "should retrieve and store its contents when its attributes are collected" do
+        content = Puppet::FileServing::Content.new("sub/path", :path => "/base")
+
+        result = "foo"
+        File.stubs(:lstat).returns(stub("stat", :ftype => "file"))
+        File.expects(:read).with("/base/sub/path").returns result
+        content.collect
+        content.content.should equal(result)
+    end
 end
 
 describe Puppet::FileServing::Content, "when returning the contents" do
diff --git a/spec/unit/file_serving/file_base.rb b/spec/unit/file_serving/file_base.rb
deleted file mode 100755
index ded6ae4a8..000000000
--- a/spec/unit/file_serving/file_base.rb
+++ /dev/null
@@ -1,124 +0,0 @@
-#!/usr/bin/env ruby
-
-require File.dirname(__FILE__) + '/../../spec_helper'
-
-require 'puppet/file_serving/file_base'
-
-describe Puppet::FileServing::FileBase do
-    it "should accept a key in the form of a URI" do
-        Puppet::FileServing::FileBase.new("puppet://host/module/dir/file").key.should == "puppet://host/module/dir/file"
-    end
-
-    it "should allow specification of whether links should be managed" do
-        Puppet::FileServing::FileBase.new("puppet://host/module/dir/file", :links => :manage).links.should == :manage
-    end
-
-    it "should consider :ignore links equivalent to :manage links" do
-        Puppet::FileServing::FileBase.new("puppet://host/module/dir/file", :links => :ignore).links.should == :manage
-    end
-
-    it "should fail if :links is set to anything other than :manage, :follow, or :ignore" do
-        proc { Puppet::FileServing::FileBase.new("puppet://host/module/dir/file", :links => :else) }.should raise_error(ArgumentError)
-    end
-
-    it "should default to :manage for :links" do
-        Puppet::FileServing::FileBase.new("puppet://host/module/dir/file").links.should == :manage
-    end
-
-    it "should allow specification of a path" do
-        FileTest.stubs(:exists?).returns(true)
-        Puppet::FileServing::FileBase.new("puppet://host/module/dir/file", :path => "/my/file").path.should == "/my/file"
-    end
-
-    it "should allow specification of a relative path" do
-        FileTest.stubs(:exists?).returns(true)
-        Puppet::FileServing::FileBase.new("puppet://host/module/dir/file", :relative_path => "my/file").relative_path.should == "my/file"
-    end
-
-    it "should have a means of determining if the file exists" do
-        Puppet::FileServing::FileBase.new("blah").should respond_to(:exist?)
-    end
-
-    it "should correctly indicate if the file is present" do
-        File.expects(:lstat).with("/my/file").returns(mock("stat"))
-        Puppet::FileServing::FileBase.new("blah", :path => "/my/file").exist?.should be_true
-    end
-
-    it "should correctly indicate if the file is asbsent" do
-        File.expects(:lstat).with("/my/file").raises RuntimeError
-        Puppet::FileServing::FileBase.new("blah", :path => "/my/file").exist?.should be_false
-    end
-
-    describe "when setting the base path" do
-        before do
-            @file = Puppet::FileServing::FileBase.new("puppet://host/module/dir/file")
-        end
-
-        it "should require that the base path be fully qualified" do
-            FileTest.stubs(:exists?).returns(true)
-            proc { @file.path = "unqualified/file" }.should raise_error(ArgumentError)
-        end
-    end
-
-    describe "when setting the relative path" do
-        it "should require that the relative path be unqualified" do
-            @file = Puppet::FileServing::FileBase.new("puppet://host/module/dir/file")
-            FileTest.stubs(:exists?).returns(true)
-            proc { @file.relative_path = "/qualified/file" }.should raise_error(ArgumentError)
-        end
-    end
-
-    describe "when determining the full file path" do
-        before do
-            @file = Puppet::FileServing::FileBase.new("mykey", :path => "/this/file")
-        end
-
-        it "should return the path if there is no relative path" do
-            @file.full_path.should == "/this/file"
-        end
-
-        it "should return the path if the relative_path is set to ''" do
-            @file.relative_path = ""
-            @file.full_path.should == "/this/file"
-        end
-
-        it "should return the path joined with the relative path if there is a relative path and it is not set to '/' or ''" do
-            @file.relative_path = "not/qualified"
-            @file.full_path.should == "/this/file/not/qualified"
-        end
-
-        it "should should fail if there is no path set" do
-            @file = Puppet::FileServing::FileBase.new("not/qualified")
-            proc { @file.full_path }.should raise_error(ArgumentError)
-        end
-    end
-
-    describe "when stat'ing files" do
-        before do
-            @file = Puppet::FileServing::FileBase.new("mykey", :path => "/this/file")
-        end
-
-        it "should stat the file's full path" do
-            @file.stubs(:full_path).returns("/this/file")
-            File.expects(:lstat).with("/this/file").returns stub("stat", :ftype => "file")
-            @file.stat
-        end
-
-        it "should fail if the file does not exist" do
-            @file.stubs(:full_path).returns("/this/file")
-            File.expects(:lstat).with("/this/file").raises(Errno::ENOENT)
-            proc { @file.stat }.should raise_error(Errno::ENOENT)
-        end
-
-        it "should use :lstat if :links is set to :manage" do
-            File.expects(:lstat).with("/this/file").returns stub("stat", :ftype => "file")
-            @file.stat
-        end
-
-        it "should use :stat if :links is set to :follow" do
-            File.expects(:stat).with("/this/file").returns stub("stat", :ftype => "file")
-            @file.links = :follow
-            @file.stat
-        end
-    end
-end
diff --git a/spec/unit/file_serving/metadata.rb b/spec/unit/file_serving/metadata.rb
index 9743370c1..e76ceb3e8 100755
--- a/spec/unit/file_serving/metadata.rb
+++ b/spec/unit/file_serving/metadata.rb
@@ -5,8 +5,8 @@ require File.dirname(__FILE__) + '/../../spec_helper'
 require 'puppet/file_serving/metadata'
 
 describe Puppet::FileServing::Metadata do
-    it "should should be a subclass of FileBase" do
-        Puppet::FileServing::Metadata.superclass.should equal(Puppet::FileServing::FileBase)
+    it "should should be a subclass of Base" do
+        Puppet::FileServing::Metadata.superclass.should equal(Puppet::FileServing::Base)
     end
 
     it "should indirect file_metadata" do
diff --git a/spec/unit/indirector/file_server.rb b/spec/unit/indirector/file_server.rb
index ba951737a..544953932 100755
--- a/spec/unit/indirector/file_server.rb
+++ b/spec/unit/indirector/file_server.rb
@@ -24,7 +24,7 @@ describe Puppet::Indirector::FileServer do
 
         @file_server = @file_server_class.new
 
-        @uri = "puppetmounts://host/my/local/file"
+        @uri = "puppet://host/my/local/file"
         @configuration = mock 'configuration'
         Puppet::FileServing::Configuration.stubs(:create).returns(@configuration)
 
@@ -34,28 +34,28 @@ describe Puppet::Indirector::FileServer do
     describe Puppet::Indirector::FileServer, " when finding files" do
 
         it "should use the path portion of the URI as the file name" do
-            @configuration.expects(:file_path).with("/my/local/file", :node => nil)
+            @configuration.expects(:file_path).with("my/local/file", :node => nil)
             @file_server.find(@request)
         end
 
         it "should use the FileServing configuration to convert the file name to a fully qualified path" do
-            @configuration.expects(:file_path).with("/my/local/file", :node => nil)
+            @configuration.expects(:file_path).with("my/local/file", :node => nil)
             @file_server.find(@request)
         end
 
         it "should pass the node name to the FileServing configuration if one is provided" do
-            @configuration.expects(:file_path).with("/my/local/file", :node => "testing")
+            @configuration.expects(:file_path).with("my/local/file", :node => "testing")
             @request.node = "testing"
             @file_server.find(@request)
         end
 
         it "should return nil if no fully qualified path is found" do
-            @configuration.expects(:file_path).with("/my/local/file", :node => nil).returns(nil)
+            @configuration.expects(:file_path).with("my/local/file", :node => nil).returns(nil)
             @file_server.find(@request).should be_nil
         end
 
         it "should return an instance of the model created with the full path if a file is found" do
-            @configuration.expects(:file_path).with("/my/local/file", :node => nil).returns("/some/file")
+            @configuration.expects(:file_path).with("my/local/file", :node => nil).returns("/some/file")
             @model.expects(:new).returns(:myinstance)
             @file_server.find(@request).should == :myinstance
         end
@@ -63,12 +63,12 @@ describe Puppet::Indirector::FileServer do
 
     describe Puppet::Indirector::FileServer, " when returning instances" do
         before :each do
-            @configuration.expects(:file_path).with("/my/local/file", :node => nil).returns("/some/file")
+            @configuration.expects(:file_path).with("my/local/file", :node => nil).returns("/some/file")
             @instance = mock 'instance'
         end
 
         it "should create the instance with the key used to find the instance" do
-            @model.expects(:new).with { |key, *options| key == @uri }
+            @model.expects(:new).with { |key, *options| key == "my/local/file" }
             @file_server.find(@request)
         end
 
@@ -149,35 +149,35 @@ describe Puppet::Indirector::FileServer do
     describe Puppet::Indirector::FileServer, " when searching for files" do
 
         it "should use the path portion of the URI as the file name" do
-            @configuration.expects(:file_path).with("/my/local/file", :node => nil)
+            @configuration.expects(:file_path).with("my/local/file", :node => nil)
             @file_server.search(@request)
         end
 
         it "should use the FileServing configuration to convert the file name to a fully qualified path" do
-            @configuration.expects(:file_path).with("/my/local/file", :node => nil)
+            @configuration.expects(:file_path).with("my/local/file", :node => nil)
             @file_server.search(@request)
         end
 
         it "should pass the node name to the FileServing configuration if one is provided" do
-            @configuration.expects(:file_path).with("/my/local/file", :node => "testing")
+            @configuration.expects(:file_path).with("my/local/file", :node => "testing")
             @request.node = "testing"
             @file_server.search(@request)
         end
 
         it "should return nil if no fully qualified path is found" do
-            @configuration.expects(:file_path).with("/my/local/file", :node => nil).returns(nil)
+            @configuration.expects(:file_path).with("my/local/file", :node => nil).returns(nil)
             @file_server.search(@request).should be_nil
         end
 
         it "should use :path2instances from the terminus_helper to return instances if a module is found and the file exists" do
-            @configuration.expects(:file_path).with("/my/local/file", :node => nil).returns("/my/file")
+            @configuration.expects(:file_path).with("my/local/file", :node => nil).returns("my/file")
             @file_server.expects(:path2instances)
             @file_server.search(@request)
         end
 
         it "should pass the request on to :path2instances" do
-            @configuration.expects(:file_path).with("/my/local/file", :node => nil).returns("/my/file")
-            @file_server.expects(:path2instances).with(@request, "/my/file")
+            @configuration.expects(:file_path).with("my/local/file", :node => nil).returns("my/file")
+            @file_server.expects(:path2instances).with(@request, "my/file")
             @file_server.search(@request)
         end
     end
-- 
cgit 


From 90e70227b0bb7cfd104ae34de8f7c2b7250edb09 Mon Sep 17 00:00:00 2001
From: Luke Kanies <luke@madstop.com>
Date: Sun, 24 Aug 2008 14:10:39 -0500
Subject: Adding weights to network formats, and sorting them based on the
 weight.

This way the new hackish RAW format will only ever be used if it's
specifically chosen.

Signed-off-by: Luke Kanies <luke@madstop.com>
---
 lib/puppet/network/format.rb         |  9 ++++++++-
 lib/puppet/network/format_handler.rb |  5 ++++-
 lib/puppet/network/formats.rb        |  3 ++-
 spec/unit/network/format.rb          | 12 ++++++++++++
 spec/unit/network/format_handler.rb  | 31 ++++++++++++++++++++++---------
 spec/unit/network/formats.rb         |  4 ++++
 6 files changed, 52 insertions(+), 12 deletions(-)

diff --git a/lib/puppet/network/format.rb b/lib/puppet/network/format.rb
index 3e0a7d8aa..21aead7cc 100644
--- a/lib/puppet/network/format.rb
+++ b/lib/puppet/network/format.rb
@@ -5,7 +5,7 @@ require 'puppet/provider/confiner'
 class Puppet::Network::Format
     include Puppet::Provider::Confiner
 
-    attr_reader :name, :mime
+    attr_reader :name, :mime, :weight
 
     def initialize(name, options = {}, &block)
         @name = name.to_s.downcase.intern
@@ -17,6 +17,13 @@ class Puppet::Network::Format
             self.mime = "text/%s" % name
         end
 
+        if weight = options[:weight]
+            @weight = weight
+            options.delete(:weight)
+        else
+            @weight = 5
+        end
+
         unless options.empty?
             raise ArgumentError, "Unsupported option(s) %s" % options.keys
         end
diff --git a/lib/puppet/network/format_handler.rb b/lib/puppet/network/format_handler.rb
index 4c9f4e59e..f3c3380e1 100644
--- a/lib/puppet/network/format_handler.rb
+++ b/lib/puppet/network/format_handler.rb
@@ -61,7 +61,10 @@ module Puppet::Network::FormatHandler
         end
 
         def supported_formats
-            format_handler.formats.collect { |f| format_handler.format(f) }.find_all { |f| f.supported?(self) }.collect { |f| f.name }
+            format_handler.formats.collect { |f| format_handler.format(f) }.find_all { |f| f.supported?(self) }.collect { |f| f.name }.sort do |a, b| 
+                # It's an inverse sort -- higher weight formats go first.
+                format_handler.format(b).weight <=> format_handler.format(a).weight
+            end
         end
     end
 
diff --git a/lib/puppet/network/formats.rb b/lib/puppet/network/formats.rb
index c2a8b7ab6..85e8ce6f8 100644
--- a/lib/puppet/network/formats.rb
+++ b/lib/puppet/network/formats.rb
@@ -55,7 +55,8 @@ end
 
 Puppet::Network::FormatHandler.create(:s, :mime => "text/plain")
 
-Puppet::Network::FormatHandler.create(:raw, :mime => "application/x-raw") do
+# A very low-weight format so it'll never get chosen automatically.
+Puppet::Network::FormatHandler.create(:raw, :mime => "application/x-raw", :weight => 1) do
     def intern_multiple(klass, text)
         raise NotImplementedError
     end
diff --git a/spec/unit/network/format.rb b/spec/unit/network/format.rb
index 818ecdce3..244bff306 100755
--- a/spec/unit/network/format.rb
+++ b/spec/unit/network/format.rb
@@ -83,6 +83,18 @@ describe Puppet::Network::Format do
             @format.mime = "Foo/Bar"
             @format.mime.should == "foo/bar"
         end
+
+        it "should support having a weight" do
+            @format.should respond_to(:weight)
+        end
+
+        it "should default to a weight of of 5" do
+            @format.weight.should == 5
+        end
+
+        it "should be able to override its weight at initialization" do
+            Puppet::Network::Format.new(:foo, :weight => 1).weight.should == 1
+        end
     end
 
     describe "when converting between instances and formatted text" do
diff --git a/spec/unit/network/format_handler.rb b/spec/unit/network/format_handler.rb
index 35e996516..3dcff6199 100755
--- a/spec/unit/network/format_handler.rb
+++ b/spec/unit/network/format_handler.rb
@@ -21,18 +21,31 @@ describe Puppet::Network::FormatHandler do
     end
 
     it "should include all supported formats" do
-        one = stub 'supported', :supported? => true, :name => "one"
-        two = stub 'supported', :supported? => false, :name => "two"
-        three = stub 'supported', :supported? => true, :name => "three"
-        four = stub 'supported', :supported? => false, :name => "four"
-        Puppet::Network::FormatHandler.expects(:formats).returns %w{one two three four}
-        Puppet::Network::FormatHandler.expects(:format).with("one").returns one
-        Puppet::Network::FormatHandler.expects(:format).with("two").returns two
-        Puppet::Network::FormatHandler.expects(:format).with("three").returns three
-        Puppet::Network::FormatHandler.expects(:format).with("four").returns four
+        one = stub 'supported', :supported? => true, :name => "one", :weight => 1
+        two = stub 'supported', :supported? => false, :name => "two", :weight => 1
+        three = stub 'supported', :supported? => true, :name => "three", :weight => 1
+        four = stub 'supported', :supported? => false, :name => "four", :weight => 1
+        Puppet::Network::FormatHandler.stubs(:formats).returns %w{one two three four}
+        Puppet::Network::FormatHandler.stubs(:format).with("one").returns one
+        Puppet::Network::FormatHandler.stubs(:format).with("two").returns two
+        Puppet::Network::FormatHandler.stubs(:format).with("three").returns three
+        Puppet::Network::FormatHandler.stubs(:format).with("four").returns four
         FormatTester.supported_formats.sort.should == %w{one three}.sort
     end
 
+    it "should return the supported formats in decreasing order of weight" do
+        one = stub 'supported', :supported? => true, :name => "one", :weight => 1
+        two = stub 'supported', :supported? => true, :name => "two", :weight => 6
+        three = stub 'supported', :supported? => true, :name => "three", :weight => 2
+        four = stub 'supported', :supported? => true, :name => "four", :weight => 8
+        Puppet::Network::FormatHandler.stubs(:formats).returns %w{one two three four}
+        Puppet::Network::FormatHandler.stubs(:format).with("one").returns one
+        Puppet::Network::FormatHandler.stubs(:format).with("two").returns two
+        Puppet::Network::FormatHandler.stubs(:format).with("three").returns three
+        Puppet::Network::FormatHandler.stubs(:format).with("four").returns four
+        FormatTester.supported_formats.should == %w{four two three one}
+    end
+
     it "should return the first format as the default format" do
         FormatTester.expects(:supported_formats).returns %w{one two}
         FormatTester.default_format.should == "one"
diff --git a/spec/unit/network/formats.rb b/spec/unit/network/formats.rb
index 527fd9d79..0e21fefa7 100755
--- a/spec/unit/network/formats.rb
+++ b/spec/unit/network/formats.rb
@@ -124,5 +124,9 @@ describe "Puppet Network Format" do
         it "should fail if its multiple_intern method is used" do
             lambda { @format.intern_multiple(String, "foo") }.should raise_error(NotImplementedError)
         end
+
+        it "should have a weight of 1" do
+            @format.weight.should == 1
+        end
     end
 end
-- 
cgit 


From 550e3d6ad5aadfe99fc1e10efa77cc193d3a9df3 Mon Sep 17 00:00:00 2001
From: Luke Kanies <luke@madstop.com>
Date: Sun, 24 Aug 2008 14:11:48 -0500
Subject: Finishing the rename of FileBase => Base.

Git did something really strange, in that it apparently didn't
add the new base.rb files even though I used 'git mv'.

Also fixing some other failing tests I hadn't previously tracked
down because of the magical tuple of autotest's suckiness and
my laziness.

Signed-off-by: Luke Kanies <luke@madstop.com>
---
 lib/puppet/file_serving/base.rb               |  76 ++++++++++++++++
 lib/puppet/indirector/module_files.rb         |   7 +-
 spec/unit/file_serving/base.rb                | 124 ++++++++++++++++++++++++++
 spec/unit/file_serving/content.rb             |   4 +-
 spec/unit/indirector/file_metadata/modules.rb |   2 +-
 spec/unit/indirector/module_files.rb          |  64 +++++++------
 6 files changed, 237 insertions(+), 40 deletions(-)
 create mode 100644 lib/puppet/file_serving/base.rb
 create mode 100755 spec/unit/file_serving/base.rb

diff --git a/lib/puppet/file_serving/base.rb b/lib/puppet/file_serving/base.rb
new file mode 100644
index 000000000..1cfd0bc4c
--- /dev/null
+++ b/lib/puppet/file_serving/base.rb
@@ -0,0 +1,76 @@
+#
+#  Created by Luke Kanies on 2007-10-22.
+#  Copyright (c) 2007. All rights reserved.
+
+require 'puppet/file_serving'
+
+# The base class for Content and Metadata; provides common
+# functionality like the behaviour around links.
+class Puppet::FileServing::Base
+    attr_accessor :key
+
+    # Does our file exist?
+    def exist?
+        begin
+            stat
+            return true
+        rescue => detail
+            return false
+        end
+    end
+
+    # Return the full path to our file.  Fails if there's no path set.
+    def full_path
+        raise(ArgumentError, "You must set a path to get a file's path") unless self.path
+
+        if relative_path.nil? or relative_path == ""
+            path
+        else
+            File.join(path, relative_path)
+        end
+    end
+
+    def initialize(key, options = {})
+        @key = key
+        @links = :manage
+
+        options.each do |param, value|
+            begin
+                send param.to_s + "=", value
+            rescue NoMethodError
+                raise ArgumentError, "Invalid option %s for %s" % [param, self.class]
+            end
+        end
+    end
+
+    # Determine how we deal with links.
+    attr_reader :links
+    def links=(value)
+        value = :manage if value == :ignore
+        raise(ArgumentError, ":links can only be set to :manage or :follow") unless [:manage, :follow].include?(value) 
+        @links = value
+    end
+
+    # Set our base path.
+    attr_reader :path
+    def path=(path)
+        raise ArgumentError.new("Paths must be fully qualified") unless path =~ /^#{::File::SEPARATOR}/
+        @path = path
+    end
+
+    # Set a relative path; this is used for recursion, and sets
+    # the file's path relative to the initial recursion point.
+    attr_reader :relative_path
+    def relative_path=(path)
+        raise ArgumentError.new("Relative paths must not be fully qualified") if path =~ /^#{::File::SEPARATOR}/
+        @relative_path = path
+    end
+
+    # Stat our file, using the appropriate link-sensitive method.
+    def stat
+        unless defined?(@stat_method)
+            @stat_method = self.links == :manage ? :lstat : :stat
+        end
+        File.send(@stat_method, full_path())
+    end
+end
diff --git a/lib/puppet/indirector/module_files.rb b/lib/puppet/indirector/module_files.rb
index cf5c29cab..40dd06941 100644
--- a/lib/puppet/indirector/module_files.rb
+++ b/lib/puppet/indirector/module_files.rb
@@ -21,7 +21,7 @@ class Puppet::Indirector::ModuleFiles < Puppet::Indirector::Terminus
 
         # Make sure our file path starts with /modules, so that we authorize
         # against the 'modules' mount.
-        path = uri.path =~ /^\/modules/ ? uri.path : "/modules" + uri.path
+        path = uri.path =~ /^modules\// ? uri.path : "modules/" + uri.path
 
         configuration.authorized?(path, :node => request.node, :ipaddress => request.ip)
     end
@@ -66,9 +66,8 @@ class Puppet::Indirector::ModuleFiles < Puppet::Indirector::Terminus
     def find_path(request)
         uri = key2uri(request.key)
 
-        # Strip off /modules if it's there -- that's how requests get routed to this terminus.
-        # Also, strip off the leading slash if present.
-        module_name, relative_path = uri.path.sub(/^\/modules\b/, '').sub(%r{^/}, '').split(File::Separator, 2)
+        # Strip off modules/ if it's there -- that's how requests get routed to this terminus.
+        module_name, relative_path = uri.path.sub(/^modules\//, '').sub(%r{^/}, '').split(File::Separator, 2)
 
         # And use the environment to look up the module.
         return nil unless mod = find_module(module_name, request.node)
diff --git a/spec/unit/file_serving/base.rb b/spec/unit/file_serving/base.rb
new file mode 100755
index 000000000..9d900e30a
--- /dev/null
+++ b/spec/unit/file_serving/base.rb
@@ -0,0 +1,124 @@
+#!/usr/bin/env ruby
+
+require File.dirname(__FILE__) + '/../../spec_helper'
+
+require 'puppet/file_serving/base'
+
+describe Puppet::FileServing::Base do
+    it "should accept a key in the form of a URI" do
+        Puppet::FileServing::Base.new("puppet://host/module/dir/file").key.should == "puppet://host/module/dir/file"
+    end
+
+    it "should allow specification of whether links should be managed" do
+        Puppet::FileServing::Base.new("puppet://host/module/dir/file", :links => :manage).links.should == :manage
+    end
+
+    it "should consider :ignore links equivalent to :manage links" do
+        Puppet::FileServing::Base.new("puppet://host/module/dir/file", :links => :ignore).links.should == :manage
+    end
+
+    it "should fail if :links is set to anything other than :manage, :follow, or :ignore" do
+        proc { Puppet::FileServing::Base.new("puppet://host/module/dir/file", :links => :else) }.should raise_error(ArgumentError)
+    end
+
+    it "should default to :manage for :links" do
+        Puppet::FileServing::Base.new("puppet://host/module/dir/file").links.should == :manage
+    end
+
+    it "should allow specification of a path" do
+        FileTest.stubs(:exists?).returns(true)
+        Puppet::FileServing::Base.new("puppet://host/module/dir/file", :path => "/my/file").path.should == "/my/file"
+    end
+
+    it "should allow specification of a relative path" do
+        FileTest.stubs(:exists?).returns(true)
+        Puppet::FileServing::Base.new("puppet://host/module/dir/file", :relative_path => "my/file").relative_path.should == "my/file"
+    end
+
+    it "should have a means of determining if the file exists" do
+        Puppet::FileServing::Base.new("blah").should respond_to(:exist?)
+    end
+
+    it "should correctly indicate if the file is present" do
+        File.expects(:lstat).with("/my/file").returns(mock("stat"))
+        Puppet::FileServing::Base.new("blah", :path => "/my/file").exist?.should be_true
+    end
+
+    it "should correctly indicate if the file is asbsent" do
+        File.expects(:lstat).with("/my/file").raises RuntimeError
+        Puppet::FileServing::Base.new("blah", :path => "/my/file").exist?.should be_false
+    end
+
+    describe "when setting the base path" do
+        before do
+            @file = Puppet::FileServing::Base.new("puppet://host/module/dir/file")
+        end
+
+        it "should require that the base path be fully qualified" do
+            FileTest.stubs(:exists?).returns(true)
+            proc { @file.path = "unqualified/file" }.should raise_error(ArgumentError)
+        end
+    end
+
+    describe "when setting the relative path" do
+        it "should require that the relative path be unqualified" do
+            @file = Puppet::FileServing::Base.new("puppet://host/module/dir/file")
+            FileTest.stubs(:exists?).returns(true)
+            proc { @file.relative_path = "/qualified/file" }.should raise_error(ArgumentError)
+        end
+    end
+
+    describe "when determining the full file path" do
+        before do
+            @file = Puppet::FileServing::Base.new("mykey", :path => "/this/file")
+        end
+
+        it "should return the path if there is no relative path" do
+            @file.full_path.should == "/this/file"
+        end
+
+        it "should return the path if the relative_path is set to ''" do
+            @file.relative_path = ""
+            @file.full_path.should == "/this/file"
+        end
+
+        it "should return the path joined with the relative path if there is a relative path and it is not set to '/' or ''" do
+            @file.relative_path = "not/qualified"
+            @file.full_path.should == "/this/file/not/qualified"
+        end
+
+        it "should should fail if there is no path set" do
+            @file = Puppet::FileServing::Base.new("not/qualified")
+            proc { @file.full_path }.should raise_error(ArgumentError)
+        end
+    end
+
+    describe "when stat'ing files" do
+        before do
+            @file = Puppet::FileServing::Base.new("mykey", :path => "/this/file")
+        end
+
+        it "should stat the file's full path" do
+            @file.stubs(:full_path).returns("/this/file")
+            File.expects(:lstat).with("/this/file").returns stub("stat", :ftype => "file")
+            @file.stat
+        end
+
+        it "should fail if the file does not exist" do
+            @file.stubs(:full_path).returns("/this/file")
+            File.expects(:lstat).with("/this/file").raises(Errno::ENOENT)
+            proc { @file.stat }.should raise_error(Errno::ENOENT)
+        end
+
+        it "should use :lstat if :links is set to :manage" do
+            File.expects(:lstat).with("/this/file").returns stub("stat", :ftype => "file")
+            @file.stat
+        end
+
+        it "should use :stat if :links is set to :follow" do
+            File.expects(:stat).with("/this/file").returns stub("stat", :ftype => "file")
+            @file.links = :follow
+            @file.stat
+        end
+    end
+end
diff --git a/spec/unit/file_serving/content.rb b/spec/unit/file_serving/content.rb
index b747ced78..82353d23d 100755
--- a/spec/unit/file_serving/content.rb
+++ b/spec/unit/file_serving/content.rb
@@ -5,8 +5,8 @@ require File.dirname(__FILE__) + '/../../spec_helper'
 require 'puppet/file_serving/content'
 
 describe Puppet::FileServing::Content do
-    it "should should be a subclass of FileBase" do
-        Puppet::FileServing::Content.superclass.should equal(Puppet::FileServing::FileBase)
+    it "should should be a subclass of Base" do
+        Puppet::FileServing::Content.superclass.should equal(Puppet::FileServing::Base)
     end
 
     it "should indirect file_content" do
diff --git a/spec/unit/indirector/file_metadata/modules.rb b/spec/unit/indirector/file_metadata/modules.rb
index 3905a49ad..838ac3b5f 100755
--- a/spec/unit/indirector/file_metadata/modules.rb
+++ b/spec/unit/indirector/file_metadata/modules.rb
@@ -24,7 +24,7 @@ describe Puppet::Indirector::FileMetadata::Modules, " when finding metadata" do
         @module = Puppet::Module.new("mymod", "/path/to")
         @finder.stubs(:find_module).returns(@module)
 
-        @request = Puppet::Indirector::Request.new(:metadata, :find, "puppetmounts://hostname/modules/mymod/my/file")
+        @request = Puppet::Indirector::Request.new(:metadata, :find, "puppet://hostname/modules/mymod/my/file")
     end
 
     it "should return nil if the file is not found" do
diff --git a/spec/unit/indirector/module_files.rb b/spec/unit/indirector/module_files.rb
index 32dedd4f1..20c9f499a 100755
--- a/spec/unit/indirector/module_files.rb
+++ b/spec/unit/indirector/module_files.rb
@@ -25,49 +25,47 @@ describe Puppet::Indirector::ModuleFiles do
   
         @module_files = @module_files_class.new
   
-        @uri = "puppetmounts://host/modules/my/local/file"
         @module = Puppet::Module.new("mymod", "/module/path")
 
-        @request = stub 'request', :key => @uri, :options => {}, :node => nil, :ip => nil, :method => :find
+        @request = Puppet::Indirector::Request.new(:mytype, :find, "puppet://host/modules/mymod/local/file")
     end
 
     describe Puppet::Indirector::ModuleFiles, " when finding files" do
+        before do
+            Puppet::Module.stubs(:find).returns @module
+        end
 
-        it "should strip off the leading '/modules' mount name" do
-            Puppet::Module.expects(:find).with('my', "myenv").returns @module
+        it "should strip off the leading 'modules/' mount name" do
+            Puppet::Module.expects(:find).with { |key, env| key == 'mymod' }.returns @module
             @module_files.find(@request)
         end
 
-        it "should not strip off leading terms that start with '/modules' but are longer words" do
-            @request.stubs(:key).returns "puppetmounts://host/modulestart/my/local/file"
-            Puppet::Module.expects(:find).with('modulestart', "myenv").returns nil
+        it "should not strip off leading terms that start with 'modules' but are longer words" do
+            @request.stubs(:key).returns "modulestart/mymod/local/file"
+            Puppet::Module.expects(:find).with { |key, env| key == 'modulestart'}.returns nil
             @module_files.find(@request)
         end
 
         it "should search for a module whose name is the first term in the remaining file path" do
-            Puppet::Module.expects(:find).with('my', "myenv").returns @module
             @module_files.find(@request)
         end
 
         it "should search for a file relative to the module's files directory" do
-            Puppet::Module.expects(:find).with('my', "myenv").returns @module
             FileTest.expects(:exists?).with("/module/path/files/local/file")
             @module_files.find(@request)
         end
 
         it "should return nil if the module does not exist" do
-            Puppet::Module.expects(:find).with('my', "myenv").returns nil
+            Puppet::Module.expects(:find).returns nil
             @module_files.find(@request).should be_nil
         end
 
         it "should return nil if the module exists but the file does not" do
-            Puppet::Module.expects(:find).with('my', "myenv").returns @module
             FileTest.expects(:exists?).with("/module/path/files/local/file").returns(false)
             @module_files.find(@request).should be_nil
         end
 
         it "should return an instance of the model if a module is found and the file exists" do
-            Puppet::Module.expects(:find).with('my', "myenv").returns @module
             FileTest.expects(:exists?).with("/module/path/files/local/file").returns(true)
             @model.expects(:new).returns(:myinstance)
             @module_files.find(@request).should == :myinstance
@@ -76,7 +74,7 @@ describe Puppet::Indirector::ModuleFiles do
         it "should use the node's environment to look up the module if the node name is provided" do
             node = stub "node", :environment => "testing"
             Puppet::Node.expects(:find).with("mynode").returns(node)
-            Puppet::Module.expects(:find).with('my', "testing")
+            Puppet::Module.expects(:find).with('mymod', "testing")
 
             @request.stubs(:node).returns "mynode"
             @module_files.find(@request)
@@ -85,7 +83,7 @@ describe Puppet::Indirector::ModuleFiles do
         it "should use the default environment setting to look up the module if the node name is not provided" do
             env = stub "environment", :name => "testing"
             Puppet::Node::Environment.stubs(:new).returns(env)
-            Puppet::Module.expects(:find).with('my', "testing")
+            Puppet::Module.expects(:find).with('mymod', "testing")
             @module_files.find(@request)
         end
     end
@@ -93,13 +91,13 @@ describe Puppet::Indirector::ModuleFiles do
     describe Puppet::Indirector::ModuleFiles, " when returning instances" do
 
         before do
-            Puppet::Module.expects(:find).with('my', "myenv").returns @module
+            Puppet::Module.expects(:find).with('mymod', "myenv").returns @module
             FileTest.expects(:exists?).with("/module/path/files/local/file").returns(true)
             @instance = mock 'instance'
         end
 
         it "should create the instance with the key used to find the instance" do
-            @model.expects(:new).with { |key, *options| key == @uri }
+            @model.expects(:new).with { |key, *options| key == @request.key }
             @module_files.find(@request)
         end
 
@@ -149,19 +147,19 @@ describe Puppet::Indirector::ModuleFiles do
         end
 
         it "should use the path directly from the URI if it already includes /modules" do
-            @request.expects(:key).returns "puppetmounts://host/modules/my/file"
-            @configuration.expects(:authorized?).with { |uri, *args| uri == "/modules/my/file" }
+            @request.expects(:key).returns "modules/my/file"
+            @configuration.expects(:authorized?).with { |uri, *args| uri == "modules/my/file" }
             @module_files.authorized?(@request)
         end
 
-        it "should add /modules to the file path if it's not included in the URI" do
-            @request.expects(:key).returns "puppetmounts://host/my/file"
-            @configuration.expects(:authorized?).with { |uri, *args| uri == "/modules/my/file" }
+        it "should add modules/ to the file path if it's not included in the URI" do
+            @request.expects(:key).returns "my/file"
+            @configuration.expects(:authorized?).with { |uri, *args| uri == "modules/my/file" }
             @module_files.authorized?(@request)
         end
 
         it "should pass the node name to the file server configuration" do
-            @request.expects(:key).returns "puppetmounts://host/my/file"
+            @request.expects(:key).returns "my/file"
             @configuration.expects(:authorized?).with { |key, options| options[:node] == "mynode" }
             @request.stubs(:node).returns "mynode"
             @module_files.authorized?(@request)
@@ -186,41 +184,41 @@ describe Puppet::Indirector::ModuleFiles do
 
     describe Puppet::Indirector::ModuleFiles, " when searching for files" do
 
-        it "should strip off the leading '/modules' mount name" do
+        it "should strip off the leading 'modules/' mount name" do
             Puppet::Node::Environment.stubs(:new).returns(stub('env', :name => "myenv"))
-            Puppet::Module.expects(:find).with('my', "myenv").returns @module
+            Puppet::Module.expects(:find).with { |key, env| key == 'mymod'}.returns @module
             @module_files.search(@request)
         end
 
         it "should not strip off leading terms that start with '/modules' but are longer words" do
             Puppet::Node::Environment.stubs(:new).returns(stub('env', :name => "myenv"))
             Puppet::Module.expects(:find).with('modulestart', "myenv").returns nil
-            @request.stubs(:key).returns "puppetmounts://host/modulestart/my/local/file"
+            @request.stubs(:key).returns "modulestart/my/local/file"
             @module_files.search @request
         end
 
         it "should search for a module whose name is the first term in the remaining file path" do
             Puppet::Node::Environment.stubs(:new).returns(stub('env', :name => "myenv"))
-            Puppet::Module.expects(:find).with('my', "myenv").returns @module
+            Puppet::Module.expects(:find).with('mymod', "myenv").returns @module
             @module_files.search(@request)
         end
 
         it "should search for a file relative to the module's files directory" do
             Puppet::Node::Environment.stubs(:new).returns(stub('env', :name => "myenv"))
-            Puppet::Module.expects(:find).with('my', "myenv").returns @module
+            Puppet::Module.expects(:find).with('mymod', "myenv").returns @module
             FileTest.expects(:exists?).with("/module/path/files/local/file")
             @module_files.search(@request)
         end
 
         it "should return nil if the module does not exist" do
             Puppet::Node::Environment.stubs(:new).returns(stub('env', :name => "myenv"))
-            Puppet::Module.expects(:find).with('my', "myenv").returns nil
+            Puppet::Module.expects(:find).with('mymod', "myenv").returns nil
             @module_files.search(@request).should be_nil
         end
 
         it "should return nil if the module exists but the file does not" do
             Puppet::Node::Environment.stubs(:new).returns(stub('env', :name => "myenv"))
-            Puppet::Module.expects(:find).with('my', "myenv").returns @module
+            Puppet::Module.expects(:find).with('mymod', "myenv").returns @module
             FileTest.expects(:exists?).with("/module/path/files/local/file").returns(false)
             @module_files.search(@request).should be_nil
         end
@@ -228,7 +226,7 @@ describe Puppet::Indirector::ModuleFiles do
         it "should use the node's environment to look up the module if the node name is provided" do
             node = stub "node", :environment => "testing"
             Puppet::Node.expects(:find).with("mynode").returns(node)
-            Puppet::Module.expects(:find).with('my', "testing")
+            Puppet::Module.expects(:find).with('mymod', "testing")
             @request.stubs(:node).returns "mynode"
             @module_files.search(@request)
         end
@@ -236,13 +234,13 @@ describe Puppet::Indirector::ModuleFiles do
         it "should use the default environment setting to look up the module if the node name is not provided and the environment is not set to ''" do
             env = stub 'env', :name => "testing"
             Puppet::Node::Environment.stubs(:new).returns(env)
-            Puppet::Module.expects(:find).with('my', "testing")
+            Puppet::Module.expects(:find).with('mymod', "testing")
             @module_files.search(@request)
         end
 
         it "should use :path2instances from the terminus_helper to return instances if a module is found and the file exists" do
             Puppet::Node::Environment.stubs(:new).returns(stub('env', :name => "myenv"))
-            Puppet::Module.expects(:find).with('my', "myenv").returns @module
+            Puppet::Module.expects(:find).with('mymod', "myenv").returns @module
             FileTest.expects(:exists?).with("/module/path/files/local/file").returns(true)
             @module_files.expects(:path2instances).with(@request, "/module/path/files/local/file")
             @module_files.search(@request)
@@ -250,7 +248,7 @@ describe Puppet::Indirector::ModuleFiles do
 
         it "should pass the request directly to :path2instances" do
             Puppet::Node::Environment.stubs(:new).returns(stub('env', :name => "myenv"))
-            Puppet::Module.expects(:find).with('my', "myenv").returns @module
+            Puppet::Module.expects(:find).with('mymod', "myenv").returns @module
             FileTest.expects(:exists?).with("/module/path/files/local/file").returns(true)
             @module_files.expects(:path2instances).with(@request, "/module/path/files/local/file")
             @module_files.search(@request)
-- 
cgit 


From 8ea25efd90b4d2281db12076cbaab3f766cac8b4 Mon Sep 17 00:00:00 2001
From: Luke Kanies <luke@madstop.com>
Date: Sun, 24 Aug 2008 14:42:48 -0500
Subject: Refactoring how files in FileServing are named.

Previously, they retained some concept of the URI used to
find them, and this uri was the primary key
for the FileServing instances.  This key was unfortunately
completely useless, as evidenced by the fact that it was never
used except to test that it worked.

I've modified the FileServing instances (through modifying
the Base class) to use their local path as their key, and they
no longer care about the URI at all.

This commit is mostly about fixing the code that interacts with
the instances to use this new API.

Signed-off-by: Luke Kanies <luke@madstop.com>
---
 lib/puppet/file_serving/base.rb                   |  8 +---
 lib/puppet/file_serving/content.rb                |  1 -
 lib/puppet/file_serving/terminus_helper.rb        |  2 +-
 lib/puppet/indirector/direct_file_server.rb       | 10 ++---
 lib/puppet/indirector/file_server.rb              |  2 +-
 lib/puppet/indirector/module_files.rb             |  2 +-
 spec/integration/file_serving/metadata.rb         |  2 +
 spec/integration/indirector/direct_file_server.rb |  4 +-
 spec/integration/indirector/module_files.rb       |  4 +-
 spec/unit/file_serving/base.rb                    | 50 +++++++++--------------
 spec/unit/file_serving/content.rb                 | 10 ++---
 spec/unit/file_serving/metadata.rb                | 33 +++++----------
 spec/unit/file_serving/terminus_helper.rb         | 10 +----
 spec/unit/indirector/direct_file_server.rb        |  9 +---
 spec/unit/indirector/file_metadata/file.rb        |  4 +-
 spec/unit/indirector/file_server.rb               |  7 +---
 16 files changed, 56 insertions(+), 102 deletions(-)

diff --git a/lib/puppet/file_serving/base.rb b/lib/puppet/file_serving/base.rb
index 1cfd0bc4c..94e337b99 100644
--- a/lib/puppet/file_serving/base.rb
+++ b/lib/puppet/file_serving/base.rb
@@ -7,8 +7,6 @@ require 'puppet/file_serving'
 # The base class for Content and Metadata; provides common
 # functionality like the behaviour around links.
 class Puppet::FileServing::Base
-    attr_accessor :key
-
     # Does our file exist?
     def exist?
         begin
@@ -21,8 +19,6 @@ class Puppet::FileServing::Base
 
     # Return the full path to our file.  Fails if there's no path set.
     def full_path
-        raise(ArgumentError, "You must set a path to get a file's path") unless self.path
-
         if relative_path.nil? or relative_path == ""
             path
         else
@@ -30,8 +26,8 @@ class Puppet::FileServing::Base
         end
     end
 
-    def initialize(key, options = {})
-        @key = key
+    def initialize(path, options = {})
+        self.path = path
         @links = :manage
 
         options.each do |param, value|
diff --git a/lib/puppet/file_serving/content.rb b/lib/puppet/file_serving/content.rb
index 64f000eaa..f13fcaa72 100644
--- a/lib/puppet/file_serving/content.rb
+++ b/lib/puppet/file_serving/content.rb
@@ -25,7 +25,6 @@ class Puppet::FileServing::Content < Puppet::FileServing::Base
             # This stat can raise an exception, too.
             raise(ArgumentError, "Cannot read the contents of links unless following links") if stat().ftype == "symlink" 
 
-            p full_path
             @content = ::File.read(full_path())
         end
         @content
diff --git a/lib/puppet/file_serving/terminus_helper.rb b/lib/puppet/file_serving/terminus_helper.rb
index e5da0e29f..bde0bd389 100644
--- a/lib/puppet/file_serving/terminus_helper.rb
+++ b/lib/puppet/file_serving/terminus_helper.rb
@@ -11,7 +11,7 @@ module Puppet::FileServing::TerminusHelper
     def path2instances(request, path)
         args = [:links, :ignore, :recurse].inject({}) { |hash, param| hash[param] = request.options[param] if request.options[param]; hash }
         Puppet::FileServing::Fileset.new(path, args).files.collect do |file|
-            inst = model.new(File.join(request.key, file), :path => path, :relative_path => file)
+            inst = model.new(path, :relative_path => file)
             inst.links = request.options[:links] if request.options[:links]
             inst
         end
diff --git a/lib/puppet/indirector/direct_file_server.rb b/lib/puppet/indirector/direct_file_server.rb
index b3b4886f3..bcda92366 100644
--- a/lib/puppet/indirector/direct_file_server.rb
+++ b/lib/puppet/indirector/direct_file_server.rb
@@ -12,16 +12,14 @@ class Puppet::Indirector::DirectFileServer < Puppet::Indirector::Terminus
     include Puppet::FileServing::TerminusHelper
 
     def find(request)
-        uri = key2uri(request.key)
-        return nil unless FileTest.exists?(uri.path)
-        instance = model.new(request.key, :path => uri.path)
+        return nil unless FileTest.exists?(request.key)
+        instance = model.new(request.key)
         instance.links = request.options[:links] if request.options[:links]
         return instance
     end
 
     def search(request)
-        uri = key2uri(request.key)
-        return nil unless FileTest.exists?(uri.path)
-        path2instances(request, uri.path)
+        return nil unless FileTest.exists?(request.key)
+        path2instances(request, request.key)
     end
 end
diff --git a/lib/puppet/indirector/file_server.rb b/lib/puppet/indirector/file_server.rb
index b0df7ff5d..476fc5b23 100644
--- a/lib/puppet/indirector/file_server.rb
+++ b/lib/puppet/indirector/file_server.rb
@@ -25,7 +25,7 @@ class Puppet::Indirector::FileServer < Puppet::Indirector::Terminus
     # Find our key using the fileserver.
     def find(request)
         return nil unless path = find_path(request)
-        result =  model.new(request.key, :path => path)
+        result =  model.new(path)
         result.links = request.options[:links] if request.options[:links]
         return result
     end
diff --git a/lib/puppet/indirector/module_files.rb b/lib/puppet/indirector/module_files.rb
index 40dd06941..7c5cf278f 100644
--- a/lib/puppet/indirector/module_files.rb
+++ b/lib/puppet/indirector/module_files.rb
@@ -30,7 +30,7 @@ class Puppet::Indirector::ModuleFiles < Puppet::Indirector::Terminus
     def find(request)
         return nil unless path = find_path(request)
 
-        result = model.new(request.key, :path => path)
+        result = model.new(path)
         result.links = request.options[:links] if request.options[:links]
         return result
     end
diff --git a/spec/integration/file_serving/metadata.rb b/spec/integration/file_serving/metadata.rb
index 067cb566a..af3e16324 100755
--- a/spec/integration/file_serving/metadata.rb
+++ b/spec/integration/file_serving/metadata.rb
@@ -15,4 +15,6 @@ describe Puppet::FileServing::Metadata, " when finding files" do
         @test_class = Puppet::FileServing::Metadata
         @indirection = Puppet::FileServing::Metadata.indirection
     end
+
+    after { Puppet::Util::Cacher.invalidate }
 end
diff --git a/spec/integration/indirector/direct_file_server.rb b/spec/integration/indirector/direct_file_server.rb
index 40b753a6c..6f3da5169 100755
--- a/spec/integration/indirector/direct_file_server.rb
+++ b/spec/integration/indirector/direct_file_server.rb
@@ -68,12 +68,12 @@ describe Puppet::Indirector::DirectFileServer, " when interacting with FileServi
         end
 
         @terminus.search(@request).each do |instance|
-            case instance.key
+            case instance.full_path
             when /one/: instance.content.should == "one content"
             when /two/: instance.content.should == "two content"
             when /\.$/: 
             else
-                raise "No valid key for %s" % instance.key.inspect
+                raise "No valid key for %s" % instance.path.inspect
             end
         end
     end
diff --git a/spec/integration/indirector/module_files.rb b/spec/integration/indirector/module_files.rb
index ae14aa5a7..6cbbd3dbd 100755
--- a/spec/integration/indirector/module_files.rb
+++ b/spec/integration/indirector/module_files.rb
@@ -20,7 +20,7 @@ describe Puppet::Indirector::ModuleFiles, " when interacting with Puppet::Module
 
         FileTest.expects(:exists?).with(filepath).returns(true)
 
-        @request = Puppet::Indirector::Request.new(:content, :find, "puppetmounts://host/modules/mymod/myfile")
+        @request = Puppet::Indirector::Request.new(:content, :find, "puppet://host/modules/mymod/myfile")
 
         @terminus.find(@request).should be_instance_of(Puppet::FileServing::Content)
     end
@@ -47,7 +47,7 @@ describe Puppet::Indirector::ModuleFiles, " when interacting with FileServing::F
 
         Dir.expects(:entries).with(filepath).returns(%w{one two})
 
-        @request = Puppet::Indirector::Request.new(:content, :search, "puppetmounts://host/modules/mymod/myfile", :recurse => true)
+        @request = Puppet::Indirector::Request.new(:content, :search, "puppet://host/modules/mymod/myfile", :recurse => true)
 
         result = @terminus.search(@request)
         result.should be_instance_of(Array)
diff --git a/spec/unit/file_serving/base.rb b/spec/unit/file_serving/base.rb
index 9d900e30a..7eccb3a52 100755
--- a/spec/unit/file_serving/base.rb
+++ b/spec/unit/file_serving/base.rb
@@ -5,64 +5,57 @@ require File.dirname(__FILE__) + '/../../spec_helper'
 require 'puppet/file_serving/base'
 
 describe Puppet::FileServing::Base do
-    it "should accept a key in the form of a URI" do
-        Puppet::FileServing::Base.new("puppet://host/module/dir/file").key.should == "puppet://host/module/dir/file"
+    it "should accept a path" do
+        Puppet::FileServing::Base.new("/module/dir/file").path.should == "/module/dir/file"
+    end
+
+    it "should require that paths be fully qualified" do
+        lambda { Puppet::FileServing::Base.new("module/dir/file") }.should raise_error(ArgumentError)
     end
 
     it "should allow specification of whether links should be managed" do
-        Puppet::FileServing::Base.new("puppet://host/module/dir/file", :links => :manage).links.should == :manage
+        Puppet::FileServing::Base.new("/module/dir/file", :links => :manage).links.should == :manage
     end
 
     it "should consider :ignore links equivalent to :manage links" do
-        Puppet::FileServing::Base.new("puppet://host/module/dir/file", :links => :ignore).links.should == :manage
+        Puppet::FileServing::Base.new("/module/dir/file", :links => :ignore).links.should == :manage
     end
 
     it "should fail if :links is set to anything other than :manage, :follow, or :ignore" do
-        proc { Puppet::FileServing::Base.new("puppet://host/module/dir/file", :links => :else) }.should raise_error(ArgumentError)
+        proc { Puppet::FileServing::Base.new("/module/dir/file", :links => :else) }.should raise_error(ArgumentError)
     end
 
     it "should default to :manage for :links" do
-        Puppet::FileServing::Base.new("puppet://host/module/dir/file").links.should == :manage
+        Puppet::FileServing::Base.new("/module/dir/file").links.should == :manage
     end
 
     it "should allow specification of a path" do
         FileTest.stubs(:exists?).returns(true)
-        Puppet::FileServing::Base.new("puppet://host/module/dir/file", :path => "/my/file").path.should == "/my/file"
+        Puppet::FileServing::Base.new("/module/dir/file", :path => "/my/file").path.should == "/my/file"
     end
 
     it "should allow specification of a relative path" do
         FileTest.stubs(:exists?).returns(true)
-        Puppet::FileServing::Base.new("puppet://host/module/dir/file", :relative_path => "my/file").relative_path.should == "my/file"
+        Puppet::FileServing::Base.new("/module/dir/file", :relative_path => "my/file").relative_path.should == "my/file"
     end
 
     it "should have a means of determining if the file exists" do
-        Puppet::FileServing::Base.new("blah").should respond_to(:exist?)
+        Puppet::FileServing::Base.new("/blah").should respond_to(:exist?)
     end
 
     it "should correctly indicate if the file is present" do
         File.expects(:lstat).with("/my/file").returns(mock("stat"))
-        Puppet::FileServing::Base.new("blah", :path => "/my/file").exist?.should be_true
+        Puppet::FileServing::Base.new("/my/file").exist?.should be_true
     end
 
-    it "should correctly indicate if the file is asbsent" do
+    it "should correctly indicate if the file is absent" do
         File.expects(:lstat).with("/my/file").raises RuntimeError
-        Puppet::FileServing::Base.new("blah", :path => "/my/file").exist?.should be_false
-    end
-
-    describe "when setting the base path" do
-        before do
-            @file = Puppet::FileServing::Base.new("puppet://host/module/dir/file")
-        end
-
-        it "should require that the base path be fully qualified" do
-            FileTest.stubs(:exists?).returns(true)
-            proc { @file.path = "unqualified/file" }.should raise_error(ArgumentError)
-        end
+        Puppet::FileServing::Base.new("/my/file").exist?.should be_false
     end
 
     describe "when setting the relative path" do
         it "should require that the relative path be unqualified" do
-            @file = Puppet::FileServing::Base.new("puppet://host/module/dir/file")
+            @file = Puppet::FileServing::Base.new("/module/dir/file")
             FileTest.stubs(:exists?).returns(true)
             proc { @file.relative_path = "/qualified/file" }.should raise_error(ArgumentError)
         end
@@ -70,7 +63,7 @@ describe Puppet::FileServing::Base do
 
     describe "when determining the full file path" do
         before do
-            @file = Puppet::FileServing::Base.new("mykey", :path => "/this/file")
+            @file = Puppet::FileServing::Base.new("/this/file")
         end
 
         it "should return the path if there is no relative path" do
@@ -86,16 +79,11 @@ describe Puppet::FileServing::Base do
             @file.relative_path = "not/qualified"
             @file.full_path.should == "/this/file/not/qualified"
         end
-
-        it "should should fail if there is no path set" do
-            @file = Puppet::FileServing::Base.new("not/qualified")
-            proc { @file.full_path }.should raise_error(ArgumentError)
-        end
     end
 
     describe "when stat'ing files" do
         before do
-            @file = Puppet::FileServing::Base.new("mykey", :path => "/this/file")
+            @file = Puppet::FileServing::Base.new("/this/file")
         end
 
         it "should stat the file's full path" do
diff --git a/spec/unit/file_serving/content.rb b/spec/unit/file_serving/content.rb
index 82353d23d..a63f7efab 100755
--- a/spec/unit/file_serving/content.rb
+++ b/spec/unit/file_serving/content.rb
@@ -18,15 +18,15 @@ describe Puppet::FileServing::Content do
     end
 
     it "should have a method for collecting its attributes" do
-        Puppet::FileServing::Content.new("sub/path", :path => "/base").should respond_to(:collect)
+        Puppet::FileServing::Content.new("/path").should respond_to(:collect)
     end
 
     it "should retrieve and store its contents when its attributes are collected" do
-        content = Puppet::FileServing::Content.new("sub/path", :path => "/base")
+        content = Puppet::FileServing::Content.new("/path")
 
         result = "foo"
         File.stubs(:lstat).returns(stub("stat", :ftype => "file"))
-        File.expects(:read).with("/base/sub/path").returns result
+        File.expects(:read).with("/path").returns result
         content.collect
         content.content.should equal(result)
     end
@@ -34,8 +34,8 @@ end
 
 describe Puppet::FileServing::Content, "when returning the contents" do
     before do
-        @path = "/my/base"
-        @content = Puppet::FileServing::Content.new("sub/path", :links => :follow, :path => @path)
+        @path = "/my/path"
+        @content = Puppet::FileServing::Content.new(@path, :links => :follow)
     end
 
     it "should fail if the file is a symlink and links are set to :manage" do
diff --git a/spec/unit/file_serving/metadata.rb b/spec/unit/file_serving/metadata.rb
index e76ceb3e8..f0e15873f 100755
--- a/spec/unit/file_serving/metadata.rb
+++ b/spec/unit/file_serving/metadata.rb
@@ -20,25 +20,22 @@ end
 
 describe Puppet::FileServing::Metadata, " when finding the file to use for setting attributes" do
     before do
-        @metadata = Puppet::FileServing::Metadata.new("my/path")
-
-        @full = "/base/path/my/path"
-
-        @metadata.path = @full
+        @path = "/my/path"
+        @metadata = Puppet::FileServing::Metadata.new(@path)
 
         # Use a link because it's easier to test -- no checksumming
         @stat = stub "stat", :uid => 10, :gid => 20, :mode => 0755, :ftype => "link"
     end
 
     it "should accept a base path path to which the file should be relative" do
-        File.expects(:lstat).with(@full).returns @stat
-        File.expects(:readlink).with(@full).returns "/what/ever"
+        File.expects(:lstat).with(@path).returns @stat
+        File.expects(:readlink).with(@path).returns "/what/ever"
         @metadata.collect_attributes
     end
 
     it "should use the set base path if one is not provided" do
-        File.expects(:lstat).with(@full).returns @stat
-        File.expects(:readlink).with(@full).returns "/what/ever"
+        File.expects(:lstat).with(@path).returns @stat
+        File.expects(:readlink).with(@path).returns "/what/ever"
         @metadata.collect_attributes()
     end
 
@@ -47,7 +44,7 @@ describe Puppet::FileServing::Metadata, " when finding the file to use for setti
     end
 
     it "should raise an exception if the file does not exist" do
-        File.expects(:lstat).with(@full).raises(Errno::ENOENT)
+        File.expects(:lstat).with(@path).raises(Errno::ENOENT)
         proc { @metadata.collect_attributes()}.should raise_error(Errno::ENOENT)
     end
 end
@@ -59,7 +56,7 @@ describe Puppet::FileServing::Metadata, " when collecting attributes" do
         @stat = stub 'stat', :uid => 10, :gid => 20, :mode => 33261, :ftype => "file"
         File.stubs(:lstat).returns(@stat)
         @checksum = Digest::MD5.hexdigest("some content\n")
-        @metadata = Puppet::FileServing::Metadata.new("file", :path => "/my/file")
+        @metadata = Puppet::FileServing::Metadata.new("/my/file")
         @metadata.stubs(:md5_file).returns(@checksum)
         @metadata.collect_attributes
     end
@@ -140,7 +137,7 @@ end
 
 describe Puppet::FileServing::Metadata, " when pointing to a link" do
     it "should store the destination of the link in :destination if links are :manage" do
-        file = Puppet::FileServing::Metadata.new("mykey", :links => :manage, :path => "/base/path/my/file")
+        file = Puppet::FileServing::Metadata.new("/base/path/my/file", :links => :manage)
 
         File.expects(:lstat).with("/base/path/my/file").returns stub("stat", :uid => 1, :gid => 2, :ftype => "link", :mode => 0755)
         File.expects(:readlink).with("/base/path/my/file").returns "/some/other/path"
@@ -150,7 +147,7 @@ describe Puppet::FileServing::Metadata, " when pointing to a link" do
     end
 
     it "should not collect the checksum" do
-        file = Puppet::FileServing::Metadata.new("my/file", :links => :manage, :path => "/base/path/my/file")
+        file = Puppet::FileServing::Metadata.new("/base/path/my/file", :links => :manage)
 
         File.expects(:lstat).with("/base/path/my/file").returns stub("stat", :uid => 1, :gid => 2, :ftype => "link", :mode => 0755)
         File.expects(:readlink).with("/base/path/my/file").returns "/some/other/path"
@@ -159,13 +156,3 @@ describe Puppet::FileServing::Metadata, " when pointing to a link" do
         file.checksum.should be_nil
     end
 end
-
-describe Puppet::FileServing::Metadata, " when converting from yaml" do
-    # LAK:FIXME This isn't in the right place, but we need some kind of
-    # control somewhere that requires that all REST connections only pull
-    # from the file-server, thus guaranteeing they go through our authorization
-    # hook.
-    it "should set the URI scheme to 'puppetmounts'" do
-        pending "We need to figure out where this should be"
-    end
-end
diff --git a/spec/unit/file_serving/terminus_helper.rb b/spec/unit/file_serving/terminus_helper.rb
index 763ce9eb0..aa9dca961 100755
--- a/spec/unit/file_serving/terminus_helper.rb
+++ b/spec/unit/file_serving/terminus_helper.rb
@@ -45,15 +45,9 @@ describe Puppet::FileServing::TerminusHelper do
             @helper.path2instances(@request, "/my/file").length.should == 2
         end
 
-        it "should set each instance's key to be the original key plus the file-specific path" do
-            @model.expects(:new).with { |key, options| key == @request.key + "/one" }.returns(:one)
-            @model.expects(:new).with { |key, options| key == @request.key + "/two" }.returns(:two)
-            @helper.path2instances(@request, "/my/file")
-        end
-
         it "should set each returned instance's path to the original path" do
-            @model.expects(:new).with { |key, options| options[:path] == "/my/file" }.returns(:one)
-            @model.expects(:new).with { |key, options| options[:path] == "/my/file" }.returns(:two)
+            @model.expects(:new).with { |key, options| key == "/my/file" }.returns(:one)
+            @model.expects(:new).with { |key, options| key == "/my/file" }.returns(:two)
             @helper.path2instances(@request, "/my/file")
         end
 
diff --git a/spec/unit/indirector/direct_file_server.rb b/spec/unit/indirector/direct_file_server.rb
index 0753f1bbe..b80519bbe 100755
--- a/spec/unit/indirector/direct_file_server.rb
+++ b/spec/unit/indirector/direct_file_server.rb
@@ -24,7 +24,7 @@ describe Puppet::Indirector::DirectFileServer do
 
         @uri = "file:///my/local"
 
-        @request = stub 'request', :key => @uri, :options => {}
+        @request = Puppet::Indirector::Request.new(:mytype, :find, @uri)
     end
 
     describe Puppet::Indirector::DirectFileServer, "when finding a single file" do
@@ -49,13 +49,8 @@ describe Puppet::Indirector::DirectFileServer do
             FileTest.expects(:exists?).with("/my/local").returns true
         end
 
-        it "should create the Content instance with the original key as the key" do
-            @model.expects(:new).with { |key, options| key == @uri }.returns(@data)
-            @server.find(@request)
-        end
-
         it "should pass the full path to the instance" do
-            @model.expects(:new).with { |key, options| options[:path] == "/my/local" }.returns(@data)
+            @model.expects(:new).with { |key, options| key == "/my/local" }.returns(@data)
             @server.find(@request)
         end
 
diff --git a/spec/unit/indirector/file_metadata/file.rb b/spec/unit/indirector/file_metadata/file.rb
index 9474620c7..b37c42c72 100755
--- a/spec/unit/indirector/file_metadata/file.rb
+++ b/spec/unit/indirector/file_metadata/file.rb
@@ -24,7 +24,7 @@ describe Puppet::Indirector::FileMetadata::File do
             @data.stubs(:collect_attributes)
             FileTest.expects(:exists?).with("/my/local").returns true
 
-            @request = stub 'request', :key => @uri, :options => {}
+            @request = Puppet::Indirector::Request.new(:file_metadata, :find, @uri)
         end
 
         it "should collect its attributes when a file is found" do
@@ -40,7 +40,7 @@ describe Puppet::Indirector::FileMetadata::File do
             @metadata = Puppet::Indirector::FileMetadata::File.new
             @uri = "file:///my/local"
 
-            @request = stub 'request', :key => @uri, :options => {}
+            @request = Puppet::Indirector::Request.new(:file_metadata, :find, @uri)
         end
 
         it "should collect the attributes of the instances returned" do
diff --git a/spec/unit/indirector/file_server.rb b/spec/unit/indirector/file_server.rb
index 544953932..90fb84d35 100755
--- a/spec/unit/indirector/file_server.rb
+++ b/spec/unit/indirector/file_server.rb
@@ -67,13 +67,8 @@ describe Puppet::Indirector::FileServer do
             @instance = mock 'instance'
         end
 
-        it "should create the instance with the key used to find the instance" do
-            @model.expects(:new).with { |key, *options| key == "my/local/file" }
-            @file_server.find(@request)
-        end
-
         it "should create the instance with the path at which the instance was found" do
-            @model.expects(:new).with { |key, options| options[:path] == "/some/file" }
+            @model.expects(:new).with { |key, options| key == "/some/file" }
             @file_server.find(@request)
         end
 
-- 
cgit 


From 40e76fb83ef466425fec736abbf1913a6426bf01 Mon Sep 17 00:00:00 2001
From: Luke Kanies <luke@madstop.com>
Date: Sun, 24 Aug 2008 20:53:25 -0500
Subject: Fixing the rest backends for webrick and mongrel so the get the whole
 request key.

Also adding the Content work necessary to demonstrate that this is actually
required.

Signed-off-by: Luke Kanies <luke@madstop.com>
---
 lib/puppet/file_serving/content.rb      | 16 +++++++++++++++-
 lib/puppet/network/http/mongrel/rest.rb |  2 +-
 lib/puppet/network/http/webrick/rest.rb |  2 +-
 spec/unit/file_serving/content.rb       | 12 +++++++++++-
 spec/unit/network/http/mongrel/rest.rb  |  6 +++---
 spec/unit/network/http/webrick/rest.rb  |  6 +++---
 6 files changed, 34 insertions(+), 10 deletions(-)

diff --git a/lib/puppet/file_serving/content.rb b/lib/puppet/file_serving/content.rb
index f13fcaa72..b30070dd6 100644
--- a/lib/puppet/file_serving/content.rb
+++ b/lib/puppet/file_serving/content.rb
@@ -14,9 +14,19 @@ class Puppet::FileServing::Content < Puppet::FileServing::Base
     extend Puppet::Indirector
     indirects :file_content, :extend => Puppet::FileServing::IndirectionHooks
 
-    attr_reader :path
+    def self.supported_formats
+        [:raw]
+    end
+
+    def self.from_raw(content)
+        instance = new("eh")
+        instance.content = content
+        instance
+    end
 
+    # Collect our data.
     def collect
+        content
     end
 
     # Read the content of our file in.
@@ -29,4 +39,8 @@ class Puppet::FileServing::Content < Puppet::FileServing::Base
         end
         @content
     end
+
+    def to_raw
+        content
+    end
 end
diff --git a/lib/puppet/network/http/mongrel/rest.rb b/lib/puppet/network/http/mongrel/rest.rb
index d265dde86..45d21ea62 100644
--- a/lib/puppet/network/http/mongrel/rest.rb
+++ b/lib/puppet/network/http/mongrel/rest.rb
@@ -35,7 +35,7 @@ class Puppet::Network::HTTP::MongrelREST < Mongrel::HttpHandler
     # return the key included in the request path
     def request_key(request)
         # LAK:NOTE See http://snurl.com/21zf8  [groups_google_com] 
-        x = request.params[Mongrel::Const::REQUEST_PATH].split('/')[2]        
+        x = request.params[Mongrel::Const::REQUEST_PATH].split('/', 3)[2]
     end
 
     # return the request body
diff --git a/lib/puppet/network/http/webrick/rest.rb b/lib/puppet/network/http/webrick/rest.rb
index 13f795fb2..f06914365 100644
--- a/lib/puppet/network/http/webrick/rest.rb
+++ b/lib/puppet/network/http/webrick/rest.rb
@@ -36,7 +36,7 @@ class Puppet::Network::HTTP::WEBrickREST < WEBrick::HTTPServlet::AbstractServlet
 
     def request_key(request)
         # LAK:NOTE See http://snurl.com/21zf8  [groups_google_com] 
-        x = request.path.split('/')[2]
+        x = request.path.split('/', 3)[2]
     end
 
     def body(request)
diff --git a/spec/unit/file_serving/content.rb b/spec/unit/file_serving/content.rb
index a63f7efab..5441dff53 100755
--- a/spec/unit/file_serving/content.rb
+++ b/spec/unit/file_serving/content.rb
@@ -28,7 +28,8 @@ describe Puppet::FileServing::Content do
         File.stubs(:lstat).returns(stub("stat", :ftype => "file"))
         File.expects(:read).with("/path").returns result
         content.collect
-        content.content.should equal(result)
+
+        content.instance_variable_get("@content").should_not be_nil
     end
 end
 
@@ -58,4 +59,13 @@ describe Puppet::FileServing::Content, "when returning the contents" do
         File.expects(:read).with(@path).returns(:mycontent)
         @content.content.should == :mycontent
     end
+
+    it "should cache the returned contents" do
+        File.expects(:stat).with(@path).returns stub("stat", :ftype => "file")
+        File.expects(:read).with(@path).returns(:mycontent)
+        @content.content
+
+        # The second run would throw a failure if the content weren't being cached.
+        @content.content
+    end
 end
diff --git a/spec/unit/network/http/mongrel/rest.rb b/spec/unit/network/http/mongrel/rest.rb
index 3b248dcfe..9dbc1c9ab 100755
--- a/spec/unit/network/http/mongrel/rest.rb
+++ b/spec/unit/network/http/mongrel/rest.rb
@@ -53,9 +53,9 @@ describe "Puppet::Network::HTTP::MongrelREST" do
                 @handler.path(@request).should == "/foo"
             end
 
-            it "should use the second part of the request path as the request key" do
-                @params.expects(:[]).with(Mongrel::Const::REQUEST_PATH).returns "/foo/bar"
-                @handler.request_key(@request).should == "bar"
+            it "should use the remainder of the request path as the request key" do
+                @params.expects(:[]).with(Mongrel::Const::REQUEST_PATH).returns "/foo/bar/baz"
+                @handler.request_key(@request).should == "bar/baz"
             end
 
             it "should return nil as the request key if no second field is present" do
diff --git a/spec/unit/network/http/webrick/rest.rb b/spec/unit/network/http/webrick/rest.rb
index 620201472..1f4ccbe29 100755
--- a/spec/unit/network/http/webrick/rest.rb
+++ b/spec/unit/network/http/webrick/rest.rb
@@ -47,9 +47,9 @@ describe Puppet::Network::HTTP::WEBrickREST do
                 @handler.path(@request).should == "/foo"
             end
 
-            it "should return the second field in the path as the request key" do
-                @request.expects(:path).returns "/foo/bar"
-                @handler.request_key(@request).should == "bar"
+            it "should return the remainder of the path as the request key" do
+                @request.expects(:path).returns "/foo/bar/baz"
+                @handler.request_key(@request).should == "bar/baz"
             end
 
             it "should return nil as the request key if there is no second field" do
-- 
cgit 


From 30dea6839b0360e2fabbeb833e6c2b8658d3f53c Mon Sep 17 00:00:00 2001
From: Luke Kanies <luke@madstop.com>
Date: Sun, 24 Aug 2008 20:54:47 -0500
Subject: Adding a 'plural?' method to the Indirection::Request class.

I'm in the process of creating a new service for handling
all of the http calls, including generation of the RESTian
URL.  This service obviously needs to know whether the url should
be plural or singular, and the request knows the method in use,
so it can easily answer the question.

Signed-off-by: Luke Kanies <luke@madstop.com>
---
 lib/puppet/indirector/request.rb | 10 ++++++++--
 spec/unit/indirector/request.rb  | 12 ++++++++++++
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/lib/puppet/indirector/request.rb b/lib/puppet/indirector/request.rb
index 194b9e031..49cc01aab 100644
--- a/lib/puppet/indirector/request.rb
+++ b/lib/puppet/indirector/request.rb
@@ -1,7 +1,8 @@
 require 'puppet/indirector'
 
-# Provide any attributes or functionality needed for indirected
-# instances.
+# This class encapsulates all of the information you need to make an
+# Indirection call, and as a a result also handles REST calls.  It's somewhat
+# analogous to an HTTP Request object, except tuned for our Indirector.
 class Puppet::Indirector::Request
     attr_accessor :indirection_name, :key, :method, :options, :instance, :node, :ip, :authenticated
 
@@ -50,6 +51,11 @@ class Puppet::Indirector::Request
         Puppet::Indirector::Indirection.instance(@indirection_name)
     end
 
+    # Are we trying to interact with multiple resources, or just one?
+    def plural?
+        method == :search
+    end
+
     private
 
     # Parse the key as a URI, setting attributes appropriately.
diff --git a/spec/unit/indirector/request.rb b/spec/unit/indirector/request.rb
index 6169a09b4..18f625946 100755
--- a/spec/unit/indirector/request.rb
+++ b/spec/unit/indirector/request.rb
@@ -135,4 +135,16 @@ describe Puppet::Indirector::Request do
 
         request.indirection.should equal(ind)
     end
+
+    it "should have a method for determining if the request is plural or singular" do
+        Puppet::Indirector::Request.new(:myind, :method, :key).should respond_to(:plural?)
+    end
+
+    it "should be considered plural if the method is 'search'" do
+        Puppet::Indirector::Request.new(:myind, :search, :key).should be_plural
+    end
+
+    it "should not be considered plural if the method is not 'search'" do
+        Puppet::Indirector::Request.new(:myind, :find, :key).should_not be_plural
+    end
 end
-- 
cgit 


From deda6465f50b582f3b8c77204965db331ba81faa Mon Sep 17 00:00:00 2001
From: Luke Kanies <luke@madstop.com>
Date: Sun, 24 Aug 2008 21:00:55 -0500
Subject: Removing the last vestiges of the 'puppetmounts' protocol marker.

I created this when I first designed the fileserving
Indirection hooks, and it's unnecessary.

Signed-off-by: Luke Kanies <luke@madstop.com>
---
 spec/shared_behaviours/file_server_terminus.rb | 2 +-
 spec/unit/file_serving/indirection_hooks.rb    | 2 +-
 spec/unit/indirector/file_server.rb            | 3 +--
 3 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/spec/shared_behaviours/file_server_terminus.rb b/spec/shared_behaviours/file_server_terminus.rb
index 0230d39e8..91e1b2dca 100644
--- a/spec/shared_behaviours/file_server_terminus.rb
+++ b/spec/shared_behaviours/file_server_terminus.rb
@@ -25,7 +25,7 @@ describe "Puppet::Indirector::FileServerTerminus", :shared => true do
         # Stub out the modules terminus
         @modules = mock 'modules terminus'
 
-        @request = Puppet::Indirector::Request.new(:indirection, :method, "puppetmounts://myhost/one/my/file")
+        @request = Puppet::Indirector::Request.new(:indirection, :method, "puppet://myhost/one/my/file")
     end
 
     it "should use the file server configuration to find files" do
diff --git a/spec/unit/file_serving/indirection_hooks.rb b/spec/unit/file_serving/indirection_hooks.rb
index 12993bf1b..83ff5848f 100755
--- a/spec/unit/file_serving/indirection_hooks.rb
+++ b/spec/unit/file_serving/indirection_hooks.rb
@@ -90,7 +90,7 @@ describe Puppet::FileServing::IndirectionHooks do
                 @object.stubs(:terminus).with(:modules).returns(modules)
                 modules.stubs(:find_module).returns(nil)
 
-                @request.stubs(:key).returns "puppetmounts://host/notmodules/file"
+                @request.stubs(:key).returns "notmodules/file"
 
                 @object.select_terminus(@request).should == :file_server
             end
diff --git a/spec/unit/indirector/file_server.rb b/spec/unit/indirector/file_server.rb
index 90fb84d35..ab8e32566 100755
--- a/spec/unit/indirector/file_server.rb
+++ b/spec/unit/indirector/file_server.rb
@@ -103,7 +103,6 @@ describe Puppet::Indirector::FileServer do
         
         describe "and finding file information" do
             before do
-                @request.key =  "puppetmounts://host/my/file"
                 @request.method = :find 
             end
 
@@ -113,7 +112,7 @@ describe Puppet::Indirector::FileServer do
             end
 
             it "should pass the file path from the URI to the file server configuration" do
-                @configuration.expects(:authorized?).with { |uri, *args| uri == "/my/file" }
+                @configuration.expects(:authorized?).with { |uri, *args| uri == "my/local/file" }
                 @file_server.authorized?(@request)
             end
 
-- 
cgit 


From 151a54ff7ac69aa2fa1708188ad75e444158e8a2 Mon Sep 17 00:00:00 2001
From: Luke Kanies <luke@madstop.com>
Date: Sun, 24 Aug 2008 21:17:15 -0500
Subject: Causing format selection to fail intelligently if no suitable format
 can be picked.

Signed-off-by: Luke Kanies <luke@madstop.com>
---
 lib/puppet/network/http/handler.rb | 13 ++++++++++++-
 spec/unit/network/http/handler.rb  | 31 +++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/lib/puppet/network/http/handler.rb b/lib/puppet/network/http/handler.rb
index 6f5117b16..7c7abccf5 100644
--- a/lib/puppet/network/http/handler.rb
+++ b/lib/puppet/network/http/handler.rb
@@ -12,7 +12,18 @@ module Puppet::Network::HTTP::Handler
     # Which format to use when serializing our response.  Just picks
     # the first value in the accept header, at this point.
     def format_to_use(request)
-        accept_header(request).split(/,\s*/)[0]
+        unless header = accept_header(request)
+            raise ArgumentError, "An Accept header must be provided to pick the right format"
+        end
+
+        format = nil
+        header.split(/,\s*/).each do |name|
+            next unless format = Puppet::Network::FormatHandler.format(name)
+            next unless format.suitable?
+            return name
+        end
+
+        raise "No specified acceptable formats (%s) are functional on this machine" % header
     end
 
     def initialize_for_puppet(args = {})
diff --git a/spec/unit/network/http/handler.rb b/spec/unit/network/http/handler.rb
index 6fc932091..ed0f25121 100755
--- a/spec/unit/network/http/handler.rb
+++ b/spec/unit/network/http/handler.rb
@@ -224,6 +224,9 @@ describe Puppet::Network::HTTP::Handler do
                 @handler.stubs(:singular?).returns(true)
                 @handler.stubs(:request_key).returns('key')
                 @model_class.stubs(:find).returns @result
+
+                @format = stub 'format', :suitable? => true
+                Puppet::Network::FormatHandler.stubs(:format).returns @format
             end
 
             it "should fail to find model if key is not specified" do
@@ -246,6 +249,28 @@ describe Puppet::Network::HTTP::Handler do
                 @handler.do_find(@request, @response)
             end
 
+            it "should fail if no accept header is provided" do
+                @handler.expects(:accept_header).with(@request).returns nil
+                lambda { @handler.do_find(@request, @response) }.should raise_error(ArgumentError)
+            end
+
+            it "should fail if the accept header does not contain a valid format" do
+                @handler.expects(:accept_header).with(@request).returns ""
+                lambda { @handler.do_find(@request, @response) }.should raise_error(RuntimeError)
+            end
+
+            it "should not use an unsuitable format" do
+                @handler.expects(:accept_header).with(@request).returns "foo,bar"
+                foo = mock 'foo', :suitable? => false
+                bar = mock 'bar', :suitable? => true
+                Puppet::Network::FormatHandler.expects(:format).with("foo").returns foo
+                Puppet::Network::FormatHandler.expects(:format).with("bar").returns bar
+
+                @handler.expects(:set_content_type).with(@response, "bar") # the suitable one
+
+                @handler.do_find(@request, @response)
+            end
+
             it "should render the result using the first format specified in the accept header" do
                 @handler.expects(:accept_header).with(@request).returns "one,two"
                 @result.expects(:render).with("one")
@@ -298,6 +323,9 @@ describe Puppet::Network::HTTP::Handler do
                 @result = [@result1, @result2]
                 @model_class.stubs(:render_multiple).returns "my rendered instances"
                 @model_class.stubs(:search).returns(@result)
+
+                @format = stub 'format', :suitable? => true
+                Puppet::Network::FormatHandler.stubs(:format).returns @format
             end
 
             it "should use a common method for determining the request parameters" do
@@ -409,6 +437,9 @@ describe Puppet::Network::HTTP::Handler do
 
                 @model_instance = stub('indirected model instance', :save => true)
                 @model_class.stubs(:convert_from).returns(@model_instance)
+
+                @format = stub 'format', :suitable? => true
+                Puppet::Network::FormatHandler.stubs(:format).returns @format
             end
 
             it "should use the 'body' hook to retrieve the body of the request" do
-- 
cgit 


From 92e144b3051ebd177c034e692a59162b3902e128 Mon Sep 17 00:00:00 2001
From: Luke Kanies <luke@madstop.com>
Date: Mon, 25 Aug 2008 17:35:28 -0500
Subject: Fixing a test in the module_files terminus

Signed-off-by: Luke Kanies <luke@madstop.com>
---
 spec/unit/indirector/module_files.rb | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/spec/unit/indirector/module_files.rb b/spec/unit/indirector/module_files.rb
index 20c9f499a..f5f9527df 100755
--- a/spec/unit/indirector/module_files.rb
+++ b/spec/unit/indirector/module_files.rb
@@ -96,13 +96,8 @@ describe Puppet::Indirector::ModuleFiles do
             @instance = mock 'instance'
         end
 
-        it "should create the instance with the key used to find the instance" do
-            @model.expects(:new).with { |key, *options| key == @request.key }
-            @module_files.find(@request)
-        end
-
         it "should create the instance with the path at which the instance was found" do
-            @model.expects(:new).with { |key, options| options[:path] == "/module/path/files/local/file" }
+            @model.expects(:new).with { |key, options| key == "/module/path/files/local/file" }
             @module_files.find(@request)
         end
 
-- 
cgit 


From 6ed8dfaf7c0cf091dca0374de310f524b0a033cc Mon Sep 17 00:00:00 2001
From: Luke Kanies <luke@madstop.com>
Date: Mon, 25 Aug 2008 17:43:35 -0500
Subject: Adding the content writer to the content class.

Also choosing a fully qualified fake name when creating
content instances from raw content.

Signed-off-by: Luke Kanies <luke@madstop.com>
---
 lib/puppet/file_serving/content.rb |  4 +++-
 spec/unit/file_serving/content.rb  | 28 ++++++++++++++++++++++++++++
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/lib/puppet/file_serving/content.rb b/lib/puppet/file_serving/content.rb
index b30070dd6..0f169c28b 100644
--- a/lib/puppet/file_serving/content.rb
+++ b/lib/puppet/file_serving/content.rb
@@ -14,12 +14,14 @@ class Puppet::FileServing::Content < Puppet::FileServing::Base
     extend Puppet::Indirector
     indirects :file_content, :extend => Puppet::FileServing::IndirectionHooks
 
+    attr_writer :content
+
     def self.supported_formats
         [:raw]
     end
 
     def self.from_raw(content)
-        instance = new("eh")
+        instance = new("/this/is/a/fake/path")
         instance.content = content
         instance
     end
diff --git a/spec/unit/file_serving/content.rb b/spec/unit/file_serving/content.rb
index 5441dff53..e471c3b29 100755
--- a/spec/unit/file_serving/content.rb
+++ b/spec/unit/file_serving/content.rb
@@ -17,6 +17,10 @@ describe Puppet::FileServing::Content do
         Puppet::FileServing::Content.indirection.metaclass.included_modules.should include(Puppet::FileServing::IndirectionHooks)
     end
 
+    it "should only support the raw format" do
+        Puppet::FileServing::Content.supported_formats.should == [:raw]
+    end
+
     it "should have a method for collecting its attributes" do
         Puppet::FileServing::Content.new("/path").should respond_to(:collect)
     end
@@ -31,6 +35,30 @@ describe Puppet::FileServing::Content do
 
         content.instance_variable_get("@content").should_not be_nil
     end
+
+    it "should have a method for setting its content" do
+        content = Puppet::FileServing::Content.new("/path")
+        content.should respond_to(:content=)
+    end
+
+    it "should make content available when set externally" do
+        content = Puppet::FileServing::Content.new("/path")
+        content.content = "foo/bar"
+        content.content.should == "foo/bar"
+    end
+
+    it "should be able to create a content instance from raw file contents" do
+        Puppet::FileServing::Content.should respond_to(:from_raw)
+    end
+
+    it "should create an instance with a fake file name and correct content when converting from raw" do
+        instance = mock 'instance'
+        Puppet::FileServing::Content.expects(:new).with("/this/is/a/fake/path").returns instance
+
+        instance.expects(:content=).with "foo/bar"
+
+        Puppet::FileServing::Content.from_raw("foo/bar").should equal(instance)
+    end
 end
 
 describe Puppet::FileServing::Content, "when returning the contents" do
-- 
cgit 


From 8b45d13ab28837caf3bb09cc1c90ab61974bf4db Mon Sep 17 00:00:00 2001
From: Luke Kanies <luke@madstop.com>
Date: Mon, 25 Aug 2008 18:00:42 -0700
Subject: Adding automatic attribute collection to the new fileserving code.

Basically, this just includes a consistent method for collecting
info (either content or metadata) and then calls that method
when returning instances via the indirector.

It's such a large commit mostly because of small changes in the normal
code and large changes in the testing to accomodate those small changes.

Signed-off-by: Luke Kanies <luke@madstop.com>
---
 lib/puppet/file_serving/content.rb                |  1 +
 lib/puppet/file_serving/fileset.rb                |  2 +-
 lib/puppet/file_serving/metadata.rb               |  2 +-
 lib/puppet/file_serving/terminus_helper.rb        |  1 +
 lib/puppet/indirector/file_metadata/file.rb       |  4 +--
 lib/puppet/indirector/file_metadata/modules.rb    |  2 +-
 lib/puppet/indirector/file_server.rb              |  1 +
 lib/puppet/network/handler/fileserver.rb          |  2 +-
 spec/integration/indirector/direct_file_server.rb | 27 +++++++------------
 spec/integration/indirector/module_files.rb       | 24 ++++++++---------
 spec/shared_behaviours/file_server_terminus.rb    | 20 +++++++-------
 spec/unit/file_serving/content.rb                 | 13 ++++++++-
 spec/unit/file_serving/metadata.rb                | 22 +++++++++-------
 spec/unit/file_serving/terminus_helper.rb         | 32 ++++++++++++++++-------
 spec/unit/indirector/direct_file_server.rb        |  2 +-
 spec/unit/indirector/file_metadata/file.rb        |  6 ++---
 spec/unit/indirector/file_metadata/modules.rb     |  2 +-
 spec/unit/indirector/file_server.rb               | 23 ++++++++++++----
 18 files changed, 112 insertions(+), 74 deletions(-)

diff --git a/lib/puppet/file_serving/content.rb b/lib/puppet/file_serving/content.rb
index 0f169c28b..c1ecff749 100644
--- a/lib/puppet/file_serving/content.rb
+++ b/lib/puppet/file_serving/content.rb
@@ -28,6 +28,7 @@ class Puppet::FileServing::Content < Puppet::FileServing::Base
 
     # Collect our data.
     def collect
+        return if stat.ftype == "directory"
         content
     end
 
diff --git a/lib/puppet/file_serving/fileset.rb b/lib/puppet/file_serving/fileset.rb
index fe54350b1..80a718c68 100644
--- a/lib/puppet/file_serving/fileset.rb
+++ b/lib/puppet/file_serving/fileset.rb
@@ -20,7 +20,7 @@ class Puppet::FileServing::Fileset
 
         # Now strip off the leading path, so each file becomes relative, and remove
         # any slashes that might end up at the beginning of the path.
-        result = files.collect { |file| file.sub(%r{^#{@path}/*}, '') }
+        result = files.collect { |file| file.sub(@path, '').sub(%r{^/},'') }
 
         # And add the path itself.
         result.unshift(".")
diff --git a/lib/puppet/file_serving/metadata.rb b/lib/puppet/file_serving/metadata.rb
index a1265dd8b..1cc3fa355 100644
--- a/lib/puppet/file_serving/metadata.rb
+++ b/lib/puppet/file_serving/metadata.rb
@@ -47,7 +47,7 @@ class Puppet::FileServing::Metadata < Puppet::FileServing::Base
     # Retrieve the attributes for this file, relative to a base directory.
     # Note that File.stat raises Errno::ENOENT if the file is absent and this
     # method does not catch that exception.
-    def collect_attributes
+    def collect
         real_path = full_path()
         stat = stat()
         @owner = stat.uid
diff --git a/lib/puppet/file_serving/terminus_helper.rb b/lib/puppet/file_serving/terminus_helper.rb
index bde0bd389..598a5007a 100644
--- a/lib/puppet/file_serving/terminus_helper.rb
+++ b/lib/puppet/file_serving/terminus_helper.rb
@@ -13,6 +13,7 @@ module Puppet::FileServing::TerminusHelper
         Puppet::FileServing::Fileset.new(path, args).files.collect do |file|
             inst = model.new(path, :relative_path => file)
             inst.links = request.options[:links] if request.options[:links]
+            inst.collect
             inst
         end
     end
diff --git a/lib/puppet/indirector/file_metadata/file.rb b/lib/puppet/indirector/file_metadata/file.rb
index c46015c38..bb586489d 100644
--- a/lib/puppet/indirector/file_metadata/file.rb
+++ b/lib/puppet/indirector/file_metadata/file.rb
@@ -11,7 +11,7 @@ class Puppet::Indirector::FileMetadata::File < Puppet::Indirector::DirectFileSer
 
     def find(request)
         return unless data = super
-        data.collect_attributes
+        data.collect
 
         return data
     end
@@ -19,7 +19,7 @@ class Puppet::Indirector::FileMetadata::File < Puppet::Indirector::DirectFileSer
     def search(request)
         return unless result = super
 
-        result.each { |instance| instance.collect_attributes }
+        result.each { |instance| instance.collect }
 
         return result
     end
diff --git a/lib/puppet/indirector/file_metadata/modules.rb b/lib/puppet/indirector/file_metadata/modules.rb
index 5ed7a8a45..4598c2175 100644
--- a/lib/puppet/indirector/file_metadata/modules.rb
+++ b/lib/puppet/indirector/file_metadata/modules.rb
@@ -11,7 +11,7 @@ class Puppet::Indirector::FileMetadata::Modules < Puppet::Indirector::ModuleFile
 
     def find(*args)
         return unless instance = super
-        instance.collect_attributes
+        instance.collect
         instance
     end
 end
diff --git a/lib/puppet/indirector/file_server.rb b/lib/puppet/indirector/file_server.rb
index 476fc5b23..46a590f9c 100644
--- a/lib/puppet/indirector/file_server.rb
+++ b/lib/puppet/indirector/file_server.rb
@@ -27,6 +27,7 @@ class Puppet::Indirector::FileServer < Puppet::Indirector::Terminus
         return nil unless path = find_path(request)
         result =  model.new(path)
         result.links = request.options[:links] if request.options[:links]
+        result.collect
         return result
     end
 
diff --git a/lib/puppet/network/handler/fileserver.rb b/lib/puppet/network/handler/fileserver.rb
index 183979429..14319ef96 100755
--- a/lib/puppet/network/handler/fileserver.rb
+++ b/lib/puppet/network/handler/fileserver.rb
@@ -75,7 +75,7 @@ class Puppet::Network::Handler
             return "" unless metadata.exist?
 
             begin
-                metadata.collect_attributes
+                metadata.collect
             rescue => detail
                 puts detail.backtrace if Puppet[:trace]
                 Puppet.err detail
diff --git a/spec/integration/indirector/direct_file_server.rb b/spec/integration/indirector/direct_file_server.rb
index 6f3da5169..417d661e8 100755
--- a/spec/integration/indirector/direct_file_server.rb
+++ b/spec/integration/indirector/direct_file_server.rb
@@ -37,21 +37,19 @@ describe Puppet::Indirector::DirectFileServer, " when interacting with FileServi
     before do
         @terminus = Puppet::Indirector::FileContent::File.new
 
-        @filepath = "/my/file"
-        FileTest.stubs(:exists?).with(@filepath).returns(true)
+        @path = Tempfile.new("direct_file_server_testing")
+        @path.close!
+        @path = @path.path
 
-        stat = stub 'stat', :directory? => true
-        File.stubs(:lstat).with(@filepath).returns(stat)
+        Dir.mkdir(@path)
+        File.open(File.join(@path, "one"), "w") { |f| f.print "one content" }
+        File.open(File.join(@path, "two"), "w") { |f| f.print "two content" }
 
-        @subfiles = %w{one two}
-        @subfiles.each do |f|
-            path = File.join(@filepath, f)
-            FileTest.stubs(:exists?).with(@path).returns(true)
-        end
-
-        Dir.expects(:entries).with(@filepath).returns @subfiles
+        @request = @terminus.indirection.request(:search, "file:///%s" % @path, :recurse => true)
+    end
 
-        @request = @terminus.indirection.request(:search, "file:///my/file", :recurse => true)
+    after do
+        system("rm -rf %s" % @path)
     end
 
     it "should return an instance for every file in the fileset" do
@@ -62,11 +60,6 @@ describe Puppet::Indirector::DirectFileServer, " when interacting with FileServi
     end
 
     it "should return instances capable of returning their content" do
-        @subfiles.each do |name|
-            File.stubs(:lstat).with(File.join(@filepath, name)).returns stub("#{name} stat", :ftype => "file", :directory? => false)
-            File.expects(:read).with(File.join(@filepath, name)).returns("#{name} content")
-        end
-
         @terminus.search(@request).each do |instance|
             case instance.full_path
             when /one/: instance.content.should == "one content"
diff --git a/spec/integration/indirector/module_files.rb b/spec/integration/indirector/module_files.rb
index 6cbbd3dbd..a54588ec5 100755
--- a/spec/integration/indirector/module_files.rb
+++ b/spec/integration/indirector/module_files.rb
@@ -30,22 +30,22 @@ describe Puppet::Indirector::ModuleFiles, " when interacting with FileServing::F
     it "should return an instance for every file in the fileset" do
         Puppet::Node::Environment.stubs(:new).returns(stub('env', :name => "myenv"))
         @terminus = Puppet::Indirector::FileContent::Modules.new
-        @module = Puppet::Module.new("mymod", "/some/path/mymod")
-        Puppet::Module.expects(:find).with("mymod", "myenv").returns(@module)
 
-        filepath = "/some/path/mymod/files/myfile"
-        FileTest.stubs(:exists?).with(filepath).returns(true)
+        @path = Tempfile.new("module_file_testing")
+        @path.close!
+        @path = @path.path
 
-        stat = stub 'stat', :directory? => true
-        File.stubs(:lstat).with(filepath).returns(stat)
+        Dir.mkdir(@path)
+        Dir.mkdir(File.join(@path, "files"))
 
-        subfiles = %w{one two}
-        subfiles.each do |f|
-            path = File.join(filepath, f)
-            FileTest.stubs(:exists?).with(path).returns(true)
-        end
+        basedir = File.join(@path, "files", "myfile")
+        Dir.mkdir(basedir)
 
-        Dir.expects(:entries).with(filepath).returns(%w{one two})
+        File.open(File.join(basedir, "one"), "w") { |f| f.print "one content" }
+        File.open(File.join(basedir, "two"), "w") { |f| f.print "two content" }
+
+        @module = Puppet::Module.new("mymod", @path)
+        Puppet::Module.expects(:find).with("mymod", "myenv").returns(@module)
 
         @request = Puppet::Indirector::Request.new(:content, :search, "puppet://host/modules/mymod/myfile", :recurse => true)
 
diff --git a/spec/shared_behaviours/file_server_terminus.rb b/spec/shared_behaviours/file_server_terminus.rb
index 91e1b2dca..fc5673a65 100644
--- a/spec/shared_behaviours/file_server_terminus.rb
+++ b/spec/shared_behaviours/file_server_terminus.rb
@@ -8,14 +8,19 @@ describe "Puppet::Indirector::FileServerTerminus", :shared => true do
     # the 'before' block in the including context.
     before do
         Puppet::Util::Cacher.invalidate
+        FileTest.stubs(:exists?).returns true
         FileTest.stubs(:exists?).with(Puppet[:fileserverconfig]).returns(true)
-        FileTest.stubs(:exists?).with("/my/mount/path").returns(true)
-        FileTest.stubs(:directory?).with("/my/mount/path").returns(true)
-        FileTest.stubs(:readable?).with("/my/mount/path").returns(true)
+
+        @path = Tempfile.new("file_server_testing")
+        @path.close!
+        @path = @path.path
+
+        Dir.mkdir(@path)
+        File.open(File.join(@path, "myfile"), "w") { |f| f.print "my content" }
 
         # Use a real mount, so the integration is a bit deeper.
         @mount1 = Puppet::FileServing::Configuration::Mount.new("one")
-        @mount1.path = "/my/mount/path"
+        @mount1.path = @path
 
         @parser = stub 'parser', :changed? => false
         @parser.stubs(:parse).returns("one" => @mount1)
@@ -25,17 +30,14 @@ describe "Puppet::Indirector::FileServerTerminus", :shared => true do
         # Stub out the modules terminus
         @modules = mock 'modules terminus'
 
-        @request = Puppet::Indirector::Request.new(:indirection, :method, "puppet://myhost/one/my/file")
+        @request = Puppet::Indirector::Request.new(:indirection, :method, "puppet://myhost/one/myfile")
     end
 
     it "should use the file server configuration to find files" do
         @modules.stubs(:find).returns(nil)
         @terminus.indirection.stubs(:terminus).with(:modules).returns(@modules)
 
-        path = "/my/mount/path/my/file"
-        FileTest.stubs(:exists?).with(path).returns(true)
-        FileTest.stubs(:exists?).with("/my/mount/path").returns(true)
-        @mount1.expects(:file).with("my/file", :node => nil).returns(path)
+        path = File.join(@path, "myfile")
 
         @terminus.find(@request).should be_instance_of(@test_class)
     end
diff --git a/spec/unit/file_serving/content.rb b/spec/unit/file_serving/content.rb
index e471c3b29..eacaff89f 100755
--- a/spec/unit/file_serving/content.rb
+++ b/spec/unit/file_serving/content.rb
@@ -25,7 +25,7 @@ describe Puppet::FileServing::Content do
         Puppet::FileServing::Content.new("/path").should respond_to(:collect)
     end
 
-    it "should retrieve and store its contents when its attributes are collected" do
+    it "should retrieve and store its contents when its attributes are collected if the file is a normal file" do
         content = Puppet::FileServing::Content.new("/path")
 
         result = "foo"
@@ -36,6 +36,17 @@ describe Puppet::FileServing::Content do
         content.instance_variable_get("@content").should_not be_nil
     end
 
+    it "should not attempt to retrieve its contents if the file is a directory" do
+        content = Puppet::FileServing::Content.new("/path")
+
+        result = "foo"
+        File.stubs(:lstat).returns(stub("stat", :ftype => "directory"))
+        File.expects(:read).with("/path").never
+        content.collect
+
+        content.instance_variable_get("@content").should be_nil
+    end
+
     it "should have a method for setting its content" do
         content = Puppet::FileServing::Content.new("/path")
         content.should respond_to(:content=)
diff --git a/spec/unit/file_serving/metadata.rb b/spec/unit/file_serving/metadata.rb
index f0e15873f..768a7c56d 100755
--- a/spec/unit/file_serving/metadata.rb
+++ b/spec/unit/file_serving/metadata.rb
@@ -16,6 +16,10 @@ describe Puppet::FileServing::Metadata do
     it "should should include the IndirectionHooks module in its indirection" do
         Puppet::FileServing::Metadata.indirection.metaclass.included_modules.should include(Puppet::FileServing::IndirectionHooks)
     end
+
+    it "should have a method that triggers attribute collection" do
+        Puppet::FileServing::Metadata.new("/foo/bar").should respond_to(:collect)
+    end
 end
 
 describe Puppet::FileServing::Metadata, " when finding the file to use for setting attributes" do
@@ -30,22 +34,22 @@ describe Puppet::FileServing::Metadata, " when finding the file to use for setti
     it "should accept a base path path to which the file should be relative" do
         File.expects(:lstat).with(@path).returns @stat
         File.expects(:readlink).with(@path).returns "/what/ever"
-        @metadata.collect_attributes
+        @metadata.collect
     end
 
     it "should use the set base path if one is not provided" do
         File.expects(:lstat).with(@path).returns @stat
         File.expects(:readlink).with(@path).returns "/what/ever"
-        @metadata.collect_attributes()
+        @metadata.collect()
     end
 
     it "should fail if a base path is neither set nor provided" do
-        proc { @metadata.collect_attributes() }.should raise_error(Errno::ENOENT)
+        proc { @metadata.collect() }.should raise_error(Errno::ENOENT)
     end
 
     it "should raise an exception if the file does not exist" do
         File.expects(:lstat).with(@path).raises(Errno::ENOENT)
-        proc { @metadata.collect_attributes()}.should raise_error(Errno::ENOENT)
+        proc { @metadata.collect()}.should raise_error(Errno::ENOENT)
     end
 end
 
@@ -58,7 +62,7 @@ describe Puppet::FileServing::Metadata, " when collecting attributes" do
         @checksum = Digest::MD5.hexdigest("some content\n")
         @metadata = Puppet::FileServing::Metadata.new("/my/file")
         @metadata.stubs(:md5_file).returns(@checksum)
-        @metadata.collect_attributes
+        @metadata.collect
     end
 
     it "should be able to produce xmlrpc-style attribute information" do
@@ -106,7 +110,7 @@ describe Puppet::FileServing::Metadata, " when collecting attributes" do
             @stat.stubs(:ftype).returns("directory")
             @time = Time.now
             @metadata.expects(:ctime_file).returns(@time)
-            @metadata.collect_attributes
+            @metadata.collect
         end
 
         it "should only use checksums of type 'ctime' for directories" do
@@ -122,7 +126,7 @@ describe Puppet::FileServing::Metadata, " when collecting attributes" do
         before do
             @stat.stubs(:ftype).returns("link")
             File.expects(:readlink).with("/my/file").returns("/path/to/link")
-            @metadata.collect_attributes
+            @metadata.collect
         end
 
         it "should read links instead of returning their checksums" do
@@ -142,7 +146,7 @@ describe Puppet::FileServing::Metadata, " when pointing to a link" do
         File.expects(:lstat).with("/base/path/my/file").returns stub("stat", :uid => 1, :gid => 2, :ftype => "link", :mode => 0755)
         File.expects(:readlink).with("/base/path/my/file").returns "/some/other/path"
 
-        file.collect_attributes
+        file.collect
         file.destination.should == "/some/other/path"
     end
 
@@ -152,7 +156,7 @@ describe Puppet::FileServing::Metadata, " when pointing to a link" do
         File.expects(:lstat).with("/base/path/my/file").returns stub("stat", :uid => 1, :gid => 2, :ftype => "link", :mode => 0755)
         File.expects(:readlink).with("/base/path/my/file").returns "/some/other/path"
 
-        file.collect_attributes
+        file.collect
         file.checksum.should be_nil
     end
 end
diff --git a/spec/unit/file_serving/terminus_helper.rb b/spec/unit/file_serving/terminus_helper.rb
index aa9dca961..e504a51be 100755
--- a/spec/unit/file_serving/terminus_helper.rb
+++ b/spec/unit/file_serving/terminus_helper.rb
@@ -35,36 +35,48 @@ describe Puppet::FileServing::TerminusHelper do
         before do
             @request.stubs(:key).returns "puppet://host/mount/dir"
 
+            @one = stub 'one', :links= => nil, :collect => nil
+            @two = stub 'two', :links= => nil, :collect => nil
+
             @fileset = mock 'fileset', :files => %w{one two}
             Puppet::FileServing::Fileset.expects(:new).returns(@fileset)
         end
 
         it "should create an instance of the model for each path returned by the fileset" do
-            @model.expects(:new).returns(:one)
-            @model.expects(:new).returns(:two)
+            @model.expects(:new).returns(@one)
+            @model.expects(:new).returns(@two)
             @helper.path2instances(@request, "/my/file").length.should == 2
         end
 
         it "should set each returned instance's path to the original path" do
-            @model.expects(:new).with { |key, options| key == "/my/file" }.returns(:one)
-            @model.expects(:new).with { |key, options| key == "/my/file" }.returns(:two)
+            @model.expects(:new).with { |key, options| key == "/my/file" }.returns(@one)
+            @model.expects(:new).with { |key, options| key == "/my/file" }.returns(@two)
             @helper.path2instances(@request, "/my/file")
         end
 
         it "should set each returned instance's relative path to the file-specific path" do
-            @model.expects(:new).with { |key, options| options[:relative_path] == "one" }.returns(:one)
-            @model.expects(:new).with { |key, options| options[:relative_path] == "two" }.returns(:two)
+            @model.expects(:new).with { |key, options| options[:relative_path] == "one" }.returns(@one)
+            @model.expects(:new).with { |key, options| options[:relative_path] == "two" }.returns(@two)
             @helper.path2instances(@request, "/my/file")
         end
 
         it "should set the links value on each instance if one is provided" do
-            one = mock 'one', :links= => :manage
-            two = mock 'two', :links= => :manage
-            @model.expects(:new).returns(one)
-            @model.expects(:new).returns(two)
+            @one.expects(:links=).with :manage
+            @two.expects(:links=).with :manage
+            @model.expects(:new).returns(@one)
+            @model.expects(:new).returns(@two)
 
             @request.options[:links] = :manage
             @helper.path2instances(@request, "/my/file")
         end
+
+        it "should collect the instance's attributes" do
+            @one.expects(:collect)
+            @two.expects(:collect)
+            @model.expects(:new).returns(@one)
+            @model.expects(:new).returns(@two)
+
+            @helper.path2instances(@request, "/my/file")
+        end
     end
 end
diff --git a/spec/unit/indirector/direct_file_server.rb b/spec/unit/indirector/direct_file_server.rb
index b80519bbe..e32fe6678 100755
--- a/spec/unit/indirector/direct_file_server.rb
+++ b/spec/unit/indirector/direct_file_server.rb
@@ -45,7 +45,7 @@ describe Puppet::Indirector::DirectFileServer do
 
         before do
             @data = mock 'content'
-            @data.stubs(:collect_attributes)
+            @data.stubs(:collect)
             FileTest.expects(:exists?).with("/my/local").returns true
         end
 
diff --git a/spec/unit/indirector/file_metadata/file.rb b/spec/unit/indirector/file_metadata/file.rb
index b37c42c72..a096d469d 100755
--- a/spec/unit/indirector/file_metadata/file.rb
+++ b/spec/unit/indirector/file_metadata/file.rb
@@ -21,14 +21,14 @@ describe Puppet::Indirector::FileMetadata::File do
             @metadata = Puppet::Indirector::FileMetadata::File.new
             @uri = "file:///my/local"
             @data = mock 'metadata'
-            @data.stubs(:collect_attributes)
+            @data.stubs(:collect)
             FileTest.expects(:exists?).with("/my/local").returns true
 
             @request = Puppet::Indirector::Request.new(:file_metadata, :find, @uri)
         end
 
         it "should collect its attributes when a file is found" do
-            @data.expects(:collect_attributes)
+            @data.expects(:collect)
 
             Puppet::FileServing::Metadata.expects(:new).returns(@data)
             @metadata.find(@request).should == @data
@@ -45,7 +45,7 @@ describe Puppet::Indirector::FileMetadata::File do
 
         it "should collect the attributes of the instances returned" do
             FileTest.expects(:exists?).with("/my/local").returns true
-            @metadata.expects(:path2instances).returns( [mock("one", :collect_attributes => nil), mock("two", :collect_attributes => nil)] )
+            @metadata.expects(:path2instances).returns( [mock("one", :collect => nil), mock("two", :collect => nil)] )
             @metadata.search(@request)
         end
     end
diff --git a/spec/unit/indirector/file_metadata/modules.rb b/spec/unit/indirector/file_metadata/modules.rb
index 838ac3b5f..7e5113df5 100755
--- a/spec/unit/indirector/file_metadata/modules.rb
+++ b/spec/unit/indirector/file_metadata/modules.rb
@@ -36,7 +36,7 @@ describe Puppet::Indirector::FileMetadata::Modules, " when finding metadata" do
         FileTest.expects(:exists?).with("/path/to/files/my/file").returns true
         instance = mock 'metadta'
         Puppet::FileServing::Metadata.expects(:new).returns instance
-        instance.expects :collect_attributes
+        instance.expects :collect
         @finder.find(@request)
     end
 end
diff --git a/spec/unit/indirector/file_server.rb b/spec/unit/indirector/file_server.rb
index ab8e32566..e17deecf9 100755
--- a/spec/unit/indirector/file_server.rb
+++ b/spec/unit/indirector/file_server.rb
@@ -56,19 +56,20 @@ describe Puppet::Indirector::FileServer do
 
         it "should return an instance of the model created with the full path if a file is found" do
             @configuration.expects(:file_path).with("my/local/file", :node => nil).returns("/some/file")
-            @model.expects(:new).returns(:myinstance)
-            @file_server.find(@request).should == :myinstance
+            instance = stub("instance", :collect => nil)
+            @model.expects(:new).returns instance
+            @file_server.find(@request).should equal(instance)
         end
     end
 
     describe Puppet::Indirector::FileServer, " when returning instances" do
         before :each do
             @configuration.expects(:file_path).with("my/local/file", :node => nil).returns("/some/file")
-            @instance = mock 'instance'
+            @instance = stub 'instance', :collect => nil
         end
 
         it "should create the instance with the path at which the instance was found" do
-            @model.expects(:new).with { |key, options| key == "/some/file" }
+            @model.expects(:new).with { |key, options| key == "/some/file" }.returns @instance
             @file_server.find(@request)
         end
 
@@ -83,6 +84,12 @@ describe Puppet::Indirector::FileServer do
             @model.expects(:new).returns(@instance)
             @file_server.find(@request)
         end
+
+        it "should collect each instance's attributes before returning" do
+            @instance.expects(:collect)
+            @model.expects(:new).returns @instance
+            @file_server.find(@request)
+        end
     end
 
     describe Puppet::Indirector::FileServer, " when checking authorization" do
@@ -171,8 +178,14 @@ describe Puppet::Indirector::FileServer do
 
         it "should pass the request on to :path2instances" do
             @configuration.expects(:file_path).with("my/local/file", :node => nil).returns("my/file")
-            @file_server.expects(:path2instances).with(@request, "my/file")
+            @file_server.expects(:path2instances).with(@request, "my/file").returns []
             @file_server.search(@request)
         end
+
+        it "should return the result of :path2instances" do
+            @configuration.expects(:file_path).with("my/local/file", :node => nil).returns("my/file")
+            @file_server.expects(:path2instances).with(@request, "my/file").returns :footest
+            @file_server.search(@request).should == :footest
+        end
     end
 end
-- 
cgit 


From 6e43c2d1179aed5ae92111afee1d3b868cf5f0a2 Mon Sep 17 00:00:00 2001
From: Luke Kanies <luke@madstop.com>
Date: Mon, 25 Aug 2008 21:31:37 -0700
Subject: Aliasing RSpec's :should method to :must.

This allows us to still use the method in the RAL,
where 'should' is already taken.

Signed-off-by: Luke Kanies <luke@madstop.com>
---
 spec/spec_helper.rb | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index 52aed5b62..41b5e7443 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -39,3 +39,9 @@ end
 # have to be correctly mocked.
 Puppet[:confdir] = "/dev/null"
 Puppet[:vardir] = "/dev/null"
+
+# We need this because the RAL uses 'should' as a method.  This
+# allows us the same behaviour but with a different method name.
+class Object
+    alias :must :should
+end
-- 
cgit 


From 98ac24a9e155b1d3c2358da3e94610071b0e3cfb Mon Sep 17 00:00:00 2001
From: Luke Kanies <luke@madstop.com>
Date: Mon, 25 Aug 2008 21:50:13 -0700
Subject: Adding a "source" attribute to fileserving instances.

This will be used to cache the source that was used
to retrieve the instance.

Signed-off-by: Luke Kanies <luke@madstop.com>
---
 lib/puppet/file_serving/base.rb | 4 ++++
 spec/unit/file_serving/base.rb  | 6 ++++++
 2 files changed, 10 insertions(+)

diff --git a/lib/puppet/file_serving/base.rb b/lib/puppet/file_serving/base.rb
index 94e337b99..9a50833de 100644
--- a/lib/puppet/file_serving/base.rb
+++ b/lib/puppet/file_serving/base.rb
@@ -7,6 +7,10 @@ require 'puppet/file_serving'
 # The base class for Content and Metadata; provides common
 # functionality like the behaviour around links.
 class Puppet::FileServing::Base
+    # This is for external consumers to store the source that was used
+    # to retrieve the metadata.
+    attr_accessor :source
+
     # Does our file exist?
     def exist?
         begin
diff --git a/spec/unit/file_serving/base.rb b/spec/unit/file_serving/base.rb
index 7eccb3a52..7cb95aa7a 100755
--- a/spec/unit/file_serving/base.rb
+++ b/spec/unit/file_serving/base.rb
@@ -17,6 +17,12 @@ describe Puppet::FileServing::Base do
         Puppet::FileServing::Base.new("/module/dir/file", :links => :manage).links.should == :manage
     end
 
+    it "should have a :source attribute" do
+        file = Puppet::FileServing::Base.new("/module/dir/file")
+        file.should respond_to(:source)
+        file.should respond_to(:source=)
+    end
+
     it "should consider :ignore links equivalent to :manage links" do
         Puppet::FileServing::Base.new("/module/dir/file", :links => :ignore).links.should == :manage
     end
-- 
cgit 


From 82714246b913087292f04190e03a885c99723f52 Mon Sep 17 00:00:00 2001
From: Luke Kanies <luke@madstop.com>
Date: Tue, 26 Aug 2008 22:06:02 -0700
Subject: One third done refactoring file[:source] -- retrieve() is done.

It now uses the FileServing::Metadata indirection and
is about 100x cleaner to boot.

Signed-off-by: Luke Kanies <luke@madstop.com>
---
 lib/puppet/type/file/source.rb | 174 +++++++++++++++-------------------------
 spec/unit/type/file/source.rb  | 176 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 238 insertions(+), 112 deletions(-)
 create mode 100755 spec/unit/type/file/source.rb

diff --git a/lib/puppet/type/file/source.rb b/lib/puppet/type/file/source.rb
index 2514d3d1e..e8db13cfa 100755
--- a/lib/puppet/type/file/source.rb
+++ b/lib/puppet/type/file/source.rb
@@ -1,3 +1,7 @@
+
+require 'puppet/file_serving/content'
+require 'puppet/file_serving/metadata'
+
 module Puppet
     # Copy files from a local or remote source.  This state *only* does any work
     # when the remote file is an actual file; in that case, this state copies
@@ -77,61 +81,48 @@ module Puppet
         end
         
         def change_to_s(currentvalue, newvalue)
-            # newvalue = "{md5}" + @stats[:checksum]
+            # newvalue = "{md5}" + @metadata.checksum
             if @resource.property(:ensure).retrieve == :absent
-                return "creating from source %s with contents %s" % [@source, @stats[:checksum]]
+                return "creating from source %s with contents %s" % [@source, @metadata.checksum]
             else
-                return "replacing from source %s with contents %s" % [@source, @stats[:checksum]]
+                return "replacing from source %s with contents %s" % [@source, @metadata.checksum]
             end
         end
         
         def checksum
-            if defined?(@stats)
-                @stats[:checksum]
+            if defined?(@metadata)
+                @metadata.checksum
             else
                 nil
             end
         end
 
-        # Ask the file server to describe our file.
-        def describe(source)
-            sourceobj, path = @resource.uri2obj(source)
-            server = sourceobj.server
+        # Copy the values from the source to the resource.  Yay.
+        def copy_source_values
+            devfail "Somehow got asked to copy source values without any metadata" unless metadata
 
-            begin
-                desc = server.describe(path, @resource[:links])
-            rescue Puppet::Network::XMLRPCClientError => detail
-                fail detail, "Could not describe %s: %s" % [path, detail]
+            # Take each of the stats and set them as states on the local file
+            # if a value has not already been provided.
+            [:owner, :mode, :group].each do |param|
+                @resource[param] ||= metadata.send(param)
             end
 
-            return nil if desc == ""
-
-            # Collect everything except the checksum
-            values = desc.split("\t")
-            other = values.pop
-            args = {}
-            pinparams.zip(values).each { |param, value|
-                if value =~ /^[0-9]+$/
-                    value = value.to_i
-                end
-                unless value.nil?
-                    args[param] = value
-                end
-            }
+            unless @resource.deleting?
+                @resource[:ensure] = metadata.ftype
+            end
 
-            # Now decide whether we're doing checksums or symlinks
-            if args[:type] == "link"
-                args[:target] = other
-            else
-                args[:checksum] = other
+            if metadata.ftype == "link"
+                @resource[:target] = metadata.destination
             end
+        end
 
-            # we can't manage ownership unless we're root, so don't even try
-            unless Puppet::Util::SUIDManager.uid == 0
-                args.delete(:owner)
+        # Ask the file server to describe our file.
+        def describe(source)
+            begin
+                Puppet::FileServing::Metadata.find source
+            rescue => detail
+                fail detail, "Could not retrieve file metadata for %s: %s" % [path, detail]
             end
-            
-            return args
         end
         
         # Use the info we get from describe() to check if we're in sync.
@@ -142,7 +133,7 @@ module Puppet
             
             # the only thing this actual state can do is copy files around.  Therefore,
             # only pay attention if the remote is a file.
-            unless @stats[:type] == "file" 
+            unless @metadata.ftype == "file" 
                 return true
             end
             
@@ -153,15 +144,15 @@ module Puppet
             end
             # Now, we just check to see if the checksums are the same
             parentchecksum = @resource.property(:checksum).retrieve
-            result = (!parentchecksum.nil? and (parentchecksum == @stats[:checksum]))
+            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]) and ! @stats[:_diffed]
-                @stats[:_remote_content] = get_remote_content
-                string_file_diff(@resource[:path], @stats[:_remote_content])
-                @stats[:_diffed] = true
+            if ! result and Puppet[:show_diff] and File.exists?(@resource[:path]) and ! @metadata._diffed
+                @metadata._remote_content = get_remote_content
+                string_file_diff(@resource[:path], @metadata._remote_content)
+                @metadata._diffed = true
             end
             return result
         end
@@ -171,55 +162,34 @@ module Puppet
         end
 
         def found?
-            ! (@stats.nil? or @stats[:type].nil?)
+            ! (@metadata.nil? or @metadata.ftype.nil?)
         end
 
-        # This basically calls describe() on our file, and then sets all
-        # of the local states appropriately.  If the remote file is a normal
-        # file then we set it to copy; if it's a directory, then we just mark
-        # that the local directory should be created.
-        def retrieve(remote = true)
-            sum = nil
-            @source = nil
-
-            # This is set to false by the File#retrieve function on the second
-            # retrieve, so that we do not do two describes.
-            if remote
-                # Find the first source that exists.  @shouldorig contains
-                # the sources as specified by the user.
-                @should.each { |source|
-                    if @stats = self.describe(source)
-                        @source = source
-                        break
+        # Provide, and retrieve if necessary, the metadata for this file.  Fail
+        # if we can't find data about this host, and fail if there are any
+        # problems in our query.
+        attr_writer :metadata
+        def metadata
+            unless defined?(@metadata) and @metadata
+                return @metadata = nil unless should
+                should.each do |source|
+                    begin
+                        if data = Puppet::FileServing::Metadata.find(source)
+                            @metadata = data
+                            @metadata.source = source
+                            break
+                        end
+                    rescue => detail
+                        fail detail, "Could not retrieve file metadata for %s: %s" % [source, detail]
                     end
-                }
-            end
-
-            if !found?
-                raise Puppet::Error, "No specified source was found from" + @should.inject("") { |s, source| s + " #{source},"}.gsub(/,$/,"")
-            end
-            
-            case @stats[:type]
-            when "directory", "file", "link":
-                @resource[:ensure] = @stats[:type] unless @resource.deleting?
-            else
-                self.info @stats.inspect
-                self.err "Cannot use files of type %s as sources" % @stats[:type]
-                return :nocopy
+                end
+                fail "Could not retrieve information from source(s) %s" % @should.join(", ") unless @metadata
             end
+            return @metadata
+        end
 
-            # Take each of the stats and set them as states on the local file
-            # if a value has not already been provided.
-            @stats.each { |stat, value|
-                next if stat == :checksum
-                next if stat == :type
-
-                # was the stat already specified, or should the value
-                # be inherited from the source?
-                @resource[stat] = value unless @resource.argument?(stat)
-            }
-
-            return @stats[:checksum]
+        def retrieve
+            copy_source_values
         end
         
         def should
@@ -238,11 +208,13 @@ module Puppet
         end
 
         def sync
-            contents = @stats[:_remote_content] || get_remote_content()
-
             exists = File.exists?(@resource[:path])
 
-            @resource.write(contents, :source, @stats[:checksum])
+            if content = Puppet::FileServing::Content.find(@metadata.source)
+                @resource.write(content.content, :source, @metadata.checksum)
+            else
+                raise "Could not retrieve content"
+            end
 
             if exists
                 return :file_changed
@@ -250,27 +222,5 @@ module Puppet
                 return :file_created
             end
         end
-
-        private
-
-        def get_remote_content
-            raise Puppet::DevError, "Got told to copy non-file %s" % @resource[:path] unless @stats[:type] == "file"
-
-            sourceobj, path = @resource.uri2obj(@source)
-
-            begin
-                contents = sourceobj.server.retrieve(path, @resource[:links])
-            rescue => detail
-                self.fail "Could not retrieve %s: %s" % [path, detail]
-            end
-
-            contents = CGI.unescape(contents) unless sourceobj.server.local
-
-            if contents == ""
-                self.notice "Could not retrieve contents for %s" % @source
-            end
-
-            return contents
-        end
     end
 end
diff --git a/spec/unit/type/file/source.rb b/spec/unit/type/file/source.rb
new file mode 100755
index 000000000..bb689ce76
--- /dev/null
+++ b/spec/unit/type/file/source.rb
@@ -0,0 +1,176 @@
+#!/usr/bin/env ruby
+
+Dir.chdir(File.dirname(__FILE__)) { (s = lambda { |f| File.exist?(f) ? require(f) : Dir.chdir("..") { s.call(f) } }).call("spec/spec_helper.rb") }
+
+source = Puppet::Type.type(:file).attrclass(:source)
+describe Puppet::Type.type(:file).attrclass(:source) do
+    before do
+        # Wow that's a messy interface to the resource.
+        @resource = stub 'resource', :uri2obj => true, :[]= => nil, :property => nil
+    end
+
+    it "should be a subclass of Property" do
+        source.superclass.must == Puppet::Property
+    end
+
+    describe "when initializing" do
+        it "should fail if the 'should' values are not URLs" do
+            @resource.expects(:uri2obj).with("foo").returns false
+
+            lambda { source.new(:resource => @resource, :should => %w{foo}) }.must raise_error(Puppet::Error)
+        end
+    end
+
+    it "should have a method for retrieving its metadata" do
+        source.new(:resource => @resource).must respond_to(:metadata)
+    end
+
+    it "should have a method for setting its metadata" do
+        source.new(:resource => @resource).must respond_to(:metadata=)
+    end
+
+    describe "when returning the metadata" do
+        before do
+            @metadata = stub 'metadata', :source= => nil
+        end
+
+        it "should return already-available metadata" do
+            @source = source.new(:resource => @resource)
+            @source.metadata = "foo"
+            @source.metadata.should == "foo"
+        end
+
+        it "should return nil if no @should value is set and no metadata is available" do
+            @source = source.new(:resource => @resource)
+            @source.metadata.should be_nil
+        end
+
+        it "should collect its metadata using the Metadata class if it is not already set" do
+            @source = source.new(:resource => @resource, :should => "/foo/bar")
+            Puppet::FileServing::Metadata.expects(:find).with("/foo/bar").returns @metadata
+            @source.metadata
+        end
+
+        it "should use the metadata from the first found source" do
+            metadata = stub 'metadata', :source= => nil
+            @source = source.new(:resource => @resource, :should => ["/foo/bar", "/fee/booz"])
+            Puppet::FileServing::Metadata.expects(:find).with("/foo/bar").returns nil
+            Puppet::FileServing::Metadata.expects(:find).with("/fee/booz").returns metadata
+            @source.metadata.should equal(metadata)
+        end
+
+        it "should store the found source as the metadata's source" do
+            metadata = mock 'metadata'
+            @source = source.new(:resource => @resource, :should => "/foo/bar")
+            Puppet::FileServing::Metadata.expects(:find).with("/foo/bar").returns metadata
+
+            metadata.expects(:source=).with("/foo/bar")
+            @source.metadata
+        end
+
+        it "should fail intelligently if an exception is encountered while querying for metadata" do
+            @source = source.new(:resource => @resource, :should => "/foo/bar")
+            Puppet::FileServing::Metadata.expects(:find).with("/foo/bar").raises RuntimeError
+
+            @source.expects(:fail).raises ArgumentError
+            lambda { @source.metadata }.should raise_error(ArgumentError)
+        end
+
+        it "should fail if no specified sources can be found" do
+            @source = source.new(:resource => @resource, :should => "/foo/bar")
+            Puppet::FileServing::Metadata.expects(:find).with("/foo/bar").returns nil
+
+            @source.expects(:fail).raises RuntimeError
+
+            lambda { @source.metadata }.should raise_error(RuntimeError)
+        end
+    end
+
+    it "should have a method for setting the desired values on the resource" do
+        source.new(:resource => @resource).must respond_to(:copy_source_values)
+    end
+
+    describe "when copying the source values" do
+        before do
+            @metadata = stub 'metadata', :owner => 100, :group => 200, :mode => 123
+
+            @source = source.new(:resource => @resource)
+            @source.metadata = @metadata
+
+            @resource.stubs(:deleting?).returns false
+        end
+
+        it "should fail if there is no metadata" do
+            @source.metadata = nil
+            @source.expects(:devfail).raises ArgumentError
+            lambda { @source.copy_source_values }.should raise_error(ArgumentError)
+        end
+
+        it "should set :ensure to the file type if the resource is not being deleted" do
+            @resource.expects(:deleting?).returns false
+            @resource.stubs(:[])
+            @resource.stubs(:[]=)
+            @metadata.stubs(:ftype).returns "foobar"
+
+            @resource.expects(:[]=).with(:ensure, "foobar")
+            @source.copy_source_values
+        end
+
+        it "should not set :ensure to the file type if the resource is being deleted" do
+            @resource.expects(:deleting?).returns true
+            @resource.stubs(:[])
+            @resource.stubs(:[]).returns "foo"
+            @metadata.expects(:ftype).returns "foobar"
+
+            @resource.expects(:[]=).with(:ensure, "foobar").never
+            @source.copy_source_values
+        end
+
+        describe "and the source is a file" do
+            before do
+                @metadata.stubs(:ftype).returns "file"
+            end
+
+            it "should copy the metadata's owner, group, and mode to the resource if they are not set on the resource" do
+                @resource.stubs(:[]).returns nil
+
+                @resource.expects(:[]=).with(:owner, 100)
+                @resource.expects(:[]=).with(:group, 200)
+                @resource.expects(:[]=).with(:mode, 123)
+
+                @source.copy_source_values
+            end
+
+            it "should not copy the metadata's owner to the resource if it is already set" do
+                @resource.stubs(:[]).returns "value"
+                @resource.expects(:[]).returns "value"
+
+                @resource.expects(:[]=).never
+
+                @source.copy_source_values
+            end
+        end
+
+        describe "and the source is a link" do
+            it "should set the target to the link destination" do
+                @metadata.stubs(:ftype).returns "link"
+                @resource.stubs(:[])
+                @resource.stubs(:[]=)
+
+                @metadata.expects(:destination).returns "/path/to/symlink"
+
+                @resource.expects(:[]=).with(:target, "/path/to/symlink")
+                @source.copy_source_values
+            end
+        end
+    end
+
+    describe "when retrieving the property state" do
+        it "should copy all metadata to the resource" do
+            @source = source.new(:resource => @resource)
+            @source.expects(:copy_source_values)
+
+            @source.retrieve
+        end
+    end
+end
-- 
cgit 


From 44c6a529d0a84d844b21f96d64de674487238f53 Mon Sep 17 00:00:00 2001
From: Luke Kanies <luke@madstop.com>
Date: Tue, 26 Aug 2008 23:11:39 -0700
Subject: Removing mention of an obselete class.

Signed-off-by: Luke Kanies <luke@madstop.com>
---
 bin/puppetd | 1 -
 1 file changed, 1 deletion(-)

diff --git a/bin/puppetd b/bin/puppetd
index c17654892..758494c4f 100755
--- a/bin/puppetd
+++ b/bin/puppetd
@@ -162,7 +162,6 @@ trap(:INT) do
 end
 
 require 'puppet'
-require 'puppet/executables/client/certhandler'
 require 'puppet/network/client'
 require 'getoptlong'
 
-- 
cgit 


From be4c0e7fbe5e652ec1d49eab2fdc7a5fbbc486f3 Mon Sep 17 00:00:00 2001
From: Luke Kanies <luke@madstop.com>
Date: Tue, 26 Aug 2008 23:12:03 -0700
Subject: The file source is now refactored and uses REST.

Signed-off-by: Luke Kanies <luke@madstop.com>
---
 lib/puppet/type/file.rb        |  7 ++++
 lib/puppet/type/file/source.rb | 54 +++++++++++++++-------------
 spec/unit/type/file.rb         | 48 +++++--------------------
 spec/unit/type/file/source.rb  | 81 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 125 insertions(+), 65 deletions(-)

diff --git a/lib/puppet/type/file.rb b/lib/puppet/type/file.rb
index ef010efda..bc2e53c9f 100644
--- a/lib/puppet/type/file.rb
+++ b/lib/puppet/type/file.rb
@@ -332,6 +332,13 @@ module Puppet
             recurse()
         end
 
+        def flush
+            # We want to make sure we retrieve metadata anew on each transaction.
+            @parameters.each do |name, param|
+                param.flush if param.respond_to?(:flush)
+            end
+        end
+
         # Deal with backups.
         def handlebackup(file = nil)
             # let the path be specified
diff --git a/lib/puppet/type/file/source.rb b/lib/puppet/type/file/source.rb
index e8db13cfa..5eefb1dcb 100755
--- a/lib/puppet/type/file/source.rb
+++ b/lib/puppet/type/file/source.rb
@@ -83,9 +83,9 @@ module Puppet
         def change_to_s(currentvalue, newvalue)
             # newvalue = "{md5}" + @metadata.checksum
             if @resource.property(:ensure).retrieve == :absent
-                return "creating from source %s with contents %s" % [@source, @metadata.checksum]
+                return "creating from source %s with contents %s" % [metadata.source, @metadata.checksum]
             else
-                return "replacing from source %s with contents %s" % [@source, @metadata.checksum]
+                return "replacing from source %s with contents %s" % [metadata.source, @metadata.checksum]
             end
         end
         
@@ -97,6 +97,19 @@ module Puppet
             end
         end
 
+        # Look up (if necessary) and return remote content.
+        def content
+            raise Puppet::DevError, "No source for content was stored with the metadata" unless metadata.source
+
+            unless defined?(@content) and @content
+                unless tmp = Puppet::FileServing::Content.find(@metadata.source)
+                    fail "Could not find any content at %s" % @metadata.source
+                end
+                @content = tmp.content
+            end
+            @content
+        end
+
         # Copy the values from the source to the resource.  Yay.
         def copy_source_values
             devfail "Somehow got asked to copy source values without any metadata" unless metadata
@@ -116,21 +129,15 @@ module Puppet
             end
         end
 
-        # Ask the file server to describe our file.
-        def describe(source)
-            begin
-                Puppet::FileServing::Metadata.find source
-            rescue => detail
-                fail detail, "Could not retrieve file metadata for %s: %s" % [path, detail]
-            end
+        # Remove any temporary attributes we manage.
+        def flush
+            @metadata = nil
+            @content = nil
         end
-        
-        # Use the info we get from describe() to check if we're in sync.
+
+        # Use the remote metadata to see if we're in sync.
+        # LAK:NOTE This method should still get refactored.
         def insync?(currentvalue)
-            if currentvalue == :nocopy
-                return true
-            end
-            
             # the only thing this actual state can do is copy files around.  Therefore,
             # only pay attention if the remote is a file.
             unless @metadata.ftype == "file" 
@@ -149,10 +156,8 @@ module Puppet
             # 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]) and ! @metadata._diffed
-                @metadata._remote_content = get_remote_content
-                string_file_diff(@resource[:path], @metadata._remote_content)
-                @metadata._diffed = true
+            if ! result and Puppet[:show_diff] and File.exists?(@resource[:path])
+                string_file_diff(@resource[:path], content)
             end
             return result
         end
@@ -188,10 +193,13 @@ module Puppet
             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
@@ -208,13 +216,9 @@ module Puppet
         end
 
         def sync
-            exists = File.exists?(@resource[:path])
+            exists = FileTest.exist?(@resource[:path])
 
-            if content = Puppet::FileServing::Content.find(@metadata.source)
-                @resource.write(content.content, :source, @metadata.checksum)
-            else
-                raise "Could not retrieve content"
-            end
+            @resource.write(content, :source, @metadata.checksum)
 
             if exists
                 return :file_changed
diff --git a/spec/unit/type/file.rb b/spec/unit/type/file.rb
index 7f9688f0b..8b7bedee6 100755
--- a/spec/unit/type/file.rb
+++ b/spec/unit/type/file.rb
@@ -31,46 +31,6 @@ describe Puppet::Type.type(:file) do
         end
     end
 
-    describe "when specifying a source" do
-        before do
-            @file[:source] = "/bar"
-        end
-
-        it "should raise if source doesn't exist" do
-            @file.property(:source).expects(:found?).returns(false)
-            lambda { @file.retrieve }.should raise_error(Puppet::Error)
-        end
-
-    end
-
-    describe "when retrieving remote files" do
-        before do
-            @filesource = Puppet::Type::File::FileSource.new
-            @filesource.server = mock 'fileserver'
-
-            @file.stubs(:uri2obj).returns(@filesource)
-
-            @file[:source] = "puppet:///test"
-        end
-
-        it "should fail without writing if it cannot retrieve remote contents" do
-            # create the file, because we only get the problem when it starts
-            # out absent.
-            File.open(@file[:path], "w") { |f| f.puts "a" }
-            @file.expects(:write).never
-
-            @filesource.server.stubs(:describe).returns("493\tfile\t100\t0\t{md5}3f5fef3bddbc4398c46a7bd7ba7b3af7")
-            @filesource.server.stubs(:retrieve).raises(RuntimeError)
-            @file.property(:source).retrieve
-            lambda { @file.property(:source).sync }.should raise_error(Puppet::Error)
-        end
-
-        it "should fail if it cannot describe remote contents" do
-            @filesource.server.stubs(:describe).raises(Puppet::Network::XMLRPCClientError.new("Testing"))
-            lambda { @file.retrieve }.should raise_error(Puppet::Error)
-        end
-    end
-
     describe "when managing links" do
         require 'puppettest/support/assertions'
         include PuppetTest
@@ -110,4 +70,12 @@ describe Puppet::Type.type(:file) do
             ("%o" % (File.stat(@file).mode & 007777)).should == "%o" % 0755
         end
     end
+
+    describe "when flushing" do
+        it "should flush all properties that respond to :flush" do
+            @resource = Puppet.type(:file).create(:path => "/foo/bar", :source => "/bar/foo")
+            @resource.property(:source).expects(:flush)
+            @resource.flush
+        end
+    end
 end
diff --git a/spec/unit/type/file/source.rb b/spec/unit/type/file/source.rb
index bb689ce76..d6aa25fe7 100755
--- a/spec/unit/type/file/source.rb
+++ b/spec/unit/type/file/source.rb
@@ -173,4 +173,85 @@ describe Puppet::Type.type(:file).attrclass(:source) do
             @source.retrieve
         end
     end
+
+    describe "when flushing" do
+        it "should set its metadata to nil" do
+            @source = source.new(:resource => @resource)
+            @source.metadata = "foo"
+            @source.flush
+            @source.instance_variable_get("@metadata").should be_nil
+        end
+
+        it "should reset its content" do
+            @source = source.new(:resource => @resource)
+            @source.instance_variable_set("@content", "foo")
+            @source.flush
+            @source.instance_variable_get("@content").should be_nil
+        end
+    end
+
+    it "should have a method for returning the content" do
+        source.new(:resource => @resource).must respond_to(:content)
+    end
+
+    describe "when looking up the content" do
+        before do
+            @source = source.new(:resource => @resource)
+            @metadata = stub 'metadata', :source => "/my/source"
+            @source.metadata = @metadata
+
+            @content = stub 'content', :content => "foobar"
+        end
+
+        it "should fail if the metadata does not have a source set" do
+            @metadata.stubs(:source).returns nil
+            lambda { @source.content }.should raise_error(Puppet::DevError)
+        end
+
+        it "should look the content up from the Content class using the metadata source if no content is set" do
+            Puppet::FileServing::Content.expects(:find).with("/my/source").returns @content
+            @source.content.should == "foobar"
+        end
+
+        it "should return previously found content" do
+            Puppet::FileServing::Content.expects(:find).with("/my/source").returns @content
+            @source.content.should == "foobar"
+            @source.content.should == "foobar"
+        end
+
+        it "should fail if no content can be retrieved" do
+            Puppet::FileServing::Content.expects(:find).with("/my/source").returns nil
+            @source.expects(:fail).raises RuntimeError
+            lambda { @source.content }.should raise_error(RuntimeError)
+        end
+    end
+
+    describe "when changing the content" do
+        before do
+            @source = source.new(:resource => @resource)
+            @source.stubs(:content).returns "foobar"
+
+            @metadata = stub 'metadata', :checksum => 123
+            @source.metadata = @metadata
+            @resource.stubs(:[]).with(:path).returns "/boo"
+        end
+
+        it "should use the file's :write method to write the content" do
+            @resource.expects(:write).with("foobar", :source, 123)
+
+            @source.sync
+        end
+
+        it "should return :file_changed if the file already existed" do
+            @resource.stubs(:write)
+            FileTest.expects(:exist?).with("/boo").returns true
+            @source.sync.should == :file_changed
+        end
+
+        it "should return :file_created if the file already existed" do
+            @resource.stubs(:write)
+            FileTest.expects(:exist?).with("/boo").returns false
+            @source.sync.should == :file_created
+        end
+    end
 end
-- 
cgit 


From ac419872e273dc31635f042bb1a23c7785dc227a Mon Sep 17 00:00:00 2001
From: Luke Kanies <luke@madstop.com>
Date: Tue, 26 Aug 2008 23:29:04 -0700
Subject: Fixing the terminus helper so it correctly catches options passed
 from clients via REST.

Signed-off-by: Luke Kanies <luke@madstop.com>
---
 lib/puppet/file_serving/terminus_helper.rb | 11 ++++++++++-
 spec/unit/file_serving/terminus_helper.rb  | 21 +++++++++++++++++++++
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/lib/puppet/file_serving/terminus_helper.rb b/lib/puppet/file_serving/terminus_helper.rb
index 598a5007a..b51e27297 100644
--- a/lib/puppet/file_serving/terminus_helper.rb
+++ b/lib/puppet/file_serving/terminus_helper.rb
@@ -9,7 +9,16 @@ require 'puppet/file_serving/fileset'
 module Puppet::FileServing::TerminusHelper
     # Create model instances for all files in a fileset.
     def path2instances(request, path)
-        args = [:links, :ignore, :recurse].inject({}) { |hash, param| hash[param] = request.options[param] if request.options[param]; hash }
+        args = [:links, :ignore, :recurse].inject({}) do |hash, param|
+            if request.options.include?(param) # use 'include?' so the values can be false
+                hash[param] = request.options[param]
+            elsif request.options.include?(param.to_s)
+                hash[param] = request.options[param.to_s]
+            end
+            hash[param] = true if hash[param] == "true"
+            hash[param] = false if hash[param] == "false"
+            hash
+        end
         Puppet::FileServing::Fileset.new(path, args).files.collect do |file|
             inst = model.new(path, :relative_path => file)
             inst.links = request.options[:links] if request.options[:links]
diff --git a/spec/unit/file_serving/terminus_helper.rb b/spec/unit/file_serving/terminus_helper.rb
index e504a51be..7f08aeb7e 100755
--- a/spec/unit/file_serving/terminus_helper.rb
+++ b/spec/unit/file_serving/terminus_helper.rb
@@ -31,6 +31,27 @@ describe Puppet::FileServing::TerminusHelper do
         @helper.path2instances(@request, "/my/file")
     end
 
+    it "should pass :recurse, :ignore, and :links settings on to the fileset if present with the keys stored as strings" do
+        fileset = mock 'fileset', :files => []
+        Puppet::FileServing::Fileset.expects(:new).with("/my/file", :links => :a, :ignore => :b, :recurse => :c).returns(fileset)
+        @request.stubs(:options).returns("links" => :a, "ignore" => :b, "recurse" => :c)
+        @helper.path2instances(@request, "/my/file")
+    end
+
+    it "should convert the string 'true' to the boolean true when setting options" do
+        fileset = mock 'fileset', :files => []
+        Puppet::FileServing::Fileset.expects(:new).with("/my/file", :recurse => true).returns(fileset)
+        @request.stubs(:options).returns(:recurse => "true")
+        @helper.path2instances(@request, "/my/file")
+    end
+
+    it "should convert the string 'false' to the boolean false when setting options" do
+        fileset = mock 'fileset', :files => []
+        Puppet::FileServing::Fileset.expects(:new).with("/my/file", :recurse => false).returns(fileset)
+        @request.stubs(:options).returns(:recurse => "false")
+        @helper.path2instances(@request, "/my/file")
+    end
+
     describe "when creating instances" do
         before do
             @request.stubs(:key).returns "puppet://host/mount/dir"
-- 
cgit 


From 7c68fdb46802dbd3a57f5f7be3333ed6feacad45 Mon Sep 17 00:00:00 2001
From: Luke Kanies <luke@madstop.com>
Date: Wed, 27 Aug 2008 21:53:00 -0700
Subject: Fixing FileServing::Base so that it can recurse on a single file.

It was throwing exceptions if you tried to use it on a file
instead of a directory.

Signed-off-by: Luke Kanies <luke@madstop.com>
---
 lib/puppet/file_serving/base.rb                  |  2 +-
 spec/integration/file_serving/fileset.rb         | 14 ++++++++++++++
 spec/integration/file_serving/terminus_helper.rb | 22 ++++++++++++++++++++++
 spec/unit/file_serving/base.rb                   |  5 +++++
 4 files changed, 42 insertions(+), 1 deletion(-)
 create mode 100755 spec/integration/file_serving/fileset.rb
 create mode 100755 spec/integration/file_serving/terminus_helper.rb

diff --git a/lib/puppet/file_serving/base.rb b/lib/puppet/file_serving/base.rb
index 9a50833de..c59a54786 100644
--- a/lib/puppet/file_serving/base.rb
+++ b/lib/puppet/file_serving/base.rb
@@ -23,7 +23,7 @@ class Puppet::FileServing::Base
 
     # Return the full path to our file.  Fails if there's no path set.
     def full_path
-        if relative_path.nil? or relative_path == ""
+        if relative_path.nil? or relative_path == "" or relative_path == "."
             path
         else
             File.join(path, relative_path)
diff --git a/spec/integration/file_serving/fileset.rb b/spec/integration/file_serving/fileset.rb
new file mode 100755
index 000000000..80bf0f376
--- /dev/null
+++ b/spec/integration/file_serving/fileset.rb
@@ -0,0 +1,14 @@
+#!/usr/bin/env ruby
+
+require File.dirname(__FILE__) + '/../../spec_helper'
+
+require 'puppet/file_serving/fileset'
+
+describe Puppet::FileServing::Fileset do
+    it "should be able to recurse on a single file" do
+        @path = Tempfile.new("fileset_integration")
+
+        fileset = Puppet::FileServing::Fileset.new(@path.path)
+        lambda { fileset.files }.should_not raise_error
+    end
+end
diff --git a/spec/integration/file_serving/terminus_helper.rb b/spec/integration/file_serving/terminus_helper.rb
new file mode 100755
index 000000000..7d2587af1
--- /dev/null
+++ b/spec/integration/file_serving/terminus_helper.rb
@@ -0,0 +1,22 @@
+#!/usr/bin/env ruby
+
+require File.dirname(__FILE__) + '/../../spec_helper'
+
+require 'puppet/file_serving/terminus_helper'
+
+class TerminusHelperIntegrationTester
+    include Puppet::FileServing::TerminusHelper
+    def model
+        Puppet::FileServing::Metadata
+    end
+end
+
+describe Puppet::FileServing::TerminusHelper do
+    it "should be able to recurse on a single file" do
+        @path = Tempfile.new("fileset_integration")
+        request = Puppet::Indirector::Request.new(:metadata, :find, @path.path, :recurse => true)
+
+        tester = TerminusHelperIntegrationTester.new
+        lambda { tester.path2instances(request, @path.path) }.should_not raise_error
+    end
+end
diff --git a/spec/unit/file_serving/base.rb b/spec/unit/file_serving/base.rb
index 7cb95aa7a..6a76d81e9 100755
--- a/spec/unit/file_serving/base.rb
+++ b/spec/unit/file_serving/base.rb
@@ -81,6 +81,11 @@ describe Puppet::FileServing::Base do
             @file.full_path.should == "/this/file"
         end
 
+        it "should return the path if the relative_path is set to '.'" do
+            @file.relative_path = "."
+            @file.full_path.should == "/this/file"
+        end
+
         it "should return the path joined with the relative path if there is a relative path and it is not set to '/' or ''" do
             @file.relative_path = "not/qualified"
             @file.full_path.should == "/this/file/not/qualified"
-- 
cgit 


From ee1a85d61bb6ee9b31ae4881adc0c136a99e42ed Mon Sep 17 00:00:00 2001
From: Luke Kanies <luke@madstop.com>
Date: Wed, 27 Aug 2008 23:27:22 -0700
Subject: Mostly finishing refactoring file recursion to use REST.

We have the majority of the work done (and it's a *lot* less
code).  We just have six more tests we need to implement the code
for.

Signed-off-by: Luke Kanies <luke@madstop.com>
---
 lib/puppet/type/file.rb        |  57 ++++++++----
 lib/puppet/type/file/source.rb |  10 ++-
 spec/unit/type/file.rb         | 191 +++++++++++++++++++++++++++++++++++++++++
 spec/unit/type/file/source.rb  |  11 ++-
 4 files changed, 250 insertions(+), 19 deletions(-)

diff --git a/lib/puppet/type/file.rb b/lib/puppet/type/file.rb
index bc2e53c9f..2e3a585c1 100644
--- a/lib/puppet/type/file.rb
+++ b/lib/puppet/type/file.rb
@@ -329,7 +329,7 @@ module Puppet
         
         # Create any children via recursion or whatever.
         def eval_generate
-            recurse()
+            recurse() if self.recurse?
         end
 
         def flush
@@ -679,27 +679,54 @@ module Puppet
             @parameters.include?(:purge) and (self[:purge] == :true or self[:purge] == "true")
         end
 
+        # Recurse the target of the link.
+        def recurse_link
+            perform_recursion(self[:target])
+        end
+
+        # Recurse the file itself, returning a Metadata instance for every found file.
+        def recurse_local
+            perform_recursion(self[:path])
+        end
+
+        # Recurse against our remote file.
+        def recurse_remote
+            total = self[:source].collect do |source|
+                next unless result = perform_recursion(source)
+                result.each { |data| data.source = "%s/%s" % [source, data.relative_path] }
+                return result if result and ! result.empty? and self[:sourceselect] == :first
+                result
+            end.flatten
+
+            # This only happens if we have sourceselect == :all
+            found = []
+            total.find_all do |data|
+                result = ! found.include?(data.relative_path)
+                found << data.relative_path unless found.include?(data.relative_path)
+                result
+            end
+        end
+
+        def perform_recursion(path)
+            Puppet::FileServing::Metadata.search(self[:path], :links => self[:links], :recurse => self[:recurse], :ignore => self[:ignore])
+        end
+
         # Recurse into the directory.  This basically just calls 'localrecurse'
         # and maybe 'sourcerecurse', returning the collection of generated
         # files.
         def recurse
-            # are we at the end of the recursion?
-            return unless self.recurse?
-
-            recurse = self[:recurse]
-            # we might have a string, rather than a number
-            if recurse.is_a?(String)
-                if recurse =~ /^[0-9]+$/
-                    recurse = Integer(recurse)
-                else # anything else is infinite recursion
-                    recurse = true
-                end
+            children = recurse_local
+
+            if self[:target]
+                children += recurse_link 
             end
 
-            if recurse.is_a?(Integer)
-                recurse -= 1
+            if self[:source]
+                recurse_remote 
             end
-            
+
+            return children.collect { |child| newchild(child.relative_path) }.reject { |child| child.nil? }
+
             children = []
             
             # We want to do link-recursing before normal recursion so that all
diff --git a/lib/puppet/type/file/source.rb b/lib/puppet/type/file/source.rb
index 5eefb1dcb..03cc311b4 100755
--- a/lib/puppet/type/file/source.rb
+++ b/lib/puppet/type/file/source.rb
@@ -66,8 +66,14 @@ module Puppet
         uncheckable
         
         validate do |source|
-            unless @resource.uri2obj(source)
-                raise Puppet::Error, "Invalid source %s" % 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]
             end
         end
             
diff --git a/spec/unit/type/file.rb b/spec/unit/type/file.rb
index 8b7bedee6..7bbd2a4b2 100755
--- a/spec/unit/type/file.rb
+++ b/spec/unit/type/file.rb
@@ -78,4 +78,195 @@ describe Puppet::Type.type(:file) do
             @resource.flush
         end
     end
+
+    it "should have a method for performing recursion" do
+        @file.must respond_to(:perform_recursion)
+    end
+
+    describe "when executing a recursive search" do
+        it "should use Metadata to do its recursion" do
+            Puppet::FileServing::Metadata.expects(:search)
+            @file.perform_recursion(@file[:path])
+        end
+
+        it "should use its path as the key to the search" do
+            Puppet::FileServing::Metadata.expects(:search).with { |key, options| key = @file[:path] }
+            @file.perform_recursion(@file[:path])
+        end
+
+        it "should return the results of the metadata search" do
+            Puppet::FileServing::Metadata.expects(:search).returns "foobar"
+            @file.perform_recursion(@file[:path]).should == "foobar"
+        end
+
+        it "should pass its recursion value to the search" do
+            @file[:recurse] = 10
+            Puppet::FileServing::Metadata.expects(:search).with { |key, options| options[:recurse] == 10 }
+            @file.perform_recursion(@file[:path])
+        end
+
+        it "should configure the search to ignore or manage links" do
+            @file[:links] = :manage
+            Puppet::FileServing::Metadata.expects(:search).with { |key, options| options[:links] == :manage }
+            @file.perform_recursion(@file[:path])
+        end
+
+        it "should pass its 'ignore' setting to the search if it has one" do
+            @file[:ignore] = %w{.svn CVS}
+            Puppet::FileServing::Metadata.expects(:search).with { |key, options| options[:ignore] == %w{.svn CVS} }
+            @file.perform_recursion(@file[:path])
+        end
+    end
+
+    it "should have a method for performing local recursion" do
+        @file.must respond_to(:recurse_local)
+    end
+
+    it "should pass its path to the :perform_recursion method to do local recursion" do
+        @file.expects(:perform_recursion).with(@file[:path]).returns "foobar"
+        @file.recurse_local.should == "foobar"
+    end
+
+    it "should have a method for performing link recursion" do
+        @file.must respond_to(:recurse_link)
+    end
+
+    it "should pass its target to the :perform_recursion method to do link recursion" do
+        @file[:target] = "mylinks"
+        @file.expects(:perform_recursion).with("mylinks").returns "foobar"
+        @file.recurse_link.should == "foobar"
+    end
+
+    it "should have a method for performing remote recursion" do
+        @file.must respond_to(:recurse_remote)
+    end
+
+    it "should pass its source to the :perform_recursion method to do source recursion" do
+        data = Puppet::FileServing::Metadata.new("/whatever", :relative_path => "foobar")
+        @file[:source] = "puppet://foo/bar"
+        @file.expects(:perform_recursion).with("puppet://foo/bar").returns [data]
+        @file.recurse_remote.should == [data]
+    end
+
+    it "should set the source of each returned file to the searched-for URI plus the found relative path" do
+        metadata = stub 'metadata', :relative_path => "foobar"
+        metadata.expects(:source=).with "puppet://foo/bar/foobar"
+        @file[:source] = "puppet://foo/bar"
+        @file.expects(:perform_recursion).with("puppet://foo/bar").returns [metadata]
+        @file.recurse_remote.should == [metadata]
+    end
+
+    describe "when multiple sources are provided" do
+        describe "and :sourceselect is set to :first" do
+            it "should return the results for the first source to return any values" do
+                data = Puppet::FileServing::Metadata.new("/whatever", :relative_path => "foobar")
+                @file[:source] = %w{/one /two /three /four}
+                @file.expects(:perform_recursion).with("/one").returns nil
+                @file.expects(:perform_recursion).with("/two").returns []
+                @file.expects(:perform_recursion).with("/three").returns [data]
+                @file.expects(:perform_recursion).with("/four").never
+                @file.recurse_remote.should == [data]
+            end
+        end
+
+        describe "and :sourceselect is set to :all" do
+            before do
+                @file[:sourceselect] = :all
+            end
+
+            it "should return every found file that is not in a previous source" do
+                klass = Puppet::FileServing::Metadata
+                @file[:source] = %w{/one /two /three /four}
+
+                one = [klass.new("/one", :relative_path => "a")]
+                @file.expects(:perform_recursion).with("/one").returns one
+
+                two = [klass.new("/two", :relative_path => "a"), klass.new("/two", :relative_path => "b")]
+                @file.expects(:perform_recursion).with("/two").returns two
+
+                three = [klass.new("/three", :relative_path => "a"), klass.new("/three", :relative_path => "c")]
+                @file.expects(:perform_recursion).with("/three").returns three
+
+                @file.expects(:perform_recursion).with("/four").returns []
+
+                @file.recurse_remote.should == [one[0], two[1], three[1]]
+            end
+        end
+    end
+
+    it "should recurse during eval_generate if recursion is enabled" do
+        @file.expects(:recurse?).returns true
+        @file.expects(:recurse).returns "foobar"
+        @file.eval_generate.should == "foobar"
+    end
+
+    it "should not recurse during eval_generate if recursion is disabled" do
+        @file.expects(:recurse?).returns false
+        @file.expects(:recurse).never
+        @file.eval_generate.should be_nil
+    end
+
+    describe "when recursing" do
+        before do
+            @file[:recurse] = true
+            @metadata = Puppet::FileServing::Metadata
+        end
+
+        describe "and a source is set" do
+            before { @file[:source] = "/my/source" }
+
+            it "should use recurse_remote" do
+                @file.stubs(:recurse_local).returns []
+                @file.expects(:recurse_remote)
+                @file.recurse
+            end
+
+            it "should create a new file resource for each remote file"
+
+            it "should set the source for each new file resource"
+
+            it "should copy the metadata to the new file's source property so the file does not have to requery the remote system for metadata"
+
+            describe "and purging is enabled" do
+                it "should configure each file not on the remote system to be removed"
+            end
+        end
+
+        describe "and a target is set" do
+            before { @file[:target] = "/link/target" }
+
+            it "should use recurse_link" do
+                @file.stubs(:recurse_local).returns []
+                @file.expects(:recurse_link).returns []
+                @file.recurse
+            end
+
+            it "should return a new file resource for each link destination found"
+
+            it "should set the target for each new file resource"
+        end
+
+        it "should use recurse_local" do
+            @file.expects(:recurse_local).returns []
+            @file.recurse
+        end
+
+        it "should attempt to turn each found file into a child resource" do
+            a = @metadata.new("/foo", :relative_path => "a")
+            @file.expects(:recurse_local).returns [a]
+            @file.expects(:newchild).with("a")
+
+            @file.recurse
+        end
+
+        it "should not return nil for those files that could not be turned into children" do
+            a = @metadata.new("/foo", :relative_path => "a")
+            b = @metadata.new("/foo", :relative_path => "b")
+            @file.expects(:recurse_local).returns [a, b]
+            @file.expects(:newchild).with("a").returns "A"
+            @file.expects(:newchild).with("b").returns nil
+
+            @file.recurse.should == ["A"]
+        end
+    end
 end
diff --git a/spec/unit/type/file/source.rb b/spec/unit/type/file/source.rb
index d6aa25fe7..7c9850822 100755
--- a/spec/unit/type/file/source.rb
+++ b/spec/unit/type/file/source.rb
@@ -15,9 +15,16 @@ describe Puppet::Type.type(:file).attrclass(:source) do
 
     describe "when initializing" do
         it "should fail if the 'should' values are not URLs" do
-            @resource.expects(:uri2obj).with("foo").returns false
+            s = source.new(:resource => @resource)
+            URI.expects(:parse).with('foo').raises RuntimeError
 
-            lambda { source.new(:resource => @resource, :should => %w{foo}) }.must raise_error(Puppet::Error)
+            lambda { s.should = %w{foo} }.must raise_error(Puppet::Error)
+        end
+
+        it "should fail if the URI is not a local file, file URI, or puppet URI" do
+            s = source.new(:resource => @resource)
+
+            lambda { s.should = %w{http://foo/bar} }.must raise_error(Puppet::Error)
         end
     end
 
-- 
cgit 


From 5da26067cc76ad318359d9287ab1267d7a6c5b0b Mon Sep 17 00:00:00 2001
From: Luke Kanies <luke@madstop.com>
Date: Thu, 28 Aug 2008 22:48:01 -0700
Subject: Recursion using REST seems to almost work.

I think this is the bulk of the work, I just
need to write some integration tests to hunt down
a couple of small issues.

Signed-off-by: Luke Kanies <luke@madstop.com>
---
 lib/puppet/type/file.rb | 246 ++++++++++++---------------------
 spec/unit/type/file.rb  | 354 ++++++++++++++++++++++++++++++++++++------------
 2 files changed, 348 insertions(+), 252 deletions(-)

diff --git a/lib/puppet/type/file.rb b/lib/puppet/type/file.rb
index 2e3a585c1..2ae1e61b7 100644
--- a/lib/puppet/type/file.rb
+++ b/lib/puppet/type/file.rb
@@ -155,8 +155,6 @@ module Puppet
                 engine, so shell metacharacters are fully supported, e.g. ``[a-z]*``.
                 Matches that would descend into the directory structure are ignored,
                 e.g., ``*/*``."
-       
-            defaultto false
 
             validate do |value|
                 unless value.is_a?(Array) or value.is_a?(String) or value == false
@@ -277,11 +275,6 @@ module Puppet
 
         @depthfirst = false
 
-
-        def argument?(arg)
-            @arghash.include?(arg)
-        end
-
         # Determine the user to write files as.
         def asuser
             if self.should(:owner) and ! self.should(:owner).is_a?(Symbol)
@@ -329,7 +322,12 @@ module Puppet
         
         # Create any children via recursion or whatever.
         def eval_generate
-            recurse() if self.recurse?
+            raise(Puppet::DevError, "File recursion cannot happen without a catalog") unless catalog
+
+            return nil unless self.recurse?
+            recurse.reject { |resource| catalog.resource(:file, resource[:path]) }.each do |child|
+                catalog.relationship_graph.add_edge self, child
+            end
         end
 
         def flush
@@ -462,17 +460,6 @@ module Puppet
             
             @title.sub!(/\/$/, "") unless @title == "/"
 
-            # Clean out as many references to any file paths as possible.
-            # This was the source of many, many bugs.
-            @arghash = tmphash
-            @arghash.delete(self.class.namevar)
-
-            [:source, :parent].each do |param|
-                if @arghash.include?(param)
-                    @arghash.delete(param)
-                end
-            end
-
             @stat = nil
         end
 
@@ -568,88 +555,15 @@ module Puppet
         
         # Create a new file or directory object as a child to the current
         # object.
-        def newchild(path, local, hash = {})
-            raise(Puppet::DevError, "File recursion cannot happen without a catalog") unless catalog
-
-            # make local copy of arguments
-            args = symbolize_options(@arghash)
-
-            # There's probably a better way to do this, but we don't want
-            # to pass this info on.
-            if v = args[:ensure]
-                v = symbolize(v)
-                args.delete(:ensure)
-            end
-
-            if path =~ %r{^#{File::SEPARATOR}}
-                self.devfail(
-                    "Must pass relative paths to PFile#newchild()"
-                )
-            else
-                path = File.join(self[:path], path)
-            end
-
-            args[:path] = path
-
-            unless hash.include?(:recurse)
-                if args.include?(:recurse)
-                    if args[:recurse].is_a?(Integer)
-                        args[:recurse] -= 1 # reduce the level of recursion
-                    end
-                end
-
-            end
-
-            hash.each { |key,value|
-                args[key] = value
-            }
+        def newchild(path)
+            full_path = File.join(self[:path], path)
 
-            child = nil
-
-            # The child might already exist because 'localrecurse' runs
-            # before 'sourcerecurse'.  I could push the override stuff into
-            # a separate method or something, but the work is the same other
-            # than this last bit, so it doesn't really make sense.
-            if child = catalog.resource(:file, path)
-                unless child.parent.object_id == self.object_id
-                    self.debug "Not managing more explicit file %s" %
-                        path
-                    return nil
-                end
+            # the right-side hash wins in the merge.
+            options = to_hash.merge(:path => full_path)
+            options.delete(:parent) if options.include?(:parent)
+            options.delete(:recurse) if options.include?(:recurse)
 
-                # This is only necessary for sourcerecurse, because we might have
-                # created the object with different 'should' values than are
-                # set remotely.
-                unless local
-                    args.each { |var,value|
-                        next if var == :path
-                        next if var == :name
-
-                        # behave idempotently
-                        unless child.should(var) == value
-                            child[var] = value
-                        end
-                    }
-                end
-                return nil
-            else # create it anew
-                #notice "Creating new file with args %s" % args.inspect
-                args[:parent] = self
-                begin
-                    # This method is used by subclasses of :file, so use the class name rather than hard-coding
-                    # :file.
-                    return nil unless child = catalog.create_implicit_resource(self.class.name, args)
-                rescue => detail
-                    self.notice "Cannot manage: %s" % [detail]
-                    return nil
-                end
-            end
-
-            # LAK:FIXME This shouldn't be necessary, but as long as we're
-            # modeling the relationship graph specifically, it is.
-            catalog.relationship_graph.add_edge self, child
-
-            return child
+            return catalog.create_implicit_resource(self.class.name, options)
         end
 
         # Files handle paths specially, because they just lengthen their
@@ -679,97 +593,103 @@ module Puppet
             @parameters.include?(:purge) and (self[:purge] == :true or self[:purge] == "true")
         end
 
+        def make_children(metadata)
+            metadata.collect { |meta| newchild(meta.relative_path) }
+        end
+
+        # Recursively generate a list of file resources, which will
+        # be used to copy remote files, manage local files, and/or make links
+        # to map to another directory.
+        def recurse
+            children = recurse_local
+
+            if self[:target]
+                recurse_link(children)
+            elsif self[:source]
+                recurse_remote(children)
+            end
+
+            return children.values.sort { |a, b| a[:path] <=> b[:path] }
+        end
+
+        # A simple method for determining whether we should be recursing.
+        def recurse?
+            return false unless @parameters.include?(:recurse)
+
+            val = @parameters[:recurse].value
+
+            if val and (val == true or val > 0)
+                return true
+            else
+                return false
+            end
+        end
+
         # Recurse the target of the link.
-        def recurse_link
-            perform_recursion(self[:target])
+        def recurse_link(children)
+            perform_recursion(self[:target]).each do |meta|
+                children[meta.relative_path] ||= newchild(meta.relative_path)
+                if meta.ftype == "directory"
+                    children[meta.relative_path][:ensure] = :directory
+                else
+                    children[meta.relative_path][:ensure] = :link
+                    children[meta.relative_path][:target] = meta.full_path
+                end
+            end
+            children
         end
 
         # Recurse the file itself, returning a Metadata instance for every found file.
         def recurse_local
-            perform_recursion(self[:path])
+            perform_recursion(self[:path]).inject({}) { |hash, meta| hash[meta.relative_path] = newchild(meta.relative_path); hash }
         end
 
         # Recurse against our remote file.
-        def recurse_remote
+        def recurse_remote(children)
+            sourceselect = self[:sourceselect]
+
             total = self[:source].collect do |source|
                 next unless result = perform_recursion(source)
                 result.each { |data| data.source = "%s/%s" % [source, data.relative_path] }
-                return result if result and ! result.empty? and self[:sourceselect] == :first
+                break result if result and ! result.empty? and sourceselect == :first
                 result
             end.flatten
 
             # This only happens if we have sourceselect == :all
-            found = []
-            total.find_all do |data|
-                result = ! found.include?(data.relative_path)
-                found << data.relative_path unless found.include?(data.relative_path)
-                result
-            end
-        end
-
-        def perform_recursion(path)
-            Puppet::FileServing::Metadata.search(self[:path], :links => self[:links], :recurse => self[:recurse], :ignore => self[:ignore])
-        end
-
-        # Recurse into the directory.  This basically just calls 'localrecurse'
-        # and maybe 'sourcerecurse', returning the collection of generated
-        # files.
-        def recurse
-            children = recurse_local
-
-            if self[:target]
-                children += recurse_link 
-            end
-
-            if self[:source]
-                recurse_remote 
-            end
-
-            return children.collect { |child| newchild(child.relative_path) }.reject { |child| child.nil? }
-
-            children = []
-            
-            # We want to do link-recursing before normal recursion so that all
-            # of the target stuff gets copied over correctly.
-            if @parameters.include? :target and ret = self.linkrecurse(recurse)
-                children += ret
-            end
-            if ret = self.localrecurse(recurse)
-                children += ret
+            unless sourceselect == :first
+                found = []
+                total.reject! do |data|
+                    result = found.include?(data.relative_path)
+                    found << data.relative_path unless found.include?(data.relative_path)
+                    result
+                end
             end
 
-            # These will be files pulled in by the file source
-            sourced = false
-            if @parameters.include?(:source)
-                ret, sourced = self.sourcerecurse(recurse)
-                if ret
-                    children += ret
-                end
+            total.each do |meta|
+                children[meta.relative_path] ||= newchild(meta.relative_path)
+                children[meta.relative_path][:source] = meta.source
+                children[meta.relative_path].property(:source).metadata = meta
             end
 
-            # The purge check needs to happen after all of the other recursion.
+            # If we're purging resources, then delete any resource that isn't on the
+            # remote system.
             if self.purge?
-                children.each do |child|
-                    if (sourced and ! sourced.include?(child[:path])) or ! child.managed?
+                # 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
 
-        # A simple method for determining whether we should be recursing.
-        def recurse?
-            return false unless @parameters.include?(:recurse)
-
-            val = @parameters[:recurse].value
-
-            if val and (val == true or val > 0)
-                return true
-            else
-                return false
-            end
+        def perform_recursion(path)
+            Puppet::FileServing::Metadata.search(path, :links => self[:links], :recurse => self[:recurse], :ignore => self[:ignore])
         end
 
         # Remove the old backup.
diff --git a/spec/unit/type/file.rb b/spec/unit/type/file.rb
index 7bbd2a4b2..27a077bed 100755
--- a/spec/unit/type/file.rb
+++ b/spec/unit/type/file.rb
@@ -8,6 +8,10 @@ describe Puppet::Type.type(:file) do
         @path.close!()
         @path = @path.path
         @file = Puppet::Type::File.create(:name => @path)
+
+        @catalog = mock 'catalog'
+        @catalog.stub_everything
+        @file.catalog = @catalog
     end
 
     describe "when used with content and replace=>false" do
@@ -89,9 +93,9 @@ describe Puppet::Type.type(:file) do
             @file.perform_recursion(@file[:path])
         end
 
-        it "should use its path as the key to the search" do
-            Puppet::FileServing::Metadata.expects(:search).with { |key, options| key = @file[:path] }
-            @file.perform_recursion(@file[:path])
+        it "should use the provided path as the key to the search" do
+            Puppet::FileServing::Metadata.expects(:search).with { |key, options| key == "/foo" }
+            @file.perform_recursion("/foo")
         end
 
         it "should return the results of the metadata search" do
@@ -122,88 +126,258 @@ describe Puppet::Type.type(:file) do
         @file.must respond_to(:recurse_local)
     end
 
-    it "should pass its path to the :perform_recursion method to do local recursion" do
-        @file.expects(:perform_recursion).with(@file[:path]).returns "foobar"
-        @file.recurse_local.should == "foobar"
+    describe "when doing local recursion" do
+        before do
+            @metadata = stub 'metadata', :relative_path => "my/file"
+        end
+
+        it "should pass its to the :perform_recursion method" do
+            @file.expects(:perform_recursion).with(@file[:path]).returns [@metadata]
+            @file.stubs(:newchild)
+            @file.recurse_local
+        end
+
+        it "should create a new child resource with each generated metadata instance's relative path" do
+            @file.expects(:perform_recursion).returns [@metadata]
+            @file.expects(:newchild).with(@metadata.relative_path).returns "fiebar"
+            @file.recurse_local
+        end
+
+        it "should return a hash of the created resources with the relative paths as the hash keys" do
+            @file.expects(:perform_recursion).returns [@metadata]
+            @file.expects(:newchild).with("my/file").returns "fiebar"
+            @file.recurse_local.should == {"my/file" => "fiebar"}
+        end
     end
 
     it "should have a method for performing link recursion" do
         @file.must respond_to(:recurse_link)
     end
 
-    it "should pass its target to the :perform_recursion method to do link recursion" do
-        @file[:target] = "mylinks"
-        @file.expects(:perform_recursion).with("mylinks").returns "foobar"
-        @file.recurse_link.should == "foobar"
+    describe "when doing link recursion" do
+        before do
+            @first = stub 'first', :relative_path => "first", :full_path => "/my/first", :ftype => "directory"
+            @second = stub 'second', :relative_path => "second", :full_path => "/my/second", :ftype => "file"
+
+            @resource = stub 'file', :[]= => nil
+        end
+
+        it "should pass its target to the :perform_recursion method" do
+            @file[:target] = "mylinks"
+            @file.expects(:perform_recursion).with("mylinks").returns [@first]
+            @file.stubs(:newchild).returns @resource
+            @file.recurse_link({})
+        end
+
+        it "should create a new child resource for each generated metadata instance's relative path that doesn't already exist in the children hash" do
+            @file.expects(:perform_recursion).returns [@first, @second]
+            @file.expects(:newchild).with(@first.relative_path).returns @resource
+            @file.recurse_link("second" => @resource)
+        end
+
+        it "should not create a new child resource for paths that already exist in the children hash" do
+            @file.expects(:perform_recursion).returns [@first]
+            @file.expects(:newchild).never
+            @file.recurse_link("first" => @resource)
+        end
+
+        it "should set the target to the full path of discovered file and set :ensure to :link if the file is not a directory" do
+            file = stub 'file'
+            file.expects(:[]=).with(:target, "/my/second")
+            file.expects(:[]=).with(:ensure, :link)
+
+            @file.stubs(:perform_recursion).returns [@first, @second]
+            @file.recurse_link("first" => @resource, "second" => file)
+        end
+
+        it "should :ensure to :directory if the file is a directory" do
+            file = stub 'file'
+            file.expects(:[]=).with(:ensure, :directory)
+
+            @file.stubs(:perform_recursion).returns [@first, @second]
+            @file.recurse_link("first" => file, "second" => @resource)
+        end
+
+        it "should return a hash with both created and existing resources with the relative paths as the hash keys" do
+            file = stub 'file', :[]= => nil
+
+            @file.expects(:perform_recursion).returns [@first, @second]
+            @file.stubs(:newchild).returns file
+            @file.recurse_link("second" => @resource).should == {"second" => @resource, "first" => file}
+        end
     end
 
     it "should have a method for performing remote recursion" do
         @file.must respond_to(:recurse_remote)
     end
 
-    it "should pass its source to the :perform_recursion method to do source recursion" do
-        data = Puppet::FileServing::Metadata.new("/whatever", :relative_path => "foobar")
-        @file[:source] = "puppet://foo/bar"
-        @file.expects(:perform_recursion).with("puppet://foo/bar").returns [data]
-        @file.recurse_remote.should == [data]
-    end
+    describe "when doing remote recursion" do
+        before do
+            @file[:source] = "puppet://foo/bar"
 
-    it "should set the source of each returned file to the searched-for URI plus the found relative path" do
-        metadata = stub 'metadata', :relative_path => "foobar"
-        metadata.expects(:source=).with "puppet://foo/bar/foobar"
-        @file[:source] = "puppet://foo/bar"
-        @file.expects(:perform_recursion).with("puppet://foo/bar").returns [metadata]
-        @file.recurse_remote.should == [metadata]
-    end
+            @first = Puppet::FileServing::Metadata.new("/my", :relative_path => "first")
+            @second = Puppet::FileServing::Metadata.new("/my", :relative_path => "second")
 
-    describe "when multiple sources are provided" do
-        describe "and :sourceselect is set to :first" do
-            it "should return the results for the first source to return any values" do
-                data = Puppet::FileServing::Metadata.new("/whatever", :relative_path => "foobar")
-                @file[:source] = %w{/one /two /three /four}
-                @file.expects(:perform_recursion).with("/one").returns nil
-                @file.expects(:perform_recursion).with("/two").returns []
-                @file.expects(:perform_recursion).with("/three").returns [data]
-                @file.expects(:perform_recursion).with("/four").never
-                @file.recurse_remote.should == [data]
-            end
+            @property = stub 'property', :metadata= => nil
+            @resource = stub 'file', :[]= => nil, :property => @property
         end
 
-        describe "and :sourceselect is set to :all" do
+        it "should pass its source to the :perform_recursion method" do
+            data = Puppet::FileServing::Metadata.new("/whatever", :relative_path => "foobar")
+            @file.expects(:perform_recursion).with("puppet://foo/bar").returns [data]
+            @file.stubs(:newchild).returns @resource
+            @file.recurse_remote({})
+        end
+
+        it "should set the source of each returned file to the searched-for URI plus the found relative path" do
+            @first.expects(:source=).with File.join("puppet://foo/bar", @first.relative_path)
+            @file.expects(:perform_recursion).returns [@first]
+            @file.stubs(:newchild).returns @resource
+            @file.recurse_remote({})
+        end
+
+        it "should create a new resource for any relative file paths that do not already have a resource" do
+            @file.stubs(:perform_recursion).returns [@first]
+            @file.expects(:newchild).with("first").returns @resource
+            @file.recurse_remote({}).should == {"first" => @resource}
+        end
+
+        it "should not create a new resource for any relative file paths that do already have a resource" do
+            @file.stubs(:perform_recursion).returns [@first]
+            @file.expects(:newchild).never
+            @file.recurse_remote("first" => @resource)
+        end
+
+        it "should set the source of each resource to the source of the metadata" do
+            @file.stubs(:perform_recursion).returns [@first]
+            @resource.expects(:[]=).with(:source, File.join("puppet://foo/bar", @first.relative_path))
+            @file.recurse_remote("first" => @resource)
+        end
+
+        it "should store the metadata in the source property for each resource so the source does not have to requery the metadata" do
+            @file.stubs(:perform_recursion).returns [@first]
+            @resource.expects(:property).with(:source).returns @property
+            
+            @property.expects(:metadata=).with(@first)
+
+            @file.recurse_remote("first" => @resource)
+        end
+
+        describe "and purging is enabled" do
             before do
-                @file[:sourceselect] = :all
+                @file[:purge] = true
             end
 
-            it "should return every found file that is not in a previous source" do
-                klass = Puppet::FileServing::Metadata
-                @file[:source] = %w{/one /two /three /four}
+            it "should configure each file not on the remote system to be removed" do
+                @file.stubs(:perform_recursion).returns [@second]
 
-                one = [klass.new("/one", :relative_path => "a")]
-                @file.expects(:perform_recursion).with("/one").returns one
+                @resource.expects(:[]=).with(:ensure, :absent)
 
-                two = [klass.new("/two", :relative_path => "a"), klass.new("/two", :relative_path => "b")]
-                @file.expects(:perform_recursion).with("/two").returns two
+                @file.expects(:newchild).returns stub('secondfile', :[]= => nil, :property => @property)
 
-                three = [klass.new("/three", :relative_path => "a"), klass.new("/three", :relative_path => "c")]
-                @file.expects(:perform_recursion).with("/three").returns three
+                @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
+                    data = Puppet::FileServing::Metadata.new("/whatever", :relative_path => "foobar")
+                    @file[:source] = %w{/one /two /three /four}
+                    @file.expects(:perform_recursion).with("/one").returns nil
+                    @file.expects(:perform_recursion).with("/two").returns []
+                    @file.expects(:perform_recursion).with("/three").returns [data]
+                    @file.expects(:perform_recursion).with("/four").never
+                    @file.expects(:newchild).with("foobar").returns @resource
+                    @file.recurse_remote({})
+                end
+            end
+
+            describe "and :sourceselect is set to :all" do
+                before do
+                    @file[:sourceselect] = :all
+                end
+
+                it "should return every found file that is not in a previous source" do
+                    klass = Puppet::FileServing::Metadata
+                    @file[:source] = %w{/one /two /three /four}
+                    @file.stubs(:newchild).returns @resource
+
+                    one = [klass.new("/one", :relative_path => "a")]
+                    @file.expects(:perform_recursion).with("/one").returns one
+                    @file.expects(:newchild).with("a").returns @resource
 
-                @file.expects(:perform_recursion).with("/four").returns []
+                    two = [klass.new("/two", :relative_path => "a"), klass.new("/two", :relative_path => "b")]
+                    @file.expects(:perform_recursion).with("/two").returns two
+                    @file.expects(:newchild).with("b").returns @resource
 
-                @file.recurse_remote.should == [one[0], two[1], three[1]]
+                    three = [klass.new("/three", :relative_path => "a"), klass.new("/three", :relative_path => "c")]
+                    @file.expects(:perform_recursion).with("/three").returns three
+                    @file.expects(:newchild).with("c").returns @resource
+
+                    @file.expects(:perform_recursion).with("/four").returns []
+
+                    @file.recurse_remote({})
+                end
             end
         end
     end
 
-    it "should recurse during eval_generate if recursion is enabled" do
-        @file.expects(:recurse?).returns true
-        @file.expects(:recurse).returns "foobar"
-        @file.eval_generate.should == "foobar"
-    end
+    describe "when returning resources with :eval_generate" do
+        before do
+            @catalog = mock 'catalog'
+            @catalog.stub_everything
+
+            @graph = stub 'graph', :add_edge => nil
+            @catalog.stubs(:relationship_graph).returns @graph
+
+            @file.catalog = @catalog
+            @file[:recurse] = true
+        end
+
+        it "should recurse if recursion is enabled" do
+            resource = stub('resource', :[] => "resource")
+            @file.expects(:recurse?).returns true
+            @file.expects(:recurse).returns [resource]
+            @file.eval_generate.should == [resource]
+        end
+
+        it "should not recurse if recursion is disabled" do
+            @file.expects(:recurse?).returns false
+            @file.expects(:recurse).never
+            @file.eval_generate.should be_nil
+        end
+
+        it "should fail if no catalog is set" do
+            @file.catalog = nil
+            lambda { @file.eval_generate }.should raise_error(Puppet::DevError)
+        end
+
+        it "should skip resources that are already in the catalog" do
+            foo = stub 'foo', :[] => "/foo"
+            bar = stub 'bar', :[] => "/bar"
+            bar2 = stub 'bar2', :[] => "/bar"
+
+            @catalog.expects(:resource).with(:file, "/foo").returns nil
+            @catalog.expects(:resource).with(:file, "/bar").returns bar2
+
+            @file.expects(:recurse).returns [foo, bar]
 
-    it "should not recurse during eval_generate if recursion is disabled" do
-        @file.expects(:recurse?).returns false
-        @file.expects(:recurse).never
-        @file.eval_generate.should be_nil
+            @file.eval_generate.should == [foo]
+        end
+
+        it "should add a relationshp edge for each returned resource" do
+            foo = stub 'foo', :[] => "/foo"
+
+            @file.expects(:recurse).returns [foo]
+
+            graph = mock 'graph'
+            @catalog.stubs(:relationship_graph).returns graph
+
+            graph.expects(:add_edge).with(@file, foo)
+
+            @file.eval_generate
+        end
     end
 
     describe "when recursing" do
@@ -215,58 +389,60 @@ describe Puppet::Type.type(:file) do
         describe "and a source is set" do
             before { @file[:source] = "/my/source" }
 
-            it "should use recurse_remote" do
-                @file.stubs(:recurse_local).returns []
-                @file.expects(:recurse_remote)
+            it "should pass the already-discovered resources to recurse_remote" do
+                @file.stubs(:recurse_local).returns(:foo => "bar")
+                @file.expects(:recurse_remote).with(:foo => "bar").returns []
                 @file.recurse
             end
-
-            it "should create a new file resource for each remote file"
-
-            it "should set the source for each new file resource"
-
-            it "should copy the metadata to the new file's source property so the file does not have to requery the remote system for metadata"
-
-            describe "and purging is enabled" do
-                it "should configure each file not on the remote system to be removed"
-            end
         end
 
         describe "and a target is set" do
             before { @file[:target] = "/link/target" }
 
             it "should use recurse_link" do
-                @file.stubs(:recurse_local).returns []
-                @file.expects(:recurse_link).returns []
+                @file.stubs(:recurse_local).returns(:foo => "bar")
+                @file.expects(:recurse_link).with(:foo => "bar").returns []
                 @file.recurse
             end
-
-            it "should return a new file resource for each link destination found"
-
-            it "should set the target for each new file resource"
         end
 
         it "should use recurse_local" do
-            @file.expects(:recurse_local).returns []
+            @file.expects(:recurse_local).returns({})
             @file.recurse
         end
 
-        it "should attempt to turn each found file into a child resource" do
-            a = @metadata.new("/foo", :relative_path => "a")
-            @file.expects(:recurse_local).returns [a]
-            @file.expects(:newchild).with("a")
-
-            @file.recurse
+        it "should return the generated resources as an array sorted by file path" do
+            one = stub 'one', :[] => "/one"
+            two = stub 'two', :[] => "/one/two"
+            three = stub 'three', :[] => "/three"
+            @file.expects(:recurse_local).returns(:one => one, :two => two, :three => three)
+            @file.recurse.should == [one, two, three]
         end
 
-        it "should not return nil for those files that could not be turned into children" do
-            a = @metadata.new("/foo", :relative_path => "a")
-            b = @metadata.new("/foo", :relative_path => "b")
-            @file.expects(:recurse_local).returns [a, b]
-            @file.expects(:newchild).with("a").returns "A"
-            @file.expects(:newchild).with("b").returns nil
+        describe "and making a new child resource" do
+            it "should create an implicit resource using the provided relative path joined with the file's path" do
+                path = File.join(@file[:path], "my/path")
+                @catalog.expects(:create_implicit_resource).with { |klass, options| options[:path] == path }
+                @file.newchild("my/path")
+            end
 
-            @file.recurse.should == ["A"]
+            it "should copy most of the parent resource's 'should' values to the new resource" do
+                @file.expects(:to_hash).returns :foo => "bar", :fee => "fum"
+                @catalog.expects(:create_implicit_resource).with { |klass, options| options[:foo] == "bar" and options[:fee] == "fum" }
+                @file.newchild("my/path")
+            end
+
+            it "should not copy the parent resource's parent" do
+                @file.expects(:to_hash).returns :parent => "foo"
+                @catalog.expects(:create_implicit_resource).with { |klass, options| ! options.include?(:parent) }
+                @file.newchild("my/path")
+            end
+
+            it "should not copy the parent resource's recurse value" do
+                @file.expects(:to_hash).returns :recurse => true
+                @catalog.expects(:create_implicit_resource).with { |klass, options| ! options.include?(:recurse) }
+                @file.newchild("my/path")
+            end
         end
     end
 end
-- 
cgit 


From bd1163a339ff66dbb9a50a1cb13f6320cb056cc3 Mon Sep 17 00:00:00 2001
From: Luke Kanies <luke@madstop.com>
Date: Thu, 28 Aug 2008 23:09:52 -0700
Subject: Fixing filesets to allow nil ignore values.

Signed-off-by: Luke Kanies <luke@madstop.com>
---
 lib/puppet/file_serving/fileset.rb | 2 ++
 spec/unit/file_serving/fileset.rb  | 7 +++++++
 2 files changed, 9 insertions(+)

diff --git a/lib/puppet/file_serving/fileset.rb b/lib/puppet/file_serving/fileset.rb
index 80a718c68..a90734a2b 100644
--- a/lib/puppet/file_serving/fileset.rb
+++ b/lib/puppet/file_serving/fileset.rb
@@ -30,6 +30,8 @@ class Puppet::FileServing::Fileset
 
     # Should we ignore this path?
     def ignore?(path)
+        return false if @ignore == [nil]
+
         # 'detect' normally returns the found result, whereas we just want true/false.
         ! @ignore.detect { |pattern| File.fnmatch?(pattern, path) }.nil?
     end
diff --git a/spec/unit/file_serving/fileset.rb b/spec/unit/file_serving/fileset.rb
index 2cd3e83dd..f95271050 100755
--- a/spec/unit/file_serving/fileset.rb
+++ b/spec/unit/file_serving/fileset.rb
@@ -149,6 +149,13 @@ describe Puppet::FileServing::Fileset, " when recursing" do
         @fileset.files.sort.should == @files.sort
     end
 
+    it "should function if the :ignore value provided is nil" do
+        mock_dir_structure(@path)
+        @fileset.recurse = true
+        @fileset.ignore = nil
+        lambda { @fileset.files }.should_not raise_error
+    end
+
     it "should ignore files that match a single pattern in the ignore list" do
         mock_dir_structure(@path)
         @fileset.recurse = true
-- 
cgit 


From 93fc1139550bd97a11529b812e77ac0fc00c6079 Mon Sep 17 00:00:00 2001
From: Luke Kanies <luke@madstop.com>
Date: Thu, 28 Aug 2008 23:28:22 -0700
Subject: Files now use the Indirector to recurse locally.

I don't yet have integration tests for remote recursion
or link recursion, but we're nearly there.

Signed-off-by: Luke Kanies <luke@madstop.com>
---
 lib/puppet/type/file.rb       | 21 ++++++++++++-----
 spec/integration/type/file.rb | 53 +++++++++++++++++++++++++++++++++++++++++++
 spec/unit/type/file.rb        | 29 +++++++++++++++++++----
 3 files changed, 93 insertions(+), 10 deletions(-)
 create mode 100755 spec/integration/type/file.rb

diff --git a/lib/puppet/type/file.rb b/lib/puppet/type/file.rb
index 2ae1e61b7..a59192af3 100644
--- a/lib/puppet/type/file.rb
+++ b/lib/puppet/type/file.rb
@@ -322,10 +322,14 @@ module Puppet
         
         # Create any children via recursion or whatever.
         def eval_generate
-            raise(Puppet::DevError, "File recursion cannot happen without a catalog") unless catalog
-
             return nil unless self.recurse?
-            recurse.reject { |resource| catalog.resource(:file, resource[:path]) }.each do |child|
+
+            raise(Puppet::DevError, "Cannot generate resources for recursion without a catalog") unless catalog
+
+            recurse.reject do |resource|
+                catalog.resource(:file, resource[:path])
+            end.each do |child|
+                catalog.add_resource child
                 catalog.relationship_graph.add_edge self, child
             end
         end
@@ -559,11 +563,11 @@ module Puppet
             full_path = File.join(self[:path], path)
 
             # the right-side hash wins in the merge.
-            options = to_hash.merge(:path => full_path)
+            options = to_hash.merge(:path => full_path, :implicit => true)
             options.delete(:parent) if options.include?(:parent)
             options.delete(:recurse) if options.include?(:recurse)
 
-            return catalog.create_implicit_resource(self.class.name, options)
+            return self.class.create(options)
         end
 
         # Files handle paths specially, because they just lengthen their
@@ -641,7 +645,12 @@ module Puppet
 
         # Recurse the file itself, returning a Metadata instance for every found file.
         def recurse_local
-            perform_recursion(self[:path]).inject({}) { |hash, meta| hash[meta.relative_path] = newchild(meta.relative_path); hash }
+            perform_recursion(self[:path]).inject({}) do |hash, meta|
+                next hash if meta.relative_path == "."
+
+                hash[meta.relative_path] = newchild(meta.relative_path)
+                hash
+            end
         end
 
         # Recurse against our remote file.
diff --git a/spec/integration/type/file.rb b/spec/integration/type/file.rb
new file mode 100755
index 000000000..39d0b62dc
--- /dev/null
+++ b/spec/integration/type/file.rb
@@ -0,0 +1,53 @@
+#!/usr/bin/env ruby
+
+require File.dirname(__FILE__) + '/../../spec_helper'
+
+describe Puppet::Type.type(:file), "when recursing" do
+    def mkdir
+    end
+
+    def build_path(dir)
+        Dir.mkdir(dir)
+        File.chmod(0750, dir)
+
+        @dirs = [dir]
+        @files = []
+
+        %w{one two}.each do |subdir|
+            fdir = File.join(dir, subdir)
+            Dir.mkdir(fdir)
+            File.chmod(0750, fdir)
+            @dirs << fdir
+
+            %w{three}.each do |file|
+                ffile = File.join(fdir, file)
+                @files << ffile
+                File.open(ffile, "w") { |f| f.puts "test %s" % file }
+                File.chmod(0640, ffile)
+            end
+        end
+    end
+
+    it "should be able to recursively set properties on existing files" do
+        @path = Tempfile.new("file_integration_tests")
+        @path.close!()
+        @path = @path.path
+
+        build_path(@path)
+
+        @file = Puppet::Type::File.create(:name => @path, :mode => 0644, :recurse => true)
+
+        @catalog = Puppet::Node::Catalog.new
+        @catalog.add_resource @file
+
+        @catalog.apply
+
+        @dirs.each do |path|
+            (File.stat(path).mode & 007777).should == 0755
+        end
+
+        @files.each do |path|
+            (File.stat(path).mode & 007777).should == 0644
+        end
+    end
+end
diff --git a/spec/unit/type/file.rb b/spec/unit/type/file.rb
index 27a077bed..9578a2d17 100755
--- a/spec/unit/type/file.rb
+++ b/spec/unit/type/file.rb
@@ -143,6 +143,14 @@ describe Puppet::Type.type(:file) do
             @file.recurse_local
         end
 
+        it "should not create a new child resource for the '.' directory" do
+            @metadata.stubs(:relative_path).returns "."
+
+            @file.expects(:perform_recursion).returns [@metadata]
+            @file.expects(:newchild).never
+            @file.recurse_local
+        end
+
         it "should return a hash of the created resources with the relative paths as the hash keys" do
             @file.expects(:perform_recursion).returns [@metadata]
             @file.expects(:newchild).with("my/file").returns "fiebar"
@@ -366,6 +374,19 @@ describe Puppet::Type.type(:file) do
             @file.eval_generate.should == [foo]
         end
 
+        it "should add each resource to the catalog" do
+            foo = stub 'foo', :[] => "/foo"
+            bar = stub 'bar', :[] => "/bar"
+            bar2 = stub 'bar2', :[] => "/bar"
+
+            @catalog.expects(:add_resource).with(foo)
+            @catalog.expects(:add_resource).with(bar)
+
+            @file.expects(:recurse).returns [foo, bar]
+
+            @file.eval_generate
+        end
+
         it "should add a relationshp edge for each returned resource" do
             foo = stub 'foo', :[] => "/foo"
 
@@ -422,25 +443,25 @@ describe Puppet::Type.type(:file) do
         describe "and making a new child resource" do
             it "should create an implicit resource using the provided relative path joined with the file's path" do
                 path = File.join(@file[:path], "my/path")
-                @catalog.expects(:create_implicit_resource).with { |klass, options| options[:path] == path }
+                Puppet::Type.type(:file).expects(:create).with { |options| options[:implicit] == true and options[:path] == path }
                 @file.newchild("my/path")
             end
 
             it "should copy most of the parent resource's 'should' values to the new resource" do
                 @file.expects(:to_hash).returns :foo => "bar", :fee => "fum"
-                @catalog.expects(:create_implicit_resource).with { |klass, options| options[:foo] == "bar" and options[:fee] == "fum" }
+                Puppet::Type.type(:file).expects(:create).with { |options| options[:foo] == "bar" and options[:fee] == "fum" }
                 @file.newchild("my/path")
             end
 
             it "should not copy the parent resource's parent" do
                 @file.expects(:to_hash).returns :parent => "foo"
-                @catalog.expects(:create_implicit_resource).with { |klass, options| ! options.include?(:parent) }
+                Puppet::Type.type(:file).expects(:create).with { |options| ! options.include?(:parent) }
                 @file.newchild("my/path")
             end
 
             it "should not copy the parent resource's recurse value" do
                 @file.expects(:to_hash).returns :recurse => true
-                @catalog.expects(:create_implicit_resource).with { |klass, options| ! options.include?(:recurse) }
+                Puppet::Type.type(:file).expects(:create).with { |options| ! options.include?(:recurse) }
                 @file.newchild("my/path")
             end
         end
-- 
cgit 


From 45f465bc98aa87e1066a9d748dbb6bfaaef61476 Mon Sep 17 00:00:00 2001
From: Luke Kanies <luke@madstop.com>
Date: Fri, 29 Aug 2008 00:48:40 -0700
Subject: Source recursion is nearly working.

It works, but you have to run it multiple times,
and there are still a couple of strangenesses with the
parameter values, such as the mode not getting set on
the first run.

Signed-off-by: Luke Kanies <luke@madstop.com>
---
 lib/puppet/type/file.rb        | 29 ++++++++++++----
 lib/puppet/type/file/source.rb | 11 ++++---
 spec/integration/type/file.rb  | 75 ++++++++++++++++++++++++++++++++++++++++++
 spec/unit/type/file.rb         | 49 +++++++++++++++++++++++++++
 spec/unit/type/file/source.rb  | 44 +++++++++++++++++--------
 5 files changed, 183 insertions(+), 25 deletions(-)

diff --git a/lib/puppet/type/file.rb b/lib/puppet/type/file.rb
index a59192af3..ce1df1c35 100644
--- a/lib/puppet/type/file.rb
+++ b/lib/puppet/type/file.rb
@@ -564,8 +564,9 @@ module Puppet
 
             # the right-side hash wins in the merge.
             options = to_hash.merge(:path => full_path, :implicit => true)
-            options.delete(:parent) if options.include?(:parent)
-            options.delete(:recurse) if options.include?(:recurse)
+            [:parent, :recurse, :target].each do |param|
+                options.delete(param) if options.include?(param)
+            end
 
             return self.class.create(options)
         end
@@ -632,6 +633,11 @@ module Puppet
         # Recurse the target of the link.
         def recurse_link(children)
             perform_recursion(self[:target]).each do |meta|
+                if meta.relative_path == "."
+                    self[:ensure] = :directory
+                    next
+                end
+
                 children[meta.relative_path] ||= newchild(meta.relative_path)
                 if meta.ftype == "directory"
                     children[meta.relative_path][:ensure] = :directory
@@ -645,7 +651,9 @@ module Puppet
 
         # Recurse the file itself, returning a Metadata instance for every found file.
         def recurse_local
-            perform_recursion(self[:path]).inject({}) do |hash, meta|
+            result = perform_recursion(self[:path])
+            return {} unless result
+            result.inject({}) do |hash, meta|
                 next hash if meta.relative_path == "."
 
                 hash[meta.relative_path] = newchild(meta.relative_path)
@@ -675,8 +683,14 @@ module Puppet
             end
 
             total.each do |meta|
+                if meta.relative_path == "."
+                    property(: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
             end
 
@@ -759,21 +773,22 @@ module Puppet
         # a wrapper method to make sure the file exists before doing anything
         def retrieve
             unless stat = self.stat(true)
-                # If the file doesn't exist but we have a source, then call
-                # retrieve on that property
 
                 propertyvalues = properties().inject({}) { |hash, property|
                                      hash[property] = :absent
                                      hash
                                   }
 
+                # If the file doesn't exist but we have a source, then call
+                # retrieve on the source property so it will set the 'should'
+                # values all around.
                 if @parameters.include?(:source)
-                    propertyvalues[:source] = @parameters[:source].retrieve
+                    @parameters[:source].copy_source_values
                 end
                 return propertyvalues
             end
 
-            return currentpropvalues()
+            currentpropvalues()
         end
 
         # This recurses against the remote source and makes sure the local
diff --git a/lib/puppet/type/file/source.rb b/lib/puppet/type/file/source.rb
index 03cc311b4..e43706051 100755
--- a/lib/puppet/type/file/source.rb
+++ b/lib/puppet/type/file/source.rb
@@ -122,13 +122,14 @@ module Puppet
 
             # Take each of the stats and set them as states on the local file
             # if a value has not already been provided.
-            [:owner, :mode, :group].each do |param|
-                @resource[param] ||= metadata.send(param)
+            [:owner, :mode, :group, :checksum].each do |param|
+                next if param == :owner and Puppet::Util::SUIDManager.uid != 0
+                unless value = @resource[param] and value != :absent
+                    @resource[param] = metadata.send(param)
+                end
             end
 
-            unless @resource.deleting?
-                @resource[:ensure] = metadata.ftype
-            end
+            @resource[:ensure] = metadata.ftype
 
             if metadata.ftype == "link"
                 @resource[:target] = metadata.destination
diff --git a/spec/integration/type/file.rb b/spec/integration/type/file.rb
index 39d0b62dc..b59fa3294 100755
--- a/spec/integration/type/file.rb
+++ b/spec/integration/type/file.rb
@@ -28,6 +28,19 @@ describe Puppet::Type.type(:file), "when recursing" do
         end
     end
 
+    it "should be able to recurse over a nonexistent file" do
+        @path = Tempfile.new("file_integration_tests")
+        @path.close!()
+        @path = @path.path
+
+        @file = Puppet::Type::File.create(:name => @path, :mode => 0644, :recurse => true)
+
+        @catalog = Puppet::Node::Catalog.new
+        @catalog.add_resource @file
+
+        lambda { @file.eval_generate }.should_not raise_error
+    end
+
     it "should be able to recursively set properties on existing files" do
         @path = Tempfile.new("file_integration_tests")
         @path.close!()
@@ -50,4 +63,66 @@ describe Puppet::Type.type(:file), "when recursing" do
             (File.stat(path).mode & 007777).should == 0644
         end
     end
+
+    it "should be able to recursively make links to other files" do
+        source = Tempfile.new("file_link_integration_source")
+        source.close!()
+        source = source.path
+
+        build_path(source)
+
+        dest = Tempfile.new("file_link_integration_dest")
+        dest.close!()
+        dest = dest.path
+
+        @file = Puppet::Type::File.create(:name => dest, :target => source, :recurse => true, :ensure => :link)
+
+        @catalog = Puppet::Node::Catalog.new
+        @catalog.add_resource @file
+
+        @catalog.apply
+
+        @dirs.each do |path|
+            link_path = path.sub(source, dest)
+
+            File.lstat(link_path).should be_directory
+        end
+
+        @files.each do |path|
+            link_path = path.sub(source, dest)
+
+            File.lstat(link_path).ftype.should == "link"
+        end
+    end
+
+    it "should be able to recursively copy files" do
+        source = Tempfile.new("file_source_integration_source")
+        source.close!()
+        source = source.path
+
+        build_path(source)
+
+        dest = Tempfile.new("file_source_integration_dest")
+        dest.close!()
+        dest = dest.path
+
+        @file = Puppet::Type::File.create(:name => dest, :source => source, :recurse => true)
+
+        @catalog = Puppet::Node::Catalog.new
+        @catalog.add_resource @file
+
+        @catalog.apply
+
+        @dirs.each do |path|
+            newpath = path.sub(source, dest)
+
+            File.lstat(newpath).should be_directory
+        end
+
+        @files.each do |path|
+            newpath = path.sub(source, dest)
+
+            File.lstat(newpath).ftype.should == "file"
+        end
+    end
 end
diff --git a/spec/unit/type/file.rb b/spec/unit/type/file.rb
index 9578a2d17..327c49d9c 100755
--- a/spec/unit/type/file.rb
+++ b/spec/unit/type/file.rb
@@ -137,6 +137,11 @@ describe Puppet::Type.type(:file) do
             @file.recurse_local
         end
 
+        it "should return an empty hash if the recursion returns nothing" do
+            @file.expects(:perform_recursion).returns nil
+            @file.recurse_local.should == {}
+        end
+
         it "should create a new child resource with each generated metadata instance's relative path" do
             @file.expects(:perform_recursion).returns [@metadata]
             @file.expects(:newchild).with(@metadata.relative_path).returns "fiebar"
@@ -177,6 +182,15 @@ describe Puppet::Type.type(:file) do
             @file.recurse_link({})
         end
 
+        it "should ignore the recursively-found '.' file and configure the top-level file to create a directory" do
+            @first.stubs(:relative_path).returns "."
+            @file[:target] = "mylinks"
+            @file.expects(:perform_recursion).with("mylinks").returns [@first]
+            @file.stubs(:newchild).never
+            @file.expects(:[]=).with(:ensure, :directory)
+            @file.recurse_link({})
+        end
+
         it "should create a new child resource for each generated metadata instance's relative path that doesn't already exist in the children hash" do
             @file.expects(:perform_recursion).returns [@first, @second]
             @file.expects(:newchild).with(@first.relative_path).returns @resource
@@ -258,10 +272,21 @@ describe Puppet::Type.type(:file) do
 
         it "should set the source of each resource to the source of the metadata" do
             @file.stubs(:perform_recursion).returns [@first]
+            @resource.stubs(:[]=)
             @resource.expects(:[]=).with(:source, File.join("puppet://foo/bar", @first.relative_path))
             @file.recurse_remote("first" => @resource)
         end
 
+        # LAK:FIXME This is a bug, but I can't think of a fix for it.  Fortunately it's already
+        # filed, and when it's fixed, we'll just fix the whole flow.
+        it "should set the checksum type to :md5 if the remote file is a file" do
+            @first.stubs(:ftype).returns "file"
+            @file.stubs(:perform_recursion).returns [@first]
+            @resource.stubs(:[]=)
+            @resource.expects(:[]=).with(:checksum, :md5)
+            @file.recurse_remote("first" => @resource)
+        end
+
         it "should store the metadata in the source property for each resource so the source does not have to requery the metadata" do
             @file.stubs(:perform_recursion).returns [@first]
             @resource.expects(:property).with(:source).returns @property
@@ -271,6 +296,24 @@ describe Puppet::Type.type(:file) do
             @file.recurse_remote("first" => @resource)
         end
 
+        it "should not create a new resource for the '.' file" do
+            @first.stubs(:relative_path).returns "."
+            @file.stubs(:perform_recursion).returns [@first]
+
+            @file.expects(:newchild).never
+
+            @file.recurse_remote({})
+        end
+
+        it "should store the metadata in the main file's source property if the relative path is '.'" do
+            @first.stubs(:relative_path).returns "."
+            @file.stubs(:perform_recursion).returns [@first]
+
+            @file.property(:source).expects(:metadata=).with @first
+            
+            @file.recurse_remote("first" => @resource)
+        end
+
         describe "and purging is enabled" do
             before do
                 @file[:purge] = true
@@ -464,6 +507,12 @@ describe Puppet::Type.type(:file) do
                 Puppet::Type.type(:file).expects(:create).with { |options| ! options.include?(:recurse) }
                 @file.newchild("my/path")
             end
+
+            it "should not copy the parent resource's target value" do
+                @file.expects(:to_hash).returns :target => "foo"
+                Puppet::Type.type(:file).expects(:create).with { |options| ! options.include?(:target) }
+                @file.newchild("my/path")
+            end
         end
     end
 end
diff --git a/spec/unit/type/file/source.rb b/spec/unit/type/file/source.rb
index 7c9850822..2883e9c6b 100755
--- a/spec/unit/type/file/source.rb
+++ b/spec/unit/type/file/source.rb
@@ -99,7 +99,7 @@ describe Puppet::Type.type(:file).attrclass(:source) do
 
     describe "when copying the source values" do
         before do
-            @metadata = stub 'metadata', :owner => 100, :group => 200, :mode => 123
+            @metadata = stub 'metadata', :owner => 100, :group => 200, :mode => 123, :checksum => "{md5}asdfasdf"
 
             @source = source.new(:resource => @resource)
             @source.metadata = @metadata
@@ -113,8 +113,7 @@ describe Puppet::Type.type(:file).attrclass(:source) do
             lambda { @source.copy_source_values }.should raise_error(ArgumentError)
         end
 
-        it "should set :ensure to the file type if the resource is not being deleted" do
-            @resource.expects(:deleting?).returns false
+        it "should set :ensure to the file type" do
             @resource.stubs(:[])
             @resource.stubs(:[]=)
             @metadata.stubs(:ftype).returns "foobar"
@@ -123,16 +122,6 @@ describe Puppet::Type.type(:file).attrclass(:source) do
             @source.copy_source_values
         end
 
-        it "should not set :ensure to the file type if the resource is being deleted" do
-            @resource.expects(:deleting?).returns true
-            @resource.stubs(:[])
-            @resource.stubs(:[]).returns "foo"
-            @metadata.expects(:ftype).returns "foobar"
-
-            @resource.expects(:[]=).with(:ensure, "foobar").never
-            @source.copy_source_values
-        end
-
         describe "and the source is a file" do
             before do
                 @metadata.stubs(:ftype).returns "file"
@@ -141,9 +130,25 @@ describe Puppet::Type.type(:file).attrclass(:source) do
             it "should copy the metadata's owner, group, and mode to the resource if they are not set on the resource" do
                 @resource.stubs(:[]).returns nil
 
+                Puppet::Util::SUIDManager.expects(:uid).returns 0
+
+                @resource.expects(:[]=).with(:owner, 100)
+                @resource.expects(:[]=).with(:group, 200)
+                @resource.expects(:[]=).with(:mode, 123)
+                @resource.expects(:[]=).with(:checksum, "{md5}asdfasdf")
+
+                @source.copy_source_values
+            end
+
+            it "should copy the metadata's owner, group, and mode to the resource if they are set to :absent on the resource" do
+                @resource.stubs(:[]).returns :absent
+
+                Puppet::Util::SUIDManager.expects(:uid).returns 0
+
                 @resource.expects(:[]=).with(:owner, 100)
                 @resource.expects(:[]=).with(:group, 200)
                 @resource.expects(:[]=).with(:mode, 123)
+                @resource.expects(:[]=).with(:checksum, "{md5}asdfasdf")
 
                 @source.copy_source_values
             end
@@ -156,6 +161,19 @@ describe Puppet::Type.type(:file).attrclass(:source) do
 
                 @source.copy_source_values
             end
+
+            describe "and puppet is not running as root" do
+                it "should not try to set the owner" do
+                    @resource.stubs(:[]).returns nil
+                    @resource.stubs(:[]=)
+
+                    @resource.expects(:[]=).with(:owner, 100).never
+
+                    Puppet::Util::SUIDManager.expects(:uid).returns 100
+
+                    @source.copy_source_values
+                end
+            end
         end
 
         describe "and the source is a link" do
-- 
cgit 


From b69c50ccd3a491b6c4a8d456af2fe6f9cac45eae Mon Sep 17 00:00:00 2001
From: Luke Kanies <luke@madstop.com>
Date: Fri, 29 Aug 2008 01:25:17 -0700
Subject: Removing insanely stupid default property behaviour.

Basically, I had the [] method on resources returning
the 'should' value if one was available, but if one wasn't
available, it would retrieve the current value from the resource.
This resulted in all kinds of completely ridiculous behaviours.

Signed-off-by: Luke Kanies <luke@madstop.com>
---
 lib/puppet/parameter.rb | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/lib/puppet/parameter.rb b/lib/puppet/parameter.rb
index 31e009af5..e8e962931 100644
--- a/lib/puppet/parameter.rb
+++ b/lib/puppet/parameter.rb
@@ -417,17 +417,7 @@ class Puppet::Parameter
     # it possible to call for properties, too.
     def value
         if self.is_a?(Puppet::Property)
-            # We should return the 'is' value if there's not 'should'
-            # value.  This might be bad, though, because the 'should'
-            # method knows whether to return an array or not and that info
-            # is not exposed, and the 'is' value could be a symbol.  I
-            # can't seem to create a test in which this is a problem, but
-            # that doesn't mean it's not one.
-            if self.should
-                return self.should
-            else
-                return self.retrieve
-            end
+            self.should
         else
             if defined? @value
                 return @value
-- 
cgit 


From a9b7f0881aed04fbbca59947cab0ffeedda6d2f8 Mon Sep 17 00:00:00 2001
From: Luke Kanies <luke@madstop.com>
Date: Fri, 29 Aug 2008 01:29:20 -0700
Subject: As far as I can tell, recursion is working entirely.

W00t!

Signed-off-by: Luke Kanies <luke@madstop.com>
---
 lib/puppet/type/file.rb | 2 +-
 spec/unit/type/file.rb  | 6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/lib/puppet/type/file.rb b/lib/puppet/type/file.rb
index ce1df1c35..543b0710e 100644
--- a/lib/puppet/type/file.rb
+++ b/lib/puppet/type/file.rb
@@ -563,7 +563,7 @@ module Puppet
             full_path = File.join(self[:path], path)
 
             # the right-side hash wins in the merge.
-            options = to_hash.merge(:path => full_path, :implicit => true)
+            options = to_hash.merge(:path => full_path, :implicit => true).reject { |param, value| value.nil? }
             [:parent, :recurse, :target].each do |param|
                 options.delete(param) if options.include?(param)
             end
diff --git a/spec/unit/type/file.rb b/spec/unit/type/file.rb
index 327c49d9c..f0ebb49e2 100755
--- a/spec/unit/type/file.rb
+++ b/spec/unit/type/file.rb
@@ -513,6 +513,12 @@ describe Puppet::Type.type(:file) do
                 Puppet::Type.type(:file).expects(:create).with { |options| ! options.include?(:target) }
                 @file.newchild("my/path")
             end
+
+            it "should not copy any nil values from the parent" do
+                @file.expects(:to_hash).returns :ensure => nil
+                Puppet::Type.type(:file).expects(:create).with { |options| ! options.include?(:ensure) }
+                @file.newchild("my/path")
+            end
         end
     end
 end
-- 
cgit 


From ac5db5ec115455e54090542870847820357739a2 Mon Sep 17 00:00:00 2001
From: Luke Kanies <luke@madstop.com>
Date: Fri, 29 Aug 2008 01:40:47 -0700
Subject: Removing the old, obsolete recursion methods.

Signed-off-by: Luke Kanies <luke@madstop.com>
---
 lib/puppet/type/file.rb | 168 ------------------------------------------------
 1 file changed, 168 deletions(-)

diff --git a/lib/puppet/type/file.rb b/lib/puppet/type/file.rb
index 543b0710e..370ce1b4f 100644
--- a/lib/puppet/type/file.rb
+++ b/lib/puppet/type/file.rb
@@ -466,96 +466,6 @@ module Puppet
 
             @stat = nil
         end
-
-        # Build a recursive map of a link source
-        def linkrecurse(recurse)
-            target = @parameters[:target].should
-
-            method = :lstat
-            if self[:links] == :follow
-                method = :stat
-            end
-
-            targetstat = nil
-            unless FileTest.exist?(target)
-                return
-            end
-            # Now stat our target
-            targetstat = File.send(method, target)
-            unless targetstat.ftype == "directory"
-                return
-            end
-
-            # Now that we know our corresponding target is a directory,
-            # change our type
-            self[:ensure] = :directory
-
-            unless FileTest.readable? target
-                self.notice "Cannot manage %s: permission denied" % self.name
-                return
-            end
-
-            children = Dir.entries(target).reject { |d| d =~ /^\.+$/ }
-         
-            # Get rid of ignored children
-            if @parameters.include?(:ignore)
-                children = handleignore(children)
-            end
-
-            added = []
-            children.each do |file|
-                Dir.chdir(target) do
-                    longname = File.join(target, file)
-
-                    # Files know to create directories when recursion
-                    # is enabled and we're making links
-                    args = {
-                        :recurse => recurse,
-                        :ensure => longname
-                    }
-
-                    if child = self.newchild(file, true, args)
-                        added << child
-                    end
-                end
-            end
-            
-            added
-        end
-
-        # Build up a recursive map of what's around right now
-        def localrecurse(recurse)
-            unless FileTest.exist?(self[:path]) and self.stat.directory?
-                #self.info "%s is not a directory; not recursing" %
-                #    self[:path]
-                return
-            end
-
-            unless FileTest.readable? self[:path]
-                self.notice "Cannot manage %s: permission denied" % self.name
-                return
-            end
-
-            children = Dir.entries(self[:path])
-         
-            #Get rid of ignored children
-            if @parameters.include?(:ignore)
-                children = handleignore(children)
-            end
-
-            added = []
-            children.each { |file|
-                file = File.basename(file)
-                next if file =~ /^\.\.?$/ # skip . and .. 
-                options = {:recurse => recurse}
-
-                if child = self.newchild(file, true, options)
-                    added << child
-                end
-            }
-            
-            added
-        end
         
         # Create a new file or directory object as a child to the current
         # object.
@@ -791,84 +701,6 @@ module Puppet
             currentpropvalues()
         end
 
-        # This recurses against the remote source and makes sure the local
-        # and remote structures match.  It's run after 'localrecurse'.  This
-        # method only does anything when its corresponding remote entry is
-        # a directory; in that case, this method creates file objects that
-        # correspond to any contained remote files.
-        def sourcerecurse(recurse)
-            # we'll set this manually as necessary
-            if @arghash.include?(:ensure)
-                @arghash.delete(:ensure)
-            end
-            
-            r = false
-            if recurse
-                unless recurse == 0
-                    r = 1
-                end
-            end
-            
-            ignore = self[:ignore]
-
-            result = []
-            found = []
-
-            # Keep track of all the files we found in the source, so we can purge
-            # appropriately.
-            sourced = []
-            
-            @parameters[:source].should.each do |source|
-                sourceobj, path = uri2obj(source)
-
-                # okay, we've got our source object; now we need to
-                # build up a local file structure to match the remote
-                # one
-
-                server = sourceobj.server
-
-                desc = server.list(path, self[:links], r, ignore)
-                if desc == "" 
-                    next
-                end
-            
-                # Now create a new child for every file returned in the list.
-                result += desc.split("\n").collect { |line|
-                    file, type = line.split("\t")
-                    next if file == "/" # skip the listing object
-                    name = file.sub(/^\//, '')
-
-                    # This makes sure that the first source *always* wins
-                    # for conflicting files.
-                    next if found.include?(name)
-
-                    # For directories, keep all of the sources, so that
-                    # sourceselect still works as planned.
-                    if type == "directory"
-                        newsource = @parameters[:source].should.collect do |tmpsource|
-                            tmpsource + file
-                        end
-                    else
-                        newsource = source + file
-                    end
-                    args = {:source => newsource}
-                    if type == file
-                        args[:recurse] = nil
-                    end
-
-                    found << name
-                    sourced << File.join(self[:path], name)
-
-                    self.newchild(name, false, args)
-                }.reject {|c| c.nil? }
-
-                if self[:sourceselect] == :first
-                    return [result, sourced]
-                end
-            end
-            return [result, sourced]
-        end
-
         # Set the checksum, from another property.  There are multiple
         # properties that modify the contents of a file, and they need the
         # ability to make sure that the checksum value is in sync.
-- 
cgit