fix alphabetic sorting of classeslcg/node-map-speedup

but we really shouldn't do this...

Signed-off-by: Lamont Granquist <lamont@scriptkiddie.org>

2 files changed, 22 insertions, 8 deletions

diff --git a/lib/chef/node_map.rb b/lib/chef/node_map.rb
index f650e56c46..0d7d986653 100644
--- a/lib/chef/node_map.rb
+++ b/lib/chef/node_map.rb
@@ -221,21 +221,19 @@ class Chef
       if cmp == 0
         # Sort by class name (ascending) as well, if all other properties
         # are exactly equal
+        # XXX: remove this in Chef-14 and use last-writer-wins (prepend if they match)
         if new_matcher[:value].is_a?(Class) && !new_matcher[:override]
-          cmp = compare_matcher_properties(new_matcher, matcher) { |m| m[:value].name }
+          cmp = compare_matcher_properties(new_matcher[:value].name, matcher[:value].name)
         end
       end
       cmp
     end
 
     def compare_matcher_properties(a, b)
-      # We treat false / true and nil / not-nil with the same comparison
-      a = nil if a == false
-      b = nil if b == false
-
-      return 1 if a.nil? && !b.nil?
-      return -1 if b.nil? && !a.nil?
-      return 0 if a.nil? && b.nil?
+      # falsity comparisons here handle both "nil" and "false"
+      return 1 if !a && b
+      return -1 if !b && a
+      return 0 if !a && !b
 
       # Check for blacklists ('!windows'). Those always come *after* positive
       # whitelists.
diff --git a/spec/unit/node_map_spec.rb b/spec/unit/node_map_spec.rb
index 64106323c2..7fa115b532 100644
--- a/spec/unit/node_map_spec.rb
+++ b/spec/unit/node_map_spec.rb
@@ -119,6 +119,22 @@ describe Chef::NodeMap do
     end
   end
 
+  describe "ordering classes" do
+    class Foo; end
+    class Bar; end
+    it "orders them alphabetically when they're set in the reverse order" do
+      node_map.set(:thing, Foo)
+      node_map.set(:thing, Bar)
+      expect(node_map.get(node, :thing)).to eql(Bar)
+    end
+
+    it "orders them alphabetically when they're set in alphabetic order" do
+      node_map.set(:thing, Bar)
+      node_map.set(:thing, Foo)
+      expect(node_map.get(node, :thing)).to eql(Bar)
+    end
+  end
+
   describe "with a block doing platform_version checks" do
     before do
       node_map.set(:thing, :foo, platform_family: "rhel") do |node|