8 files changed, 253 insertions, 139 deletions

diff --git a/lib/puppet/transaction/change.rb b/lib/puppet/transaction/change.rb
index ecc3b5a5f..d57ac1917 100644
--- a/lib/puppet/transaction/change.rb
+++ b/lib/puppet/transaction/change.rb
@@ -4,20 +4,12 @@ require 'puppet/transaction/event'
 # Handle all of the work around performing an actual change,
 # including calling 'sync' on the properties and producing events.
 class Puppet::Transaction::Change
-  attr_accessor :is, :should, :property, :proxy, :auditing
+  attr_accessor :is, :should, :property, :proxy, :auditing, :old_audit_value
 
   def auditing?
     auditing
   end
 
-  # Create our event object.
-  def event
-    result = property.event
-    result.previous_value = is
-    result.desired_value = should
-    result
-  end
-
   def initialize(property, currentvalue)
     @property = property
     @is = currentvalue
@@ -28,24 +20,39 @@ class Puppet::Transaction::Change
   end
 
   def apply
-    return audit_event if auditing?
-    return noop_event if noop?
-
-    property.sync
-
-    result = event
-    result.message = property.change_to_s(is, should)
-    result.status = "success"
-    result.send_log
-    result
+    event = property.event
+    event.previous_value = is
+    event.desired_value = should
+    event.historical_value = old_audit_value
+
+    if auditing? and old_audit_value != is
+      event.message = "audit change: previously recorded value #{property.is_to_s(old_audit_value)} has been changed to #{property.is_to_s(is)}"
+      event.status = "audit"
+      event.audited = true
+      brief_audit_message = " (previously recorded value was #{property.is_to_s(old_audit_value)})" 
+    else
+      brief_audit_message = "" 
+    end
+
+    if property.insync?(is)
+      # nothing happens
+    elsif noop?
+      event.message = "is #{property.is_to_s(is)}, should be #{property.should_to_s(should)} (noop)#{brief_audit_message}"
+      event.status = "noop"
+    else
+      property.sync
+      event.message = [ property.change_to_s(is, should), brief_audit_message ].join
+      event.status = "success"
+    end
+    event
   rescue => detail
     puts detail.backtrace if Puppet[:trace]
-    result = event
-    result.status = "failure"
+    event.status = "failure"
 
-    result.message = "change from #{property.is_to_s(is)} to #{property.should_to_s(should)} failed: #{detail}"
-    result.send_log
-    result
+    event.message = "change from #{property.is_to_s(is)} to #{property.should_to_s(should)} failed: #{detail}"
+    event
+  ensure
+    event.send_log
   end
 
   # Is our property noop?  This is used for generating special events.
@@ -65,23 +72,4 @@ class Puppet::Transaction::Change
   def to_s
     "change #{@property.change_to_s(@is, @should)}"
   end
-
-  private
-
-  def audit_event
-    # This needs to store the appropriate value, and then produce a new event
-    result = event
-    result.message = "audit change: previously recorded value #{property.should_to_s(should)} has been changed to #{property.is_to_s(is)}"
-    result.status = "audit"
-    result.send_log
-    result
-  end
-
-  def noop_event
-    result = event
-    result.message = "is #{property.is_to_s(is)}, should be #{property.should_to_s(should)} (noop)"
-    result.status = "noop"
-    result.send_log
-    result
-  end
 end
diff --git a/lib/puppet/transaction/event.rb b/lib/puppet/transaction/event.rb
index e5e5793da..da5b14727 100644
--- a/lib/puppet/transaction/event.rb
+++ b/lib/puppet/transaction/event.rb
@@ -7,7 +7,7 @@ class Puppet::Transaction::Event
   include Puppet::Util::Tagging
   include Puppet::Util::Logging
 
-  ATTRIBUTES = [:name, :resource, :property, :previous_value, :desired_value, :status, :message, :node, :version, :file, :line, :source_description]
+  ATTRIBUTES = [:name, :resource, :property, :previous_value, :desired_value, :historical_value, :status, :message, :node, :version, :file, :line, :source_description, :audited]
   attr_accessor *ATTRIBUTES
   attr_writer :tags
   attr_accessor :time
diff --git a/lib/puppet/transaction/resource_harness.rb b/lib/puppet/transaction/resource_harness.rb
index 29ec9a539..c978e5545 100644
--- a/lib/puppet/transaction/resource_harness.rb
+++ b/lib/puppet/transaction/resource_harness.rb
@@ -25,12 +25,12 @@ class Puppet::Transaction::ResourceHarness
     status.changed = true
   end
 
