diff --git a/lib/puppet/type/user.rb b/lib/puppet/type/user.rb
index 8905b8d1d..ac6ce461d 100755
--- a/lib/puppet/type/user.rb
+++ b/lib/puppet/type/user.rb
@@ -1,400 +1,397 @@
 require 'etc'
 require 'facter'
 require 'puppet/type/state'
 
 module Puppet
     newtype(:user) do
         newstate(:ensure) do
             newvalue(:present, :event => :user_created) do
                 # Verify that they have provided everything necessary, if we
                 # are trying to manage the user
 #                if @parent.managed?
 #                    @parent.class.states.each { |state|
 #                        next if stateobj = @parent.state(state.name)
 #                        next if state.name == :ensure
 #
 #                        unless state.autogen? or state.isoptional?
 #                            if state.method_defined?(:autogen)
 #                                @parent[state.name] = :auto
 #                            else
 #                                @parent.fail "Users require a value for %s" %
 #                                    state.name
 #                            end
 #                        end
 #                    }
 #
 #                    #if @states.empty?
 #                    #    @parent[:comment] = @parent[:name]
 #                    #end
 #                end
                 provider.create
             end
 
             newvalue(:absent, :event => :user_removed) do
                 provider.delete
             end
 
             desc "The basic state that the object should be in."
 
             # If they're talking about the thing at all, they generally want to
             # say it should exist.
             #defaultto :present
             defaultto do
                 if @parent.managed?
                     :present
                 else
                     nil
                 end
             end
 
             def change_to_s
                 begin
                     if @is == :absent
                         return "created"
                     elsif self.should == :absent
                         return "removed"
                     else
                         return "%s changed '%s' to '%s'" %
                             [self.name, self.is_to_s, self.should_to_s]
                     end
                 rescue Puppet::Error, Puppet::DevError
                     raise
                 rescue => detail
                     raise Puppet::DevError,
                         "Could not convert change %s to string: %s" %
                         [self.name, detail]
                 end
             end
 
             def retrieve
                 if provider.exists?
                     @is = :present
                 else
                     @is = :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
             desc "The user ID.  Must be specified numerically.  For new users
                 being created, if no user ID is specified then one will be
                 chosen automatically, which will likely result in the same user
                 having different IDs on different systems, which is not
                 recommended."
 
             munge do |value|
                 case value
                 when String
                     if value =~ /^[-0-9]+$/
                         value = Integer(value)
                     end
                 when Symbol
                     unless value == :absent or value == :auto
                         self.devfail "Invalid UID %s" % value
                     end
 
                     if value == :auto
                         value = autogen()
                     end
                 end
 
                 return value
             end
         end
 
         newstate(:gid) do
             desc "The user's primary group.  Can be specified numerically or
                 by name."
 
             munge do |gid|
                 method = :getgrgid
                 case gid
                 when String
                     if gid =~ /^[-0-9]+$/
                         gid = Integer(gid)
                     else
                         method = :getgrnam
                     end
                 when Symbol
                     unless gid == :auto or gid == :absent
                         self.devfail "Invalid GID %s" % gid
                     end
                     # these are treated specially by sync()
                     return gid
                 end
 
                 if group = Puppet::Util.gid(gid)
                     @found = true
                     return group
                 else
                     @found = false
                     return gid
                 end
             end
 
             # *shudder*  Make sure that we've looked up the group and gotten
             # an ID for it.  Yuck-o.
             def should
                 unless defined? @should
                     return super
                 end
                 unless defined? @found and @found
                     @should = @should.each { |val|
                         next unless val
                         Puppet::Util.gid(val)
                     }
                 end
                 super
             end
         end
 
         newstate(:comment) do
             desc "A description of the user.  Generally is a user's full name."
         end
 
         newstate(:home) do
             desc "The home directory of the user.  The directory must be created
                 separately and is not currently checked for existence."
         end
 
         newstate(:shell) do
             desc "The user's login shell.  The shell must exist and be
                 executable."
         end
 
         newstate(:groups) do
             desc "The groups of which the user is a member.  The primary
                 group should not be listed.  Multiple groups should be
                 specified as an array."
 
             def should_to_s
