HADOOP-7341. Fix options parsing in CommandFormat. Contributed by Daryn Sharp.

git-svn-id: https://svn.apache.org/repos/asf/hadoop/common/trunk@1132505 13f79535-47bb-0310-9956-ffa450edef68

CHANGES.txt

@@ -278,6 +278,8 @@ Trunk (unreleased changes)
 
     HADOOP-7284 Trash and shell's rm does not work for viewfs (Sanjay Radia)
 
+    HADOOP-7341. Fix options parsing in CommandFormat (Daryn Sharp via todd)
+
 Release 0.22.0 - Unreleased
 
   INCOMPATIBLE CHANGES

src/java/org/apache/hadoop/fs/FsShellPermissions.java

@@ -80,7 +80,7 @@ public class FsShellPermissions extends FsCommand {
 
     @Override
     protected void processOptions(LinkedList<String> args) throws IOException {
-      CommandFormat cf = new CommandFormat(null, 2, Integer.MAX_VALUE, "R", null);
+      CommandFormat cf = new CommandFormat(2, Integer.MAX_VALUE, "R", null);
       cf.parse(args);
       setRecursive(cf.getOpt("R"));
 
@@ -143,7 +143,7 @@ public class FsShellPermissions extends FsCommand {
 
     @Override
     protected void processOptions(LinkedList<String> args) throws IOException {
-      CommandFormat cf = new CommandFormat(null, 2, Integer.MAX_VALUE, "R");
+      CommandFormat cf = new CommandFormat(2, Integer.MAX_VALUE, "R");
       cf.parse(args);
       setRecursive(cf.getOpt("R"));
       parseOwnerGroup(args.removeFirst());

src/java/org/apache/hadoop/fs/shell/CommandFormat.java

@@ -29,14 +29,30 @@ import java.util.Set;
  * Parse the args of a command and check the format of args.
  */
 public class CommandFormat {
-  final String name;
   final int minPar, maxPar;
   final Map<String, Boolean> options = new HashMap<String, Boolean>();
   boolean ignoreUnknownOpts = false;
   
-  /** constructor */
+  /**
+   * @deprecated use replacement since name is an unused parameter
+   * @param name of command, but never used
+   * @param min see replacement
+   * @param max see replacement
+   * @param possibleOpt see replacement
+   * @see #CommandFormat(int, int, String...)
+   */
+  @Deprecated
   public CommandFormat(String n, int min, int max, String ... possibleOpt) {
-    name = n;
+    this(min, max, possibleOpt);
+  }
+  
+  /**
+   * Simple parsing of command line arguments
+   * @param min minimum arguments required
+   * @param max maximum arguments permitted
+   * @param possibleOpt list of the allowed switches
+   */
+  public CommandFormat(int min, int max, String ... possibleOpt) {
     minPar = min;
     maxPar = max;
     for (String opt : possibleOpt) {
@@ -71,16 +87,23 @@ public class CommandFormat {
     int pos = 0;
     while (pos < args.size()) {
       String arg = args.get(pos);
-      if (arg.startsWith("-") && arg.length() > 1) {
-        String opt = arg.substring(1);
-        if (options.containsKey(opt)) {
-          args.remove(pos);
-          options.put(opt, Boolean.TRUE);
-          continue;
-        }
-        if (!ignoreUnknownOpts) throw new UnknownOptionException(arg);
+      // stop if not an opt, or the stdin arg "-" is found
+      if (!arg.startsWith("-") || arg.equals("-")) { 
+        break;
+      } else if (arg.equals("--")) { // force end of option processing
+        args.remove(pos);
+        break;
+      }
+      
+      String opt = arg.substring(1);
+      if (options.containsKey(opt)) {
+        args.remove(pos);
+        options.put(opt, Boolean.TRUE);
+      } else if (ignoreUnknownOpts) {
+        pos++;
+      } else {
+        throw new UnknownOptionException(arg);
       }
-      pos++;
     }
     int psize = args.size();
     if (psize < minPar) {

src/java/org/apache/hadoop/fs/shell/CopyCommands.java

@@ -64,7 +64,7 @@ class CopyCommands {
 
     @Override
     protected void processOptions(LinkedList<String> args) throws IOException {
-      CommandFormat cf = new CommandFormat(null, 2, 3);
+      CommandFormat cf = new CommandFormat(2, 3);
       cf.parse(args);
 
       // TODO: this really should be a -nl option
@@ -94,7 +94,7 @@ class CopyCommands {
     
     @Override
     protected void processOptions(LinkedList<String> args) throws IOException {
-      CommandFormat cf = new CommandFormat(null, 2, Integer.MAX_VALUE);
+      CommandFormat cf = new CommandFormat(2, Integer.MAX_VALUE);
       cf.parse(args);
       getRemoteDestination(args);
     }
@@ -137,7 +137,7 @@ class CopyCommands {
     throws IOException {
       localFs = FileSystem.getLocal(getConf());
       CommandFormat cf = new CommandFormat(
-          null, 1, Integer.MAX_VALUE, "crc", "ignoreCrc");
+          1, Integer.MAX_VALUE, "crc", "ignoreCrc");
       cf.parse(args);
       copyCrc = cf.getOpt("crc");
       verifyChecksum = !cf.getOpt("ignoreCrc");
@@ -216,7 +216,7 @@ class CopyCommands {
 
     @Override
     protected void processOptions(LinkedList<String> args) throws IOException {
-      CommandFormat cf = new CommandFormat(null, 1, Integer.MAX_VALUE);
+      CommandFormat cf = new CommandFormat(1, Integer.MAX_VALUE);
       cf.parse(args);
       getRemoteDestination(args);
     }

src/java/org/apache/hadoop/fs/shell/Count.java

@@ -73,7 +73,7 @@ public class Count extends FsCommand {
 
   @Override
   protected void processOptions(LinkedList<String> args) {
-    CommandFormat cf = new CommandFormat(NAME, 1, Integer.MAX_VALUE, "q");
+    CommandFormat cf = new CommandFormat(1, Integer.MAX_VALUE, "q");
     cf.parse(args);
     if (args.isEmpty()) { // default path is the current working directory
       args.add(".");

src/java/org/apache/hadoop/fs/shell/Delete.java

@@ -57,7 +57,7 @@ class Delete extends FsCommand {
     @Override
     protected void processOptions(LinkedList<String> args) throws IOException {
       CommandFormat cf = new CommandFormat(
-          null, 1, Integer.MAX_VALUE, "r", "R", "skipTrash");
+          1, Integer.MAX_VALUE, "r", "R", "skipTrash");
       cf.parse(args);
       deleteDirs = cf.getOpt("r") || cf.getOpt("R");
       skipTrash = cf.getOpt("skipTrash");
@@ -115,7 +115,7 @@ class Delete extends FsCommand {
     // TODO: should probably allow path arguments for the filesystems
     @Override
     protected void processOptions(LinkedList<String> args) throws IOException {
-      CommandFormat cf = new CommandFormat(null, 0, 0);
+      CommandFormat cf = new CommandFormat(0, 0);
       cf.parse(args);
     }
 

src/java/org/apache/hadoop/fs/shell/Display.java

@@ -66,7 +66,7 @@ class Display extends FsCommand {
     @Override
     protected void processOptions(LinkedList<String> args)
     throws IOException {
-      CommandFormat cf = new CommandFormat(null, 1, Integer.MAX_VALUE, "ignoreCrc");
+      CommandFormat cf = new CommandFormat(1, Integer.MAX_VALUE, "ignoreCrc");
       cf.parse(args);
       verifyChecksum = !cf.getOpt("ignoreCrc");
     }

src/java/org/apache/hadoop/fs/shell/FsUsage.java

@@ -67,7 +67,7 @@ class FsUsage extends FsCommand {
     @Override
     protected void processOptions(LinkedList<String> args)
     throws IOException {
-      CommandFormat cf = new CommandFormat(null, 0, Integer.MAX_VALUE, "h");
+      CommandFormat cf = new CommandFormat(0, Integer.MAX_VALUE, "h");
       cf.parse(args);
       humanReadable = cf.getOpt("h");
       if (args.isEmpty()) args.add(Path.SEPARATOR);
@@ -123,7 +123,7 @@ class FsUsage extends FsCommand {
     
     @Override
     protected void processOptions(LinkedList<String> args) throws IOException {
-      CommandFormat cf = new CommandFormat(null, 0, Integer.MAX_VALUE, "h", "s");
+      CommandFormat cf = new CommandFormat(0, Integer.MAX_VALUE, "h", "s");
       cf.parse(args);
       humanReadable = cf.getOpt("h");
       summary = cf.getOpt("s");

src/java/org/apache/hadoop/fs/shell/Ls.java

@@ -62,7 +62,7 @@ class Ls extends FsCommand {
   @Override
   protected void processOptions(LinkedList<String> args)
   throws IOException {
-    CommandFormat cf = new CommandFormat(null, 0, Integer.MAX_VALUE, "R");
+    CommandFormat cf = new CommandFormat(0, Integer.MAX_VALUE, "R");
     cf.parse(args);
     setRecursive(cf.getOpt("R"));
     if (args.isEmpty()) args.add(Path.CUR_DIR);

src/java/org/apache/hadoop/fs/shell/Mkdir.java

@@ -45,7 +45,7 @@ class Mkdir extends FsCommand {
 
   @Override
   protected void processOptions(LinkedList<String> args) {
-    CommandFormat cf = new CommandFormat(null, 1, Integer.MAX_VALUE);
+    CommandFormat cf = new CommandFormat(1, Integer.MAX_VALUE);
     cf.parse(args);
   }
 

src/java/org/apache/hadoop/fs/shell/MoveCommands.java

@@ -78,7 +78,7 @@ class MoveCommands {
 
     @Override
     protected void processOptions(LinkedList<String> args) throws IOException {
-      CommandFormat cf = new CommandFormat(null, 2, Integer.MAX_VALUE);
+      CommandFormat cf = new CommandFormat(2, Integer.MAX_VALUE);
       cf.parse(args);
       getRemoteDestination(args);
     }

src/java/org/apache/hadoop/fs/shell/SetReplication.java

@@ -33,7 +33,7 @@ import org.apache.hadoop.fs.shell.PathExceptions.PathIOException;
 @InterfaceAudience.Private
 @InterfaceStability.Unstable
 
-public class SetReplication extends FsCommand {
+class SetReplication extends FsCommand {
   public static void registerCommands(CommandFactory factory) {
     factory.addClass(SetReplication.class, "-setrep");
   }
@@ -51,7 +51,7 @@ public class SetReplication extends FsCommand {
   
   @Override
   protected void processOptions(LinkedList<String> args) throws IOException {
-    CommandFormat cf = new CommandFormat(null, 2, Integer.MAX_VALUE, "R", "w");
+    CommandFormat cf = new CommandFormat(2, Integer.MAX_VALUE, "R", "w");
     cf.parse(args);
     waitOpt = cf.getOpt("w");
     setRecursive(cf.getOpt("R"));

src/java/org/apache/hadoop/fs/shell/Stat.java

@@ -64,7 +64,7 @@ class Stat extends FsCommand {
 
   @Override
   protected void processOptions(LinkedList<String> args) throws IOException {
-    CommandFormat cf = new CommandFormat(null, 1, Integer.MAX_VALUE, "R");
+    CommandFormat cf = new CommandFormat(1, Integer.MAX_VALUE, "R");
     cf.parse(args);
     setRecursive(cf.getOpt("R"));
     if (args.getFirst().contains("%")) format = args.removeFirst();

src/java/org/apache/hadoop/fs/shell/Tail.java

@@ -51,7 +51,7 @@ class Tail extends FsCommand {
   
   @Override
   protected void processOptions(LinkedList<String> args) throws IOException {
-    CommandFormat cf = new CommandFormat(null, 1, 1, "f");
+    CommandFormat cf = new CommandFormat(1, 1, "f");
     cf.parse(args);
     follow = cf.getOpt("f");
   }

src/java/org/apache/hadoop/fs/shell/Test.java

@@ -46,7 +46,7 @@ class Test extends FsCommand {
   
   @Override
   protected void processOptions(LinkedList<String> args) {
-    CommandFormat cf = new CommandFormat(null, 1, 1, "e", "d", "z");
+    CommandFormat cf = new CommandFormat(1, 1, "e", "d", "z");
     cf.parse(args);
     
     String[] opts = cf.getOpts().toArray(new String[0]);

src/java/org/apache/hadoop/fs/shell/Touchz.java

@@ -52,7 +52,7 @@ class Touch extends FsCommand {
 
     @Override
     protected void processOptions(LinkedList<String> args) {
-      CommandFormat cf = new CommandFormat(null, 1, Integer.MAX_VALUE);
+      CommandFormat cf = new CommandFormat(1, Integer.MAX_VALUE);
       cf.parse(args);
     }
 

src/test/core/org/apache/hadoop/fs/TestCommandFormat.java

@@ -119,63 +119,59 @@ public class TestCommandFormat {
   @Test
   public void testArgOpt() {
     args = listOf("b", "-a");
-    expectedOpts = setOf("a");
-    expectedArgs = listOf("b");
+    expectedArgs = listOf("b", "-a");
 
-    checkArgLimits(UnknownOptionException.class, 0, 0);
     checkArgLimits(TooManyArgumentsException.class, 0, 0, "a", "b");
-    checkArgLimits(null, 0, 1, "a", "b");
-    checkArgLimits(null, 1, 1, "a", "b");
     checkArgLimits(null, 1, 2, "a", "b");
-    checkArgLimits(NotEnoughArgumentsException.class, 2, 2, "a", "b");
+    checkArgLimits(null, 2, 2, "a", "b");
+    checkArgLimits(NotEnoughArgumentsException.class, 3, 4, "a", "b");
   }
 
   @Test
-  public void testOptArgOpt() {
-    args = listOf("a", "-b", "c");
-    expectedOpts = setOf("b");
-    expectedArgs = listOf("a", "c");
+  public void testOptStopOptArg() {
+    args = listOf("-a", "--", "-b", "c");
+    expectedOpts = setOf("a");
+    expectedArgs = listOf("-b", "c");
 
     checkArgLimits(UnknownOptionException.class, 0, 0);
-    checkArgLimits(TooManyArgumentsException.class, 0, 0, "b");
-    checkArgLimits(TooManyArgumentsException.class, 1, 1, "b");
-    checkArgLimits(null, 0, 2, "b");
-    checkArgLimits(NotEnoughArgumentsException.class, 3, 3, "b");
+    checkArgLimits(TooManyArgumentsException.class, 0, 1, "a", "b");
+    checkArgLimits(null, 2, 2, "a", "b");
+    checkArgLimits(NotEnoughArgumentsException.class, 3, 4, "a", "b");
   }
 
   @Test
   public void testOptDashArg() {
-    args = listOf("-b", "-", "c");
+    args = listOf("-b", "-", "-c");
     expectedOpts = setOf("b");
-    expectedArgs = listOf("-", "c");
+    expectedArgs = listOf("-", "-c");
 
     checkArgLimits(UnknownOptionException.class, 0, 0);
-    checkArgLimits(TooManyArgumentsException.class, 0, 0, "b");
-    checkArgLimits(TooManyArgumentsException.class, 1, 1, "b");
-    checkArgLimits(null, 2, 2, "b");
-    checkArgLimits(NotEnoughArgumentsException.class, 3, 4, "b");
+    checkArgLimits(TooManyArgumentsException.class, 0, 0, "b", "c");
+    checkArgLimits(TooManyArgumentsException.class, 1, 1, "b", "c");
+    checkArgLimits(null, 2, 2, "b", "c");
+    checkArgLimits(NotEnoughArgumentsException.class, 3, 4, "b", "c");
   }
   
   @Test
   public void testOldArgsWithIndex() {
     String[] arrayArgs = new String[]{"ignore", "-a", "b", "-c"};
     {
-      CommandFormat cf = new CommandFormat("", 0, 9, "a", "c");
+      CommandFormat cf = new CommandFormat(0, 9, "a", "c");
       List<String> parsedArgs = cf.parse(arrayArgs, 0);
-      assertEquals(setOf("a", "c"), cf.getOpts());
-      assertEquals(listOf("ignore", "b"), parsedArgs);
+      assertEquals(setOf(), cf.getOpts());
+      assertEquals(listOf("ignore", "-a", "b", "-c"), parsedArgs);
     }
     { 
-      CommandFormat cf = new CommandFormat("", 0, 9, "a", "c");
+      CommandFormat cf = new CommandFormat(0, 9, "a", "c");
       List<String> parsedArgs = cf.parse(arrayArgs, 1);
-      assertEquals(setOf("a", "c"), cf.getOpts());
-      assertEquals(listOf("b"), parsedArgs);
+      assertEquals(setOf("a"), cf.getOpts());
+      assertEquals(listOf("b", "-c"), parsedArgs);
     }
     { 
-      CommandFormat cf = new CommandFormat("", 0, 9, "a", "c");
+      CommandFormat cf = new CommandFormat(0, 9, "a", "c");
       List<String> parsedArgs = cf.parse(arrayArgs, 2);
-      assertEquals(setOf("c"), cf.getOpts());
-      assertEquals(listOf("b"), parsedArgs);
+      assertEquals(setOf(), cf.getOpts());
+      assertEquals(listOf("b", "-c"), parsedArgs);
     }
   }
   
@@ -183,7 +179,7 @@ public class TestCommandFormat {
       Class<? extends IllegalArgumentException> expectedErr,
       int min, int max, String ... opts)
   {
-    CommandFormat cf = new CommandFormat("", min, max, opts);
+    CommandFormat cf = new CommandFormat(min, max, opts);
     List<String> parsedArgs = new ArrayList<String>(args);
     
     Class<?> cfError = null;
@@ -202,11 +198,13 @@ public class TestCommandFormat {
     return cf;
   }
   
-  private static <T> List<T> listOf(T ... objects) {
+  // Don't use generics to avoid warning:
+  // unchecked generic array creation of type T[] for varargs parameter
+  private static List<String> listOf(String ... objects) {
     return Arrays.asList(objects);
   }
   
-  private static <T> Set<T> setOf(T ... objects) {
-    return new HashSet<T>(listOf(objects));
+  private static Set<String> setOf(String ... objects) {
+    return new HashSet<String>(listOf(objects));
   }
 }