Fix exception on conflicts with recursive merge

When there are conflicts with a recursive merge, the conflicting paths
are stored in unmergedPaths (field in ResolveMerger). Later, when the
MergeResult is constructed in MergeCommand, getBaseCommit is called,
which computes the merge base a second time.

In case of RecursiveMerger, getBaseCommit merges the multiple merge
bases into one. It does this not by creating a new ResolveMerger but
instead calling mergeTrees. The problem with mergeTrees is that at the
end, it checks if unmergedPaths is non-empty and returns false in that
case.

Because unmergedPaths was already non-empty because of the real merge,
it thinks that there were conflicts when computing the merge base again,
when there really were none.

This can be fixed by storing the base commit when computing it and then
returning that instead of computing it a second time.

Note that another possible fix would be to just use a new ResolveMerger
for merging the merge bases instead. This would also remove the need to
remember the old value of dircache, inCore and workingTreeIterator (see
RecursiveMerger#getBaseCommit).

Bug: 419641
Change-Id: Ib2ebf4e177498c22a9098aa225e3cfcf16bbd958
Signed-off-by: Robin Stocker <robin@nibor.org>

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/MergeCommandTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/MergeCommandTest.java
index 29146dc5852d93734f44d3205745da497dc9a8f8..1eeb9f7c0d06817bd49b6865f29176ebe1530b63 100644 (file)
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/MergeCommandTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/MergeCommandTest.java
@@ -56,8 +56,11 @@ import org.eclipse.jgit.api.MergeCommand.FastForwardMode;
 import org.eclipse.jgit.api.MergeResult.MergeStatus;
 import org.eclipse.jgit.api.errors.InvalidMergeHeadsException;
 import org.eclipse.jgit.junit.RepositoryTestCase;
+import org.eclipse.jgit.junit.TestRepository;
+import org.eclipse.jgit.junit.TestRepository.BranchBuilder;
 import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.Ref;
+import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.lib.RepositoryState;
 import org.eclipse.jgit.merge.MergeStrategy;
 import org.eclipse.jgit.merge.ResolveMerger.MergeFailureReason;
@@ -1532,6 +1535,37 @@ public class MergeCommandTest extends RepositoryTestCase {
 
                assertEquals(MergeStatus.ABORTED, result.getMergeStatus());
        }
+
+       @Test
+       public void testRecursiveMergeWithConflict() throws Exception {
+               TestRepository<Repository> db_t = new TestRepository<Repository>(db);
+               BranchBuilder master = db_t.branch("master");
+               RevCommit m0 = master.commit().add("f", "1\n2\n3\n4\n5\n6\n7\n8\n9\n")
+                               .message("m0").create();
+               RevCommit m1 = master.commit()
+                               .add("f", "1-master\n2\n3\n4\n5\n6\n7\n8\n9\n").message("m1")
+                               .create();
+               db_t.getRevWalk().parseCommit(m1);
+
+               BranchBuilder side = db_t.branch("side");
+               RevCommit s1 = side.commit().parent(m0)
+                               .add("f", "1\n2\n3\n4\n5\n6\n7\n8\n9-side\n").message("s1")
+                               .create();
+               RevCommit s2 = side.commit().parent(m1)
+                               .add("f", "1-master\n2\n3\n4\n5\n6\n7-res(side)\n8\n9-side\n")
+                               .message("s2(merge)").create();
+               master.commit().parent(s1)
+                               .add("f", "1-master\n2\n3\n4\n5\n6\n7-conflict\n8\n9-side\n")
+                               .message("m2(merge)").create();
+
+               Git git = Git.wrap(db);
+               git.checkout().setName("master").call();
+
+               MergeResult result = git.merge().setStrategy(MergeStrategy.RECURSIVE)
+                               .include("side", s2).call();
+               assertEquals(MergeStatus.CONFLICTING, result.getMergeStatus());
+       }
+
        private static void setExecutable(Git git, String path, boolean executable) {
                FS.DETECTED.setExecute(
                                new File(git.getRepository().getWorkTree(), path), executable);

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/api/MergeCommand.java b/org.eclipse.jgit/src/org/eclipse/jgit/api/MergeCommand.java
index 509203e528fde64c6ccda9a3b9f58e0aafdbcced..78b260df71bb55927e5d988944b73df976c57747 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/api/MergeCommand.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/api/MergeCommand.java
@@ -379,8 +379,7 @@ public class MergeCommand extends GitCommand<MergeResult> {
                                        if (failingPaths != null) {
                                                repo.writeMergeCommitMsg(null);
                                                repo.writeMergeHeads(null);
-                                               return new MergeResult(null,
-                                                               merger.getBaseCommit(0, 1),
+                                               return new MergeResult(null, merger.getBaseCommitId(),
                                                                new ObjectId[] {
                                                                                headCommit.getId(), srcCommit.getId() },
                                                                MergeStatus.FAILED, mergeStrategy,
@@ -390,8 +389,7 @@ public class MergeCommand extends GitCommand<MergeResult> {
                                                                .formatWithConflicts(mergeMessage,
                                                                                unmergedPaths);
                                                repo.writeMergeCommitMsg(mergeMessageWithConflicts);
-                                               return new MergeResult(null,
-                                                               merger.getBaseCommit(0, 1),
+                                               return new MergeResult(null, merger.getBaseCommitId(),
                                                                new ObjectId[] { headCommit.getId(),
                                                                                srcCommit.getId() },
                                                                MergeStatus.CONFLICTING, mergeStrategy,

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/api/RevertCommand.java b/org.eclipse.jgit/src/org/eclipse/jgit/api/RevertCommand.java
index 2bc9d224492a3f877f1ab223af01fa83df1d213a..bc1b29c2f6b9b4b4ce42c7d4b70762dc345c3ae0 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/api/RevertCommand.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/api/RevertCommand.java
@@ -191,14 +191,14 @@ public class RevertCommand extends GitCommand<RevCommit> {
                                                        .getFailingPaths();
                                        if (failingPaths != null)
                                                failingResult = new MergeResult(null,
-                                                               merger.getBaseCommit(0, 1),
+                                                               merger.getBaseCommitId(),
                                                                new ObjectId[] { headCommit.getId(),
                                                                                srcParent.getId() },
                                                                MergeStatus.FAILED, MergeStrategy.RECURSIVE,
                                                                merger.getMergeResults(), failingPaths, null);
                                        else
                                                failingResult = new MergeResult(null,
-                                                               merger.getBaseCommit(0, 1),
+                                                               merger.getBaseCommitId(),
                                                                new ObjectId[] { headCommit.getId(),
                                                                                srcParent.getId() },
                                                                MergeStatus.CONFLICTING,

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/merge/Merger.java b/org.eclipse.jgit/src/org/eclipse/jgit/merge/Merger.java
index 13463287611e4709b8d50b37700bb1f03bc82b21..6dba41298729a3e9868d1ead153d8354ec41a492 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/merge/Merger.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/merge/Merger.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2008-2009, Google Inc.
+ * Copyright (C) 2008-2013, Google Inc.
  * and other copyright owners as documented in the project's IP log.
  *
  * This program and the accompanying materials are made available
@@ -54,8 +54,8 @@ import org.eclipse.jgit.lib.AnyObjectId;
 import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.ObjectInserter;
-import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.lib.ObjectReader;
+import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevObject;
 import org.eclipse.jgit.revwalk.RevTree;
@@ -63,7 +63,6 @@ import org.eclipse.jgit.revwalk.RevWalk;
 import org.eclipse.jgit.revwalk.filter.RevFilter;
 import org.eclipse.jgit.treewalk.AbstractTreeIterator;
 import org.eclipse.jgit.treewalk.CanonicalTreeParser;
-import org.eclipse.jgit.treewalk.EmptyTreeIterator;
 
 /**
  * Instance of a specific {@link MergeStrategy} for a single {@link Repository}.
@@ -186,24 +185,11 @@ public abstract class Merger {
        }
 
        /**
-        * Create an iterator to walk the merge base of two commits.
-        *
-        * @param a
-        *            the first commit in {@link #sourceObjects}.
-        * @param b
-        *            the second commit in {@link #sourceObjects}.
-        * @return the new iterator
-        * @throws IncorrectObjectTypeException
-        *             one of the input objects is not a commit.
-        * @throws IOException
-        *             objects are missing or multiple merge bases were found.
-        * @since 3.0
+        * @return the ID of the commit that was used as merge base for merging, or
+        *         null if no merge base was used or it was set manually
+        * @since 3.2
         */
-       protected AbstractTreeIterator mergeBase(RevCommit a, RevCommit b)
-                       throws IOException {
-               RevCommit base = getBaseCommit(a, b);
-               return (base == null) ? new EmptyTreeIterator() : openTree(base.getTree());
-       }
+       public abstract ObjectId getBaseCommitId();
 
        /**
         * Return the merge base of two commits.
@@ -217,7 +203,10 @@ public abstract class Merger {
         *             one of the input objects is not a commit.
         * @throws IOException
         *             objects are missing or multiple merge bases were found.
+        * @deprecated use {@link #getBaseCommitId()} instead, as that does not
+        *             require walking the commits again
         */
+       @Deprecated
        public RevCommit getBaseCommit(final int aIdx, final int bIdx)
                        throws IncorrectObjectTypeException,
                        IOException {

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/merge/StrategyOneSided.java b/org.eclipse.jgit/src/org/eclipse/jgit/merge/StrategyOneSided.java
index 34bc9f5e4dcc6605c77b5246a188803b8f153e4a..12d6c6b4139a3469b003f8d89cc400d960669a0a 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/merge/StrategyOneSided.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/merge/StrategyOneSided.java
@@ -106,5 +106,10 @@ public class StrategyOneSided extends MergeStrategy {
                public ObjectId getResultTreeId() {
                        return sourceTrees[treeIndex];
                }
+
+               @Override
+               public ObjectId getBaseCommitId() {
+                       return null;
+               }
        }
 }

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/merge/ThreeWayMerger.java b/org.eclipse.jgit/src/org/eclipse/jgit/merge/ThreeWayMerger.java
index 1ad791bb799f003c3f1beff7923b8fe2eab40a65..fbedaef86596e4e1642e6eef57e0f4e5deb59878 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/merge/ThreeWayMerger.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/merge/ThreeWayMerger.java
@@ -49,14 +49,19 @@ import java.io.IOException;
 import org.eclipse.jgit.errors.IncorrectObjectTypeException;
 import org.eclipse.jgit.errors.MissingObjectException;
 import org.eclipse.jgit.lib.AnyObjectId;
+import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevTree;
 import org.eclipse.jgit.treewalk.AbstractTreeIterator;
+import org.eclipse.jgit.treewalk.EmptyTreeIterator;
 
 /** A merge of 2 trees, using a common base ancestor tree. */
 public abstract class ThreeWayMerger extends Merger {
        private RevTree baseTree;
 
+       private ObjectId baseCommitId;
+
        /**
         * Create a new merge instance for a repository.
         *
@@ -109,6 +114,11 @@ public abstract class ThreeWayMerger extends Merger {
                return super.merge(tips);
        }
 
+       @Override
+       public ObjectId getBaseCommitId() {
+               return baseCommitId;
+       }
+
        /**
         * Create an iterator to walk the merge base.
         *
@@ -119,6 +129,15 @@ public abstract class ThreeWayMerger extends Merger {
        protected AbstractTreeIterator mergeBase() throws IOException {
                if (baseTree != null)
                        return openTree(baseTree);
-               return mergeBase(sourceCommits[0], sourceCommits[1]);
+               RevCommit baseCommit = (baseCommitId != null) ? walk
+                               .parseCommit(baseCommitId) : getBaseCommit(sourceCommits[0],
+                               sourceCommits[1]);
+               if (baseCommit == null) {
+                       baseCommitId = null;
+                       return new EmptyTreeIterator();
+               } else {
+                       baseCommitId = baseCommit.toObjectId();
+                       return openTree(baseCommit.getTree());
+               }
        }
 }