-                self.should.join(",")
+                self.should
             end
 
             def is_to_s
                 @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
+                    return @should.sort.join(",")
                 else
                     members = @should
                     if @is.is_a?(Array)
                         members += @is
                     end
-                    members.uniq.sort
+                    return members.uniq.sort.join(",")
                 end
             end
 
             def retrieve
                 if tmp = provider.groups
                     @is = tmp.split(",")
                 else
                     @is = :absent
                 end
             end
 
             def insync?
                 unless defined? @should and @should
                     return false
                 end
                 unless defined? @is and @is
                     return false
                 end
-                unless @is.class == @should.class
-                    return false
+                tmp = @is
+                if @is.is_a? Array
+                    tmp = @is.sort.join(",")
                 end
-                return @is.sort == @should.sort
+
+                return tmp == self.should
             end
 
             validate do |value|
                 if value =~ /^\d+$/
                     raise ArgumentError, "Group names must be provided, not numbers"
                 end
             end
-
-            def sync
-                provider.groups = self.should.join(",")
-                :user_changed
-            end
         end
 
         # these three states are all implemented differently on each platform,
         # so i'm disabling them for now
 
         # FIXME Puppet::State::UserLocked is currently non-functional
         #newstate(:locked) do
         #    desc "The expected return code.  An error will be returned if the
         #        executed command returns something else."
         #end
 
         # FIXME Puppet::State::UserExpire is currently non-functional
         #newstate(:expire) do
         #    desc "The expected return code.  An error will be returned if the
         #        executed command returns something else."
         #    @objectaddflag = "-e"
         #end
 
         # FIXME Puppet::State::UserInactive is currently non-functional
         #newstate(:inactive) do
         #    desc "The expected return code.  An error will be returned if the
         #        executed command returns something else."
         #    @objectaddflag = "-f"
         #end
 
         newparam(:name) do
             desc "User name.  While limitations are determined for
                 each operating system, it is generally a good idea to keep to
                 the degenerate 8 characters, beginning with a letter."
             isnamevar
         end
 
         newparam(:membership) do
             desc "Whether specified groups should be treated as the only groups
                 of which the user is a member or whether they should merely
                 be treated as the minimum membership list."
                 
             newvalues(:inclusive, :minimum)
 
             defaultto :minimum
         end
 
         newparam(:allowdupe) do
             desc "Whether to allow duplicate UIDs."
                 
             newvalues(:true, :false)
 
             defaultto false
         end
 
         @doc = "Manage users.  Currently can create and modify users, but
             cannot delete them.  Theoretically all of the parameters are
             optional, but if no parameters are specified the comment will
             be set to the user name in order to make the internals work out
             correctly.
             
             This element type uses the prescribed native tools for creating
             groups and generally uses POSIX APIs for retrieving information
             about them.  It does not directly modify /etc/passwd or anything.
             
             For most platforms, the tools used are ``useradd`` and its ilk;
             for Mac OS X, NetInfo is used.  This is currently unconfigurable,
             but if you desperately need it to be so, please contact us."
 
         # Autorequire the group, if it's around
         autorequire(:group) do
             #return nil unless @states.include?(:gid)
             #return nil unless groups = @states[:gid].shouldorig
             autos = []
 
             if @states.include?(:gid) and groups = @states[:gid].shouldorig
                 groups = groups.collect { |group|
                     if group =~ /^\d+$/
                         Integer(group)
                     else
                         group
                     end
                 }
                 groups.each { |group|
                     case group
                     when Integer:
                         if obj = Puppet.type(:group).find { |gobj|
                             gobj.should(:gid) == group
                         }
                             autos << obj
                             
                         end
                     else
                         autos << group
                     end
                 }
             end
 
