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>

org.eclipse.jgit.pgm.test/tst/org/eclipse/jgit/pgm/BranchTest.java

@@ -43,6 +43,11 @@
 package org.eclipse.jgit.pgm;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+import java.io.File;
 
 import org.eclipse.jgit.api.Git;
 import org.eclipse.jgit.lib.CLIRepositoryTestCase;
@@ -62,8 +67,9 @@ public class BranchTest extends CLIRepositoryTestCase {
 
 	@Test
 	public void testList() throws Exception {
+		assertEquals("* master", toString(execute("git branch")));
 		assertEquals("* master 6fd41be initial commit",
-				execute("git branch -v")[0]);
+				toString(execute("git branch -v")));
 	}
 
 	@Test
@@ -71,8 +77,10 @@ public class BranchTest extends CLIRepositoryTestCase {
 		RefUpdate updateRef = db.updateRef(Constants.HEAD, true);
 		updateRef.setNewObjectId(db.resolve("6fd41be"));
 		updateRef.update();
-		assertEquals("* (no branch) 6fd41be initial commit",
+		assertEquals(
-				execute("git branch -v")[0]);
+				toString("* (no branch) 6fd41be initial commit",
+						"master      6fd41be initial commit"),
+				toString(execute("git branch -v")));
 	}
 
 	@Test
