CommitBuilder should check for duplicate parents

When setting the parents of a commit with setParentIds() or
addParentId() it should be checked that we don't have duplicate parents.
An IllegalArgumentException should be thrown in this case.

Change-Id: I9fa9f31149b7732071b304bca232f037146de454
Signed-off-by: Christian Halstrick <christian.halstrick@sap.com>

org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/SimpleMergeTest.java

@@ -47,16 +47,22 @@ package org.eclipse.jgit.merge;
 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.IOException;
+import java.util.Arrays;
+import java.util.List;
 
 import org.eclipse.jgit.dircache.DirCache;
 import org.eclipse.jgit.dircache.DirCacheBuilder;
+import org.eclipse.jgit.lib.AnyObjectId;
 import org.eclipse.jgit.lib.CommitBuilder;
 import org.eclipse.jgit.lib.FileMode;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.ObjectInserter;
 import org.eclipse.jgit.lib.PersonIdent;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevWalk;
 import org.eclipse.jgit.test.resources.SampleDataRepositoryTestCase;
 import org.eclipse.jgit.treewalk.TreeWalk;
 import org.junit.Test;
@@ -87,6 +93,81 @@ public class SimpleMergeTest extends SampleDataRepositoryTestCase {
 		assertEquals("02ba32d3649e510002c21651936b7077aa75ffa9",ourMerger.getResultTreeId().name());
 	}
 
+	@Test
+	public void testDuplicateParents() throws Exception {
+		ObjectId commitId;
+		RevCommit newCommit;
+		final ObjectInserter ow = db.newObjectInserter();
+		RevWalk rw = new RevWalk(db);
+		ObjectId parentA = db.resolve("a");
+		ObjectId parentB = db.resolve("b");
+		ObjectId[] parentIds_AA = new ObjectId[] { parentA, parentA };
+		ObjectId[] parentIds_AB = new ObjectId[] { parentA, parentB };
+		ObjectId[] parentIds_BA = new ObjectId[] { parentB, parentA };
+		ObjectId[] parentIds_BBAB = new ObjectId[] { parentB, parentB, parentA,
+				parentB };
+
+		try {
+			commitId = commit(ow, db.readDirCache(), parentA, parentA);
+			fail("an expected exception did not occur");
+		} catch (IllegalArgumentException e) {
+			//
+		}
+
+		commitId = commit(ow, db.readDirCache(), parentA, parentB);
+		newCommit = rw.parseCommit(commitId);
+		assertEquals(2, newCommit.getParentCount());
+
+		commitId = commit(ow, db.readDirCache(), parentB, parentA);
+		newCommit = rw.parseCommit(commitId);
+		assertEquals(2, newCommit.getParentCount());
+
+		try {
+			commitId = commit(ow, db.readDirCache(), parentIds_AA);
+			fail("an expected exception did not occur");
+		} catch (IllegalArgumentException e) {
+			//
+		}
+
+		commitId = commit(ow, db.readDirCache(), parentIds_AB);
+		newCommit = rw.parseCommit(commitId);
+		assertEquals(2, newCommit.getParentCount());
+
+		commitId = commit(ow, db.readDirCache(), parentIds_BA);
+		newCommit = rw.parseCommit(commitId);
+		assertEquals(2, newCommit.getParentCount());
+
+		try {
+			commitId = commit(ow, db.readDirCache(), parentIds_BBAB);
+			fail("an expected exception did not occur");
+		} catch (IllegalArgumentException e) {
+			//
+		}
+
+		try {
+			commitId = commit(ow, db.readDirCache(),
+					Arrays.asList(parentIds_AA));
+			fail("an expected exception did not occur");
+		} catch (IllegalArgumentException e) {
+			//
+		}
+
+		commitId = commit(ow, db.readDirCache(), parentIds_AB);
+		newCommit = rw.parseCommit(commitId);
+		assertEquals(2, newCommit.getParentCount());
+
+		commitId = commit(ow, db.readDirCache(), parentIds_BA);
+		newCommit = rw.parseCommit(commitId);
+		assertEquals(2, newCommit.getParentCount());
+
+		try {
+			commitId = commit(ow, db.readDirCache(), parentIds_BBAB);
+			fail("an expected exception did not occur");
+		} catch (IllegalArgumentException e) {
+			//
+		}
+	}
+
 	@Test
 	public void testTrivialTwoWay_disjointhistories() throws IOException {
 		Merger ourMerger = MergeStrategy.SIMPLE_TWO_WAY_IN_CORE.newMerger(db);
@@ -391,4 +472,31 @@ public class SimpleMergeTest extends SampleDataRepositoryTestCase {
 		odi.flush();
 		return id;
 	}
+
+	private ObjectId commit(final ObjectInserter odi, final DirCache treeB,
+			final AnyObjectId parentId1, final AnyObjectId parentId2)
+			throws Exception {
+		final CommitBuilder c = new CommitBuilder();
+		c.setTreeId(treeB.writeTree(odi));
+		c.setAuthor(new PersonIdent("A U Thor", "a.u.thor", 1L, 0));
+		c.setCommitter(c.getAuthor());
+		c.setParentIds(parentId1, parentId2);
+		c.setMessage("Tree " + c.getTreeId().name());
+		ObjectId id = odi.insert(c);
+		odi.flush();
+		return id;
+	}
+
+	private ObjectId commit(final ObjectInserter odi, final DirCache treeB,
+			List<ObjectId> parents) throws Exception {
+		final CommitBuilder c = new CommitBuilder();
+		c.setTreeId(treeB.writeTree(odi));
+		c.setAuthor(new PersonIdent("A U Thor", "a.u.thor", 1L, 0));
+		c.setCommitter(c.getAuthor());
+		c.setParentIds(parents);
+		c.setMessage("Tree " + c.getTreeId().name());
+		ObjectId id = odi.insert(c);
+		odi.flush();
+		return id;
+	}
 }

