This is the initial commit of the changes to sync and retrieve. The

structure itself is now in place, and a few of the types (the most
complicated ones -- file, user, group, plus exec since it was my easy
first test) have been converted.

I'm now going to finish going through all of the types and finish
chnaging them so that they don't produce any warnings.



git-svn-id: https://reductivelabs.com/svn/puppet/trunk@1546 980ebf18-57e1-0310-9a29-db15c13687c0

25 files changed, 368 insertions, 325 deletions

diff --git a/lib/puppet/server/fileserver.rb b/lib/puppet/server/fileserver.rb
index 49af26c54..93f3310d1 100755
--- a/lib/puppet/server/fileserver.rb
+++ b/lib/puppet/server/fileserver.rb
@@ -63,11 +63,7 @@ class Server
             desc = []
             CHECKPARAMS.each { |check|
                 if state = obj.state(check)
-                    unless state.is
-                        mount.debug "Manually retrieving info for %s" % check
-                        state.retrieve
-                    end
-                    desc << state.is
+                    desc << state.retrieve
                 else
                     if check == "checksum" and obj.state(:type).is == "file"
                         mount.notice "File %s does not have data for %s" %
diff --git a/lib/puppet/statechange.rb b/lib/puppet/statechange.rb
index 4884026fe..989f41280 100644
--- a/lib/puppet/statechange.rb
+++ b/lib/puppet/statechange.rb
@@ -11,19 +11,51 @@ module Puppet
         # The log file generated when this object was changed.
         attr_reader :report
 
-        def initialize(state)
-            @state = state
-            @path = [state.path,"change"].flatten
-            @is = state.is
+        # Switch the goals of the state, thus running the change in reverse.
+        def backward
+            @state.should = @is
+            @should = @is
+            @is = @state.retrieve
+            @state.is = @is
 
-            if state.insync?
-                raise Puppet::Error.new(
-                    "Tried to create a change for in-sync state %s" % state.name
-                )
+            unless defined? @transaction
+                raise Puppet::Error,
+                    "StateChange '%s' tried to be executed outside of transaction" %
+                    self
             end
-            @should = state.should
+            unless @state.insync?
+                @state.info "Backing %s" % self
+                return self.go
+            else
+                @state.debug "rollback is already in sync: %s vs. %s" %
+                    [@state.is.inspect, @state.should.inspect]
+                return nil
+            end
+        end
 
-            @changed = false
+        def forward
+            #@state.debug "moving change forward"
+
+            unless defined? @transaction
+                raise Puppet::Error,
+                    "StateChange '%s' tried to be executed outside of transaction" %
+                    self
+            end
+
+            return self.go
+        end
+
+        # Generate an appropriate event from the is/should values.
+        def genevent
+            tail = if @is == :absent
+                       "created"
+                   elsif @should == :absent
+                       "deleted"
+                   else
+                       "changed"
+                   end
+
+            [state.parent.class.name.to_s, tail].join("_").intern
         end
 
         # Perform the actual change.  This method can go either forward or
@@ -45,28 +77,39 @@ module Puppet
             #    [@state.is.inspect, @state.should.inspect]
 
             # The transaction catches any exceptions here.
-            events = @state.sync
-            if events.nil?
-                return nil
+            if @state.method(:sync).arity == 0
+                @state.warnstamp :syncwoutvalue, "sync() should accept a value"
+                events = @state.sync
+            else
+                events = @state.sync(@should)
             end
 
-            if events.is_a?(Array)
-                if events.empty?
-                    return nil
-                end
-            else
+            unless events.is_a? Array
                 events = [events]
             end
-            
-            return events.collect { |event|
-                # default to a simple event type
-                if ! event.is_a?(Symbol)
-                    @state.warning("State '%s' returned invalid event '%s'; resetting to default" %
-                        [@state.class,event])
 
-                    event = @state.parent.class.name.id2name + "_changed"
+            events = events.collect do |e|
+                if e.nil?
+                    genevent()
+                else
+                    if ! e.is_a?(Symbol)
+                        @state.warning(
+                            "State '%s' returned invalid event '%s'; resetting" %
+                                [@state.class,e]
+                        )
+                        genevent()
+                    else
+                        e
+                    end
                 end
+            end.reject { |e| e == :nochange }
+
 
+            if events.empty?
+                return nil
+            end
+            
+            return events.collect { |event|
                 # i should maybe include object type, but the event type
                 # should basically point to that, right?
                     #:state => @state,
@@ -82,36 +125,25 @@ module Puppet
             }
         end
 
-        def forward
-            #@state.debug "moving change forward"
+        def initialize(state, is = nil)
+            @state = state
+            @path = [state.path,"change"].flatten
 
-            unless defined? @transaction
-                raise Puppet::Error,
-                    "StateChange '%s' tried to be executed outside of transaction" %
-                    self
+            if is
+                @is = is
+            else
+                state.warning "did not pass 'is' to statechange"
+                @is = state.is
             end
 
-            return self.go
-        end
-
-        # Switch the goals of the state, thus running the change in reverse.
-        def backward
-            @state.should = @is
-            @state.retrieve
-
-            unless defined? @transaction
-                raise Puppet::Error,
-                    "StateChange '%s' tried to be executed outside of transaction" %
-                    self
-            end
-            unless @state.insync?
-                @state.info "Backing %s" % self
-                return self.go
-            else
-                @state.debug "rollback is already in sync: %s vs. %s" %
-                    [@state.is.inspect, @state.should.inspect]
-                return nil
+            if state.insync?
+                raise Puppet::Error.new(
+                    "Tried to create a change for in-sync state %s" % state.name
+                )
             end
+            @should = state.should
+
+            @changed = false
         end
         
         def noop
diff --git a/lib/puppet/transaction.rb b/lib/puppet/transaction.rb
index 899aaa456..5e66b63a1 100644
--- a/lib/puppet/transaction.rb
+++ b/lib/puppet/transaction.rb
@@ -90,7 +90,7 @@ class Transaction
 
             # Mark that our change happened, so it can be reversed
             # if we ever get to that point
-            unless events.nil? or (events.is_a?(Array) and events.empty?)
+            unless events.nil?
                 change.changed = true
                 @objectmetrics[:applied] += 1
             end
@@ -196,6 +196,9 @@ class Transaction
             [self.object_id, @count]
 
         allevents
+    ensure
+        # Unset 'is' everywhere.  This is relatively hackish, but, eh.
+        @objects.each do |o| o.clear end
     end
 
     # Determine whether a given object has failed.
@@ -317,6 +320,9 @@ class Transaction
             # And return the events for collection
             events
         }.flatten.reject { |e| e.nil? }
+    ensure
+        # Unset 'is' everywhere.  This is relatively hackish, but, eh.
+        @objects.each do |o| o.clear end
     end
 
     # Trigger any subscriptions to a child.  This does an upwardly recursive
diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb
index d87cbcbd3..21e27ee6d 100644
--- a/lib/puppet/type.rb
+++ b/lib/puppet/type.rb
@@ -29,13 +29,10 @@ class Type < Puppet::Element
     attr_accessor :file, :line
     attr_reader :tags, :parent
 
-    attr_writer :implicit, :title
+    attr_accessor :implicit
+    attr_writer :title
     def implicit?
-        if defined? @implicit and @implicit
-            return true
-        else
-            return false
-        end
+        self.implicit
     end
 
     include Enumerable
@@ -56,10 +53,7 @@ class Type < Puppet::Element
     # iterate across all of the subclasses of Type
     def self.eachtype
         @types.each do |name, type|
-            # Only consider types that have names
-            #if ! type.parameters.empty? or ! type.validstates.empty?
-                yield type 
-            #end
+            yield type 
         end
     end
 
@@ -1707,6 +1701,15 @@ class Type < Puppet::Element
         #@cache[name] = value
     end
 
+    # Remove any vestages of the transaction we're in.  This is currently the only
+    # way to know if you're within a transaction -- if '@is' is set, yes, else,
+    # no.
+    def clear
+        @states.each do |name, state|
+            state.is = nil
+        end
+    end
+
     # Look up the schedule and set it appropriately.  This is done after
     # the instantiation phase, so that the schedule can be anywhere in the
     # file.
@@ -1947,9 +1950,12 @@ class Type < Puppet::Element
     def retrieve
         # it's important to use the method here, as it follows the order
         # in which they're defined in the object
+        is = {}
         states().each { |state|
-            state.retrieve
+            is[state.name] = state.retrieve
         }
+
+        is
     end
 
     # convert to a string
@@ -2000,30 +2006,22 @@ class Type < Puppet::Element
     end
 
     # Retrieve the changes associated with all of the states.
-    def statechanges
+    def statechanges(is)
         # If we are changing the existence of the object, then none of
         # the other states matter.
         changes = []
+
         if @states.include?(:ensure) and ! @states[:ensure].insync?
-            #self.info "ensuring %s from %s" %
-            #    [@states[:ensure].should, @states[:ensure].is]
-            changes = [Puppet::StateChange.new(@states[:ensure])]
+            changes = [Puppet::StateChange.new(@states[:ensure], is[:ensure])]
         # Else, if the 'ensure' state is correctly absent, then do
         # nothing
-        elsif @states.include?(:ensure) and @states[:ensure].is == :absent
-            #self.info "Object is correctly absent"
+        elsif @states.include?(:ensure) and is[:ensure] == :absent
             return []
         else
-            #if @states.include?(:ensure)
-            #    self.info "ensure: Is: %s, Should: %s" %
-            #        [@states[:ensure].is, @states[:ensure].should]
-            #else
-            #    self.info "no ensure state"
-            #end
             changes = states().find_all { |state|
                 ! state.insync?
             }.collect { |state|
-                Puppet::StateChange.new(state)
+                Puppet::StateChange.new(state, is[state.name])
             }
         end
 
@@ -2059,11 +2057,35 @@ class Type < Puppet::Element
         # it's important that we call retrieve() on the type instance,
         # not directly on the state, because it allows the type to override
         # the method, like pfile does
-        self.retrieve
+        is = self.retrieve
+
+        unless is.is_a? Hash
+            warnstamp :nohashreturned, "'retrieve' did not return hash"
+            is = {}
+        end
+
+        @@novalreturned ||= {}
+        states.each do |state|
+            unless is.include? state.name
+                warnstamp "no#{state.name}returned",
+                    "did not get %s from retrieve" % state.name
+                is[state.name] = state.is
+            end
+        end
+
+        # For now, set the 'is' values
+        is.each do |name, value|
+            # A special value to indicate we shouldn't do anything here; usually
+            # the result of an internal error.
+            next if value == :unmanaged
+            if @states.include? name
+                @states[name].is = value
+            end
+        end
 
         # states() is a private method, returning an ordered list
         unless self.class.depthfirst?
-            changes += statechanges()
+            changes += statechanges(is)
         end
 
         changes << @children.collect { |child|
@@ -2073,7 +2095,7 @@ class Type < Puppet::Element
         }
 
         if self.class.depthfirst?
-            changes += statechanges()
+            changes += statechanges(is)
         end
 
         changes.flatten!
diff --git a/lib/puppet/type/exec.rb b/lib/puppet/type/exec.rb
index 0dc3ec02a..8ccebd242 100755
--- a/lib/puppet/type/exec.rb
+++ b/lib/puppet/type/exec.rb
@@ -111,19 +111,29 @@ module Puppet
                 end
             end
 
-            # First verify that all of our checks pass.
-            def retrieve
-                # Default to somethinng
+            def insync?
+                case self.is
+                when :failedchecks
+                    return true
+                when :shouldrun
+                    return false
+                else
+                    raise ArgumentError, "Invalid 'is' value '%s'" % self.is.inspect
+                end
+            end
 
+            # Figure out whether we should run.
+            def retrieve
+                # First make sure all of our checks pass.
                 if @parent.check
-                    self.is = :notrun
+                    return :shouldrun
                 else
-                    self.is = self.should
+                    return :failedchecks
                 end
             end
 
             # Actually execute the command.
-            def sync
+            def sync(value = nil)
                 olddir = nil
 
                 self.checkexe
@@ -444,6 +454,7 @@ module Puppet
                     val = [val] unless val.is_a? Array
                     val.each do |value|
                         unless @parameters[check].check(value)
+                            info "Failed '%s' check" % check
                             return false
                         end
                     end
diff --git a/lib/puppet/type/group.rb b/lib/puppet/type/group.rb
index b03f57907..c6e3324a7 100755
--- a/lib/puppet/type/group.rb
+++ b/lib/puppet/type/group.rb
@@ -26,16 +26,12 @@ module Puppet
         newstate(:ensure) do
             desc "The basic state that the object should be in."
 
-            newvalue(:present) do
+            newvalue(:present, :event => :group_created) do
                 provider.create
-
-                :group_created
             end
 
-            newvalue(:absent) do
+            newvalue(:absent, :event => :group_deleted) do
                 provider.delete
-
-                :group_removed
             end
 
             # If they're talking about the thing at all, they generally want to
@@ -50,7 +46,7 @@ module Puppet
 
             def change_to_s
                 begin
-                    if @is == :absent
+                    if self.is == :absent
                         return "created"
                     elsif self.should == :absent
                         return "removed"
@@ -69,30 +65,11 @@ module Puppet
 
             def retrieve
                 if provider.exists?
-                    @is = :present
+                    return :present
                 else
-                    @is = :absent
+                    return :absent
                 end
             end
-
-            # The default 'sync' method only selects among a list of registered
-            # values.
-            def sync
-                if self.insync?
-                    self.info "already in sync"
-                    return nil
-                #else
-                    #self.info "%s vs %s" % [self.is.inspect, self.should.inspect]
-                end
-                unless self.class.values
-                    self.devfail "No values defined for %s" %
-                        self.class.name
-                end
-
-                # Set ourselves to whatever our should value is.
-                self.set
-            end
-
         end
 
         newstate(:gid) do
@@ -123,16 +100,17 @@ module Puppet
             end
 
             def retrieve
-                @is = provider.gid
+                return provider.gid
             end
 
-            def sync
+            def sync(value)
                 if self.should == :absent
                     raise Puppet::DevError, "GID cannot be deleted"
                 else
-                    provider.gid = self.should
-                    :group_modified
+                    provider.gid = value
                 end
+
+                return nil
             end
 
             munge do |gid|
@@ -184,18 +162,19 @@ module Puppet
 
         def retrieve
             if @provider.exists?
-                super
+                return super
             else
                 # the group does not exist
                 #unless @states.include?(:gid)
                 #    self[:gid] = :auto
                 #end
 
+                current = {}
                 @states.each { |name, state|
-                    state.is = :absent
+                    current[state.name] = :absent
                 }
 
-                return
+                return current
             end
         end
     end
diff --git a/lib/puppet/type/package.rb b/lib/puppet/type/package.rb
index f7ae0f0d4..3c4d20cc9 100644
--- a/lib/puppet/type/package.rb
+++ b/lib/puppet/type/package.rb
@@ -136,8 +136,7 @@ module Puppet
                 @is = @parent.retrieve
             end
 
-            def sync
-                value = self.should
+            def sync(value)
                 unless value.is_a?(Symbol)
                     value = value.intern
                 end
diff --git a/lib/puppet/type/parsedtype.rb b/lib/puppet/type/parsedtype.rb
index b0e5f618d..f6c227541 100755
--- a/lib/puppet/type/parsedtype.rb
+++ b/lib/puppet/type/parsedtype.rb
@@ -59,7 +59,7 @@ module Puppet
 
             # If the ensure state is out of sync, it will always be called
             # first, so I don't need to worry about that.
-            def sync(nostore = false)
+            def sync(value = nil, nostore = false)
                 ebase = @parent.class.name.to_s
 
                 tail = nil
diff --git a/lib/puppet/type/pfile.rb b/lib/puppet/type/pfile.rb
index ec666497d..ac892c069 100644
--- a/lib/puppet/type/pfile.rb
+++ b/lib/puppet/type/pfile.rb
@@ -691,7 +691,7 @@ module Puppet
 
                 if child = self.newchild(file, true, options)
                     # Mark any unmanaged files for removal if purge is set.
-                    if self[:purge] == :true and child.implicit?
+                    if self[:purge] == :true and ! child.managed?
                         child[:ensure] = :absent
                     end
 
@@ -761,6 +761,8 @@ module Puppet
 
         # a wrapper method to make sure the file exists before doing anything
         def retrieve
+            is = {}
+
             if @states.include?(:source)
                 # This probably isn't the best place for it, but we need
                 # to make sure that we have a corresponding checksum state.
@@ -770,35 +772,35 @@ module Puppet
 
                 # We have to retrieve the source info before the recursion happens,
                 # although I'm not exactly clear on why.
-                @states[:source].retrieve
+                is[:source] = @states[:source].retrieve
             end
 
             if @parameters.include?(:recurse)
                 self.recurse
             end
 
-            unless stat = self.stat(true)
+            if stat = self.stat(true)
+                states().each { |state|
+                    # We don't want to call 'describe()' twice, so only do a local
+                    # retrieve on the source.
+                    if state.name == :source
+                        is[state.name] = state.retrieve(false)
+                    else
+                        is[state.name] = state.retrieve
+                    end
+                }
+            else
                 self.debug "File does not exist"
                 @states.each { |name,state|
                     # We've already retreived the source, and we don't
                     # want to overwrite whatever it did.  This is a bit
                     # of a hack, but oh well, source is definitely special.
                     next if name == :source
-                    state.is = :absent
+                    is[name] = :absent
                 }
-
-                return
             end
 
-            states().each { |state|
-                # We don't want to call 'describe()' twice, so only do a local
-                # retrieve on the source.
-                if state.name == :source
-                    state.retrieve(false)
-                else
-                    state.retrieve
-                end
-            }
+            return is
         end
 
         # Set the checksum, from another state.  There are multiple states that
diff --git a/lib/puppet/type/pfile/checksum.rb b/lib/puppet/type/pfile/checksum.rb
index cd82a4439..514c47690 100755
--- a/lib/puppet/type/pfile/checksum.rb
+++ b/lib/puppet/type/pfile/checksum.rb
@@ -191,7 +191,7 @@ module Puppet
 
                 if self.insync?
                     self.debug "Checksum is already in sync"
-                    return nil
+                    return :nochange
                 end
                 #@parent.debug "%s(%s): after refresh, is '%s'" %
                 #    [self.class.name,@parent.name,@is]
@@ -207,7 +207,7 @@ module Puppet
                             @parent[:path]
                         )
                     end
