From 5967b658381f281496ed61e1dd170cef9bacd7aa Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Wed, 18 Mar 2015 16:26:05 -0700 Subject: [PATCH] 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 Change-Id: Ia682238397eb1de8541802210fa875fdd50f62f0 Signed-off-by: Jonathan Nieder --- .../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, 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 d86139da9..db9d87dd1 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 @@ 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; @@ -93,81 +87,6 @@ public void testTrivialTwoWay() throws IOException { 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); @@ -472,31 +391,4 @@ private static ObjectId commit(final ObjectInserter odi, 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 0ee9f570b..ecc119b29 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 void testEgitHistory() throws Exception { 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 61f396c0b..a003b02cc 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 9a7a46101..63b788f6b 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 static JGitText get() { /***/ 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 70b4d3b20..c5c488dac 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.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 void setParentId(AnyObjectId newParent) { * 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 void setParentIds(AnyObjectId parent1, AnyObjectId parent2) { * 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 void setParentIds(ObjectId... newParents) { * the entire list of parents for this commit. */ public void setParentIds(List 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 void addParentId(AnyObjectId additionalParent) { 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();