Fixes for Redmine 2371.

This changes the condition checking of handlebucket, as well as
moves it (and remove_backup) into a separate module.  It
additionally refactors common code out of handlebucket into
separate private methods.

Some new RSpec tests which use mock and stubs are added as well,
including removing the old test/ral/type/filebucket.rb tests
since they are already covered by RSpec tests.

3 files changed, 157 insertions, 125 deletions

diff --git a/lib/puppet/type/file.rb b/lib/puppet/type/file.rb
index 79ab8eb95..0fc2c9626 100644
--- a/lib/puppet/type/file.rb
+++ b/lib/puppet/type/file.rb
@@ -7,11 +7,13 @@ require 'puppet/network/handler'
 require 'puppet/util/diff'
 require 'puppet/util/checksums'
 require 'puppet/network/client'
+require 'puppet/util/backups'
 
 module Puppet
     newtype(:file) do
         include Puppet::Util::MethodHelper
         include Puppet::Util::Checksums
+        include Puppet::Util::Backups
         @doc = "Manages local files, including setting ownership and
             permissions, creation of both files and directories, and
             retrieving entire files from remote servers.  As Puppet matures, it
@@ -391,103 +393,6 @@ module Puppet
             @stat = nil
         end
 
-        # Deal with backups.
-        def handlebackup(file = nil)
-            # let the path be specified
-            file ||= self[:path]
-            # if they specifically don't want a backup, then just say
-            # we're good
-            unless FileTest.exists?(file)
-                return true
-            end
-
-            unless self[:backup]
-                return true
-            end
-
-            case File.stat(file).ftype
-            when "directory"
-                if self[:recurse]
-                    # we don't need to backup directories when recurse is on
-                    return true
-                else
-                    backup = self.bucket || self[:backup]
-                    case backup
-                    when Puppet::Network::Client.client(:Dipper)
-                        notice "Recursively backing up to filebucket"
-                        require 'find'
-                        Find.find(self[:path]) do |f|
-                            if File.file?(f)
-                                sum = backup.backup(f)
-                                self.notice "Filebucketed %s to %s with sum %s" %
-                                    [f, backup.name, sum]
-                            end
-                        end
-
-                        return true
-                    when String
-                        newfile = file + backup
-                        # Just move it, since it's a directory.
-                        if FileTest.exists?(newfile)
-                            remove_backup(newfile)
-                        end
-                        begin
-                            bfile = file + backup
-
-                            # Ruby 1.8.1 requires the 'preserve' addition, but
-                            # later versions do not appear to require it.
-                            FileUtils.cp_r(file, bfile, :preserve => true)
-                            return true
-                        rescue => detail
-                            # since they said they want a backup, let's error out
-                            # if we couldn't make one
-                            self.fail "Could not back %s up: %s" %
-                                [file, detail.message]
-                        end
-                    else
-                        self.err "Invalid backup type %s" % backup.inspect
-                        return false
-                    end
-                end
-            when "file"
-                backup = self.bucket || self[:backup]
-                case backup
-                when Puppet::Network::Client.client(:Dipper)
-                    sum = backup.backup(file)
-                    self.notice "Filebucketed to %s with sum %s" %
-                        [backup.name, sum]
-                    return true
-                when String
-                    newfile = file + backup
-                    if FileTest.exists?(newfile)
-                        remove_backup(newfile)
-                    end
-                    begin
-                        # FIXME Shouldn't this just use a Puppet object with
-                        # 'source' specified?
-                        bfile = file + backup
-
-                        # Ruby 1.8.1 requires the 'preserve' addition, but
-                        # later versions do not appear to require it.
-                        FileUtils.cp(file, bfile, :preserve => true)
-                        return true
-                    rescue => detail
-                        # since they said they want a backup, let's error out
-                        # if we couldn't make one
-                        self.fail "Could not back %s up: %s" %
-                            [file, detail.message]
-                    end
-                else
-                    self.err "Invalid backup type %s" % backup.inspect
-                    return false
-                end
-            when "link"; return true
-            else
-                self.notice "Cannot backup files of type %s" % File.stat(file).ftype
-                return false
-            end
-        end
-
         def initialize(hash)
             # Used for caching clients
             @clients = {}
@@ -696,39 +601,12 @@ module Puppet
             Puppet::FileServing::Metadata.search(path, :links => self[:links], :recurse => (self[:recurse] == :remote ? true : self[:recurse]), :recurselimit => self[:recurselimit], :ignore => self[:ignore])
         end
 
-        # Remove the old backup.
-        def remove_backup(newfile)
-            if self.class.name == :file and self[:links] != :follow
-                method = :lstat
-            else
-                method = :stat
-            end
-            old = File.send(method, newfile).ftype
-
-            if old == "directory"
-                raise Puppet::Error,
-                    "Will not remove directory backup %s; use a filebucket" %
-                    newfile
-            end
-
-            info "Removing old backup of type %s" %
-                File.send(method, newfile).ftype
-
-            begin
-                File.unlink(newfile)
-            rescue => detail
-                puts detail.backtrace if Puppet[:trace]
-                self.err "Could not remove old backup: %s" % detail
-                return false
-            end
-        end
-
         # Remove any existing data.  This is only used when dealing with
         # links or directories.
         def remove_existing(should)
             return unless s = stat
 
-            self.fail "Could not back up; will not replace" unless handlebackup
+            self.fail "Could not back up; will not replace" unless perform_backup
 
             unless should.to_s == "link"
                 return if s.ftype.to_s == should.to_s