-                    return nil
+                    return :nochange
                 end
             end
 
@@ -215,7 +215,7 @@ module Puppet
             if self.updatesum
                 return :file_changed
             else
-                return nil
+                return :nochange
             end
         end
 
@@ -270,6 +270,8 @@ module Puppet
             end
 
             #@parent.debug "checksum state is %s" % self.is
+
+            return @is
         end
 
         # Store the new sum to the state db.
diff --git a/lib/puppet/type/pfile/content.rb b/lib/puppet/type/pfile/content.rb
index a8422431f..3eb875dc2 100755
--- a/lib/puppet/type/pfile/content.rb
+++ b/lib/puppet/type/pfile/content.rb
@@ -29,36 +29,36 @@ module Puppet
         def retrieve
             stat = nil
             unless stat = @parent.stat
-                @is = :absent
-                return
+                return :absent
             end
 
             if stat.ftype == "link" and @parent[:links] == :ignore
-                self.is = self.should
-                return
+                return self.should
             end
 
             # Don't even try to manage the content on directories
             if stat.ftype == "directory" and @parent[:links] == :ignore
                 @parent.delete(:content)
-                return
+                return :notmanaged
             end
 
             begin
-                @is = File.read(@parent[:path])
+                retval = File.read(@parent[:path])
             rescue => detail
