(#6749) Fix optional vs mandatory argument handling.

optparse will treat '--foo --bar' as "foo with the argument --bar" when foo
takes a mandatory argument.  We need to emulate that behaviour in our
pre-parse of the command line.

Incidentally, fix up a bug in boolean options, and improve our testing.

Reviewed-By: Nick Lewis <nick@puppetlabs.com>

3 files changed, 35 insertions, 16 deletions

diff --git a/lib/puppet/application/string_base.rb b/lib/puppet/application/string_base.rb
index 6032e32f8..a082ba0e2 100644
--- a/lib/puppet/application/string_base.rb
+++ b/lib/puppet/application/string_base.rb
@@ -65,16 +65,15 @@ class Puppet::Application::StringBase < Puppet::Application
     # non-option word to use as the action.
     action = nil
     index  = -1
-    while (index += 1) < command_line.args.length do
+    until @action or (index += 1) >= command_line.args.length do
       item = command_line.args[index]
       if item =~ /^-/ then
         option = @string.options.find { |a| item =~ /^-+#{a}\b/ }
         if option then
-          if @string.get_option(option).takes_argument? then
-            # We don't validate if the argument is optional or mandatory,
-            # because it doesn't matter here.  We just assume that errors will
-            # be caught later. --daniel 2011-03-30
-            index += 1 unless command_line.args[index + 1] =~ /^-/
+          option = @string.get_option(option)
+          if option.takes_argument? then
+            index += 1 unless
+              (option.optional_argument? and command_line.args[index + 1] =~ /^-/)
           end
         else
           raise ArgumentError, "Unknown option #{item.sub(/=.*$/, '').inspect}"
diff --git a/lib/puppet/string/option.rb b/lib/puppet/string/option.rb
index e7b6f187c..f499e4b95 100644
--- a/lib/puppet/string/option.rb
+++ b/lib/puppet/string/option.rb
@@ -63,7 +63,8 @@ class Puppet::String::Option
   end
 
   # to_s and optparse_to_name are roughly mirrored, because they are used to
-  # transform strings to name symbols, and vice-versa.
+  # transform strings to name symbols, and vice-versa.  This isn't a full
+  # bidirectional transformation though.
   def to_s
     @name.to_s.tr('_', '-')
   end
@@ -72,7 +73,7 @@ class Puppet::String::Option
     unless found = declaration.match(/^-+([^= ]+)/) or found.length != 1 then
       raise ArgumentError, "Can't find a name in the declaration #{declaration.inspect}"
     end
-    name = found.captures.first.tr('-', '_')
+    name = found.captures.first.sub('[no-]', '').tr('-', '_')
     raise "#{name.inspect} is an invalid option name" unless name.to_s =~ /^[a-z]\w*$/
     name.to_sym
   end
diff --git a/spec/unit/application/string_base_spec.rb b/spec/unit/application/string_base_spec.rb
index 62869fe61..7f06c05f4 100755
--- a/spec/unit/application/string_base_spec.rb
+++ b/spec/unit/application/string_base_spec.rb
@@ -5,7 +5,6 @@ require 'puppet/application/string_base'
 require 'tmpdir'
 
 class Puppet::Application::StringBase::Basetest < Puppet::Application::StringBase
-  option("--[no-]foo")
 end
 
 describe Puppet::Application::StringBase do
@@ -17,9 +16,15 @@ describe Puppet::Application::StringBase do
       f.puts "Puppet::String.define(:basetest, '0.0.1')"
     end
 
-    Puppet::String[:basetest, '0.0.1'].action :foo do
-      option "--foo"
-      invoke { |*args| args.length }
+    Puppet::String.define(:basetest, '0.0.1') do
+      option("--[no-]boolean")
+      option("--mandatory MANDATORY")
+      option("--optional [OPTIONAL]")
+
+      action :foo do
+        option("--action")
+        invoke { |*args| args.length }
+      end
     end
   end
 
@@ -69,15 +74,15 @@ describe Puppet::Application::StringBase do
       end
 
       it "should report a sensible error when options with = fail" do
-        app.command_line.stubs(:args).returns %w{--foo=bar foo}
+        app.command_line.stubs(:args).returns %w{--action=bar foo}
         expect { app.preinit }.
-          should raise_error ArgumentError, /Unknown option "--foo"/
+          should raise_error ArgumentError, /Unknown option "--action"/
       end
 
       it "should fail if an action option is before the action" do
-        app.command_line.stubs(:args).returns %w{--foo foo}
+        app.command_line.stubs(:args).returns %w{--action foo}
         expect { app.preinit }.
-          should raise_error ArgumentError, /Unknown option "--foo"/
+          should raise_error ArgumentError, /Unknown option "--action"/
       end
 
       it "should fail if an unknown option is before the action" do
@@ -93,6 +98,20 @@ describe Puppet::Application::StringBase do
         app.string.should_not be_option :bar
         app.action.should_not be_option :bar
       end
+
+      it "should accept --bar as an argument to a mandatory option after action" do
+        app.command_line.stubs(:args).returns %w{foo --mandatory --bar}
+        app.preinit and app.parse_options
+        app.action.name.should == :foo
+        app.options.should == { :mandatory => "--bar" }
+      end
+
+      it "should accept --bar as an argument to a mandatory option before action" do
+        app.command_line.stubs(:args).returns %w{--mandatory --bar foo}
+        app.preinit and app.parse_options
+        app.action.name.should == :foo
+        app.options.should == { :mandatory => "--bar" }
+      end
     end
   end