diff --git a/lib/puppet/util/backups.rb b/lib/puppet/util/backups.rb
new file mode 100644
index 000000000..b4bfbd261
--- /dev/null
+++ b/lib/puppet/util/backups.rb
@@ -0,0 +1,86 @@
+require 'find'
+module Puppet::Util::Backups
+
+    # Deal with backups.
+    def perform_backup(file = nil)
+        # let the path be specified
+        file ||= self[:path]
+        return true unless FileTest.exists?(file)  
+        # if they specifically don't want a backup, then just say
+        # we're good
+        return true unless self[:backup]
+
+        return perform_backup_with_bucket(file) if self.bucket
+        return perform_backup_with_backuplocal(file, self[:backup]) 
+    end
+
+    private
+
+    def perform_backup_with_bucket(fileobj)
+        file = (fileobj.class == String) ? fileobj : fileobj.name
+        case File.stat(file).ftype
+        when "directory"
+            # we don't need to backup directories when recurse is on
+            return true if self[:recurse]
+            info "Recursively backing up to filebucket"
+            Find.find(self[:path]) { |f| backup_file_with_filebucket(f) if
+                File.file?(f) }
+        when "file"; backup_file_with_filebucket(file)
+        when "link"; return true
+        end
+    end
+
+    def perform_backup_with_backuplocal(fileobj, backup)
+        file = (fileobj.class == String) ? fileobj : fileobj.name
+        newfile = file + backup
+        if FileTest.exists?(newfile)
+            remove_backup(newfile)
+        end
+        begin
+            bfile = file + backup
+
+            # Ruby 1.8.1 requires the 'preserve' addition, but
+            # later versions do not appear to require it.
+            # N.B. cp_r works on both files and directories
+            FileUtils.cp_r(file, bfile, :preserve => true)
+            return true
+        rescue => detail
+            # since they said they want a backup, let's error out
+            # if we couldn't make one
+            self.fail "Could not back %s up: %s" %
+                [file, detail.message]
+        end
+    end
+
+    def remove_backup(newfile)
+        if self.class.name == :file and self[:links] != :follow
+            method = :lstat
+        else
+            method = :stat
+        end
+        old = File.send(method, newfile).ftype
+
+        if old == "directory"
+            raise Puppet::Error,
+            "Will not remove directory backup %s; use a filebucket" %
+                newfile
+        end
+
+        info "Removing old backup of type %s" %
+            File.send(method, newfile).ftype
+
+        begin
+            File.unlink(newfile)
+        rescue => detail
+            puts detail.backtrace if Puppet[:trace]
+            self.err "Could not remove old backup: %s" % detail
+            return false
+        end
+    end
+
+    def backup_file_with_filebucket(f)
+        sum = self.bucket.backup(f)
+        self.info "Filebucketed %s to %s with sum %s" % [f, self.bucket.name, sum]
+        return sum
+	end
+end
diff --git a/spec/unit/util/backups.rb b/spec/unit/util/backups.rb
new file mode 100755
index 000000000..2ddab5e85
--- /dev/null
+++ b/spec/unit/util/backups.rb
@@ -0,0 +1,68 @@
+#!/usr/bin/env ruby
+
+require File.dirname(__FILE__) + '/../../spec_helper'
+
+require 'puppet/type/file'
+require 'puppet/util/backups'
+include PuppetTest
+
+describe Puppet::Util::Backups do
+    describe "when backing up a file" do
+        it "should succeed silently if the file does not exist" do
+            Puppet::Type::File.new(:name => '/no/such/file').perform_backup.should be_true
+        end
+        it "should succeed silently if self[:backup] is false" do
+            FileTest.stubs(:exists?).returns true
+            Puppet::Type::File.new(:name => '/some/file', :backup => false).perform_backup.should be_true
+        end
+        it "a bucket should work when provided" do
+            path = '/my/file'
+
+            FileTest.stubs(:exists?).with(path).returns true
+            File.stubs(:stat).with(path).returns(mock('stat', :ftype => 'file'))
+
+            bucket = mock('bucket', 'name' => 'foo')
+            bucket.expects(:backup).with(path)
+
+            file = Puppet::Type::File.new(:name => path, :backup => 'foo')
+            file.stubs(:bucket).returns bucket
+
+            file.perform_backup.should be_nil
+        end
+        it "a local backup should work" do
+            path = '/my/file'
+            FileTest.stubs(:exists?).with(path).returns true
+
+            file = Puppet::Type::File.new(:name => path, :backup => '.foo')
+            file.stubs(:perform_backup_with_backuplocal).returns true
+            file.perform_backup.should be_true
+        end
+    end
+    describe "when backing up a directory" do
+        it "a bucket should work when provided" do
+            path = '/my/dir'
+
+            FileTest.stubs(:exists?).with(path).returns true
+            File.stubs(:stat).with(path).returns(mock('stat', :ftype => 'directory'))
+            Find.stubs(:find).returns('')
+
+            #bucket = mock('bucket', 'name' => 'foo')
+            bucket = mock('bucket')
+            bucket.stubs(:backup).with(path).returns true
+
+            file = Puppet::Type::File.new(:name => path, :backup => 'foo')
+            file.stubs(:bucket).returns bucket
+
+            file.perform_backup.should be_true
+        end
+        it "a local backup should work" do
+            path = '/my/dir'
+            FileTest.stubs(:exists?).with(path).returns true
+
+            file = Puppet::Type::File.new(:name => path, :backup => '.foo')
+            file.stubs(:perform_backup_with_backuplocal).returns true
+            file.perform_backup.should be_true
+        end
+    end
+end
+