RenameBranchCommand: more consistent handling of short ref names

Several problems:

* The command didn't specify whether it expected short or full names.
* For the new name, it expected a short name, but then got confused
  if tags or both local and remote branches with the same name existed.
* For the old name, it accepted either a short or a full name, but
  again got confused if a short name was given and a tag with the
  same name existed.

With such an interface, one cannot use Repository.findRef() to
reliably find the branch to rename. Use exactRef() for the new
name as by the time the Ref is needed its full name is known.
For determining the old Ref from the name, do the resolution
explicitly: first try exactRef (assuming the old name is a full
name); if that doesn't find anything, try "refs/heads/<old>" and
"refs/remotes/<old>" explicitly. Throw an exception if the name
is ambiguous, or if exactRef returned something that is not a
branch (refs/tags/... or also refs/notes/...).

Document in the javadoc what kind of names are valid, and add tests.

A user can still shoot himself in the foot if he chooses exceptionally
stupid branch names. For instance, it is still possible to rename a
branch to "refs/heads/foo" (full name "refs/heads/refs/heads/foo"),
but it cannot be renamed further using the new short name if a branch
with the full name "refs/heads/foo" exists. Similar edge cases exist
for other dumb branch names, like a branch with the short name
"refs/tags/foo". Renaming using the full name is always possible.

Bug: 542446
Change-Id: I34ac91c80c0a00c79a384d16ce1e727c550d54e9
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>

org.eclipse.jgit.test/tst/org/eclipse/jgit/api/RenameBranchCommandTest.java

@@ -49,14 +49,18 @@ import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 
+import org.eclipse.jgit.api.errors.RefAlreadyExistsException;
 import org.eclipse.jgit.junit.RepositoryTestCase;
 import org.eclipse.jgit.lib.BranchConfig.BranchRebaseMode;
 import org.eclipse.jgit.lib.ConfigConstants;
 import org.eclipse.jgit.lib.Constants;
