Merge branch 'next'

* next:
  (#6928) Don't blow up when the method is undefined...
  (#6928) backport Symbol#to_proc for Ruby < 1.8.7
  (#7183) Implement "invisible glob" version matching for faces
  maint: better disabling of Signal#trap in our tests.
  maint: more robust listing of valid faces.
  maint: clean up testing code a fraction...
  maint: better error report for a missing version of a face.
  maint: handle face clear/reset sanely in the interface spec.
  maint: stop stubbing log level setting.
  Move tests from Puppet-acceptance repo
  (#7116) Handle application-level options in parse_options
  maint: fix gratuitous whitespace in the code.
  maint: remove redundant context from the test.
  (#7062) better argument handling in the action wrapper methods
  maint: move method comments outside the comment.
  Fixed #7166 - Replaced deprecated stomp "send" method with "publish"
  maint: Remove unused faces code

8 files changed, 118 insertions, 92 deletions

diff --git a/lib/puppet/application/face_base.rb b/lib/puppet/application/face_base.rb
index 9da48af55..7bebd18bb 100644
--- a/lib/puppet/application/face_base.rb
+++ b/lib/puppet/application/face_base.rb
@@ -92,8 +92,8 @@ class Puppet::Application::FaceBase < Puppet::Application
 
     # REVISIT: These should be configurable versions, through a global
     # '--version' option, but we don't implement that yet... --daniel 2011-03-29
-    @type      = self.class.name.to_s.sub(/.+:/, '').downcase.to_sym
-    @face      = Puppet::Face[@type, :current]
+    @type = self.class.name.to_s.sub(/.+:/, '').downcase.to_sym
+    @face = Puppet::Face[@type, :current]
 
     # Now, walk the command line and identify the action.  We skip over
     # arguments based on introspecting the action and all, and find the first
@@ -122,6 +122,8 @@ class Puppet::Application::FaceBase < Puppet::Application
             # a mandatory argument. --daniel 2011-04-05
             index += 1          # ...so skip the argument.
           end
+        elsif option = find_application_argument(item) then
+          index += 1 if (option[:argument] and option[:optional])
         else
           raise OptionParser::InvalidOption.new(item.sub(/=.*$/, ''))
         end
@@ -158,6 +160,21 @@ class Puppet::Application::FaceBase < Puppet::Application
     return nil                  # nothing found.
   end
 
+  def find_application_argument(item)
+    self.class.option_parser_commands.each do |options, function|
+      options.each do |option|
+        next unless option =~ /^-/
+        pattern = /^#{option.sub('[no-]', '').sub(/[ =].*$/, '')}(?:[ =].*)?$/
+        next unless pattern.match(item)
+        return {
+          :argument => option =~ /[ =]/,
+          :optional => option =~ /[ =]\[/
+        }
+      end
+    end
+    return nil                  # not found
+  end
+
   def setup
     Puppet::Util::Log.newdestination :console
 
diff --git a/lib/puppet/faces/help/action.erb b/lib/puppet/faces/help/action.erb
deleted file mode 100644
index eaf131464..000000000
--- a/lib/puppet/faces/help/action.erb
+++ /dev/null
@@ -1,3 +0,0 @@
-Use: puppet <%= face.name %> [options] <%= action.name %> [options]
-
-Summary: <%= action.summary %>
diff --git a/lib/puppet/faces/help/face.erb b/lib/puppet/faces/help/face.erb
deleted file mode 100644
index efe5fd809..000000000
--- a/lib/puppet/faces/help/face.erb
+++ /dev/null
@@ -1,7 +0,0 @@
-Use: puppet <%= face.name %> [options] <action> [options]
-
-Available actions:
-% face.actions.each do |actionname|
-%   action = face.get_action(actionname)
-  <%= action.name.to_s.ljust(16) %>  <%= action.summary %>
-% end
diff --git a/lib/puppet/faces/help/global.erb b/lib/puppet/faces/help/global.erb
deleted file mode 100644
index e123367a2..000000000
--- a/lib/puppet/faces/help/global.erb
+++ /dev/null
@@ -1,20 +0,0 @@
-puppet <subcommand> [options] <action> [options]
-
-Available subcommands, from Puppet Faces:
-% Puppet::Faces.faces.sort.each do |name|
-%   face = Puppet::Faces[name, :current]
-  <%= face.name.to_s.ljust(16) %>  <%= face.summary %>
-% end
-
-% unless legacy_applications.empty? then # great victory when this is true!
-Available applications, soon to be ported to Faces:
-%   legacy_applications.each do |appname|
-%     summary = horribly_extract_summary_from appname
-  <%= appname.to_s.ljust(16) %>  <%= summary %>
-%   end
-% end
-
-See 'puppet help <subcommand> <action>' for help on a specific subcommand action.
-See 'puppet help <subcommand>' for help on a specific subcommand.
-See 'puppet man  <subcommand>' for the full man page.
-Puppet v<%= Puppet::PUPPETVERSION %>
diff --git a/lib/puppet/interface.rb b/lib/puppet/interface.rb
index 5c8ade749..ced00863d 100644
--- a/lib/puppet/interface.rb
+++ b/lib/puppet/interface.rb
@@ -26,18 +26,13 @@ class Puppet::Interface
       Puppet::Interface::FaceCollection.faces
     end
 
-    def face?(name, version)
-      Puppet::Interface::FaceCollection.face?(name, version)
-    end
-
     def register(instance)
       Puppet::Interface::FaceCollection.register(instance)
     end
 
     def define(name, version, &block)
-      if face?(name, version)
-        face = Puppet::Interface::FaceCollection[name, version]
-      else
+      face = Puppet::Interface::FaceCollection[name, version]
+      if face.nil? then
         face = self.new(name, version)
         Puppet::Interface::FaceCollection.register(face)
         # REVISIT: Shouldn't this be delayed until *after* we evaluate the
@@ -50,10 +45,14 @@ class Puppet::Interface
       return face
     end
 
+    def face?(name, version)
+      Puppet::Interface::FaceCollection[name, version]
+    end
+
     def [](name, version)
       unless face = Puppet::Interface::FaceCollection[name, version]
         if current = Puppet::Interface::FaceCollection[name, :current]
-          raise Puppet::Error, "Could not find version #{version} of #{current}"
+          raise Puppet::Error, "Could not find version #{version} of #{name}"
         else
           raise Puppet::Error, "Could not find Puppet Face #{name.inspect}"
         end
diff --git a/lib/puppet/interface/action.rb b/lib/puppet/interface/action.rb
index 23366b407..08bc0a345 100644
--- a/lib/puppet/interface/action.rb
+++ b/lib/puppet/interface/action.rb
@@ -129,38 +129,59 @@ class Puppet::Interface::Action
   #   @face.send(name, *args, &block)
   # end
 
+
+  # We need to build an instance method as a wrapper, using normal code, to be
+  # able to expose argument defaulting between the caller and definer in the
+  # Ruby API.  An extra method is, sadly, required for Ruby 1.8 to work since
+  # it doesn't expose bind on a block.
+  #
+  # Hopefully we can improve this when we finally shuffle off the last of Ruby
+  # 1.8 support, but that looks to be a few "enterprise" release eras away, so
+  # we are pretty stuck with this for now.
+  #
+  # Patches to make this work more nicely with Ruby 1.9 using runtime version
+  # checking and all are welcome, provided that they don't change anything
+  # outside this little ol' bit of code and all.
+  #
+  # Incidentally, we though about vendoring evil-ruby and actually adjusting
+  # the internal C structure implementation details under the hood to make
+  # this stuff work, because it would have been cleaner.  Which gives you an
+  # idea how motivated we were to make this cleaner.  Sorry.
+  # --daniel 2011-03-31
   def when_invoked=(block)
-    # We need to build an instance method as a wrapper, using normal code, to
-    # be able to expose argument defaulting between the caller and definer in
-    # the Ruby API.  An extra method is, sadly, required for Ruby 1.8 to work.
-    #
-    # In future this also gives us a place to hook in additional behaviour
-    # such as calling out to the action instance to validate and coerce
-    # parameters, which avoids any exciting context switching and all.
-    #
-    # Hopefully we can improve this when we finally shuffle off the last of
-    # Ruby 1.8 support, but that looks to be a few "enterprise" release eras
-    # away, so we are pretty stuck with this for now.
-    #
-    # Patches to make this work more nicely with Ruby 1.9 using runtime
-    # version checking and all are welcome, but they can't actually help if
-    # the results are not totally hidden away in here.
-    #
-    # Incidentally, we though about vendoring evil-ruby and actually adjusting
-    # the internal C structure implementation details under the hood to make
-    # this stuff work, because it would have been cleaner.  Which gives you an
-    # idea how motivated we were to make this cleaner.  Sorry. --daniel 2011-03-31
 
     internal_name = "#{@name} implementation, required on Ruby 1.8".to_sym
-    file    = __FILE__ + "+eval"
-    line    = __LINE__ + 1
+
+    arity = block.arity
+    if arity == 0 then
+      # This will never fire on 1.8.7, which treats no arguments as "*args",
+      # but will on 1.9.2, which treats it as "no arguments".  Which bites,
+      # because this just begs for us to wind up in the horrible situation
+      # where a 1.8 vs 1.9 error bites our end users. --daniel 2011-04-19
+      raise ArgumentError, "action when_invoked requires at least one argument (options)"
+    elsif arity > 0 then
+      range = Range.new(1, arity - 1)
+      decl = range.map { |x| "arg#{x}" } << "options = {}"
+      optn = ""
+      args = "[" + (range.map { |x| "arg#{x}" } << "options").join(", ") + "]"
+    else
+      range = Range.new(1, arity.abs - 1)
+      decl = range.map { |x| "arg#{x}" } << "*rest"
+      optn = "rest << {} unless rest.last.is_a?(Hash)"
+      if arity == -1 then
+        args = "rest"
+      else
+        args = "[" + range.map { |x| "arg#{x}" }.join(", ") + "] + rest"
+      end
+    end
+
+    file    = __FILE__ + "+eval[wrapper]"
+    line    = __LINE__ + 2 # <== points to the same line as 'def' in the wrapper.
     wrapper = <<WRAPPER
-def #{@name}(*args)
-  if args.last.is_a? Hash then
-    options = args.last
-  else
-    args << (options = {})
-  end
+def #{@name}(#{decl.join(", ")})
+  #{optn}
+  args = #{args}
+  options = args.last
 
   action = get_action(#{name.inspect})
   action.validate_args(args)
diff --git a/lib/puppet/interface/face_collection.rb b/lib/puppet/interface/face_collection.rb
index 591471d4b..6e6afc545 100644
--- a/lib/puppet/interface/face_collection.rb
+++ b/lib/puppet/interface/face_collection.rb
@@ -24,19 +24,21 @@ module Puppet::Interface::FaceCollection
         end
       end
     end
-    return @faces.keys
+    return @faces.keys.select {|name| @faces[name].length > 0 }
   end
 
   def self.validate_version(version)
     !!(SEMVER_VERSION =~ version.to_s)
   end
 
+  def self.semver_to_array(v)
+    parts = SEMVER_VERSION.match(v).to_a[1..4]
+    parts[0..2] = parts[0..2].map { |e| e.to_i }
+    parts
+  end
+
   def self.cmp_semver(a, b)
-    a, b = [a, b].map do |x|
-      parts = SEMVER_VERSION.match(x).to_a[1..4]
-      parts[0..2] = parts[0..2].map { |e| e.to_i }
-      parts
-    end
+    a, b = [a, b].map do |x| semver_to_array(x) end
 
     cmp = a[0..2] <=> b[0..2]
     if cmp == 0
@@ -47,18 +49,38 @@ module Puppet::Interface::FaceCollection
     cmp
   end
 
-  def self.[](name, version)
-    @faces[underscorize(name)][version] if face?(name, version)
+  def self.prefix_match?(desired, target)
+    # Can't meaningfully do a prefix match with current on either side.
+    return false if desired == :current
+    return false if target  == :current
+
+    # REVISIT: Should probably fail if the matcher is not valid.
+    prefix = desired.split('.').map {|x| x =~ /^\d+$/ and x.to_i }
+    have   = semver_to_array(target)
+
+    while want = prefix.shift do
+      return false unless want == have.shift
+    end
+    return true
   end
 
-  def self.face?(name, version)
+  def self.[](name, version)
     name = underscorize(name)
+    get_face(name, version) or load_face(name, version)
+  end
 
-    # Note: be careful not to accidentally create the top level key, either,
-    # because it will result in confusion when people try to enumerate the
-    # list of valid faces later. --daniel 2011-04-11
-    return true if @faces.has_key?(name) and @faces[name].has_key?(version)
+  # get face from memory, without loading.
+  def self.get_face(name, desired_version)
+    return nil unless @faces.has_key? name
 
+    return @faces[name][:current] if desired_version == :current
+
+    found = @faces[name].keys.select {|v| prefix_match?(desired_version, v) }.sort.last
+    return @faces[name][found]
+  end
+
+  # try to load the face, and return it.
+  def self.load_face(name, version)
     # We always load the current version file; the common case is that we have
     # the expected version and any compatibility versions in the same file,
     # the default.  Which means that this is almost always the case.
@@ -104,17 +126,7 @@ module Puppet::Interface::FaceCollection
       # ...guess we didn't find the file; return a much better problem.
     end
 
-    # Now, either we have the version in our set of faces, or we didn't find
-    # the version they were looking for.  In the future we will support
-    # loading versioned stuff from some look-aside part of the Ruby load path,
-    # but we don't need that right now.
-    #
-    # So, this comment is a place-holder for that.  --daniel 2011-04-06
-    #
-    # Note: be careful not to accidentally create the top level key, either,
-    # because it will result in confusion when people try to enumerate the
-    # list of valid faces later. --daniel 2011-04-11
-    return !! (@faces.has_key?(name) and @faces[name].has_key?(version))
+    return get_face(name, version)
   end
 
   def self.register(face)
diff --git a/lib/puppet/util/monkey_patches.rb b/lib/puppet/util/monkey_patches.rb
index 10a268409..bd954c665 100644
--- a/lib/puppet/util/monkey_patches.rb
+++ b/lib/puppet/util/monkey_patches.rb
@@ -104,3 +104,10 @@ class Array
     end
   end unless method_defined? :combination
 end
+
+
+class Symbol
+  def to_proc
+    Proc.new { |*args| args.shift.__send__(self, *args) }
+  end unless method_defined? :to_proc
+end