branch command: provide convenient and meaningful options help

Added tests for all options, fixed multi-valued options parsing.

Bug: 484951
Change-Id: I5558589049544ea6c84932bc01f1f9df09e1f682
Signed-off-by: Andrey Loskutov <loskutov@gmx.de>

5 files changed, 154 insertions, 36 deletions

diff --git a/org.eclipse.jgit.pgm/resources/org/eclipse/jgit/pgm/internal/CLIText.properties b/org.eclipse.jgit.pgm/resources/org/eclipse/jgit/pgm/internal/CLIText.properties
index 3dcbda3b93..a24b817949 100644
--- a/org.eclipse.jgit.pgm/resources/org/eclipse/jgit/pgm/internal/CLIText.properties
+++ b/org.eclipse.jgit.pgm/resources/org/eclipse/jgit/pgm/internal/CLIText.properties
@@ -20,6 +20,7 @@ branchAlreadyExists=A branch named ''{0}'' already exists.
 branchCreatedFrom=branch: Created from {0}
 branchDetachedHEAD=detached HEAD
 branchIsNotAnAncestorOfYourCurrentHEAD=The branch ''{0}'' is not an ancestor of your current HEAD.\nIf you are sure you want to delete it, run ''jgit branch -D {0}''.
+branchNameRequired=branch name required
 branchNotFound=branch ''{0}'' not found.
 cacheTreePathInfo="{0}": {1} entries, {2} children
 cannotBeRenamed={0} cannot be renamed
@@ -89,7 +90,9 @@ metaVar_author=AUTHOR
 metaVar_base=base
 metaVar_blameL=START,END
 metaVar_blameReverse=START..END
+metaVar_branchAndStartPoint=branch [start-name]
 metaVar_branchName=branch
+metaVar_branchNames=branch ...
 metaVar_bucket=BUCKET
 metaVar_command=command
 metaVar_commandDetail=DETAIL
@@ -109,6 +112,7 @@ metaVar_message=message
 metaVar_n=n
 metaVar_name=name
 metaVar_object=object
+metaVar_oldNewBranchNames=[oldbranch] newbranch
 metaVar_op=OP
 metaVar_pass=PASS
 metaVar_path=path
@@ -125,6 +129,7 @@ metaVar_treeish=tree-ish
 metaVar_uriish=uri-ish
 metaVar_url=URL
 metaVar_user=USER
+metaVar_values=value ...
 metaVar_version=VERSION
 mostCommonlyUsedCommandsAre=The most commonly used commands are:
 needApprovalToDestroyCurrentRepository=Need approval to destroy current repository
diff --git a/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/Branch.java b/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/Branch.java
index 65aa24f356..045f3571e5 100644
--- a/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/Branch.java
+++ b/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/Branch.java
@@ -45,7 +45,6 @@ package org.eclipse.jgit.pgm;
 
 import java.io.IOException;
 import java.text.MessageFormat;
-import java.util.ArrayList;
 import java.util.Collection;
 import java.util.LinkedHashMap;
 import java.util.List;
@@ -65,15 +64,18 @@ import org.eclipse.jgit.lib.RefUpdate;
 import org.eclipse.jgit.lib.RefUpdate.Result;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.pgm.internal.CLIText;
-import org.eclipse.jgit.pgm.opt.CmdLineParser;
+import org.eclipse.jgit.pgm.opt.OptionWithValuesListHandler;
 import org.eclipse.jgit.revwalk.RevWalk;
 import org.kohsuke.args4j.Argument;
