From c59d86c0a79f913a8d6a62f2aa37dddcb1e2142c Mon Sep 17 00:00:00 2001 From: Andrey Loskutov Date: Mon, 28 Dec 2015 23:27:09 +0100 Subject: [PATCH] Don't treat command termination due '-h' option as a fatal error 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 --- .../org/eclipse/jgit/pgm/CLIGitCommand.java | 31 +++++- .../org/eclipse/jgit/pgm/DescribeTest.java | 22 +++++ .../src/org/eclipse/jgit/pgm/Main.java | 17 +++- .../src/org/eclipse/jgit/pgm/Remote.java | 3 +- .../src/org/eclipse/jgit/pgm/TextBuiltin.java | 58 ++++++++++- .../eclipse/jgit/pgm/opt/CmdLineParser.java | 97 +++++++++++++++++-- 6 files changed, 208 insertions(+), 20 deletions(-) diff --git a/org.eclipse.jgit.pgm.test/src/org/eclipse/jgit/pgm/CLIGitCommand.java b/org.eclipse.jgit.pgm.test/src/org/eclipse/jgit/pgm/CLIGitCommand.java index bf15fed81..7d2cbca72 100644 --- a/org.eclipse.jgit.pgm.test/src/org/eclipse/jgit/pgm/CLIGitCommand.java +++ b/org.eclipse.jgit.pgm.test/src/org/eclipse/jgit/pgm/CLIGitCommand.java @@ -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; + } + } } diff --git a/org.eclipse.jgit.pgm.test/tst/org/eclipse/jgit/pgm/DescribeTest.java b/org.eclipse.jgit.pgm.test/tst/org/eclipse/jgit/pgm/DescribeTest.java index 6352a2652..1c8ba0c2f 100644 --- a/org.eclipse.jgit.pgm.test/tst/org/eclipse/jgit/pgm/DescribeTest.java +++ b/org.eclipse.jgit.pgm.test/tst/org/eclipse/jgit/pgm/DescribeTest.java @@ -43,6 +43,10 @@ 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")); + } } diff --git a/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/Main.java b/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/Main.java index 9c72b4aad..d04cef0c7 100644 --- a/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/Main.java +++ b/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/Main.java @@ -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; + } + } } diff --git a/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/Remote.java b/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/Remote.java index 70868e920..24916bd1c 100644 --- a/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/Remote.java +++ b/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/Remote.java @@ -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 remotes) throws IOException { diff --git a/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/TextBuiltin.java b/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/TextBuiltin.java index 40a42e520..95938326f 100644 --- a/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/TextBuiltin.java +++ b/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/TextBuiltin.java @@ -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); + } + + } } diff --git a/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/opt/CmdLineParser.java b/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/opt/CmdLineParser.java index 3f77aa668..f794f91ca 100644 --- a/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/opt/CmdLineParser.java +++ b/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/opt/CmdLineParser.java @@ -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 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 unsetRequiredOptions() { + List options = getOptions(); + List backup = new ArrayList<>(options); + for (Iterator 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 backup) { + List 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 getOptions() { + List options = null; + try { + Field field = org.kohsuke.args4j.CmdLineParser.class + .getDeclaredField("options"); //$NON-NLS-1$ + field.setAccessible(true); + options = (List) field.get(this); + } catch (NoSuchFieldException | SecurityException + | IllegalArgumentException | IllegalAccessException e) { + // ignore + } + if (options == null) { + return Collections.emptyList(); + } + return options; + } }