-            if @states.include?(:groups) and groups = @states[:groups].should
+            if @states.include?(:groups) and groups = @states[:groups].should.split(",")
                 autos += groups
             end
 
             autos
         end
 
         autorequire(:file) do
             dir = self.should(:home) or self.is(:home)
             if dir =~ /^#{File::SEPARATOR}/
                 dir
             else
                 nil
             end
         end
 
         def self.list_by_name
             users = []
             defaultprovider.listbyname do |user|
                 users << user
             end
             return users
         end
 
         def self.list
             defaultprovider.list
 
             self.collect do |user|
                 user
             end
         end
 
         def retrieve
             absent = false
             states().each { |state|
                 if absent
                     state.is = :absent
                 else
                     state.retrieve
                 end
 
                 if state.name == :ensure and state.is == :absent
                     absent = true
                     next
                 end
             }
             #if provider.exists?
             #    super
             #else
             #    # the user does not exist
             #    @states.each { |name, state|
             #        state.is = :absent
             #    }
             #    return
             #end
         end
     end
 end
 
 # $Id$
diff --git a/test/types/user.rb b/test/types/user.rb
index 958cf9441..703004f57 100755
--- a/test/types/user.rb
+++ b/test/types/user.rb
@@ -1,436 +1,444 @@
 require 'etc'
 require 'puppet/type'
 require 'puppettest'
 
 class TestUser < Test::Unit::TestCase
 	include PuppetTest
 
     p = Puppet::Type.type(:user).provide :fake, :parent => PuppetTest::FakeProvider do
         @name = :fake
         apimethods
         def create
             @ensure = :present
             @model.eachstate do |state|
                 next if state.name == :ensure
                 state.sync
             end
         end
 
         def delete
             @ensure = :absent
             @model.eachstate do |state|
                 send(state.name.to_s + "=", :absent)
             end
         end
 
         def exists?
             if defined? @ensure and @ensure == :present
                 true
             else
                 false
             end
         end
     end
 
     FakeUserProvider = p
 
     @@fakeproviders[:group] = p
 
     def findshell(old = nil)
         %w{/bin/sh /bin/bash /sbin/sh /bin/ksh /bin/zsh /bin/csh /bin/tcsh
             /usr/bin/sh /usr/bin/bash /usr/bin/ksh /usr/bin/zsh /usr/bin/csh
             /usr/bin/tcsh}.find { |shell|
                 if old
                     FileTest.exists?(shell) and shell != old
                 else
                     FileTest.exists?(shell)
                 end
         }
     end
 
     def setup
         super
         Puppet::Type.type(:user).defaultprovider = FakeUserProvider
     end
 
     def teardown
         Puppet::Type.type(:user).defaultprovider = nil
         super
     end
 
     def mkuser(name)
         user = nil
         assert_nothing_raised {
             user = Puppet.type(:user).create(
                 :name => name,
                 :comment => "Puppet Testing User",
                 :gid => Process.gid,
                 :shell => findshell(),
                 :home => "/home/%s" % name
             )
         }
 
         assert(user, "Did not create user")
 
         return user
     end
 
     def attrtest_ensure(user)
         old = user.provider.ensure
         user[:ensure] = :absent
 
         comp = newcomp("ensuretest", user)
         assert_apply(user)
         assert(!user.provider.exists?, "User is still present")
         user[:ensure] = :present
         assert_events([:user_created], comp)
         assert(user.provider.exists?, "User is absent")
         user[:ensure] = :absent
         trans = assert_events([:user_removed], comp)
 
         assert_rollback_events(trans, [:user_created], "user")
 
         user[:ensure] = old
         assert_apply(user)
     end
 
     def attrtest_comment(user)
         user.retrieve
         old = user.provider.comment
         user[:comment] = "A different comment"
 
         comp = newcomp("commenttest", user)
 
         trans = assert_events([:user_changed], comp, "user")
 
         assert_equal("A different comment", user.provider.comment,
             "Comment was not changed")
 
         assert_rollback_events(trans, [:user_changed], "user")
 
         assert_equal(old, user.provider.comment,
             "Comment was not reverted")
     end
 
     def attrtest_home(user)
         obj = nil
         comp = newcomp("hometest", user)
 
         old = user.provider.home
         user[:home] = old
 
         trans = assert_events([], comp, "user")
 
         user[:home] = "/tmp"
 
         trans = assert_events([:user_changed], comp, "user")
 
         assert_equal("/tmp", user.provider.home, "Home was not changed")
 
         assert_rollback_events(trans, [:user_changed], "user")
 
         assert_equal(old, user.provider.home, "Home was not reverted")
     end
 
     def attrtest_shell(user)
         old = user.provider.shell
         comp = newcomp("shelltest", user)
 
         user[:shell] = old
 
         trans = assert_events([], comp, "user")
 
         newshell = findshell(old)
 
         unless newshell
             $stderr.puts "Cannot find alternate shell; skipping shell test"
             return
         end
 
         user[:shell] = newshell
 
         trans = assert_events([:user_changed], comp, "user")
 
         user.retrieve
         assert_equal(newshell, user.provider.shell,
             "Shell was not changed")
 
         assert_rollback_events(trans, [:user_changed], "user")
         user.retrieve
 
         assert_equal(old, user.provider.shell, "Shell was not reverted")
     end
 
     def attrtest_gid(user)
         obj = nil
         old = user.provider.gid
         comp = newcomp("gidtest", user)
 
         user.retrieve
 
         user[:gid] = old
 
         trans = assert_events([], comp, "user")
 
         newgid = %w{nogroup nobody staff users daemon}.find { |gid|
                 begin
                     group = Etc.getgrnam(gid)
                 rescue ArgumentError => detail
                     next
                 end
                 old != group.gid
         }
 
         unless newgid
             $stderr.puts "Cannot find alternate group; skipping gid test"
             return
         end
 
         # first test by name
         assert_nothing_raised("Failed to specify group by name") {
             user[:gid] = newgid
         }
 
         trans = assert_events([:user_changed], comp, "user")
 
         # then by id
         newgid = Etc.getgrnam(newgid).gid
 
         assert_nothing_raised("Failed to specify group by id") {
             user[:gid] = newgid
         }
 
         user.retrieve
 
         assert_events([], comp, "user")
 
         assert_equal(newgid, user.provider.gid, "GID was not changed")
 
         assert_rollback_events(trans, [:user_changed], "user")
 
         assert_equal(old, user.provider.gid, "GID was not reverted")
     end
 
     def attrtest_uid(user)
         obj = nil
         comp = newcomp("uidtest", user)
 
         user.provider.uid = 1
 
         old = 1
         newuid = 1
         while true
             newuid += 1
 
             if newuid - old > 1000
                 $stderr.puts "Could not find extra test UID"
                 return
             end
             begin
                 newuser = Etc.getpwuid(newuid)
             rescue ArgumentError => detail
                 break
             end
         end
 
         assert_nothing_raised("Failed to change user id") {
             user[:uid] = newuid
         }
 
         trans = assert_events([:user_changed], comp, "user")
 
         assert_equal(newuid, user.provider.uid, "UID was not changed")
 
         assert_rollback_events(trans, [:user_changed], "user")
 
         assert_equal(old, user.provider.uid, "UID was not reverted")
     end
 
     def attrtest_groups(user)
         Etc.setgrent
         max = 0
         while group = Etc.getgrent
             if group.gid > max and group.gid < 5000
                 max = group.gid
             end
         end
 
         groups = []
         main = []
         extra = []
         5.times do |i|
             i += 1
             name = "pptstgr%s" % i
             groups << name
             if i < 3
                 main << name
             else
                 extra << name
             end
         end
 
         assert(user[:membership] == :minimum, "Membership did not default correctly")
 
         assert_nothing_raised {
             user.retrieve
         }
 
         # Now add some of them to our user
         assert_nothing_raised {
             user[:groups] = extra
         }
         assert_nothing_raised {
             user.retrieve
         }
 
