]> source.dussan.org Git - jgit.git/commitdiff
Don't treat command termination due '-h' option as a fatal error 30/63330/3
authorAndrey Loskutov <loskutov@gmx.de>
Mon, 28 Dec 2015 22:27:09 +0000 (23:27 +0100)
committerAndrey Loskutov <loskutov@gmx.de>
Tue, 29 Dec 2015 13:35:08 +0000 (14:35 +0100)
Signal early command termination due '-h' or '--help' option via
TerminatedByHelpException. This allows tests using
CLIGitCommand differentiate between unexpected command parsing errors
and expected command cancellation "on help" (which also allows
validation of expected/unexpected help messages).

Additional side-effect: jgit supports now git style of handling help
option: any unexpected command line options before help are reported as
errors, but after help ignored.

Bug: 484951
Change-Id: If45c41c0d32895ab6822a7ff9d851877dcef5771
Signed-off-by: Andrey Loskutov <loskutov@gmx.de>
org.eclipse.jgit.pgm.test/src/org/eclipse/jgit/pgm/CLIGitCommand.java
org.eclipse.jgit.pgm.test/tst/org/eclipse/jgit/pgm/DescribeTest.java
org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/Main.java
org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/Remote.java
org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/TextBuiltin.java
org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/opt/CmdLineParser.java

index bf15fed812daa8b93b9aa1d6348404df2f6594ce..7d2cbca729bfd4e9ec820e5801c664a5e07fb0ab 100644 (file)
@@ -50,6 +50,7 @@ import java.util.List;
 
 import org.eclipse.jgit.internal.storage.file.FileRepository;
 import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.pgm.TextBuiltin.TerminatedByHelpException;
 import org.eclipse.jgit.pgm.internal.CLIText;
 import org.eclipse.jgit.pgm.opt.CmdLineParser;
 import org.eclipse.jgit.pgm.opt.SubcommandHandler;
@@ -120,12 +121,15 @@ public class CLIGitCommand {
                System.arraycopy(args, 1, argv, 0, args.length - 1);
 
                CLIGitCommand bean = new CLIGitCommand();
-               final CmdLineParser clp = new CmdLineParser(bean);
+               final CmdLineParser clp = new TestCmdLineParser(bean);
                clp.parseArgument(argv);
 
                final TextBuiltin cmd = bean.getSubcommand();
                ByteArrayOutputStream baos = new ByteArrayOutputStream();
                cmd.outs = baos;
+               ByteArrayOutputStream errs = new ByteArrayOutputStream();
+               cmd.errs = errs;
+               boolean seenHelp = TextBuiltin.containsHelp(argv);
                if (cmd.requiresRepository())
                        cmd.init(db, null);
                else
@@ -133,9 +137,22 @@ public class CLIGitCommand {
                try {
                        cmd.execute(bean.getArguments().toArray(
                                        new String[bean.getArguments().size()]));
+               } catch (TerminatedByHelpException e) {
+                       seenHelp = true;
+                       // this is not a failure, command execution should just not happen
                } finally {
-                       if (cmd.outw != null)
+                       if (cmd.outw != null) {
                                cmd.outw.flush();
+                       }
+                       if (cmd.errw != null) {
+                               cmd.errw.flush();
+                       }
+                       if (seenHelp) {
+                               return errs.toByteArray();
+                       } else if (errs.size() > 0) {
+                               // forward the errors to the standard err
+                               System.err.print(errs.toString());
+                       }
                }
                return baos.toByteArray();
        }
@@ -195,4 +212,14 @@ public class CLIGitCommand {
                return list.toArray(new String[list.size()]);
        }
 
+       static class TestCmdLineParser extends CmdLineParser {
+               public TestCmdLineParser(Object bean) {
+                       super(bean);
+               }
+
+               @Override
+               protected boolean containsHelp(String... args) {
+                       return false;
+               }
+       }
 }
index 6352a265240dfe392603c8ff2f1b1b0e9978355c..1c8ba0c2f602160c375d1076a2693b5b1c30fd53 100644 (file)
 package org.eclipse.jgit.pgm;
 
 import static org.junit.Assert.assertArrayEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+import java.util.Arrays;
 
 import org.eclipse.jgit.api.Git;
 import org.eclipse.jgit.lib.CLIRepositoryTestCase;
@@ -103,4 +107,22 @@ public class DescribeTest extends CLIRepositoryTestCase {
                assertArrayEquals(new String[] { "v1.0-0-g6fd41be", "" },
                                execute("git describe --long HEAD"));
        }
+
+       @Test
+       public void testHelpArgumentBeforeUnknown() throws Exception {
+               String[] output = execute("git describe -h -XYZ");
+               String all = Arrays.toString(output);
+               assertTrue("Unexpected help output: " + all,
+                               all.contains("jgit describe"));
+               assertFalse("Unexpected help output: " + all, all.contains("fatal"));
+       }
+
+       @Test
+       public void testHelpArgumentAfterUnknown() throws Exception {
+               String[] output = execute("git describe -XYZ -h");
+               String all = Arrays.toString(output);
+               assertTrue("Unexpected help output: " + all,
+                               all.contains("jgit describe"));
+               assertTrue("Unexpected help output: " + all, all.contains("fatal"));
+       }
 }