-                @is = nil
+                retval = :unknown
                 raise Puppet::Error, "Could not read %s: %s" %
                     [@parent.title, detail]
             end
+
+            return retval
         end
 
 
         # Just write our content out to disk.
-        def sync
-            @parent.write { |f| f.print self.should }
+        def sync(value)
+            @parent.write { |f| f.print value }
 
-            if @is == :absent
+            if self.is == :absent
                 return :file_created
             else
                 return :file_changed
diff --git a/lib/puppet/type/pfile/ensure.rb b/lib/puppet/type/pfile/ensure.rb
index ac045dfd6..0debc245a 100755
--- a/lib/puppet/type/pfile/ensure.rb
+++ b/lib/puppet/type/pfile/ensure.rb
@@ -35,19 +35,25 @@ module Puppet
         # Most 'ensure' states have a default, but with files we, um, don't.
         nodefault
 
-        newvalue(:absent) do
+        newvalue(:absent, :event => :file_deleted) do
             File.unlink(@parent[:path])
         end
 
         aliasvalue(:false, :absent)
 
-        newvalue(:file) do
+        newvalue(:file, :event => :file_created) do
             # Make sure we're not managing the content some other way
-            if state = @parent.state(:content) or state = @parent.state(:source)
-                state.sync
+            if state = (@parent.state(:content) || @parent.state(:source))
+                # Manually sync the state, and reset its is value to it knows it's
+                # in sync.
+                should = state.should
+                state.commit
+
+                # The 'sync' method here syncs any states that might still be
+                # out of sync like 'mode', so we need to mark this in sync.
+                state.is = should
             else
                 @parent.write(false) { |f| f.flush }