+        assert_instance_of(String, user.state(:groups).should)
+
         # Some tests to verify that groups work correctly startig from nothing
         # Remove our user
         user[:ensure] = :absent
         assert_apply(user)
 
         assert_nothing_raised do
             user.retrieve
         end
 
         # And add it again
         user[:ensure] = :present
         assert_apply(user)
 
         # Make sure that the groups are a string, not an array
         assert(user.provider.groups.is_a?(String),
             "Incorrectly passed an array to groups")
 
         user.retrieve
 
         assert(user.state(:groups).is, "Did not retrieve group list")
 
         list = user.state(:groups).is
         assert_equal(extra.sort, list.sort, "Group list is not equal")
 
         # Now set to our main list of groups
         assert_nothing_raised {
             user[:groups] = main
         }
 
-        assert_equal((main + extra).sort, user.state(:groups).should.sort)
+        assert_equal((main + extra).sort, user.state(:groups).should.split(",").sort)
 
         assert_nothing_raised {
             user.retrieve
         }
 
         assert(!user.insync?, "User is incorrectly in sync")
 
         assert_apply(user)
 
         assert_nothing_raised {
             user.retrieve
         }
 
         # We're not managing inclusively, so it should keep the old group
         # memberships and add the new ones
         list = user.state(:groups).is
         assert_equal((main + extra).sort, list.sort, "Group list is not equal")
 
         assert_nothing_raised {
             user[:membership] = :inclusive
         }
         assert_nothing_raised {
             user.retrieve
         }
 
         assert(!user.insync?, "User is incorrectly in sync")
 
         assert_events([:user_changed], user)
         assert_nothing_raised {
             user.retrieve
         }
 
         list = user.state(:groups).is
         assert_equal(main.sort, list.sort, "Group list is not equal")
 
