Revert "CommitBuilder should check for duplicate parents"

This reverts commit 6bc48cdc62287934ce1b7003280b19a5994e7668.

Until git v1.7.10.2~29^2~1 (builtin/merge.c: reduce parents early,
2012-04-17), C git merge would make merge commits with duplicate parents
when asked to with a series of commands like the following:

  git checkout origin/master
  git merge --no-ff origin/master

Nowadays "git merge" removes redundant parents more aggressively
(whenever one parent is an ancestor of another and not just when
duplicates exist) but merges with duplicate parents are still permitted
and can be created with git fast-import or git commit-tree and history
viewers need to be able to cope with them.

CommitBuilder is an interface analagous to commit-tree, so it should
allow duplicate parents.  (That said, an option to automatically remove
redundant parents would be useful.)

Reported-by: Dave Borowitz <dborowitz@google.com>
Change-Id: Ia682238397eb1de8541802210fa875fdd50f62f0
Signed-off-by: Jonathan Nieder <jrn@google.com>

5 files changed, 33 insertions, 147 deletions

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/SimpleMergeTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/SimpleMergeTest.java
index d86139da95..db9d87dd17 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/SimpleMergeTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/SimpleMergeTest.java
@@ -47,22 +47,16 @@ 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;
@@ -94,81 +88,6 @@ public class SimpleMergeTest extends SampleDataRepositoryTestCase {
 	}
 
 	@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);
 		boolean merge = ourMerger.merge(new ObjectId[] { db.resolve("a"), db.resolve("c~4") });
@@ -472,31 +391,4 @@ 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;
-	}
 }
diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/revplot/PlotCommitListTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/revplot/PlotCommitListTest.java
index 0ee9f570ba..ecc119b29a 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/revplot/PlotCommitListTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/revplot/PlotCommitListTest.java
@@ -393,6 +393,32 @@ 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)
diff --git a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
index 61f396c0b5..a003b02ccd 100644
--- a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
+++ b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
@@ -173,7 +173,6 @@ 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
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
index 9a7a461018..63b788f6b2 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
@@ -232,7 +232,6 @@ 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;
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/CommitBuilder.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/CommitBuilder.java
index 70b4d3b203..c5c488dac3 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/CommitBuilder.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/CommitBuilder.java
@@ -50,11 +50,8 @@ 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.
  *
@@ -169,11 +166,7 @@ public class CommitBuilder {
 	 *            branch being merged into the current branch.
 	 */
 	public void setParentIds(AnyObjectId parent1, AnyObjectId parent2) {
-		if (!parent1.equals(parent2))
-			parentIds = new ObjectId[] { parent1.copy(), parent2.copy() };
-		else
-			throw new IllegalArgumentException(MessageFormat.format(
-					JGitText.get().duplicateParents, parent1.getName()));
+		parentIds = new ObjectId[] { parent1.copy(), parent2.copy() };
 	}
 
 	/**
@@ -183,18 +176,9 @@ public class CommitBuilder {
 	 *            the entire list of parents for this commit.
 	 */
 	public void setParentIds(ObjectId... newParents) {
-		ObjectId[] tmpIds = new ObjectId[newParents.length];
-
-		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;
+		parentIds = new ObjectId[newParents.length];
+		for (int i = 0; i < newParents.length; i++)
+			parentIds[i] = newParents[i].copy();
 	}
 
 	/**
@@ -204,18 +188,9 @@ public class CommitBuilder {
 	 *            the entire list of parents for this commit.
 	 */
 	public void setParentIds(List<? extends AnyObjectId> newParents) {
-		ObjectId[] tmpIds = new ObjectId[newParents.size()];
-
-		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;
+		parentIds = new ObjectId[newParents.size()];
+		for (int i = 0; i < newParents.size(); i++)
+			parentIds[i] = newParents.get(i).copy();
 	}
 
 	/**
@@ -228,11 +203,6 @@ 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();