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 <loskutov@gmx.de>

org.eclipse.jgit.pgm.test/src/org/eclipse/jgit/pgm/CLIGitCommand.java

@@ -50,6 +50,7 @@
 
 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 static byte[] rawExecute(String str, Repository db)
 		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 static byte[] rawExecute(String str, Repository db)
 		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 @@ else if (r.length() > 0) {
 		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;
+		}
+	}
 }

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 void testDescribeTagLong() throws Exception {
 		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"));
+	}
 }

org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/Main.java

@@ -170,7 +170,7 @@ protected void run(final String[] argv) {
 	}
 
 	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 @@ private static void configureHttpProxy() throws MalformedURLException {
 			}
 		}
 	}
+
+	/**
+	 * 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;
+		}
+	}
 }

org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/Remote.java

@@ -144,7 +144,7 @@ protected void run() throws Exception {
 	}
 
 	@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 @@ public void printUsageAndExit(final String message, final CmdLineParser clp)
 		errw.println();
 
 		errw.flush();
-		throw die(true);
 	}
 
 	private void print(List<RemoteConfig> remotes) throws IOException {

org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/TextBuiltin.java

@@ -212,17 +212,20 @@ public final void execute(String[] args) throws Exception {
 	 */
 	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()));
-				this.errw.println(MessageFormat.format(CLIText.get().fatalError, err.getMessage()));
+			if (help) {
-				throw die(true, err);
+				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 void printUsageAndExit(final CmdLineParser clp) throws IOException {
 	 * @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 void printUsageAndExit(final String message, final CmdLineParser clp) thr
 		errw.println();
 
 		errw.flush();
-		throw die(true);
 	}
 
 	/**
@@ -346,4 +362,36 @@ else if (abbreviateRemote && dst.startsWith(R_REMOTES))
 			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);
+		}
+
+	}
 }

org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/opt/CmdLineParser.java

@@ -45,15 +45,10 @@
 
 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.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 void parseArgument(final String... args) throws CmdLineException {
 			}
 
 			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();
 		}
 
-		super.parseArgument(tmp.toArray(new String[tmp.size()]));
+		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);
+	}
+
+	/**
+	 * @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 RevWalk getRevWalkGently() {
 		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 String toString() {
 				return metaVar();
 			}
 		}
+
+		@Override
+		public boolean required() {
+			return seenHelp ? false : super.required();
+		}
 	}
 
 	@Override
@@ -211,4 +270,22 @@ protected OptionHandler createOptionHandler(OptionDef o, Setter setter) {
 			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;
+	}
 }