-                mode = @parent.should(:mode)
             end
             return :file_created
         end
@@ -59,7 +65,7 @@ module Puppet
             set_file
         end
 
-        newvalue(:directory) do
+        newvalue(:directory, :event => :directory_created) do
             mode = @parent.should(:mode)
             parent = File.dirname(@parent[:path])
             unless FileTest.exists? parent
@@ -81,7 +87,7 @@ module Puppet
         end
 
 
-        newvalue(:link) do
+        newvalue(:link, :event => :link_created) do
             if state = @parent.state(:target)
                 state.retrieve
 
@@ -97,7 +103,7 @@ module Puppet
         end
 
         # Symlinks.
-        newvalue(/./) do
+        newvalue(/./, :event => :link_created) do
             # This code never gets executed.  We need the regex to support
             # specifying it, but the work is done in the 'symlink' code block.
         end
@@ -130,8 +136,9 @@ module Puppet
         # We have to treat :present specially, because it works with any
         # type of file.
         def insync?
+            is = self.is
             if self.should == :present
-                if @is.nil? or @is == :absent
+                if is.nil? or is == :absent
                     return false
                 else
                     return true
@@ -142,19 +149,22 @@ module Puppet
         end
 
         def retrieve
+            retval = nil
             if stat = @parent.stat(false)
-                @is = stat.ftype.intern
+                retval = stat.ftype.intern
             else
                 if self.should == :false
-                    @is = :false
+                    retval = :false
                 else
-                    @is = :absent
+                    retval = :absent
                 end
             end
+
+            return retval
         end
 