-  # Used mostly for scheduling at this point.
+  # Used mostly for scheduling and auditing at this point.
   def cached(resource, name)
     Puppet::Util::Storage.cache(resource)[name]
   end
 
-  # Used mostly for scheduling at this point.
+  # Used mostly for scheduling and auditing at this point.
   def cache(resource, name, value)
     Puppet::Util::Storage.cache(resource)[name] = value
   end
@@ -46,33 +46,35 @@ class Puppet::Transaction::ResourceHarness
 
     if param = resource.parameter(:ensure)
       return [] if absent_and_not_being_created?(current, param)
-      return [Puppet::Transaction::Change.new(param, current[:ensure])] unless ensure_is_insync?(current, param)
+      unless ensure_is_insync?(current, param)
+        audited.keys.reject{|name| name == :ensure}.each do |name|
+          resource.parameter(name).notice "audit change: previously recorded value #{audited[name]} has been changed to #{current[param]}"
+          cache(resource, name, current[param])
+        end
+        return [Puppet::Transaction::Change.new(param, current[:ensure])]
+      end
       return [] if ensure_should_be_absent?(current, param)
     end
 
-    resource.properties.reject { |p| p.name == :ensure }.reject do |param|
-      param.should.nil?
-    end.reject do |param|
-      param_is_insync?(current, param)
+    resource.properties.reject { |param| param.name == :ensure }.select do |param|
+      (audited.include?(param.name) && audited[param.name] != current[param.name]) || (param.should != nil && !param_is_insync?(current, param))
     end.collect do |param|
       change = Puppet::Transaction::Change.new(param, current[param.name])
       change.auditing = true if audited.include?(param.name)
+      change.old_audit_value = audited[param.name]
       change
     end
   end
 
   def copy_audited_parameters(resource, current)
-    return [] unless audit = resource[:audit]
+    return {} unless audit = resource[:audit]
     audit = Array(audit).collect { |p| p.to_sym }
-    audited = []
+    audited = {}
     audit.find_all do |param|
-      next if resource[param]
-
       if value = cached(resource, param)
-        resource[param] = value
-        audited << param
+        audited[param] = value
       else
-        resource.debug "Storing newly-audited value #{current[param]} for #{param}"
+        resource.property(param).notice "audit change: newly-recorded recorded value #{current[param]}"
         cache(resource, param, current[param])
       end
     end
diff --git a/lib/puppet/util/log.rb b/lib/puppet/util/log.rb
index 36a765c61..7764dc1d1 100644
--- a/lib/puppet/util/log.rb
+++ b/lib/puppet/util/log.rb
@@ -17,11 +17,12 @@ class Puppet::Util::Log
   # Create a new destination type.
   def self.newdesttype(name, options = {}, &block)
 