@@ -80,15 +88,175 @@ public class BranchTest extends CLIRepositoryTestCase {
 		new Git(db).branchCreate().setName("initial").call();
 		RevCommit second = new Git(db).commit().setMessage("second commit")
 				.call();
-		assertArrayOfLinesEquals(new String[] { "  initial", "* master", "" },
+		assertEquals(toString("  initial", "* master"),
-				execute("git branch --contains 6fd41be"));
+				toString(execute("git branch --contains 6fd41be")));
-		assertArrayOfLinesEquals(new String[] { "* master", "" },
+		assertEquals("* master",
-				execute("git branch --contains " + second.name()));
+				toString(execute("git branch --contains " + second.name())));
 	}
 
 	@Test
 	public void testExistingBranch() throws Exception {
 		assertEquals("fatal: A branch named 'master' already exists.",
-				executeUnchecked("git branch master")[0]);
+				toString(executeUnchecked("git branch master")));
+	}
+
+	@Test
+	public void testRenameSingleArg() throws Exception {
+		try {
+			toString(execute("git branch -m"));
+			fail("Must die");
+		} catch (Die e) {
+			// expected, requires argument
+		}
+		String result = toString(execute("git branch -m slave"));
+		assertEquals("", result);
+		result = toString(execute("git branch -a"));
+		assertEquals("* slave", result);
+	}
+
+	@Test
+	public void testRenameTwoArgs() throws Exception {
+		String result = toString(execute("git branch -m master slave"));
+		assertEquals("", result);
+		result = toString(execute("git branch -a"));
+		assertEquals("* slave", result);
+	}
+
+	@Test
+	public void testCreate() throws Exception {
+		try {
+			toString(execute("git branch a b"));
+			fail("Must die");
+		} catch (Die e) {
+			// expected, too many arguments
+		}
+		String result = toString(execute("git branch second"));
+		assertEquals("", result);
+		result = toString(execute("git branch"));
+		assertEquals(toString("* master", "second"), result);
+		result = toString(execute("git branch -v"));
+		assertEquals(toString("* master 6fd41be initial commit",
+				"second 6fd41be initial commit"), result);
+	}
+
+	@Test
+	public void testDelete() throws Exception {
+		try {
+			toString(execute("git branch -d"));
+			fail("Must die");
+		} catch (Die e) {
+			// expected, requires argument
+		}
+		String result = toString(execute("git branch second"));
+		assertEquals("", result);
+		result = toString(execute("git branch -d second"));
+		assertEquals("", result);
+		result = toString(execute("git branch"));
+		assertEquals("* master", result);
+	}
+
+	@Test
+	public void testDeleteMultiple() throws Exception {
+		String result = toString(execute("git branch second",
+				"git branch third", "git branch fourth"));
+		assertEquals("", result);
+		result = toString(execute("git branch -d second third fourth"));
+		assertEquals("", result);
+		result = toString(execute("git branch"));
+		assertEquals("* master", result);
+	}
+
+	@Test
+	public void testDeleteForce() throws Exception {
+		try {
+			toString(execute("git branch -D"));
+			fail("Must die");
+		} catch (Die e) {
+			// expected, requires argument
+		}
+		String result = toString(execute("git branch second"));
+		assertEquals("", result);
+		result = toString(execute("git checkout second"));
+		assertEquals("Switched to branch 'second'", result);
+
+		File a = writeTrashFile("a", "a");
+		assertTrue(a.exists());
+		execute("git add a", "git commit -m 'added a'");
+
+		result = toString(execute("git checkout master"));
+		assertEquals("Switched to branch 'master'", result);
+
+		result = toString(execute("git branch"));
+		assertEquals(toString("* master", "second"), result);
+
+		try {
+			toString(execute("git branch -d second"));
+			fail("Must die");
+		} catch (Die e) {
+			// expected, the current HEAD is on second and not merged to master
+		}
+		result = toString(execute("git branch -D second"));
+		assertEquals("", result);
+
+		result = toString(execute("git branch"));
+		assertEquals("* master", result);
+	}
+
+	@Test
+	public void testDeleteForceMultiple() throws Exception {
+		String result = toString(execute("git branch second",
+				"git branch third", "git branch fourth"));
+
+		assertEquals("", result);
+		result = toString(execute("git checkout second"));
+		assertEquals("Switched to branch 'second'", result);
+
+		File a = writeTrashFile("a", "a");
+		assertTrue(a.exists());
+		execute("git add a", "git commit -m 'added a'");
+
+		result = toString(execute("git checkout master"));
+		assertEquals("Switched to branch 'master'", result);
+
+		result = toString(execute("git branch"));
+		assertEquals(toString("fourth", "* master", "second", "third"), result);
+
+		try {
+			toString(execute("git branch -d second third fourth"));
+			fail("Must die");
+		} catch (Die e) {
+			// expected, the current HEAD is on second and not merged to master
+		}
+		result = toString(execute("git branch"));
+		assertEquals(toString("fourth", "* master", "second", "third"), result);
+
+		result = toString(execute("git branch -D second third fourth"));
+		assertEquals("", result);
+
+		result = toString(execute("git branch"));
+		assertEquals("* master", result);
+	}
+
+	@Test
+	public void testCreateFromOldCommit() throws Exception {
+		File a = writeTrashFile("a", "a");
+		assertTrue(a.exists());
+		execute("git add a", "git commit -m 'added a'");
+		File b = writeTrashFile("b", "b");
+		assertTrue(b.exists());
+		execute("git add b", "git commit -m 'added b'");
+		String result = toString(execute("git log -n 1 --reverse"));
+		String firstCommitId = result.substring("commit ".length(),
+				result.indexOf('\n'));
+
+		result = toString(execute("git branch -f second " + firstCommitId));
+		assertEquals("", result);
+
+		result = toString(execute("git branch"));
+		assertEquals(toString("* master", "second"), result);
+
+		result = toString(execute("git checkout second"));
+		assertEquals("Switched to branch 'second'", result);
+		assertFalse(b.exists());
 	}
 }

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

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 List<String> delete;
-	private boolean delete = false;
 
-	@Option(name = "--delete-force", aliases = { "-D" }, usage = "usage_deleteBranchEvenIfNotMerged")
+	@Option(name = "--delete", aliases = {
-	private boolean deleteForce = false;
+			"-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 List<String> deleteForce;
-	private boolean createForce = false;
 
-	@Option(name = "-m", usage = "usage_moveRenameABranch")
+	@Option(name = "--delete-force", aliases = {
-	private boolean rename = false;
+			"-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
+	@Argument(metaVar = "metaVar_name")
-	private List<String> branches = new ArrayList<String>();
+	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)
+		if (delete != null || deleteForce != null) {
-			delete(deleteForce);
+			if (delete != null) {
-		else {
+				delete(delete, false);
-			if (branches.size() > 2)
+			}
-				throw die(CLIText.get().tooManyRefsGiven + new CmdLineParser(this).printExample(ExampleMode.ALL));
+			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) {
+			} else if (createForce || branch != null) {
-				String newHead = branches.get(0);
+				String newHead = branch;
 				String startBranch;
-				if (branches.size() == 2)
+				if (createForce) {
-					startBranch = branches.get(1);
+					startBranch = otherBranch;
-				else
+				} 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) {

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;

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;

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;
+	}
+
+}