-        def sync
-            event = super
+        def sync(value)
+            event = super(value)
 
             # There are some cases where all of the work does not get done on
             # file creation, so we have to do some extra checking.
@@ -164,7 +174,7 @@ module Puppet
 
                 thing.retrieve
                 unless thing.insync?
-                    thing.sync
+                    thing.commit
                 end
             end
 
diff --git a/lib/puppet/type/pfile/group.rb b/lib/puppet/type/pfile/group.rb
index 9431b0e65..ce6d43422 100755
--- a/lib/puppet/type/pfile/group.rb
+++ b/lib/puppet/type/pfile/group.rb
@@ -41,8 +41,7 @@ module Puppet
             stat = @parent.stat(false)
 
             unless stat
-                self.is = :absent
-                return
+                return :absent
             end
 
             # Set our method appropriately, depending on links.
@@ -51,7 +50,7 @@ module Puppet
             else
                 @method = :chown
             end
-            self.is = stat.gid
+            return stat.gid
         end
 
         # Determine if the group is valid, and if so, return the UID
@@ -82,7 +81,7 @@ module Puppet
         # Normal users will only be able to manage certain groups.  Right now,
         # we'll just let it fail, but we should probably set things up so
         # that users get warned if they try to change to an unacceptable group.
-        def sync
+        def sync(value)
             if @is == :absent
                 @parent.stat(true)
                 self.retrieve
@@ -90,17 +89,17 @@ module Puppet
                 if @is == :absent
                     self.debug "File '%s' does not exist; cannot chgrp" %
                         @parent[:path]
-                    return nil
+                    return :nochange
                 end
 
                 if self.insync?
-                    return nil
+                    return :nochange
                 end
             end
 
             gid = nil
-            unless gid = Puppet::Util.gid(self.should)
-                raise Puppet::Error, "Could not find group %s" % self.should
+            unless gid = Puppet::Util.gid(value)
+                raise Puppet::Error, "Could not find group %s" % value
             end
 
             begin
@@ -108,7 +107,7 @@ module Puppet
                 File.send(@method,nil,gid,@parent[:path])
             rescue => detail
                 error = Puppet::Error.new( "failed to chgrp %s to %s: %s" %
-                    [@parent[:path], self.should, detail.message])
+                    [@parent[:path], value, detail.message])
                 raise error
             end
             return :file_changed
diff --git a/lib/puppet/type/pfile/mode.rb b/lib/puppet/type/pfile/mode.rb
index ee6fb37d8..9dd993682 100755
--- a/lib/puppet/type/pfile/mode.rb
+++ b/lib/puppet/type/pfile/mode.rb
@@ -89,35 +89,35 @@ module Puppet
             # off mode management entirely.
 
             if stat = @parent.stat(false)
-                self.is = stat.mode & 007777
                 unless defined? @fixed
                     if defined? @should and @should
                         @should = @should.collect { |s| self.dirmask(s) }
                     end
                 end
+                return stat.mode & 007777
             else
-                self.is = :absent
+                return :absent
             end
 
             #self.debug "chmod state is %o" % self.is
         end
 
-        def sync
+        def sync(value)
             if @is == :absent
                 @parent.stat(true)
                 self.retrieve
                 if @is == :absent
                     self.debug "File does not exist; cannot set mode"
-                    return nil
+                    return :nochange
                 end
 
                 if self.insync?
                     # we're already in sync
-                    return nil
+                    return :nochange
                 end
             end
 
-            mode = self.should
+            mode = value
 
             if mode == :absent
                 # This is really only valid for create states...
diff --git a/lib/puppet/type/pfile/source.rb b/lib/puppet/type/pfile/source.rb
index 744d66f34..057673457 100755
--- a/lib/puppet/type/pfile/source.rb
+++ b/lib/puppet/type/pfile/source.rb
@@ -116,6 +116,7 @@ module Puppet
                 return nil
             end
 
+            retval = nil
             # If we're a normal file, then set things up to copy the file down.
             case @stats[:type]
             when "file":
@@ -124,20 +125,20 @@ module Puppet
                         if sum.is == :absent
                             sum.retrieve(true)
                         end
-                        @is = sum.is
+                        retval = sum.is
                     else
-                        @is = :absent
+                        retval = :absent
                     end
                 else
                     self.info "File does not have checksum"
-                    @is = :absent
+                    retval = :absent
                 end
                 # if replace => false then fake the checksum so that the file
                 # is not overwritten.
-                unless @is == :absent
+                unless retval == :absent
                     if @parent[:replace] == :false
                         info "Not replacing existing file"
-                        @is = @stats[:checksum]
+                        retval = @stats[:checksum]
                     end
                 end
                 @should = [@stats[:checksum]]
@@ -154,11 +155,11 @@ module Puppet
                 end
                 # we'll let the :ensure state do our work
                 @should.clear
-                @is = true
+                retval = true
             when "link":
                 case @parent[:links]
                 when :ignore
-                    @is = :nocopy
+                    retval = :nocopy
                     @should = [:nocopy]
                     self.info "Ignoring link %s" % @source
                     return
@@ -175,7 +176,7 @@ module Puppet
                 self.err "Cannot use files of type %s as sources" %
                     @stats[:type]
                 @should = [:nocopy]
-                @is = :nocopy
+                retval = :nocopy
             end
 
             # Take each of the stats and set them as states on the local file
@@ -196,6 +197,8 @@ module Puppet
                 #    @parent.info "Already specified %s" % stat
                 end
             }
+
+            return retval
         end
 
         # The special thing here is that we need to make sure that 'should'
@@ -226,22 +229,19 @@ module Puppet
             end
         end
 
-        def sync
+        def sync(value)
             if @is == :notdescribed
                 self.retrieve # try again
                 if @is == :notdescribed
                     @parent.log "Could not retreive information on %s" %
                         @parent.title
-                    return nil
+                    return :nochange
                 end
                 if @is == @should