+import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.lib.StoredConfig;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.junit.Before;
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.ExpectedException;
 
 /**
  * Unit tests of {@link RenameBranchCommand}
@@ -69,6 +73,9 @@ public class RenameBranchCommandTest extends RepositoryTestCase {
 
 	private Git git;
 
+	@Rule
+	public ExpectedException thrown = ExpectedException.none();
+
 	@Override
 	@Before
 	public void setUp() throws Exception {
@@ -80,6 +87,45 @@ public class RenameBranchCommandTest extends RepositoryTestCase {
 		assertNotNull(head);
 	}
 
+	@Test
+	public void renameToExisting() throws Exception {
+		assertNotNull(git.branchCreate().setName("foo").call());
+		thrown.expect(RefAlreadyExistsException.class);
+		git.branchRename().setOldName("master").setNewName("foo").call();
+	}
+
+	@Test
+	public void renameToTag() throws Exception {
+		Ref ref = git.tag().setName("foo").call();
+		assertNotNull(ref);
+		assertEquals("Unexpected tag name", Constants.R_TAGS + "foo",
+				ref.getName());
+		ref = git.branchRename().setNewName("foo").call();
+		assertNotNull(ref);
+		assertEquals("Unexpected ref name", Constants.R_HEADS + "foo",
+				ref.getName());
+		// Check that we can rename it back
+		ref = git.branchRename().setOldName("foo").setNewName(Constants.MASTER)
+				.call();
+		assertNotNull(ref);
+		assertEquals("Unexpected ref name",
+				Constants.R_HEADS + Constants.MASTER, ref.getName());
+	}
+
+	@Test
+	public void renameToStupidName() throws Exception {
+		Ref ref = git.branchRename().setNewName(Constants.R_HEADS + "foo")
+				.call();
+		assertEquals("Unexpected ref name",
+				Constants.R_HEADS + Constants.R_HEADS + "foo",
+				ref.getName());
+		// And check that we can rename it back to a sane name
+		ref = git.branchRename().setNewName("foo").call();
+		assertNotNull(ref);
+		assertEquals("Unexpected ref name", Constants.R_HEADS + "foo",
+				ref.getName());
+	}
+
 	@Test
 	public void renameBranchNoConfigValues() throws Exception {
 		StoredConfig config = git.getRepository().getConfig();

org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties

@@ -590,8 +590,9 @@ remoteConfigHasNoURIAssociated=Remote config "{0}" has no URIs associated
 remoteDoesNotHaveSpec=Remote does not have {0} available for fetch.
 remoteDoesNotSupportSmartHTTPPush=remote does not support smart HTTP push
 remoteHungUpUnexpectedly=remote hung up unexpectedly
-remoteNameCantBeNull=Remote name can't be null.
+remoteNameCannotBeNull=Remote name cannot be null.
-renameBranchFailedBecauseTag=Can not rename as Ref {0} is a tag
+renameBranchFailedAmbiguous=Cannot rename branch {0}; name is ambiguous: {1} or {2}
+renameBranchFailedNotABranch=Cannot rename {0}: this is not a branch
 renameBranchFailedUnknownReason=Rename failed with unknown reason
 renameBranchUnexpectedResult=Unexpected rename result {0}
 renameCancelled=Rename detection was cancelled

org.eclipse.jgit/src/org/eclipse/jgit/api/RenameBranchCommand.java

@@ -94,25 +94,38 @@ public class RenameBranchCommand extends GitCommand<Ref> {
 			RefAlreadyExistsException, DetachedHeadException {
 		checkCallable();
 
-		if (newName == null)
+		if (newName == null) {
 			throw new InvalidRefNameException(MessageFormat.format(JGitText
 					.get().branchNameInvalid, "<null>")); //$NON-NLS-1$
-
+		}
 		try {
 			String fullOldName;
 			String fullNewName;
-			if (repo.findRef(newName) != null)
-				throw new RefAlreadyExistsException(MessageFormat.format(
-						JGitText.get().refAlreadyExists1, newName));
 			if (oldName != null) {
-				Ref ref = repo.findRef(oldName);
+				// Don't just rely on findRef -- if there are local and remote
-				if (ref == null)
+				// branches with the same name, and oldName is a short name, it
-					throw new RefNotFoundException(MessageFormat.format(
+				// does not uniquely identify the ref and we might end up
-							JGitText.get().refNotResolved, oldName));
+				// renaming the wrong branch or finding a tag instead even
-				if (ref.getName().startsWith(Constants.R_TAGS))
+				// if a unique branch for the name exists!
-					throw new RefNotFoundException(MessageFormat.format(
+				//
-							JGitText.get().renameBranchFailedBecauseTag,
+				// OldName may be a either a short or a full name.
-							oldName));
+				Ref ref = repo.exactRef(oldName);
+				if (ref == null) {
+					ref = repo.exactRef(Constants.R_HEADS + oldName);
+					Ref ref2 = repo.exactRef(Constants.R_REMOTES + oldName);
+					if (ref != null && ref2 != null) {
+						throw new RefNotFoundException(MessageFormat.format(
+								JGitText.get().renameBranchFailedAmbiguous,
+								oldName, ref.getName(), ref2.getName()));
+					} else if (ref == null) {
+						if (ref2 != null) {
+							ref = ref2;
+						} else {
+							throw new RefNotFoundException(MessageFormat.format(
+									JGitText.get().refNotResolved, oldName));
+						}
+					}
+				}
 				fullOldName = ref.getName();
 			} else {
 				fullOldName = repo.getFullBranch();
@@ -124,26 +137,34 @@ public class RenameBranchCommand extends GitCommand<Ref> {
 					throw new DetachedHeadException();
 			}
 
-			if (fullOldName.startsWith(Constants.R_REMOTES))
+			if (fullOldName.startsWith(Constants.R_REMOTES)) {
 				fullNewName = Constants.R_REMOTES + newName;
-			else {
+			} else if (fullOldName.startsWith(Constants.R_HEADS)) {
 				fullNewName = Constants.R_HEADS + newName;
+			} else {
+				throw new RefNotFoundException(MessageFormat.format(
+						JGitText.get().renameBranchFailedNotABranch,
+						fullOldName));
 			}
 
-			if (!Repository.isValidRefName(fullNewName))
+			if (!Repository.isValidRefName(fullNewName)) {
 				throw new InvalidRefNameException(MessageFormat.format(JGitText
 						.get().branchNameInvalid, fullNewName));
-
+			}
+			if (repo.exactRef(fullNewName) != null) {
+				throw new RefAlreadyExistsException(MessageFormat
+						.format(JGitText.get().refAlreadyExists1, fullNewName));
+			}
 			RefRename rename = repo.renameRef(fullOldName, fullNewName);
 			Result renameResult = rename.rename();
 
 			setCallable(false);
 
-			if (Result.RENAMED != renameResult)
+			if (Result.RENAMED != renameResult) {
 				throw new JGitInternalException(MessageFormat.format(JGitText
 						.get().renameBranchUnexpectedResult, renameResult
 						.name()));
-
+			}
 			if (fullNewName.startsWith(Constants.R_HEADS)) {
 				String shortOldName = fullOldName.substring(Constants.R_HEADS
 						.length());
@@ -154,8 +175,9 @@ public class RenameBranchCommand extends GitCommand<Ref> {
 					String[] values = repoConfig.getStringList(
 							ConfigConstants.CONFIG_BRANCH_SECTION,
 							shortOldName, name);
-					if (values.length == 0)
+					if (values.length == 0) {
 						continue;
+					}
 					// Keep any existing values already configured for the
 					// new branch name
 					String[] existing = repoConfig.getStringList(
@@ -180,10 +202,11 @@ public class RenameBranchCommand extends GitCommand<Ref> {
 				repoConfig.save();
 			}
 
-			Ref resultRef = repo.findRef(newName);
+			Ref resultRef = repo.exactRef(fullNewName);
-			if (resultRef == null)
+			if (resultRef == null) {
 				throw new JGitInternalException(
 						JGitText.get().renameBranchFailedUnknownReason);
+			}
 			return resultRef;
 		} catch (IOException ioe) {
 			throw new JGitInternalException(ioe.getMessage(), ioe);
@@ -191,7 +214,13 @@ public class RenameBranchCommand extends GitCommand<Ref> {
 	}
 
 	/**
-	 * Set the new name of the branch
+	 * Sets the new short name of the branch.
+	 * <p>
+	 * The full name is constructed using the prefix of the branch to be renamed
+	 * defined by either {@link #setOldName(String)} or HEAD. If that old branch
+	 * is a local branch, the renamed branch also will be, and if the old branch
+	 * is a remote branch, so will be the renamed branch.
+	 * </p>
 	 *
 	 * @param newName
 	 *            the new name
@@ -204,7 +233,11 @@ public class RenameBranchCommand extends GitCommand<Ref> {
 	}
 
 	/**
-	 * Set the old name of the branch
+	 * Sets the old name of the branch.
+	 * <p>
+	 * {@code oldName} may be a short or a full name. Using a full name is
+	 * recommended to unambiguously identify the branch to be renamed.
+	 * </p>
 	 *
 	 * @param oldName
 	 *            the name of the branch to rename; if not set, the currently

org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java

@@ -651,8 +651,9 @@ public class JGitText extends TranslationBundle {
 	/***/ public String remoteDoesNotHaveSpec;
 	/***/ public String remoteDoesNotSupportSmartHTTPPush;
 	/***/ public String remoteHungUpUnexpectedly;
-	/***/ public String remoteNameCantBeNull;
+	/***/ public String remoteNameCannotBeNull;
-	/***/ public String renameBranchFailedBecauseTag;
+	/***/ public String renameBranchFailedAmbiguous;
+	/***/ public String renameBranchFailedNotABranch;
 	/***/ public String renameBranchFailedUnknownReason;
 	/***/ public String renameBranchUnexpectedResult;
 	/***/ public String renameCancelled;

org.eclipse.jgit/src/org/eclipse/jgit/transport/RemoteRefUpdate.java

@@ -293,7 +293,7 @@ public class RemoteRefUpdate {
 			final boolean forceUpdate, final String localName,
 			final ObjectId expectedOldObjectId) throws IOException {
 		if (remoteName == null)
-			throw new IllegalArgumentException(JGitText.get().remoteNameCantBeNull);
+			throw new IllegalArgumentException(JGitText.get().remoteNameCannotBeNull);
 		if (srcId == null && srcRef != null)
 			throw new IOException(MessageFormat.format(
 					JGitText.get().sourceRefDoesntResolveToAnyObject, srcRef));