+        # Set the values a bit differently.
+        user.state(:groups).should = list.sort { |a,b| b <=> a }
+        user.state(:groups).is = list.sort
+
+        assert(user.state(:groups).insync?, "Groups state did not sort groups")
+
         user.delete(:groups)
     end
 
     def test_autorequire
         file = tempfile()
         comp = nil
         user = nil
         group =nil
         home = nil
         ogroup = nil
         assert_nothing_raised {
             user = Puppet.type(:user).create(
                 :name => "pptestu",
                 :home => file,
                 :gid => "pptestg",
                 :groups => "yayness"
             )
             home = Puppet.type(:file).create(
                 :path => file,
                 :ensure => "directory"
             )
             group = Puppet.type(:group).create(
                 :name => "pptestg"
             )
             ogroup = Puppet.type(:group).create(
                 :name => "yayness"
             )
             comp = newcomp(user, group, home, ogroup)
         }
         comp.finalize
         comp.retrieve
 
         assert(user.requires?(group), "User did not require group")
         assert(user.requires?(home), "User did not require home dir")
         assert(user.requires?(ogroup), "User did not require other groups")
     end
 
     def test_simpleuser
         name = "pptest"
 
         user = mkuser(name)
 
         comp = newcomp("usercomp", user)
 
         trans = assert_events([:user_created], comp, "user")
 
         assert_equal(user.should(:comment), user.provider.comment,
             "Comment was not set correctly")
 
         assert_rollback_events(trans, [:user_removed], "user")
 
         assert(! user.provider.exists?, "User did not get deleted")
     end
 
     def test_allusermodelstates
         user = nil
         name = "pptest"
 
         user = mkuser(name)
 
         assert(! user.provider.exists?, "User %s is present" % name)
 
         comp = newcomp("usercomp", user)
 
         trans = assert_events([:user_created], comp, "user")
 
         user.retrieve
         assert_equal("Puppet Testing User", user.provider.comment,
             "Comment was not set")
 
         tests = Puppet.type(:user).validstates
 
         tests.each { |test|
             if self.respond_to?("attrtest_%s" % test)
                 self.send("attrtest_%s" % test, user)
             else
                 Puppet.err "Not testing attr %s of user" % test
             end
         }
 
         user[:ensure] = :absent
         assert_apply(user)
     end
 end
 
 # $Id$