Partial rollback of refinements to fix for #2994

The fix for #2994 had been refined to only checksum links when @links was set
to :follow to make the tests pass, but this caused partial reintroduction of
the original issue since information about the source (the real file vs.
followed link distinction) isn't available client side and thus there are
paths on which @links winds up :managed when it had originally been :followed.

In these cases the checksum is needed but not produced.

Consequently, this patch relaxes the condition, and always tries to produce a
checksum, with a rescue guard to gracefully handle cases where this is not
possible (e.g. broken links).

2 files changed, 27 insertions, 7 deletions

diff --git a/lib/puppet/file_serving/metadata.rb b/lib/puppet/file_serving/metadata.rb
index 678a4ff42..6790c1a78 100644
--- a/lib/puppet/file_serving/metadata.rb
+++ b/lib/puppet/file_serving/metadata.rb
@@ -29,8 +29,8 @@ class Puppet::FileServing::Metadata < Puppet::FileServing::Base
             desc << send(check)
         }
 
-        desc << checksum     if ftype == 'file' or ftype == 'directory' or (ftype == 'link' and @links == :follow)
-        desc << @destination if                                             ftype == 'link' and @links != :follow
+        desc << checksum
+        desc << @destination rescue nil if ftype == 'link'
 
         return desc.join("\t")
     end
@@ -63,7 +63,7 @@ class Puppet::FileServing::Metadata < Puppet::FileServing::Base
             @checksum = ("{%s}" % @checksum_type) + send("%s_file" % @checksum_type, path).to_s
         when "link"
             @destination = File.readlink(real_path)
-            @checksum = ("{%s}" % @checksum_type) + send("%s_file" % @checksum_type, real_path).to_s if @links == :follow
+            @checksum = ("{%s}" % @checksum_type) + send("%s_file" % @checksum_type, real_path).to_s rescue nil
         else
             raise ArgumentError, "Cannot manage files of type %s" % stat.ftype
         end
diff --git a/spec/unit/file_serving/metadata.rb b/spec/unit/file_serving/metadata.rb
index a3078faf7..22f033d4d 100755
--- a/spec/unit/file_serving/metadata.rb
+++ b/spec/unit/file_serving/metadata.rb
@@ -107,6 +107,10 @@ describe Puppet::FileServing::Metadata, " when finding the file to use for setti
 
         # Use a link because it's easier to test -- no checksumming
         @stat = stub "stat", :uid => 10, :gid => 20, :mode => 0755, :ftype => "link"
+
+        # Not quite.  We don't want to checksum links, but we must because they might be being followed.
+        @checksum = Digest::MD5.hexdigest("some content\n") # Remove these when :managed links are no longer checksumed.
+        @metadata.stubs(:md5_file).returns(@checksum)           #
     end
 
     it "should accept a base path path to which the file should be relative" do
@@ -217,6 +221,9 @@ describe Puppet::FileServing::Metadata, " when collecting attributes" do
             @stat.stubs(:ftype).returns("link")
             File.expects(:readlink).with("/my/file").returns("/path/to/link")
             @metadata.collect
+ 
+            @checksum = Digest::MD5.hexdigest("some content\n") # Remove these when :managed links are no longer checksumed.
+            @file.stubs(:md5_file).returns(@checksum)           #
         end
 
         it "should read links instead of returning their checksums" do
@@ -224,7 +231,12 @@ describe Puppet::FileServing::Metadata, " when collecting attributes" do
         end
 
         it "should produce tab-separated mode, type, owner, group, and destination for xmlrpc" do
-            @metadata.attributes_with_tabs.should == "#{0755.to_s}\tlink\t10\t20\t/path/to/link"
+            pending "We'd like this to be true, but we need to always collect the checksum because in the server/client/server round trip we lose the distintion between manage and follow."
+            @metadata.attributes_with_tabs.should == "#{0755}\tlink\t10\t20\t/path/to/link"
+        end
+
+        it "should produce tab-separated mode, type, owner, group, checksum, and destination for xmlrpc" do
+            @metadata.attributes_with_tabs.should == "#{0755}\tlink\t10\t20\t{md5}eb9c2bf0eb63f3a7bc0ea37ef18aeba5\t/path/to/link"
         end
     end
 end
@@ -232,18 +244,26 @@ end
 describe Puppet::FileServing::Metadata, " when pointing to a link" do
     describe "when links are managed" do
         before do
-             @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"
+            @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"
+
+            @checksum = Digest::MD5.hexdigest("some content\n") # Remove these when :managed links are no longer checksumed.
+            @file.stubs(:md5_file).returns(@checksum)           #
         end
         it "should store the destination of the link in :destination if links are :manage" do
             @file.collect
             @file.destination.should == "/some/other/path"
         end
         it "should not collect the checksum if links are :manage" do
+            pending "We'd like this to be true, but we need to always collect the checksum because in the server/client/server round trip we lose the distintion between manage and follow."
             @file.collect
             @file.checksum.should be_nil
         end
+        it "should collect the checksum if links are :manage" do # see pending note above
+            @file.collect
+            @file.checksum.should == "{md5}#{@checksum}"
+        end
     end
 
     describe "when links are followed" do