-                    return nil
+                    return :nochange
                 end
             end
 
-            case @stats[:type]
-            when "link":
-            end
             unless @stats[:type] == "file"
                 #if @stats[:type] == "directory"
                         #[@parent.name, @is.inspect, @should.inspect]
diff --git a/lib/puppet/type/pfile/target.rb b/lib/puppet/type/pfile/target.rb
index 4a725d652..6bdc408f2 100644
--- a/lib/puppet/type/pfile/target.rb
+++ b/lib/puppet/type/pfile/target.rb
@@ -56,8 +56,8 @@ module Puppet
 
         def retrieve
             if @parent.state(:ensure).should == :directory
-                @is = self.should
                 @linkmaker = true
+                return self.should
             else
                 if stat = @parent.stat
                     # If we're just checking the value
@@ -73,18 +73,18 @@ module Puppet
                         warning "Changing ensure to directory; recurse is %s but %s" %
                             [@parent[:recurse].inspect, @parent.recurse?]
                         @parent[:ensure] = :directory
-                        @is = should
                         @linkmaker = true
+                        return should
                     else
                         if stat.ftype == "link"
-                            @is = File.readlink(@parent[:path])
                             @linkmaker = false
+                            return File.readlink(@parent[:path])
                         else
-                            @is = :notlink
+                            return :notlink
                         end
                     end
                 else
-                    @is = :absent
+                    return :absent
                 end
             end
         end
diff --git a/lib/puppet/type/pfile/type.rb b/lib/puppet/type/pfile/type.rb
index e5db7d694..4a7b9641d 100755
--- a/lib/puppet/type/pfile/type.rb
+++ b/lib/puppet/type/pfile/type.rb
@@ -8,18 +8,21 @@ module Puppet
         #end
         
         def retrieve
+            retval = nil
             if stat = @parent.stat(false)
-                @is = stat.ftype
+                retval = stat.ftype
             else
-                @is = :absent
+                retval = :absent
             end
 
             # so this state is never marked out of sync
-            @should = [@is]
+            @should = [retval]
+
+            return retval
         end
 
 
-        def sync
+        def sync(value)
             raise Puppet::Error, ":type is read-only"
         end
     end
diff --git a/lib/puppet/type/pfile/uid.rb b/lib/puppet/type/pfile/uid.rb
index a492b31f4..e05ba4f4c 100755
--- a/lib/puppet/type/pfile/uid.rb
+++ b/lib/puppet/type/pfile/uid.rb
@@ -81,8 +81,7 @@ module Puppet
 
         def retrieve
             unless stat = @parent.stat(false)
-                @is = :absent
-                return
+                return :absent
             end
 
             # Set our method appropriately, depending on links.
@@ -92,15 +91,17 @@ module Puppet
                 @method = :chown
             end
 
-            self.is = stat.uid
+            retval = stat.uid
 
             # On OS X, files that are owned by -2 get returned as really
             # large UIDs instead of negative ones.  This isn't a Ruby bug,
             # it's an OS X bug, since it shows up in perl, too.
-            if @is > 120000
-                self.warning "current state is silly: %s" % @is
-                @is = :absent
+            if retval > 120000
+                self.warning "current state is silly: %s" % retval
+                retval = :absent
             end
+
+            return retval
         end
 
         # If we're not root, we can check the values but we cannot change
@@ -116,19 +117,19 @@ module Puppet
             end
         end
 
-        def sync
+        def sync(value)
             unless Process.uid == 0
                 unless defined? @@notifieduid
                     self.notice "Cannot manage ownership unless running as root"
                     #@parent.delete(self.name)
                     @@notifieduid = true
                 end
-                return nil
+                return :nochange
             end
 
             user = nil
-            unless user = self.validuser?(self.should)
-                tmp = self.should
+            unless user = self.validuser?(value)
+                tmp = value
                 unless defined? @@usermissing
                     @@usermissing = {}
                 end
@@ -139,7 +140,7 @@ module Puppet
                     self.notice "user %s does not exist" % tmp
                     @@usermissing[tmp] = 1
                 end
-                return nil
+                return :nochange
             end
 
             if @is == :absent
@@ -147,10 +148,10 @@ module Puppet
                 self.retrieve
                 if @is == :absent
                     self.debug "File does not exist; cannot set owner"
-                    return nil
+                    return :nochange
                 end
                 if self.insync?
-                    return nil
+                    return :nochange
                 end
                 #self.debug "%s: after refresh, is '%s'" % [self.class.name,@is]
             end
diff --git a/lib/puppet/type/state.rb b/lib/puppet/type/state.rb
index de0cfeb00..f7f2ed48a 100644
--- a/lib/puppet/type/state.rb
+++ b/lib/puppet/type/state.rb
@@ -8,12 +8,11 @@ require 'puppet/parameter'
 
 module Puppet
 class State < Puppet::Parameter
-    attr_accessor :is
-
     # Because 'should' uses an array, we have a special method for handling
     # it.  We also want to keep copies of the original values, so that
     # they can be retrieved and compared later when merging.
     attr_reader :shouldorig
+    attr_writer :is
 
     class << self
         attr_accessor :unmanaged
@@ -109,6 +108,11 @@ class State < Puppet::Parameter
                 [self.name, detail]
         end
     end
+
+    # Just a simple wrapper method to sync our current 'should' value.
+    def commit
+        self.sync(self.should)
+    end
     
     # initialize our state
     def initialize(hash)
@@ -167,8 +171,9 @@ class State < Puppet::Parameter
         end
 
         # Look for a matching value
+        is = self.is
         @should.each { |val|
-            if @is == val
+            if is == val
                 return true
             end
         }
@@ -177,12 +182,17 @@ class State < Puppet::Parameter
         return false
     end
 
+    # If the '@is' value is set, then return it, else retrieve it.
+    def is
+        @is || self.retrieve
+    end
+
     # because the @should and @is vars might be in weird formats,
     # we need to set up a mechanism for pretty printing of the values
     # default to just the values, but this way individual states can
     # override these methods
     def is_to_s