-import org.kohsuke.args4j.ExampleMode;
 import org.kohsuke.args4j.Option;
 
 @Command(common = true, usage = "usage_listCreateOrDeleteBranches")
 class Branch extends TextBuiltin {
 
+	private String otherBranch;
+	private boolean createForce;
+	private boolean rename;
+
 	@Option(name = "--remote", aliases = { "-r" }, usage = "usage_actOnRemoteTrackingBranches")
 	private boolean remote = false;
 
@@ -83,23 +85,69 @@ class Branch extends TextBuiltin {
 	@Option(name = "--contains", metaVar = "metaVar_commitish", usage = "usage_printOnlyBranchesThatContainTheCommit")
 	private String containsCommitish;
 
-	@Option(name = "--delete", aliases = { "-d" }, usage = "usage_deleteFullyMergedBranch")
-	private boolean delete = false;
+	private List<String> delete;
 
-	@Option(name = "--delete-force", aliases = { "-D" }, usage = "usage_deleteBranchEvenIfNotMerged")
-	private boolean deleteForce = false;
+	@Option(name = "--delete", aliases = {
+			"-d" }, metaVar = "metaVar_branchNames", usage = "usage_deleteFullyMergedBranch", handler = OptionWithValuesListHandler.class)
+	public void delete(List<String> names) {
+		if (names.isEmpty()) {
+			throw die(CLIText.get().branchNameRequired);
+		}
+		delete = names;
+	}
 
-	@Option(name = "--create-force", aliases = { "-f" }, usage = "usage_forceCreateBranchEvenExists")
-	private boolean createForce = false;
+	private List<String> deleteForce;
 
-	@Option(name = "-m", usage = "usage_moveRenameABranch")
-	private boolean rename = false;
+	@Option(name = "--delete-force", aliases = {
+			"-D" }, metaVar = "metaVar_branchNames", usage = "usage_deleteBranchEvenIfNotMerged", handler = OptionWithValuesListHandler.class)
+	public void deleteForce(List<String> names) {
+		if (names.isEmpty()) {
+			throw die(CLIText.get().branchNameRequired);
+		}
+		deleteForce = names;
+	}
+
+	@Option(name = "--create-force", aliases = {
+			"-f" }, metaVar = "metaVar_branchAndStartPoint", usage = "usage_forceCreateBranchEvenExists", handler = OptionWithValuesListHandler.class)
+	public void createForce(List<String> branchAndStartPoint) {
+		createForce = true;
+		if (branchAndStartPoint.isEmpty()) {
+			throw die(CLIText.get().branchNameRequired);
+		}
+		if (branchAndStartPoint.size() > 2) {
+			throw die(CLIText.get().tooManyRefsGiven);
+		}
+		if (branchAndStartPoint.size() == 1) {
+			branch = branchAndStartPoint.get(0);
+		} else {
+			branch = branchAndStartPoint.get(0);
+			otherBranch = branchAndStartPoint.get(1);
+		}
+	}
+
+	@Option(name = "--move", aliases = {
+			"-m" }, metaVar = "metaVar_oldNewBranchNames", usage = "usage_moveRenameABranch", handler = OptionWithValuesListHandler.class)
+	public void moveRename(List<String> currentAndNew) {
+		rename = true;
+		if (currentAndNew.isEmpty()) {
+			throw die(CLIText.get().branchNameRequired);
+		}
+		if (currentAndNew.size() > 2) {
+			throw die(CLIText.get().tooManyRefsGiven);
+		}
+		if (currentAndNew.size() == 1) {
+			branch = currentAndNew.get(0);
+		} else {
+			branch = currentAndNew.get(0);
+			otherBranch = currentAndNew.get(1);
+		}
+	}
 
 	@Option(name = "--verbose", aliases = { "-v" }, usage = "usage_beVerbose")
 	private boolean verbose = false;
 
-	@Argument
-	private List<String> branches = new ArrayList<String>();
+	@Argument(metaVar = "metaVar_name")
+	private String branch;
 
 	private final Map<String, Ref> printRefs = new LinkedHashMap<String, Ref>();
 
@@ -110,30 +158,33 @@ class Branch extends TextBuiltin {
 
 	@Override
 	protected void run() throws Exception {
-		if (delete || deleteForce)
-			delete(deleteForce);
-		else {
-			if (branches.size() > 2)
-				throw die(CLIText.get().tooManyRefsGiven + new CmdLineParser(this).printExample(ExampleMode.ALL));
-
+		if (delete != null || deleteForce != null) {
+			if (delete != null) {
+				delete(delete, false);
+			}
+			if (deleteForce != null) {
+				delete(deleteForce, true);
+			}
+		} else {
 			if (rename) {
 				String src, dst;
-				if (branches.size() == 1) {
+				if (otherBranch == null) {
 					final Ref head = db.getRef(Constants.HEAD);
-					if (head != null && head.isSymbolic())
+					if (head != null && head.isSymbolic()) {
 						src = head.getLeaf().getName();
-					else
+					} else {
 						throw die(CLIText.get().cannotRenameDetachedHEAD);
-					dst = branches.get(0);
+					}
+					dst = branch;
 				} else {
-					src = branches.get(0);
+					src = branch;
 					final Ref old = db.getRef(src);
 					if (old == null)
 						throw die(MessageFormat.format(CLIText.get().doesNotExist, src));
 					if (!old.getName().startsWith(Constants.R_HEADS))
 						throw die(MessageFormat.format(CLIText.get().notABranch, src));
 					src = old.getName();
-					dst = branches.get(1);
+					dst = otherBranch;
 				}
 
 				if (!dst.startsWith(Constants.R_HEADS))
@@ -145,13 +196,14 @@ class Branch extends TextBuiltin {
 				if (r.rename() != Result.RENAMED)
 					throw die(MessageFormat.format(CLIText.get().cannotBeRenamed, src));
 
-			} else if (branches.size() > 0) {
-				String newHead = branches.get(0);
+			} else if (createForce || branch != null) {
+				String newHead = branch;
 				String startBranch;
-				if (branches.size() == 2)
-					startBranch = branches.get(1);
-				else
+				if (createForce) {
+					startBranch = otherBranch;
+				} else {
 					startBranch = Constants.HEAD;
+				}
 				Ref startRef = db.getRef(startBranch);
 				ObjectId startAt = db.resolve(startBranch + "^0"); //$NON-NLS-1$
 				if (startRef != null) {
@@ -164,22 +216,27 @@ class Branch extends TextBuiltin {
 				}
 				startBranch = Repository.shortenRefName(startBranch);
 				String newRefName = newHead;
-				if (!newRefName.startsWith(Constants.R_HEADS))
+				if (!newRefName.startsWith(Constants.R_HEADS)) {
 					newRefName = Constants.R_HEADS + newRefName;
-				if (!Repository.isValidRefName(newRefName))
+				}
+				if (!Repository.isValidRefName(newRefName)) {
 					throw die(MessageFormat.format(CLIText.get().notAValidRefName, newRefName));
-				if (!createForce && db.resolve(newRefName) != null)
+				}
+				if (!createForce && db.resolve(newRefName) != null) {
 					throw die(MessageFormat.format(CLIText.get().branchAlreadyExists, newHead));
+				}
 				RefUpdate updateRef = db.updateRef(newRefName);
 				updateRef.setNewObjectId(startAt);
 				updateRef.setForceUpdate(createForce);
 				updateRef.setRefLogMessage(MessageFormat.format(CLIText.get().branchCreatedFrom, startBranch), false);
 				Result update = updateRef.update();
-				if (update == Result.REJECTED)
+				if (update == Result.REJECTED) {
 					throw die(MessageFormat.format(CLIText.get().couldNotCreateBranch, newHead, update.toString()));
+				}
 			} else {
-				if (verbose)
+				if (verbose) {
 					rw = new RevWalk(db);
+				}
 				list();
 			}
 		}
@@ -249,7 +306,8 @@ class Branch extends TextBuiltin {
 		outw.println();
 	}
 
-	private void delete(boolean force) throws IOException {
+	private void delete(List<String> branches, boolean force)
+			throws IOException {
 		String current = db.getBranch();
 		ObjectId head = db.resolve(Constants.HEAD);
 		for (String branch : branches) {
diff --git a/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/internal/CLIText.java b/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/internal/CLIText.java
index f5d581ad01..fd86ddf2b7 100644
--- a/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/internal/CLIText.java
+++ b/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/internal/CLIText.java
@@ -85,6 +85,7 @@ public class CLIText extends TranslationBundle {
 	/***/ public String branchCreatedFrom;
 	/***/ public String branchDetachedHEAD;
 	/***/ public String branchIsNotAnAncestorOfYourCurrentHEAD;
+	/***/ public String branchNameRequired;
 	/***/ public String branchNotFound;
 	/***/ public String cacheTreePathInfo;
 	/***/ public String configFileNotFound;
@@ -184,6 +185,7 @@ public class CLIText extends TranslationBundle {
 	/***/ public String metaVar_uriish;
 	/***/ public String metaVar_url;
 	/***/ public String metaVar_user;
+	/***/ public String metaVar_values;
 	/***/ public String metaVar_version;
 	/***/ public String mostCommonlyUsedCommandsAre;
 	/***/ public String needApprovalToDestroyCurrentRepository;
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 66d4816422..a1e39502c7 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
@@ -86,6 +86,7 @@ public class CmdLineParser extends org.kohsuke.args4j.CmdLineParser {
 		registerHandler(RefSpec.class, RefSpecHandler.class);
 		registerHandler(RevCommit.class, RevCommitHandler.class);
 		registerHandler(RevTree.class, RevTreeHandler.class);
+		registerHandler(List.class, OptionWithValuesListHandler.class);
 	}
 
 	private final Repository db;
diff --git a/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/opt/OptionWithValuesListHandler.java b/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/opt/OptionWithValuesListHandler.java
new file mode 100644
index 0000000000..3de7a81091
--- /dev/null
+++ b/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/opt/OptionWithValuesListHandler.java
@@ -0,0 +1,52 @@
+package org.eclipse.jgit.pgm.opt;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.eclipse.jgit.pgm.internal.CLIText;
+import org.kohsuke.args4j.CmdLineException;
+import org.kohsuke.args4j.CmdLineParser;
+import org.kohsuke.args4j.OptionDef;
+import org.kohsuke.args4j.spi.OptionHandler;
+import org.kohsuke.args4j.spi.Parameters;
+import org.kohsuke.args4j.spi.Setter;
+
+/**
+ * Handler which allows to parse option with few values
+ *
+ * @since 4.2
+ */
+public class OptionWithValuesListHandler extends OptionHandler<List<?>> {
+
+	/**
+	 * @param parser
+	 * @param option
+	 * @param setter
+	 */
+	public OptionWithValuesListHandler(CmdLineParser parser,
+			OptionDef option, Setter<List<?>> setter) {
+		super(parser, option, setter);
+	}
+
+	@Override
+	public int parseArguments(Parameters params) throws CmdLineException {
+		final List<String> list = new ArrayList<>();
+		for (int idx = 0; idx < params.size(); idx++) {
+			final String p;
+			try {
+				p = params.getParameter(idx);
+			} catch (CmdLineException cle) {
+				break;
+			}
+			list.add(p);
+		}
+		setter.addValue(list);
+		return list.size();
+	}
+
+	@Override
+	public String getDefaultMetaVariable() {
+		return CLIText.get().metaVar_values;
+	}
+
+}