index 9c72b4aada2dfd537ba7948c247e13ae0050315b..d04cef0c7fc0aa00e3f8d7473b2169c418134bd2 100644 (file)
@@ -170,7 +170,7 @@ public class Main {
        }
 
        private void execute(final String[] argv) throws Exception {
-               final CmdLineParser clp = new CmdLineParser(this);
+               final CmdLineParser clp = new SubcommandLineParser(this);
                PrintWriter writer = new PrintWriter(System.err);
                try {
                        clp.parseArgument(argv);
@@ -335,4 +335,19 @@ public class Main {
                        }
                }
        }
+
+       /**
+        * Parser for subcommands which doesn't stop parsing on help options and so
+        * proceeds all specified options
+        */
+       static class SubcommandLineParser extends CmdLineParser {
+               public SubcommandLineParser(Object bean) {
+                       super(bean);
+               }
+
+               @Override
+               protected boolean containsHelp(String... args) {
+                       return false;
+               }
+       }
 }
index 70868e920e0e8940fab91eef0b9e5958f975b11c..24916bd1c2d7767173eda230b88301be83e775e2 100644 (file)
@@ -144,7 +144,7 @@ class Remote extends TextBuiltin {
        }
 
        @Override
-       public void printUsageAndExit(final String message, final CmdLineParser clp)
+       public void printUsage(final String message, final CmdLineParser clp)
                        throws IOException {
                errw.println(message);
                errw.println("jgit remote [--verbose (-v)] [--help (-h)]"); //$NON-NLS-1$
@@ -160,7 +160,6 @@ class Remote extends TextBuiltin {
                errw.println();
 
                errw.flush();
-               throw die(true);
        }
 
        private void print(List<RemoteConfig> remotes) throws IOException {
index 40a42e5207a5696e8d76283f23518b43d83dff75..95938326fd7d8d231c8ccc0d761f5a834eb2a55a 100644 (file)
@@ -212,17 +212,20 @@ public abstract class TextBuiltin {
         */
        protected void parseArguments(final String[] args) throws IOException {
                final CmdLineParser clp = new CmdLineParser(this);
+               help = containsHelp(args);
                try {
                        clp.parseArgument(args);
                } catch (CmdLineException err) {
-                       if (!help) {
-                               this.errw.println(MessageFormat.format(CLIText.get().fatalError, err.getMessage()));
-                               throw die(true, err);
+                       this.errw.println(MessageFormat.format(CLIText.get().fatalError, err.getMessage()));
+                       if (help) {
+                               printUsage("", clp); //$NON-NLS-1$
                        }
+                       throw die(true, err);
                }
 
                if (help) {
-                       printUsageAndExit(clp);
+                       printUsage("", clp); //$NON-NLS-1$
+                       throw new TerminatedByHelpException();
                }
 
                argWalk = clp.getRevWalkGently();
@@ -246,6 +249,20 @@ public abstract class TextBuiltin {
         * @throws IOException
         */
        public void printUsageAndExit(final String message, final CmdLineParser clp) throws IOException {
+               printUsage(message, clp);
+               throw die(true);
+       }
+
+       /**
+        * @param message
+        *            non null
+        * @param clp
+        *            parser used to print options
+        * @throws IOException
+        * @since 4.2
+        */
+       protected void printUsage(final String message, final CmdLineParser clp)
+                       throws IOException {
                errw.println(message);
                errw.print("jgit "); //$NON-NLS-1$
                errw.print(commandName);
@@ -257,7 +274,6 @@ public abstract class TextBuiltin {
                errw.println();
 
                errw.flush();
-               throw die(true);
        }
 
        /**
@@ -346,4 +362,36 @@ public abstract class TextBuiltin {
                        dst = dst.substring(R_REMOTES.length());
                return dst;
        }
+
+       /**
+        * @param args
+        *            non null
+        * @return true if the given array contains help option
+        * @since 4.2
+        */
+       public static boolean containsHelp(String[] args) {
+               for (String str : args) {
+                       if (str.equals("-h") || str.equals("--help")) { //$NON-NLS-1$ //$NON-NLS-2$
+                               return true;
+                       }
+               }
+               return false;
+       }
+
+       /**
+        * Exception thrown by {@link TextBuiltin} if it proceeds 'help' option
+        *
+        * @since 4.2
+        */
+       public static class TerminatedByHelpException extends Die {
+               private static final long serialVersionUID = 1L;
+
+               /**
+                * Default constructor
+                */
+               public TerminatedByHelpException() {
+                       super(true);
+               }
+
+       }
 }
index 3f77aa6687e042a42adaf62dab611c3b5552109c..f794f91cab32bffad041a9d180f24b86d0f3ae5f 100644 (file)
@@ -45,15 +45,10 @@ package org.eclipse.jgit.pgm.opt;
 
 import java.lang.reflect.Field;
 import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Iterator;
+import java.util.List;
 
-import org.kohsuke.args4j.Argument;
-import org.kohsuke.args4j.CmdLineException;
-import org.kohsuke.args4j.IllegalAnnotationError;
-import org.kohsuke.args4j.NamedOptionDef;
-import org.kohsuke.args4j.Option;
-import org.kohsuke.args4j.OptionDef;
-import org.kohsuke.args4j.spi.OptionHandler;
-import org.kohsuke.args4j.spi.Setter;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.pgm.TextBuiltin;
@@ -63,6 +58,14 @@ import org.eclipse.jgit.revwalk.RevTree;
 import org.eclipse.jgit.revwalk.RevWalk;
 import org.eclipse.jgit.transport.RefSpec;
 import org.eclipse.jgit.treewalk.AbstractTreeIterator;
+import org.kohsuke.args4j.Argument;
+import org.kohsuke.args4j.CmdLineException;
+import org.kohsuke.args4j.IllegalAnnotationError;
+import org.kohsuke.args4j.NamedOptionDef;
+import org.kohsuke.args4j.Option;
+import org.kohsuke.args4j.OptionDef;
+import org.kohsuke.args4j.spi.OptionHandler;
+import org.kohsuke.args4j.spi.Setter;
 
 /**
  * Extended command line parser which handles --foo=value arguments.
@@ -86,6 +89,8 @@ public class CmdLineParser extends org.kohsuke.args4j.CmdLineParser {
 
        private RevWalk walk;
 
+       private boolean seenHelp;
+
        /**
         * Creates a new command line owner that parses arguments/options and set
         * them into the given object.
@@ -143,9 +148,58 @@ public class CmdLineParser extends org.kohsuke.args4j.CmdLineParser {
                        }
 
                        tmp.add(str);
+
+                       if (containsHelp(args)) {
+                               // suppress exceptions on required parameters if help is present
+                               seenHelp = true;
+                               // stop argument parsing here
+                               break;
+                       }
                }
+               List<OptionHandler> backup = null;
+               if (seenHelp) {
+                       backup = unsetRequiredOptions();
+               }
+
+               try {
+                       super.parseArgument(tmp.toArray(new String[tmp.size()]));
+               } finally {
+                       // reset "required" options to defaults for correct command printout
+                       if (backup != null && !backup.isEmpty()) {
+                               restoreRequiredOptions(backup);
+                       }
+                       seenHelp = false;
+               }
+       }
+
+       private List<OptionHandler> unsetRequiredOptions() {
+               List<OptionHandler> options = getOptions();
+               List<OptionHandler> backup = new ArrayList<>(options);
+               for (Iterator<OptionHandler> iterator = options.iterator(); iterator
+                               .hasNext();) {
+                       OptionHandler handler = iterator.next();
+                       if (handler.option instanceof NamedOptionDef
+                                       && handler.option.required()) {
+                               iterator.remove();
+                       }
+               }
+               return backup;
+       }
+
+       private void restoreRequiredOptions(List<OptionHandler> backup) {
+               List<OptionHandler> options = getOptions();
+               options.clear();
+               options.addAll(backup);
+       }
 
-               super.parseArgument(tmp.toArray(new String[tmp.size()]));
+       /**
+        * @param args
+        *            non null
+        * @return true if the given array contains help option
+        * @since 4.2
+        */
+       protected boolean containsHelp(final String... args) {
+               return TextBuiltin.containsHelp(args);
        }
 
        /**
@@ -181,7 +235,7 @@ public class CmdLineParser extends org.kohsuke.args4j.CmdLineParser {
                return walk;
        }
 
-       static class MyOptionDef extends OptionDef {
+       class MyOptionDef extends OptionDef {
 
                public MyOptionDef(OptionDef o) {
                        super(o.usage(), o.metaVar(), o.required(), o.handler(), o
@@ -201,6 +255,11 @@ public class CmdLineParser extends org.kohsuke.args4j.CmdLineParser {
                                return metaVar();
                        }
                }
+
+               @Override
+               public boolean required() {
+                       return seenHelp ? false : super.required();
+               }
        }
 
        @Override
@@ -211,4 +270,22 @@ public class CmdLineParser extends org.kohsuke.args4j.CmdLineParser {
                        return super.createOptionHandler(new MyOptionDef(o), setter);
 
        }
+
+       @SuppressWarnings("unchecked")
+       private List<OptionHandler> getOptions() {
+               List<OptionHandler> options = null;
+               try {
+                       Field field = org.kohsuke.args4j.CmdLineParser.class
+                                       .getDeclaredField("options"); //$NON-NLS-1$
+                       field.setAccessible(true);
+                       options = (List<OptionHandler>) field.get(this);
+               } catch (NoSuchFieldException | SecurityException
+                               | IllegalArgumentException | IllegalAccessException e) {
+                       // ignore
+               }
+               if (options == null) {
+                       return Collections.emptyList();
+               }
+               return options;
+       }
 }