Mailing-List: contact dev-help@commons.apache.org; run by ezmlm
Precedence: bulk
Reply-To: "Commons Developers List" <dev@commons.apache.org>
Received-SPF: neutral (athena.apache.org: local policy)
Message-ID: <48142A6F.1040406@home.paulcager.org>
Date: Sun, 27 Apr 2008 08:25:35 +0100
From: Paul Cager <paul-debian@home.paulcager.org>
User-Agent: Mozilla-Thunderbird 2.0.0.9 (X11/20080110)
MIME-Version: 1.0
To: dev@commons.apache.org
Subject: [cli] Issue 137 - Can I help?
Content-Type: multipart/mixed;
 boundary="------------010807020604070306040901"

--------------010807020604070306040901
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

Hi,

I've fallen over the CLI bug described in issue 137, and I'm wondering
if I can help provide a patch for it? I'd appreciate someone just
checking my logic in case I'm missing something obvious. I'm using
release 1.1, by the way.

As far as I can see it is simply not possible to use commons-cli to
parse repeating options, such as occur in ant:

      ant -Dproperty1=value1 -Dproperty2=value2 compile

Cli *does* have a junit test for ant-type options, but it unfortunately
doesn't catch the error because it tests the command line:

      ant -Dproperty1=value1 -Dproperty2=value2 -projecthelp

I attach a patch for the junit test (Cli-junit.patch) that will provoke 
the error, if you want to try it yourself.


_Here's what I think is happening:_

The unit test (and the work-around suggested in issue 137) creates the
"D" option using "hasArgs()" [note the 's' on the end of the method
name]. Using hasArgs() for the "D" option causes cli to assume that all
arguments following the "-D" (until it encounters another option) belong
to the "-D" option. I've probably not explained that clearly so here's
an example:

      ant -Dproperty1=value1 -Dproperty2=value2 compile

will treat "compile" as though it was "-Dcompile". getArgs() will return
0 elements, rather than one ("compile").

If instead you construct the "D" option using hasArg() [singular] then
it will *not* interpret the second "-D" as an option. getArgs() will
return 2 elements: "property2=value2" and "compile". getOptionValues()
will return only the first -D value.

This problem first came to light when Debian upgraded CLI from 1.0 to 
1.1, and caused Debian's Maven package to fail (the standard Maven 
package uses cli 1.0 of course). As part of the Debian packaging I have 
created a quick-and-dirty patch which allows Maven to work (see 
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=469082 for the Debian 
bug report, and I have attached the patch (MultiOptions.patch). I would 
guess that a proper fix would be rather more elegant than my patch.

Thanks,
Paul Cager

--------------010807020604070306040901
Content-Type: text/x-diff;
 name="Cli-Junit.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
 filename="Cli-Junit.patch"

Index: src/test/org/apache/commons/cli/ApplicationTest.java
===================================================================
--- src/test/org/apache/commons/cli/ApplicationTest.java	(revision 635884)
+++ src/test/org/apache/commons/cli/ApplicationTest.java	(working copy)
@@ -96,15 +96,16 @@
         options.addOption( "listener", true, "add an instance of a class as a project listener" );
         options.addOption( "buildfile", true, "use given buildfile" );
         options.addOption( OptionBuilder.withDescription( "use value for given property" )
-                                        .hasArgs()
+                                        .hasArgs(1)
                                         .withValueSeparator()
                                         .create( 'D' ) );
                            //, null, true, , false, true );
         options.addOption( "find", true, "search for buildfile towards the root of the filesystem and use it" );
 
         String[] args = new String[]{ "-buildfile", "mybuild.xml",
+            "-projecthelp",
             "-Dproperty=value", "-Dproperty1=value1",
-            "-projecthelp" };
+            "compile" };
 
         try {
             CommandLine line = parser.parse( options, args );
@@ -121,6 +122,7 @@
 
             // check option
             assertTrue( line.hasOption( "projecthelp") );
+            assertEquals(1, line.getArgs().length);
         }
         catch( ParseException exp ) {
             fail( "Unexpected exception:" + exp.getMessage() );
@@ -128,4 +130,4 @@
 
     }
 
-}
\ No newline at end of file
+}


--------------010807020604070306040901
Content-Type: text/x-diff;
 name="MultOptions.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
 filename="MultOptions.patch"

Index: src/java/org/apache/commons/cli/Option.java
===================================================================
--- src/java/org/apache/commons/cli/Option.java	(revision 635884)
+++ src/java/org/apache/commons/cli/Option.java	(working copy)
@@ -437,13 +437,15 @@
             // while there are more value separators
             while (index != -1)
             {
-                // next value to be added 
-                if (values.size() == (numberOfArgs - 1))
-                {
-                    break;
-                }
+                // Next few lines commented out in the patch. You
+            	// can have multiple values in one argument, so it does not
+            	// make sense to terminate the loop early (you would lose
+            	// the excess arguments anyway).
+//                if (values.size() == (numberOfArgs - 1))
+//                {
+//                    break;
+//                }
 
-
                 // store
                 add(value.substring(0, index));
 
@@ -463,9 +465,7 @@
     }
 
     /**
-     * Add the value to this Option.  If the number of arguments
-     * is greater than zero and there is enough space in the list then
-     * add the value.  Otherwise, throw a runtime exception.
+     * Add the value to this Option.
      *
      * @param value The value to be added to this Option
      *
@@ -473,10 +473,14 @@
      */
     private void add(String value)
     {
-        if ((numberOfArgs > 0) && (values.size() > (numberOfArgs - 1)))
-        {
-            throw new RuntimeException("Cannot add value, list full.");
-        }
+    	// The patch removes the following code as we check for
+    	// numberOfArgs in the caller (where we can tell the difference
+    	// between (e.g) multiple instances of an option versus one
+    	// instance of the option with multiple arguments).
+//        if ((numberOfArgs > 0) && (values.size() > (numberOfArgs - 1)))
+//        {
+//            throw new RuntimeException("Cannot add value, list full.");
+//        }
 
 
         // store value
Index: src/java/org/apache/commons/cli/Parser.java
===================================================================
--- src/java/org/apache/commons/cli/Parser.java	(revision 635884)
+++ src/java/org/apache/commons/cli/Parser.java	(working copy)
@@ -325,7 +325,9 @@
     public void processArgs(Option opt, ListIterator iter)
         throws ParseException
     {
-        // loop until an option is found
+        // loop until another option is found, or we have read the maximum
+    	// number of args.
+    	int argCount = 0;
         while (iter.hasNext())
         {
             String str = (String) iter.next();
@@ -347,6 +349,12 @@
                 iter.previous();
                 break;
             }
+            
+            argCount++;
+            if (argCount >= opt.getArgs() && opt.getArgs() >= 0)
+            {
+            	break;
+            }
         }
 
         if ((opt.getValues() == null) && !opt.hasOptionalArg())


--------------010807020604070306040901
Content-Type: text/plain; charset=us-ascii

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
--------------010807020604070306040901--