Fixes #4822 -- Puppet doc -o option broken

The global "-o" option ("--onetime") was overriding the
application-specific option "-o" because global options were being
sent to the OptionParser after application-specific options.

Modified the order in which options are sent to the OptionParser to
have the correct behavior.  Also merged together the two methods that
were applying options so that the order is more explicit.

3 files changed, 40 insertions, 46 deletions

diff --git a/lib/puppet/application.rb b/lib/puppet/application.rb
index 2fec38bf2..f0159a65d 100644
--- a/lib/puppet/application.rb
+++ b/lib/puppet/application.rb
@@ -254,19 +254,6 @@ class Application
   def preinit
   end
 
-  def option_parser
-    return @option_parser if defined?(@option_parser)
-
-    @option_parser = OptionParser.new(self.class.banner)
-
-    self.class.option_parser_commands.each do |options, fname|
-      @option_parser.on(*options) do |value|
-        self.send(fname, value)
-      end
-    end
-    @option_parser
-  end
-
   def initialize(command_line = nil)
     require 'puppet/util/command_line'
     @command_line = command_line || Puppet::Util::CommandLine.new
@@ -323,20 +310,29 @@ class Application
   end
 
   def parse_options
-    # get all puppet options
-    optparse_opt = []
-    optparse_opt = Puppet.settings.optparse_addargs(optparse_opt)
+    # Create an option parser
+    option_parser = OptionParser.new(self.class.banner)
 
-    # convert them to OptionParser format
-    optparse_opt.each do |option|
-      self.option_parser.on(*option) do |arg|
+    # Add all global options to it.
+    Puppet.settings.optparse_addargs([]).each do |option|
+      option_parser.on(*option) do |arg|
         handlearg(option[0], arg)
       end
     end
 
-    # scan command line argument
+    # Add options that are local to this application, which were
+    # created using the "option()" metaprogramming method.  If there
+    # are any conflicts, this application's options will be favored.
+    self.class.option_parser_commands.each do |options, fname|
+      option_parser.on(*options) do |value|
+        # Call the method that "option()" created.
+        self.send(fname, value)
+      end
+    end
+
+    # scan command line.
     begin
-      self.option_parser.parse!(self.command_line.args)
+      option_parser.parse!(self.command_line.args)
     rescue OptionParser::ParseError => detail
       $stderr.puts detail
       $stderr.puts "Try 'puppet #{command_line.subcommand_name} --help'"
diff --git a/spec/integration/application/doc_spec.rb b/spec/integration/application/doc_spec.rb
index cb9f47868..eaf5442a0 100644
--- a/spec/integration/application/doc_spec.rb
+++ b/spec/integration/application/doc_spec.rb
@@ -45,4 +45,11 @@ describe Puppet::Application::Doc do
       Dir.chdir(old_dir)
     end
   end
+
+  it "should respect the -o option" do
+    puppetdoc = Puppet::Application[:doc]
+    puppetdoc.command_line.stubs(:args).returns(['foo', '-o', 'bar'])
+    puppetdoc.parse_options
+    puppetdoc.options[:outputdir].should == 'bar'
+  end
 end
diff --git a/spec/unit/application_spec.rb b/spec/unit/application_spec.rb
index 433809172..be7cda340 100755
--- a/spec/unit/application_spec.rb
+++ b/spec/unit/application_spec.rb
@@ -191,24 +191,17 @@ describe Puppet::Application do
       Puppet.settings.stubs(:optparse_addargs).returns([])
     end
 
-    it "should create a new option parser when needed" do
-      option_parser = stub "option parser"
-      option_parser.stubs(:on)
-      OptionParser.expects(:new).returns(option_parser).once
-      @app.option_parser.should == option_parser
-      @app.option_parser.should == option_parser
-    end
-
     it "should pass the banner to the option parser" do
       option_parser = stub "option parser"
       option_parser.stubs(:on)
+      option_parser.stubs(:parse!)
       @app.class.instance_eval do
         banner "banner"
       end
 
       OptionParser.expects(:new).with("banner").returns(option_parser)
 
