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>

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 db9d87dd17e00ce3dd2f83850b6f8d7d80024f43..d86139da95b9ac90f3754be7bd7e569001f9ddc4 100644 (file)
--- 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,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;
+       }
 }

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 ecc119b29a7c4d2895b6369508f71d9acd0d6af7..0ee9f570ba9f116bd6c8d6a88c8e4de00c8d38f4 100644 (file)
--- 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,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)

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 a003b02ccdda829c68331c243c7995a4b39d7111..61f396c0b5467a39f639b300ba29308ea9621659 100644 (file)
--- a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
+++ b/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

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 63b788f6b260f3589a35523428caab5e4c635cbb..9a7a461018fd43201fdb956ca80c3a950e6f0eae 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
+++ b/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;

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 c5c488dac373525fcf76d1448103f550c66a0e92..70b4d3b2038d8f4f461afadbeccdd6845eabef8c 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/CommitBuilder.java
+++ b/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];
-               for (int i = 0; i < newParents.length; i++)
-                       parentIds[i] = newParents[i].copy();
+               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;
        }
 
        /**
@@ -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()];
-               for (int i = 0; i < newParents.size(); i++)
-                       parentIds[i] = newParents.get(i).copy();
+               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;
        }
 
        /**
@@ -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();