-        @is
+        self.is
     end
 
     # Send a log message.
@@ -236,7 +246,7 @@ class State < Puppet::Parameter
     # provider.  In other words, if the state name is 'gid', we'll call
     # 'provider.gid' to retrieve the current value.
     def retrieve
-        @is = provider.send(self.class.name)
+        provider.send(self.class.name)
     end
 
     # Call the method associated with a given value.
@@ -276,28 +286,18 @@ class State < Puppet::Parameter
             end
         end
 
+        if event == :nochange
+            return :nochange
+        end
+
         if setevent = self.class.event(value)
             return setevent
         else
             if event and event.is_a?(Symbol)
-                if event == :nochange
-                    return nil
-                else
-                    return event
-                end
-            else
-                # Return the appropriate event.
-                event = case self.should
-                when :present: (@parent.class.name.to_s + "_created").intern
-                when :absent: (@parent.class.name.to_s + "_removed").intern
-                else
-                    (@parent.class.name.to_s + "_changed").intern
-                end
-
-                #self.log "made event %s because 'should' is %s, 'is' is %s" %
-                #    [event, self.should.inspect, self.is.inspect]
-
                 return event
+            else
+                # StateChange will autogenerate an event.
+                return nil
             end
         end
     end
@@ -347,10 +347,16 @@ class State < Puppet::Parameter
 
     # The default 'sync' method only selects among a list of registered
     # values.
-    def sync
+    def sync(value = nil)
+
+        unless value
+            warnstamp :novalsynced, "No value passed to sync"
+            value = self.should
+        end
+
         if self.insync?
             self.info "already in sync"
-            return nil
+            return :nochange
         #else
             #self.info "%s vs %s" % [self.is.inspect, self.should.inspect]
         end
@@ -385,15 +391,9 @@ class State < Puppet::Parameter
             @doc ||= "The basic state that the object should be in."
         end
 
-        def self.inherited(sub)
-            # Add in the two states that everyone will have.
-            sub.class_eval do
-            end
-        end
-
         def change_to_s
             begin
-                if @is == :absent
+                if self.is == :absent
                     return "created"
                 elsif self.should == :absent
                     return "removed"
@@ -416,9 +416,9 @@ class State < Puppet::Parameter
             # @is values set.  This seems to be the source of quite a few bugs,
             # although they're mostly logging bugs, not functional ones.
             if @parent.exists?
-                @is = :present
+                return :present
             else
-                @is = :absent
+                return :absent
             end
         end
 
diff --git a/lib/puppet/type/user.rb b/lib/puppet/type/user.rb
index 3a78d9669..b0c3081e4 100755
--- a/lib/puppet/type/user.rb
+++ b/lib/puppet/type/user.rb
@@ -50,7 +50,7 @@ module Puppet
 
             def change_to_s
                 begin
-                    if @is == :absent
+                    if self.is == :absent
                         return "created"
                     elsif self.should == :absent
                         return "removed"
@@ -69,30 +69,11 @@ module Puppet
 
             def retrieve
                 if provider.exists?
-                    @is = :present
+                    return :present
                 else
-                    @is = :absent
+                    return :absent
                 end
             end
-
-            # The default 'sync' method only selects among a list of registered
-            # values.
-            def sync
-                if self.insync?
-                    self.info "already in sync"
-                    return nil
-                #else
-                    #self.info "%s vs %s" % [self.is.inspect, self.should.inspect]
-                end
-                unless self.class.values
-                    self.devfail "No values defined for %s" %
-                        self.class.name
-                end
-
-                # Set ourselves to whatever our should value is.
-                self.set
-            end
-
         end
 
         newstate(:uid) do
@@ -193,24 +174,21 @@ module Puppet
             end
 
             def is_to_s
-                @is.join(",")
+                self.is.join(",")
             end
 
             # We need to override this because the groups need to
             # be joined with commas
             def should
-                unless defined? @is
-                    retrieve
-                end
-
                 @should ||= []
 
                 if @parent[:membership] == :inclusive
                     @should.sort
                 else
+                    current = self.is
                     members = @should
-                    if @is.is_a?(Array)
-                        members += @is
+                    if current.is_a?(Array)
+                        members += current
                     end
                     members.uniq.sort
                 end
@@ -218,9 +196,13 @@ module Puppet
 
             def retrieve
                 if tmp = provider.groups
-                    @is = tmp.split(",")
+                    if tmp == :absent
+                        return tmp
+                    else
+                        return tmp.split(",")
+                    end
                 else
-                    @is = :absent
+                    return :absent
                 end
             end
 
@@ -228,13 +210,13 @@ module Puppet
                 unless defined? @should and @should
                     return false
                 end
-                unless defined? @is and @is
+                unless defined? self.is and self.is
                     return false
                 end
-                unless @is.class == @should.class
+                unless self.is.class == @should.class
                     return false
                 end
-                return @is.sort == @should.sort
+                return self.is.sort == @should.sort
             end
 
             validate do |value|
@@ -243,8 +225,8 @@ module Puppet
                 end
             end
 
-            def sync
-                provider.groups = self.should.join(",")
+            def sync(value)
+                provider.groups = value.join(",")
                 :user_changed
             end
         end
@@ -374,27 +356,16 @@ module Puppet
 
         def retrieve
             absent = false
+            current = {}
             states().each { |state|
-                if absent
-                    state.is = :absent
+                if current[:ensure] == :absent
+                    current[state.name] = :absent
                 else
-                    state.retrieve
-                end
-
-                if state.name == :ensure and state.is == :absent
-                    absent = true
-                    next
+                    current[state.name] = state.retrieve
                 end
             }