org.eclipse.jgit.test/tst/org/eclipse/jgit/revplot/PlotCommitListTest.java

@@ -393,32 +393,6 @@ public class PlotCommitListTest extends RevWalkTestCase {
 		test.noMoreCommits();
 	}
 
-	// test a history where a merge commit has two time the same parent
-	@Test
-	public void testDuplicateParents() throws Exception {
-		final RevCommit m1 = commit();
-		final RevCommit m2 = commit(m1);
-		final RevCommit m3 = commit(m2, m2);
-
-		final RevCommit s1 = commit(m2);
-		final RevCommit s2 = commit(s1);
-
-		PlotWalk pw = new PlotWalk(db);
-		pw.markStart(pw.lookupCommit(m3));
-		pw.markStart(pw.lookupCommit(s2));
-		PlotCommitList<PlotLane> pcl = new PlotCommitList<PlotLane>();
-		pcl.source(pw);
-		pcl.fillTo(Integer.MAX_VALUE);
-
-		CommitListAssert test = new CommitListAssert(pcl);
-		test.commit(s2).nrOfPassingLanes(0);
-		test.commit(s1).nrOfPassingLanes(0);
-		test.commit(m3).nrOfPassingLanes(1);
-		test.commit(m2).nrOfPassingLanes(0);
-		test.commit(m1).nrOfPassingLanes(0);
-		test.noMoreCommits();
-	}
-
 	/**
 	 * The graph shows the problematic original positioning. Due to this some
 	 * lanes are no straight lines here, but they are with the new layout code)

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

@@ -173,6 +173,7 @@ doesNotHandleMode=Does not handle mode {0} ({1})
 downloadCancelled=Download cancelled
 downloadCancelledDuringIndexing=Download cancelled during indexing
 duplicateAdvertisementsOf=duplicate advertisements of {0}
+duplicateParents=The following parent was specified multiple times: {0}
 duplicateRef=Duplicate ref: {0}
 duplicateRemoteRefUpdateIsIllegal=Duplicate remote ref update is illegal. Affected remote name: {0}
 duplicateStagesNotAllowed=Duplicate stages not allowed

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

@@ -232,6 +232,7 @@ public class JGitText extends TranslationBundle {
 	/***/ public String downloadCancelled;
 	/***/ public String downloadCancelledDuringIndexing;
 	/***/ public String duplicateAdvertisementsOf;