-      @app.option_parser
+      @app.parse_options
     end
 
     it "should get options from Puppet.settings.optparse_addargs" do
@@ -219,15 +212,14 @@ describe Puppet::Application do
 
     it "should add Puppet.settings options to OptionParser" do
       Puppet.settings.stubs(:optparse_addargs).returns( [["--option","-o", "Funny Option"]])
-
-      @app.option_parser.expects(:on).with { |*arg| arg == ["--option","-o", "Funny Option"] }
-
+      Puppet.settings.expects(:handlearg).with("--option", 'true')
+      @app.command_line.stubs(:args).returns(["--option"])
       @app.parse_options
     end
 
     it "should ask OptionParser to parse the command-line argument" do
       @app.command_line.stubs(:args).returns(%w{ fake args })
-      @app.option_parser.expects(:parse!).with(%w{ fake args })
+      OptionParser.any_instance.expects(:parse!).with(%w{ fake args })
 
       @app.parse_options
     end
@@ -258,31 +250,30 @@ describe Puppet::Application do
 
     describe "when dealing with an argument not declared directly by the application" do
       it "should pass it to handle_unknown if this method exists" do
-        Puppet.settings.stubs(:optparse_addargs).returns([["--not-handled"]])
-        @app.option_parser.stubs(:on).yields("value")
+        Puppet.settings.stubs(:optparse_addargs).returns([["--not-handled", :REQUIRED]])
 
         @app.expects(:handle_unknown).with("--not-handled", "value").returns(true)
-
+        @app.command_line.stubs(:args).returns(["--not-handled", "value"])
         @app.parse_options
       end
 
       it "should pass it to Puppet.settings if handle_unknown says so" do
-        Puppet.settings.stubs(:optparse_addargs).returns([["--topuppet"]])
-        @app.option_parser.stubs(:on).yields("value")
+        Puppet.settings.stubs(:optparse_addargs).returns([["--topuppet", :REQUIRED]])
 
         @app.stubs(:handle_unknown).with("--topuppet", "value").returns(false)
 
         Puppet.settings.expects(:handlearg).with("--topuppet", "value")
+        @app.command_line.stubs(:args).returns(["--topuppet", "value"])
         @app.parse_options
       end
 
       it "should pass it to Puppet.settings if there is no handle_unknown method" do
-        Puppet.settings.stubs(:optparse_addargs).returns([["--topuppet"]])
-        @app.option_parser.stubs(:on).yields("value")
+        Puppet.settings.stubs(:optparse_addargs).returns([["--topuppet", :REQUIRED]])
 
         @app.stubs(:respond_to?).returns(false)
 
         Puppet.settings.expects(:handlearg).with("--topuppet", "value")
+        @app.command_line.stubs(:args).returns(["--topuppet", "value"])
         @app.parse_options
       end
 
@@ -310,7 +301,7 @@ describe Puppet::Application do
 
     it "should exit if OptionParser raises an error" do
       $stderr.stubs(:puts)
-      @app.option_parser.stubs(:parse!).raises(OptionParser::ParseError.new("blah blah"))
+      OptionParser.any_instance.stubs(:parse!).raises(OptionParser::ParseError.new("blah blah"))
 
       @app.expects(:exit)
 
@@ -478,7 +469,7 @@ describe Puppet::Application do
           @app.class.option("--[no-]test3","-t") do
           end
 
-          @app.option_parser
+          @app.parse_options
         end
 
         it "should pass a block that calls our defined method" do
@@ -490,7 +481,7 @@ describe Puppet::Application do
           @app.class.option("--test4","-t") do
           end
 
-          @app.option_parser
+          @app.parse_options
         end
       end
 
@@ -501,7 +492,7 @@ describe Puppet::Application do
 
           @app.class.option("--test4","-t")
 
-          @app.option_parser
+          @app.parse_options
         end
 
         it "should give to OptionParser a block that adds the the value to the options array" do
@@ -512,7 +503,7 @@ describe Puppet::Application do
 
           @app.class.option("--test4","-t")
 
-          @app.option_parser
+          @app.parse_options
         end
       end
     end