From 0c4553d28a34dfb64f4af68032a9a33ca297975e Mon Sep 17 00:00:00 2001 From: Christian Halstrick Date: Fri, 18 Jul 2014 11:04:33 +0200 Subject: [PATCH] Fix RecursiveMerger's internal use of merge to find a merge base When RecursiveMerger tried to determine a common base tree it was recursively tried to merge multiple common bases. But these intermediate merges which have just been done to determine a single common base for the final merge already filled some important fields (toBeCheckedOut, toBeDeleted, ...). These side effects of the intermediate merges led to wrong results of the final merge. One symptom was that after a recursive merge which should be succesful you could still see leftover files in the worktree: files which existed in the (virtual) common base but which don't exist anymore in the branches to be merged. The solution is easy: Clear the appropriate fields after common base determination and start the final merge with a clean state. Change-Id: I644ea9e1cb15360f7901bc0483cdb9286308c226 Signed-off-by: Robin Rosenberg --- .../org/eclipse/jgit/junit/JGitTestUtil.java | 5 + .../jgit/junit/RepositoryTestCase.java | 4 + .../jgit/merge/RecursiveMergerTest.java | 180 +++++++++++++++++- .../eclipse/jgit/merge/RecursiveMerger.java | 6 + 4 files changed, 194 insertions(+), 1 deletion(-) diff --git a/org.eclipse.jgit.junit/src/org/eclipse/jgit/junit/JGitTestUtil.java b/org.eclipse.jgit.junit/src/org/eclipse/jgit/junit/JGitTestUtil.java index 1079d9843..136c64726 100644 --- a/org.eclipse.jgit.junit/src/org/eclipse/jgit/junit/JGitTestUtil.java +++ b/org.eclipse.jgit.junit/src/org/eclipse/jgit/junit/JGitTestUtil.java @@ -229,6 +229,11 @@ public static String read(final Repository db, final String name) return read(file); } + public static boolean check(final Repository db, final String name) { + File file = new File(db.getWorkTree(), name); + return file.exists(); + } + public static void deleteTrashFile(final Repository db, final String name) throws IOException { File path = new File(db.getWorkTree(), name); diff --git a/org.eclipse.jgit.junit/src/org/eclipse/jgit/junit/RepositoryTestCase.java b/org.eclipse.jgit.junit/src/org/eclipse/jgit/junit/RepositoryTestCase.java index c1e0a2dcf..bc225cc01 100644 --- a/org.eclipse.jgit.junit/src/org/eclipse/jgit/junit/RepositoryTestCase.java +++ b/org.eclipse.jgit.junit/src/org/eclipse/jgit/junit/RepositoryTestCase.java @@ -119,6 +119,10 @@ protected String read(final String name) throws IOException { return JGitTestUtil.read(db, name); } + protected boolean check(final String name) { + return JGitTestUtil.check(db, name); + } + protected void deleteTrashFile(final String name) throws IOException { JGitTestUtil.deleteTrashFile(db, name); } diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/RecursiveMergerTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/RecursiveMergerTest.java index 8f43f7f86..4e39daaf4 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/RecursiveMergerTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/RecursiveMergerTest.java @@ -44,6 +44,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; import java.io.BufferedReader; import java.io.File; @@ -178,7 +179,7 @@ public void crissCrossMerge(MergeStrategy strategy, IndexState indexState, /** * Merging m2,s2 from the following topology. The same file is modified * in both branches. The modifications should be mergeable. m2 and s2 - * contain branch specific conflict resolutions. Therefore m2 and don't contain the same content. + * contain branch specific conflict resolutions. Therefore m2 and s2 don't contain the same content. * * 
 	 * m0--m1--m2
@@ -257,6 +258,183 @@ else if (worktreeState == WorktreeState.DifferentFromHeadAndOther
 		}
 	}
 
+	@Theory
+	/**
+	 * Merging m2,s2 from the following topology. The same file is modified
+	 * in both branches. The modifications should be mergeable but only if the automerge of m1 and s1
+	 * is choosen as parent. Choosing m0 as parent would not be sufficient (in contrast to the merge in
+	 * crissCrossMerge_mergeable). m2 and s2 contain branch specific conflict resolutions. Therefore m2
+	 * and s2 don't contain the same content.
+	 *
+	 * 
+	 * m0--m1--m2
+	 *   \   \/
+	 *    \  /\
+	 *     s1--s2
+	 * 

+	 */
+	public void crissCrossMerge_mergeable2(MergeStrategy strategy,
+			IndexState indexState, WorktreeState worktreeState)
+			throws Exception {
+		if (!validateStates(indexState, worktreeState))
+			return;
+
+		BranchBuilder master = db_t.branch("master");
+		RevCommit m0 = master.commit().add("f", "1\n2\n3\n")
+				.message("m0")
+				.create();
+		RevCommit m1 = master.commit().add("f", "1-master\n2\n3\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-side\n")
+				.message("s1").create();
+		RevCommit s2 = side.commit().parent(m1)
+				.add("f", "1-master\n2\n3-side-r\n")
+				.message("s2(merge)")
+				.create();
+		RevCommit m2 = master.commit().parent(s1)
+				.add("f", "1-master-r\n2\n3-side\n")
+				.message("m2(merge)")
+				.create();
+
+		Git git = Git.wrap(db);
+		git.checkout().setName("master").call();
+		modifyWorktree(worktreeState, "f", "side");
+		modifyIndex(indexState, "f", "side");
+
+		ResolveMerger merger = (ResolveMerger) strategy.newMerger(db,
+				worktreeState == WorktreeState.Bare);
+		if (worktreeState != WorktreeState.Bare)
+			merger.setWorkingTreeIterator(new FileTreeIterator(db));
+		try {
+			boolean expectSuccess = true;
+			if (!(indexState == IndexState.Bare
+					|| indexState == IndexState.Missing || indexState == IndexState.SameAsHead))
+				// index is dirty
+				expectSuccess = false;
+			else if (worktreeState == WorktreeState.DifferentFromHeadAndOther
+					|| worktreeState == WorktreeState.SameAsOther)
+				expectSuccess = false;
+			assertEquals(Boolean.valueOf(expectSuccess),
+					Boolean.valueOf(merger.merge(new RevCommit[] { m2, s2 })));
+			assertEquals(MergeStrategy.RECURSIVE, strategy);
+			if (!expectSuccess)
+				// if the merge was not successful skip testing the state of
+				// index and workingtree
+				return;
+			assertEquals(
+					"1-master-r\n2\n3-side-r",
+					contentAsString(db, merger.getResultTreeId(), "f"));
+			if (indexState != IndexState.Bare)
+				assertEquals(
+						"[f, mode:100644, content:1-master-r\n2\n3-side-r\n]",
+						indexState(RepositoryTestCase.CONTENT));
+			if (worktreeState != WorktreeState.Bare
+					&& worktreeState != WorktreeState.Missing)
+				assertEquals(
+						"1-master-r\n2\n3-side-r\n",
+						read("f"));
+		} catch (NoMergeBaseException e) {
+			assertEquals(MergeStrategy.RESOLVE, strategy);
+			assertEquals(e.getReason(),
+					MergeBaseFailureReason.MULTIPLE_MERGE_BASES_NOT_SUPPORTED);
+		}
+	}
+
+	@Theory
+	/**
+	 * Merging m2,s2 from the following topology. The same file is modified
+	 * in both branches. The modifications should be mergeable but only if the automerge of m1 and s1
+	 * is choosen as parent. On both branches delete and modify files untouched on the other branch.
+	 * On both branches create new files. Make sure these files are correctly merged and
+	 * exist in the workingtree.
+	 *
+	 * 
+	 * m0--m1--m2
+	 *   \   \/
+	 *    \  /\
+	 *     s1--s2
+	 * 

+	 */
+	public void crissCrossMerge_checkOtherFiles(MergeStrategy strategy,
+			IndexState indexState, WorktreeState worktreeState)
+			throws Exception {
+		if (!validateStates(indexState, worktreeState))
+			return;
+
+		BranchBuilder master = db_t.branch("master");
+		RevCommit m0 = master.commit().add("f", "1\n2\n3\n").add("m.m", "0")
+				.add("m.d", "0").add("s.m", "0").add("s.d", "0").message("m0")
+				.create();
+		RevCommit m1 = master.commit().add("f", "1-master\n2\n3\n")
+				.add("m.c", "0").add("m.m", "1").rm("m.d").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-side\n")
+				.add("s.c", "0").add("s.m", "1").rm("s.d").message("s1")
+				.create();
+		RevCommit s2 = side.commit().parent(m1)
+				.add("f", "1-master\n2\n3-side-r\n").add("m.m", "1")
+				.add("m.c", "0").rm("m.d").message("s2(merge)").create();
+		RevCommit m2 = master.commit().parent(s1)
+				.add("f", "1-master-r\n2\n3-side\n").add("s.m", "1")
+				.add("s.c", "0").rm("s.d").message("m2(merge)").create();
+
+		Git git = Git.wrap(db);
+		git.checkout().setName("master").call();
+		modifyWorktree(worktreeState, "f", "side");
+		modifyIndex(indexState, "f", "side");
+
+		ResolveMerger merger = (ResolveMerger) strategy.newMerger(db,
+				worktreeState == WorktreeState.Bare);
+		if (worktreeState != WorktreeState.Bare)
+			merger.setWorkingTreeIterator(new FileTreeIterator(db));
+		try {
+			boolean expectSuccess = true;
+			if (!(indexState == IndexState.Bare
+					|| indexState == IndexState.Missing || indexState == IndexState.SameAsHead))
+				// index is dirty
+				expectSuccess = false;
+			else if (worktreeState == WorktreeState.DifferentFromHeadAndOther
+					|| worktreeState == WorktreeState.SameAsOther)
+				expectSuccess = false;
+			assertEquals(Boolean.valueOf(expectSuccess),
+					Boolean.valueOf(merger.merge(new RevCommit[] { m2, s2 })));
+			assertEquals(MergeStrategy.RECURSIVE, strategy);
+			if (!expectSuccess)
+				// if the merge was not successful skip testing the state of
+				// index and workingtree
+				return;
+			assertEquals(
+					"1-master-r\n2\n3-side-r",
+					contentAsString(db, merger.getResultTreeId(), "f"));
+			if (indexState != IndexState.Bare)
+				assertEquals(
+						"[f, mode:100644, content:1-master-r\n2\n3-side-r\n][m.c, mode:100644, content:0][m.m, mode:100644, content:1][s.c, mode:100644, content:0][s.m, mode:100644, content:1]",
+						indexState(RepositoryTestCase.CONTENT));
+			if (worktreeState != WorktreeState.Bare
+					&& worktreeState != WorktreeState.Missing) {
+				assertEquals(
+						"1-master-r\n2\n3-side-r\n",
+						read("f"));
+				assertTrue(check("s.c"));
+				assertFalse(check("s.d"));
+				assertTrue(check("s.m"));
+				assertTrue(check("m.c"));
+				assertFalse(check("m.d"));
+				assertTrue(check("m.m"));
+			}
+		} catch (NoMergeBaseException e) {
+			assertEquals(MergeStrategy.RESOLVE, strategy);
+			assertEquals(e.getReason(),
+					MergeBaseFailureReason.MULTIPLE_MERGE_BASES_NOT_SUPPORTED);
+		}
+	}
+
 	@Theory
 	/**
 	 * Merging m2,s2 from the following topology. The same file is modified
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/merge/RecursiveMerger.java b/org.eclipse.jgit/src/org/eclipse/jgit/merge/RecursiveMerger.java
index 5802850a3..cce3de9b2 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/merge/RecursiveMerger.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/merge/RecursiveMerger.java
@@ -212,6 +212,12 @@ protected RevCommit getBaseCommit(RevCommit a, RevCommit b, int callDepth)
 			inCore = oldIncore;
 			dircache = oldDircache;
 			workingTreeIterator = oldWTreeIt;
+			toBeCheckedOut.clear();
+			toBeDeleted.clear();
+			modifiedFiles.clear();
+			unmergedPaths.clear();
+			mergeResults.clear();
+			failingPaths.clear();
 		}
 		return currentBase;
 	}