From 6bc48cdc62287934ce1b7003280b19a5994e7668 Mon Sep 17 00:00:00 2001 From: Christian Halstrick Date: Wed, 4 Jul 2012 15:08:13 +0200 Subject: [PATCH] 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 --- .../eclipse/jgit/merge/SimpleMergeTest.java | 108 ++++++++++++++++++ .../jgit/revplot/PlotCommitListTest.java | 26 ----- .../eclipse/jgit/internal/JGitText.properties | 1 + .../org/eclipse/jgit/internal/JGitText.java | 1 + .../org/eclipse/jgit/lib/CommitBuilder.java | 44 +++++-- 5 files changed, 147 insertions(+), 33 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 db9d87dd17..d86139da95 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,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 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 ecc119b29a..0ee9f570ba 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,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 pcl = new PlotCommitList(); - 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 a003b02ccd..61f396c0b5 100644 --- 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 63b788f6b2..9a7a461018 100644 --- 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 c5c488dac3..70b4d3b203 100644 --- 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 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(); -- 2.39.5