-            #if provider.exists?
-            #    super
-            #else
-            #    # the user does not exist
-            #    @states.each { |name, state|
-            #        state.is = :absent
-            #    }
-            #    return
-            #end
+
+            return current
         end
     end
 end
diff --git a/lib/puppet/util.rb b/lib/puppet/util.rb
index a818a77bf..515a91c36 100644
--- a/lib/puppet/util.rb
+++ b/lib/puppet/util.rb
@@ -512,6 +512,15 @@ module Util
         return seconds
     end
 
+    def warnstamp(var, msg)
+        $stampwarnings ||= {}
+        $stampwarnings[self.class] ||= {}
+        unless $stampwarnings[self.class][var]
+            warning msg
+            $stampwarnings[self.class][var] = true
+        end
+    end
+
     module_function :memory
 end
 end
diff --git a/test/types/file.rb b/test/types/file.rb
index 3dc296cc0..194d00958 100644
--- a/test/types/file.rb
+++ b/test/types/file.rb
@@ -704,8 +704,6 @@ class TestFile < Test::Unit::TestCase
 
         assert_events([:file_created], obj)
 
-        obj.retrieve
-
         assert(obj.insync?, "Object is not in sync")
 
         text = File.read(file)
@@ -795,18 +793,17 @@ class TestFile < Test::Unit::TestCase
         dest = tempfile()
 
         file = nil
+        str = "some content, ok?"
         assert_nothing_raised {
             file = Puppet.type(:file).create(
                 :name => dest,
                 :ensure => "file",
-                :content => "this is some content, yo"
+                :content => str
             )
         }
 
-        file.retrieve
-
         assert_events([:file_created], file)
-        file.retrieve
+        assert_equal(str, File.read(dest))
         assert_events([], file)
         assert_events([], file)
     end
@@ -1200,7 +1197,7 @@ class TestFile < Test::Unit::TestCase
         }
 
         # First run through without :force
-        assert_events([], file)
+        assert_events([], file, "Link replaced directory without force enabled")
 
         assert(FileTest.directory?(link), "Link replaced dir without force")
 
@@ -1376,11 +1373,12 @@ class TestFile < Test::Unit::TestCase
 
         lfobj = Puppet::Type.newfile(:path => localfile, :content => "rahtest")
 
+        assert(! lfobj.implicit?, "object incorrectly implicit")
+
         destobj = Puppet::Type.newfile(:path => destdir,
                                     :source => sourcedir,
                                     :recurse => true)
 
-
         assert_apply(lfobj, destobj)
 
         assert(FileTest.exists?(dsourcefile), "File did not get copied")
diff --git a/test/types/group.rb b/test/types/group.rb
index 508b8436c..2b47f14d4 100755
--- a/test/types/group.rb
+++ b/test/types/group.rb
@@ -18,6 +18,9 @@ class TestGroup < Test::Unit::TestCase
 
         def create
             @ensure = :present
+
+            # Just set a fake gid
+            self.gid = 10
         end
 
         def delete
@@ -76,7 +79,7 @@ class TestGroup < Test::Unit::TestCase
         assert_events([:group_created], comp)
         assert_equal(:present, group.provider.ensure,  "Group is absent")
         group[:ensure] = :absent
-        trans = assert_events([:group_removed], comp)
+        trans = assert_events([:group_deleted], comp)
         assert_equal(:absent, group.provider.ensure,  "Group is present")
 
         assert_rollback_events(trans, [:group_created], "group")
@@ -86,7 +89,6 @@ class TestGroup < Test::Unit::TestCase
     # This is a bit odd, since we're not actually doing anything on the machine.
     # Just make sure we can set the gid and that it will work correctly.
     def attrtest_gid(group)
-
         # Check the validation.
         assert_nothing_raised {
             group[:gid] = "15"
@@ -96,7 +98,8 @@ class TestGroup < Test::Unit::TestCase
                      "Did not convert gid to number")
 
         comp = newcomp(group)
-        trans = assert_events([:group_modified], comp, "group")
+
+        trans = assert_events([:group_changed], comp, "group")
         assert_equal(15, group.provider.gid, "GID was not changed")
 
         assert_nothing_raised {
@@ -107,12 +110,12 @@ class TestGroup < Test::Unit::TestCase
                      "Did not keep gid as number")
 
         # Now switch to 16
-        trans = assert_events([:group_modified], comp, "group")
+        trans = assert_events([:group_changed], comp, "group")
         assert_equal(16, group.provider.gid, "GID was not changed")
 
         # And then rollback
-        assert_rollback_events(trans, [:group_modified], "group")
-        assert_equal(15, group.provider.gid, "GID was not changed")
+        assert_rollback_events(trans, [:group_changed], "group")
+        assert_equal(15, group.provider.gid, "GID was not reverted")
     end
 
     def test_owngroups
@@ -167,7 +170,7 @@ class TestGroup < Test::Unit::TestCase
             end
         }
 
-        assert_rollback_events(trans, [:group_removed], "group")
+        assert_rollback_events(trans, [:group_deleted], "group")
 
         assert(! gobj.provider.exists?,
                 "Did not delete group")
diff --git a/test/types/package.rb b/test/types/package.rb
index 779c693fe..9ffd846ef 100644
--- a/test/types/package.rb
+++ b/test/types/package.rb
@@ -167,7 +167,7 @@ class TestPackages < Test::Unit::TestCase
         obj[:source] = file
         assert_raise(Puppet::PackageError,
             "Successfully installed nonexistent package") {
-            state.sync
+            state.sync(state.should)
         }
     end
 
diff --git a/test/types/user.rb b/test/types/user.rb
index 008f39272..7522a041b 100755
--- a/test/types/user.rb
+++ b/test/types/user.rb
@@ -19,7 +19,7 @@ class TestUser < Test::Unit::TestCase
             @ensure = :present
             @model.eachstate do |state|
                 next if state.name == :ensure
-                state.sync
+                state.commit
             end
         end