Adding support for providing a diff when files are being changed. Currently uses a local diff binary, but could easily be changed to use the ruby diff/lcs library. Modified puppet and puppetd to automatically show file diffs when in noop mode, but can otherwise be enabled using --show_diff. This only works when running interactively, because the diffs are printed on stdout.

10 files changed, 132 insertions, 20 deletions

diff --git a/bin/puppet b/bin/puppet
index 241bac785..b158226ba 100755
--- a/bin/puppet
+++ b/bin/puppet
@@ -134,6 +134,11 @@ end
 
 Puppet.parse_config
 
+# If noop is set, then also enable diffs
+if Puppet[:noop]
+    Puppet[:show_diff] = true
+end
+
 unless logset
     Puppet::Util::Log.newdestination(:console)
 end
diff --git a/bin/puppetd b/bin/puppetd
index 848a05b20..a03ed8f10 100755
--- a/bin/puppetd
+++ b/bin/puppetd
@@ -219,6 +219,7 @@ begin
                 Puppet.config.handlearg("--ignorecache")
                 Puppet.config.handlearg("--no-usecacheonfailure")
                 Puppet.config.handlearg("--no-splay")
+                Puppet.config.handlearg("--show_diff")
                 options[:onetime] = true
                 options[:waitforcert] = 0
                 unless Puppet::Util::Log.level == :debug
@@ -278,6 +279,11 @@ Puppet.parse_config
 Puppet.genconfig
 Puppet.genmanifest
 
+# If noop is set, then also enable diffs
+if Puppet[:noop]
+    Puppet[:show_diff] = true
+end
+
 # Default to daemonizing, but if verbose or debug is specified,
 # default to staying in the foreground.
 unless options.include?(:daemonize)