+	/***/ public String duplicateParents;
 	/***/ public String duplicateRef;
 	/***/ public String duplicateRemoteRefUpdateIsIllegal;
 	/***/ public String duplicateStagesNotAllowed;

org.eclipse.jgit/src/org/eclipse/jgit/lib/CommitBuilder.java

@@ -50,8 +50,11 @@ import java.io.IOException;
 import java.io.OutputStreamWriter;
 import java.io.UnsupportedEncodingException;
 import java.nio.charset.Charset;
+import java.text.MessageFormat;
 import java.util.List;
 
+import org.eclipse.jgit.internal.JGitText;
+
 /**
  * Mutable builder to construct a commit recording the state of a project.
  *
@@ -166,7 +169,11 @@ public class CommitBuilder {
 	 *            branch being merged into the current branch.
 	 */
 	public void setParentIds(AnyObjectId parent1, AnyObjectId parent2) {
-		parentIds = new ObjectId[] { parent1.copy(), parent2.copy() };
+		if (!parent1.equals(parent2))
+			parentIds = new ObjectId[] { parent1.copy(), parent2.copy() };
+		else
+			throw new IllegalArgumentException(MessageFormat.format(
+					JGitText.get().duplicateParents, parent1.getName()));
 	}
 
 	/**
@@ -176,9 +183,18 @@ public class CommitBuilder {
 	 *            the entire list of parents for this commit.
 	 */
 	public void setParentIds(ObjectId... newParents) {
-		parentIds = new ObjectId[newParents.length];
+		ObjectId[] tmpIds = new ObjectId[newParents.length];
-		for (int i = 0; i < newParents.length; i++)
+
-			parentIds[i] = newParents[i].copy();
+		int newParentCount = 0;
+		for (int i = 0; i < newParents.length; i++) {
+			for (int j = 0; j < newParentCount; j++)
+				if (tmpIds[j].equals(newParents[i]))
+					throw new IllegalArgumentException(MessageFormat.format(
+							JGitText.get().duplicateParents,
+							tmpIds[j].getName()));
+			tmpIds[newParentCount++] = newParents[i].copy();
+		}
+		parentIds = tmpIds;
 	}
 
 	/**
@@ -188,9 +204,18 @@ public class CommitBuilder {
 	 *            the entire list of parents for this commit.
 	 */
 	public void setParentIds(List<? extends AnyObjectId> newParents) {
-		parentIds = new ObjectId[newParents.size()];
+		ObjectId[] tmpIds = new ObjectId[newParents.size()];
-		for (int i = 0; i < newParents.size(); i++)
+
-			parentIds[i] = newParents.get(i).copy();
+		int newParentCount = 0;
+		for (AnyObjectId newId : newParents) {
+			for (int j = 0; j < newParentCount; j++)
+				if (tmpIds[j].equals(newId))
+					throw new IllegalArgumentException(MessageFormat.format(
+							JGitText.get().duplicateParents,
+							tmpIds[j].getName()));
+			tmpIds[newParentCount++] = newId.copy();
+		}
+		parentIds = tmpIds;
 	}
 
 	/**
@@ -203,6 +228,11 @@ public class CommitBuilder {
 		if (parentIds.length == 0) {
 			setParentId(additionalParent);
 		} else {
+			for (int i = 0; i < parentIds.length; i++)
+				if (parentIds[i].equals(additionalParent))
+					throw new IllegalArgumentException(MessageFormat.format(
+							JGitText.get().duplicateParents,
+							parentIds[i].getName()));
 			ObjectId[] newParents = new ObjectId[parentIds.length + 1];
 			System.arraycopy(parentIds, 0, newParents, 0, parentIds.length);
 			newParents[parentIds.length] = additionalParent.copy();