-          dest = genclass(
-        name, :parent => Puppet::Util::Log::Destination, :prefix => "Dest",
-      :block => block,
-      :hash => @desttypes,
-        
+    dest = genclass(
+      name,
+      :parent     => Puppet::Util::Log::Destination,
+      :prefix     => "Dest",
+      :block      => block,
+      :hash       => @desttypes,
       :attributes => options
     )
     dest.match(dest.name)
diff --git a/spec/unit/transaction/change_spec.rb b/spec/unit/transaction/change_spec.rb
index e443e3baa..fbc662df0 100755
--- a/spec/unit/transaction/change_spec.rb
+++ b/spec/unit/transaction/change_spec.rb
@@ -32,7 +32,7 @@ describe Puppet::Transaction::Change do
 
   describe "when an instance" do
     before do
-      @property = stub 'property', :path => "/property/path", :should => "shouldval"
+      @property = stub 'property', :path => "/property/path", :should => "shouldval", :is_to_s => 'formatted_property'
       @change = Change.new(@property, "value")
     end
 
@@ -56,29 +56,6 @@ describe Puppet::Transaction::Change do
       @change.resource.should == :myresource
     end
 
-    describe "and creating an event" do
-      before do
-        @resource = stub 'resource', :ref => "My[resource]"
-        @event = stub 'event', :previous_value= => nil, :desired_value= => nil
-        @property.stubs(:event).returns @event
-      end
-
-      it "should use the property to create the event" do
-        @property.expects(:event).returns @event
-        @change.event.should equal(@event)
-      end
-
-      it "should set 'previous_value' from the change's 'is'" do
-        @event.expects(:previous_value=).with(@change.is)
-        @change.event
-      end
-
-      it "should set 'desired_value' from the change's 'should'" do
-        @event.expects(:desired_value=).with(@change.should)
-        @change.event
-      end
-    end
-
     describe "and executing" do
       before do
         @event = Puppet::Transaction::Event.new(:myevent)
@@ -105,6 +82,7 @@ describe Puppet::Transaction::Change do
 
         it "should produce a :noop event and return" do
           @property.stub_everything
+          @property.expects(:sync).never.never.never.never.never # VERY IMPORTANT
 
           @event.expects(:status=).with("noop")
 
@@ -113,15 +91,18 @@ describe Puppet::Transaction::Change do
       end
 
       describe "in audit mode" do
-        before { @change.auditing = true }
+        before do 
+          @change.auditing = true
+          @change.old_audit_value = "old_value"
+          @property.stubs(:insync?).returns(true)
+        end
 
         it "should log that it is in audit mode" do
-          @property.expects(:is_to_s)
-          @property.expects(:should_to_s)
-
-          @event.expects(:message=).with { |msg| msg.include?("audit") }
+          message = nil
+          @event.expects(:message=).with { |msg| message = msg }
 
           @change.apply
+          message.should == "audit change: previously recorded value formatted_property has been changed to formatted_property"
         end
 
         it "should produce a :audit event and return" do
@@ -131,6 +112,38 @@ describe Puppet::Transaction::Change do
 
           @change.apply.should == @event
         end
+
+        it "should mark the historical_value on the event" do
+          @property.stub_everything
+
+          @change.apply.historical_value.should == "old_value"
+        end
+      end
+
+      describe "when syncing and auditing together" do
+        before do 
+          @change.auditing = true
+          @change.old_audit_value = "old_value"
+          @property.stubs(:insync?).returns(false)
+        end
+
+        it "should sync the property" do
+          @property.expects(:sync)
+
+          @change.apply
+        end
+
+        it "should produce a success event" do
+          @property.stub_everything
+
+          @change.apply.status.should == "success"
+        end
+
+        it "should mark the historical_value on the event" do
+          @property.stub_everything
+
+          @change.apply.historical_value.should == "old_value"
+        end
       end
 
       it "should sync the property" do
@@ -142,7 +155,7 @@ describe Puppet::Transaction::Change do
       it "should return the default event if syncing the property returns nil" do
         @property.stubs(:sync).returns nil
 
-        @change.expects(:event).with(nil).returns @event
+        @property.expects(:event).with(nil).returns @event
 
         @change.apply.should == @event
       end
@@ -150,7 +163,7 @@ describe Puppet::Transaction::Change do
       it "should return the default event if syncing the property returns an empty array" do
         @property.stubs(:sync).returns []
 
-        @change.expects(:event).with(nil).returns @event
+        @property.expects(:event).with(nil).returns @event
 
         @change.apply.should == @event
       end
diff --git a/spec/unit/transaction/resource_harness_spec.rb b/spec/unit/transaction/resource_harness_spec.rb
index 4611409d5..b143c21ed 100755
--- a/spec/unit/transaction/resource_harness_spec.rb
+++ b/spec/unit/transaction/resource_harness_spec.rb
@@ -1,10 +1,13 @@
 #!/usr/bin/env ruby
 
 require File.dirname(__FILE__) + '/../../spec_helper'
+require 'puppet_spec/files'
 
 require 'puppet/transaction/resource_harness'
 
 describe Puppet::Transaction::ResourceHarness do
+  include PuppetSpec::Files
+
   before do
     @transaction = Puppet::Transaction.new(Puppet::Resource::Catalog.new)
     @resource = Puppet::Type.type(:file).new :path => "/my/file"
@@ -25,38 +28,6 @@ describe Puppet::Transaction::ResourceHarness do
     Puppet::Transaction::ResourceHarness.new(@transaction).relationship_graph.should == "relgraph"
   end
 
-  describe "when copying audited parameters" do
-    before do
-      @resource = Puppet::Type.type(:file).new :path => "/foo/bar", :audit => :mode
-    end
-
-    it "should do nothing if no parameters are being audited" do
-      @resource[:audit] = []
-      @harness.expects(:cached).never
-      @harness.copy_audited_parameters(@resource, {}).should == []
-    end
-
-    it "should do nothing if an audited parameter already has a desired value set" do
-      @resource[:mode] = "755"
-      @harness.expects(:cached).never
-      @harness.copy_audited_parameters(@resource, {}).should == []
-    end
-
-    it "should copy any cached values to the 'should' values" do
-      @harness.cache(@resource, :mode, "755")
-      @harness.copy_audited_parameters(@resource, {}).should == [:mode]
-
-      @resource[:mode].should == "755"
-    end
-
-    it "should cache and log the current value if no cached values are present" do
-      @resource.expects(:debug)
-      @harness.copy_audited_parameters(@resource, {:mode => "755"}).should == []
-
-      @harness.cached(@resource, :mode).should == "755"
-    end
-  end
-
   describe "when evaluating a resource" do
     it "should create and return a resource status instance for the resource" do
       @harness.evaluate(@resource).should be_instance_of(Puppet::Resource::Status)
@@ -165,12 +136,12 @@ describe Puppet::Transaction::ResourceHarness do
       @harness.changes_to_perform(@status, @resource)
     end
 
-    it "should copy audited parameters" do
-      @resource[:audit] = :mode
-      @harness.cache(@resource, :mode, "755")
-      @harness.changes_to_perform(@status, @resource)
-      @resource[:mode].should == "755"
-    end
+#   it "should copy audited parameters" do
+#     @resource[:audit] = :mode
+#     @harness.cache(@resource, :mode, "755")
+#     @harness.changes_to_perform(@status, @resource)
+#     @resource[:mode].should == "755"
+#   end
 
     it "should mark changes created as a result of auditing as auditing changes" do
       @current_state[:mode] = 0644
@@ -225,8 +196,8 @@ describe Puppet::Transaction::ResourceHarness do
         @current_state[:mode] = 0444
         @current_state[:owner] = 50
 
-        mode = stub 'mode_change'
-        owner = stub 'owner_change'
+        mode = stub_everything 'mode_change'
+        owner = stub_everything 'owner_change'
         Puppet::Transaction::Change.expects(:new).with(@resource.parameter(:mode), 0444).returns mode
         Puppet::Transaction::Change.expects(:new).with(@resource.parameter(:owner), 50).returns owner
 
@@ -285,6 +256,148 @@ describe Puppet::Transaction::ResourceHarness do
 
       @harness.cached("myres", "foo").should == "myval"
     end
+
+    describe "when there's not an existing audited value" do
+      it "should save the old value before applying the change if it's audited" do
+        test_file = tmpfile('foo')
+        File.open(test_file, "w", 0750).close
+
+        resource = Puppet::Type.type(:file).new :path => test_file, :mode => '755', :audit => :mode
+
+        @harness.evaluate(resource)
+        @harness.cached(resource, :mode).should == "750"
+
+        (File.stat(test_file).mode & 0777).should == 0755
+        @logs.map {|l| "#{l.level}: #{l.source}: #{l.message}"}.should =~ [
+          "notice: /#{resource}/mode: mode changed '750' to '755'",
+          "notice: /#{resource}/mode: audit change: newly-recorded recorded value 750"
+        ]
+      end
+
+      it "should audit the value if there's no change" do
+        test_file = tmpfile('foo')
+        File.open(test_file, "w", 0755).close
+
+        resource = Puppet::Type.type(:file).new :path => test_file, :mode => '755', :audit => :mode
+
+        @harness.evaluate(resource)
+        @harness.cached(resource, :mode).should == "755"
+
+        (File.stat(test_file).mode & 0777).should == 0755
+
+        @logs.map {|l| "#{l.level}: #{l.source}: #{l.message}"}.should =~ [
+          "notice: /#{resource}/mode: audit change: newly-recorded recorded value 755"
+        ]
+      end
+
+      it "should have :absent for audited value if the file doesn't exist" do
+        test_file = tmpfile('foo')
+
+        resource = Puppet::Type.type(:file).new :ensure => 'present', :path => test_file, :mode => '755', :audit => :mode
+
+        @harness.evaluate(resource)
+        @harness.cached(resource, :mode).should == :absent
+
+        (File.stat(test_file).mode & 0777).should == 0755
+        @logs.map {|l| "#{l.level}: #{l.source}: #{l.message}"}.should =~ [
+          "notice: /#{resource}/ensure: created",
+          "notice: /#{resource}/mode: audit change: newly-recorded recorded value absent"
+        ]
+      end
+
+      it "should do nothing if there are no changes to make and the stored value is correct" do
+        test_file = tmpfile('foo')
+
+        resource = Puppet::Type.type(:file).new :path => test_file, :mode => '755', :audit => :mode, :ensure => 'absent'
+        @harness.cache(resource, :mode, :absent)
+
+        @harness.evaluate(resource)
+        @harness.cached(resource, :mode).should == :absent
+
+        File.exists?(test_file).should == false
+        @logs.map {|l| "#{l.level}: #{l.source}: #{l.message}"}.should =~ []
+      end
+    end
+
+    describe "when there's an existing audited value" do
+      it "should save the old value before applying the change" do
+        test_file = tmpfile('foo')
+        File.open(test_file, "w", 0750).close
+
+        resource = Puppet::Type.type(:file).new :path => test_file, :audit => :mode
+        @harness.cache(resource, :mode, '555')
+
+        @harness.evaluate(resource)
+        @harness.cached(resource, :mode).should == "750"
+
+        (File.stat(test_file).mode & 0777).should == 0750
+        @logs.map {|l| "#{l.level}: #{l.source}: #{l.message}"}.should =~ [
+          "notice: /#{resource}/mode: audit change: previously recorded value 555 has been changed to 750"
+        ]
+      end
+
+      it "should save the old value before applying the change" do
+        test_file = tmpfile('foo')
+        File.open(test_file, "w", 0750).close
+
+        resource = Puppet::Type.type(:file).new :path => test_file, :mode => '755', :audit => :mode
+        @harness.cache(resource, :mode, '555')
+
+        @harness.evaluate(resource)
+        @harness.cached(resource, :mode).should == "750"
+
+        (File.stat(test_file).mode & 0777).should == 0755
+        @logs.map {|l| "#{l.level}: #{l.source}: #{l.message}"}.should =~ [
+          "notice: /#{resource}/mode: mode changed '750' to '755' (previously recorded value was 555)"
+        ]
+      end
+
+      it "should audit the value if there's no change" do
+        test_file = tmpfile('foo')
+        File.open(test_file, "w", 0755).close
+
+        resource = Puppet::Type.type(:file).new :path => test_file, :mode => '755', :audit => :mode
+        @harness.cache(resource, :mode, '555')
+
+        @harness.evaluate(resource)
+        @harness.cached(resource, :mode).should == "755"
+
+        (File.stat(test_file).mode & 0777).should == 0755
+        @logs.map {|l| "#{l.level}: #{l.source}: #{l.message}"}.should =~ [
+          "notice: /#{resource}/mode: audit change: previously recorded value 555 has been changed to 755"
+        ]
+      end
+
+      it "should have :absent for audited value if the file doesn't exist" do
+        test_file = tmpfile('foo')
+
+        resource = Puppet::Type.type(:file).new :ensure => 'present', :path => test_file, :mode => '755', :audit => :mode
+        @harness.cache(resource, :mode, '555')
+
+        @harness.evaluate(resource)
+        @harness.cached(resource, :mode).should == :absent
+
+        (File.stat(test_file).mode & 0777).should == 0755
+
+        @logs.map {|l| "#{l.level}: #{l.source}: #{l.message}"}.should =~ [
+          "notice: /#{resource}/ensure: created", "notice: /#{resource}/mode: audit change: previously recorded value 555 has been changed to absent"
+        ]
+      end
+
+      it "should do nothing if there are no changes to make and the stored value is correct" do
+        test_file = tmpfile('foo')
+        File.open(test_file, "w", 0755).close
+
+        resource = Puppet::Type.type(:file).new :path => test_file, :mode => '755', :audit => :mode
+        @harness.cache(resource, :mode, '755')
+
+        @harness.evaluate(resource)
+        @harness.cached(resource, :mode).should == "755"
+
+        (File.stat(test_file).mode & 0777).should == 0755
+        @logs.map {|l| "#{l.level}: #{l.source}: #{l.message}"}.should =~ []
+      end
+    end
   end
 
   describe "when determining whether the resource can be changed" do
diff --git a/test/lib/puppettest/support/utils.rb b/test/lib/puppettest/support/utils.rb
index e022f123c..bca5d9634 100644
--- a/test/lib/puppettest/support/utils.rb
+++ b/test/lib/puppettest/support/utils.rb
@@ -92,7 +92,7 @@ module PuppetTest::Support::Utils
     method = type
 
     trans.send(method)
-    newevents = trans.events.reject { |e| e.status == 'failure' }.collect { |e|
+    newevents = trans.events.reject { |e| ['failure', 'audit'].include? e.status }.collect { |e|
       e.name
     }
 
diff --git a/test/ral/type/filesources.rb b/test/ral/type/filesources.rb
index dd73cea27..242a82e83 100755
--- a/test/ral/type/filesources.rb
+++ b/test/ral/type/filesources.rb
@@ -327,12 +327,9 @@ class TestFileSources < Test::Unit::TestCase
 
     file = nil
     assert_nothing_raised {
-
-            file = Puppet::Type.type(:file).new(
-                
+      file = Puppet::Type.type(:file).new(
         :name => dest,
         :ensure => "file",
-        
         :source => source
       )
     }