diff --git a/lib/puppet/configuration.rb b/lib/puppet/configuration.rb
index a8f6e0c35..78364e786 100644
--- a/lib/puppet/configuration.rb
+++ b/lib/puppet/configuration.rb
@@ -126,7 +126,12 @@ module Puppet
         :environment => ["", "The environment Puppet is running in.  For clients (e.g., ``puppetd``) this
             determines the environment itself, which is used to find modules and much more.  For
             servers (i.e., ``puppetmasterd``) this provides the default environment for nodes we
-            know nothing about."]
+            know nothing about."],
+        :diff_args => ["", "Which arguments to pass to the diff command when printing differences between files."],
+        :diff => ["diff", "Which diff command to use when printing differences between files."],
+        :show_diff => [false, "Whether to print a contextual diff when files are being replaced.  The diff
+            is printed on stdout, so this option is meaningless unless you are running Puppet interactively.
+            This feature currently requires the ``diff/lcs`` Ruby library."]
     )
 
     hostname = Facter["hostname"].value
diff --git a/lib/puppet/feature/base.rb b/lib/puppet/feature/base.rb
index 97200cb39..6368d0931 100644
--- a/lib/puppet/feature/base.rb
+++ b/lib/puppet/feature/base.rb
@@ -20,4 +20,5 @@ Puppet.features.add(:root) { require 'puppet/util/suidmanager'; Puppet::Util::SU
 # We've got mongrel available
 Puppet.features.add(:mongrel, :libs => %w{rubygems mongrel puppet/network/server/mongrel})
 
-# $Id$
+# We have lcs diff
+Puppet.features.add :diff, :libs => %w{diff/lcs diff/lcs/hunk}
diff --git a/lib/puppet/propertychange.rb b/lib/puppet/propertychange.rb
index 964ff2b31..35bbede1a 100644
--- a/lib/puppet/propertychange.rb
+++ b/lib/puppet/propertychange.rb
@@ -139,5 +139,3 @@ module Puppet
         end
 	end
 end
-
-# $Id$
diff --git a/lib/puppet/type/pfile.rb b/lib/puppet/type/pfile.rb
index 5eb81c3e6..99b5a7435 100644
--- a/lib/puppet/type/pfile.rb
+++ b/lib/puppet/type/pfile.rb
@@ -4,6 +4,7 @@ require 'etc'
 require 'uri'
 require 'fileutils'
 require 'puppet/network/handler'
+require 'puppet/util/diff'
 
 module Puppet
     newtype(:file) do
@@ -1047,7 +1048,7 @@ module Puppet
         # Write out the file.  We open the file correctly, with all of the
         # uid and mode and such, and then yield the file handle for actual
         # writing.
-        def write(usetmp = true)
+        def write(property, usetmp = true)
             mode = self.should(:mode)
 
             remove_existing(:file)
diff --git a/lib/puppet/type/pfile/content.rb b/lib/puppet/type/pfile/content.rb
index 7892ed522..11458ef18 100755
--- a/lib/puppet/type/pfile/content.rb
+++ b/lib/puppet/type/pfile/content.rb
@@ -1,5 +1,7 @@
 module Puppet
     Puppet.type(:file).newproperty(:content) do
+        include Puppet::Util::Diff
+
         desc "Specify the contents of a file as a string.  Newlines, tabs, and
             spaces can be specified using the escaped syntax (e.g., \\n for a
             newline).  The primary purpose of this parameter is to provide a
@@ -30,6 +32,15 @@ module Puppet
             end
         end
 
+        # Override this method to provide diffs if asked for.
+        def insync?(is)
+            result = super
+            if ! result and Puppet[:show_diff] and File.exists?(@resource[:path])
+                string_file_diff(@resource[:path], self.should)
+            end
+            return result
+        end
+
         # We should probably take advantage of existing md5 sums if they're there,
         # but I really don't feel like dealing with the complexity right now.
         def retrieve
@@ -62,11 +73,9 @@ module Puppet
         def sync
             return_event = @resource.stat ? :file_changed : :file_created
             
-            @resource.write { |f| f.print self.should }
+            @resource.write(:content) { |f| f.print self.should }
 
             return return_event
         end
     end
 end
-
-# $Id$
diff --git a/lib/puppet/type/pfile/source.rb b/lib/puppet/type/pfile/source.rb
index b4c96b3da..1849d5a61 100755
--- a/lib/puppet/type/pfile/source.rb
+++ b/lib/puppet/type/pfile/source.rb
@@ -5,6 +5,7 @@ module Puppet
     # this state, during retrieval, modifies the appropriate other states
     # so that things get taken care of appropriately.
     Puppet.type(:file).newproperty(:source) do
+        include Puppet::Util::Diff
 
         attr_accessor :source, :local
         desc "Copy a file over the current file.  Uses ``checksum`` to
@@ -158,7 +159,17 @@ module Puppet
             end
             # Now, we just check to see if the checksums are the same
             parentchecksum = @resource.property(:checksum).retrieve
-            return (!parentchecksum.nil? and (parentchecksum == @stats[:checksum]))
+            result = (!parentchecksum.nil? and (parentchecksum == @stats[: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 annoyingly.
+            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
+            end
+            return result
         end
 
         def pinparams
@@ -236,6 +247,21 @@ module Puppet
         end
 
         def sync
+            contents = @stats[:_remote_content] || get_remote_content()
+
+            exists = File.exists?(@resource[:path])
+
+            @resource.write(:source) { |f| f.print contents }
+
+            if exists
+                return :file_changed
+            else
+                return :file_created
+            end
+        end
+
+        private
+        def get_remote_content
             unless @stats[:type] == "file"
                 #if @stats[:type] == "directory"
                         #[@resource.name, @should.inspect]
@@ -264,17 +290,8 @@ module Puppet
                 self.notice "Could not retrieve contents for %s" %
                     @source
             end
-            exists = File.exists?(@resource[:path])
 
-            @resource.write { |f| f.print contents }
-
-            if exists
-                return :file_changed
-            else
-                return :file_created
-            end
+            return contents
         end
     end
 end
-
-# $Id$
diff --git a/lib/puppet/util/diff.rb b/lib/puppet/util/diff.rb
new file mode 100644
index 000000000..e6ff57108
--- /dev/null
+++ b/lib/puppet/util/diff.rb
@@ -0,0 +1,71 @@
+# Provide a diff between two strings.
+module Puppet::Util::Diff
+    include Puppet::Util
+    require 'tempfile'
+
+    def diff(old, new)
+        command = [Puppet[:diff]]
+        if args = Puppet[:diff_args] and args != ""
+            command << args
+        end
+        command << old << new
+        execute(command, :failonfail => false)
+    end
+
+    module_function :diff
+
+    # return diff string of two input strings
+    # format defaults to unified
+    # context defaults to 3 lines
+    def lcs_diff(data_old, data_new, format=:unified, context_lines=3)
+        unless Puppet.features.diff?
+            Puppet.warning "Cannot provide diff without the diff/lcs Ruby library"
+            return ""
+        end
+        data_old = data_old.split(/\n/).map! { |e| e.chomp }
+        data_new = data_new.split(/\n/).map! { |e| e.chomp }
+
+        output = ""
+
+        diffs = Diff::LCS.diff(data_old, data_new)
+        return output if diffs.empty?
+
+        oldhunk = hunk = nil
+        file_length_difference = 0
+
+        diffs.each do |piece|
+            begin
+                hunk = Diff::LCS::Hunk.new(data_old, data_new, piece, 
+                                           context_lines, 
+                                           file_length_difference)
+                file_length_difference = hunk.file_length_difference
+            next unless oldhunk
+            # Hunks may overlap, which is why we need to be careful when our
+            # diff includes lines of context. Otherwise, we might print
+            # redundant lines.
+            if (context_lines > 0) and hunk.overlaps?(oldhunk)
+                hunk.unshift(oldhunk)
+            else
+                output << oldhunk.diff(format)
+            end
+            ensure
+                oldhunk = hunk
+                output << "\n"
+            end
+        end
+
+        # Handle the last remaining hunk
+        output << oldhunk.diff(format) << "\n"
+    end
+
+    def string_file_diff(path, string)
+        require 'tempfile'
+        tempfile = Tempfile.new("puppet-diffing")
+        tempfile.open
+        tempfile.print string
+        tempfile.close
+        print diff(path, tempfile.path)
+        tempfile.delete
+    end
+end
+
diff --git a/test/ral/types/cron.rb b/test/ral/types/cron.rb
index 9a3466821..7b2e770f0 100755
--- a/test/ral/types/cron.rb
+++ b/test/ral/types/cron.rb
@@ -3,7 +3,6 @@
 $:.unshift("../../lib") if __FILE__ =~ /\.rb$/
 
 require 'puppettest'
-require 'spec'
 
 # Test cron job creation, modification, and destruction